diff --git a/.coverage-baseline b/.coverage-baseline index 641c13bc..427656bc 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -75.5 +75.6 diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index adbfd50b..a6af7b73 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,42 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## Unreleased — a folder kustomize cannot build is now refused (next minor; behavior change) + +The analyzer now **builds** every render root with kustomize, instead of only parsing the +kustomization structurally. A root that fails to build refuses the `GitTarget`, quoting +kustomize's own error. + +This refuses folders that were previously accepted, and all of them were folders no GitOps +controller could deploy: + +- a `resources:` entry that does not resolve (a manifest that was moved or renamed); +- a **diamond** — one render root reaching a shared base through two overlays — which + kustomize rejects outright with *"may not add resource with an already registered id"*; +- a **cycle** — `a` referencing `b` referencing `a`. A cycle has no render root at all + (every directory in it is referenced by another), so it used to be invisible rather than + refused: nothing was built, nothing failed, and the folder passed. It is now built, and + kustomize says *"cycle detected"*; +- an `images:` entry whose `name:` is not a valid **regular expression** — `- name: "ngin["`. + A kustomization `name:` is a regex, not a literal string, and kustomize compiles it without + checking the compile error, so such an entry does not fail the build, it **panics** inside + it. We refuse the folder before the build rather than hand it over. (Note the corollary, + which is not new but is easy to miss: `- name: "ngin."` **matches** `nginx`, because it is + a regex.) + +**Why this is a safety fix, not just strictness.** The override chain, and therefore the +write-fan-in guard, is derived from the render. A root that does not build yields no chain, +so no ambiguity is recorded, so the fan-in precondition never fires — and the operator would +write straight through into a base shared by two render paths, which is the one edit +`fan-in = 1` exists to forbid. Silently accepting an unbuildable folder disarmed the guard +protecting it. + +**Migration** + +- Run `kustomize build` on the folder your `GitTarget` points at. If it fails, the operator + now fails the target with `GitPathAccepted=False` / `UnsupportedContent` instead of writing + into a folder whose render it cannot know. Fix the build. + ## Unreleased — a `digest:` override no longer strips the tag out of your source file (next patch; bug fix) **If any of your kustomizations use `images:` with `digest:`, or `newTag:` on an image diff --git a/docs/design/support-boundary/acceptance-precision.md b/docs/design/support-boundary/acceptance-precision.md new file mode 100644 index 00000000..e3168034 --- /dev/null +++ b/docs/design/support-boundary/acceptance-precision.md @@ -0,0 +1,199 @@ +# Acceptance precision: we refuse too much, and we explain too little + +> **design** — direction-setting; ships no code. Nothing it describes is supported today. +> Captured: 2026-07-14 +> Related: +> [README.md](README.md), +> [support-contract.md](support-contract.md), +> [kustomize-support-boundary.md](kustomize-support-boundary.md), +> [orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md), +> [render-root-scoping.md](render-root-scoping.md), +> [values-file-projection.md](values-file-projection.md) + +Small, independent fixes to the acceptance gate, each cheap, each visible in +[support-today.md](../../../test/fixtures/gitops-layouts/support-today.md) the moment it +lands. One of them is a correctness bug in folders we support **today**. + +They share one theme: **a refusal must name the thing it refuses, and refuse only that.** + +> **Everything below is still open.** One item this document originally carried has since +> landed and is gone from it: **`vars` was not refused**, so a source document containing +> `$(SOME_VAR)` had the substituted value mirrored back over the variable — silent corruption +> in an accepted folder. #229 closed it, and closed it *structurally*: the unsupported set is +> now derived by **reflecting over kustomize's own `Kustomization` struct**, so a field we +> have never heard of refuses the folder instead of being silently tolerated. The +> hand-maintained 17-key deny-list this document was written against no longer exists. That is +> the shape the remaining items should be fixed in: not "add the case we forgot", but "stop +> being able to forget". + +--- + +## 1. A stray file does not degrade the target. It stops it dead. + +Acceptance is all-or-nothing — `Accepted = len(issues) == 0` — and `writeBatch.refusal()` +aborts the **entire flush before a byte is written**. There is no partial mode. So a single +unrecognised file in a `GitTarget` does not mean "that file is skipped". It means the +operator writes **nothing, ever**, for that target. + +The corpus shows what that costs: + +| Fixture | The stray | Collateral | +|---|---|---| +| `repo-per-environment` | three `.gitignore` files | **20 valid manifests**, across all three environment roots | +| `argocd-plain/apps/frontend` | `ci-metadata.yaml` | six valid manifests | +| `argocd-external-helm/platform/cert-manager` | `values.yaml` | an `Application` **and** a valid `ClusterIssuer` | + +And the three strays form a clean spectrum, which is the whole design: + +| File | Does the repo say anything about it? | What it actually is | +|---|---|---| +| `.gitignore` | No. Referenced by nothing. | Git plumbing. Genuinely not desired state. | +| `ci-metadata.yaml` | **Yes — the Application excludes it by name** (`spec.source.directory.exclude`) | CI output. The repo already declares it non-desired-state. | +| `values.yaml` | **Yes — the Application includes it by name** (`helm.valueFiles`) | Load-bearing desired state. Refusing it refuses real config. | + +We refuse all three identically, as `foreign-file` or `non-krm-yaml`, on the grounds that we +cannot name them. Two of the three are named **in the repository, in fields we already +parse**. + +### The escape hatch that is never there + +Every one of these messages ends *"remove it or name it in `.gittargetignore`."* There is +**no `.gittargetignore` anywhere in the corpus** — and the shipped template at +[`internal/git/bootstrapped-repo-template/.gittargetignore`](../../../internal/git/bootstrapped-repo-template/.gittargetignore) +lists `.gitignore` as a commented example. The hatch was designed for exactly this case and +is never present, because it only exists in repos we bootstrapped. Onboarding an existing +repo means hand-writing one before anything works. + +It also cannot rescue the values file: `*.yaml` is on the catastrophic-pattern deny-list, so +the only way to exempt `values.yaml` is to enumerate it by name — a file that is not a +passenger at all, but the configuration itself. + +### The fixes + +**1a. A default inert set.** The category already exists: +[`isRecognizedArtifact`](../../../internal/manifestanalyzer/gittargetignore.go) treats +`README.md`, `.sops.yaml` and `kustomization.yaml` as known-and-never-written. It is simply +too small — three entries, and none of them the file every repository actually has. Add the +files every repository has — `.gitignore`, `.gitattributes`, `.gitkeep`, `LICENSE`, +`CODEOWNERS`. Inert means: *not KRM, never written by us, and referenced by no build +directive.* The last clause is what keeps it honest — this is not a blanket "ignore what we +don't understand", it is "ignore what nothing in the repo depends on". + +**1b. Honour the exclusions the repo already declares.** Argo's +`spec.source.directory.exclude` is, in effect, an in-repo `.gittargetignore` that we do not +read. It fits the closed claim vocabulary in +[orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md) — a claim about a +path, read from a document we already parse, needing no orchestrator code. Reading it turns +`ci-metadata.yaml` from a fatal stray into a declared non-target. + +**1c. A referenced values file is context.** Handled in +[values-file-projection.md](values-file-projection.md) §2. + +**1d. Consider partial acceptance.** The three fixes above shrink the blast radius but do +not remove it — the next unknown file still stops the target. Whether an unmanaged file +should *ever* be able to halt writes to the manifests around it is a bigger question than +this document, but the corpus is a strong argument that the answer is no. + +--- + +## 2. `labels` / `commonLabels` / `annotations` leak into source files + +**This is the correctness bug, and it is live.** These three are classed as benign and +explicitly supported — they are named in `supportedKustomizationFields()`, with a comment +admitting the defect. They inject metadata into every rendered object; mirroring bakes that +metadata into the source file as drift. + +It is the last surviving instance of the general hazard that `vars` was the sharpest case of, +and that [render-root-scoping.md](render-root-scoping.md) §2 states in full: **any transformer +we tolerate but do not invert leaks its output into the source.** `vars` got refused; these +did not, because they looked harmless. + +F1 solved exactly this problem for `images` and `replicas` — `SplitDesiredForOverrides` +subtracts the override's effect from the live object before the diff engine ever sees it — and +the same subtraction is what these transformers need. + +Two ways out, and they should be chosen deliberately rather than left to drift: + +- **subtract** them, F1-style (they are trivially invertible: a known key/value applied to + every object), or +- **refuse** them, and admit that "benign" was a guess. + +Note that this is *not* the same as the GitOps-tool label stripping in `internal/sanitize` — +`commonLabels` are the user's own labels (`app: frontend`), not a tool's stamp, and nothing +strips them. + +--- + +## 3. We accept generated output and refuse its source + +In `rendered-manifests`, the folder `rendered/production/` is **accepted** (`3/3/0`). Its +contents open with: + +```text +# Generated by `kustomize build src/frontend/overlays/production`. DO NOT EDIT. +# Source commit: . Regenerate with src/render.sh. +``` + +Anything we write there is clobbered by the next run of `src/render.sh`. Meanwhile the +authored overlay that produced it is **refused** (on `namePrefix`). The polarity is exactly +backwards: we accept the artifact and refuse the intent. + +The `Generated{path}` claim already exists in the vocabulary +([orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md)), sourced from +`DO NOT EDIT` headers. Nothing emits it. Emitting it would make this a **Refused** folder +with a reason a user can act on, instead of a silent write that loses their edit at the next +CI run. + +--- + +## 4. Name the feature we actually objected to + +`IssueUnsupportedKustomize` says, for every refusal: + +> *"kustomization … uses an unsupported feature +> (generators/patches/components/helm/replacements/transformers/namePrefix/nameSuffix/remote +> bases) or malformed images/replicas overrides…"* + +The user must now diff that list against their file to work out which one we meant — and +since #229 the list in the message is not even the list we check against, because the check is +now a reflection over kustomize's struct and the sentence is a hand-written string. It can +name a feature we no longer refuse, or miss one we do. + +Meanwhile **the feature is already computed, per file.** `parseKustomization` returns the exact +offending keys, and the repo scan already prints them — *"kustomization uses unsupported +feature(s): **patches**"* ([`scan_repo.go`](../../../internal/manifestanalyzer/scan_repo.go)). +The acceptance gate throws that away and substitutes the static sentence. + +Have the operator's message name the features it found. The data is already in hand; this is +a plumbing job, not a design one. + +While there: give each refused feature a **reason and an action**, not just a name. The +reasons differ, and flattening them into one sentence is what makes the boundary feel +arbitrary. + +| Feature | Why it is refused | What the user can do | +|---|---|---| +| `remote-base` | the base is fetched over the network; it has no home in this repo, and we never fetch | vendor the base, or target a folder that owns its resources | +| generators **with** a name hash | the hash couples content to name; the object you edited is replaced by a differently-named one on the next render | set `disableNameSuffixHash: true` | +| `secretGenerator` | policy, not invertibility — plaintext secrets in Git contradict the SOPS stance | use SOPS | +| `replacements` / `vars` | the value is derived from another field; writing it here is overwritten on the next render | edit the source field | +| plugins (`generators`, `transformers`) | arbitrary code — the render is unknowable | — | +| `patches` | we cannot author one, today | see [render-root-scoping.md](render-root-scoping.md) §6 | + +--- + +## 5. Order + +Ranked by value per hour, and all five are independent: + +1. **The default inert set** (§1a) — unblocks three environment roots and 20 manifests in + one fixture, and every real repo has a `.gitignore`. +2. **Per-feature messages** (§4) — the data is already computed; pass it through. +3. **Referenced values files as context** (§1c) — rescues a folder that holds real config. +4. **`Generated{path}`** (§3) — stops us writing into files a script will overwrite. +5. **The transformer leak** (§2) — the live correctness bug, but last, because it needs the + subtract-or-refuse decision made first and neither answer is one line. + +Each one changes `support-today.md`, which is the point: the baseline is descriptive, it +regenerates with `task gitops-layouts-baseline`, and *"when it disagrees with the support +contract, that disagreement is the backlog."* diff --git a/docs/design/support-boundary/generated-repo-map.md b/docs/design/support-boundary/generated-repo-map.md new file mode 100644 index 00000000..ca548828 --- /dev/null +++ b/docs/design/support-boundary/generated-repo-map.md @@ -0,0 +1,321 @@ +# The generated repo map: draw the inverse, and show where it does not exist + +> **design** — direction-setting; ships no code. Nothing it describes is supported today. +> Captured: 2026-07-14 +> Related: +> [README.md](README.md), +> [support-contract.md](support-contract.md), +> [kustomize-support-boundary.md](kustomize-support-boundary.md), +> [render-attribution.md](render-attribution.md), +> [render-root-scoping.md](render-root-scoping.md), +> [acceptance-precision.md](acceptance-precision.md), +> [repo-discovery-and-onboarding-scan.md](repo-discovery-and-onboarding-scan.md) + +Can we generate, from a user's GitOps repo, a diagram that shows how their repo is +actually built — and that explains, visually, why parts of it cannot be reversed? + +Yes. And it is cheaper than it looks, because **we already compute the graph and throw it +away**. But the diagram is only worth shipping if it is honest, and honesty here has a +precise meaning: *never draw an edge you cannot justify*. Kustomize will happily hand you +an edge that means "ran over this object" and let you mislabel it "changed this object". +The whole design problem is the fidelity of the arrows, not the drawing of them. + +## 1. The reframe: the build DAG is the boring half + +Every kustomize visualiser ever written draws the same picture — sources fan into an +overlay, an overlay fans into rendered YAML. Left to right, top to bottom, and it tells +the user nothing they did not already know from their directory listing. + +We are not a build tool. We are a *reverser*. Our user's question is never "what does this +render to"; they can run `kustomize build`. Their question is: + +> I changed the replica count in the cluster. **Which file are you going to write, and +> why can't you write it for this other thing?** + +That is the inverse arrow. So the diagram must be drawn in the direction of the question: +from the live object back to the source. And once you draw it that way, the interesting +content is not the arrows that exist — it is **the arrows that have no inverse**. Those +are exactly the support boundary, and they are exactly what the user is confused about. + +The build DAG is the substrate. The missing inverse edges are the product. + +## 2. Four graphs, three of which we can already draw + +| Tier | Edge | Source of truth | Fidelity | Cost to us | +|---|---|---|---|---| +| **1. Inclusion** | kustomization → resources / bases / components / patches / generators | `kustomization.yaml`, decoded with `kustypes.Kustomization` | **Exact.** Declared by the user. | Free — [`parseKustomizations`](../../../internal/manifestanalyzer/kustomization_parse.go) already runs on every scan. | +| **2. Origin** | source file → rendered object | kustomize's `config.kubernetes.io/origin` annotation | **Exact.** Kustomize stamps the file that produced the object. | Free — [`renderedObject.OriginPath`](../../../internal/manifestanalyzer/kustomize_render.go) is already populated, then discarded. | +| **3. Transformed-by** | kustomization → rendered object | kustomize's `alpha.config.kubernetes.io/transformations` annotation | **Over-approximate.** A superset. See below. | Free — [`renderedObject.TransformedBy`](../../../internal/manifestanalyzer/kustomize_render.go) is already populated, then discarded. | +| **4. Field attribution** | override *entry* → field of an object | — | **Does not exist.** | Requires the dye ([render-attribution.md](render-attribution.md) §3), or a fork. | + +Tier 3 is the trap, and it is worth being exact about why, because the temptation to +mislabel it is enormous. Kustomize runs a transformer and then annotates **every resource +currently in the ResMap**, with no check that the transformer touched any of them +(`api/resmap/reswrangler.go`, `AddTransformerAnnotation` — it iterates `m.rList` +unconditionally; upstream's own `transformerannotation_test.go` shows a `Namespace` object +carrying two `PrefixTransformer` entries despite the prefix transformer excluding +namespaces by fieldspec). So: + +> "Transformer X is in this object's annotation" means **X ran while this object was in +> the map**. It does not mean X changed it. + +A diagram that draws that as `overlays/prod --changed--> Deployment/web` is lying, and it +is lying in the direction that makes the user trust a write we cannot actually perform. +Tier 3 edges must be drawn, but they must be drawn as *may have* — and they must look +different from tier 2 edges at a glance. + +Tier 4 — "the `images:` entry at index 1 supplied `spec.template.spec.containers[0].image`" — +is the edge users most want, and kustomize does not have it at any level of its API. It is +not hidden behind an internal package; it is not computed at all. That is the subject of +[render-attribution.md](render-attribution.md); this doc simply must not pretend otherwise. + +## 3. The legend is the thesis + +Three line styles, and they are not cosmetic — each one is a claim about how much we know. + +```mermaid +flowchart LR + subgraph source["source — what the user edits"] + BK["base/kustomization.yaml"] + BD["base/deployment.yaml
image: web:v1"] + OK["overlays/prod/kustomization.yaml
images: web → newTag v2
replicas: web → 3"] + end + subgraph rendered["rendered — what the cluster gets"] + RD["Deployment/web
image: web:v2
replicas: 3"] + RS["Service/web"] + end + + OK -->|declares| BK + BK -->|declares| BD + BD ==>|origin| RD + BK -.->|may have shaped| RS + OK -.->|may have shaped| RD + OK -.->|may have shaped| RS + + classDef editable fill:#dfd,stroke:#3a3,color:#111 + classDef obj fill:#eef,stroke:#66a,color:#111 + class BK,BD,OK editable + class RD,RS obj +``` + +| Style | Meaning | Guarantee | +|---|---|---| +| `-->` **declares** | inclusion, read straight out of `kustomization.yaml` | exact | +| `==>` **origin** | this file produced this object | exact, from kustomize | +| `-.->` **may have shaped** | this kustomization's transformers ran over this object | **superset** — it may have changed nothing | + +Note what the picture already tells the user, honestly: `Service/web` is dashed from both +kustomizations. It has no `images:` or `replicas:` entry that could possibly apply to it — +and yet kustomize's annotation names them both. The dashed edge is *correct* and the user +can see for themselves that it is weak. That is the diagram doing its job. + +## 4. Now reverse it, and the boundary draws itself + +Same repo, arrows reversed, and the question changed from "what renders" to "where does an +edit land". + +```mermaid +flowchart LR + LIVE["live edit
Deployment/web
image → web:v3"] + + LIVE ==>|"origin⁻¹ — exact"| BD["base/deployment.yaml"] + LIVE -.->|"which entry?
no inverse"| OK["overlays/prod/kustomization.yaml
images: web → newTag v2"] + BD -->|"but the write jail (L1)
forbids writing the base
from a prod edit"| JAIL["refused"] + + classDef editable fill:#dfd,stroke:#3a3,color:#111 + classDef refused fill:#fdd,stroke:#c33,color:#111 + class OK editable + class BD,JAIL refused +``` + +Two different failures, in one picture, and the user can tell them apart: + +- **The `newTag` arrow has no inverse.** Kustomize will not tell us that the `v2` in the + rendered image came from the overlay's `newTag` rather than from the file's own tag. We + can *guess* (and today we guess by re-implementing the transformer in + [`overrides_projection.go`](../../../internal/manifestanalyzer/overrides_projection.go) — + ~400 lines of re-implementation that [render-attribution.md](render-attribution.md) is + trying to delete). The diagram should say *we cannot see this*, not invent an arrow. +- **The origin arrow exists and is exact, and we still refuse it.** Writing + `base/deployment.yaml` because production changed would silently change staging too. + That is [render-root-scoping.md](render-root-scoping.md) §4 / the L1 write jail in + [`plan_flush.go`](../../../internal/git/plan_flush.go): *an edit to an object rendered by + an overlay lands in that overlay, never in the base.* + +These are the two sentences we currently make users discover by having a write refused. A +picture states both before they ever try. + +## 5. The strongest visual we have: an arrow with no tail + +The single clearest "this cannot be reversed" is not a colour or a label. It is a +**missing tail**. + +```mermaid +flowchart LR + subgraph source["source"] + K["apps/web/kustomization.yaml
configMapGenerator: app-config
helmCharts: redis"] + D["apps/web/deployment.yaml"] + end + subgraph rendered["rendered"] + RD["Deployment/web"] + CM["ConfigMap/app-config-g9df72cd5b"] + RH["StatefulSet/redis"] + end + + D ==>|origin| RD + K -->|declares| D + K ==>|"configuredIn — no file"| CM + K ==>|"configuredIn — no file"| RH + + NOTE["no source document exists
to write an edit back into"] + CM --- NOTE + RH --- NOTE + + classDef editable fill:#dfd,stroke:#3a3,color:#111 + classDef refused fill:#fdd,stroke:#c33,color:#111 + classDef obj fill:#eef,stroke:#66a,color:#111 + class K,D editable + class RD obj + class CM,RH,NOTE refused +``` + +This is not a metaphor we are imposing — it is literally what kustomize reports. For a +generated resource, the origin annotation carries **no `path`**; it carries `configuredIn: +kustomization.yaml` and `configuredBy: {apiVersion: builtin, kind: ConfigMapGenerator}`. +Our renderer already records this as an empty `OriginPath` +([`kustomize_render.go`](../../../internal/manifestanalyzer/kustomize_render.go): *"Empty +for a generated resource"*). + +An object with an empty origin path is an object with no file. There is nowhere to write. +The diagram shows the arrow starting at a kustomization stanza instead of a document, and +the user understands the refusal in about two seconds — which is roughly two seconds +faster than any refusal message we could write. + +The same trick covers the whole permanent boundary: `helmCharts`, `configMapGenerator`, +`secretGenerator`, and anything else that manufactures an object rather than including one. + +## 6. The refusal overlay: colouring a graph we already have + +The repo-level map needs no new analysis whatsoever. [`scan_repo.go`](../../../internal/manifestanalyzer/scan_repo.go) +already produces, per candidate subtree, exactly the node attributes a diagram needs — and +already emits them as JSON: + +| Field in `RepoCandidate` | What it becomes in the diagram | +|---|---| +| `Path` | the node | +| `Layout` (`plain`, `kustomize-single`, `kustomize-overlay`, `refused-structural`) | the node's shape/colour | +| `AcceptedByOperator` | green vs red | +| `RefusalReasons[].Code` | the label on the red node — and the two codes must stay distinguishable: `overlay-fan-out-unsupported` is a **"not yet"**, `refused-structural` is the **permanent** boundary. A diagram that paints them the same red destroys the one distinction the discovery scan exists to preserve. | +| `ReadScope` | the dashed edges leaving the subtree — i.e. *this overlay reads a base you did not give us* | +| `Resources.Rendered` vs `.Editable` | the gap, printed on the node. `12 rendered / 0 editable` is the entire overlay problem in five characters. | +| `OverlapsWith` | a conflict edge — two candidates that can never both be adopted | + +```mermaid +flowchart TB + BASE["base/
(not a candidate on its own)"] + PROD["overlays/prod
kustomize-overlay
12 rendered / 0 editable"] + STAG["overlays/staging
kustomize-overlay
12 rendered / 0 editable"] + PLAIN["clusters/dev
plain — accepted
9 rendered / 9 editable"] + HELM["platform/cert-manager
refused-structural
helmCharts"] + + PROD -.->|reads outside its subtree| BASE + STAG -.->|reads outside its subtree| BASE + + classDef editable fill:#dfd,stroke:#3a3,color:#111 + classDef notyet fill:#ffe9c7,stroke:#d89614,color:#111 + classDef refused fill:#fdd,stroke:#c33,color:#111 + class PLAIN editable + class PROD,STAG notyet + class HELM refused + class BASE obj +``` + +Amber for "not yet", red for "never", green for "adopt it now". That is the onboarding +conversation, generated. + +And the same colouring extends to the per-document diagnostics the store already carries — +`kustomize-build-failed` ([`override_chain.go`](../../../internal/manifestanalyzer/override_chain.go)), +`ambiguous-kustomize-overrides` ([`overrides.go`](../../../internal/manifestanalyzer/overrides.go), +the fan-in > 1 case), `duplicate-identity`, `non-editable`. Each is already a +machine-readable reason attached to a path. Each is a node colour and a label. **We are not +building an analyser. We are building a renderer for the analyser we have.** + +## 7. Traps — edges the drawing will beg you to add + +| Tempting edge | Why it is wrong | +|---|---| +| `transformer --changed--> object` | Kustomize's annotation means *ran over*, not *changed* (§2). Every solid arrow here is a promise we cannot keep. | +| `images[1] --> containers[0].image` | Tier 4. Does not exist. The dye ([render-attribution.md](render-attribution.md) §3) is how you would earn it; until then, do not draw it. | +| `patch.yaml --> spec.replicas` | Same class of lie. A strategic-merge patch's *effect* is not reported per field. We know the patch ran; we do not know what it hit. | +| `object --edit-lands-here--> base/deployment.yaml` | Origin is exact, but the write jail forbids it (§4). **Origin is where it came from, not where the edit goes.** Conflating those two is the single most dangerous arrow on the page. | +| `origin ⇒ always a file` | Not always. When a transformer produces a resource that had no origin, kustomize writes the *transformer's* origin into the origin annotation. An origin can name a kustomization, not a document. | +| a diagram of `base/` drawn standalone | `buildMetadata` is honoured **only on the root kustomization**, and it is force-propagated down into bases (a base's own setting is overwritten). So a base rendered on its own and the same base seen through an overlay are two different graphs. Always diagram *a render root*, and say which one. | + +## 8. Where it ships + +Not the operator. This is an onboarding/explanation artifact, so it belongs with the +discovery work in the `manifest-analyzer` CLI — beside +[`RenderText` / `RenderJSON` / `RenderScanText`](../../../internal/manifestanalyzer/render.go), +as one more output format over data structures that already exist: + +``` +manifest-analyzer scan --repo . --format mermaid # tier 1 repo map (§6) +manifest-analyzer explain --root overlays/prod --format mermaid # tiers 1-3 (§3) +manifest-analyzer explain --object Deployment/web --format mermaid # the inverse (§4) +``` + +Three zoom levels, because **mermaid does not scale** and pretending otherwise wastes the +feature. A 200-app monorepo has thousands of objects; a single graph of it is an +unreadable hairball that no browser will lay out. The repo map is tens of nodes (one per +candidate). A render-root map is tens to low hundreds. An object's ancestry is a handful. +There is no fourth level, and "the whole repo's objects" is not a diagram, it is a denial +of service. + +Two implementation notes that are easy to get wrong: + +- **Node labels are user data.** Paths, resource names and kustomization stanzas come from + the scanned repo and land in a client-rendered diagram. Sanitise IDs (index or hash them; + never interpolate a path into a mermaid node id) and escape label text. A path containing + a quote or a bracket must not be able to break — or extend — the diagram. +- **The render-root map needs one retention change.** Today + [`renderChains`](../../../internal/manifestanalyzer/override_chain.go) keeps only the + override assignments and drops the `renderedObject`s. The graph needs those objects kept. + That is the same change [render-attribution.md](render-attribution.md) §7 step 1 already + proposes for the oracle — so the diagram rides on it for free rather than justifying it + alone. + +## 9. Order of work + +1. **Repo map from `RepoCandidate`** — pure rendering over an existing, already-JSON + struct. No new analysis, no dependency on the render-attribution work. This is the piece + that is worth doing *now*, and it is the one users see first. +2. **Render-root map** — after the `renderedObject` retention change lands for the oracle. + Tiers 1–3, with the three-line-style legend. +3. **Object ancestry / the inverse view** — the §4 picture. This one is worth waiting for, + because it should be generated from the *real* attribution (the dye) rather than from + our re-implementation's guess. A diagram sourced from + [`simulateImageRender`](../../../internal/manifestanalyzer/overrides_projection.go) would + render our own bugs as if they were kustomize's behaviour. +4. **Refusal colouring everywhere** — fold `DiagReason` and `RefusalReason` into every + level. Cheap once the nodes exist. + +## Still open + +- **Do the docs get generated diagrams too?** The corpus fixtures are real repos. A CI step + that regenerates a mermaid per fixture would make the support boundary *visibly* + regress-tested — a diff in the picture is a diff in the boundary. Tempting; it also makes + every fixture change a diagram review. +- **Where does the user see it?** A CLI that prints mermaid is useful to us and to a + motivated user pasting into a viewer. It is not yet a product surface. If the answer is + eventually "in the UI", the graph should be emitted as data (nodes + typed edges + the + fidelity of each edge) and rendered client-side — mermaid then becomes one renderer, not + the format. +- **Should tier 3 be shown at all at the object level?** The dashed "may have shaped" edge + is honest, but on a real overlay it connects nearly every kustomization to nearly every + object, and a graph where everything is dashed-connected to everything teaches nothing. + Possibly it should only appear when the user asks about a specific object (§4), where the + fan-out is bounded and the ambiguity is the point. +- **Upstream.** A `BuildObserver`-style hook around the generator/transformer loop would + give exact per-field provenance and collapse tiers 3 and 4 into one exact tier. That is + worth *proposing* upstream ([render-attribution.md](render-attribution.md) §4 argues the + fork is not), but nothing here should be blocked on it. diff --git a/docs/design/support-boundary/patching-kustomize.md b/docs/design/support-boundary/patching-kustomize.md new file mode 100644 index 00000000..8f96ef30 --- /dev/null +++ b/docs/design/support-boundary/patching-kustomize.md @@ -0,0 +1,262 @@ +# Patching kustomize: there is no seam, and there are thirty lines that would be one + +> **design** — direction-setting; ships no code. Nothing it describes is supported today. +> Captured: 2026-07-14 +> Related: +> [render-attribution.md](render-attribution.md), +> [generated-repo-map.md](generated-repo-map.md), +> [render-root-scoping.md](render-root-scoping.md), +> [kustomize-support-boundary.md](kustomize-support-boundary.md), +> [support-contract.md](support-contract.md) + +> **This doc revises [render-attribution.md](render-attribution.md) §4.** That section rules +> out "get the DAG out of kustomize" on the grounds that it means a fork, and that a fork +> means re-implementation. The first half is right. **The second half is wrong**, and the +> measurement below is why: the change is ~30 lines in one file, it cannot alter rendered +> output, and it yields the field-level attribution the dye was invented to approximate. + +Three routes to "make kustomize tell us more": **(a)** upstream it, **(b)** carry a patch, +**(c)** stay outside and be clever. This doc measures all three against the checkout in +`external-sources/kustomize`. + +## 1. Route (c) is dead. There is no seam. + +Not "awkward" — absent. Three independent walls, any one of which is fatal: + +| Hoped-for seam | What the code says | +|---|---| +| A hook on `krusty.Options` | The struct has **exactly four fields**: `Reorder`, `AddManagedbyLabel`, `LoadRestrictions`, `PluginConfig` (`api/krusty/options.go:22-47`). No callback, observer, or listener. `MakeKustomizer` hardcodes its `DepProvider`. | +| Register a Go transformer that wraps the builtins | The builtin list is a **hardcoded slice literal** (`api/internal/target/kusttarget_configplugin.go:69-91`), resolved against factory maps in `api/internal/plugins/builtinhelpers`. `api/internal/...` is unreachable from our module by Go's internal rule. There is no `RegisterTransformer` of any kind; `PluginConfig` has four fields and none is a registry. | +| A custom plugin that *observes* the build | Custom transformers are appended **after** every builtin, as peers in one flat slice (`kusttarget.go:330-345`). A plugin never sees a pre-builtin state and cannot intercept one. | + +So the choice is genuinely binary: **patch kustomize, or accept resource-level "ran over" +semantics forever.** Everything clever we could do from outside — the dye +([render-attribution.md](render-attribution.md) §3), leave-one-out probing, N+1 builds — is +a workaround for this absence, not an alternative to it. + +## 2. The one seam that would work is already half-built + +Every builtin transformer runs through one loop, `multiTransformer.Transform` +(`api/internal/target/multitransformer.go:27-41`): + +```go +for _, t := range o.transformers { + if err := t.Transform(m); err != nil { return err } + if t.Origin != nil { + if err := m.AddTransformerAnnotation(t.Origin); err != nil { return err } + } + m.DropEmpties() +} +``` + +`AddTransformerAnnotation` then walks **every resource in the map, unconditionally** +(`api/resmap/reswrangler.go:526-550`). That single unconditional loop is precisely where +"ran over" gets baked in — and it has **exactly one production call site**, the line above. + +Everything needed to do better already exists and is already used this way elsewhere: + +- `ResMap.DeepCopy()` (`api/resmap/reswrangler.go:377`) — and `kusttarget.go:385-390` + **already deep-copies the ResMap before and after running a validator and compares it.** + The pattern we want is upstream's own pattern, applied one loop over. +- `resource.AsYAML()` (`api/resource/resource.go:382`) for content equality; `ResMap.GetById()` + (`reswrangler.go:214`) matches across renames via `PrevIds()`. +- The whole cost is gated on `t.Origin != nil`, and origins are only constructed when + `len(BuildMetadata) != 0` (`kusttarget.go:131-135`). **A default `kustomize build` pays + literally zero.** + +Snapshot before, diff after, annotate only what changed: ~30 added lines, one file, no +interface change. And `multitransformer.go` has been touched **seven times in its life, +most recently in January 2022** — a four-year-stable file. + +## 3. The decisive fact: one transformer instance per entry + +This is the finding that changes the shape of the argument, and it is easy to miss. + +Kustomize does **not** build one `ImageTagTransformer` holding all your images. It builds +**one per `images:` entry**, in file order (`kusttarget_configplugin.go:412-429`): + +```go +for _, args := range kt.kustomization.Images { + c.ImageTag = args + p := f() + ... + result = append(result, p) // one transformer instance per entry +} +``` + +And it is the rule, not an images-only quirk: + +| Stanza | Instances | Cite | +|---|---|---| +| `images:` | **one per entry** | `kusttarget_configplugin.go:419` | +| `replicas:` | **one per entry** | `:455` | +| `patches:` | **one per entry** | `:270-278` | +| `labels:` | **one per entry** | `:294` | +| `replacements:` | *one instance for all* — the exception | `:439-441` | + +Therefore the before/after diff in §2 is **not merely "did this transformer change +anything."** Because the loop iterates *entries*, a structural field-path diff inside it +yields, exactly: + +> `overlays/prod/kustomization.yaml` → `images[1]` → set +> `Deployment/web` `spec.template.spec.containers[0].image` → `web:v2` + +That is **tier 4** — the field-level attribution that +[generated-repo-map.md](generated-repo-map.md) §2 records as *"does not exist"* and that +[render-attribution.md](render-attribution.md) is entirely about approximating. It does not +exist in kustomize's **output**. It is one structural diff away from existing in kustomize's +**execution**. Upstream never exposed it because it never needed it: `transformerOrigin` is +built once per transformer *type* and shared across every instance +(`kusttarget_configplugin.go:88-101`), so the annotation is structurally incapable of +naming an entry even in principle. The information is there at runtime; only the reporting +throws it away. + +## 4. What this does to the dye + +The dye is a good idea born of a false constraint. Compare honestly: + +| | The dye (render-attribution §3) | The patched loop | +|---|---|---| +| Attribution | inferred from a nonce surviving into the output | **observed directly** | +| `newTag`, `digest`, `replicas` | works | works | +| `newName` | **cannot work** — it is the join key, not a sink | works | +| `patches:` | no | **works** (one instance per patch) | +| Charset constraints | **a correctness requirement** — the nonce must survive a regex over the whole image string | none | +| Builds per render | 2 | 1 | +| Failure mode | silent mis-attribution if a nonce collides or is rewritten | none of that class | + +The dye's central caveat — *sound only for pure sinks* — is a consequence of standing +outside the process and inferring. From inside the loop there are no pure-sink +restrictions, because nothing is being inferred. + +**But be precise about what this buys, because it is easy to overclaim: attribution is +necessary for reversal, not sufficient.** Knowing that `patches[0]` set `spec.replicas: 3` +tells us where the value came from. It does not tell us how to *edit the patch* so it +produces `5` — for a scalar set that is mechanical, for a strategic merge with list +semantics it is not. So this does not make `patches:` supported. It removes the reason we +cannot even *see* what a patch did, which is the first of several locks on that door. The +oracle in [render-root-scoping.md](render-root-scoping.md) §3 — re-render and require the +proposal to reproduce the live object exactly — remains the gate, and remains necessary. + +## 5. Fork risk: this is an observability fork, not a semantics fork + +The standing objection to forking kustomize is exactly right in general and does not apply +here, and the distinction is worth naming precisely, because it is the whole argument. + +**Our correctness contract is: render what the user's controller renders.** A fork that +drifts from that is not merely a maintenance cost, it is a correctness hazard — we would +propose writes against a render nobody actually deploys. + +The measured skew today is **zero**: + +| | kustomize `api` | how | +|---|---|---| +| Us | **v0.21.1** | `go.mod:33-34` | +| Flux | **v0.21.1** | `fluxcd/pkg/kustomize@v1.32.0` pins *and `replace`s* it — the controller links `krusty` with `LoadRestrictionsNone` + `DisabledPluginConfig()`, **byte-identical options to ours** | +| Argo (default) | **v0.21.1** | execs the `kustomize` **binary**, shipped at 5.8.1, which pins api v0.21.1 | + +Now the key property: **the patch is structurally incapable of changing rendered content.** + +- It only writes **annotations** — and only `config.kubernetes.io/origin` / + `alpha.config.kubernetes.io/transformations`, which our renderer already **strips** before + anything reaches Git ([`kustomize_render.go`](../../../internal/manifestanalyzer/kustomize_render.go), + `collectRendered`). +- It only runs when `BuildMetadata` is non-empty — a path **upstream users never take** and + **we always take** (we inject it into our in-memory copy of the root). Flux does not set + it; Argo does not set it. +- It touches no transformer, no fieldspec, no merge logic. The object graph is untouched. + +A fork is dangerous when it changes the thing you must match. This one cannot reach it. +Those are different risk classes and should not be priced the same. + +*(Aside, and not caused by this: Argo is the real fidelity problem, and it is unrelated to +forking. It execs a user-swappable binary, at a user-chosen version, and defaults to +`LoadRestrictionsRootOnly` where we and Flux use `LoadRestrictionsNone` — so there exist +repos we render and Argo refuses. That belongs in its own doc.)* + +## 6. The cost that is actually real: `replace` does not compose + +The maintenance cost of the patch is small (one four-year-stable file; rebase when Flux +bumps). The cost that will bite is subtler: + +**A `replace` directive is honoured only in the main module.** It is ignored when our code +is consumed as a library. We *have* a public library — `pkg/manifestanalyzer` — so a third +party importing it would silently link **upstream** kustomize, the patched loop would not +exist, the trace would come back empty, and attribution would **degrade silently to +nothing** rather than fail. + +That failure mode is unacceptable and it is cheap to close: the analyzer must **probe for +the patched build at startup and fail loudly** if the trace hook is absent, rather than +quietly falling back to guesswork. Any design that carries this patch must carry that probe +with it. (This also argues for keeping the dye's *verification* half — +re-render-and-compare — regardless: it is the check that catches exactly this.) + +## 7. Upstream: not hostile, but slow, and the goldens are against us + +The resource-level half of this is genuinely framable as a **bug fix**, not a feature — +upstream's own documentation already describes the semantics we want while the code +implements the other one: + +- `site/content/en/docs/Tasks/build_metadata.md:259` — *"the transformer that **updated** the + resource"* (what we want). +- Same file, `:214` and the proposal — *"transformers that have **acted on** them"* / *"**touched** + each resource"* (what the code does). +- And `:209-212` states the annotation is **alpha**: *"We are not guaranteeing that the + annotation content will be stable during alpha, and reserve the right to make changes."* + +That is a strong opening. The countervailing facts are equally concrete: + +- **The existing goldens assert the current semantics.** `api/krusty/transformerannotation_test.go` + has a `Namespace` object carrying two `PrefixTransformer` entries despite the prefix + transformer excluding namespaces by fieldspec. Our patch deletes those annotations — i.e. + it rewrites tests written by the feature's own author. +- **kustomize is vendored into `kubectl`.** Changing `buildMetadata` output changes + `kubectl kustomize` output, which per `proposals/README.md` pushes toward a full KEP. +- **`CONTRIBUTING.md:216-218`**: a feature PR is not reviewable without a triaged/accepted + issue first. +- **Staffing.** `ROADMAP.md` is titled *"Kustomize roadmap 2023-2024"*, is largely about + understaffing, and says of this very feature area that *"due to limited staffing, we have + been unable to drive this feature out of alpha."* The annotation has been alpha for four + years and nobody has promoted it. +- **The precedent's price tag.** `buildMetadata` itself shipped as a **469-line in-repo + proposal** followed by 3–5 PRs of 500–800 lines each, authored by the project owner. + +Field-level attribution (§3) is a larger ask than the resource-level fix, with **zero +in-repo precedent** — the original proposal scoped itself to resource granularity +deliberately, and nothing about field-level provenance exists in the tree. + +So: propose it, but do not plan around it landing. + +## 8. Recommendation + +1. **Carry the patch** (`replace` directive, ~30–50 lines in `multitransformer.go`), + emitting a per-(entry, resource) changed-field-path trace. It cannot alter rendered + content (§5), and it is the only route to tier 4 (§3). +2. **Ship the loud probe** (§6) in the same change. Silent degradation to no-attribution is + worse than not having the feature. +3. **Keep the oracle regardless** — re-render and require byte-identical reproduction + ([render-root-scoping.md](render-root-scoping.md) §3). Attribution may be observed; + verification must still be independent. A patched loop that we ourselves wrote is not + permitted to be its own witness. +4. **File the upstream issue in parallel**, framed as a bug fix against + `build_metadata.md:259`, with a regression test as a separate first commit per + `CONTRIBUTING.md:226-235`. Treat acceptance as upside. If it lands, our `replace` + evaporates — which is the quiet virtue of an observability-only patch: it is + forward-compatible with its own obsolescence. +5. **Demote the dye** from *the* attribution mechanism to the fallback for an unpatched + build — and keep its verification half permanently. + +## Still open + +- **Does the trace escape the process, or stay a side-channel?** Annotating per-field + provenance onto the resources themselves would bloat output and change what + `RemoveBuildAnnotations` has to strip. A side-channel (`Kustomizer` returning a trace + alongside the `ResMap`) is cleaner for us but is a public API change, which makes the + upstream story harder. These two goals pull in opposite directions and the doc does not + resolve it. +- **`replacements:` stays coarse** (one instance for all — §3 table). We refuse them today, + so it costs nothing now; it would need an index inside the transformer to fix later. +- **Which version do we fork from, and what happens when Flux bumps?** Today the answer is + trivially v0.21.1 because all three of us are there. The first divergence between Flux's + pin and Argo's shipped binary is the moment this question gets a real answer, and we + should decide *then* whether we track Flux or track the user. diff --git a/docs/design/support-boundary/render-attribution.md b/docs/design/support-boundary/render-attribution.md new file mode 100644 index 00000000..e1098679 --- /dev/null +++ b/docs/design/support-boundary/render-attribution.md @@ -0,0 +1,449 @@ +# Render attribution: which override entry supplied this value? + +> **design** — direction-setting; ships no code. Captured: 2026-07-14 +> Related: +> [kustomize-support-boundary.md](kustomize-support-boundary.md) §7 — the decision to embed the renderer · +> [render-root-scoping.md](render-root-scoping.md) — the oracle, and §6's tolerate-don't-author plan · +> [finished/images-and-replicas-edit-through.md](finished/images-and-replicas-edit-through.md) — what shipped · +> [unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md) + +The workstream that replaced our hand-written kustomize with the real one has one step +left, and it is the load-bearing one: **deleting the re-implemented transformers the +write path still uses to decide which file an edit belongs in** — +[`renderImage`, `imageSuppliers`, `simulateImageRender`, `isReplicaKind`](../../../internal/manifestanalyzer/overrides_projection.go), +~400 lines. + +That needs an answer to a question the renderer does not answer. This document states +the approach we had settled on, a second approach that is better, and a third that keeps +getting proposed and cannot work — each **measured against kustomize v0.21.1**, not +argued from its source. + +Everything marked *measured* was run. §6 is the bug ledger the measuring produced; it is +longer than expected, and one of its entries blocks the PR that is currently open. + +--- + +## 1. The question the renderer cannot answer + +Routing an edit needs **attribution**: the live Deployment runs `web:2.0`, and we must +know *who supplied that `2.0`* — the source manifest, or an `images:` entry in one of the +kustomizations above it, and if an entry, **which one**. Only then do we know which file +to write. + +A render does not tell us. `kustomize build` returns `web:2.0`; it never says where the +`2.0` came from. And that gap does not close by looking harder: + +> **kustomize has no field-level provenance, anywhere, at any visibility level.** The +> image filter (`api/filters/imagetag/`) rewrites the field and records nothing about +> having done so. kyaml carries comment-based `SetBy` machinery +> (`kyaml/fieldmeta/fieldmeta.go:29`) and `api` never imports it. A build is a fold over a +> flat `ResMap` — each transformer mutates in place — so there is no value lineage to +> expose, and no API, exported or internal, that maps +> `spec.template.spec.containers[0].image` to the thing that wrote it. + +So attribution cannot be *read out of* kustomize. It can only be *inferred by questioning* +it: perturb an input, render, see what moves. **Every viable design is a query strategy**, +and they differ only in how many questions they ask and how distinguishable the answers +are. That framing is the whole document. + +### What kustomize does give us — and what #232 says about it that isn't true + +`alpha.config.kubernetes.io/transformations` is object-level provenance, and #232 reads +the override chain off it. Right move; but the annotation is weaker than the code's +comments claim, in two measured ways. + +| [`override_chain.go`](../../../internal/manifestanalyzer/override_chain.go) says | kustomize actually does | +|---|---| +| *"kustomize records the transformers that touched each resource"* | It records every transformer that **ran**, on **every object in the build**, modified or not (`api/resmap/reswrangler.go:527-548` loops the whole ResMap with no diff check). A ConfigMap no image transformer could touch still collects `ImageTagTransformer` records. | +| *"attributed per OBJECT, not per file"* | It is attributed **per build**. Every object in one build gets the same records. | + +And it is coarser again. kustomize builds **one `ImageTagTransformer` per `images:` entry** +(`api/internal/target/kusttarget_configplugin.go:405-423`), but all N share one `*Origin` +whose `configuredBy` carries only `{apiVersion: builtin, kind: ImageTagTransformer}` — no +index, no image name, no field — and records are appended without dedupe. Measured: two +`images:` entries stamp **two byte-identical records** on every object. So +[`chainOf`](../../../internal/manifestanalyzer/override_chain.go), which appends *all* of +a file's entries once per record, builds a chain containing every entry **N times**. + +Benign today (re-applying an entry lands on the same result) and not a reason to hold +#232. But it means the annotation answers *"which kustomizations' transformers ran in the +pipeline that produced this object"* — never *"what touched this object"*, and **never +which entry**. The mechanism is sound; the comments describing it are not, and they should +be fixed rather than inherited. + +--- + +## 2. Approach A — differential probing (leave-one-out) + +The settled design: to find a value's supplier, **re-render the root with one entry +removed**; whatever moves is what that entry supplied. Walk the chain last-to-first so the +first hit is the last writer. `1 + N` builds per root. + +Clean, needs no model of kustomize — and **wrong**. Not subtly: wrong on the two most +ordinary configurations in the wild, and measurably *worse than the code it replaces*. + +**The idempotent pin.** A base holding `image: app:v1` under an overlay declaring +`newTag: v1` — the state every repo is in the moment a release lands in both places. + +```text +source=app:v1, entry newTag:v1 PRESENT -> app:v1 +source=app:v1, entry REMOVED -> app:v1 # nothing moved +``` + +Removal moves nothing, so the probe concludes *the source file supplies the tag*. The user +then sets `v2`; we write `v2` into the **base manifest**; the overlay's `newTag: v1` +overrides it straight back on the next render. The edit never lands — and never lands +again, on every reconcile, forever. + +**The tie.** Two entries in the chain declaring the same tag: + +```text +two entries, both newTag:v9 -> app:v9 +entry[1] removed -> app:v9 # nothing moved +``` + +Neither is attributable. Same outcome. + +The cause is structural, not a detail to patch: **removal probes the *value*, and values +collide.** Attribution by "what changed" is blind to a writer whose write is invisible +because something else wrote the same bytes — and "the overlay pins the tag the base +already has" is not a corner case, it is the steady state of a GitOps repo. + +Today's [`renderImage`](../../../internal/manifestanalyzer/overrides_projection.go) gets +both right, because it attributes by *position in the chain* (last matching entry wins) and +never compares values. So Approach A is not merely imperfect — **shipping it would regress +behaviour that works today**, in exchange for deleting the code that makes it work. Worth +saying plainly, because the design was settled before the measurement contradicted it. + +--- + +## 3. Approach B — the dye, and why it is Approach A done right + +Put a **unique, self-identifying value into every override entry**, render once, read the +dyes off the output. Wherever a dye lands, *that entry supplied that field*. + +```yaml +# what the user wrote # what we render, in memory, once +images: images: + - name: app - name: app + newTag: v1 newTag: grdye-0001 + - name: app - name: app + newTag: v1 newTag: grdye-0002 +``` + +Render → `app:grdye-0002` → **entry 2 supplied the tag.** The tie removal cannot see, the +dye reads straight off the output. Measured, on both of §2's failing cases: + +```text +idempotent pin, entry DYED -> app:grdye-0001 # attributed to the entry. correct. +tie, both entries DYED -> app:grdye-0001 (=[1]) # the LAST writer. correct. +``` + +This is not an alternative to differential probing — **it is differential probing with the +flaw removed.** Both perturb an input and watch the output. Removal perturbs an entry into +*absence*, and absence is indistinguishable from "someone else wrote the same value". A dye +perturbs it into a value **nothing else can produce**, so every writer stays +distinguishable — and because the dyes are mutually distinguishable, they can all go in at +once: **2 builds per root, constant in N**, instead of `1 + N`. + +The cost win is real but secondary, and I don't want the design sold on it: N is small and +builds are milliseconds. The dye is right because it is **correct**, not because it is +cheap. + +### Measured: kustomize does not validate the dye. It *matches* on it. + +The obvious objection — "a tag can't hold that" — is half false, and the instinct to insert +*long describing strings* is closer to viable than it deserves to be. There is **no +validation of `newTag` or `digest` anywhere in kustomize**. Measured: a 200-character tag, +a tag containing `/` and `#`, a tag containing `:`, and a digest of `gr-probe-0002` that +isn't even sha-shaped all render straight through, verbatim. + +But **the matcher is a regex over the whole image string**, and that is where the freedom +ends (`api/internal/image/image.go:17`): + +```go +pattern, _ := regexp.Compile("^" + name + "(:[a-zA-Z0-9_.{}-]*)?(@sha256:[a-zA-Z0-9_.{}-]*)?$") +``` + +An out-of-charset dye leaves the image un-matchable, so **every later entry silently stops +firing** — no error, a different render. Measured: + +| dye | renders | next entry (`newTag: FINAL`) | +|---|---|---| +| `newTag: zzprobe-1` | `nginx:zzprobe-1` | ✅ applies | +| `newTag: "zz/probe"` | `nginx:zz/probe` | ❌ **silently skipped** | +| `digest: zzprobe-4` | `nginx@zzprobe-4` | ❌ **silently skipped** | +| `digest: sha256:zzprobe4b` | `nginx@sha256:zzprobe4b` | ✅ applies | + +So the charset is not a style preference, it is a **correctness requirement**, and it is +narrow enough to state once and enforce in one place: + +- **tag** → `[a-zA-Z0-9_.{}-]+`. Still roomy enough to be self-describing: + `grdye.0007.overlays-prod.images-2.newTag` is legal. +- **digest** → `sha256:` + `[a-zA-Z0-9]+`. The prefix is **mandatory** — the regex requires it. +- **replica count** → a reserved integer (`2_000_000_000 + n`). Measured: renders through untouched. +- **`name:` → never dye it.** It is a raw regex (`name: "ngin."` matches `nginx` — measured), + and a malformed one **nil-derefs kustomize** (§6, P1). + +Collisions with real values are excluded by construction and *checkable* for free: we hold +the undyed render too, so "this nonce appears nowhere in the real output" is an assertion, +not a hope. And the dye is applied to the typed `kustypes.Kustomization` — the same struct +[`withBuildMetadata`](../../../internal/manifestanalyzer/kustomize_render.go) already +re-serialises — so YAML quoting is the encoder's problem, not ours. (It has to be: `newTag: y` +is a YAML **boolean**, and kustomize's own `Unmarshal` rejects it. Measured.) + +### The soundness condition, stated exactly + +A dye is a counterfactual, and it is sound exactly when the dyed field is a **pure sink** — +never an input to a matcher. + +| Dyed field | Sink? | Measured | +|---|---|---| +| `newTag` (in charset) | ✅ | dyeing a tag never changes whether a later entry matches | +| `digest` (sha256-prefixed) | ✅ | same | +| `replicas[].count` | ✅ | nothing selects on a count | +| **`newName`** | ❌ | **it is the join key for every later entry** | + +The last one bites, and it is the reason the dye needs a guard rather than a caveat: + +```text +images: [{name: app, newName: renamed}, {name: renamed, newTag: "4.0"}] +undyed -> renamed:4.0 +newName DYED -> grdye-0000:v1 # entry 2 stopped matching. the render changed shape. +tag-only DYED -> renamed:grdye-0001 # correct, even inside a rename chain +``` + +The mitigation needs no model of kustomize's matching, only string equality over fields +kustomize already parsed for us: **dyeing `newName` can only change a matching decision if +some entry's `name:` matches some other entry's `newName:`.** Where no rename chain exists +(essentially every real repo), dye names too. Where one does, don't dye names in that root — +probe them by removal, or refuse. A confound you can *detect* is a confound you can manage. + +### What the dye buys that nothing else does + +It separates **"no entry supplies this field"** from **"something we do not model supplies +this field"**. A `patches:` entry (or a `replacements:` block) that clobbers the image +field *erases the dye*. No dye, and the value differs from source → an unmodelled owner +exists → refuse, don't route. + +Today's code cannot make that distinction. `simulateImageRender` "verifies" the inversion +against a simulation that **shares its own blind spot**, so a value owned by a patch is +confidently written into a file that does not own it. That is invisible only because +`patches:` currently refuses the whole folder — and +[render-root-scoping.md §6](render-root-scoping.md) is a plan to **stop doing that**: +tolerate patches as read-only context, refuse per *field* instead of per folder. Per-field +refusal requires per-field attribution. **The dye is the mechanism that milestone is +waiting for**, which puts it on the critical path rather than beside it. + +And the technique was never about images, so it *can* generalise — to a `vars` value, a +`replacements` source, a generator literal, a scalar inside a patch. But "any knob" is +exactly the overclaim this design must not make. **Each new field costs a sink proof.** +Dyeing is sound only where the dyed value is a pure sink (below), and the fields worth +reaching for next are precisely the ones most likely not to be: a `replacements` source is +read *by* a selector, a generator literal feeds a *name hash*, and a patch's `containers[].name` +is a merge key. Extending the dye to a field means showing that field is a sink and keeping +the baseline-first guardrail (§7) that catches it when the showing is wrong — not assuming +the mechanism travels for free. + +Within that bound it remains the only field-level provenance available for kustomize at any +price, and it costs one build. + +--- + +## 4. Approach C — "just get the DAG out of kustomize" + +Two different graphs hide under this, and separating them settles it. + +**The base/accumulation DAG** (which kustomization includes which) is real, and we should +take whatever kustomize will give us — which is exactly what #232 does. But there is no API +for it: `KustTarget` and `ResAccumulator` are under `api/internal/`, `krusty.Run` returns a +**flat** `ResMap`, and — the part worth internalising — **even internally there is no DAG +object**. `accumulateTarget` merges children depth-first into a flat accumulator; the graph +is *control flow*, never a retained structure. What survives a build is per-resource +annotation: `config.kubernetes.io/origin` (which file) and `configuredIn` (which +kustomization). We reconstruct what we need from those. + +**The dataflow DAG** (field ← entry) is the one we actually want, and it **does not exist +in kustomize at any level**. Not exported, not internal, not computed-and-discarded — +never computed. §1 says why. + +So "get it from the source" reduces, stated honestly, to **fork kustomize and add +provenance**: vendor `internal/builtins` and `internal/image`, thread an origin through the +filters, redo it every bump. That is re-implementation wearing a better hat, and it fails +in precisely the way §7 decided to stop failing — *our fork's semantics are not the +semantics Flux ships*. The reason to embed kustomize was to stop having a second opinion +about what a folder renders to. Forking it re-creates the second opinion and adds a +maintenance bill. + +There is one legitimate version of the idea, and it is upstream: propose field-level +provenance to kustomize (`buildMetadata: [fieldOriginAnnotations]`). `kustomize edit` would +benefit too. Worth an issue; not worth blocking on — and the dye is what we would build in +the meantime regardless. + +```mermaid +flowchart TB + Q["which entry supplied this value?"] + Q --> C["C: read it out of kustomize"] + Q --> A["A: remove an entry,
re-render, see what moved"] + Q --> B["B: dye every entry,
render once, read the dyes"] + C --> Cx["no such data exists.
provenance stops at the object;
forking = re-implementation"] + A --> Ax["1+N builds. blind whenever two
writers agree — which is the
steady state of a repo"] + B --> Bok["2 builds. resolves ties.
detects unmodelled owners.
unsound only on newName —
and that is detectable"] + + classDef bad fill:#fdd,stroke:#c33,color:#111 + classDef good fill:#dfd,stroke:#3a3,color:#111 + class Cx,Ax bad + class Bok good +``` + +--- + +## 5. Attribution may be heuristic. Verification may not. + +None of the above is the safety property, and the difference has to be explicit, because it +is what makes it safe to prefer a cheaper, less-than-total attribution. + +The safety property is the one [render-root-scoping.md §3](render-root-scoping.md) already +names: **apply the proposed entry edits and the projected source document in memory, +re-render, and require the result to reproduce the live object exactly — and to leave every +other object in the build byte-identical.** That check is total, and it does not care how +the proposal was arrived at. + +So attribution's job is only to produce *a candidate good enough to usually pass*. When a +dye's precondition fails, or a rename chain defeats it, we route nothing, and the proposal +falls to what the source document alone can carry — which the re-render then adjudicates. + +It is worth being exact about that, because "we fall back to today's write-through" is the +comfortable phrasing and it is not quite true. Writing a live value into a source document +whose field an **entry governs does not converge**: the entry overrides it on the next +render, which is the same non-convergence §2 convicts leave-one-out of. What makes the +fallback safe is not that write-through is harmless — it is that the verification re-render +*catches* it: the proposed source write does not reproduce the live object, so the flush is +refused instead of landing and quietly doing nothing forever. + +That is what licenses reasoning probabilistically at all, and it is exactly why +`simulateImageRender` must die. It is a verification step that shares the blind spot of the +thing it verifies — it replays *our* chain, not kustomize's, over only the images it planned — +so it cannot catch this, and it converts a wrong attribution into a *confident* wrong write. + +**Replace the simulation with a real re-render first, before anything else changes.** Then +every later deletion in this workstream is protected by a check that cannot share the bug it +is checking for. + +--- + +## 6. The bug ledger: what the probes found + +Every stage of this workstream found a shipped bug by making kustomize the arbiter. Writing +this document found more — none by reading code, all from ~60 lines of throwaway probe +against the existing `imageFixture` harness. **P1 and C1 block #232.** + +| | What | Measured | Effect | +|---|---|---|---| +| **P1** | **`images[].name` is a raw regex, and kustomize discards the compile error** (`pattern, _ := regexp.Compile(...)`, then dereferences it). A malformed name — `- name: "ngin["` — **nil-derefs inside the build**. | `*** PANIC *** invalid memory address or nil pointer dereference` | krusty has no non-test caller on `main`; **#232 is what makes it reachable**, from the store build, on **user-controlled repo content**. controller-runtime recovers reconciler panics by default (v0.24.1), so the operator does not die — it hot-loops: panic → recover → requeue → panic, forever, on a GitTarget nobody can fix except by editing the repo. The `manifest-analyzer` CLI has no such net and simply crashes. **Merge blocker. Also a genuine upstream kustomize bug — worth the PR.** | +| **C1** | **A disconnected cycle has no render root.** For `a → b → a`, every directory is referenced, so `renderRoots` returns nothing, `renderChains` builds nothing and records **no failure**. (Found by CodeRabbit on #232; confirmed here.) | `renderRoots=[]`, `renderChains → 0 chains, 0 failures`. Asked directly, kustomize says `cycle detected` | This is the exact hole #232's own comment says it closes: *"a root that does not build yields no chain, so no ambiguity is recorded, so the write-fan-in guard never fires."* An unbuildable component bypasses the refusal path entirely. **Merge blocker.** | +| **B1** | **Our matcher is string equality; kustomize's is that regex over the full image string.** Two witnesses: an entry whose `name:` carries a tag (`- name: app:v1`) matches in kustomize and not in ours; and `- name: "ngin."` / `- name: ".*"` match in kustomize and not in ours. | `kustomize=nginx:X`, `renderImage=nginx:v1` — **diverge** (all three forms) | We believe the folder renders `app:v1` where it renders `app:X`. The projection reads the difference as a user edit, writes `app:X` into the **source manifest**, and silently kills the `images:` entry (it now matches nothing). Same species as the digest/tag corruption in #231. **Shipped.** | +| **B2** | **ReplicationController.** kustomize's replica fieldspec is `[Deployment, StatefulSet, ReplicaSet, ReplicationController]`; [`isReplicaKind`](../../../internal/manifestanalyzer/overrides_projection.go) lists three of four. | `count: 7` applied to an RC by kustomize; our projection sees no governing entry | A scale on an RC governed by a `replicas:` entry gets written into the source document, where the transformer overrides it. Non-converging drift, silently, forever. **Shipped.** | +| **B3** | **`ephemeralContainers` and OCI volume images.** kustomize's traversal is *any element of any `containers`/`initContainers` sequence, at any depth* ∪ a fieldspec including `spec/volumes[]/image/reference` — it does **not** touch `ephemeralContainers`. [`collectContainerSlots`](../../../internal/manifestanalyzer/overrides_projection.go) collects `ephemeralContainers` (kustomize won't rewrite them) and misses OCI volume images (kustomize will). | rendered: `containers[0].image=app:DYED`, `ephemeralContainers[0].image=app:v1`, `volumes[0].image.reference=app:DYED`; we collect the first two and not the third | Both directions produce a field we mis-attribute. Low blast radius today; it is the *fieldspec* being re-derived, which is the thing we are deleting. | +| **B4** | Rendered objects carry Go **`int`**, not `int64`. `unstructured.NestedInt64(rendered, "spec", "replicas")` returns **`found=false`**. | `value=2000000001 found=false gotype=int` | Not a bug — a landmine directly under step 1 of the plan ("attach the rendered object to each document"). The first code to read a number off a rendered object with the standard helper silently gets zero. | + +**B1, B2 and B3 exist only because `renderImage`, `isReplicaKind` and `collectContainerSlots` +exist.** They are not bugs to fix; they are bugs to *delete*. That is this workstream's +argument, made three more times — and it is a warning about +[`kustomize_render_semantics_test.go`](../../../internal/manifestanalyzer/kustomize_render_semantics_test.go): +eleven hand-picked hard cases, and B1 is not among them. **A table of cases we thought of is +not a substitute for making kustomize the arbiter at runtime.** The probe rows belong in that +table — where they will fail — and then get deleted along with the code they indict. + +One near-miss worth recording: `tagSuffix` looked like another B1, and it is not. +[`imageOverrides`](../../../internal/manifestanalyzer/kustomization_parse.go) refuses any +entry declaring it, on purpose, with a comment saying exactly why. The fence works where +someone thought to build it. (For the record, kustomize applies `tagSuffix` **twice** — +`nginx:v1` → `nginx:v1-debug-debug` — because two filters run per entry. Nobody would have +re-implemented *that* correctly, which is the thesis in one line.) + +--- + +## 7. The shape of the change + +**The primitive** — all three approaches need it; it is the entire new API: + +```go +// renderRootWith builds rootDir with `replace` layered over the scanned files: the +// counterfactual render. Dyed entries, proposed edits, and the verification re-render +// are the same call with a different overlay. +func renderRootWith(files []manifestedit.FileContent, rootDir string, + replace map[string][]byte) ([]renderedObject, error) +``` + +**In order. Each stage is independently shippable, and the risky one is last:** + +0. **Land #232 clean** — P1 and C1 first. Neither is optional: one hot-loops a GitTarget on + a hostile repo, the other silently disarms the write-fan-in guard. Fix the annotation + comments (§1) while there. +1. **`renderRootWith`.** `renderRoot` becomes it with an empty map. +2. **Verification by real re-render** — delete `simulateImageRender`. Attribution still comes + from `renderImage`, so no routing changes; it only makes everything after it safe. *Ship + this alone if the rest slips.* +3. **Attribution by dye** — one dyed render per root at store build; a side table mapping + nonce → (kustomization path, entry index, field); names dyed only when the rename-chain + guard is clear. Delete `renderImage`, `imageSuppliers`, `isReplicaKind` — and with them + B1, B2, and the fieldspec half of B3. +4. **The chain shrinks to a set.** With dye attribution, `KustomizeOverrides` no longer needs + to be an *ordered chain* — order is kustomize's problem now. It needs only the *set* of + entries and where they live, so §1's N-fold duplication stops mattering, and the fan-in + ambiguity test can become the sharper question it was always trying to ask: *do two roots + attribute this field to different entries?* + +**The test net.** The 12 `TestSplitDesired_*` tests construct `(gitRaw, desired, overrides)` +by hand and must be rebuilt **in the same change** that changes the signature. Rebuild them +against real fixtures: build a small in-memory tree, render it with kustomize, drive the +projection with the result. They stop testing our re-derivation and start testing an +inversion against ground truth — the only kind that would have caught B1, #231, or the digest +bug. + +**Guardrails the dye needs** (all measured; see §8 for provenance): + +- **Baseline first, then dye.** If the dyed build errors where the real one didn't, that is + the `replacements`-consumes-the-image class, and it is reliably detectable. **"Fall back" + means fall back to NO ATTRIBUTION — never to another heuristic.** Not to `renderImage`, not + to leave-one-out, not to a guess at the last matching entry. Those are the silent-corruption + paths this design exists to delete, and reaching for one at the moment the renderer says *I + cannot tell you* is the worst possible time to trust them. No attribution means no entry + edits; the proposal is then whatever the source document alone can carry, and it still has + to survive the verification re-render (§5) — which, for a field an entry governs, it will + not. The edit becomes a refused flush. That is the correct outcome and it must be reported, + not absorbed. +- **Only read the dye in the fields you are attributing.** Never grep the whole output: `vars` + and `replacements` can leak a dyed value into `args`, `env`, or ConfigMap data. +- **Never align real↔dyed objects by resource name.** A generator hash suffix can drift + between the two builds. +- **Only dye an entry that already declares the field.** Injecting a `newTag` into a + `newName`-only entry fabricates a supplier that does not exist. + +**Honest costs.** Two builds per render root at store build (was one), one more at flush for +verification. Milliseconds on these trees — but the flush one is on the hot path, and whether +store-build renders can be cached across reconciles is an **open question, not an answered +one**. + +--- + +## 8. Open + +- **A dyed value can escape the field we dyed** — this is measured, not speculative. + `replacements` may take a container image as its **source** (`fieldPath: + …containers.[name=app].image`), and `vars` may `fieldref` it. Consequences seen: a + `delimiter`/`index` on a replacement source **fails the dyed build** where the real build + succeeds (`options.index 2 is out of bounds`); a replacement target pinned to + `[image=nginx:1.21.0]` **errors** under dye; a `vars` fieldref silently carries the dye into + `args`. All three are caught by the baseline-first guardrail, and none is silent — but the + guardrail is load-bearing, not belt-and-braces. +- **Never dye a merge key.** A patch's `containers[].name` is a selector. The sink/selector + distinction (§3) is the rule, and it must be restated for every field the dye is extended to. +- **Version skew** — unchanged and unfixable: we render with the kustomize we pinned; Flux + renders with theirs. +- **P1 is an upstream bug.** `pattern, _ := regexp.Compile(...)` followed by a dereference, in + `api/internal/image/image.go`. Report it; a two-line fix upstream is worth more than our + workaround, and we need the workaround anyway. +- **B1 and B2 are shipped**, and want `fix:` commits plus an [UPGRADING.md](../../UPGRADING.md) + entry, whether they arrive as fixes or as deletions. diff --git a/docs/design/support-boundary/render-root-scoping.md b/docs/design/support-boundary/render-root-scoping.md new file mode 100644 index 00000000..f3de515c --- /dev/null +++ b/docs/design/support-boundary/render-root-scoping.md @@ -0,0 +1,352 @@ +# Render-root scoping: an overlay is a write partition, and the renderer is the proof + +> **design** — direction-setting; ships no code. Nothing it describes is supported today. +> Captured: 2026-07-14 +> Related: +> [README.md](README.md), +> [support-contract.md](support-contract.md), +> [kustomize-support-boundary.md](kustomize-support-boundary.md), +> [gittarget-granularity-and-cross-environment-edits.md](gittarget-granularity-and-cross-environment-edits.md), +> [unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md), +> [orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md), +> [acceptance-precision.md](acceptance-precision.md), +> [finished/images-and-replicas-edit-through.md](finished/images-and-replicas-edit-through.md) + +A `GitTarget` may point at a kustomize overlay. The overlay reads a base through +`../../base`; the operator reads that base as context and writes only inside the overlay. +This is the layout half of Kustomize support, and it is launch-critical. + +It is **not** patch authoring. That stays deferred — see §6, which also explains why the +folder should nonetheless stop being refused for containing a patch. + +--- + +## 1. The problem, stated honestly + +An overlay's live object is `render(base, overlay)`. `render` is lossy and many-to-one: +transformers rename, generate, derive, and drop. **A general inverse does not exist**, and +any design that claims one is lying. + +We do not need one. We need two much weaker things: + +1. a **proposal** — a guess at which file an edit belongs in, and +2. a **decision procedure** — something that tells us whether the guess was right. + +The second is the whole design. It is cheap, total, and already prototyped in the shipped +code: + +> **Propose a source edit, re-render, and compare. If the render does not reproduce the +> live object exactly — and leave every other object in the build byte-identical — refuse +> the write.** + +This is not a new idea imported from outside. F1 shipped the *shape* of it in +[`simulateImageRender`](../../../internal/manifestanalyzer/overrides_projection.go): propose +the entry edits, replay, and discard the whole inversion unless every planned source image +comes back as its live value. + +But be precise about how far short of the statement above that falls, because the gap is the +work: `simulateImageRender` replays **our re-implemented image chain, not kustomize**, and it +checks **only the images it planned** — never that the rest of the build is untouched. So it +is image-specific verification, and it shares a blind spot with the thing it is verifying: a +value owned by a patch, or by a matching rule we got wrong, reproduces perfectly in the +simulation and wrongly in reality. F1 shipped the pattern. **What F1 did not have is a +renderer** — and without one, neither half of the guarantee above is actually in force. See +[render-attribution.md](render-attribution.md) §5. + +--- + +## 2. What we had instead of a renderer + +> **Historical.** When this was written, `sigs.k8s.io/kustomize` was **not a dependency of +> this module** — zero hits in `go.mod` — and what the code called a render was a hand-written +> structural model. That is what the section argues against, and the argument won: +> [kustomize-support-boundary.md](kustomize-support-boundary.md) §7 took the decision, and +> #229/#231/#232 shipped it. `krusty` is now the renderer, `kustomization.yaml` is parsed with +> kustomize's own type, and the resource-DAG walk is gone. **The projection's transformers are +> the last of the re-implementation still standing** — see +> [render-attribution.md](render-attribution.md), which is the design for deleting them. +> +> The inventory below is kept because it is the *evidence*, and because the last two rows are +> still true today. + +| Piece | What it is | Status | +|---|---|---| +| [`renderRoots`](../../../internal/manifestanalyzer/override_chain.go) | every kustomization directory no other kustomization references | **kept** — something must decide which directories a build is invoked on. Everything the walk did *beyond* that now comes from the renderer. | +| [`renderImage`](../../../internal/manifestanalyzer/overrides_projection.go) | a ~20-line reimplementation of kustomize's image transformer | **still there.** Measured to diverge from kustomize: its matcher is string equality where kustomize's is a *regex over the whole image string*. | +| [`isReplicaKind`](../../../internal/manifestanalyzer/overrides_projection.go) | the replica transformer's fieldspec, hardcoded to three kinds | **still there.** kustomize's fieldspec has four — it misses `ReplicationController`. | +| [`unsupportedKustomizeFeatureKeys`](../../../internal/manifestanalyzer/store.go) | 17 keys that refuse the folder outright | **replaced** (#229): the unsupported set is now derived by *reflecting over kustomize's own struct*, so a field we have never heard of refuses rather than being silently tolerated. | + +The deny-list is not a statement about what is editable. **It is a fence around the +reimplementation** — a list of everything we chose not to re-derive. That is why +[images-and-replicas-edit-through.md](finished/images-and-replicas-edit-through.md) says it +plainly: *"No `kustomize build`, no source maps."* + +The fence has a cost we are already paying, and it is not the cost we think: + +- **`vars` is not on the list.** A source document containing `$(SOME_VAR)` renders to a + substituted value. Mirroring that live object writes the *substituted* value over the + `$(VAR)` in the source. That is silent corruption, in a folder we accept **today**. +- **`labels` / `commonLabels` / `annotations` are explicitly classed as benign.** They + inject metadata into every rendered object; mirroring bakes it into the source file as + drift. This is the metadata-transformer leak, live today, in supported folders. + +So the inversion problem is not something overlay support introduces. **We already have it, +and we currently handle it in three inconsistent ways**: explicitly and verified for +`images`/`replicas`; by blanket folder refusal for `patches` and friends; and silently, +incorrectly, for the transformers we called benign. + +A renderer replaces three policies with one. + +--- + +## 3. The oracle + +[kustomize-support-boundary.md](kustomize-support-boundary.md) §7 already sanctions the +move, and names the seat it sits in: + +> *"The worthwhile upgrade is kustomize's Go API (`krusty`) as a **verification oracle, not +> a renderer**: build each render root in-memory and compare against our own projection; +> mismatch → refuse."* + +Adopt it, with the sandbox stated as part of the contract: + +> **This section proposed `LoadRestrictionsRootOnly`. The shipped renderer +> ([`kustomize_render.go`](../../../internal/manifestanalyzer/kustomize_render.go), #231/#232) +> uses `LoadRestrictionsNone`, and the reasoning below is why the proposal was wrong.** The +> contract as built is recorded here rather than quietly corrected. + +| Sandbox setting, as shipped | Consequence | +|---|---| +| `LoadRestrictionsNone` | what **Flux itself** builds with. The in-memory filesystem holds only the scanned tree, so **the filesystem is the jail** — "unrestricted" loading cannot reach the real disk. `RootOnly` would be the wrong kind of strict: it forbids `resources: [../shared.yaml]`, which Flux renders happily, so we would fail to build a root that deploys in production — and failing to build a root silently disarms the write-fan-in guard. Refusing to look is not a safety property. | +| `DisabledPluginConfig` | no exec, no Go plugins — *"arbitrary code = unknowable render"* stays true by construction | +| **pre-build refusal** | remote bases, and `images:` entry names kustomize cannot compile. Both are properties of the *build*, not of what we can model, and both must be caught before krusty is called. | + +**The sandbox does not stop the network, and this was measured rather than assumed.** Given +a remote base, kustomize shells out to `/usr/bin/git fetch` — under +`LoadRestrictionsRootOnly` *and* under an in-memory filesystem. Both were tried; both +fetched. + +So `hasRemoteResource`/`isRemoteResource` are **not** made redundant by the renderer. They +are promoted to a **security precondition that runs before krusty is ever called**, and they +are the one piece of the current kustomize code that must survive. *"We do not run kustomize +on a remote base"* stays literally true — now enforced, rather than merely implied by having +no renderer at all. + +The same slot now also holds a second precondition, found the same way: an `images:` entry's +`name:` is a **regular expression**, and kustomize compiles it while discarding the compile +error before dereferencing it — so `- name: "ngin["` does not fail the build, it **panics +inside it**, on bytes from a user's repository. Refused before the build, with a `recover()` +under krusty for the panics not yet found. See +[render-attribution.md](render-attribution.md) §6. + +The oracle guards two directions: + +```mermaid +flowchart LR + live["live object
(edited via K8s API)"] -->|propose| src["source edit
(overlay-local file)"] + src -->|krusty build| out["rendered object"] + out -->|compare| live + out -->|"compare (must be unchanged)"| rest["every other object
in the build"] + src -.->|mismatch| refuse["refuse the flush
name the file and the field"] + + classDef editable fill:#dfd,stroke:#3a3,color:#111 + classDef refused fill:#fdd,stroke:#c33,color:#111 + class src,out editable + class refuse refused +``` + +The blast-radius half — *every other object in the build is unchanged* — is what makes it +safe to guess. A proposal that would move another environment's object cannot land, so the +proposal function is allowed to be simple. + +### What the oracle does not promise + +We would run *a* kustomize; Flux and Argo run *theirs*. A version skew between our pinned +library and the orchestrator's is a residual risk, and it should be written down rather than +finessed: pin to the version Flux ships, and accept that the guarantee is *"this renders to +what you edited, under the kustomize we pinned."* + +That is strictly better than the guarantee we have now, which is *"this renders to what you +edited, under twenty lines we wrote by hand."* + +--- + +## 4. The proposal function + +It is nearly trivial, and it is forced by decisions already made. + +> **An edit to an object rendered by an overlay lands in that overlay. Never in the base.** + +Not merely unsafe — *wrong*. A user who edits production's replicas did not ask to change +staging. Writing the base would change what another environment renders, and +[gittarget-granularity-and-cross-environment-edits.md](gittarget-granularity-and-cross-environment-edits.md) +already forecloses it four times over: Option A, base read-only by **L1**, the filesystem +guarantee that cannot be wrong. Nothing here opens a base-write path. The only route to a +base write remains Option C (base-as-variant, its own `GitTarget`, its own RBAC role), and +it stays deferred. + +So routing is: *which file **inside this overlay** should carry this field?* — and the +oracle adjudicates. + +### The launch scope + +Unchanged from [kustomize-support-boundary.md](kustomize-support-boundary.md) §2 and §5: + +| Observed change in overlay X | Lands in | Status | +|---|---|---| +| image tag / repository | overlay X's `images:` entry | **Editable** | +| replica count | overlay X's `replicas:` entry | **Editable** | +| new object | new overlay-local file **+ a `resources:` entry** | **Editable** | +| overlay-local document's own fields | that document, in place | **Editable** | +| a base-owned field (env var, limits, args…) | an operator-authored patch | **Refused** — deferred, §6 | +| delete of a base-owned object in one env | a `$patch: delete` directive | **Refused** | + +Two mechanical extensions the current writer does not have, both small and both gated by +the oracle: + +- **Create an entry, not just turn a knob.** [`applyKustomizationEdit`](../../../internal/git/manifestedit/kustomization.go) + today requires the field to already exist as a scalar; it never adds an entry. An overlay + that has no `replicas:` at all cannot receive a replica edit. Adding an entry to an + existing sequence is the same shape as + [`AppendKustomizationResource`](../../../internal/git/manifestedit/kustomization.go), which + already exists. +- **Create the sequence.** Same rule, one level up. Guard both with the oracle: an entry we + add must render to the value the user set, or it does not get written. + +### Read scope grows; write scope does not + +Render-root scoping's one concrete new capability, per §5 of the granularity doc: **follow +`../../base` for reading** (today it is dropped), while the write jail stays at `spec.path`. +That asymmetry *is* L1 — *"reads may reach shared context; writes never leave it"* — and it +is already enforced and tested in +[`pathScopePrecondition`](../../../internal/git/plan_flush.go). + +### L2's blind spot closes here + +[`fanInPrecondition`](../../../internal/git/plan_flush.go) refuses a write to a file reached +by more than one override chain **with override entries at stake** (`anyOverrides`). Its +generalisation — *any file reachable from more than one render root* — is exactly this +workstream's job. With render roots first-class, the check stops leaning on the emergent +side effect that a namespace-ambiguous base document never becomes dirty. + +--- + +## 5. What the corpus says + +Every `refused-structural` row in +[support-today.md](../../../test/fixtures/gitops-layouts/support-today.md) is an overlay, +and the corpus has already sorted them by difficulty for us. + +| Fixture candidate | Refused on | Reading | +|---|---|---| +| `flux-monorepo/apps/{staging,production}` | **`patches` only** | `namespace` + `resources` + one strategic-merge patch on a base Deployment. **No name mutation, stable name, known namespace.** The most tractable overlay in the corpus — and a *category-1 desired-state* layout, the kind we claim to support. | +| `rendered-manifests/src/frontend/overlays/{staging,production}` | **`namePrefix` only** | The overlay dirs contain *nothing but* a kustomization.yaml. `namespace`, `resources`, `images` all pass. Deleting one key is the entire delta between refused and accepted. | +| `flux-helmrelease/apps/frontend` | **`configMapGenerator` only** | With `generatorOptions.disableNameSuffixHash: true`. The output name is deterministic (`frontend-values`); there is no hash, no prefix, no patch. Refused solely because the key is on the list at all. | +| `kustomize-overlays/apps/frontend/overlays/*` | the full zoo | `patches` + generators + `namePrefix`/`nameSuffix` + a hidden `.argocd-source-*.yaml` that silently outranks the `images:` block. Correctly refused. | +| `kustomize-overlays/apps/backend/overlays/production` | `remote-base` | `0/0/0` — literally zero local files. Nothing to render, nothing to edit. Correctly refused — and it must be refused *before* the build, because kustomize would fetch it (§3). | + +Three things the corpus makes visible that no design doc had said out loud: + +1. **We accept the generated artifact and refuse its source.** In `rendered-manifests`, + `rendered/production/` is **accepted** (`3/3/0`) — a file carrying `# DO NOT EDIT`, + regenerated by `src/render.sh`, which will clobber anything we write. Meanwhile the + authored overlay it came from is refused. We have the polarity exactly backwards. The + `Generated{path}` claim already exists in + [orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md)'s vocabulary; + nothing emits it. See [acceptance-precision.md](acceptance-precision.md). +2. **In-repo bases are invisible.** A base referenced by an in-repo overlay is not reported + as a candidate at all — `flux-monorepo/apps/base/frontend`, + `kustomize-overlays/apps/frontend/base` and `rendered-manifests/src/frontend/base` are + all absorbed. Any design that starts rendering overlays must decide what those bases + become. **They become read-only context**, which is the right answer and should be said + explicitly rather than achieved by accident. +3. **No fixture exercises the `kustomize-overlay` layout at all.** Every overlay in the + corpus also uses `patches` or `namePrefix`, so `refused-structural` fires first and + hides the verdict — `overlay-fan-out-unsupported` has never been observed. **A minimal + overlay fixture (`namespace` + `images` over `../../base`, nothing else) is a + prerequisite**, not a nice-to-have: it is the only way to see the code path this + document is about. + +--- + +## 6. Why a patch still blocks the folder — and why it should not + +Patch authoring is deferred, and this document does not un-defer it. Writing a +strategic-merge patch means modelling merge keys, `$patch` directives, and CRD fallback +behaviour, and it is priced against the tier-2 metrics for a reason. + +But **accepting a folder and being able to express every edit in it are two different +questions, and today one deny-list answers both.** + +A `patches:` entry refuses the entire `GitTarget`. Not the edit — the *target*. Acceptance +is all-or-nothing (`Accepted = len(issues) == 0`), and a refusal aborts the whole flush +before a byte is written. So `flux-monorepo/apps/production` — whose patch touches +`spec.replicas`, an env var, and CPU requests — also loses `images:` and `replicas:` +edit-through, which the patch has nothing to do with. + +Separate the gates: + +| Gate | Question | Granularity | +|---|---|---| +| **Renderable** | can we compute the objects this folder produces? | per folder | +| **Routable** | can we place *this* edit in a file, and prove it? | **per (object, field)** | + +With the oracle, a folder containing a hand-written patch is perfectly **renderable**. The +patch is a sparse KRM document; the fields it sets are readable directly from it. So: + +- **Tolerate `patches` as read-only context.** Render it, mirror the result, accept the + folder. +- **Route what we can already route.** An `images:` or `replicas:` edit goes to its entry, + as today — and the oracle *proves* it, including the interesting case where the patch + itself pins `spec.replicas` and the transformer overrides it anyway. We do not have to + reason about kustomize's transformer ordering. We check. +- **Refuse the edits we cannot express**, per field, naming the patch that owns the field + and the fact that authoring one is not supported. + +This changes one decided position — §5's *"pre-existing hand-written patches would still +refuse the folder"* — and it should be argued, not smuggled. The argument: that sentence was +written when the only way to know what a patch does was to model it. With a renderer we do +not model it, we execute it. The reason for the refusal was the fence, and the fence is what +we are removing. + +**Prerequisite, not a follow-on.** None of this may ship before the tier-2 unreflected-edit +accounting in [unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md): +*"overlay support without the unreflected set would reintroduce silent divergence."* A +per-field refusal is only honest if the refused edit is **reported and reverted, never +silently lost**. That is the gate on all of this, and it is the right one. + +--- + +## 7. The order of work + +1. **The minimal-overlay fixture.** Until it exists, the overlay code path is unobserved. +2. **Tier-2 accounting** — `FullyReflected` per edit; refused edits reported and reverted. +3. **The oracle**: krusty, sandboxed, in the acceptance gate and in the write-plan + precondition. Ship it first against `images`/`replicas`, where it must agree with + `simulateImageRender` on every corpus fixture — a free differential test of the thing + we are about to trust. +4. **Render-root scoping proper**: read `../../base`; bases become declared read-only + context; generalise `fanInPrecondition` to any file reachable from more than one render + root. +5. **Entry creation** (`replicas:`/`images:` entries that do not yet exist), oracle-gated. +6. **Tolerate-don't-author**: patches, `namePrefix`/`nameSuffix`, and hash-free generators + become read-only context; refusals move from the folder to the edit. + +Steps 1–4 are the launch unit. Steps 5–6 are what turn `flux-monorepo` and +`rendered-manifests` from refused into supported, and they are worth stating as the goal +because they are what the corpus is asking for. + +## 8. Still open + +- **Does the oracle run in the gate, the write path, or both?** §7 of the kustomize doc says + the gate. The blast-radius check only means something on an actual planned write. Probably + both, with different failure modes: gate → `GitPathAccepted=False`; write → refuse the + flush. +- **Cost per flush.** A build is milliseconds on these trees, but it is not free, and it is + on the hot path. +- **Generators with `disableNameSuffixHash: true`.** `flux-helmrelease/apps/frontend` is the + case: deterministic name, single file input. It is refused as a generator, but it is not + *structurally* non-invertible — the hash is what makes generators non-invertible, and it + is switched off. This is the seam between here and + [values-file-projection.md](values-file-projection.md), which needs the same file. +- **Version skew** between our kustomize and the orchestrator's (§3). diff --git a/docs/design/support-boundary/values-file-projection.md b/docs/design/support-boundary/values-file-projection.md new file mode 100644 index 00000000..e382b692 --- /dev/null +++ b/docs/design/support-boundary/values-file-projection.md @@ -0,0 +1,181 @@ +# The values file: refused for where it sits, not for what it is + +> **design** — direction-setting; ships no code. Nothing it describes is supported today. +> Captured: 2026-07-14 +> Related: +> [README.md](README.md), +> [support-contract.md](support-contract.md), +> [expansion-boundary-and-corpus-organisation.md](expansion-boundary-and-corpus-organisation.md), +> [write-only-encrypted-secrets.md](write-only-encrypted-secrets.md), +> [orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md), +> [acceptance-precision.md](acceptance-precision.md), +> [finished/higher-level-krm-documents.md](finished/higher-level-krm-documents.md) + +[expansion-boundary-and-corpus-organisation.md](expansion-boundary-and-corpus-organisation.md) +names the free-standing Helm values file as *"the single highest-leverage thing available +for the Helm story"* and says it *"deserves a design of its own rather than a decision +here."* This is that design. + +The boundary it must not move: **we edit the intent layer, never the expansion layer.** We +never render `templates/`. We never learn what a value *means*. Everything below treats a +values file as **bytes with a home in Git**, and nothing more. + +--- + +## 1. The finding: the refusal is location-dependent, not content-dependent + +The same Helm values, expressed six ways in the corpus, get five different verdicts: + +| Where the values live | Fixture | Verdict today | +|---|---|---| +| `Application.spec.source.helm.valuesObject` (structured map) | `argocd-external-helm/applications/external-dns.yaml` | **Editable** | +| `Application.spec.source.helm.values` (YAML **as a string**) | `argocd-external-helm/applications/cert-manager.yaml` | **Editable** — as an opaque blob | +| `Application.spec.source.helm.parameters` | `helm-environment-values/argocd/*.yaml` | **Editable** | +| `HelmRelease.spec.values` inline, `valuesFrom` → a hand-written `ConfigMap` | `flux-helmrelease/infrastructure/controllers/ingress-nginx/` | **Editable** | +| a `values.yaml` alone in a `values/` directory | `argocd-external-helm/values/**` | **Not seen at all** — no KRM in the dir, so never a candidate | +| a `values.yaml` **next to** KRM | `argocd-external-helm/platform/cert-manager/values.yaml` | **Refused** — and it takes the folder with it | +| a `values.yaml` wrapped by a `configMapGenerator` | `flux-helmrelease/apps/frontend/` | **Refused** — the generator is on the deny-list | + +Read that column again. Values are editable when a human wraps them in a `ConfigMap`, and +refused when `configMapGenerator` does the identical wrapping — with +`disableNameSuffixHash: true`, so the result is deterministic. They are editable as a *string* +inside an Application, and refused as a *file* on disk. They are invisible when alone and +fatal when adjacent. + +The worst case is worth spelling out. `platform/cert-manager/values.yaml` is: + +- **referenced by name** by the Application in the same folder + (`helm.valueFiles: [$values/platform/cert-manager/values.yaml]`), +- therefore **load-bearing desired state** — it is *the* configuration of cert-manager, and +- refused as `non-krm-yaml: "YAML is not a Kubernetes manifest"`, which **also takes down a + perfectly valid `ClusterIssuer`** sitting beside it, because acceptance is all-or-nothing. + +We refuse the folder on the grounds that we do not know what the file is. The repository is +telling us what it is, in a field we already parse. + +--- + +## 2. Two moves, in order + +### Move 1 (cheap, immediate): a referenced values file is context, not junk + +A values file named by an `Application`'s **`helm.valueFiles`**, or by a `HelmRelease`'s +**`spec.chart.spec.valuesFiles`**, is **Read-only context** — a file we understand, never +write, and never refuse the folder over. + +**Only the path-valued fields, and this distinction is load-bearing**, because the three +spellings are three different surfaces and only one of them is a file at all (both verified +against upstream, not inferred from the names): + +| Field | What it holds | Surface | +|---|---|---| +| Argo `helm.valueFiles`, Flux `spec.chart.spec.valuesFiles` | a **path** — `$values/platform/cert-manager/values.yaml` | **a file in the repo.** This document's subject. | +| Argo `helm.valuesObject` (a `runtime.RawExtension`), Flux `spec.values` | **inline YAML**, embedded in the Application/HelmRelease itself | not a file. It is a *field of a KRM document we already parse*, so it is editable exactly as any other field of that document — no projection, no new claim. | +| Flux `spec.valuesFrom` | a **KRM object reference** — `Secret/my-secret-values`, with a `valuesKey` | not a file either. It names a ConfigMap or Secret, which is *already* a document the store handles (and a Secret drags in [write-only-encrypted-secrets.md](write-only-encrypted-secrets.md), a different problem with a different answer). | + +Collapsing the three would claim a file-projection capability over content that lives inside +another document, or inside a Secret. The read-only-context rule below is scoped to the first +row. + +This needs no new API and no renderer. It is one more claim in the closed vocabulary that +[orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md) already defines — +knowledge arriving *"as ordinary KRM documents in the repository that Tier 0 has already +parsed"*, requiring only a group, a kind, and a handful of field paths. Never Argo's or +Flux's code. + +It rescues `platform/cert-manager/` and its `ClusterIssuer` on its own. It is filed with the +other classification fixes in [acceptance-precision.md](acceptance-precision.md), because it +is the same bug: **we refuse files we cannot name, in a repo that names them.** + +### Move 2: project the file as an object + +Read-only context is honest but unsatisfying — the user still cannot change a value. The +precedent for going further is already in this folder. + +[write-only-encrypted-secrets.md](write-only-encrypted-secrets.md) faced the same shape: a +document we cannot store as itself. Its answer was to **project it into a kind we can**, and +its punchline is the one to reuse — *"a refusal becomes a capability."* + +> **Project a free-standing values file as a synthetic KRM object. The user edits the +> object; the operator writes the bytes straight back to the file.** + +Why this is not chart inflation, and not a widening of the boundary: + +- **No renderer stands between the object and Git.** The file is plain YAML we own end to + end. Round-tripping it is the same comment-preserving `yaml.Node` edit the operator + already performs on every manifest — the pipeline is kind-agnostic, which is precisely + what [finished/higher-level-krm-documents.md](finished/higher-level-krm-documents.md) + proved when a `HelmRelease` needed no new code. +- **Fan-in = 1 gates it for free.** The analyzer can already see which + `Application`/`HelmRelease` references which file. A values file referenced by exactly one + is editable. `values/ingress-nginx/common.yaml` — referenced by several — has fan-in > 1 + and **is refused by the existing rule with no new machinery**. The corpus contains both + cases, plus two orphan values files referenced by nobody. +- **We still never learn what a value means.** `replicaCount: 4` is a scalar at a path. The + operator edits the field a human pointed it at. It does not know that the chart turns it + into a `Deployment`. + +What the chart *produces* remains untouchable: chart-rendered objects are expansion output, +carry no `ownerReference`, and are **Not mirrored**. That line does not move. + +--- + +## 3. The open questions this design must answer + +The projection is the right shape. The details are genuinely undecided, and pretending +otherwise would be the same mistake this document is correcting. + +**What is the object?** A cluster-scoped or namespaced CR (`ValuesFile`? `HelmValues`?) whose +identity is a path. That is uncomfortable: every other object the operator mirrors is *live +state observed in a cluster*, and this one is a file lifted into the API so it can be +edited. The `EncryptedSecret` precedent says the discomfort is acceptable when the +alternative is a permanent refusal. It should still be stated as the cost it is. + +**Who creates it?** Nothing in the cluster reconciles a values file. The object exists only +as an editing surface, which means the operator would be *serving* an object rather than +*mirroring* one — a real departure, and the crux of the design. + +**Does it fit the aggregated API?** The read side is a projection of Git; the write side is a +write-back. This is closer to the existing aggregated-API write path than to the watch path, +and that is probably where it belongs. + +**Layered values.** `helm-environment-values` layers `chart/values.yaml` → +`values/common.yaml` → `values/.yaml` → the Application's `parameters` → and then a +hidden `chart/.argocd-source.yaml` that **overrides all of them**. Editing "the value" is +ambiguous when five files can supply it. Fan-in = 1 refuses the shared layers; the hidden +dotfile is an `OverriddenBy` claim and must refuse the folder or the edit, loudly. **This +document should not try to resolve the layering. It should refuse it and say so.** + +**The `helm.values` string blob.** `Application.spec.source.helm.values` is a YAML document +embedded in a string field. It is "editable" today only in the sense that you can replace +the whole blob. Editing a scalar *inside* it means parsing a nested document, and the +comment-preservation guarantee does not survive the round trip. The honest recommendation is +to **name `valuesObject` as the supported surface** — structured, ordinary YAML, editable +with no special case — and to document the string form as a blob we do not reach into. +`valuesObject` is un-adjudicated in the docs today and this is the moment to adjudicate it. + +**What makes a chart a chart?** Move 1 needs to tell a values file apart from a chart's own +`values.yaml`. `Chart.yaml` + `templates/` is the signal, and the whole chart folder is +skipped as a unit. `mixed-and-hostile` plants a `templates/` directory with no `Chart.yaml`, +so the detector must not be fooled by either half alone. This residual is already logged in +[expansion-boundary-and-corpus-organisation.md](expansion-boundary-and-corpus-organisation.md). + +--- + +## 4. Where this leaves the Helm story + +| What a user wants to do | Verdict | +|---|---| +| Bump a chart version | **Editable** today | +| Change a value on a `HelmRelease` (`spec.values`) | **Editable** today | +| Change a value via `Application` `parameters` / `valuesObject` | **Editable** today | +| Change a value held in a hand-written `ConfigMap` (`valuesFrom`) | **Editable** today | +| Change a value in a values file **referenced by exactly one release** | **Planned: Editable** — §2, Move 2 | +| Keep a values file in a folder without killing the folder | **Planned: Read-only context** — §2, Move 1 | +| Change a value in a **shared** values file (`common.yaml`) | **Refused** — fan-in > 1, and correctly so | +| Change a value that a hidden `.argocd-source.yaml` overrides | **Refused** — say why, loudly | +| Edit the chart's `templates/` | **Refused**, permanently | +| Edit a chart-rendered object | **Not mirrored** — expansion layer | + +The first four rows already work. What the product is missing is rows five and six, and row +six is a week's work in the acceptance gate. diff --git a/internal/git/acceptance_gate_test.go b/internal/git/acceptance_gate_test.go index 8fc22e6c..0b9d895a 100644 --- a/internal/git/acceptance_gate_test.go +++ b/internal/git/acceptance_gate_test.go @@ -25,6 +25,11 @@ const plainKustomizeYAML = "apiVersion: kustomize.config.k8s.io/v1beta1\n" + "kind: Kustomization\n" + "resources:\n - cm.yaml\n" +// cmYAML is the resource plainKustomizeYAML points at. It has to be there: the +// analyzer now builds every render root with kustomize, and a dangling resources: +// entry fails the build. +const cmYAML = "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: seeded\n namespace: default\ndata:\n k: v\n" + // A GitTarget subtree holding a kustomization.yaml that uses an unsupported feature is // refused: the live flush returns an *AcceptanceRefusedError naming the file and writes // nothing, so the operator never edits a folder it cannot safely manage. @@ -55,6 +60,9 @@ func TestPlanFlush_AcceptsPlainKustomizeFolder(t *testing.T) { writer := newContentWriter(types.SensitiveResourcePolicy{}) worktree := newWorktreeForTest(t) + // cm.yaml must actually exist: a render root whose resources: entry does not + // resolve is refused now (kustomize cannot build it, so neither can Flux). + seedPlacedManifest(t, worktree, "cm.yaml", cmYAML) seedPlacedManifest(t, worktree, "kustomization.yaml", plainKustomizeYAML) w := &BranchWorker{contentWriter: writer} diff --git a/internal/git/write_boundary_precondition_test.go b/internal/git/write_boundary_precondition_test.go index 9253c4ec..51cfa4ae 100644 --- a/internal/git/write_boundary_precondition_test.go +++ b/internal/git/write_boundary_precondition_test.go @@ -101,7 +101,11 @@ func diamondOverlayKust(newTag string) string { // override chains (write-fan-in > 1). Ordered so seeding it into Git produces stable commits. func diamondFiles() []struct{ rel, content string } { return []struct{ rel, content string }{ - {"kustomization.yaml", "resources:\n - a\n - b\n"}, + // No root kustomization: a/ and b/ are two SEPARATE render roots that both + // read ../base. A true diamond (one root reaching base through both) is not + // buildable by kustomize at all — "may not add resource with an already + // registered id" — so it is refused at the acceptance gate and never reaches + // the write-boundary preconditions this test is about. {"a/kustomization.yaml", diamondOverlayKust("1.0.0")}, {"b/kustomization.yaml", diamondOverlayKust("2.0.0")}, {"base/kustomization.yaml", "resources:\n - deployment.yaml\n"}, diff --git a/internal/manifestanalyzer/acceptance.go b/internal/manifestanalyzer/acceptance.go index f3d598bc..918442c4 100644 --- a/internal/manifestanalyzer/acceptance.go +++ b/internal/manifestanalyzer/acceptance.go @@ -252,9 +252,10 @@ func unsupportedKustomizeRefusals(store *ManifestStore) []AcceptanceIssue { Path: rd.Location.Path, DocumentIndex: rd.Location.DocumentIndex, Message: "kustomization " + rd.Location.Path + " uses an unsupported feature " + - "(generators/patches/components/helm/replacements/transformers/namePrefix/nameSuffix/remote bases) " + - "or malformed images/replicas overrides; " + - "the operator cannot map it back to editable source documents and will not write into this folder", + "(generators/patches/components/helm/replacements/transformers/namePrefix/nameSuffix/remote bases), " + + "declares malformed images/replicas overrides, or is a render root kustomize cannot build; " + + "the operator cannot map it back to editable source documents and will not write into this folder " + + "(a kustomize-build-failed diagnostic on this path carries the build error)", }) } return out diff --git a/internal/manifestanalyzer/acceptance_test.go b/internal/manifestanalyzer/acceptance_test.go index ed8ad67d..7689b6e1 100644 --- a/internal/manifestanalyzer/acceptance_test.go +++ b/internal/manifestanalyzer/acceptance_test.go @@ -5,6 +5,7 @@ package manifestanalyzer import ( "context" "errors" + "io/fs" "strings" "testing" "testing/fstest" @@ -35,7 +36,7 @@ func snapMapper() typeset.Lookup { // acceptanceOf builds a store with the given allowlist and runs the gate. func acceptanceOf( t *testing.T, - fsys fstest.MapFS, + fsys fs.FS, mapper typeset.Lookup, policy AcceptancePolicy, ) (*ManifestStore, Acceptance) { @@ -224,9 +225,13 @@ func TestAccept_MultipleRefusalsSorted(t *testing.T) { } func TestAccept_MultipleRetainedSorted(t *testing.T) { + // Both kustomizations must actually BUILD: a render root whose resources: entry + // does not resolve is refused now (kustomize cannot build it, so neither can + // Flux), where the old structural parse simply never looked. + sharedKust := "apiVersion: kustomize.config.k8s.io/v1beta1\nkind: Kustomization\nresources:\n - ../deploy.yaml\n" fsys := fstest.MapFS{ - "b/kustomization.yaml": {Data: []byte(kustomizationY)}, - "a/kustomization.yaml": {Data: []byte(kustomizationY)}, + "b/kustomization.yaml": {Data: []byte(sharedKust)}, + "a/kustomization.yaml": {Data: []byte(sharedKust)}, "deploy.yaml": {Data: []byte(deployYAML)}, } store, acc := acceptanceOf(t, fsys, snapMapper(), AcceptancePolicy{Allowlist: DefaultAllowlist()}) diff --git a/internal/manifestanalyzer/kustomize_render.go b/internal/manifestanalyzer/kustomize_render.go index f9aaf555..d3a913af 100644 --- a/internal/manifestanalyzer/kustomize_render.go +++ b/internal/manifestanalyzer/kustomize_render.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "path" + "regexp" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/kustomize/api/krusty" @@ -33,8 +34,10 @@ import ( // configuredBy: {apiVersion: builtin, kind: ImageTagTransformer} // // The first says which source file produced the object. The second says which -// kustomization's transformers touched it, in build order — the override chain, -// handed to us by the renderer that applies it. +// kustomization's transformers RAN over it, in build order — the override chain, +// handed to us by the renderer that applies it. (Ran over, not touched: kustomize +// records a transformer against every object in the build, whether or not it changed +// anything. See chainOf.) const ( originAnnotation = "config.kubernetes.io/origin" transformationsAnnotation = "alpha.config.kubernetes.io/transformations" @@ -57,6 +60,36 @@ const ( // remote base" true. var errRemoteBase = errors.New("kustomization reaches a remote base; the operator never fetches one") +// errInvalidImageName refuses a build whose images: entry carries a name kustomize +// cannot compile. +// +// An images: entry's name: is a REGULAR EXPRESSION, not a literal, and kustomize +// compiles it while DISCARDING the compile error (api/internal/image/image.go): +// +// pattern, _ := regexp.Compile("^" + name + "(:[a-zA-Z0-9_.{}-]*)?(@sha256:...)?$") +// +// It then dereferences the nil *Regexp. So `- name: "ngin["` does not fail the build — +// it PANICS inside it, on content that came straight from a user's repository. Like the +// remote-base check, this one must run before krusty, and for the same reason: it is not +// a modelling question, it is what keeps a hostile kustomization.yaml from taking the +// process somewhere it cannot come back from. +var errInvalidImageName = errors.New("images: entry name is not a valid regular expression") + +// errBuildPanicked is the net under krusty. errInvalidImageName covers the one panic we +// found; this covers the ones we have not. A build runs library code we do not own over +// bytes we do not control, so a panic there has to become a refused folder — never a +// crashed CLI, and never a GitTarget that panics, requeues and panics again for as long +// as the repository stays as it is (controller-runtime recovers reconciler panics by +// default, which turns a crash into a hot loop, not into safety). +var errBuildPanicked = errors.New("kustomize build panicked") + +// imageNamePattern is the pattern kustomize compiles for an images: entry name +// (api/internal/image/image.go). We validate the WHOLE pattern, not the name alone, so +// that what we accept is exactly what kustomize can compile. +func imageNamePattern(name string) string { + return "^" + name + "(:[a-zA-Z0-9_.{}-]*)?(@sha256:[a-zA-Z0-9_.{}-]*)?$" +} + // renderedObject is one object kustomize produced, with the provenance saying // where it came from and what shaped it. type renderedObject struct { @@ -91,33 +124,56 @@ const renderMountPoint = "/scan" // build never touches the real disk, never executes a plugin, and never reaches the // network. func renderRoot(files []manifestedit.FileContent, rootDir string) ([]renderedObject, error) { - if err := refuseRemoteBases(parseKustomizations(files), rootDir); err != nil { + if err := refuseBeforeBuild(parseKustomizations(files), rootDir); err != nil { return nil, err } fSys, err := renderFilesystem(files, rootDir) if err != nil { return nil, err } - k := krusty.MakeKustomizer(&krusty.Options{ - LoadRestrictions: kustypes.LoadRestrictionsRootOnly, - PluginConfig: kustypes.DisabledPluginConfig(), // no exec, no Go plugins - }) - resMap, err := k.Run(fSys, path.Join(renderMountPoint, rootDir)) + resMap, err := build(fSys, path.Join(renderMountPoint, rootDir)) if err != nil { return nil, fmt.Errorf("kustomize build: %w", err) } return collectRendered(resMap, rootDir) } -// refuseRemoteBases refuses the build when any kustomization THIS ROOT REACHES -// declares a remote base. +// build runs krusty, converting a panic into an error (errBuildPanicked). +// +// LoadRestrictionsNone is what Flux itself builds with, and it is safe here for the same +// reason it is safe there: THE FILESYSTEM IS THE JAIL. The in-memory filesystem contains +// only the scanned tree, so "unrestricted" loading cannot reach the real disk, and a +// remote base is refused before we get here. +// +// RootOnly would be the wrong kind of strict. It forbids loading a FILE from outside the +// render root — `resources: [../shared.yaml]` — which Flux renders happily. We would then +// fail to build a root that deploys in production, see no chain for it, and quietly stop +// enforcing write-fan-in on the file it shares. Refusing to look is not a safety property. +func build(fSys filesys.FileSystem, target string) (_ resmap.ResMap, err error) { + // On a panic the result is the zero ResMap (nil) and err is what the deferred + // function leaves here, so only the error needs naming. + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("%w: %v", errBuildPanicked, r) + } + }() + k := krusty.MakeKustomizer(&krusty.Options{ + LoadRestrictions: kustypes.LoadRestrictionsNone, + PluginConfig: kustypes.DisabledPluginConfig(), // no exec, no Go plugins + }) + return k.Run(fSys, target) +} + +// refuseBeforeBuild refuses the build when any kustomization THIS ROOT REACHES is one we +// must not hand to krusty: it declares a remote base (kustomize would fetch it), or an +// images: entry name kustomize would nil-deref on (see errInvalidImageName). // // Scoping it to the reachable graph is deliberate, and it is both safer and more -// accurate than a scan-wide check: kustomize only fetches what it actually loads, +// accurate than a scan-wide check: kustomize only loads what it actually reaches, // so a remote base in an unrelated sibling folder cannot make this build reach the // network — and refusing on its account would refuse a folder that is perfectly // renderable. -func refuseRemoteBases(kusts map[string]*kustomizationDoc, rootDir string) error { +func refuseBeforeBuild(kusts map[string]*kustomizationDoc, rootDir string) error { visited := map[string]struct{}{} var walk func(dir string) error walk = func(dir string) error { @@ -129,8 +185,8 @@ func refuseRemoteBases(kusts map[string]*kustomizationDoc, rootDir string) error if cur == nil { return nil } - if hasRemoteResource(cur.resources) { - return fmt.Errorf("%s: %w", cur.path, errRemoteBase) + if err := unbuildable(cur); err != nil { + return err } for _, entry := range cur.resources { target := cleanJoin(dir, entry) @@ -148,6 +204,21 @@ func refuseRemoteBases(kusts map[string]*kustomizationDoc, rootDir string) error return walk(rootDir) } +// unbuildable reports why one kustomization must not be handed to krusty, or nil when +// it may be. Both reasons are properties of the BUILD, not of what we can model: a +// kustomization we refuse here is one kustomize would fetch the network for, or crash on. +func unbuildable(k *kustomizationDoc) error { + if hasRemoteResource(k.resources) { + return fmt.Errorf("%s: %w", k.path, errRemoteBase) + } + for _, img := range k.images { + if _, err := regexp.Compile(imageNamePattern(img.Name)); err != nil { + return fmt.Errorf("%s: images[%d].name %q: %w", k.path, img.Index, img.Name, errInvalidImageName) + } + } + return nil +} + // renderFilesystem materialises the scan's files in memory and asks the render root // for provenance. func renderFilesystem(files []manifestedit.FileContent, rootDir string) (filesys.FileSystem, error) { diff --git a/internal/manifestanalyzer/kustomize_render_hostile_test.go b/internal/manifestanalyzer/kustomize_render_hostile_test.go new file mode 100644 index 00000000..d2a8068b --- /dev/null +++ b/internal/manifestanalyzer/kustomize_render_hostile_test.go @@ -0,0 +1,121 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "testing" + + "github.com/stretchr/testify/require" + "sigs.k8s.io/kustomize/kyaml/filesys" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" +) + +// A kustomization.yaml is untrusted input: it comes out of a user's repository, and the +// renderer we now hand it to is a library we do not own. These are the tests for what it +// does with content nobody would write on purpose — and for the one shape that makes a +// folder INVISIBLE to the refusal path rather than merely broken. + +// An images: entry's name: is a regular expression, and kustomize compiles it while +// discarding the compile error, then dereferences the nil *Regexp. `- name: "ngin["` does +// not fail the build; it panics inside it. We must refuse before krusty ever sees it. +func TestRenderRoot_InvalidImageNameIsRefusedBeforeTheBuild(t *testing.T) { + files := imageFixture("nginx:v1", " - name: \"ngin[\"\n newTag: \"2.0\"\n") + + rendered, err := renderRoot(files, ".") // must not panic + + require.Nil(t, rendered) + require.ErrorIs(t, err, errInvalidImageName) + require.Contains(t, err.Error(), "images[0].name") +} + +// A name that IS a valid regex still builds — kustomize's matching is regex matching, and +// refusing more than kustomize refuses would refuse folders that deploy in production. +func TestRenderRoot_ValidRegexImageNameStillBuilds(t *testing.T) { + files := imageFixture("nginx:v1", " - name: \"ngin.\"\n newTag: \"2.0\"\n") + + rendered, err := renderRoot(files, ".") + + require.NoError(t, err) + require.Len(t, rendered, 1) + slots := collectContainerSlots(rendered[0].Object.Object) + require.Len(t, slots, 1) + // Measured against kustomize: `ngin.` matches `nginx`. Our own renderImage compares + // names for equality and does NOT — see docs/design/support-boundary/render-attribution.md. + require.Equal(t, "nginx:2.0", slots[0].image) +} + +// The net under krusty: whatever panics in there, the caller gets an error and the process +// keeps its footing. Driven straight at build(), because the refusal above means the panic +// we know about can no longer reach it. +func TestBuild_PanicBecomesAnError(t *testing.T) { + fSys := filesys.MakeFsInMemory() + require.NoError(t, fSys.WriteFile("/scan/kustomization.yaml", + []byte("resources:\n - deployment.yaml\nimages:\n - name: \"ngin[\"\n newTag: \"2.0\"\n"))) + require.NoError(t, fSys.WriteFile("/scan/deployment.yaml", []byte( + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: web\nspec:\n template:\n spec:\n"+ + " containers:\n - name: web\n image: nginx:v1\n"))) + + resMap, err := build(fSys, "/scan") // must not panic + + require.Nil(t, resMap) + require.ErrorIs(t, err, errBuildPanicked) +} + +// A cycle has no render root — every directory in it is referenced by another — so a walk +// over renderRoots alone builds nothing there, records no failure, and the folder passes +// silently. That is the dangerous direction: no build means no chain, no chain means no +// ambiguity, and no ambiguity means the write-fan-in guard never fires on a folder +// kustomize cannot build at all. +func TestRenderChains_CycleIsBuiltAndRefused(t *testing.T) { + files := []manifestedit.FileContent{ + {Path: "a/kustomization.yaml", Content: []byte("resources:\n - ../b\n - cm.yaml\n")}, + {Path: "a/cm.yaml", Content: []byte("apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: a\n")}, + {Path: "b/kustomization.yaml", Content: []byte("resources:\n - ../a\n")}, + } + kusts := parseKustomizations(files) + require.Empty(t, renderRoots(kusts), "a cycle has no render root; that is the whole problem") + + chains, failed := renderChains(files, kusts) + + require.Empty(t, chains) + require.Len(t, failed, 1, "the cycle must produce exactly one refusal, not one per member") + require.Contains(t, failed["a/kustomization.yaml"], "cycle detected", + "kustomize is the one that gets to call it a cycle; we only make sure it is asked") +} + +// The representative built for a cycle is deterministic (sorted first), so the refusal +// names the same file on every reconcile instead of flapping between them. +func TestRenderTargets_CycleRepresentativeIsDeterministic(t *testing.T) { + files := []manifestedit.FileContent{ + {Path: "z/kustomization.yaml", Content: []byte("resources:\n - ../m\n")}, + {Path: "m/kustomization.yaml", Content: []byte("resources:\n - ../z\n")}, + {Path: "standalone/kustomization.yaml", Content: []byte("resources:\n - cm.yaml\n")}, + {Path: "standalone/cm.yaml", Content: []byte("apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: c\n")}, + } + kusts := parseKustomizations(files) + + for range 5 { + require.Equal(t, []string{"standalone", "m"}, renderTargets(kusts), + "real roots first, then one representative per rootless component") + } +} + +// kustomize builds ONE ImageTagTransformer PER images: ENTRY and stamps them all with the +// same origin, so a kustomization with three entries leaves three byte-identical +// transformation records on every object in the build. Each record would otherwise +// contribute that file's whole entry list again, tripling the chain. +func TestChainOf_DuplicateTransformationRecordsDoNotDuplicateEntries(t *testing.T) { + files := imageFixture("app:v1", + " - name: app\n newTag: v2\n - name: other\n newTag: v3\n - name: third\n newTag: v4\n") + rendered, err := renderRoot(files, ".") + require.NoError(t, err) + require.Len(t, rendered, 1) + require.Len(t, rendered[0].TransformedBy, 3, + "kustomize records one transformation per images: entry — if this changes, the dedupe below is why") + + chain := chainOf(rendered[0], parseKustomizations(files)) + + require.NotNil(t, chain) + require.Len(t, chain.Images, 3, "three entries, not three records × three entries") +} diff --git a/internal/manifestanalyzer/kustomize_render_test.go b/internal/manifestanalyzer/kustomize_render_test.go index 1c7ce1eb..29a6d2b4 100644 --- a/internal/manifestanalyzer/kustomize_render_test.go +++ b/internal/manifestanalyzer/kustomize_render_test.go @@ -17,19 +17,15 @@ import ( ) // This is the differential test that licenses deleting the re-implemented -// transformers. For every kustomize render root in both corpora it renders the -// folder with kustomize itself and checks two independent claims: +// transformers: for every kustomize render root in both corpora, the image our +// renderImage chain produces must be byte-for-byte the image kustomize produces. // -// 1. the override CHAIN our resources-graph walk attributes to a document is the -// same chain kustomize says it applied (its transformations annotation), and -// 2. the IMAGE our renderImage chain produces is byte-for-byte the image kustomize -// produces. -// -// A disagreement is either a bug in our re-implementation or a misunderstanding of -// kustomize. Either way it is exactly what we want to find before the write path -// starts trusting the renderer. +// It used to also compare the override CHAIN against kustomize's transformations +// annotation. That assertion has done its job and is gone: the chain is now READ +// from that annotation (override_chain.go), so comparing the two would be comparing +// kustomize to itself. -func TestRenderRoot_ChainAndImagesAgreeWithKustomize(t *testing.T) { +func TestRenderRoot_ImagesAgreeWithKustomize(t *testing.T) { roots := allCorpusRenderRoots(t) require.NotEmpty(t, roots, "no render roots found — the test would prove nothing") @@ -45,6 +41,7 @@ func TestRenderRoot_ChainAndImagesAgreeWithKustomize(t *testing.T) { } kusts := parseKustomizations(root.files) + chains, _ := renderChains(root.files, kusts) // once per fixture, not once per object for _, ro := range rendered { if ro.OriginPath == "" { continue // a generated resource; generators are refused @@ -53,8 +50,7 @@ func TestRenderRoot_ChainAndImagesAgreeWithKustomize(t *testing.T) { if src == nil { continue // renamed by a transformer we refuse; not a supported shape } - chain, ambiguous := ourChainFor(kusts, root.files, ro.OriginPath) - assertChainMatchesKustomize(t, ro, chain, ambiguous) + chain, ambiguous := ourChainFor(chains, ro) if ambiguous { continue // we route nothing through it; there is no claim to check } @@ -66,52 +62,6 @@ func TestRenderRoot_ChainAndImagesAgreeWithKustomize(t *testing.T) { compared, skipped) } -// assertChainMatchesKustomize checks that the kustomizations our graph walk -// attributes to a document are exactly the ones kustomize says ran an -// ImageTagTransformer over it, in the same order. -// -// The ambiguous case is a deliberate, documented divergence rather than a bug. When -// more than one render root reaches a file with differing chains, we attach NO chain -// (fan-in = 1: we will not route an edit through a file two roots disagree about). -// kustomize, asked to build one root, naturally reports the transformer that root -// ran. So for an ambiguous file we assert the opposite thing: that kustomize DID -// apply a chain and we deliberately declined to claim one. -func assertChainMatchesKustomize( - t *testing.T, - ro renderedObject, - chain *KustomizeOverrides, - ambiguous bool, -) { - t.Helper() - var kustomizeSays []string - for _, tr := range ro.TransformedBy { - if tr.Kind == imageTagTransformer { - kustomizeSays = append(kustomizeSays, tr.ConfiguredIn) - } - } - if ambiguous { - require.Nil(t, chain, "%s: an ambiguous file must carry no chain", ro.OriginPath) - require.NotEmpty(t, kustomizeSays, - "%s: we refused to attribute a chain, so kustomize must have applied one — "+ - "otherwise the ambiguity refusal is guarding nothing", ro.OriginPath) - return - } - - var weSay []string - seen := map[string]bool{} - if chain != nil { - for _, img := range chain.Images { - if !seen[img.Source] { - seen[img.Source] = true - weSay = append(weSay, img.Source) - } - } - } - require.Equal(t, kustomizeSays, weSay, - "%s: kustomize ran ImageTagTransformer configured in %v; our graph walk attributes %v", - ro.OriginPath, kustomizeSays, weSay) -} - // assertImagesMatchKustomize renders each source container image through our own // chain and requires it to equal what kustomize actually produced. Returns the // number of images compared. @@ -147,15 +97,18 @@ func assertImagesMatchKustomize( return compared } -// ourChainFor is the override chain our resources-graph walk attributes to a file, -// and whether the walk found the file ambiguous (reached by more than one render -// root with differing chains, which we refuse to route through). +// ourChainFor is the override chain the store attributes to a document, and whether +// it was found ambiguous (reached by more than one render root with differing +// chains, which we refuse to route through). func ourChainFor( - kusts map[string]*kustomizationDoc, - files []manifestedit.FileContent, - originPath string, + chains map[chainKey]*overrideAssignment, + ro renderedObject, ) (*KustomizeOverrides, bool) { - a := kustomizeOverrideAssignments(kusts, resourceFilePaths(files))[originPath] + a := chains[chainKey{ + originPath: ro.OriginPath, + kind: ro.Object.GetKind(), + name: ro.Object.GetName(), + }] if a == nil { return nil, false } diff --git a/internal/manifestanalyzer/override_chain.go b/internal/manifestanalyzer/override_chain.go new file mode 100644 index 00000000..934f3515 --- /dev/null +++ b/internal/manifestanalyzer/override_chain.go @@ -0,0 +1,264 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "sort" + "strings" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" +) + +// This file derives the kustomize override chain governing each document from +// kustomize's own provenance, instead of walking the resources graph by hand. +// +// Every rendered object carries alpha.config.kubernetes.io/transformations: the +// ordered list of which kustomization configured which builtin transformer. Filtered +// to the two transformers behind the edit-through channels, that IS the chain — read +// off the renderer that applies it rather than reconstructed from a DFS that has to +// re-derive kustomize's build order, its cycle rules, and its diamond behaviour. +// +// What the annotation does NOT say is which images: ENTRY supplied a value — kustomize +// keeps no field-level provenance at all — so the projection still has to derive the +// supplier itself. See overrides_projection.go. +// +// See docs/design/support-boundary/kustomize-support-boundary.md §7. + +// reasonRenderFailed marks a render root kustomize could not build. The folder is +// refused: if the build fails, Flux cannot deploy it either. +const reasonRenderFailed manifestedit.DiagReason = "kustomize-build-failed" + +// chainKey identifies a rendered document: the source file kustomize says produced +// it, plus its kind and name. Namespace is deliberately absent — a namespace +// transformer rewrites it, and the source document may not carry one at all. +type chainKey struct { + originPath string + kind string + name string +} + +// renderRoots returns the kustomization directories no other kustomization in the +// subtree references — the directories a build would be invoked on — in sorted order +// for deterministic walks. +// +// This is the one piece of graph reasoning we keep: kustomize renders the root you +// hand it, so something has to decide WHICH directories are roots. Everything the +// walk used to do beyond that — build order, the override chain, the diamond — now +// comes from the renderer itself. +func renderRoots(kusts map[string]*kustomizationDoc) []string { + referenced := map[string]struct{}{} + for dir, k := range kusts { + for _, entry := range k.resources { + target := cleanJoin(dir, entry) + if target == "" { + continue + } + if _, ok := kusts[target]; ok { + referenced[target] = struct{}{} + } + } + } + roots := make([]string, 0, len(kusts)) + for dir := range kusts { + if _, ok := referenced[dir]; !ok { + roots = append(roots, dir) + } + } + sort.Strings(roots) + return roots +} + +// renderTargets returns the directories renderChains must BUILD: every render root, plus +// a deterministic representative of every component that has no root at all. +// +// A component with no root is a cycle — `a` referencing `b` referencing `a` — and it is +// the one shape renderRoots cannot see: every directory in it is referenced by another, +// so none of them is a root, so a plain walk over renderRoots builds nothing there, +// records no failure, and leaves the component INVISIBLE. That is precisely the hole the +// refusal exists to close: no build means no chain, no chain means no ambiguity, and no +// ambiguity means the write-fan-in guard never fires on a folder kustomize cannot build +// at all. Silence is the dangerous answer here, so we make sure every kustomization is +// covered by some build attempt and let kustomize give the verdict (it says "cycle +// detected"; Flux would say the same). +func renderTargets(kusts map[string]*kustomizationDoc) []string { + targets := renderRoots(kusts) + covered := map[string]struct{}{} + for _, root := range targets { + markReachable(kusts, root, covered) + } + + rest := make([]string, 0, len(kusts)) + for dir := range kusts { + if _, ok := covered[dir]; !ok { + rest = append(rest, dir) + } + } + sort.Strings(rest) + for _, dir := range rest { + if _, ok := covered[dir]; ok { + continue // reached from an earlier representative of the same cycle + } + markReachable(kusts, dir, covered) + targets = append(targets, dir) + } + return targets +} + +// markReachable records dir and every kustomization it reaches through resources:. +func markReachable(kusts map[string]*kustomizationDoc, dir string, covered map[string]struct{}) { + if _, seen := covered[dir]; seen { + return + } + covered[dir] = struct{}{} + doc := kusts[dir] + if doc == nil { + return + } + for _, entry := range doc.resources { + target := cleanJoin(dir, entry) + if target == "" { + continue + } + if _, isKust := kusts[target]; isKust { + markReachable(kusts, target, covered) + } + } +} + +// renderChains renders every render root and returns, per rendered document, the +// override chain governing it — or an ambiguity marker when more than one render +// root reaches the same document with DIFFERENT chains, which is the fan-in > 1 case +// we refuse to route through. +// +// It also returns, per kustomization path, the roots that FAILED to build. Those +// must refuse the folder, and it is important to see why a silent skip would be +// unsafe rather than merely unhelpful: a root that does not build yields no chain, +// so no ambiguity is recorded, so the write-fan-in guard never fires — and the +// writer would then write straight through into a base shared by two render paths. +// A diamond (one root reaching a base through two overlays) is exactly this shape: +// kustomize refuses it with "may not add resource with an already registered id", +// which means Flux cannot deploy the folder either. If kustomize cannot build it, we +// cannot reason about it, and we say so. +func renderChains( + files []manifestedit.FileContent, + kusts map[string]*kustomizationDoc, +) (map[chainKey]*overrideAssignment, map[string]string) { + out := map[chainKey]*overrideAssignment{} + failed := map[string]string{} + + for _, rootDir := range renderTargets(kusts) { + rendered, err := renderRoot(files, rootDir) + if err != nil { + if doc := kusts[rootDir]; doc != nil { + failed[doc.path] = err.Error() + } + continue + } + for _, ro := range rendered { + if ro.OriginPath == "" { + continue // a generated resource: it has no source document to edit + } + key := chainKey{ + originPath: ro.OriginPath, + kind: ro.Object.GetKind(), + name: ro.Object.GetName(), + } + record(out, key, chainOf(ro, kusts)) + } + } + return out, failed +} + +// chainOf reads the override chain kustomize applied to one object: the images: and +// replicas: entries of every kustomization whose ImageTagTransformer or +// ReplicaCountTransformer RAN over it, in the order kustomize ran them (innermost base +// first). +// +// "Ran over it", not "touched it", and the distinction is measured: kustomize appends a +// transformations record to EVERY object in the build for EVERY transformer that ran, +// modified or not (api/resmap/reswrangler.go loops the whole ResMap with no diff check). +// A ConfigMap no image transformer could possibly touch still collects ImageTagTransformer +// records. So the annotation names the kustomizations whose transformers were in this +// object's pipeline — never which entry did anything, and never whether anything was done. +// That is enough to know which files GOVERN the object, which is what the chain is for. +// +// The records are deduped because kustomize builds ONE TRANSFORMER PER ENTRY and gives +// them all the same origin: a kustomization with three images: entries stamps three +// byte-identical records, and each record would otherwise contribute that file's whole +// entry list again. +func chainOf(ro renderedObject, kusts map[string]*kustomizationDoc) *KustomizeOverrides { + ov := &KustomizeOverrides{} + seen := map[transformation]struct{}{} + for _, tr := range ro.TransformedBy { + if _, dup := seen[tr]; dup { + continue + } + seen[tr] = struct{}{} + doc := kusts[slashDir(tr.ConfiguredIn)] + if doc == nil { + continue + } + switch tr.Kind { + case imageTagTransformer: + ov.Images = append(ov.Images, doc.images...) + case replicaCountTransformer: + ov.Replicas = append(ov.Replicas, doc.replicas...) + } + } + if len(ov.Images) == 0 && len(ov.Replicas) == 0 { + return nil + } + return ov +} + +// record accumulates one root's view of a document. Two roots agreeing on the chain +// is fine — the same document rendered the same way twice. Two roots DISAGREEING is +// the diamond: the file is shared context, and an edit routed through either chain +// would change what the other root renders. +// +// anyOverrides preserves the existing narrowness of the fan-in refusal: a base +// document reached by two roots that declare no images:/replicas: at all is shared +// context, but nothing is at stake in it, so it is not refused. +func record(out map[chainKey]*overrideAssignment, key chainKey, ov *KustomizeOverrides) { + fp := fingerprint(ov) + prev, seen := out[key] + if !seen { + out[key] = &overrideAssignment{ + chainKeys: map[string]struct{}{fp: {}}, + overrides: ov, + anyOverrides: ov != nil, + } + return + } + if _, same := prev.chainKeys[fp]; same { + return // the same chain, reached twice; not an ambiguity + } + prev.chainKeys[fp] = struct{}{} + prev.anyOverrides = prev.anyOverrides || ov != nil + prev.overrides = nil // more than one distinct chain: route through none of them +} + +// fingerprint reduces a chain to a comparable string, so two roots reaching one +// document can be compared for agreement. +func fingerprint(ov *KustomizeOverrides) string { + if ov == nil { + return "" + } + var b strings.Builder + for _, e := range ov.Images { + b.WriteString(e.Source) + b.WriteByte('\x00') + b.WriteString(e.Name) + b.WriteByte('\x00') + b.WriteString(e.NewName + "|" + e.NewTag + "|" + e.Digest) + b.WriteByte('\x01') + } + b.WriteByte('\x02') + for _, e := range ov.Replicas { + b.WriteString(e.Source) + b.WriteByte('\x00') + b.WriteString(e.Name) + b.WriteByte('\x01') + } + return b.String() +} diff --git a/internal/manifestanalyzer/overrides.go b/internal/manifestanalyzer/overrides.go index 96fa76e4..7445a5b1 100644 --- a/internal/manifestanalyzer/overrides.go +++ b/internal/manifestanalyzer/overrides.go @@ -3,9 +3,6 @@ package manifestanalyzer import ( - "sort" - "strings" - "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" ) @@ -82,141 +79,20 @@ func (a *overrideAssignment) ambiguous() bool { return a != nil && len(a.chainKeys) > 1 && a.anyOverrides } -// kustomizeOverrideAssignments walks every render root — a kustomization no other -// kustomization references, i.e. the directory a human would `kustomize build` — -// and records, per resource file, the kustomization chain along the reference -// path. Unlike the namespace walk (which treats every namespace-bearing -// kustomization as a root and refuses parent/child conflicts), a referenced base -// is not a root here: its transformers compose with its parent's, innermost -// first, exactly as kustomize applies them. -// -// Cycle protection is on the CURRENT PATH, not per walk: a diamond (one root -// reaching a shared base through two overlays) must record both paths so their -// differing chains trip the ambiguity refusal, while a true reference cycle -// still terminates. Real kustomize rejects the diamond outright (duplicate -// resources), so ambiguity — never silent first-path attribution — is the -// honest outcome. -func kustomizeOverrideAssignments( - kusts map[string]*kustomizationDoc, - resourceFiles map[string]struct{}, -) map[string]*overrideAssignment { - out := map[string]*overrideAssignment{} - for _, rootDir := range renderRoots(kusts) { - root := kusts[rootDir] - if root == nil || root.unsupported { - continue - } - onPath := map[string]struct{}{} - var stack []*kustomizationDoc - var walk func(dir string, cur *kustomizationDoc) - walk = func(dir string, cur *kustomizationDoc) { - if cur == nil || cur.unsupported { - return - } - if _, cycling := onPath[dir]; cycling { - return - } - onPath[dir] = struct{}{} - stack = append(stack, cur) - for _, entry := range cur.resources { - target := cleanJoin(dir, entry) - switch { - case target == "": - // empty, or escapes the scanned root: contributes no chain. - case mapHasKey(resourceFiles, target): - recordOverrideChain(out, target, stack) - default: - walk(target, kusts[target]) - } - } - stack = stack[:len(stack)-1] - delete(onPath, dir) - } - walk(rootDir, root) - } - return out -} - -// renderRoots returns the kustomization directories no other kustomization in -// the subtree references — the directories a build would be invoked on — in -// sorted order for deterministic walks. -func renderRoots(kusts map[string]*kustomizationDoc) []string { - referenced := map[string]struct{}{} - for dir, k := range kusts { - for _, entry := range k.resources { - target := cleanJoin(dir, entry) - if target == "" { - continue - } - if _, ok := kusts[target]; ok { - referenced[target] = struct{}{} - } - } - } - roots := make([]string, 0, len(kusts)) - for dir := range kusts { - if _, ok := referenced[dir]; !ok { - roots = append(roots, dir) - } - } - sort.Strings(roots) - return roots -} - -// recordOverrideChain records one root→file chain. The walk descends root-first, -// so the stack is outermost-first; build order (innermost kustomization's -// transformers first) is its reverse. -func recordOverrideChain(out map[string]*overrideAssignment, file string, stack []*kustomizationDoc) { - chain := make([]*kustomizationDoc, len(stack)) - for i, k := range stack { - chain[len(stack)-1-i] = k - } - keys := make([]string, len(chain)) - for i, k := range chain { - keys[i] = k.path - } - key := strings.Join(keys, "\x00") - - a := out[file] - if a == nil { - a = &overrideAssignment{chainKeys: map[string]struct{}{}} - out[file] = a - } - flat := flattenOverrideChain(chain) - if _, seen := a.chainKeys[key]; !seen { - a.chainKeys[key] = struct{}{} - if a.overrides == nil { - a.overrides = flat - } - } - if flat != nil { - a.anyOverrides = true - } -} - -// flattenOverrideChain concatenates the chain's entries in build order. Nil when -// no kustomization in the chain declares any override. -func flattenOverrideChain(chain []*kustomizationDoc) *KustomizeOverrides { - var ov KustomizeOverrides - for _, k := range chain { - ov.Images = append(ov.Images, k.images...) - ov.Replicas = append(ov.Replicas, k.replicas...) - } - if len(ov.Images) == 0 && len(ov.Replicas) == 0 { - return nil - } - return &ov -} - // resolveOverrides returns the overrides to attach to a document in the given // file, plus an ambiguity diagnostic when distinct chains with overrides at // stake reach it. Attribution is purely structural (no API source needed), so it // also works in structure-only analysis. func resolveOverrides( loc manifestedit.Location, - assignments map[string]*overrideAssignment, + id manifestedit.Identity, + assignments map[chainKey]*overrideAssignment, ) (*KustomizeOverrides, *manifestedit.Diagnostic) { - a := assignments[filepathToSlash(loc.Path)] + a := assignments[chainKey{ + originPath: filepathToSlash(loc.Path), + kind: id.Kind, + name: id.Name, + }] if a == nil { return nil, nil } diff --git a/internal/manifestanalyzer/overrides_test.go b/internal/manifestanalyzer/overrides_test.go index 468e032c..545866c4 100644 --- a/internal/manifestanalyzer/overrides_test.go +++ b/internal/manifestanalyzer/overrides_test.go @@ -69,16 +69,46 @@ func TestKustomizeOverridesCorpus_ReplicasOverlayChain(t *testing.T) { } // A diamond under ONE render root (root → a → base, root → b → base) must -// record both paths: their chains differ, so no overrides attach and the -// ambiguity diagnostic fires. Pins the on-path (not per-walk) cycle protection. -func TestKustomizeOverridesCorpus_DiamondUnderOneRoot(t *testing.T) { +// TestKustomizeOverridesCorpus_DiamondIsUnbuildable pins the diamond's fate. +// +// A diamond — one render root reaching a shared base through two overlays — is not +// merely ambiguous to us: kustomize REFUSES to build it ("may not add resource with +// an already registered id"), which means Flux cannot deploy the folder either. So +// the folder is refused, and no document in it is routable. +// +// This used to be accepted and merely guarded: the hand-written walk recorded both +// paths and attached no overrides, leaving the write-fan-in precondition to refuse +// the write later. Refusing the unbuildable folder up front is strictly stronger, +// and it is what the renderer tells us for free. +func TestKustomizeOverridesCorpus_DiamondIsUnbuildable(t *testing.T) { store := corpusStore(t, "unsupported/diamond-images") dm := corpusDeployment(t, store) if dm.Overrides != nil { - t.Errorf("a diamond's conflicting chains must attach no overrides, got %+v", dm.Overrides) + t.Errorf("a folder kustomize cannot build must attach no overrides, got %+v", dm.Overrides) } - if !hasOverrideAmbiguityDiag(store) { - t.Errorf("want an %s diagnostic for the diamond", reasonAmbiguousOverrides) + if !hasRenderFailure(store) { + t.Errorf("want a %s diagnostic naming kustomize's build error", reasonRenderFailed) + } +} + +// TestAccept_UnbuildableRenderRootRefusesTheFolder is the other half: the diamond is +// not merely unrouted, it is REFUSED. A folder whose render root kustomize cannot +// build is one no GitOps controller can deploy, and one whose renders we cannot +// reason about — so the operator will not write into it. +func TestAccept_UnbuildableRenderRootRefusesTheFolder(t *testing.T) { + fsys := os.DirFS(filepath.Join("testdata", "contextual-namespace", "unsupported", "diamond-images")) + _, acc := acceptanceOf(t, fsys, snapMapper(), AcceptancePolicy{Allowlist: DefaultAllowlist()}) + if acc.Accepted { + t.Fatalf("a render root kustomize cannot build must refuse the folder") + } + found := false + for _, iss := range acc.Issues { + if iss.Kind == IssueUnsupportedKustomize { + found = true + } + } + if !found { + t.Errorf("want an %s issue, got %+v", IssueUnsupportedKustomize, acc.Issues) } } @@ -93,30 +123,42 @@ func TestKustomizeOverridesCorpus_AmbiguousImages(t *testing.T) { } } -// TestOverridesAmbiguousAt pins the store-side signal the writer's write-fan-in precondition -// consults: the shared document a diamond reaches two ways reports ambiguous, a build -// directive / unknown path does not, and a store with no ambiguous chain never reports it. +// TestOverridesAmbiguousAt pins the store-side signal the writer's write-fan-in +// precondition consults: the shared document TWO render roots reach with differing +// chains reports ambiguous, a build directive / unknown path does not, and a store +// with no ambiguous chain never reports it. +// +// The fixture is ambiguous-images, not the diamond: the diamond does not build at +// all now, so it is refused before any write is planned, while ambiguous-images +// builds cleanly from both roots and disagrees — which is exactly the shape the +// fan-in guard exists for. func TestOverridesAmbiguousAt(t *testing.T) { - diamond := corpusStore(t, "unsupported/diamond-images") - if !diamond.OverridesAmbiguousAt("base/deployment.yaml") { - t.Errorf("the diamond's shared base/deployment.yaml must report an ambiguous override chain") + shared := corpusStore(t, "unsupported/ambiguous-images") + if !shared.OverridesAmbiguousAt("shared.yaml") { + t.Errorf("the document two roots reach with differing chains must report ambiguous") } - if diamond.OverridesAmbiguousAt("base/kustomization.yaml") { + if shared.OverridesAmbiguousAt("kustomization.yaml") { t.Errorf("a build directive is not an ambiguous managed write path") } - if diamond.OverridesAmbiguousAt("no/such/file.yaml") { - t.Errorf("an unknown path must not report ambiguity") + if shared.OverridesAmbiguousAt("no/such/file.yaml") { + t.Errorf("an unknown path must not report ambiguous") } - clean := corpusStore(t, "supported/images-overlay") - if clean.OverridesAmbiguousAt("base/deployment.yaml") { - t.Errorf("a store with no ambiguous chain must never report ambiguity") + if clean.OverridesAmbiguousAt("deployment.yaml") { + t.Errorf("a single unambiguous chain must never report ambiguous") + } +} + +// hasRenderFailure reports whether the store recorded a kustomize build failure. +func hasRenderFailure(store *ManifestStore) bool { + for _, d := range store.Diagnostics { + if d.Reason == reasonRenderFailed { + return true + } } + return false } -// TestKustomizeOverridesNestedBaseIsNotARoot pins the render-root rule: a base -// referenced by another kustomization is not walked as its own root, so the -// nested layout yields ONE chain (base+parent composed), not two conflicting ones. func TestKustomizeOverridesNestedBaseIsNotARoot(t *testing.T) { store := corpusStore(t, "supported/replicas-overlay") for _, d := range store.Diagnostics { diff --git a/internal/manifestanalyzer/scan_repo.go b/internal/manifestanalyzer/scan_repo.go index ab980ca0..dc414c57 100644 --- a/internal/manifestanalyzer/scan_repo.go +++ b/internal/manifestanalyzer/scan_repo.go @@ -671,8 +671,9 @@ func pathWithin(p, dir string) bool { return p == dir || strings.HasPrefix(p, dir+"/") } -// sortedKeysOf returns the sorted keys of a string set. -func sortedKeysOf(m map[string]struct{}) []string { +// sortedKeysOf returns the sorted keys of a string-keyed map, so a walk over it emits +// in deterministic order. +func sortedKeysOf[V any](m map[string]V) []string { out := make([]string, 0, len(m)) for k := range m { out = append(out, k) diff --git a/internal/manifestanalyzer/store.go b/internal/manifestanalyzer/store.go index d43546f1..495785b2 100644 --- a/internal/manifestanalyzer/store.go +++ b/internal/manifestanalyzer/store.go @@ -107,12 +107,19 @@ type RetainedDocument struct { Location manifestedit.Location Identity manifestedit.Identity GVK schema.GroupVersionKind - // Unsupported is true for a whole-file kustomization retention that uses a feature - // outside the supported contextual-namespace subset (generators / patches / - // components / helm / replacements / transformers / name(pre|suf)fix / remote bases). - // The operator cannot map such a folder back to editable source documents, so the - // acceptance gate refuses it (IssueUnsupportedKustomize) rather than writing into - // content it cannot safely manage. Only ever set on a whole-file retention. + // Unsupported is true for a whole-file kustomization retention that the operator + // cannot map back to editable source documents, for either of two reasons: + // + // - it uses a feature outside the supported contextual-namespace subset + // (generators / patches / components / helm / replacements / transformers / + // name(pre|suf)fix / remote bases), or declares malformed images/replicas; or + // - it is a render root KUSTOMIZE CANNOT BUILD (reasonRenderFailed). If the build + // fails, Flux cannot deploy the folder either, and we cannot know what it renders + // to — and a silent pass would be worse than useless, because a root that yields + // no chain also yields no ambiguity, which disarms the write-fan-in guard. + // + // The acceptance gate refuses either (IssueUnsupportedKustomize) rather than writing + // into content it cannot safely manage. Only ever set on a whole-file retention. Unsupported bool } @@ -363,7 +370,7 @@ func buildStore( kusts := parseKustomizations(yamlFiles) resourceFiles := resourceFilePaths(yamlFiles) nsAssignments := kustomizeNamespaceAssignments(kusts, resourceFiles) - ovAssignments := kustomizeOverrideAssignments(kusts, resourceFiles) + ovAssignments, renderFailures := renderChains(yamlFiles, kusts) store := &ManifestStore{ FilesByPath: map[string]*FileModel{}, @@ -402,14 +409,32 @@ func buildStore( // so it is known to acceptance (and shown) but never becomes a FileModel. for _, f := range yamlFiles { if allowlist.Allows(f.Path) && !hasNamedRecord[f.Path] { + // A render root kustomize cannot BUILD is unsupported too, not just one + // declaring a feature we do not model: if the build fails, Flux cannot + // deploy the folder, and we cannot know what it renders to. Refusing is + // the only honest answer — and a silent pass would disarm the + // write-fan-in guard, which needs the render to see the shared file. + _, buildFailed := renderFailures[filepathToSlash(f.Path)] store.Retained = append(store.Retained, RetainedDocument{ - Location: manifestedit.Location{Path: f.Path}, - Unsupported: isKustomizationFile(f.Path) && kustomizationUsesUnsupportedFeature(f.Content), + Location: manifestedit.Location{Path: f.Path}, + Unsupported: isKustomizationFile(f.Path) && + (kustomizationUsesUnsupportedFeature(f.Content) || buildFailed), }) } } sortRetained(store.Retained) + // Say WHY a build failed, in the store's diagnostics: "refused-structural" on its + // own is not something a user can act on, and kustomize's error usually is. + for _, path := range sortedKeysOf(renderFailures) { + store.Diagnostics = append(store.Diagnostics, manifestedit.Diagnostic{ + Level: manifestedit.DiagWarning, + Reason: reasonRenderFailed, + Message: renderFailures[path], + Path: path, + }) + } + return store } @@ -464,14 +489,14 @@ func (s *ManifestStore) materialize( r manifestedit.DocumentRecord, lookup typeset.Lookup, nsAssignments map[string]namespaceAssignment, - ovAssignments map[string]*overrideAssignment, + ovAssignments map[chainKey]*overrideAssignment, ) { gvk := gvkOf(r.Identity) identity, nsSource, diag := resolveNamespaceContext(ctx, r.Identity, gvk, lookup, r.Location, nsAssignments) if diag != nil { s.Diagnostics = append(s.Diagnostics, *diag) } - overrides, ovDiag := resolveOverrides(r.Location, ovAssignments) + overrides, ovDiag := resolveOverrides(r.Location, r.Identity, ovAssignments) if ovDiag != nil { s.Diagnostics = append(s.Diagnostics, *ovDiag) }