From 79ce69a3b49c087aebabb97a2ac169cf610e983b Mon Sep 17 00:00:00 2001 From: Ivan Okhotnikov Date: Fri, 3 Jul 2026 15:59:07 +0300 Subject: [PATCH] fix(vertical-pod-autoscaler): widen renewBefore + document selfSigned checksum, add existingSecret belt-and-suspenders assertion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the three non-blocking follow-ups on myasnikovdaniil's approval: * renewBefore: 360h → 2160h (15d → 90d). With webhook `failurePolicy: Ignore`, an expired in-memory cert silently drops admission traffic until a Helm action rolls the ReplicaSet. helm-controller does not re-render on reconcile ticks after the release converges, only on install/upgrade/rollback — a 15-day window ties admission-webhook liveness to release cadence. 90 days survives realistic release schedules on customer clusters. Rotate-and-restart requirement is unchanged; the window is widened. * Document on deployment.yaml why hashing only `tls.crt` is safe: the Issuer is `selfSigned`, so `ca.crt == tls.crt` and every rotation re-issues `tls.crt`. If a future refactor ever swaps the Issuer to a CA-backed one (external CA, ACME, Vault), the checksum MUST be extended to also cover `ca.crt` — otherwise a CA-only rotation would leave the self-registered MutatingWebhookConfiguration serving a stale caBundle until the next leaf rotation. * Add a belt-and-suspenders helm-unittest assertion pinning that under `existingSecret` the admission-controller pod-template carries NEITHER `checksum/tls-secret` (legacy amplifier) NOR `checksum/tls-cert` (cert-manager runtime lookup). Templates are already structurally gated; this catches a future refactor that accidentally moves either checksum outside its `if` gate. Regenerated patches/cert-manager-admission-tls.diff against upstream via `make update` flow so the vendored chart edits stay in sync with the patch file (avoids silent revert on the next update). Verified: 16/16 helm-unittest cases pass, `helm lint` clean, `helm template ... certManagerEnabled=true` renders `renewBefore: 2160h`, and re-applying the regenerated patch onto a fresh upstream pull produces a byte-identical chart tree. Signed-off-by: Ivan Okhotnikov --- .../admission-controller/certificate.yaml | 19 +++++++- .../admission-controller/deployment.yaml | 12 ++++++ .../patches/cert-manager-admission-tls.diff | 43 ++++++++++++++++--- .../tests/vpa_test.yaml | 18 ++++++++ 4 files changed, 84 insertions(+), 8 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 763bd62b7..ad99d6403 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 @@ -107,7 +107,24 @@ spec: dnsNames: - {{ printf "vpa-webhook.%s.svc" (include "common.names.namespace" .) }} duration: 8760h - renewBefore: 360h + {{- /* + renewBefore is intentionally 90 days rather than the more common 15 + days. cert-manager rotates the Secret at (duration - renewBefore), but + the running pod holds the old certificate in memory and only picks up + the new one when the ReplicaSet rolls. The ReplicaSet only rolls when a + Helm action fires: helm-controller does not re-render on reconcile + ticks after the release converges, only on install/upgrade/rollback. + With webhook `failurePolicy: Ignore`, an expired cert silently drops + admission traffic instead of failing loudly, so operators only notice + when VPA silently stops mutating pods. + + Wider `renewBefore` shifts the "must have a Helm action land after + rotation" window from 15 days to ~90 days, decoupling admission-webhook + liveness from release cadence on clusters that go months without a + chart bump. This does not eliminate the rotate-and-restart requirement + — it just makes it survive realistic release schedules. + */}} + renewBefore: 2160h issuerRef: name: {{ include "vertical-pod-autoscaler.admissionController.fullname" . }}-tls-issuer kind: Issuer 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 302da0345..0a2d8ecf3 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 @@ -105,6 +105,18 @@ spec: 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. + + Hashing only `tls.crt` is sufficient because the Issuer is + `selfSigned` (see `certificate.yaml`): the leaf is its own CA, + so `ca.crt == tls.crt` and every rotation re-issues `tls.crt` + regardless of key-rotation policy. If a future change ever + switches the Issuer to a CA-backed one (external CA, ACME, + Vault, etc.), the CA can rotate independently of the leaf and + this checksum MUST be extended to also cover `ca.crt` — + otherwise a CA-only rotation would leave the pod serving with + a stale caBundle in its self-registered + MutatingWebhookConfiguration until the next full leaf + rotation. */}} {{- 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/patches/cert-manager-admission-tls.diff b/packages/system/vertical-pod-autoscaler/patches/cert-manager-admission-tls.diff index 1938e7829..1c00b6c47 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..763bd62b7 +index 0000000..ad99d64 --- /dev/null +++ b/charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml -@@ -0,0 +1,122 @@ +@@ -0,0 +1,139 @@ +{{- /* + cert-manager-managed TLS for the VPA admission-controller webhook. + @@ -113,7 +113,24 @@ index 000000000..763bd62b7 + dnsNames: + - {{ printf "vpa-webhook.%s.svc" (include "common.names.namespace" .) }} + duration: 8760h -+ renewBefore: 360h ++ {{- /* ++ renewBefore is intentionally 90 days rather than the more common 15 ++ days. cert-manager rotates the Secret at (duration - renewBefore), but ++ the running pod holds the old certificate in memory and only picks up ++ the new one when the ReplicaSet rolls. The ReplicaSet only rolls when a ++ Helm action fires: helm-controller does not re-render on reconcile ++ ticks after the release converges, only on install/upgrade/rollback. ++ With webhook `failurePolicy: Ignore`, an expired cert silently drops ++ admission traffic instead of failing loudly, so operators only notice ++ when VPA silently stops mutating pods. ++ ++ Wider `renewBefore` shifts the "must have a Helm action land after ++ rotation" window from 15 days to ~90 days, decoupling admission-webhook ++ liveness from release cadence on clusters that go months without a ++ chart bump. This does not eliminate the rotate-and-restart requirement ++ — it just makes it survive realistic release schedules. ++ */}} ++ renewBefore: 2160h + issuerRef: + name: {{ include "vertical-pod-autoscaler.admissionController.fullname" . }}-tls-issuer + kind: Issuer @@ -127,10 +144,10 @@ index 000000000..763bd62b7 +*/}} +{{- 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..302da0345 100644 +index b7a9237..0a2d8ec 100644 --- a/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml +++ b/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml -@@ -30,9 +30,88 @@ spec: +@@ -30,9 +30,100 @@ spec: {{- include "common.tplvalues.render" ( dict "value" .Values.commonLabels "context" $ ) | nindent 8 }} {{- end }} annotations: @@ -210,6 +227,18 @@ index b7a923721..302da0345 100644 + 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. ++ ++ Hashing only `tls.crt` is sufficient because the Issuer is ++ `selfSigned` (see `certificate.yaml`): the leaf is its own CA, ++ so `ca.crt == tls.crt` and every rotation re-issues `tls.crt` ++ regardless of key-rotation policy. If a future change ever ++ switches the Issuer to a CA-backed one (external CA, ACME, ++ Vault, etc.), the CA can rotate independently of the leaf and ++ this checksum MUST be extended to also cover `ca.crt` — ++ otherwise a CA-only rotation would leave the pod serving with ++ a stale caBundle in its self-registered ++ MutatingWebhookConfiguration until the next full leaf ++ rotation. + */}} + {{- 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" .) }} @@ -221,7 +250,7 @@ index b7a923721..302da0345 100644 {{- include "common.tplvalues.render" ( dict "value" .Values.admissionController.podAnnotations "context" $ ) | nindent 8 }} {{- end }} diff --git a/charts/vertical-pod-autoscaler/templates/admission-controller/tls-secret.yaml b/charts/vertical-pod-autoscaler/templates/admission-controller/tls-secret.yaml -index f36b4d833..0bab86ed9 100644 +index f36b4d8..0bab86e 100644 --- a/charts/vertical-pod-autoscaler/templates/admission-controller/tls-secret.yaml +++ b/charts/vertical-pod-autoscaler/templates/admission-controller/tls-secret.yaml @@ -1,5 +1,16 @@ @@ -247,7 +276,7 @@ index f36b4d833..0bab86ed9 100644 {{- end }} +{{- end }} diff --git a/charts/vertical-pod-autoscaler/values.yaml b/charts/vertical-pod-autoscaler/values.yaml -index 7d9ac33be..3b731ceac 100644 +index 7d9ac33..3b731ce 100644 --- a/charts/vertical-pod-autoscaler/values.yaml +++ b/charts/vertical-pod-autoscaler/values.yaml @@ -323,6 +323,9 @@ admissionController: diff --git a/packages/system/vertical-pod-autoscaler/tests/vpa_test.yaml b/packages/system/vertical-pod-autoscaler/tests/vpa_test.yaml index cd383ee7a..ea8c23417 100644 --- a/packages/system/vertical-pod-autoscaler/tests/vpa_test.yaml +++ b/packages/system/vertical-pod-autoscaler/tests/vpa_test.yaml @@ -181,6 +181,24 @@ tests: - hasDocuments: count: 0 + # Belt-and-suspenders: the templates are structurally gated on + # `existingSecret`, but pinning that the pod-template carries NEITHER + # `checksum/tls-secret` (legacy amplifier) NOR `checksum/tls-cert` + # (cert-manager runtime lookup) locks the contract at the observable + # boundary. If a future refactor accidentally moves either checksum + # outside its `if` gate, this catches it before it hits a customer + # cluster and starts a fresh ReplicaSet-churn incident. + - it: existingSecret emits neither tls-secret nor tls-cert checksum on the admission-controller pod + set: + vertical-pod-autoscaler.admissionController.tls.existingSecret: my-external-tls + vertical-pod-autoscaler.admissionController.tls.certManagerEnabled: true + template: charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml + asserts: + - notExists: + path: spec.template.metadata.annotations["checksum/tls-secret"] + - notExists: + path: spec.template.metadata.annotations["checksum/tls-cert"] + # `checksum/tls-cert` uses a runtime `lookup` on the Secret content — # `helm template` in a test suite has no cluster, so the Secret is never # found and the annotation is absent. That's actually the correct