Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Helm chart: controller.reportIngressStatus.leaderElectionLockName is not auto-generated #5389

Open
kyrofa opened this issue Apr 13, 2024 · 5 comments · May be fixed by #5390
Open

Helm chart: controller.reportIngressStatus.leaderElectionLockName is not auto-generated #5389

kyrofa opened this issue Apr 13, 2024 · 5 comments · May be fixed by #5390
Labels
bug An issue reporting a potential bug documentation Pull requests/issues for documentation ready for refinement An issue that was triaged and it is ready to be refined

Comments

@kyrofa
Copy link

kyrofa commented Apr 13, 2024 •
edited

The helm chart README contains the following line:

|controller.reportIngressStatus.leaderElectionLockName | Specifies the name of the ConfigMap, within the same namespace as the controller, used as the lock for leader election. controller.reportIngressStatus.enableLeaderElection must be set to true. | Autogenerated |

And indeed, this needs to be auto-generated in order to support multiple ingress controllers in the same namespace. However, it's actually hard-coded in the values.yaml to nginx-ingress-leader, which means by default you cannot install multiple ingress controllers in the same namespace.

It does look like the nginx-ingress.leaderElectionName definition does actually support auto-generating it, but the fact that the value is hard-coded means it's not done by default.

I propose updating the the default leaderElectionLockName value to be an empty string so that the README is telling the truth.
@github-actions GitHub Actions
Copy link

Hi @kyrofa thanks for reporting!

Be sure to check out the docs and the Contributing Guidelines while you wait for a human to take a look at this 🙂

Cheers!

kyrofa added a commit to kyrofa/kubernetes-ingress that referenced this issue Apr 13, 2024
@kyrofa
values: auto-generate leaderElectionLockName by default
11151bb 
The README claims that it's auto-generated today, but it's actually
hard-coded. By changing it to use an empty string by default, the
`nginx-ingress.leaderElectionName` helper will take care of
auto-generating it.

Fix nginxinc#5389

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
@kyrofa kyrofa linked a pull request Apr 13, 2024 that will close this issue
Open
@brianehlert
Copy link
Collaborator

brianehlert commented Apr 13, 2024 •
edited

Leader election lock name was invalidated when this project moved to using the leases API.
This bit of code causes two leader election methods to be active at the same time and thus causes inappropriate load on the K8s API.
The change that started this shift was released with 3.4
#4276

@kyrofa
Copy link
Author

kyrofa commented Apr 13, 2024

I'm not completely following. Are you saying we can actually remove this configmap altogether? Because the way it stands today, if you try to install two ingress controllers alongside each other, you get something along the lines of:

Error: rendered manifests contain a resource that already exists. Unable to continue with install: ConfigMap "nginx-ingress-leader" in namespace "nginx-ingress" exists and cannot be imported into the current release

@vepatel
Copy link
Contributor

vepatel commented Apr 15, 2024 •
edited

Hi @kyrofa, thanks for opening the issue. autogenerating name makes sense!

We're keeping the both objects i.e. old configmap and new lease type to provide some overlap after it was introduced in 3.4.0.

Now just leaving leaderElectionLockName as "" won't fix it as lease object directly reads it and doesn't have a template like configmap , so probably have to create a template like:

{{- if .Values.controller.reportIngressStatus.enableLeaderElection }}
apiVersion: coordination.k8s.io/v1
kind: Lease
metadata:
  name: {{ include "nginx-ingress.leaderElectionName" . }}
  namespace: {{ .Release.Namespace }}
  labels:
    {{- include "nginx-ingress.labels" . | nindent 4 }}
{{- end }}

We will be talking about this issue in our next community call, please feel free to join us on 6th May 3pm GMT (Zoom details on home page)

kyrofa added a commit to kyrofa/kubernetes-ingress that referenced this issue Apr 15, 2024
@kyrofa
values: auto-generate leaderElectionLockName by default
dd8c35c 
The README claims that it's auto-generated today, but it's actually
hard-coded. By changing it to use an empty string by default, the
`nginx-ingress.leaderElectionName` helper will take care of
auto-generating it. Finally, make sure
`nginx-ingress.leaderElectionName` is used in the RBAC Role instead of
the hard-coded value.

Fix nginxinc#5389

Signed-off-by: Kyle Fazzari <kyrofa@ubuntu.com>
@danielnginx danielnginx added bug An issue reporting a potential bug documentation Pull requests/issues for documentation ready for refinement An issue that was triaged and it is ready to be refined labels May 7, 2024
@danielnginx
Copy link
Collaborator

We should implement #5388 before fixing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug An issue reporting a potential bug documentation Pull requests/issues for documentation ready for refinement An issue that was triaged and it is ready to be refined
Projects
NGINX Ingress Controller
Status: Todo ☑
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

values: auto-generate leaderElectionLockName by default kyrofa/kubernetes-ingress
4 participants
@kyrofa @brianehlert @vepatel @danielnginx