mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-05 19:47:37 +09:00
IvanHunters
a1c779854b
## Summary
The VPA admission-controller webhook TLS Secret is currently rendered by
an in-chart `genCA` + `genSignedCert` template. Those functions are not
idempotent across `helm template` invocations — every Flux HelmRelease
reconcile produces a fresh CA + cert. A `checksum/tls-secret`
podAnnotation on the admission-controller Deployment then flips its
sha, materialising a new ReplicaSet on every reconcile.
Under load — the E2E install phase where cert-manager / CDI / LINSTOR
cascade races repeatedly refire the VPA HelmRelease — this produces
10+ admission-controller ReplicaSets in minutes and blocks the whole
cozystack-platform HR: the admission-controller pod livelocks in
startup (waits on its own election lease which its previous
ReplicaSet is still holding, hits livenessProbe timeout,
CrashLoopBackOffs), VPA never becomes Ready, PackageSources stay
InProgress, and cozystack-platform aborts at its 15-minute install
timeout.
Fix: replace the source of the TLS Secret with a namespace-scoped
cert-manager `Issuer` + `Certificate`. cert-manager only rewrites the
Secret when the certificate is close to expiry (`renewBefore`), so
reconcile-driven cert churn stops. Pod rotation is delegated to a
runtime-lookup-based `checksum/tls-cert` podAnnotation that only
changes when the Secret's `tls.crt` content actually changes — not on
every render, not on every reconcile.
## What
-
`charts/vertical-pod-autoscaler/templates/admission-controller/certificate.yaml`
(new): namespace-scoped selfSigned Issuer + Certificate. The
Certificate mirrors the legacy `genSignedCert` shape (single DNS SAN
`vpa-webhook.<ns>.svc`, 365d validity, matching Secret name), so
cert-manager on the first upgrade adopts the same Secret name the
legacy path wrote — no rename cascade in the Deployment volume mount.
Rendered only when `admissionController.tls.certManagerEnabled=true`
AND no `existingSecret` is supplied.
-
`charts/vertical-pod-autoscaler/templates/admission-controller/tls-secret.yaml`
(modified): wrapped in an extra `certManagerEnabled=false` guard so
the legacy genCA path remains available as an explicit opt-out.
-
`charts/vertical-pod-autoscaler/templates/admission-controller/deployment.yaml`
(modified): the legacy `checksum/tls-secret` annotation is gated
behind `certManagerEnabled=false`. Under the cert-manager path the
Deployment carries a `checksum/tls-cert` annotation computed from
`lookup`-ing the runtime Secret's `tls.crt` — see the template
comment for the full timing analysis and the day-350 renewal
trade-off it documents.
- `charts/vertical-pod-autoscaler/values.yaml` (modified): new
`admissionController.tls.certManagerEnabled` value defaulting to
`true`. Values-block header documents the switch as effectively
one-way (cert-manager writes `type: kubernetes.io/tls`;
`Secret.type` is immutable, so falling back requires `kubectl
delete` first).
- `patches/cert-manager-admission-tls.diff` (new) +
`Makefile update` extended: the four subchart edits above live as a
single reviewable diff that Makefile re-applies over the freshly
vendored chart on every `make update`, matching the fluxcd-operator
package convention. Prevents a bump from silently reverting the
fix.
- `packages/core/platform/sources/vertical-pod-autoscaler.yaml`
(modified): VPA PackageSource now depends on `cozystack.cert-manager`.
Non-circular: cert-manager depends on `cozystack.networking` +
`cozystack.gateway-api-crds`, neither reaches VPA.
- `tests/vpa_test.yaml` (modified): +11 helm-unittest cases (15 total,
4 pre-existing) covering `certManagerEnabled={true,false}`,
`existingSecret` bypass, disabled admission-controller, and
the offline-render behaviour of the `checksum/tls-cert` lookup.
## Why (evidence)
Snapshot from CI run 28587656430 (this branch, pre-workaround-review):
```
admission-controller ReplicaSets after first-install churn window:
vertical-pod-autoscaler-admission-controller-55d888d6d4
vertical-pod-autoscaler-admission-controller-56f8d4c747
vertical-pod-autoscaler-admission-controller-5775dbc9b7
vertical-pod-autoscaler-admission-controller-585b7c56c9
vertical-pod-autoscaler-admission-controller-6fc44944fb
vertical-pod-autoscaler-admission-controller-74bccf4849
vertical-pod-autoscaler-admission-controller-84df98f98b
vertical-pod-autoscaler-admission-controller-8666dffb85
vertical-pod-autoscaler-admission-controller-86b469457c
vertical-pod-autoscaler-admission-controller-9b979f9d6
→ 10 ReplicaSets, one per Flux reconcile, each with a different
genCA-rooted TLS Secret sha in checksum/tls-secret.
```
## Migration for existing clusters
On upgrade the legacy Opaque Secret disappears from the release
manifest, Helm garbage-collects it, and cert-manager immediately
writes a fresh `kubernetes.io/tls` Secret of the same name via the
new Certificate. `Secret.type` is immutable, so in-place adoption is
impossible — the delete/create pass is the correct path, and the
seconds-long window it produces is safe: the `vpa-webhook-config`
MutatingWebhookConfiguration ships with `failurePolicy: Ignore`
(verified on cozy-dev3), so Pod admissions in that window skip VPA
mutation but never get rejected — no cluster-wide disruption.
The Deployment rolls in the same upgrade because the new
`checksum/tls-cert` annotation differs from anything the pod had
before, so the admission-controller re-registers `vpa-webhook-config`
with the fresh caBundle on the same pass.
## Test plan
- [x] `helm unittest packages/system/vertical-pod-autoscaler` — 15/15
pass.
- [x] `helm template` diff between main and this branch is scoped
exactly to the cert-manager path (new Certificate + Issuer,
new `checksum/tls-cert` annotation, guarded legacy Secret, new
values key, patch file + Makefile step, PackageSource
dependsOn). No unrelated lines change.
- [x] `make update`-style simulation (rm -rf charts && helm pull &&
patch) produces a byte-identical subchart to what git tracks —
the patch stays clean across future upstream bumps until it
genuinely conflicts.
- [ ] Live migration on cozy-dev3: `helm upgrade` VPA release from
pre-fix version → observe cert-manager write the new tls Secret
→ verify `checksum/tls-cert` on the new pod's podTemplate →
verify MutatingWebhookConfiguration `vpa-webhook-config`
caBundle updated in place → confirm no ReplicaSet churn on
subsequent Flux reconciles. Blocking the draft's removal.
- [ ] Full E2E on this branch — verify only 1 admission-controller
ReplicaSet after install, VPA HR reaches Ready inside
`cozystack-platform`'s 15m install timeout, and
`kubernetes-latest.bats` / `kubernetes-previous.bats` no longer
time out on VPA-attributable resource churn.
## Review pass — kvaps' CHANGES_REQUESTED addressed
- ✅ Vendored chart mods now carried as `patches/*.diff` re-applied by
`Makefile update` (matches fluxcd-operator convention).
- ✅ Retired the tls-migrator hook — kvaps correctly showed it was
a no-op: Helm's delete decision is driven by the stored release
manifest not by live labels (`ResourcePolicyAnno` is the only opt-
out), and `Secret.type` is immutable so cert-manager could not have
adopted the legacy Opaque Secret in place anyway. ~200 lines
removed.
- ✅ PR description rewritten to match the actual diff (no more
reference to the removed `tls-rollout.yaml` post-upgrade hook, real
test count, dev3 verification honestly listed as pending).
- ✅ `checksum/tls-cert` timing comment corrected: `lookup` runs on
Helm actions only, day-350 renewal trade-off spelled out.
- ✅ `certManagerEnabled=false` documented as effectively one-way
(Secret.type immutable).
- ✅ Dropped the `secretTemplate` labels — `app.kubernetes.io/managed-
by: Helm` on a cert-manager-owned Secret was misleading.
- ✅ Banner comment indentation restored in `vpa_test.yaml`.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **New Features**
* VPA admission-controller webhook TLS now uses cert-manager by default,
with options to disable cert-manager or supply an existing TLS secret.
* Webhook pod restarts are now tied to cert-manager Secret contents via
a content-based checksum.
* **Bug Fixes**
* Improved webhook TLS provisioning reliability when TLS readiness is
delayed.
* Corrected conditional rendering/rollout behavior to avoid mismatched
TLS ownership.
* **Tests**
* Added rendering-invariant tests covering cert-manager, legacy TLS,
existing-secret bypass, and admission-controller disabled behavior.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->