mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-09 14:02:56 +09:00
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 <ivan.okhotnikov@aenix.io>
This commit is contained in:
4 files changed
+84
-8
No files matched your search
+18
-1
@@ -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
|
||||
|
||||
+12
@@ -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" .) }}
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user