From a344b046c9a06ecb8b7c8b54930b27ad280f2625 Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Tue, 1 Sep 2026 01:33:16 -0500 Subject: [PATCH] =?UTF-8?q?docs:=20research=20findings=20=E2=80=94=20measu?= =?UTF-8?q?red=20Flux=20substitution=20semantics=20+=20ranked=20remediatio?= =?UTF-8?q?n=20plan?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Records three parallel research passes and, importantly, TWO CORRECTIONS. Correction 1 (mine): StrictPostBuildSubstitutions alone is INSUFFICIENT — it fixes nothing about ordering. The actual fix is one line: platform.dependsOn: [flux-system]. Verified against kustomization_controller.go::checkDependencies at our exact version — dependsOn is REVISION-gated, not merely Ready-gated, so a dependent sharing a GitRepository cannot reconcile revision N until the dependency has APPLIED revision N. Correction 2 (a research agent's): the claim that Helm considered the bad release healthy, so remediation never fired, is WRONG — measured on the live cluster, the Helm release secrets show v9 = failed. The machinery did fire. maxUnavailable: 2 of 3 IS confirmed, so the real harm was blast radius, not missed detection. Adds a MEASURED substitution-semantics table, including the two anti-patterns nobody had written down: ${VAR:?msg} renders the MESSAGE as the value, and ${VAR:=default} silently ships the dev value to prod. Both are exempt from strict mode by design — which is exactly why strict would never have caught the gitea/nextcloud dead-guard bug. Also corrects repo doctrine: escaping is TOTAL ($${ARR[0]} is safe); only the unescaped indexed form is fatal. And qualifies an earlier overstatement in this log: kustomize strips structural YAML comments before envsubst, so the hazard is block-scalar bodies, not all comments. Notes Flux 2.4.0 is out of support and that strict became default-on in kustomize-controller v1.9.0 — so the audit is a prerequisite for the upgrade regardless. Ranked plan P0-P5 with what is done, what is ready, and what needs an operator call. --- DECISIONS-LOG.md | 112 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/DECISIONS-LOG.md b/DECISIONS-LOG.md index 1e97184..186e673 100644 --- a/DECISIONS-LOG.md +++ b/DECISIONS-LOG.md @@ -1582,3 +1582,115 @@ A Cilium rolling upgrade on this cluster is not risk-free even when the values a `config` init container resolves the API server through the ClusterIP unless `k8sServiceHost` is set, and the agent that translates that ClusterIP is the one being replaced. Keep `k8sServiceHost`/`k8sServicePort` correct at all times, and roll Cilium one node at a time. + +--- + +## 2026-09-01 — RESEARCH: Flux substitution + HelmRelease best practice. **Measured semantics + ranked plan.** + +Three parallel research passes (substitution/ordering, HelmRelease/CNI, and a full repo audit), +prompted by the incident above. Two of my own earlier conclusions were WRONG and are corrected +here. Everything below marked *measured* was run against flux 2.4.0 / the live cluster; everything +else is cited inference. + +### THE SUBSTITUTION SEMANTICS TABLE (measured — trust this over prose elsewhere) + +| form | behaviour | +|---|---| +| `${DEFINED}` | substituted | +| `${UNDEFINED}` | **silently empty** (non-strict) · fails loudly (strict) | +| `${UNDEFINED:-d}` | **default applied at reconcile time — `--strict` does NOT catch it** | +| `${VAR:?msg}` | renders **the message as the value** — a non-empty WRONG value. ANTI-PATTERN. | +| `${VAR:=192.168.1.50}` | silently ships the dev value to prod. ANTI-PATTERN. | +| `$${VAR}` / `$${VAR:-d}` / `$${ARR[0]}` | literal — **escaping is TOTAL** | +| `$VAR` / `$(cmd)` | untouched, no escaping needed | +| `${...}` / `${a b}` / `${ARR[0]}` unescaped | **hard parse error — fails the whole Kustomization** | + +Flux exempts default-providing operators (`-`, `:-`, `+`, `:+`, `=`, `:=`, `?`, `:?`) from strict +mode by design ("they intentionally handle the unset case"). **That is why strict mode would never +have caught the gitea/nextcloud dead-guard bug**, and why a targeted check was required. + +**Doctrine corrected:** `clusters/base/apps-vms.yaml` claimed an indexed expansion is fatal +"escaped or not". Measured: escaping is total; only the UNESCAPED form is fatal. Fixed in place. + +**Comment nuance:** `kustomize build` strips *structural* YAML comments before envsubst sees them, +so those are usually harmless. But `#` lines INSIDE a block scalar are data and DO reach envsubst, +and a `$`-carrying body comment was observed surviving into built `apps/wave-1`. Do not rely on +stripping — escape in comments too. (An earlier claim in this log that a YAML comment "nearly broke +every merge" was overstated for the structural case; the hazard is real for block-scalar bodies.) + +### CORRECTION 1 (mine) — `StrictPostBuildSubstitutions` alone is INSUFFICIENT + +It fixes nothing about ordering; it converts "render a corrupt value and ship it" into "fail loudly +and keep last-good". **The actual ordering fix is one line**, verified against +`kustomization_controller.go :: checkDependencies` at our exact version (v1.4.0): `dependsOn` is +**revision-gated**, not merely Ready-gated — when the dependency shares a GitRepository, the +dependent cannot reconcile revision N until the dependency has APPLIED revision N. + +`platform` is the only Kustomization in the graph with no `dependsOn`; every other consumer chains +transitively through it. So: + +```yaml +# clusters/base/platform.yaml +spec: + dependsOn: + - name: flux-system # cluster-vars must be applied at THIS revision first +``` + +**Cost, recorded honestly:** the root becomes a gate — a malformed manifest anywhere under +`clusters/dev` now stops the whole graph where today it would not. Fail-closed, consistent with +doctrine, but a real blast-radius change. No deadlock risk: the root runs `wait: false`. + +Rejected alternative: moving `cluster-vars` into the consuming Kustomization is **strictly worse** — +`SubstituteVariables` reads the ConfigMap from the API at build time, *before* the Kustomization +applies its own resources, so it would always read the PREVIOUS value. That converts an occasional +race into a permanent one-reconcile lag. + +### CORRECTION 2 (an agent's) — the safety machinery DID fire + +A research pass concluded that Helm considered the bad release healthy (because +`updateStrategy.rollingUpdate.maxUnavailable: 2` on 3 nodes means `expectedReady = 1`), and +therefore remediation never engaged. **Measured on the live cluster: that is wrong.** The Helm +release secrets show **v9 = `failed`**; Helm's wait DID fail and the release was remediated, which +is why it self-healed. `maxUnavailable: 2 of desired 3` IS confirmed live — so the real harm was +blast radius (2 of 3 agents down at once), not a missed detection. + +### Verified, and worth knowing + +- **Flux 2.4.0 is OUT OF SUPPORT** — the project supports the last three minors; 2.9.x is current. +- **Strict substitution became DEFAULT-ON** in kustomize-controller v1.9.0. So upgrading Flux flips + this behaviour whether or not we opt in — the variable audit is a prerequisite for the upgrade + regardless of today's decision. +- **Cilium's `values.schema.json` permits `k8sServiceHost: ""`** — no `minLength`, no `pattern`. + Helm was never going to reject the bad render. +- **Cilium docs make `k8sServiceHost` REQUIRED under kube-proxy replacement**, precisely because the + environment exports the API ClusterIP and there is no kube-proxy to provision it. +- **The Cilium chart ships zero test templates**, so `spec.test` is useless for it. A post-upgrade + check must live outside Helm. +- `valuesFrom` fails HARD on a missing key (verified in `chartutil/values.go`) unlike postBuild's + silent empty — but only on a MISSING key, not an empty value, and it cannot cross namespaces + (`cluster-vars` is in `flux-system`; the Cilium HelmRelease is in `kube-system`). +- **Do NOT enable `driftDetection.mode: enabled` on a CNI.** It compares the cluster against the + Helm storage manifest — during the incident that manifest WAS the bad render, so drift correction + would have actively re-asserted the broken value. + +### RANKED PLAN + +| P | Action | State | +|---|---|---| +| **P0** | `platform.dependsOn: [flux-system]` — closes the race completely | **PENDING — operator call** (blast-radius change) | +| **P0** | Escape the dead OIDC guards; renderer now refuses the class | ✅ DONE | +| **P1** | Substitution-safety L0 gate, unconditional in `ci/l0.sh` | ✅ DONE — 102 assertions | +| **P1** | Enable `StrictPostBuildSubstitutions=true` | **READY — proven safe**, all 11 paths exit 0 under `flux envsubst --strict`. Sequence AFTER P0. | +| **P2** | `updateStrategy.rollingUpdate.maxUnavailable: 1` on Cilium, as a new **[parity]** field in BOTH the HelmRelease and the Ansible j2 | PENDING — bounds blast radius to one node | +| **P3** | Split Cilium into its own Kustomization (`wait: true`, `interval: 1h`, `timeout: 10m`); `platform` dependsOn it | PENDING — gives `flux suspend kustomization cilium` as an emergency valve | +| **P4** | Upgrade Flux 2.4.0 → 2.9.x | PENDING — out of support; brings strict-by-default | +| **P5** | Cilium MINOR bumps become manual (Renovate PR → human runs the documented pre-flight → merge). Never automerge a CNI minor. | PENDING — process | +| — | Record `${VAR:?}` / `${VAR:=}` as anti-patterns | ✅ DONE (table above) | +| — | *Not recommended*: drift detection on Cilium; `spec.test` on Cilium; `k8sServiceHost: auto` | — | + +### Follow-ups surfaced, not yet actioned +- 6 dead `cluster-vars` knobs; `BASE_DOMAIN` and `CLUSTER_ENV` look like genuine rot. +- `platform/{cilium,longhorn,velero,oauth2-proxy}/values.yaml` reference mirrors carry decorative + `${...}` that can silently drift from the real HelmRelease. +- `platform` Kustomization `timeout: 5m` vs Longhorn HelmRelease `timeout: 15m` — the tier can time + out while a legitimate Longhorn upgrade is still running.