leader election namespace should default to .Release.Namespace, not kube-system

[sdickhoven]

Describe the bug:

The helm chart defaults the leader election namespace to kube-system.

https://github.com/cert-manager/cert-manager/blob/v1.14.1/deploy/charts/cert-manager/values.yaml#L50

however, when deploying cert-manager to a different namespace (which should be an uncontroversial deployment strategy), this default results in the following error:

E0204 17:44:38.929540       1 leaderelection.go:332] error retrieving resource lock kube-system/cert-manager-controller: leases.coordination.k8s.io "cert-manager-controller" is forbidden: User "system:serviceaccount:cert-manager:cert-manager" cannot get resource "leases" in API group "coordination.k8s.io" in the namespace "kube-system"

E0204 17:40:15.517186       1 leaderelection.go:332] error retrieving resource lock kube-system/cert-manager-cainjector-leader-election: leases.coordination.k8s.io "cert-manager-cainjector-leader-election" is forbidden: User "system:serviceaccount:cert-manager:cert-manager-cainjector" cannot get resource "leases" in API group "coordination.k8s.io" in the namespace "kube-system"

this bug can easily be circumvented by adding the helm config

global:
  leaderElection:
    namespace: cert-manager

but that should not be necessary.

Expected behaviour:

the rbac config specifically only allows cert-manager to manipulate leases in the same namespace that it is deployed in.

https://github.com/cert-manager/cert-manager/blob/v1.14.1/deploy/charts/cert-manager/templates/rbac.yaml#L3
https://github.com/cert-manager/cert-manager/blob/v1.14.1/deploy/charts/cert-manager/templates/rbac.yaml#L14-L17
https://github.com/cert-manager/cert-manager/blob/v1.14.1/deploy/charts/cert-manager/templates/cainjector-rbac.yaml#L55
https://github.com/cert-manager/cert-manager/blob/v1.14.1/deploy/charts/cert-manager/templates/cainjector-rbac.yaml#L71-L74

so the leader election leases should default to that same namespace.

i.e. i would expect the default value for the leader election namespace to be omitted in values.yaml and for the helm chart to implement logic along the lines of:

- --leader-election-namespace={{ .Values.global.leaderElection.namespace | default .Release.Namespace }}

ref:

https://github.com/cert-manager/cert-manager/blob/v1.14.1/deploy/charts/cert-manager/templates/deployment.yaml#L95
https://github.com/cert-manager/cert-manager/blob/v1.14.1/deploy/charts/cert-manager/templates/cainjector-deployment.yaml#L68

also, why have leader election at all? cert-manager is usually deployed with a single replica:

https://github.com/cert-manager/cert-manager/blob/v1.14.1/deploy/charts/cert-manager/values.yaml#L85

but i understand that adding extra logic to the helm chart to enable leader election only when more than one replica is used is complexity for little gain. 🤷

Steps to reproduce the bug:

deploy cert-manager in a namespace other than kube-system using the helm chart.

Anything else we need to know?:

no.

Environment details::

• Kubernetes version: 1.25
• Cloud-provider/provisioner: n/a
• cert-manager version: 1.13.3
• Install method: helm

/kind bug

[inteon]

@sdickhoven I'm not able to reproduce your issue. I can successfully install cert-manager in the cert-manager namespace and let it use kube-system for its leader election.
The RBAC and the --leader-election-namespace flag (https://github.com/cert-manager/cert-manager/blob/v1.14.1/deploy/charts/cert-manager/templates/deployment.yaml#L94-L96) indeed result in the controller creating its lease resources in the kube-system namespace.
This was done originally to prevent an unexpecting user from installing two cert-manager instances in the same cluster (eg. in two different namespaces).

I hope this makes the original reasoning behind this behavior a bit clearer.
Could you, given this information, give some more information about the issue you are experiencing? Right now, I'm not able to reproduce (I can successfully run cert-manager in a custom namespace, without changing the leader election namespace).

[sdickhoven]

hi @inteon 👋

ah, yes. i recently ran into the same issue and it has to do with how i am using this helm chart. 🤦

specifically i'm using helm as a templating engine only to render out the kube manifests with helm template ... and then i kustomize over the result to fix/add anything i can't configure with the values.yaml.

and my kustomization.yaml looks like this:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
namespace: cert-manager    # <<<<<<<<<<<<<<<<<<<<<<<
resources:
- cluster-ca.yaml
- letsencrypt-issuer-prod.yaml
- letsencrypt-issuer-staging.yaml
- manifest.yaml

so the Role and RoleBinding that your helm chart would have put into the kube-system namespace ended up in the cert-manager namespace

https://github.com/cert-manager/cert-manager/blob/v1.14.2/deploy/charts/cert-manager/templates/rbac.yaml#L2-L45
https://github.com/cert-manager/cert-manager/blob/v1.14.2/deploy/charts/cert-manager/values.yaml#L50

...while the command-line arg still pointed to the kube-system namespace

https://github.com/cert-manager/cert-manager/blob/v1.14.2/deploy/charts/cert-manager/templates/deployment.yaml#L96-L100

sorry. my bad!

though... may i ask what the benefit is of creating the leader lease in a different namespace?

it seems somewhat nonsensical to me to not put all the (namespaced) cert-manager resources into the same namespace... in which case what i'm doing above would have worked fine. 🤷

i.e. why not default the leader election namespace to cert-manager.namespace (iff .Values.global.leaderElection.namespace is blank)?

https://github.com/cert-manager/cert-manager/blob/v1.14.2/deploy/charts/cert-manager/templates/_helpers.tpl#L172-L174

anyway, thanks for looking into this and sorry again for leading you on a wild goose chase. 😊

[inteon]

though... may i ask what the benefit is of creating the leader lease in a different namespace?

I think we discussed this before in one of our meetings and concluded that it might be a good idea to change the behavior.
It would also make it easier to uninstall cert-manager and to install cert-manager in clusters where you do not have access to the kube-system namespace.
We do have to make sure that we do not break any of our existing users however.
This last point is something we have to be really careful about. This is the most important thing we will check when we review a PR to change the current behavior.

[AurimasNav]

Just encountered the exact scenario as described by @sdickhoven, using kustomize on top of helm, with "kustomized" namespace.

Cloud-provider/provisioner: AKS
cert-manager version: 1.14.4
Install method: helm

[PaigingAlyssa]

+1