Skip to content

Commit 79ce69a

Browse files
IvanHuntersIvanHunters
committed
fix(vertical-pod-autoscaler): widen renewBefore + document selfSigned checksum, add existingSecret belt-and-suspenders assertion
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 <[email protected]>
1 parent b407035 commit 79ce69a

4 files changed

Lines changed: 84 additions & 8 deletions

File tree

‎packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,24 @@ spec:
107107
dnsNames:
108108
- {{ printf "vpa-webhook.%s.svc" (include "common.names.namespace" .) }}
109109
duration: 8760h
110-
renewBefore: 360h
110+
{{- /*
111+
renewBefore is intentionally 90 days rather than the more common 15
112+
days. cert-manager rotates the Secret at (duration - renewBefore), but
113+
the running pod holds the old certificate in memory and only picks up
114+
the new one when the ReplicaSet rolls. The ReplicaSet only rolls when a
115+
Helm action fires: helm-controller does not re-render on reconcile
116+
ticks after the release converges, only on install/upgrade/rollback.
117+
With webhook `failurePolicy: Ignore`, an expired cert silently drops
118+
admission traffic instead of failing loudly, so operators only notice
119+
when VPA silently stops mutating pods.
120+
121+
Wider `renewBefore` shifts the "must have a Helm action land after
122+
rotation" window from 15 days to ~90 days, decoupling admission-webhook
123+
liveness from release cadence on clusters that go months without a
124+
chart bump. This does not eliminate the rotate-and-restart requirement
125+
— it just makes it survive realistic release schedules.
126+
*/}}
127+
renewBefore: 2160h
111128
issuerRef:
112129
name: {{ include "vertical-pod-autoscaler.admissionController.fullname" . }}-tls-issuer
113130
kind: Issuer

‎packages/system/vertical-pod-autoscaler/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,18 @@ spec:
105105
the same ReplicaSet-churn pattern in a different disguise.
106106
Runtime-content-based checksum only mutates when the runtime
107107
content actually mutates, so the loop cannot form.
108+
109+
Hashing only `tls.crt` is sufficient because the Issuer is
110+
`selfSigned` (see `certificate.yaml`): the leaf is its own CA,
111+
so `ca.crt == tls.crt` and every rotation re-issues `tls.crt`
112+
regardless of key-rotation policy. If a future change ever
113+
switches the Issuer to a CA-backed one (external CA, ACME,
114+
Vault, etc.), the CA can rotate independently of the leaf and
115+
this checksum MUST be extended to also cover `ca.crt` —
116+
otherwise a CA-only rotation would leave the pod serving with
117+
a stale caBundle in its self-registered
118+
MutatingWebhookConfiguration until the next full leaf
119+
rotation.
108120
*/}}
109121
{{- if and .Values.admissionController.tls.certManagerEnabled (not .Values.admissionController.tls.existingSecret) }}
110122
{{- $secret := lookup "v1" "Secret" (include "common.names.namespace" .) (include "vertical-pod-autoscaler.admissionController.tls.secretName" .) }}

‎packages/system/vertical-pod-autoscaler/patches/cert-manager-admission-tls.diff‎

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
diff --git a/charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml b/charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml
22
new file mode 100644
3-
index 000000000..763bd62b7
3+
index 0000000..ad99d64
44
--- /dev/null
55
+++ b/charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml
6-
@@ -0,0 +1,122 @@
6+
@@ -0,0 +1,139 @@
77
+{{- /*
88
+ cert-manager-managed TLS for the VPA admission-controller webhook.
99
+
@@ -113,7 +113,24 @@ index 000000000..763bd62b7
113113
+ dnsNames:
114114
+ - {{ printf "vpa-webhook.%s.svc" (include "common.names.namespace" .) }}
115115
+ duration: 8760h
116-
+ renewBefore: 360h
116+
+ {{- /*
117+
+ renewBefore is intentionally 90 days rather than the more common 15
118+
+ days. cert-manager rotates the Secret at (duration - renewBefore), but
119+
+ the running pod holds the old certificate in memory and only picks up
120+
+ the new one when the ReplicaSet rolls. The ReplicaSet only rolls when a
121+
+ Helm action fires: helm-controller does not re-render on reconcile
122+
+ ticks after the release converges, only on install/upgrade/rollback.
123+
+ With webhook `failurePolicy: Ignore`, an expired cert silently drops
124+
+ admission traffic instead of failing loudly, so operators only notice
125+
+ when VPA silently stops mutating pods.
126+
+
127+
+ Wider `renewBefore` shifts the "must have a Helm action land after
128+
+ rotation" window from 15 days to ~90 days, decoupling admission-webhook
129+
+ liveness from release cadence on clusters that go months without a
130+
+ chart bump. This does not eliminate the rotate-and-restart requirement
131+
+ — it just makes it survive realistic release schedules.
132+
+ */}}
133+
+ renewBefore: 2160h
117134
+ issuerRef:
118135
+ name: {{ include "vertical-pod-autoscaler.admissionController.fullname" . }}-tls-issuer
119136
+ kind: Issuer
@@ -127,10 +144,10 @@ index 000000000..763bd62b7
127144
+*/}}
128145
+{{- end }}
129146
diff --git a/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml b/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml
130-
index b7a923721..302da0345 100644
147+
index b7a9237..0a2d8ec 100644
131148
--- a/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml
132149
+++ b/charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml
133-
@@ -30,9 +30,88 @@ spec:
150+
@@ -30,9 +30,100 @@ spec:
134151
{{- include "common.tplvalues.render" ( dict "value" .Values.commonLabels "context" $ ) | nindent 8 }}
135152
{{- end }}
136153
annotations:
@@ -210,6 +227,18 @@ index b7a923721..302da0345 100644
210227
+ the same ReplicaSet-churn pattern in a different disguise.
211228
+ Runtime-content-based checksum only mutates when the runtime
212229
+ content actually mutates, so the loop cannot form.
230+
+
231+
+ Hashing only `tls.crt` is sufficient because the Issuer is
232+
+ `selfSigned` (see `certificate.yaml`): the leaf is its own CA,
233+
+ so `ca.crt == tls.crt` and every rotation re-issues `tls.crt`
234+
+ regardless of key-rotation policy. If a future change ever
235+
+ switches the Issuer to a CA-backed one (external CA, ACME,
236+
+ Vault, etc.), the CA can rotate independently of the leaf and
237+
+ this checksum MUST be extended to also cover `ca.crt` —
238+
+ otherwise a CA-only rotation would leave the pod serving with
239+
+ a stale caBundle in its self-registered
240+
+ MutatingWebhookConfiguration until the next full leaf
241+
+ rotation.
213242
+ */}}
214243
+ {{- if and .Values.admissionController.tls.certManagerEnabled (not .Values.admissionController.tls.existingSecret) }}
215244
+ {{- $secret := lookup "v1" "Secret" (include "common.names.namespace" .) (include "vertical-pod-autoscaler.admissionController.tls.secretName" .) }}
@@ -221,7 +250,7 @@ index b7a923721..302da0345 100644
221250
{{- include "common.tplvalues.render" ( dict "value" .Values.admissionController.podAnnotations "context" $ ) | nindent 8 }}
222251
{{- end }}
223252
diff --git a/charts/vertical-pod-autoscaler/templates/admission-controller/tls-secret.yaml b/charts/vertical-pod-autoscaler/templates/admission-controller/tls-secret.yaml
224-
index f36b4d833..0bab86ed9 100644
253+
index f36b4d8..0bab86e 100644
225254
--- a/charts/vertical-pod-autoscaler/templates/admission-controller/tls-secret.yaml
226255
+++ b/charts/vertical-pod-autoscaler/templates/admission-controller/tls-secret.yaml
227256
@@ -1,5 +1,16 @@
@@ -247,7 +276,7 @@ index f36b4d833..0bab86ed9 100644
247276
{{- end }}
248277
+{{- end }}
249278
diff --git a/charts/vertical-pod-autoscaler/values.yaml b/charts/vertical-pod-autoscaler/values.yaml
250-
index 7d9ac33be..3b731ceac 100644
279+
index 7d9ac33..3b731ce 100644
251280
--- a/charts/vertical-pod-autoscaler/values.yaml
252281
+++ b/charts/vertical-pod-autoscaler/values.yaml
253282
@@ -323,6 +323,9 @@ admissionController:

‎packages/system/vertical-pod-autoscaler/tests/vpa_test.yaml‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,24 @@ tests:
181181
- hasDocuments:
182182
count: 0
183183

184+
# Belt-and-suspenders: the templates are structurally gated on
185+
# `existingSecret`, but pinning that the pod-template carries NEITHER
186+
# `checksum/tls-secret` (legacy amplifier) NOR `checksum/tls-cert`
187+
# (cert-manager runtime lookup) locks the contract at the observable
188+
# boundary. If a future refactor accidentally moves either checksum
189+
# outside its `if` gate, this catches it before it hits a customer
190+
# cluster and starts a fresh ReplicaSet-churn incident.
191+
- it: existingSecret emits neither tls-secret nor tls-cert checksum on the admission-controller pod
192+
set:
193+
vertical-pod-autoscaler.admissionController.tls.existingSecret: my-external-tls
194+
vertical-pod-autoscaler.admissionController.tls.certManagerEnabled: true
195+
template: charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml
196+
asserts:
197+
- notExists:
198+
path: spec.template.metadata.annotations["checksum/tls-secret"]
199+
- notExists:
200+
path: spec.template.metadata.annotations["checksum/tls-cert"]
201+
184202
# `checksum/tls-cert` uses a runtime `lookup` on the Secret content —
185203
# `helm template` in a test suite has no cluster, so the Secret is never
186204
# found and the annotation is absent. That's actually the correct

0 commit comments

Comments
 (0)