Skip to content

Optionally allow to disable cert-manager's http01-edit-in-place annotation

Konst Tchernov requested to merge update-cert-manager-version into master

What does this MR do?

Adds a new global configuration global.ingress.useNewIngressForCerts. Which defaults to false for full backwards compatibility.

Once enabled, the functional change is that a new temporary Ingress will now be created for each ACME certificate validation request.

There are two changes to achieve this:

  1. On the cert-manager Issuer, the Ingress property key for the http-01 solver is changed from ingress.class.name to ingress.ingressClassName.
  2. The annotation "acme.cert-manager.io/http01-edit-in-place": "true" is removed from the Issuer.

Explanation

See cert-manager's docs on ingressClassName. This property unsupported until the recent upgrade (!3446 (merged)) of cert-manager to 1.12+ (cert-manager release notes)

This removes the need for the "acme.cert-manager.io/http01-edit-in-place": "true" Ingress annotation. The workaround was previously needed with the old cert-manager version because without the ingress.ingressClassName property, a new Ingress could not be successfully added for most current Ingress-controllers (e.g. nginx-ingress has required this new property from 2020). So we would set this edit-in-place annotation to true in order for us to re-use an existing Ingress.

The previous logic of re-using the original Ingress through edit-in-place has the problem that the original Ingress might have annotations that make it unreachable from Let's Encrypt. One example of this is on the GitLab dedicated project we often have an IP address restriction on the existing Ingresses (nginx.ingress.kubernetes.io/whitelist-source-range), which will not allow Let's Encrypt connections.

Related issues

GitLab Dedicated internal issue: gitlab-com/gl-infra/gitlab-dedicated/team#3045

Test Plan

Fresh install

  1. Set the global.ingress.useNewIngressForCerts value to true
  2. Checkout the branch and run helm dep up (this pulls the updated cert-manager chart)
  3. Deploy the branch to an cluster with a public IP (to allow ACME challenges to succeed)
  4. Verify cert-manager was installed and TLS/SSL works as expected

Upgrade path

  1. Deploy a chart with previous version of the chart (e.g. tag 7.5.1 to jump before the recent and related cert-manager version upgrade)
  2. Verify cert-manager was installed and TLS/SSL works as expected
  3. Upgrade the release to this branch
  4. Verify cert-manager was installed and TLS/SSL works as expected
  5. Verify certificate renew works as expected (e.g. by using cmctl (cmctl renew --namespace <namespace> <release>-gitlab-tls) or by setting a renewBefore on the certificate resource)

Author checklist

See Definition of done.

For anything in this list which will not be completed, please provide a reason in the MR discussion.

Required

  • Merge Request Title and Description are up to date, accurate, and descriptive
  • MR targeting the appropriate branch
  • MR has a green pipeline on GitLab.com
  • When ready for review, follow the instructions in the "Reviewer Roulette" section of the Danger Bot MR comment, as per the Distribution experimental MR workflow

Expected (please provide an explanation if not completing)

  • Test plan indicating conditions for success has been posted and passes
  • Documentation created/updated
  • Tests added/updated
  • Integration tests added to GitLab QA
  • Equivalent MR/issue for omnibus-gitlab opened
  • Equivalent MR/issue for Gitlab Operator project opened (see Operator documentation on impact of Charts changes)
  • Validate potential values for new configuration settings. Formats such as integer 10, duration 10s, URI scheme://user:passwd@host:port may require quotation or other special handling when rendered in a template and written to a configuration file.
Edited by Konst Tchernov

Merge request reports