Add cli flag for leader-election-resource-lock

[shafeeqes]

What this PR does / why we need it:

Support for configmaps lock for leaderelection is removed in K8s 1.24 with kubernetes/kubernetes#106852.
More details about the motivation to switch to leases - ref kubernetes/kubernetes#80289.

ingress-nginx is currently using configmaps . This PR proposes to add a cli flag for leader-election-resource-lock which is defaulted to configmaps. If the operator has updated the necessary RBAC permissions, they should first use configmapsleases for migration purposes and then switch to leases in the next release.

Release Notes:

 A CLI flag `--leader-election-resource-lock` is introduced which defaults to the `configmaps`. If you want to migrate to `configmapsleases` and then `leases` in the future, necessary RBAC should be added in your component. You should atleast be running a release with `configmapsleases` as the resourcelock before setting the flag to `leases`.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• [x ] New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

Which issue/s this PR fixes

fixes #8091

How Has This Been Tested?

Tested by creating an image and updating on an existing deployment.
Resourcelocks were created as per set in the container spec of the ingress-nginx-controller deployment manifest.

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added tests to cover my changes.
• All new and existing tests passed.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Shafeeque E S (6a0edbc, 773fc59)

[k8s-ci-robot]

@shafeeqes: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

Welcome @shafeeqes!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @shafeeqes. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ChiefAlexander]

/ok-to-test

[shafeeqes]

/test pull-ingress-nginx-codegen

[ChiefAlexander]

/assign @rikatz

[rikatz]

Would like to ask for a soft transition here, let me explain:

• Test if we CAN use Leases, then we use it.
• If not, test if we can use ConfigMapLeases, then we use it
• Finally, if everything goes wrong, fallback to ConfigMapLock

This way, users that already rely on ConfigMap wont need to change/break their environment. Forcefully changing to Leases may break environments where the admins just update the image, and not the Roles, etc.

Makes sense?

[shafeeqes]

I did not get completely what you are suggesting. First, leaderelection should try to acquire resourcelock using leases, and if it fails, try configmapleases, and then configmaps?
Please explain.

[shafeeqes]

TBH, admins not updating the Roles does not feel like a reason enough to me. We will be mentioning that in the release notes, so admins should be noticing that and acting accordingly.
And anyways if we delay this too, after we revendor K8s, we have to get rid of the old resourcelocks.
WDYT?

[rikatz]

@shafeeqes this is true in theory, but not in practice and we must be empathetic with the admins on this :)

As a previous admin, I know that sometimes in a rush we simply update the image. This is a breaking change from the users perspective. If someone just update the image on a cluster previously created with old manifests (instead of reapplying all the manifests, and well, kubectl set image is here for that!) Ingress wont start, and this may lead into their ingresses being unavailable.

So, my suggestion is:

• Test if ingress can do a get/update/list/watch on leases. If so, let's use it
• Otherwise, fallback to the previously working scenario.

[rfranzke]

What about exposing a CLI flag similar to --election-id for configuring the lock type? This would prevent this fake call to check whether the nginx-ingress pod has required privilege and it would also decouple the leader election lock migration from nginx-ingress releases. Users of nginx-ingress can configure this as they want when they are ready (i.e., when they have provided the required RBAC privileges).

[rikatz]

/retest
I've fixed the test. If it fails again, please rebase over main :)

Thanks!

[shafeeqes]

@rikatz I have pushed the code changes as discussed. PTAL.
Will open another PR for the helm chart changes and the tests.

[rikatz]

/lgtm
Let's see how this goes
Thanks!

[shafeeqes]

/test pull-ingress-nginx-test

[k8s-ci-robot]

@shafeeqes: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-ingress-nginx-test	2fdaefe	link	true	/test pull-ingress-nginx-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[shafeeqes]

/tide merge-method-squash

[rikatz]

/lgtm
/approve
/label tide/merge-method-squash
Thanks

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz, shafeeqes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [rikatz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[shafeeqes]

@rikatz is there some issue in prow? or should I rebase?

[longwuyuan]

@shafeeqes , can you please help and clarify. Have you decided not to write any tests for this.
@rikatz , if @shafeeqes can not add tests for some reason, we should hold this temporarily until somebody else adds a test for this change.

It will not be fair to make this change and have it run on a real world user's cluster.

[rikatz]

I will write the tests. Just need to add to my week todo.

We need this in the next release as k8s v1.25 will remove the old lease way

[bmv126]

I thought it is getting removed in 1.24
https://github.com/kubernetes/kubernetes/blob/4ce5a8954017644c5420bae81d72b09b735c21f0/staging/src/k8s.io/client-go/tools/leaderelection/resourcelock/interface.go#L195

[bmv126]

@rikatz @shafeeqes
Is this needed from 1.25 and not from 1.24?

[shafeeqes]

@rikatz @shafeeqes Is this needed from 1.25 and not from 1.24?

Support for configmaps lock for leaderelection is removed in K8s 1.24 with kubernetes/kubernetes#106852.

[tao12345666333]

Sorry, I missed this PR.

I created a new PR #8733 for the migration.
I have cherry-picked your second commit into my PR, thanks

[shafeeqes]

/close as #8733 is merged

[k8s-ci-robot]

@shafeeqes: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.