Fix nginx controller leader election failed on certain setup #10282

vsamtuc · 2023-07-10T16:33:20Z

Under the original code, leader election failed for ingress controllers as a result of mismatch between election-id in the controller config, and the resourceName in the relevant rule of role 'ingress-nginx'. This appeared in the contoller logs.

To fix the issue, a command-line option was added to container execution (--election-id=...).

Now, the electio-id agrees with the resourceName provided in the role-ingress-nginx.yml file. A comment in that file was changed to reflect the new logic.

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #10276

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix nginx controller leader election failed on certain setup

Under the original code, leader election failed for ingress controllers as a result of mismatch between election-id in the controller config, and the resourceName in the relevant rule of role 'ingress-nginx'. This appeared in the contoller logs. To fix the issue, a command-line option was added to container execution (--election-id=...). Now, the electio-id agrees with the resourceName provided in the role-ingress-nginx.yml file. A comment in that file was changed to reflect the new logic.

linux-foundation-easycla · 2023-07-10T16:33:23Z

The committers listed above are authorized under a signed CLA.

✅ login: vsamtuc / name: Vasilis Samoladas (530e77f)
✅ login: mzaian / name: Mohamed Omar Zaian (797fdbd)

k8s-ci-robot · 2023-07-10T16:33:28Z

Welcome @vsamtuc!

It looks like this is your first PR to kubernetes-sigs/kubespray 🎉. 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-sigs/kubespray 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 · 2023-07-10T16:33:29Z

Hi @vsamtuc. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

vsamtuc · 2023-07-10T16:39:30Z

Adding more detail to original comment:

Under the original code, leader election failed for ingress controllers as a result of mismatch between election-id in the controller config, and the resourceName in the relevant rule of role 'ingress-nginx'. This appeared in the contoller logs.

To fix the issue, a command-line option was added to container execution (--election-id=...).

Now, the election-id agrees with the resourceName provided in the role-ingress-nginx.yml file. A comment in that file was changed to reflect the new logic.

What type of PR is this?

/kind bug

What this PR does / why we need it:

Without it the ingress-nginx controller did not deploy correctly.

Which issue(s) this PR fixes:

Fixes # 10276

Special notes for your reviewer: Sorry for a confusing submission, this is my first PR in kubespray :-)

Does this PR introduce a user-facing change?: No

yankay · 2023-07-11T04:03:09Z

/ok-to-test

roles/kubernetes-apps/ingress_controller/ingress_nginx/templates/role-ingress-nginx.yml.j2

yankay · 2023-07-14T04:22:20Z

Thanks @vsamtuc for the PR

There are comments :-)
HI @mzaian , would you please help to review it :-)

yankay · 2023-07-14T04:25:47Z

The PR title and commit message can be changed to "Fix nginx controller leader election failed on certain setup"

mzaian · 2023-07-14T07:35:12Z

Hello @vsamtuc

Please look at the below comment. I've reviewed and looks fine to me. Thank you!

The PR title and commit message can be changed to "Fix nginx controller leader election failed on certain setup"

mzaian · 2023-08-14T08:22:49Z

The PR title and commit message can be changed to "Fix nginx controller leader election failed on certain setup"

@vsamtuc Would you please check this and maybe rebase so we can merge this.

tobiasehlert · 2023-09-19T19:19:47Z

Is there any update on this PR? :)

I updated my ClusterRole until merge.. adjusted it like this:
bitnami/charts#11192 (comment)

michaelkebe · 2023-10-27T14:38:25Z

Please merge this. 🙏

I used the latest version v2.23.0 and stumpled upon this. 💥

Patched manually the fix from this PR and fixed it. ✅

mzaian

/lgtm

roles/kubernetes-apps/ingress_controller/ingress_nginx/templates/role-ingress-nginx.yml.j2

k8s-ci-robot · 2023-10-27T15:08:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mzaian, vsamtuc

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~~ [mzaian]

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

…es/role-ingress-nginx.yml.j2

k8s-ci-robot · 2023-10-27T15:10:48Z

New changes are detected. LGTM label has been removed.

mzaian · 2023-10-27T15:27:15Z

Please merge this. 🙏

I used the latest version v2.23.0 and stumpled upon this. 💥

Patched manually the fix from this PR and fixed it. ✅

We will consider it in v2.23.1 see here: #10569

VannTen · 2024-01-22T10:23:11Z

I think you should rebase, the CI failures might because master has changed stuff.

VannTen · 2024-02-06T09:47:57Z

@vsamtuc ^

VannTen · 2024-02-06T10:30:42Z

@mzaian do you have the write perms on this PR ? I think if we rebase it on master it should work

mzaian · 2024-02-06T10:56:01Z

@mzaian do you have the write perms on this PR ? I think if we rebase it on master it should work

I cannot remember but I did a cherry-pick and referenced to this PR here https://github.com/kubernetes-sigs/kubespray/pull/10569/files We could do the same since that no response.

floryut · 2024-02-09T13:19:32Z

@mzaian do you have the write perms on this PR ? I think if we rebase it on master it should work

I cannot remember but I did a cherry-pick and referenced to this PR here https://github.com/kubernetes-sigs/kubespray/pull/10569/files We could do the same since that no response.

Or I can force merge it if need be

mzaian · 2024-02-09T13:51:03Z

@mzaian do you have the write perms on this PR ? I think if we rebase it on master it should work

I cannot remember but I did a cherry-pick and referenced to this PR here https://github.com/kubernetes-sigs/kubespray/pull/10569/files We could do the same since that no response.

Or I can force merge it if need be

We need to rebase to not break other changes. I would suggest a cherry-pick PR with the same stuff since that he is not available.

VannTen · 2024-02-09T14:11:51Z

Doesn't Github allow repo maintainers to push on PR branch ? So we could the rebase ourselves.

mzaian · 2024-02-09T14:33:54Z

Doesn't Github allow repo maintainers to push on PR branch ? So we could the rebase ourselves.

This has to be allowed by the PR owner while he is creating the PR. To allow maintainers to edit the PR.

VannTen · 2024-02-12T10:48:20Z

/close
Superseded by #10913 (which is a simple rebase of this one)

k8s-ci-robot · 2024-02-12T10:48:25Z

@VannTen: Closed this PR.

In response to this:

/close
Superseded by #10913 (which is a simple rebase of this one)

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 requested review from jayonlau and yankay July 10, 2023 16:33

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 10, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 10, 2023

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 10, 2023

vsamtuc marked this pull request as draft July 10, 2023 16:34

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2023

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 10, 2023

vsamtuc marked this pull request as ready for review July 10, 2023 16:39

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 10, 2023

k8s-ci-robot requested review from cristicalin and holmsten July 10, 2023 16:39

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 10, 2023

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 11, 2023

yankay reviewed Jul 14, 2023

View reviewed changes

roles/kubernetes-apps/ingress_controller/ingress_nginx/templates/role-ingress-nginx.yml.j2 Outdated Show resolved Hide resolved

vsamtuc changed the title ~~This commit fixes Issue # 10276.~~ Fix nginx controller leader election failed on certain setup Jul 14, 2023

mzaian approved these changes Oct 27, 2023

View reviewed changes

roles/kubernetes-apps/ingress_controller/ingress_nginx/templates/role-ingress-nginx.yml.j2 Outdated Show resolved Hide resolved

roles/kubernetes-apps/ingress_controller/ingress_nginx/templates/role-ingress-nginx.yml.j2 Outdated Show resolved Hide resolved

k8s-ci-robot assigned mzaian Oct 27, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 27, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2023

mzaian mentioned this pull request Oct 27, 2023

Release Proposal v2.23.1 #10555

Closed

Update roles/kubernetes-apps/ingress_controller/ingress_nginx/templat…

797fdbd

…es/role-ingress-nginx.yml.j2

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 27, 2023

mzaian mentioned this pull request Oct 27, 2023

[release-2.23] [ingress-nginx] Fix nginx controller leader election RBAC permissions #10569

Merged

VannTen mentioned this pull request Feb 6, 2024

Fix election problem of ingress-nginx daemonset #10888

Closed

VannTen mentioned this pull request Feb 12, 2024

Fix ingress-nginx controller election #10913

Merged

k8s-ci-robot closed this Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix nginx controller leader election failed on certain setup #10282

Fix nginx controller leader election failed on certain setup #10282

vsamtuc commented Jul 10, 2023 •

edited by floryut

Loading

linux-foundation-easycla bot commented Jul 10, 2023 •

edited

Loading

k8s-ci-robot commented Jul 10, 2023

k8s-ci-robot commented Jul 10, 2023

vsamtuc commented Jul 10, 2023

yankay commented Jul 11, 2023

yankay commented Jul 14, 2023

yankay commented Jul 14, 2023 •

edited

Loading

mzaian commented Jul 14, 2023

mzaian commented Aug 14, 2023

tobiasehlert commented Sep 19, 2023 •

edited

Loading

michaelkebe commented Oct 27, 2023

mzaian left a comment

k8s-ci-robot commented Oct 27, 2023

k8s-ci-robot commented Oct 27, 2023

mzaian commented Oct 27, 2023

VannTen commented Jan 22, 2024

VannTen commented Feb 6, 2024

VannTen commented Feb 6, 2024 •

edited

Loading

mzaian commented Feb 6, 2024 •

edited

Loading

floryut commented Feb 9, 2024

mzaian commented Feb 9, 2024

VannTen commented Feb 9, 2024

mzaian commented Feb 9, 2024

VannTen commented Feb 12, 2024

k8s-ci-robot commented Feb 12, 2024

Fix nginx controller leader election failed on certain setup #10282

Fix nginx controller leader election failed on certain setup #10282

Conversation

vsamtuc commented Jul 10, 2023 • edited by floryut Loading

linux-foundation-easycla bot commented Jul 10, 2023 • edited Loading

k8s-ci-robot commented Jul 10, 2023

k8s-ci-robot commented Jul 10, 2023

vsamtuc commented Jul 10, 2023

yankay commented Jul 11, 2023

yankay commented Jul 14, 2023

yankay commented Jul 14, 2023 • edited Loading

mzaian commented Jul 14, 2023

mzaian commented Aug 14, 2023

tobiasehlert commented Sep 19, 2023 • edited Loading

michaelkebe commented Oct 27, 2023

mzaian left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 27, 2023

k8s-ci-robot commented Oct 27, 2023

mzaian commented Oct 27, 2023

VannTen commented Jan 22, 2024

VannTen commented Feb 6, 2024

VannTen commented Feb 6, 2024 • edited Loading

mzaian commented Feb 6, 2024 • edited Loading

floryut commented Feb 9, 2024

mzaian commented Feb 9, 2024

VannTen commented Feb 9, 2024

mzaian commented Feb 9, 2024

VannTen commented Feb 12, 2024

k8s-ci-robot commented Feb 12, 2024

vsamtuc commented Jul 10, 2023 •

edited by floryut

Loading

linux-foundation-easycla bot commented Jul 10, 2023 •

edited

Loading

yankay commented Jul 14, 2023 •

edited

Loading

tobiasehlert commented Sep 19, 2023 •

edited

Loading

VannTen commented Feb 6, 2024 •

edited

Loading

mzaian commented Feb 6, 2024 •

edited

Loading