Add ConfigMap name to values.yaml and allow the option to specify the leader election lock name

[paigr]

Proposed changes

Allow the customisation of the ConfigMap name within the helm template. The value is used for .metadata.name in the ConfigMap yaml, as well as with the -nginx-configmaps flag for the container.

Also allows specifying the name used for the lock with leader election enabled. This was implemented as a new CLI flag, -leader-election-lock-name, and an accompanying entry in the Helm template's values.yaml.

Resolves #528

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have checked that all unit tests pass after adding my changes
• I have updated necessary documentation
• I have rebased my branch onto master
• I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

[pleshakov]

@paigr thanks for the PR!

The part about the leader election looks good to me! Thanks for changing all the necessary docs. The only thing I would change is the default for the leader-election-lock-name
-- from leader-election to something more specific like nginx-ingress-leader-election. I understand that that default was already present. But I think it is a good opportunity to change it.

Regarding the part about the controller.config.nameparameter. I am not sure if it solves the problem completely. Here is how I see the problem:

$ kubectl create ns nginx-ingress-1
$ helm install --namespace nginx-ingress-1 --name my-r-1 .
. . .
NOTES:
The NGINX Ingress Controller has been installed.
$ kubectl create ns nginx-ingress-2
$ helm install --namespace nginx-ingress-2 --name my-r-2 .
Error: release my-r-2 failed: clusterroles.rbac.authorization.k8s.io "nginx-ingress" already exists

Because some of the resources in the helm chart are created with the same names, the second helm install command fails.
Is it the problem that you're trying to solve? Or you have a different problem?

[paigr]

Thanks for your comments @pleshakov.

For controller.config.name, the main use-case for this was to be able to deploy multiple ICs to the same namespace. We typically keep all of our ingress related resources within a single namespace, including two copies of the IC; one for internal traffic, and one for external. When using the Helm chart, there is no option of templating a different value for the ConfigMap name, so both ICs are required to share a config.

I agree with your point that the default value for the leader lock should be nginx-ingress-leader-election. However, have you thought about prefixing your resources with {{ .Values.controller.name }}? It would set sensible defaults, and also guarantee unique names for cluster-level objects, solving the issue of installing via Helm twice on a single cluster, as you mentioned above.

[pleshakov]

@paigr
Thanks for the updates. Just a small suggestion: could you change the defaults in the helm chart README and the values file for leaderElectionLockName?

If this solves your problem, we're are happy to merge. However, please address the suggestion above and squash the leaderElectionLockName-related commits into a single one.

However, have you thought about prefixing your resources with {{ .Values.controller.name }}

Yes, we're looking into that. We might use the release name as part of the name as well.
When we introduce that change, it will make sense to change the default for controller.config.name from nginx-config to a generated name as well as to use a generated name for controller.reportIngressStatus.leaderElectionLockName. Could this potentially break your installation?

[paigr]

@pleshakov during my tests I identified another potential bug. The template helper that defines nginx-ingress.name was using the name of the chart instead of the value for controller.name, thus controller.name is not actually used. Was this intentional?

I think generating the ConfigMap name will not break anything on our end. If we retain the option to set the name in the Helm template, as well as via CLI flag, there should be no problems.

[pleshakov]

@paigr

thus controller.name is not actually used. Was this intentional?

this was not intentional. good catch! However, I suggest leaving that fix out the scope of this PR. We're gonna change the logic of how the name is generated for the resources anyway -- as I mentioned we will use the release name as part of the name as well.

thanks

[paigr]

@pleshakov will this be done soon? Happy to remove it from this PR, but we do need that change as it gets in the way of deploying multiple ICs into the same namespace (which was the motivation for this PR).

[pleshakov]

@paigr yes, this should be done in the next two weeks (by April 26th or sooner). would you be ok to use a modified copy for the time being?

[paigr]

@paigr yes, this should be done in the next two weeks (by April 26th or sooner). would you be ok to use a modified copy for the time being?

Sure, no problem, I have removed the commit.

[pleshakov]

@paigr great! we'll merge shortly

[Dean-Coakley]

@paigr thanks a lot!