fix(cert-manager): bump chart to v1.20.2#2562
Conversation
cert-manager v1.20.0 introduced the acme.cert-manager.io/http01-ingress-ingressclassname Ingress annotation (cert-manager#8244). Cozystack templates already emit it (since 2b6e20c) but v1.19.x silently ignored it, so cert-manager fell back to the ClusterIssuer's solver class. HTTP-01 solver Ingresses for non-publishing tenants ended up on the wrong IngressClass, breaking Let's Encrypt issuance for any tenant ingress not on tenant-root. Targeting v1.20.2 picks up cert-manager#8655 (clusterissuers/issuers finalizer RBAC restored after v1.20.0 dropped it) and #8665 (webhook helm-template fix). Verified live on dev10: - v1.19.3: Challenge solver class = "tenant-root" (BUG) - v1.20.2: Challenge solver class = "<tenant>" (FIXED) Notable upgrade considerations: - Container UID/GID change 1000/0 -> 65532/65532 - New per-component NetworkPolicy templates (gated, default off) - Image template signature refactored (cozystack values.yaml empty, renders identically) - CRDs gain selectableFields (k8s >=1.30) and Azure DNS01 zoneType - RBAC adds clusterissuers/issuers finalizers + listenersets Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request performs a necessary upstream update of the cert-manager Helm chart to v1.20.2. The primary driver for this change is to enable proper support for the 'ingressClassName' API, which was not correctly handled in the previous version, causing validation failures for tenant-scoped Ingresses. The update also includes various CRD enhancements and structural adjustments to image handling and network policies. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
📝 WalkthroughWalkthroughThis PR upgrades cert-manager from v1.19.3 to v1.20.2, refactoring the Helm image helper template to use explicit registry/namespace parameters, updating multiple CRD schemas with new Azure DNS zone types and issuer reference field selectors, adding network policies and pod disruption budget enhancements, and expanding webhook client verification options. ChangesCert-Manager v1.19.3 → v1.20.2 Upgrade
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.69.3)Failed to read Trivy output file: ENOENT: no such file or directory, open '/inmem/1278/nsjail-46a56d98-ca5a-401c-b355-e908f55e6f7b/merged/.trivy-output.json' Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
Code Review
This pull request updates the cert-manager-crds package, refactoring the image helper template to support explicit registry and namespace arguments and updating CRD definitions with new fields such as zoneType for Azure DNS and selectableFields for issuer references. It also includes rebranding updates from Venafi to CyberArk Certificate Manager and various grammatical fixes. Review feedback identifies a potential logic issue in the image helper regarding registry precedence that should be addressed, and suggests clarifying the default values in the TokenAudiences description within the source definitions for the generated CRDs.
| {{- if $image.registry -}} | ||
| {{- $repository = printf "%s/%s" $image.registry $repository -}} | ||
| {{- end -}} |
There was a problem hiding this comment.
The logic for constructing the image repository string in the image helper appears to have a potential bug or at least an unclear precedence for the image.registry field when image.repository is empty. Specifically, the if $image.registry block on lines 227-229 will prepend image.registry (from the $image object) after $imageNamespace and $imageRegistry (from the arguments) have already been prepended. This could lead to an incorrect image path like image.registry/imageRegistry/imageNamespace/image.name instead of a more logical imageRegistry/imageNamespace/image.name or image.registry/image.name.
Given that this file is part of a vendored upstream Helm chart, this issue should be reported to the upstream cert-manager project for correction in their source definitions.
References
- Never suggest editing files inside any
charts/directory underpackages/. Those are upstream Helm charts vendored viamake update(which runshelm pull). Any direct edit is overwritten on the next update and provides zero value. If you find an issue that appears to live in vendored chart code, for true upstream bugs, point to the upstream repository and suggest an upstream issue/PR. Similarly, never propose edits to generated files likezz_generated.*.goorpkg/generated/.
There was a problem hiding this comment.
This file is vendored from upstream jetstack/cert-manager — packages/system/cert-manager/Makefile's update target runs helm pull jetstack/cert-manager and then cp charts/cert-manager/templates/_helpers.tpl ../cert-manager-crds/templates/. Any edit here is overwritten on the next bump. The header comment in the file itself states: "This function is standardized across all charts in the cert-manager GH organization. Any changes to this function should also be made in cert-manager, trust-manager, approver-policy, ..." and links to cert-manager/cert-manager#6329.
Also worth noting: the precedence isn't a bug, it's a documented backwards-compatibility shim. The two adjacent comment blocks (lines 206-208 and 224-226) explicitly call this out: when image.registry is set on the values object alongside the new imageRegistry/imageNamespace arguments, both prefixes are applied so existing user values keep working. Cozystack's values.yaml doesn't set image.registry for any cert-manager component, so this branch never fires for us.
If you believe this is genuinely wrong, please raise it upstream at https://github.com/cert-manager/cert-manager.
| TokenAudiences is an optional list of extra audiences to include in the token passed to Vault. | ||
| The default audiences are always included in the token. |
There was a problem hiding this comment.
The description for TokenAudiences on lines 3517-3518 is improved, but could still be more precise. While it states "The default audiences are always included in the token," it doesn't explicitly mention what those default audiences are, which was clearer in the previous version's description. Consider rephrasing to combine the clarity of the old description with the conciseness of the new one, or explicitly state the default audiences.
Since this is a generated file, please consider updating the source definition that generates this CRD.
References
- Never propose edits to generated files like
zz_generated.*.goorpkg/generated/. CRD YAMLs are also generated, so changes should be made in the relevant package schema or source definition rather than hand-editing generated types.
There was a problem hiding this comment.
The CRD YAMLs in this directory are generated upstream by cert-manager from its Go API types and vendored into cozystack via make update in packages/system/cert-manager/Makefile (which moves charts/cert-manager/templates/crd-*.yaml here). The bot's own note acknowledges this: "Since this is a generated file, please consider updating the source definition that generates this CRD."
The wording change is upstream's call. Description text for TokenAudiences lives at https://github.com/cert-manager/cert-manager/blob/v1.20.2/internal/apis/certmanager/types_issuer.go and would need to be changed there. Not actionable in this PR.
| TokenAudiences is an optional list of extra audiences to include in the token passed to Vault. | ||
| The default audiences are always included in the token. |
There was a problem hiding this comment.
The description for TokenAudiences on lines 3516-3517 is improved, but could still be more precise. While it states "The default audiences are always included in the token," it doesn't explicitly mention what those default audiences are, which was clearer in the previous version's description. Consider rephrasing to combine the clarity of the old description with the conciseness of the new one, or explicitly state the default audiences.
Since this is a generated file, please consider updating the source definition that generates this CRD.
References
- Never propose edits to generated files like
zz_generated.*.goorpkg/generated/. CRD YAMLs are also generated, so changes should be made in the relevant package schema or source definition rather than hand-editing generated types.
There was a problem hiding this comment.
Same situation as the identical comment on crd-cert-manager.io_clusterissuers.yaml: this CRD is generated upstream by cert-manager and vendored verbatim via make update. Description wording for TokenAudiences would need to change in cert-manager's Go source. Not actionable here.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/system/cert-manager-crds/templates/crd-acme.cert-manager.io_orders.yaml (1)
270-273:selectableFieldsrequires Kubernetes ≥ 1.31 to take effect.cert-manager v1.20.0 explicitly added selectable fields to CRDs for
.spec.issuerRef.{group, kind, name}— so the entries here are correct and expected.However, the
CustomResourceFieldSelectorsfeature gate backing this field was alpha and disabled by default in Kubernetes v1.30, then enabled by default since Kubernetes v1.31, and reached GA in v1.32. On clusters running Kubernetes < 1.31 the field is silently dropped on admission (no CRD apply error, just no field-selector functionality). Confirm that the Cozystack management-cluster version meets this requirement to get the intended filtering capability for these CRDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/cert-manager-crds/templates/crd-acme.cert-manager.io_orders.yaml` around lines 270 - 273, The CRD uses selectableFields entries (jsonPath: .spec.issuerRef.group, .spec.issuerRef.kind, .spec.issuerRef.name) which only take effect on Kubernetes >= 1.31; update the deployment to either (A) detect/require Kubernetes >= 1.31 and document this prerequisite in the chart/README and release notes, or (B) make the CRD generation conditional so that selectableFields is omitted for clusters < 1.31 (e.g., templating/helm conditional around the selectableFields block or a build flag), ensuring the selectors are only applied when the cluster supports CustomResourceFieldSelectors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/system/cert-manager-crds/templates/crd-acme.cert-manager.io_orders.yaml`:
- Around line 270-273: The CRD uses selectableFields entries (jsonPath:
.spec.issuerRef.group, .spec.issuerRef.kind, .spec.issuerRef.name) which only
take effect on Kubernetes >= 1.31; update the deployment to either (A)
detect/require Kubernetes >= 1.31 and document this prerequisite in the
chart/README and release notes, or (B) make the CRD generation conditional so
that selectableFields is omitted for clusters < 1.31 (e.g., templating/helm
conditional around the selectableFields block or a build flag), ensuring the
selectors are only applied when the cluster supports
CustomResourceFieldSelectors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1165ecd9-e3a5-4e5f-a79d-d7d13d71e3c5
📒 Files selected for processing (27)
packages/system/cert-manager-crds/templates/_helpers.tplpackages/system/cert-manager-crds/templates/crd-acme.cert-manager.io_challenges.yamlpackages/system/cert-manager-crds/templates/crd-acme.cert-manager.io_orders.yamlpackages/system/cert-manager-crds/templates/crd-cert-manager.io_certificaterequests.yamlpackages/system/cert-manager-crds/templates/crd-cert-manager.io_certificates.yamlpackages/system/cert-manager-crds/templates/crd-cert-manager.io_clusterissuers.yamlpackages/system/cert-manager-crds/templates/crd-cert-manager.io_issuers.yamlpackages/system/cert-manager/charts/cert-manager/Chart.yamlpackages/system/cert-manager/charts/cert-manager/README.mdpackages/system/cert-manager/charts/cert-manager/templates/NOTES.txtpackages/system/cert-manager/charts/cert-manager/templates/_helpers.tplpackages/system/cert-manager/charts/cert-manager/templates/cainjector-deployment.yamlpackages/system/cert-manager/charts/cert-manager/templates/cainjector-poddisruptionbudget.yamlpackages/system/cert-manager/charts/cert-manager/templates/deployment.yamlpackages/system/cert-manager/charts/cert-manager/templates/networkpolicy-cainjector.yamlpackages/system/cert-manager/charts/cert-manager/templates/networkpolicy-cert-manager.yamlpackages/system/cert-manager/charts/cert-manager/templates/networkpolicy-egress.yamlpackages/system/cert-manager/charts/cert-manager/templates/networkpolicy-webhooks.yamlpackages/system/cert-manager/charts/cert-manager/templates/poddisruptionbudget.yamlpackages/system/cert-manager/charts/cert-manager/templates/podmonitor.yamlpackages/system/cert-manager/charts/cert-manager/templates/rbac.yamlpackages/system/cert-manager/charts/cert-manager/templates/servicemonitor.yamlpackages/system/cert-manager/charts/cert-manager/templates/startupapicheck-job.yamlpackages/system/cert-manager/charts/cert-manager/templates/webhook-deployment.yamlpackages/system/cert-manager/charts/cert-manager/templates/webhook-poddisruptionbudget.yamlpackages/system/cert-manager/charts/cert-manager/values.schema.jsonpackages/system/cert-manager/charts/cert-manager/values.yaml
💤 Files with no reviewable changes (1)
- packages/system/cert-manager/charts/cert-manager/templates/networkpolicy-egress.yaml
|
Re: CodeRabbit's nitpick on Also: the CRD YAML is vendored upstream from cert-manager, so the file itself can't be conditionally templated without diverging from |
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — clean upstream vendor bump (v1.19.3 → v1.20.2) that closes a real bug. All 27 vendored files match upstream byte-for-byte; cozystack's empty wrapper values.yaml renders identically.
Business context
Tenant-scoped Ingresses inside non-tenant-root tenants couldn't complete Let's Encrypt validation: cozystack templates have been emitting acme.cert-manager.io/http01-ingress-ingressclassname since 2b6e20c, but cert-manager v1.19.x silently ignored it (annotation only landed upstream in v1.20.0 via cert-manager#8244). Solver Ingresses fell back to the cluster's root IngressClass instead of the tenant's, breaking ACME for any tenant ingress not on tenant-root.
What I verified
- Mechanical bump:
make updateis the actual mechanism. cert-manager-crds_helpers.tplis byte-identical to cert-manager_helpers.tpl(the Makefile copies it), and CRD YAMLs matchhelm pull jetstack/cert-manager v1.20.2 --untaroutput. No local edits leaked. - Right patch level: targeting v1.20.2 (not .0) picks up cert-manager#8655 (clusterissuers/issuers/finalizers RBAC restored after v1.20.0 dropped it) and cert-manager#8665 (webhook helm-template fix). Good due diligence.
- Renders cleanly:
helm templateof bothcozy-cert-managerandcozy-cert-manager-crdssucceeds. With the empty wrappervalues.yaml, all four image references render identically (quay.io/jetstack/cert-manager-{controller,webhook,cainjector,acmesolver}:v1.20.2). Legacyimage.repositoryoverrides still work via the new helper's backwards-compat shim. - Dormant new RBAC: the new
gateway.networking.k8s.io.listenersets[/finalizers]rules don't engage anything in cozystack — controller ListenerSet support is alpha and gated off by default, and cozystack's bundled gateway-api-crds (v1.2.0 experimental) predates ListenerSet (added in Gateway API v1.3 alpha). Forward-compatible plumbing. - UID/GID 1000→65532 is safe here: wrapper values don't pin
runAsUser/runAsGroup, no PSP/SCC anywhere in the platform pins UID 1000, and the chart defaultpodSecurityPolicy.enabled: falsekeeps the upstream PSP templates dead. Talos clusters on k8s ≥ 1.30 have PSPs removed regardless. - New CRD fields are pure additions:
selectableFieldsand Azure DNS01zoneTypeenum land cleanly. Across cozystack's supported k8s range (1.30–1.35),selectableFieldsis alpha-stripped on 1.30 (no error, just no-op) and beta-default-on from 1.31. - CI green including the full ~1h E2E matrix.
Non-blocking follow-ups (not for this PR)
- The UID/GID change is the only operationally-relevant note for downstream operators. Worth surfacing in the next cozystack release notes verbatim from the PR body's "Upgrade considerations".
packages/system/cert-manager-issuers/values.yamlcarries a stalecert-manager.installCRDs: trueblock with no effect (the Chart.yaml has nocert-managersubchart dependency). Cleanup PR worth filing separately.
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM. Verified this is a clean upstream bump:
- All 27 changed files are confined to
packages/system/cert-manager/charts/cert-manager/(vendored chart) andpackages/system/cert-manager-crds/templates/(vendored CRDs). No changes to Cozystack-side wrapper values, templates, Makefile, or Chart.yaml —make updatedid exactly what it should. - The breaking changes flagged in the description (UID/GID 1000/0 → 65532/65532, image structure refactor, networkpolicy template restructure, RBAC additions,
DefaultPrivateKeyRotationPolicyAlwaysGA) are correctly documented. Cozystack's wrappervalues.yamlis empty so the image-structure refactor produces zero rendered diff. - The
ingressClassNamepropagation fix is the right reason to land this —cert-manager#8244(v1.20.0) is what makes the per-tenantacme.cert-manager.io/http01-ingress-ingressclassnameannotation actually take effect; dev10 verification confirms the chain end-to-end. - Targeting v1.20.2 (skipping v1.20.0/.1) picks up the post-release RBAC restoration (
#8655) and webhook helm-template fix (#8665) — right call.
Ship it.
What this PR does
Bumps the vendored cert-manager chart from v1.19.3 to v1.20.2. Pure upstream bump
via
make update— no cozystack-side template changes.Why
Commit
2b6e20ccmigratedthe platform's ACME HTTP-01 setup to the modern
ingressClassNameAPI on boththe ClusterIssuer's solver and the per-tenant override annotation
acme.cert-manager.io/http01-ingress-ingressclassname. The override annotation,however, was only added to upstream cert-manager in v1.20.0
(cert-manager#8244) — v1.19.x silently ignores it. Result: cert-manager always
falls back to the ClusterIssuer's solver class (
tenant-root), so HTTP-01 solverIngresses for non-
tenant-roottenants land on the wrong controller and Let'sEncrypt validation fails for tenant-scoped Ingresses (grafana, harbor, alerta,
etc. inside child tenants).
Targeting v1.20.2 (not .0):
clusterissuers/issuers/finalizersRBAC dropped in v1.20.0.
Verified live on dev10
Challenge.spec.solver.http01.ingress.ingressClassName"tenant-root"(bug)"<tenant>"(fixed)Full propagation chain confirmed end-to-end: source Ingress annotation -> ingress-shim
sets
http01-override-ingress-ingressclassnameon the Certificate -> acmeorders setsspec.solver.http01.ingress.ingressClassNameon the Challenge -> http01 solver createsthe solver Ingress on the tenant's class.
Upgrade considerations
cainjector. PSP / SecurityContext rules pinning UID 1000 will need updating.
image.repositorytoimageRegistry+imageNamespace+image.name. Cozystack's wrappervalues.yamlis empty, so rendered image references are unchanged(
quay.io/jetstack/cert-manager-{controller,webhook,cainjector,acmesolver}:v1.20.2).networkpolicy-egress.yamlremoved; per-component
networkpolicy-cainjector.yamlandnetworkpolicy-cert-manager.yamladded. All gated on*.networkPolicy.enabled: false(default). No behavioral change.selectableFieldsadded (Kubernetes >=1.30) and Azure DNS01zoneTypeenum. Both pure additions.
clusterissuers/issuers/finalizersandgateway.networking.k8s.io.listenersets[/finalizers].DefaultPrivateKeyRotationPolicyAlwaysfeature gate moves Beta -> GA, can nolonger be disabled. Already default since v1.18; no behavior change.
Release note
Summary by CodeRabbit
Release Notes
New Features
Updates
Documentation