Missing retry implementation at cainjector

[jkroepke]

Describe the bug:

I have a Helm Chart which bundles cert-manager and ingress-nginx. The ingress-nginx has an option to use the cert-manager for managing the certifications between ValidationingWebhook and and Webhook endpoint.

https://github.com/kubernetes/ingress-nginx/blob/main/charts/ingress-nginx/templates/admission-webhooks/cert-manager.yaml
https://github.com/kubernetes/ingress-nginx/blob/06c64bf5672ed7167f0c030b750ec9062bc86c83/charts/ingress-nginx/templates/admission-webhooks/validating-webhook.yaml#L10C5-L10C35

On installation, it cainjector wants too early inject the ca. cainjector does not observe the status of the certificate itself. cainject trying to find the certificate and fails, because of the secret is not present.

However, after a minute the secret is present because cert-manager process the certificate and the certificate gets ready. But cainjector never retries to re-inject the ca secret.

A manual restart of cainjector is required to solve this issue. After restart, cainject does not have any issues to inject the secret.

Expected behaviour:
cainjector should observe the certificate status and should inject the secret once its ready/completed.

Steps to reproduce the bug:

Please let me know, if I should provide a reproduce way. It may takes some time.

Anything else we need to know?:

Logs

2023-09-12T17:10:03+02:00	{"ts":1694531403998.7822,"caller":"cainjector/reconciler.go:142","msg":"cert-manager: Updated object","v":2,"kind":"validatingwebhookconfiguration","kind":"validatingwebhookconfiguration","name":"opsstack-ingress-nginx-admission"}
2023-09-12T14:17:38+02:00	{"ts":1694521058594.3582,"caller":"cainjector/reconciler.go:142","msg":"cert-manager: Updated object","v":2,"kind":"validatingwebhookconfiguration","kind":"validatingwebhookconfiguration","name":"opsstack-ingress-nginx-admission"}
2023-09-12T14:17:38+02:00	{"ts":1694521058565.5972,"caller":"cainjector/reconciler.go:142","msg":"cert-manager: Updated object","v":2,"kind":"validatingwebhookconfiguration","kind":"validatingwebhookconfiguration","name":"opsstack-ingress-nginx-admission"}
2023-09-11T18:07:10+02:00	{"ts":1694448430391.8206,"caller":"cainjector/reconciler.go:118","msg":"cert-manager: could not find any ca data in data source for target","v":2,"kind":"validatingwebhookconfiguration","kind":"validatingwebhookconfiguration","name":"opsstack-ingress-nginx-admission"}
2023-09-11T18:07:10+02:00	{"ts":1694448430391.778,"caller":"cainjector/sources.go:116","msg":"cert-manager: unable to fetch associated secret","kind":"validatingwebhookconfiguration","kind":"validatingwebhookconfiguration","name":"opsstack-ingress-nginx-admission","certificate":{"name":"opsstack-ingress-nginx-admission","namespace":"opsstack"},"secret":{"name":"opsstack-ingress-nginx-admission","namespace":"opsstack"},"err":"Secret \"opsstack-ingress-nginx-admission\" not found"}

Environment details::

• Kubernetes version: 1.27
• Cloud-provider/provisioner: AKS / minikube
• cert-manager version: v1.12.4
• Install method: helm

/kind bug

[irbekrm]

cainjector never retries to re-inject the ca secret

cainjector watches Secret resources, so an update to the Secret should be what triggers a reconcile
Do you pass any flags to cainjector?

[jkroepke]

Here are the flags:

--v=2 --leader-election-namespace=opsstack --logging-format=json --namespace=$(POD_NAMESPACE) --enable-customresourcedefinitions-injectable=false --enable-certificates-data-source=false --enable-apiservices-injectable=false

I will retest the issue on the new release

[jkroepke]

Here is a reproducible way (sometimes, it need more try. In that case please manually delete the generated secrets):

Chart.yaml

apiVersion: v2
name: issue6340
version: 0.0.0
dependencies:
  - name: ingress-nginx
    version: 4.8.0
    repository: https://kubernetes.github.io/ingress-nginx
  - name: cert-manager
    version: v1.13.0
    repository: https://charts.jetstack.io

values.yaml

global:
  leaderElection:
    namespace: "default"

ingress-nginx:
  controller:
    admissionWebhooks:
      patch:
        enabled: false
      certManager:
        enabled: true
cert-manager:
  enableCertificateOwnerRef: true
  cainjector:
    extraArgs:
      - --namespace=$(POD_NAMESPACE)
      - --enable-customresourcedefinitions-injectable=false
      - --enable-certificates-data-source=false
      - --enable-apiservices-injectable=false

kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v1.13.0/cert-manager.crds.yaml
helm upgrade -i issue6340 .

[irbekrm]

• --enable-certificates-data-source=false

That's why it doesnt' work - the ingress webhooks actually use the Certificates data source and you have it disabled.

I think it would be better if the ingress webhooks used the Secrets data source (should be just a matter of changing the annotation name and pointing at the Secret)

I think we should also update our docs to put the Secret data source at top and recommend that folks use that for their webhooks/CRDs etc. The Certificate one is somewhat redundant as the same can be achieved with the Secret one and users might want to disable it to reduce cache size CPU cycles

[jkroepke]

Thanks for figure it out. I was a bit confused, because at certain point the Webhook gets the certificate injected.

I will upgrade the upstream helm charts to use the Secrets data source.

Is there any breaking change by changing the annotation from cert-manager.io/inject-ca-from to cert-manager.io/inject-ca-from-secret

[jkroepke]

(should be just a matter of changing the annotation name and pointing at the Secret)

The Secret which are generated by the Certificates needs the cert-manager.io/allow-direct-injection annotation. Normally, this can be done by certificate.spec.secretTemplate.annotations, but this is not allowed (Invalid value: "cert-manager.io/allow-direct-injection": cert-manager.io/* annotations are not allowed).

If you recommend this data source by top, the secret data source should also allowed the annotation cert-manager.io/certificate-name=*, too.

[irbekrm]

If you recommend this data source by top, the secret data source should also allowed the annotation cert-manager.io/certificate-name=*, too.

Thanks for catching this!
Yes, I think we should allow that particular annotation

[irbekrm]

Thanks for figure it out. I was a bit confused, because at certain point the Webhook gets the certificate injected.

Yes, it would have sometimes worked, but it would not react on Secret events because the event handler that checks whether that Secret has an owning Certificate that is a data source for something that needs a CA injected wouldn't be started

[jkroepke]

Thanks! I guess the issue can be closed here, since all my question are answers. The migration to the secret data source will be delayed unless the other annotation in the secret injection whitelist.