[cert-manager] Update cert-manager to v1.19.3#2070
Conversation
myasnikovdaniil
requested review from
IvanHunters,
androndo,
kvaps,
lexfrei and
lllamnyp
as code owners
dosubot
bot
added
the
size:XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCRD manifests and helpers were moved/added into a dedicated cert-manager-crds chart; cert-manager chart bumped to v1.19.3 and gained global.nodeSelector and global.hostUsers with template changes (merged nodeSelector rendering, hostUsers injection, RBAC naming, and conditional ServiceMonitor fields). Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello @myasnikovdaniil, 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 primarily focuses on upgrading the cert-manager component to version 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. Changelog
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-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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates cert-manager to v1.19.3. The changes include version bumps in Chart.yaml, and updates to documentation, values, and templates to align with the new upstream version. While many of the changes are positive improvements, such as better Helm templating and support for new features, I've identified two critical issues related to how the upstream chart was vendored, which result in missing Custom Resource Definitions (CRDs). These issues will prevent cert-manager from deploying or functioning correctly and must be addressed.
packages/system/cert-manager/charts/cert-manager/templates/serviceaccount.yaml
Show resolved
Hide resolved
![coderabbitai[bot]](proxy.php?url=https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/system/cert-manager-crds/charts/cert-manager/README.md (1)
96-140:⚠️ Potential issue | 🟡 MinorAdd [cert-manager-crds] prefix and --signoff to commit message.
The commit message should follow the
[component] Descriptionformat with a Signed-off-by line, per repository guidelines. The current message "Update cert-manager v1.19.3" is missing the [cert-manager-crds] component prefix and lacks signoff. Amend the commit usinggit commit --amend --signoffand update the message to[cert-manager-crds] Update cert-manager v1.19.3.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/cert-manager-crds/charts/cert-manager/README.md` around lines 96 - 140, Amend the last commit message to include the component prefix and a sign-off: change the message to "[cert-manager-crds] Update cert-manager v1.19.3" and add a Signed-off-by line; update the existing commit (amend) so the repository history contains the corrected message and signoff for this cert-manager-crds change.
🧹 Nitpick comments (1)
packages/system/cert-manager/charts/cert-manager/README.md (1)
1-2011: Direct edits to vendored upstream chart files violate the**/*.yamlcoding guidelineAll YAML and Markdown files changed in this PR reside under
charts/cert-manager/— the vendored upstream chart directory. The guideline prohibits directly editing files there; the proper mechanism is chart vendoring (e.g.,helm pull jetstack/cert-manager --version v1.19.3 --untar). Direct manual edits make it difficult to track local divergences from upstream and can be silently overwritten on the next vendor operation.If the Makefile already has a
helm pull-based update target, use that. Otherwise, consider adding one so future updates follow the proper mechanism.As per coding guidelines: "Do NOT directly edit upstream Helm charts in vendored charts directory; use proper chart vendoring mechanisms."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/cert-manager/charts/cert-manager/README.md` around lines 1 - 2011, The README.md under the vendored cert-manager chart was edited directly (local modifications to the vendored upstream chart), which violates the repo rule to not edit vendored YAML/Markdown; revert these manual edits and replace them by vendoring the upstream chart using Helm (e.g., run helm pull jetstack/cert-manager --version v1.19.3 --untar to regenerate the chart files), commit the regenerated chart instead of hand-editing README.md, and add a Makefile target (e.g., update-cert-manager-chart or vendor-charts) that encapsulates the helm pull/untar step so future updates follow the proper chart vendoring mechanism.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/system/cert-manager/charts/cert-manager/templates/deployment.yaml`:
- Around line 69-71: The hostUsers template line contains trailing spaces after
the Helm expression; update the line "hostUsers: {{ .Values.global.hostUsers }}"
to eliminate trailing whitespace and trim surrounding template whitespace (e.g.,
use the Helm whitespace-control hyphen on the expression such as changing the
expression to trim output) and apply the same change to the corresponding lines
that render hostUsers in the other templates (the cainjector,
startupapicheck-job, and webhook templates) so no trailing spaces remain after
the closing braces.
- Around line 215-222: The merge call currently uses "$nodeSelector = merge
$nodeSelector (.Values.nodeSelector | default dict)" which makes global values
win; change the merge operand order so the component-level nodeSelector is the
destination and thus takes precedence, e.g. "$nodeSelector = merge
(.Values.nodeSelector | default dict) $nodeSelector" (keep the initial
"$nodeSelector := .Values.global.nodeSelector | default dict" setup), and apply
the identical operand swap for the same merge usage found in the other templates
(the cainjector, startupapicheck job, and webhook templates) so
component-specific nodeSelector overrides global.
---
Outside diff comments:
In `@packages/system/cert-manager-crds/charts/cert-manager/README.md`:
- Around line 96-140: Amend the last commit message to include the component
prefix and a sign-off: change the message to "[cert-manager-crds] Update
cert-manager v1.19.3" and add a Signed-off-by line; update the existing commit
(amend) so the repository history contains the corrected message and signoff for
this cert-manager-crds change.
---
Duplicate comments:
In
`@packages/system/cert-manager/charts/cert-manager/templates/cainjector-deployment.yaml`:
- Around line 70-72: The hostUsers block has trailing whitespace and the Helm
tpl merge operands are inverted; remove the trailing space after {{
.Values.global.hostUsers }} and correct the merge operand order wherever you
used merge (e.g., the same pattern around the other block) so that the
destination map is the first argument and the source map is the second — update
the conditional block using hasKey .Values.global "hostUsers" and the merge(...)
call that handles global values to use merge(dest, src) and ensure
.Values.global.hostUsers has no trailing whitespace.
In
`@packages/system/cert-manager/charts/cert-manager/templates/crd-acme.cert-manager.io_orders.yaml`:
- Around line 1-274: The change directly edits the vendored Helm chart template
crd-acme.cert-manager.io_orders.yaml in packages/system/cert-manager/charts,
which violates the guideline to not modify upstream charts in-place; revert the
edits to crd-acme.cert-manager.io_orders.yaml and instead apply your change via
the proper vendoring mechanism (e.g., update the upstream cert-manager chart
version and re-vendor, add a patch/overlay using kustomize/helmfile/helm
chart-operator, or supply values in values.yaml) so the CRD change is introduced
through the chart dependency or an overlay; ensure you update the chart
lock/metadata (or add a documented patch) rather than committing direct edits to
the template.
In
`@packages/system/cert-manager/charts/cert-manager/templates/crd-cert-manager.io_certificates.yaml`:
- Around line 1-816: This change directly edits the vendored Helm chart template
(the Certificate CRD defined as kind: CustomResourceDefinition with metadata
name "certificates.cert-manager.io" in
cert-manager/templates/crd-cert-manager.io_certificates.yaml), which violates
the rule to not modify upstream charts in-place; revert this file to the
vendored upstream state and instead apply your modifications by updating the
vendored chart properly (bump the chart version / update Chart.yaml to the
desired upstream cert-manager version and re-vendor using the project’s chart
vendoring workflow such as running helm repo update + helm pull/helm dependency
update or your repository’s chart-vendoring script), or put required overrides
into values.yaml / a wrapper chart so no direct edits to templates/*.yaml are
made.
In
`@packages/system/cert-manager/charts/cert-manager/templates/startupapicheck-job.yaml`:
- Around line 44-46: The template's conditional uses hasKey with inverted
operand order and the hostUsers value line contains trailing whitespace; update
the hasKey call to use the correct operand order (call hasKey with the map first
and the key second as in hasKey .Values.global "hostUsers") and remove the
trailing spaces after {{ .Values.global.hostUsers }} so the line is trimmed;
apply the same fixes to the other duplicate block referenced (lines 82–89) to
mirror the corrections made in deployment.yaml.
In
`@packages/system/cert-manager/charts/cert-manager/templates/webhook-deployment.yaml`:
- Around line 69-71: The helm template snippet uses hasKey and hostUsers but
leaves trailing whitespace after the hostUsers value and also uses the merge
operator with inverted operand order elsewhere; remove the trailing space after
"{{ .Values.global.hostUsers }}" and fix the merge operand order by swapping the
operands so the target/default ordering matches the other deployment.yaml fixes
(look for usages of merge and ensure the left/right operands are in the correct
order), updating the template block that references hasKey and
.Values.global.hostUsers accordingly.
---
Nitpick comments:
In `@packages/system/cert-manager/charts/cert-manager/README.md`:
- Around line 1-2011: The README.md under the vendored cert-manager chart was
edited directly (local modifications to the vendored upstream chart), which
violates the repo rule to not edit vendored YAML/Markdown; revert these manual
edits and replace them by vendoring the upstream chart using Helm (e.g., run
helm pull jetstack/cert-manager --version v1.19.3 --untar to regenerate the
chart files), commit the regenerated chart instead of hand-editing README.md,
and add a Makefile target (e.g., update-cert-manager-chart or vendor-charts)
that encapsulates the helm pull/untar step so future updates follow the proper
chart vendoring mechanism.
packages/system/cert-manager/charts/cert-manager/templates/deployment.yaml
Show resolved
Hide resolved
packages/system/cert-manager/charts/cert-manager/templates/deployment.yaml
Show resolved
Hide resolved
myasnikovdaniil
force-pushed
the
update-cert-manager
branch
from
4807247 to
1d12cc4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/system/cert-manager-crds/templates/_helpers.tpl (1)
1-213: Consider trimming unused helper templates from the CRDs chart.This
_helpers.tplis a full copy of the main cert-manager chart's helpers, but the CRD templates only invokecert-manager.crd-labels(and its transitive dependencies:cert-manager.name,labels,chartName). The remaining templates (webhook.*,cainjector.*,startupapicheck.*,image,cert-manager.serviceAccountName,cert-manager.fullname,cert-manager.namespace,cert-manager.crd-check) are dead code in this chart context.Keeping the full file simplifies re-vendoring but increases the maintenance surface. A minimal
_helpers.tplwith only the needed templates would be cleaner, though this can be deferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/cert-manager-crds/templates/_helpers.tpl` around lines 1 - 213, This helpers file contains many templates not used by the CRDs chart; keep only the templates required by the CRD manifests (cert-manager.crd-labels and its transitive dependencies: cert-manager.name, labels, chartName) and remove dead templates (e.g., webhook.*, cainjector.*, startupapicheck.*, image, cert-manager.serviceAccountName, cert-manager.fullname, cert-manager.namespace, cert-manager.crd-check) so the file is minimal; ensure you preserve any small helper fragments referenced by cert-manager.crd-labels (and update any include/ template names if needed) and run helm template to verify no missing template references.packages/system/cert-manager/charts/cert-manager/values.yaml (1)
529-550: Heads-up: v1.19 removes a Prometheus metric label — existing dashboards/alerts may break.The v1.19 release removes the high-cardinality
pathlabel from thecertmanager_acme_client_request_countandcertmanager_acme_client_request_duration_secondsmetrics, replacing it with a new bounded-cardinalityactionlabel. This may require updates to dashboards and alerts. Additionally, certificate metrics were updated to the collector approach.Before deploying, audit any Prometheus alerts or Grafana dashboards that query these metrics using
.pathand update them to use.actioninstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/cert-manager/charts/cert-manager/values.yaml` around lines 529 - 550, Add a clear upgrade note in this values.yaml (near the servicemonitor/prometheus settings) warning that v1.19 removed the high-cardinality path label and replaced it with action for certmanager_acme_client_request_count and certmanager_acme_client_request_duration_seconds, and that certificate metrics now use the collector approach; instruct operators to audit and update any Prometheus alerts or Grafana dashboards that reference .path to use .action (or update queries for the collector-style certificate metrics) before enabling ServiceMonitor/Prometheus scraping via the servicemonitor/prometheusInstance/targetPort configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/system/cert-manager-crds/values.yaml`:
- Around line 1-6: The values.yaml is missing the top-level creator key so the
cert-manager.crd-labels template (which checks {{- if eq .Values.creator "helm"
}}) omits managed-by/helm.sh/chart labels; add a top-level creator: "helm" entry
in this values.yaml (next to existing global/crds keys) so .Values.creator
resolves to "helm" and the labels are rendered.
---
Duplicate comments:
In
`@packages/system/cert-manager/charts/cert-manager/templates/cainjector-deployment.yaml`:
- Around line 70-72: The hostUsers line in the cainjector-deployment.yaml
template contains trailing whitespace ("hostUsers: {{ .Values.global.hostUsers
}} "); remove the extra spaces after the template expression so the line
ends immediately after the closing braces, and mirror the same fix applied to
deployment.yaml; update the hostUsers line in the cainjector-deployment.yaml
template and run your YAML/lint checks to confirm no trailing spaces remain.
- Around line 142-143: The nodeSelector merge currently uses merge $nodeSelector
(.Values.cainjector.nodeSelector | default dict) so the global map ends up
winning; flip the operands so the cainjector-specific map takes precedence by
changing the merge call to merge (.Values.cainjector.nodeSelector | default
dict) $nodeSelector in the cainjector-deployment.yaml block that defines
$nodeSelector to ensure .Values.cainjector.nodeSelector overrides
.Values.global.nodeSelector.
In `@packages/system/cert-manager/charts/cert-manager/templates/deployment.yaml`:
- Around line 69-71: The deployment template block that checks for the presence
of hostUsers (the conditional using hasKey .Values.global "hostUsers" and the
hostUsers: {{ .Values.global.hostUsers }} line) contains trailing whitespace
after the interpolation; remove the extra spaces after the closing }} so the
line is exactly `hostUsers: {{ .Values.global.hostUsers }}` (no trailing spaces)
and ensure similar lines have no trailing whitespace.
- Around line 215-221: The merge currently gives global nodeSelector precedence;
swap the operands so the component-specific selector wins by changing the merge
invocation from merge $nodeSelector (.Values.nodeSelector | default dict) to
merge (.Values.nodeSelector | default dict) $nodeSelector (and make the
identical change in cainjector-deployment.yaml and webhook-deployment.yaml where
lines show the same merge usage) so component-specific .Values.nodeSelector
overrides the global value.
In
`@packages/system/cert-manager/charts/cert-manager/templates/webhook-deployment.yaml`:
- Around line 69-71: The template line setting hostUsers contains trailing
whitespace; remove the extra spaces after {{ .Values.global.hostUsers }} so the
hostUsers line is clean (in the webhook-deployment template where hostUsers is
rendered using .Values.global.hostUsers) to match other deployment templates and
avoid formatting lint errors.
- Around line 186-192: The nodeSelector merge uses merge $nodeSelector
(.Values.webhook.nodeSelector | default dict) with global values taking
precedence; change the merge operand order so the webhook-specific selector
overrides globals — i.e., compute $nodeSelector by merging
(.Values.webhook.nodeSelector | default dict) with .Values.global.nodeSelector |
default dict instead of the current order, updating the $nodeSelector assignment
where merge is called (referencing the $nodeSelector variable and the
.Values.webhook.nodeSelector and .Values.global.nodeSelector operands).
---
Nitpick comments:
In `@packages/system/cert-manager-crds/templates/_helpers.tpl`:
- Around line 1-213: This helpers file contains many templates not used by the
CRDs chart; keep only the templates required by the CRD manifests
(cert-manager.crd-labels and its transitive dependencies: cert-manager.name,
labels, chartName) and remove dead templates (e.g., webhook.*, cainjector.*,
startupapicheck.*, image, cert-manager.serviceAccountName,
cert-manager.fullname, cert-manager.namespace, cert-manager.crd-check) so the
file is minimal; ensure you preserve any small helper fragments referenced by
cert-manager.crd-labels (and update any include/ template names if needed) and
run helm template to verify no missing template references.
In `@packages/system/cert-manager/charts/cert-manager/values.yaml`:
- Around line 529-550: Add a clear upgrade note in this values.yaml (near the
servicemonitor/prometheus settings) warning that v1.19 removed the
high-cardinality path label and replaced it with action for
certmanager_acme_client_request_count and
certmanager_acme_client_request_duration_seconds, and that certificate metrics
now use the collector approach; instruct operators to audit and update any
Prometheus alerts or Grafana dashboards that reference .path to use .action (or
update queries for the collector-style certificate metrics) before enabling
ServiceMonitor/Prometheus scraping via the
servicemonitor/prometheusInstance/targetPort configuration.
There was a problem hiding this comment.
E2E Test Failure: kubernetes-latest (v1.33) — Nodes Never Become Ready
Test Failure Summary
The kubernetes-latest E2E test consistently fails (3 attempts, all fail). The kubernetes-previous (v1.32) test passes on this same PR. Both tests pass on main.
| cert-manager | tenant K8s | Node Ready time | Result |
|---|---|---|---|
| v1.17.2 (main) | v1.33 | ~71s | PASS |
| v1.17.2 (main) | v1.32 | ~95s | PASS |
| v1.19.3 (this PR) | v1.33 | >120s, never | FAIL |
| v1.19.3 (this PR) | v1.32 | ~83s | PASS |
The management cluster is K8s v1.33.2 in both cases. cert-manager is disabled in the tenant cluster (certManager: enabled: false).
What happens: The tenant v1.33 control plane comes up fully Ready (KamajiControlPlane INITIALIZED=true, READY=true, all deployments available, MachineDeployment replicas=2). Worker nodes appear (2 nodes found), but they never become Ready — even after 7+ minutes across 3 retries. This is not a flaky test or timeout edge case — something is fundamentally broken.
Refs:
cert-manager v1.17 → v1.19 Breaking Changes
This update skips v1.18, which introduced significant behavioral changes:
v1.17 → v1.18:
PrivateKeyRotationPolicydefault changed fromNevertoAlways(feature gateDefaultPrivateKeyRotationPolicyAlways). Any certificate renewal now generates a new private key. For CA certificates, this can break the entire trust chain if downstream certs aren't re-issued.RevisionHistoryLimitdefault: nil → 1. Old CertificateRequest resources are auto-deleted.- RSA signing: SHA-256 → SHA-384 for ≥3072-bit keys, SHA-512 for ≥4096-bit keys.
v1.18 → v1.19:
- CRD-based API defaults for
issuerRef:groupnow defaults tocert-manager.io,kindtoIssuer. This caused mass certificate re-issuance (#8158, #8182). - v1.19.3 fixes infinite re-issuance loop when issuer returns cert with mismatched public key.
- SKI (Subject Key Identifier) generation changed due to Go runtime update (#8289).
Affected Certificate Resources in Cozystack
All webhook Certificate resources in the management cluster define issuerRef without the group field:
Kamaji (packages/system/kamaji/charts/kamaji/templates/certmanager_certificate.yaml):
issuerRef:
kind: Issuer
name: kamaji-selfsigned-issuer
# group: NOT specifiedCAPI Core (packages/system/capi-providers-core/files/core-components.yaml):
issuerRef:
kind: Issuer
name: capi-selfsigned-issuer
# group: NOT specifiedCAPI Bootstrap and Infrastructure providers follow the same pattern.
With v1.19 CRDs, the API server adds group: cert-manager.io automatically. Combined with PrivateKeyRotationPolicy: Always, this could trigger webhook certificate re-issuance with new keys, temporarily or permanently breaking webhook availability for Kamaji and CAPI providers.
Values Configuration
# Old (main) - correctly nested under subchart name:
cert-manager:
installCRDs: false
# New (this PR) - NOT nested under subchart:
crds:
enabled: trueThe crds: enabled: true is at the top level of the umbrella chart, not nested under cert-manager:. The subchart receives only its default values. Functionally this works since CRDs are in a separate package and the only override was installCRDs: false, but the structure is misleading.
Also: _helpers.tpl in the CRDs package references .Values.creator which is not defined in cert-manager-crds/values.yaml (it comes from the cert-manager chart defaults where it equals "helm").
Recommended Next Steps
To debug the v1.33 node readiness failure, you can add temporary diagnostic output to the E2E test. For example, in hack/e2e-apps/run-kubernetes.sh before the wait node --all --for=condition=Ready call (line 101), add commands to dump node conditions and pod status via the tenant kubeconfig:
kubectl --kubeconfig "tenantkubeconfig-${test_name}" get nodes -o wide
kubectl --kubeconfig "tenantkubeconfig-${test_name}" describe nodes
kubectl -n tenant-test get hr # Cilium/CoreDNS HelmRelease status
kubectl -n tenant-test get certificates # cert-manager certificate healthPush the change, and the CI output will show what's blocking node readiness (missing CNI, uninitialized taint, etc.).
Beyond debugging, consider these fixes:
-
Pin
privateKey.rotationPolicy: Neverfor webhook certificates (Kamaji, CAPI providers) to avoid unexpected key rotation introduced in v1.18+ -
Add explicit
group: cert-manager.iotoissuerRefin Kamaji/CAPI Certificate resources to avoid CRD defaulting side effects (see #8158, #8182) -
Ensure
kubernetes-latestE2E passes before merging
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/e2e-apps/run-kubernetes.sh`:
- Line 101: The kubectl node readiness wait in run-kubernetes.sh currently
swallows failures with "|| true"; change it to capture the kubectl exit code
from the wait (the command starting with "kubectl --kubeconfig
\"tenantkubeconfig-${test_name}\" wait node --all --timeout=2m
--for=condition=Ready"), run the diagnostic dump when that exit code is
non-zero, then exit the script with that captured non-zero code so the test
still fails after diagnostics are printed.
- Around line 104-105: The two diagnostic kubectl commands (kubectl -n
tenant-test get hr and kubectl -n tenant-test get certificates) can fail under
set -e and should not abort the script; modify those commands to append "||
true" so they always succeed (e.g., change "kubectl -n tenant-test get hr" and
"kubectl -n tenant-test get certificates" to "kubectl -n tenant-test get hr ||
true" and "kubectl -n tenant-test get certificates || true").
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@hack/e2e-apps/run-kubernetes.sh`:
- Around line 101-105: The current replacement of "|| true" with the if !
kubectl --kubeconfig "tenantkubeconfig-${test_name}" wait node --all
--timeout=2m --for=condition=Ready; then ... fi block prints diagnostics when
nodes are not Ready but still allows the script to continue; update the block so
that after running diagnostics (the kubectl describe nodes and kubectl -n
tenant-test get hr commands) the script exits with a non-zero status
(propagating the kubectl wait failure) — e.g., capture the wait command's exit
code or use an explicit exit with that code — to ensure the test gate fails when
nodes are NotReady.
- Line 104: The diagnostic kubectl call "kubectl -n tenant-test get hr" can
return non-zero and abort the script under set -e; change that invocation in
hack/e2e-apps/run-kubernetes.sh (the kubectl -n tenant-test get hr line) to be
non-fatal by appending a safe-fail suffix (e.g., use "|| true") so it won't
terminate the script if the HelmRelease CRD is missing while still running the
rest of the diagnostics and failure handling.
There was a problem hiding this comment.
I don't think this changed is related to the PRs intent.
myasnikovdaniil
force-pushed
the
update-cert-manager
branch
from
e1e458b to
4a33162
Compare
myasnikovdaniil
force-pushed
the
update-cert-manager
branch
from
cd94b77 to
3d341c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/system/cert-manager/charts/cert-manager/templates/cainjector-deployment.yaml`:
- Around line 142-143: The merge invocation uses global first so
component-specific values are being preserved instead of overriding; fix by
creating a vendored-chart overlay/patch that flips the merge argument order
(i.e., call merge with the component-specific nodeSelector as the first arg and
the global one second) for the templates referencing $nodeSelector and merge
(specifically in the cainjector-deployment.yaml, deployment.yaml,
webhook-deployment.yaml and startupapicheck-job.yaml templates), or
alternatively open an upstream cert-manager bug/PR to change the merge
precedence in those template usages rather than editing upstream directly.
In
`@packages/system/cert-manager/charts/cert-manager/templates/startupapicheck-job.yaml`:
- Around line 82-83: The merge call for $nodeSelector is using the wrong
argument order so global values take precedence; change the merge invocation so
component-specific values override global by merging the global dict into the
component dict (i.e. call merge with (.Values.startupapicheck.nodeSelector |
default dict) as the first/primary map and .Values.global.nodeSelector as the
second), updating the lines that set $nodeSelector (the variables $nodeSelector,
.Values.global.nodeSelector, .Values.startupapicheck.nodeSelector and the merge
call).
In `@packages/system/cert-manager/charts/cert-manager/values.yaml`:
- Around line 41-50: The comment for the configuration key global.hostUsers is
ambiguous; update the block so it clearly states the boolean semantics in one
place (e.g., "global.hostUsers: true = pods run with host user IDs and may
access host resources; false = pods run in user namespaces without host
access"), include the required Kubernetes version/feature-gate note and the
limitations link, and ensure any previous contradictory sentence ("Set to
false...") is removed or replaced so only the explicit true/false behavior is
described for global.hostUsers.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
packages/system/cert-manager-crds/Makefilepackages/system/cert-manager-crds/charts/cert-manager/Chart.yamlpackages/system/cert-manager-crds/charts/cert-manager/README.mdpackages/system/cert-manager-crds/charts/cert-manager/templates/crds.yamlpackages/system/cert-manager-crds/charts/cert-manager/values.schema.jsonpackages/system/cert-manager-crds/charts/cert-manager/values.yamlpackages/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-crds/values.yamlpackages/system/cert-manager/Makefilepackages/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/crds.yamlpackages/system/cert-manager/charts/cert-manager/templates/deployment.yamlpackages/system/cert-manager/charts/cert-manager/templates/rbac.yamlpackages/system/cert-manager/charts/cert-manager/templates/serviceaccount.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/values.schema.jsonpackages/system/cert-manager/charts/cert-manager/values.yamlpackages/system/cert-manager/values.yaml
💤 Files with no reviewable changes (5)
- packages/system/cert-manager-crds/charts/cert-manager/values.schema.json
- packages/system/cert-manager/values.yaml
- packages/system/cert-manager-crds/charts/cert-manager/README.md
- packages/system/cert-manager-crds/charts/cert-manager/Chart.yaml
- packages/system/cert-manager-crds/charts/cert-manager/values.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/system/cert-manager/charts/cert-manager/templates/servicemonitor.yaml
- packages/system/cert-manager/charts/cert-manager/templates/deployment.yaml
- packages/system/cert-manager/charts/cert-manager/templates/webhook-deployment.yaml
- packages/system/cert-manager/charts/cert-manager/templates/rbac.yaml
- packages/system/cert-manager-crds/templates/_helpers.tpl
packages/system/cert-manager/charts/cert-manager/templates/cainjector-deployment.yaml
Show resolved
Hide resolved
packages/system/cert-manager/charts/cert-manager/templates/startupapicheck-job.yaml
Show resolved
Hide resolved
Signed-off-by: Myasnikov Daniil <[email protected]>
lexfrei
force-pushed
the
update-cert-manager
branch
from
3d341c9 to
4df456f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/system/cert-manager/charts/cert-manager/templates/cainjector-deployment.yaml (1)
142-143:⚠️ Potential issue | 🟠 MajorComponent
nodeSelectorprecedence is currently reversed (and mutates shared map state).At Line 142 and Line 143,
mergeis called with the global selector as the first argument, so conflicting keys keep global values instead of component values. Becausemergemutates its first argument, component-only keys can also bleed into later renders.Suggested fix
- {{- $nodeSelector := .Values.global.nodeSelector | default dict }} - {{- $nodeSelector = merge $nodeSelector (.Values.cainjector.nodeSelector | default dict) }} + {{- $nodeSelector := deepCopy (.Values.global.nodeSelector | default dict) }} + {{- $nodeSelector = mergeOverwrite $nodeSelector (.Values.cainjector.nodeSelector | default dict) }}Based on learnings: In Helm templates,
mergeOverwritehas right-to-left precedence, so defaults should be left and user/component values should be right to take precedence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/system/cert-manager/charts/cert-manager/templates/cainjector-deployment.yaml` around lines 142 - 143, The nodeSelector merge is using merge with the global selector first which preserves global keys and mutates the first map (variable $nodeSelector), causing component keys to be ignored or bleed into later renders; change the logic to use mergeOverwrite (which has right-to-left precedence) and pass defaults on the left and the component/user selector on the right so .Values.cainjector.nodeSelector overrides .Values.global.nodeSelector, and avoid mutating shared state by assigning the merged result back to $nodeSelector (reference the variables $nodeSelector, .Values.global.nodeSelector, .Values.cainjector.nodeSelector and replace merge with mergeOverwrite).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/system/cert-manager/charts/cert-manager/Chart.yaml`:
- Line 9: The Chart.yaml change bumping appVersion to v1.19.3 was made directly
in the vendored cert-manager chart; revert that direct edit and update the
vendored chart using the project's chart dependency/vendoring workflow instead
(e.g., update the Helm dependency entry or run the vendoring tool to fetch
cert-manager v1.19.3), ensure Chart.yaml and Chart.lock (or vendor metadata) are
updated via the tooling rather than manual edits, and include a short commit
message referencing the cert-manager version and the vendoring command used so
reviewers can verify the proper dependency update.
In `@packages/system/cert-manager/charts/cert-manager/values.schema.json`:
- Around line 727-729: The description for helm-values.global.hostUsers in
values.schema.json is contradictory about the true/false behavior; update the
source values metadata (the original values.yaml/metadata used by
cozyvalues-gen) so the description unambiguously states what true and false do
(e.g., "true = run pods in a user namespace without host access; false = do not
use user namespaces and allow host access"), ensure any mention of requirements
remains, then regenerate values.schema.json and docs using cozyvalues-gen so the
produced schema contains the clarified description for
helm-values.global.hostUsers.
---
Duplicate comments:
In
`@packages/system/cert-manager/charts/cert-manager/templates/cainjector-deployment.yaml`:
- Around line 142-143: The nodeSelector merge is using merge with the global
selector first which preserves global keys and mutates the first map (variable
$nodeSelector), causing component keys to be ignored or bleed into later
renders; change the logic to use mergeOverwrite (which has right-to-left
precedence) and pass defaults on the left and the component/user selector on the
right so .Values.cainjector.nodeSelector overrides .Values.global.nodeSelector,
and avoid mutating shared state by assigning the merged result back to
$nodeSelector (reference the variables $nodeSelector,
.Values.global.nodeSelector, .Values.cainjector.nodeSelector and replace merge
with mergeOverwrite).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
packages/system/cert-manager-crds/Makefilepackages/system/cert-manager-crds/charts/cert-manager/Chart.yamlpackages/system/cert-manager-crds/charts/cert-manager/README.mdpackages/system/cert-manager-crds/charts/cert-manager/templates/crds.yamlpackages/system/cert-manager-crds/charts/cert-manager/values.schema.jsonpackages/system/cert-manager-crds/charts/cert-manager/values.yamlpackages/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-crds/values.yamlpackages/system/cert-manager/Makefilepackages/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/crds.yamlpackages/system/cert-manager/charts/cert-manager/templates/deployment.yamlpackages/system/cert-manager/charts/cert-manager/templates/rbac.yamlpackages/system/cert-manager/charts/cert-manager/templates/serviceaccount.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/values.schema.jsonpackages/system/cert-manager/charts/cert-manager/values.yamlpackages/system/cert-manager/values.yaml
💤 Files with no reviewable changes (5)
- packages/system/cert-manager/values.yaml
- packages/system/cert-manager-crds/charts/cert-manager/README.md
- packages/system/cert-manager-crds/charts/cert-manager/values.schema.json
- packages/system/cert-manager-crds/charts/cert-manager/Chart.yaml
- packages/system/cert-manager-crds/charts/cert-manager/values.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/system/cert-manager-crds/values.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/system/cert-manager-crds/templates/_helpers.tpl
- packages/system/cert-manager/charts/cert-manager/templates/_helpers.tpl
- packages/system/cert-manager/charts/cert-manager/templates/webhook-deployment.yaml
- packages/system/cert-manager/charts/cert-manager/templates/startupapicheck-job.yaml
lexfrei
left a comment
lexfrei
left a comment
There was a problem hiding this comment.
Overview
The structural change from a subchart-based CRD packaging to direct templates is a solid improvement — cleaner, simpler, and aligned with upstream cert-manager's evolution in v1.19.x where the monolithic crds.yaml was split into individual crd-*.yaml files with native Helm templating.
What looks good
- CRD/chart separation is correct:
installCRDs: falseandcrds.enabled: falsein cert-manager,crds.enabled: trueandcrds.keep: truein cert-manager-crds — no CRD duplication, resource-policy keep annotation is in place. - Version alignment: both cert-manager and cert-manager-crds now use v1.19.3 from the same chart pull (previously they were on different versions: v1.17.2 and v1.16.3 respectively).
- Dependency chain (cert-manager-crds → cert-manager → cert-manager-issuers) is not affected, no platform-level changes needed.
NAMEandNAMESPACEexports added tocert-manager-crds/Makefilefix what was previously missing formake show,make apply, etc.- Both charts render cleanly with
helm template.
Blocking issues
make updatedoes not clean stale CRD files — see inline comment on Makefile.- Breaking behavior change crossing v1.18.0 — the
DefaultPrivateKeyRotationPolicyAlwaysfeature gate is now Beta and enabled by default. This changes the defaultCertificate.Spec.PrivateKey.RotationPolicyfromNevertoAlways— all Certificate resources without an explicitrotationPolicywill start regenerating private keys on every renewal. This should either be explicitly disabled inpackages/system/cert-manager/values.yamlto preserve pre-1.18 behavior:or documented as a known behavior change so operators are aware before upgrading.cert-manager: config: featureGates: DefaultPrivateKeyRotationPolicyAlways: false
Non-blocking suggestions
-
Missing
appVersionincert-manager-crds/Chart.yaml:_helpers.tplrendersapp.kubernetes.io/version: {{ .Chart.AppVersion }}which produces an empty string. Consider addingappVersion: v1.19.3for informational purposes. -
No
updatetarget incert-manager-crds/Makefile: CRD updates now happen throughcert-manager/Makefile, but there's no indication of this for a developer runningmake updatein the cert-manager-crds directory. A stub target would help:update: @echo "CRDs are updated via ../cert-manager/Makefile: make update"
| helm repo add jetstack https://charts.jetstack.io | ||
| helm repo update jetstack | ||
| helm pull jetstack/cert-manager --untar --untardir charts | ||
| mv charts/cert-manager/templates/crd-*.yaml ../cert-manager-crds/templates/ |
There was a problem hiding this comment.
mv and cp add files to ../cert-manager-crds/templates/ but never clean up existing ones first. If a future upstream release renames or removes a CRD file, stale files will remain and continue to be rendered.
Add a cleanup step before mv:
rm -f ../cert-manager-crds/templates/crd-*.yaml ../cert-manager-crds/templates/_helpers.tpl
mv charts/cert-manager/templates/crd-*.yaml ../cert-manager-crds/templates/
cp charts/cert-manager/templates/_helpers.tpl ../cert-manager-crds/templates/There was a problem hiding this comment.
added less specific cleanup for crds, anyway if upstream cert-manager chart will change its structure again (filenames, etc.) than we'll need to refactor this script again
Signed-off-by: Myasnikov Daniil <[email protected]>
4 participants
What this PR does
Release note
Summary by CodeRabbit
New Features
Documentation
Chores