From 5e65a3e792f65a7f57ba0801c446c4815b6cd31c Mon Sep 17 00:00:00 2001 From: Ivan Okhotnikov Date: Thu, 2 Jul 2026 18:56:18 +0300 Subject: [PATCH] docs(vertical-pod-autoscaler): tighten cert-manager comments per review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Kvaps' review on #3178 called out three imprecisions in the supporting comments. Fix them without touching runtime behaviour: 1. certificate.yaml previously emitted a secretTemplate whose labels include \`app.kubernetes.io/managed-by: Helm\`. Helm does NOT own the Secret written by cert-manager — the Certificate resource does — and stamping that label on it misleads operators reading the object with \`kubectl get secret -oyaml\`. Drop secretTemplate; cert-manager labels its outputs unambiguously (\`cert-manager.io/*\`, \`controller.cert-manager.io/*\`). 2. deployment.yaml comment on checksum/tls-cert claimed the lookup \"resolves at every reconcile\". Not true. \`lookup\` runs only on Helm actions (install / upgrade / rollback); helm-controller does not re-render a deployed release on interval ticks. Under cozystack's release cadence this is fine — any real chart bump, values change, or manual \`flux reconcile hr\` triggers a re-template within days — but at day ~350 renewal the pod continues serving the certificate it loaded at startup until the next Helm action. If no Helm action lands in the ~15 days between renewal and expiry the webhook silently stops mutating. The comment now spells that trade-off out so a future reader can find it, and points at a small post-renewal Job as the follow-up without re-introducing the ReplicaSet-churn amplifier the whole PR retires. 3. values.yaml comment on certManagerEnabled did not warn that the switch is effectively one-way. Once cert-manager has written the Secret with \`type: kubernetes.io/tls\`, flipping the flag back to false and rolling back to the legacy Opaque path fails — \`Secret.type\` is immutable, so the legacy Secret template cannot overwrite the cert-manager-owned one in place. The doc now says operators must \`kubectl delete\` the Secret first if they need to roll back. Also refreshed patches/cert-manager-admission-tls.diff to reflect these comment-only edits and re-verified end-to-end that \`make update\`-style pull + patch produces a byte-identical tree to what git tracks now. 15/15 helm-unittest cases still pass. Signed-off-by: Ivan Okhotnikov --- .../admission-controller/certificate.yaml | 11 +- .../admission-controller/deployment.yaml | 78 +++++++++----- .../vertical-pod-autoscaler/values.yaml | 2 +- .../patches/cert-manager-admission-tls.diff | 101 ++++++++++++------ 4 files changed, 129 insertions(+), 63 deletions(-) diff --git a/packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml b/packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml index 5eace3dfb..883ab5593 100644 --- a/packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml +++ b/packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml @@ -90,7 +90,12 @@ spec: issuerRef: name: {{ include "vertical-pod-autoscaler.admissionController.fullname" . }}-tls-issuer kind: Issuer - secretTemplate: - labels: - {{- include "vertical-pod-autoscaler.admissionController.labels" . | nindent 6 }} +{{- /* + Intentionally NO secretTemplate. Copying the chart's shared labels onto + the generated Secret (which includes `app.kubernetes.io/managed-by: Helm` + from the labels include) would mislead operators — Helm does not own + this Secret, cert-manager does. Let cert-manager own the metadata it + writes (`cert-manager.io/*`, `controller.cert-manager.io/*`) and leave + it at that. +*/}} {{- end }} diff --git a/packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml b/packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml index 076e59d9b..302da0345 100644 --- a/packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml +++ b/packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml @@ -47,36 +47,64 @@ spec: checksum/tls-secret: {{ include (print $.Template.BasePath "/admission-controller/tls-secret.yaml") . | sha256sum }} {{- end }} {{- /* - Under cert-manager we need pod rotation ONLY when the Secret content - actually changes — never on unrelated reconciles. `checksum/tls-cert` - hashes the runtime Secret's `tls.crt` (via `lookup`), so: + Under cert-manager we need pod rotation ONLY when the Secret + content actually changes. `checksum/tls-cert` hashes the runtime + Secret's `tls.crt` (via `lookup`), so: - * fresh install: Secret does not exist yet at first render; - this annotation is absent, pod comes up on - whichever Secret cert-manager writes next. - * steady state: Secret content stable → sha stable → no - rotation on any Flux reconcile. - * cert-manager adopt: cert-manager rewrites Secret once - (issuer changed on upgrade) → sha flips → - pod restarts ONCE and picks up the new - CA + cert. - * renewal (~350d): cert-manager rotates Secret → sha flips → - pod restarts ONCE. + * fresh install: Secret does not exist yet at first + render; the annotation is absent, pod + comes up on whichever Secret cert- + manager writes next. + * cert-manager adopt: cert-manager rewrites the Secret once + (issuer changed on upgrade) → sha + flips → pod restarts ONCE and picks up + the new CA + cert. + * steady state: Secret content stable → sha stable + → no rotation. + * renewal (~350d): cert-manager rotates Secret → sha + flips → pod restarts ONCE **only if a + Helm action fires**; see the trade-off + below. + + `lookup` runs ONLY on Helm actions (install / upgrade / + rollback) — helm-controller does not re-render a deployed + release on interval ticks, so `lookup` does not fire on every + reconcile. Concrete trade-off: + + - Any Helm action (chart bump, values change, dependency + re-apply, manual `flux reconcile hr`) re-templates the + chart, `lookup` sees the current Secret content, the + annotation is re-computed, and — if the Secret has been + rotated since the last render — Kubernetes rolls the pod + on the resulting podTemplate change. + - Between Helm actions the pod holds the certificate it + loaded at startup. If cert-manager renews at day ~350 and + NO Helm action happens in the following ~15 days, the pod + keeps serving with an expired in-memory cert (--reload- + cert alone would make this worse — the mounted cert would + rotate while the pod's self-registered + MutatingWebhookConfiguration caBundle went stale). Given + cozystack's release cadence a Helm action realistically + lands well before day 365, so we accept this. If ever it + stops holding, a small post-renewal Job that annotates + the Deployment podTemplate would close the gap without + re-introducing the ReplicaSet-churn amplifier. `lookup` returns nil during `helm template --dry-run=client` - (offline CI renders) — that's fine, the annotation just doesn't - appear until a live-cluster reconcile fetches the Secret. Flux - HelmController runs via the Helm SDK against the live apiserver, - so `lookup` resolves at every reconcile there. + (offline CI renders / this chart's helm-unittest suite) — that + is intentional; the annotation is only meaningful against a + live cluster where cert-manager has actually written the + Secret. Tests assert the annotation is ABSENT under offline + render, exactly for that reason. Crucially, this replaces the earlier `kubectl rollout restart` - post-upgrade hook. That hook itself was the amplifier this - iteration: every Flux reconcile stamped a fresh - `kubectl.kubernetes.io/restartedAt` timestamp, which mutated the - Deployment podTemplate, which spawned a new ReplicaSet — the - same ReplicaSet-churn pattern in a different disguise. Runtime- - content-based checksum only mutates when the runtime content - actually mutates, so the loop cannot form. + post-upgrade hook. That hook itself was the amplifier of the + previous iteration: every Flux reconcile stamped a fresh + `kubectl.kubernetes.io/restartedAt` timestamp, which mutated + the Deployment podTemplate, which spawned a new ReplicaSet — + the same ReplicaSet-churn pattern in a different disguise. + Runtime-content-based checksum only mutates when the runtime + content actually mutates, so the loop cannot form. */}} {{- if and .Values.admissionController.tls.certManagerEnabled (not .Values.admissionController.tls.existingSecret) }} {{- $secret := lookup "v1" "Secret" (include "common.names.namespace" .) (include "vertical-pod-autoscaler.admissionController.tls.secretName" .) }} diff --git a/packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/values.yaml b/packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/values.yaml index ba202b4c8..3b731ceac 100644 --- a/packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/values.yaml +++ b/packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/values.yaml @@ -323,7 +323,7 @@ admissionController: relabelings: [] tls: - ## @param admissionController.tls.certManagerEnabled Provision the admission-controller webhook TLS via cert-manager Issuer + Certificate. When true, `caCert` / `cert` / `key` below are ignored, no in-chart `genCA` Secret is rendered, and the `checksum/tls-secret` podAnnotation that used to rotate the ReplicaSet on every reconcile is dropped. Requires cert-manager installed in the cluster. See `certificate.yaml` header for rationale. + ## @param admissionController.tls.certManagerEnabled Provision the admission-controller webhook TLS via cert-manager Issuer + Certificate. When true, `caCert` / `cert` / `key` below are ignored, no in-chart `genCA` Secret is rendered, and the `checksum/tls-secret` podAnnotation that used to rotate the ReplicaSet on every reconcile is dropped. Requires cert-manager installed in the cluster. See `certificate.yaml` header for rationale. Note: switching this back to `false` after a cluster has migrated to cert-manager is effectively one-way — cert-manager writes the Secret with `type: kubernetes.io/tls`, and `Secret.type` is immutable, so the legacy Opaque Secret template cannot overwrite it in place. To roll back the operator must first `kubectl delete` the cert-manager-owned Secret. certManagerEnabled: true ## @param admissionController.tls.caCert TLS CA certificate (Generated using the `genCA` function if not set) diff --git a/packages/system/vertical-pod-autoscaler/patches/cert-manager-admission-tls.diff b/packages/system/vertical-pod-autoscaler/patches/cert-manager-admission-tls.diff index 3f9ca927d..8acef3c77 100644 --- a/packages/system/vertical-pod-autoscaler/patches/cert-manager-admission-tls.diff +++ b/packages/system/vertical-pod-autoscaler/patches/cert-manager-admission-tls.diff @@ -1,9 +1,9 @@ diff --git a/charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml b/charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml new file mode 100644 -index 000000000..5eace3dfb +index 000000000..883ab5593 --- /dev/null +++ b/charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml -@@ -0,0 +1,96 @@ +@@ -0,0 +1,101 @@ +{{- /* + cert-manager-managed TLS for the VPA admission-controller webhook. + @@ -96,15 +96,20 @@ index 000000000..5eace3dfb + issuerRef: + name: {{ include "vertical-pod-autoscaler.admissionController.fullname" . }}-tls-issuer + kind: Issuer -+ secretTemplate: -+ labels: -+ {{- include "vertical-pod-autoscaler.admissionController.labels" . | nindent 6 }} ++{{- /* ++ Intentionally NO secretTemplate. Copying the chart's shared labels onto ++ the generated Secret (which includes `app.kubernetes.io/managed-by: Helm` ++ from the labels include) would mislead operators — Helm does not own ++ this Secret, cert-manager does. Let cert-manager own the metadata it ++ writes (`cert-manager.io/*`, `controller.cert-manager.io/*`) and leave ++ it at that. ++*/}} +{{- end }} diff --git a/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml b/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml -index b7a923721..076e59d9b 100644 +index b7a923721..302da0345 100644 --- a/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml +++ b/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml -@@ -30,9 +30,60 @@ spec: +@@ -30,9 +30,88 @@ spec: {{- include "common.tplvalues.render" ( dict "value" .Values.commonLabels "context" $ ) | nindent 8 }} {{- end }} annotations: @@ -126,36 +131,64 @@ index b7a923721..076e59d9b 100644 checksum/tls-secret: {{ include (print $.Template.BasePath "/admission-controller/tls-secret.yaml") . | sha256sum }} {{- end }} + {{- /* -+ Under cert-manager we need pod rotation ONLY when the Secret content -+ actually changes — never on unrelated reconciles. `checksum/tls-cert` -+ hashes the runtime Secret's `tls.crt` (via `lookup`), so: ++ Under cert-manager we need pod rotation ONLY when the Secret ++ content actually changes. `checksum/tls-cert` hashes the runtime ++ Secret's `tls.crt` (via `lookup`), so: + -+ * fresh install: Secret does not exist yet at first render; -+ this annotation is absent, pod comes up on -+ whichever Secret cert-manager writes next. -+ * steady state: Secret content stable → sha stable → no -+ rotation on any Flux reconcile. -+ * cert-manager adopt: cert-manager rewrites Secret once -+ (issuer changed on upgrade) → sha flips → -+ pod restarts ONCE and picks up the new -+ CA + cert. -+ * renewal (~350d): cert-manager rotates Secret → sha flips → -+ pod restarts ONCE. ++ * fresh install: Secret does not exist yet at first ++ render; the annotation is absent, pod ++ comes up on whichever Secret cert- ++ manager writes next. ++ * cert-manager adopt: cert-manager rewrites the Secret once ++ (issuer changed on upgrade) → sha ++ flips → pod restarts ONCE and picks up ++ the new CA + cert. ++ * steady state: Secret content stable → sha stable ++ → no rotation. ++ * renewal (~350d): cert-manager rotates Secret → sha ++ flips → pod restarts ONCE **only if a ++ Helm action fires**; see the trade-off ++ below. ++ ++ `lookup` runs ONLY on Helm actions (install / upgrade / ++ rollback) — helm-controller does not re-render a deployed ++ release on interval ticks, so `lookup` does not fire on every ++ reconcile. Concrete trade-off: ++ ++ - Any Helm action (chart bump, values change, dependency ++ re-apply, manual `flux reconcile hr`) re-templates the ++ chart, `lookup` sees the current Secret content, the ++ annotation is re-computed, and — if the Secret has been ++ rotated since the last render — Kubernetes rolls the pod ++ on the resulting podTemplate change. ++ - Between Helm actions the pod holds the certificate it ++ loaded at startup. If cert-manager renews at day ~350 and ++ NO Helm action happens in the following ~15 days, the pod ++ keeps serving with an expired in-memory cert (--reload- ++ cert alone would make this worse — the mounted cert would ++ rotate while the pod's self-registered ++ MutatingWebhookConfiguration caBundle went stale). Given ++ cozystack's release cadence a Helm action realistically ++ lands well before day 365, so we accept this. If ever it ++ stops holding, a small post-renewal Job that annotates ++ the Deployment podTemplate would close the gap without ++ re-introducing the ReplicaSet-churn amplifier. + + `lookup` returns nil during `helm template --dry-run=client` -+ (offline CI renders) — that's fine, the annotation just doesn't -+ appear until a live-cluster reconcile fetches the Secret. Flux -+ HelmController runs via the Helm SDK against the live apiserver, -+ so `lookup` resolves at every reconcile there. ++ (offline CI renders / this chart's helm-unittest suite) — that ++ is intentional; the annotation is only meaningful against a ++ live cluster where cert-manager has actually written the ++ Secret. Tests assert the annotation is ABSENT under offline ++ render, exactly for that reason. + + Crucially, this replaces the earlier `kubectl rollout restart` -+ post-upgrade hook. That hook itself was the amplifier this -+ iteration: every Flux reconcile stamped a fresh -+ `kubectl.kubernetes.io/restartedAt` timestamp, which mutated the -+ Deployment podTemplate, which spawned a new ReplicaSet — the -+ same ReplicaSet-churn pattern in a different disguise. Runtime- -+ content-based checksum only mutates when the runtime content -+ actually mutates, so the loop cannot form. ++ post-upgrade hook. That hook itself was the amplifier of the ++ previous iteration: every Flux reconcile stamped a fresh ++ `kubectl.kubernetes.io/restartedAt` timestamp, which mutated ++ the Deployment podTemplate, which spawned a new ReplicaSet — ++ the same ReplicaSet-churn pattern in a different disguise. ++ Runtime-content-based checksum only mutates when the runtime ++ content actually mutates, so the loop cannot form. + */}} + {{- if and .Values.admissionController.tls.certManagerEnabled (not .Values.admissionController.tls.existingSecret) }} + {{- $secret := lookup "v1" "Secret" (include "common.names.namespace" .) (include "vertical-pod-autoscaler.admissionController.tls.secretName" .) }} @@ -193,14 +226,14 @@ index f36b4d833..0bab86ed9 100644 {{- end }} +{{- end }} diff --git a/charts/vertical-pod-autoscaler/values.yaml b/charts/vertical-pod-autoscaler/values.yaml -index 7d9ac33be..ba202b4c8 100644 +index 7d9ac33be..3b731ceac 100644 --- a/charts/vertical-pod-autoscaler/values.yaml +++ b/charts/vertical-pod-autoscaler/values.yaml @@ -323,6 +323,9 @@ admissionController: relabelings: [] tls: -+ ## @param admissionController.tls.certManagerEnabled Provision the admission-controller webhook TLS via cert-manager Issuer + Certificate. When true, `caCert` / `cert` / `key` below are ignored, no in-chart `genCA` Secret is rendered, and the `checksum/tls-secret` podAnnotation that used to rotate the ReplicaSet on every reconcile is dropped. Requires cert-manager installed in the cluster. See `certificate.yaml` header for rationale. ++ ## @param admissionController.tls.certManagerEnabled Provision the admission-controller webhook TLS via cert-manager Issuer + Certificate. When true, `caCert` / `cert` / `key` below are ignored, no in-chart `genCA` Secret is rendered, and the `checksum/tls-secret` podAnnotation that used to rotate the ReplicaSet on every reconcile is dropped. Requires cert-manager installed in the cluster. See `certificate.yaml` header for rationale. Note: switching this back to `false` after a cluster has migrated to cert-manager is effectively one-way — cert-manager writes the Secret with `type: kubernetes.io/tls`, and `Secret.type` is immutable, so the legacy Opaque Secret template cannot overwrite it in place. To roll back the operator must first `kubectl delete` the cert-manager-owned Secret. + certManagerEnabled: true + ## @param admissionController.tls.caCert TLS CA certificate (Generated using the `genCA` function if not set)