nginxinc / kubernetes-ingress

NGINX and NGINX Plus Ingress Controllers for Kubernetes
https://docs.nginx.com/nginx-ingress-controller
Apache License 2.0
4.63k stars 1.96k forks source link

Ensure a unique `lease` object is created for each install of NIC via Helm #5388

Closed brianehlert closed 5 days ago

brianehlert commented 5 months ago

As an engineer, I would like to ensure that users can deploy multiple Ingress Controllers in the same name space without modifying the name of the leaderElectionLockName.

NOTE: This bug does not cover the lease object being managed by Helm

### UACs:
- [x] Update `_helper.tpl` to only generate the name of the lease
- [x] Update `charts/nginx-ingress/templates/controller-role.yaml` to use `nginx-ingress.leaderElectionName` instead of `.Values.controller.reportIngressStatus.leaderElectionLockName`

Changes required for fulfil requirements

  1. Update _helper.tpl to only generate the name:

Before:

{{- define "nginx-ingress.leaderElectionName" -}}
{{- if .Values.controller.reportIngressStatus.leaderElectionLockName -}}
{{ .Values.controller.reportIngressStatus.leaderElectionLockName }}
{{- else -}}
{{- printf "%s-%s" (include "nginx-ingress.fullname" .) "leader-election" -}}
{{- end -}}
{{- end -}}

After:

 {{- define "nginx-ingress.leaderElectionName" -}}
 {{- printf "%s-%s" (include "nginx-ingress.fullname" .) "leader-election" -}}
 {{- end -}}

Consideration:

 {{- define "nginx-ingress.leaderElectionName" -}}
 {{- printf "%s-%s" (include "nginx-ingress.fullname" .) .Values.controller.reportIngressStatus.leaderElectionLockName -}}
 {{- end -}}
  1. Update charts/nginx-ingress/templates/controller-role.yaml to use nginx-ingress.leaderElectionName instead of .Values.controller.reportIngressStatus.leaderElectionLockName

Before:

   resources:
   - leases
   resourceNames:
   - {{ .Values.controller.reportIngressStatus.leaderElectionLockName }}

After:

   resources:
   - leases
   resourceNames:
   - {{ include "nginx-ingress.leaderElectionName" . }}
  1. Remove .Values.controller.reportIngressStatus.leaderElectionLockName from values.yaml

Tested two deployments in the same namespace:

k get leases.coordination.k8s.io
NAME                                                   HOLDER                                                            AGE
leader-election-test-1-nginx-ingress-leader-election   leader-election-test-1-nginx-ingress-controller-75ff476494lbmnc   8m14s
leader-election-test-2-nginx-ingress-leader-election   leader-election-test-2-nginx-ingress-controller-7785c4fbc4qbckk   2s
k get deploy | grep leader-election
leader-election-test-1-nginx-ingress-controller   1/1     1            1           8m30s
leader-election-test-2-nginx-ingress-controller   1/1     1            1           17s
vepatel commented 5 months ago

guess this is for 3.7.0 as the change was introduced in 3.4.0? https://github.com/nginxinc/kubernetes-ingress/pull/4276/files#diff-43f7b0ca4debd314bccac5167f596b1394e23a307451e41d93ddb422c66c7d9c

Also lock name is still required but the name should be exclusively that of lease object not the configMap

danielnginx commented 1 month ago

References: https://github.com/nginxinc/kubernetes-ingress/issues/5389