From 5d77e3976b32c0414e6b1082407afebaa4b5dbc4 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 17:03:08 +0000 Subject: [PATCH 01/13] docs(support-boundary): record what building the kustomize observer patch changed The patch argued for in patching-kustomize.md has now been built and measured on feat/build-trace-observer in external-sources/kustomize (290d04199, on top of upstream 79bb1aa2b): 638 lines added across 11 files, upstream's api suite still green. Building it corrected the doc on three points, now recorded in a new section 9: - the patch should be an *additive* krusty.Options.Observer, not a change to the transformer annotation's semantics -- no golden churn, no kubectl output change, and it carries the entry index and field paths the annotation structurally cannot; - the zero-cost gate is structural rather than merely safe, and is now asserted by TestBuildTraceDoesNotChangeTheBuild; - two facts only the running code produced: matching a resource across a rename needs an EffectiveNamespace-normalised key, and ConfiguredIn is relative to the build root rather than the repo root. The measured trace also settles section 3's headline claim by test rather than by argument: an idempotent pin produces no event at all, which is the case the dye was invented for and the one leave-one-out probing cannot answer. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../support-boundary/patching-kustomize.md | 66 ++++++++++++++++++- 1 file changed, 63 insertions(+), 3 deletions(-) diff --git a/docs/design/support-boundary/patching-kustomize.md b/docs/design/support-boundary/patching-kustomize.md index 8f96ef30..45c65d3c 100644 --- a/docs/design/support-boundary/patching-kustomize.md +++ b/docs/design/support-boundary/patching-kustomize.md @@ -15,6 +15,12 @@ > 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. +> **Status: built and measured**, on branch `feat/build-trace-observer` in +> `external-sources/kustomize` (commit `290d04199`, on top of upstream `79bb1aa2b`). +> **638 lines added, 9 changed, across 11 files.** The full upstream `api` test suite passes: +> the only six failures are identical on the unpatched base commit (they need a container +> runtime, the network, or version stamping). See §9 for what the measurement changed. + 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`. @@ -229,9 +235,9 @@ 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). +1. **Carry the patch** (`replace` directive) — an *additive* observer, not a change to the + annotation's semantics (§9). 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 @@ -246,6 +252,60 @@ So: propose it, but do not plan around it landing. 5. **Demote the dye** from *the* attribution mechanism to the fallback for an unpatched build — and keep its verification half permanently. +## 9. What building it changed + +Three things the argument above got wrong, or only half-right. + +**The patch should be additive, not a semantics change.** §2 proposed making the transformer +annotation *mean* "changed". That was the wrong instinct: it rewrites goldens, it changes +`kubectl kustomize` output, and it turns a cheap patch into a standards fight (§7) — all to +deliver strictly *less* than the alternative. What shipped instead is a new +`krusty.Options.Observer`, called once per (transformer instance, resource) pair that the +transformer actually altered. It changes **no** existing behaviour, so the entire upstream +test suite passes untouched, and it carries the field paths and entry index that the +annotation could never carry anyway. **The annotation is left exactly as it is** — we simply +stop needing it. That also collapses §7's hardest objection: an additive option with no +golden churn is a far easier upstream conversation than a semantics change to an alpha +annotation vendored into `kubectl`. + +**The zero-cost gate is real, and it fell out for free.** An observer needs origins (they +carry the `ConfiguredIn` path), so setting one turns origin tracking on internally — and +`Kustomizer.Run` *already* strips the origin and transformer annotations unless +`buildMetadata` asked for them. So an observed build renders byte-identically to an +unobserved one with no extra work, and an unobserved build does no snapshotting and no +diffing at all (one nil check per transformer). §5 argued this was *safe*; it is in fact +*structural*, and it is now asserted by a test that fails loudly if it ever stops being +true (`TestBuildTraceDoesNotChangeTheBuild`). + +**Two facts only the running code produced:** + +- **Renames need a normalised key.** Matching a resource across a transformer by + `ResId.String()` does not work. `StorePreviousId` records a resource's *effective* + namespace (`default`), while `CurId` on an unnamespaced resource has *none* (`[noNs]`) — + so the id a resource had before a rename and the id it has after disagree on how to spell + "no namespace", and a `namePrefix` reads as a deletion plus a creation rather than a + rename. Normalising both sides through `EffectiveNamespace` fixes it. Nothing in the + source says this; the first test run did. +- **`ConfiguredIn` is relative to the build root, not the repo root.** An overlay's own + kustomization comes back as plain `kustomization.yaml`. Any consumer building several + roots must re-root these paths itself. + +And the headline claim of §3 now has a test rather than an argument behind it. For a base +pinned at `web:v1` with an overlay carrying `images: [web→v2, redis→6.2]` and +`replicas: [web→3]`, the observed trace is exactly two events: + +``` +kustomization.yaml ReplicaCountTransformer[0] Deployment/web spec.replicas: 1 -> 3 +kustomization.yaml ImageTagTransformer[0] Deployment/web spec.template.spec.containers[0].image: web:v1 -> web:v2 +``` + +The `Service` is not mentioned, though the annotation names both transformers on it. The +sidecar container is not mentioned. And `images[1]` — the **idempotent pin**, `redis:6.2` +over a base already at `redis:6.2` — produces **no event at all**, because it changed +nothing. That is the honest answer, it is the one leave-one-out probing structurally cannot +give ([render-attribution.md](render-attribution.md) §2), and it is the case that motivated +the dye. + ## Still open - **Does the trace escape the process, or stay a side-channel?** Annotating per-field From 4b0c7b1e6b047b52e0f3b393763447ff1ab9928a Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 17:13:45 +0000 Subject: [PATCH 02/13] fix(kustomize): a kustomization.yml render root gets its provenance renderFilesystem asked for buildMetadata only on a root spelled exactly "kustomization.yaml". Every other part of the analyzer accepts kustomization.yml too (isKustomizationFile), so a repository whose root is spelled .yml built happily with no buildMetadata at all: no config.kubernetes.io/origin, no alpha.config.kubernetes.io/transformations. Downstream that is not a missing annotation, it is a wrong answer. Every rendered object came back with an empty OriginPath and an empty transformation list, which reads as "this document is governed by no kustomization" -- so the images:/replicas: override chain was empty, and a live tag bump governed by an entry was written into the SOURCE MANIFEST, where the overlay shadows it straight back on the next render. Non-converging drift, silently, on every reconcile. Measured against main by the new test, which fails there with OriginPath="" and an empty chain. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/manifestanalyzer/kustomize_render.go | 11 +++++++++-- .../kustomize_render_hostile_test.go | 16 ++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/internal/manifestanalyzer/kustomize_render.go b/internal/manifestanalyzer/kustomize_render.go index d3a913af..ab99e9cf 100644 --- a/internal/manifestanalyzer/kustomize_render.go +++ b/internal/manifestanalyzer/kustomize_render.go @@ -223,7 +223,6 @@ func unbuildable(k *kustomizationDoc) error { // for provenance. func renderFilesystem(files []manifestedit.FileContent, rootDir string) (filesys.FileSystem, error) { fSys := filesys.MakeFsInMemory() - rootKust := path.Join(rootDir, "kustomization.yaml") for _, f := range files { rel := filepathToSlash(f.Path) @@ -231,7 +230,15 @@ func renderFilesystem(files []manifestedit.FileContent, rootDir string) (filesys // Only the root needs to ask for provenance: the annotations describe the // whole build, bases included. - if rel == rootKust { + // + // The root is matched the way the rest of the analyzer matches one — + // isKustomizationFile, which accepts kustomization.yml as well. Comparing + // against the literal "kustomization.yaml" instead left a .yml root building + // happily with NO buildMetadata: no origin, no transformations, so every + // object came back with an empty OriginPath and an empty override chain, and + // a governed image was written into the source manifest for the overlay to + // shadow straight back. + if slashDir(rel) == rootDir && isKustomizationFile(rel) { var k kustypes.Kustomization if err := k.Unmarshal(content); err != nil { return nil, fmt.Errorf("%s: %w", rel, err) diff --git a/internal/manifestanalyzer/kustomize_render_hostile_test.go b/internal/manifestanalyzer/kustomize_render_hostile_test.go index d2a8068b..13c5f90e 100644 --- a/internal/manifestanalyzer/kustomize_render_hostile_test.go +++ b/internal/manifestanalyzer/kustomize_render_hostile_test.go @@ -45,6 +45,22 @@ func TestRenderRoot_ValidRegexImageNameStillBuilds(t *testing.T) { require.Equal(t, "nginx:2.0", slots[0].image) } +// kustomization.yml is a build directive everywhere else in the analyzer, so it has to be +// one here too. Matching the root against the literal "kustomization.yaml" left a .yml root +// building with no buildMetadata at all: the objects came back with no origin and no +// transformations, which reads downstream as "this file is governed by nothing". +func TestRenderRoot_KustomizationYMLCarriesProvenance(t *testing.T) { + files := imageFixture("nginx:v1", " - name: nginx\n newTag: v2\n") + files[1].Path = "kustomization.yml" + + rendered, err := renderRoot(files, ".") + + require.NoError(t, err) + require.Len(t, rendered, 1) + require.Equal(t, "deployment.yaml", rendered[0].OriginPath, "the source file must be attributable") + require.NotEmpty(t, rendered[0].TransformedBy, "the override chain must be readable") +} + // 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. From 78919492079abfdb90ed82052cf8f5923ac4673c Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 17:14:37 +0000 Subject: [PATCH 03/13] refactor(kustomize): renderRootWith, the counterfactual render renderRoot becomes renderRootWith over an empty overlay. The new call builds a render root with in-memory replacements layered over the scanned files, so a question about what kustomize would do with a DIFFERENT tree goes through the same sandbox, the same pre-build refusals and the same krusty invocation as the render we ship. It is the primitive the rest of this workstream is made of. Both remaining questions are this call with a different overlay: dye an entry and see where the nonce lands (attribution), or apply a proposed write and see what it renders (verification). No behaviour change on its own: every existing caller renders with no overlay. See docs/design/support-boundary/render-attribution.md section 7. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/manifestanalyzer/kustomize_render.go | 47 ++++++++++++++++++- .../kustomize_render_hostile_test.go | 30 ++++++++++++ 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/internal/manifestanalyzer/kustomize_render.go b/internal/manifestanalyzer/kustomize_render.go index ab99e9cf..195031f3 100644 --- a/internal/manifestanalyzer/kustomize_render.go +++ b/internal/manifestanalyzer/kustomize_render.go @@ -124,10 +124,34 @@ 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 := refuseBeforeBuild(parseKustomizations(files), rootDir); err != nil { + return renderRootWith(files, rootDir, nil) +} + +// renderRootWith is renderRoot over a COUNTERFACTUAL tree: replace layers in-memory +// content for any scanned path, and the build sees that instead of what the scan holds. +// +// It is the whole new API this workstream needs, and every question we ask kustomize is +// this call with a different overlay: +// +// - dye an entry and see where the nonce lands -> attribution; +// - apply a proposed write and see what it renders -> verification. +// +// The point is that the counterfactual goes through the SAME sandbox, the same refusals +// and the same kustomize invocation as the real render. A question answered by a +// different renderer than the one that produces the answer we ship is not an answer. +// +// files is never mutated: callers hold the scan, and a probe must not be able to corrupt it. +// See docs/design/support-boundary/render-attribution.md §7. +func renderRootWith( + files []manifestedit.FileContent, + rootDir string, + replace map[string][]byte, +) ([]renderedObject, error) { + input := replacedFiles(files, replace) + if err := refuseBeforeBuild(parseKustomizations(input), rootDir); err != nil { return nil, err } - fSys, err := renderFilesystem(files, rootDir) + fSys, err := renderFilesystem(input, rootDir) if err != nil { return nil, err } @@ -138,6 +162,25 @@ func renderRoot(files []manifestedit.FileContent, rootDir string) ([]renderedObj return collectRendered(resMap, rootDir) } +// replacedFiles returns a copy of files with replace layered over it, keyed by slash path. +// +// A replacement for a path the scan does not hold is ignored rather than appended: a +// counterfactual may only perturb files that are actually in the tree kustomize is about +// to build, and inventing one would be a way to render something the repository does not +// contain. +func replacedFiles(files []manifestedit.FileContent, replace map[string][]byte) []manifestedit.FileContent { + if len(replace) == 0 { + return files + } + out := append([]manifestedit.FileContent(nil), files...) + for i := range out { + if content, ok := replace[filepathToSlash(out[i].Path)]; ok { + out[i].Content = content + } + } + return out +} + // 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 diff --git a/internal/manifestanalyzer/kustomize_render_hostile_test.go b/internal/manifestanalyzer/kustomize_render_hostile_test.go index 13c5f90e..d7410153 100644 --- a/internal/manifestanalyzer/kustomize_render_hostile_test.go +++ b/internal/manifestanalyzer/kustomize_render_hostile_test.go @@ -45,6 +45,36 @@ func TestRenderRoot_ValidRegexImageNameStillBuilds(t *testing.T) { require.Equal(t, "nginx:2.0", slots[0].image) } +// The counterfactual render must go through the same renderer as the baseline, and must not +// be able to corrupt the scan it is probing. Both halves matter: attribution and verification +// are only worth anything if the question is asked of the renderer that gives the answer. +func TestRenderRootWith_RendersTheReplacementAndLeavesTheScanAlone(t *testing.T) { + files := imageFixture("nginx:v1", " - name: nginx\n newTag: v2\n") + replacement := []byte("resources:\n - deployment.yaml\nimages:\n - name: nginx\n newTag: v3\n") + + rendered, err := renderRootWith(files, ".", map[string][]byte{"kustomization.yaml": replacement}) + + require.NoError(t, err) + require.Len(t, rendered, 1) + slots := collectContainerSlots(rendered[0].Object.Object) + require.Len(t, slots, 1) + require.Equal(t, "nginx:v3", slots[0].image, "the build must see the counterfactual") + require.Contains(t, string(files[1].Content), "newTag: v2", "the scan must survive the probe unchanged") +} + +// A replacement for a path the scan does not hold is ignored, never appended: a probe may +// perturb the tree kustomize builds, not invent a file the repository does not contain. +func TestRenderRootWith_IgnoresAReplacementForAnAbsentPath(t *testing.T) { + files := imageFixture("nginx:v1", " - name: nginx\n newTag: v2\n") + + rendered, err := renderRootWith(files, ".", map[string][]byte{ + "not/in/the/scan.yaml": []byte("apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: ghost\n"), + }) + + require.NoError(t, err) + require.Len(t, rendered, 1, "the phantom file must not have been rendered") +} + // kustomization.yml is a build directive everywhere else in the analyzer, so it has to be // one here too. Matching the root against the literal "kustomization.yaml" left a .yml root // building with no buildMetadata at all: the objects came back with no origin and no From 476b0dd506fbf4a4240f1e42827eaf8e4f9e8139 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 17:31:39 +0000 Subject: [PATCH 04/13] fix(kustomize)!: verify a kustomize write by re-rendering it, not by simulating it simulateImageRender "verified" the projection's inversion by replaying OUR chain over the images OUR projection had planned. A check that shares the blind spot of the thing it checks cannot fail where that thing is wrong -- it agrees with itself -- so it did not catch a wrong attribution, it made one CONFIDENT. Both shipped image bugs (#231's digest/tag corruption, and the regex-vs-equality matcher) sailed straight through it. Replace it with the real thing. A flush that routes anything through a kustomization now re-renders every render root twice -- as it found the tree, and as it would leave it -- and proves both halves of the oracle: 1. every document the flush writes renders to exactly the live object, and 2. every object it does not write comes out byte-for-byte unchanged. (2) is the half that makes it safe for the projection to guess. A kustomization is shared context: an images: entry edited to converge one Deployment governs every other object it matches. A proposal that fixes its own target and moves a second object has written a live value into a file another render root also reads. It runs ONCE PER FLUSH, not once per resource, and that is correctness rather than economy. Asked resource by resource, "did anything else move?" is YES for the first of two Deployments that share an image and are bumped together -- so a per-resource oracle refuses a write that converges perfectly well. Only the whole batch can see that the sibling is moving because it, too, is being written to exactly where it now lives. That is why every document a flush touches declares a WriteIntent, and why a governed one declares it even when its own bytes did not change. A refused proposal REFUSES THE FLUSH -- an AcceptanceRefusedError surfacing as GitPathAccepted=False / WriteBoundaryRefused, naming the file and the object. It is not absorbed into a silent skip: a write that does not survive the re-render is one the entry overrides straight back on the next render, so absorbing it would leave the resource un-mirrored forever with nothing to show for it (render-attribution.md section 7). Two writes are declared UNCHECKED rather than compared, and it is stated rather than hidden: a SENSITIVE document (SOPS ciphertext -- no plaintext live object can equal it) and a bounded FIELD PATCH (a few audited assignments, never a whole object). Both are still held to disturbing nothing else. Attribution is untouched here and still comes from renderImage, so no routing changes. That is the point: the check now cannot share the blind spot of what it checks, which is what makes the attribution swap that follows safe to make at all. See docs/design/support-boundary/render-attribution.md section 5 and docs/design/support-boundary/render-root-scoping.md section 3. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../support-boundary/render-plan-artifact.md | 237 +++++++++++++++++ internal/git/kustomize_oracle_test.go | 165 ++++++++++++ internal/git/plan_flush.go | 179 ++++++++++++- internal/manifestanalyzer/acceptance.go | 14 +- .../manifestanalyzer/overrides_projection.go | 43 +--- internal/manifestanalyzer/render_verify.go | 241 ++++++++++++++++++ internal/watch/event_router.go | 11 +- 7 files changed, 850 insertions(+), 40 deletions(-) create mode 100644 docs/design/support-boundary/render-plan-artifact.md create mode 100644 internal/git/kustomize_oracle_test.go create mode 100644 internal/manifestanalyzer/render_verify.go diff --git a/docs/design/support-boundary/render-plan-artifact.md b/docs/design/support-boundary/render-plan-artifact.md new file mode 100644 index 00000000..5641a6fd --- /dev/null +++ b/docs/design/support-boundary/render-plan-artifact.md @@ -0,0 +1,237 @@ +# The render plan: attribution as an artifact, not a capability + +> **design** — direction-setting; ships no code. Nothing it describes is supported today. +> Captured: 2026-07-14 +> Related: +> [patching-kustomize.md](patching-kustomize.md), +> [render-attribution.md](render-attribution.md), +> [render-root-scoping.md](render-root-scoping.md), +> [generated-repo-map.md](generated-repo-map.md), +> [support-contract.md](support-contract.md), +> [acceptance-precision.md](acceptance-precision.md) + +The fork ([patching-kustomize.md](patching-kustomize.md)) can tell us exactly which +kustomization entry supplied which field. But carrying a forked kustomize in the operator — +on the reconciliation hot path, in the same process that must render byte-identically to +the user's controller — is a cost we would rather not pay, and a `replace` directive does +not even survive being consumed as a library (§6 there). + +So don't. **Run the fork offline, once, and emit a file.** + +The operator links stock kustomize and reads the file. Attribution stops being something +the operator can *do* and becomes something it can *look up*. This doc is about what that +file has to contain to be safe, and about the one way of monetising it that does not +destroy the product. + +## 1. The shape + +```mermaid +flowchart LR + subgraph offline["analyse run — the fork lives HERE, and only here"] + A["patched kustomize
(buildtrace.Observer)"] --> P["render plan
(the artifact)"] + end + subgraph hot["operator — stock kustomize, no fork"] + P --> PROP["propose an edit"] + PROP --> OR["the oracle:
re-render, require the live
object reproduced exactly"] + OR -->|reproduces| W["write"] + OR -->|does not| R["refuse"] + end + + classDef editable fill:#dfd,stroke:#3a3,color:#111 + classDef refused fill:#fdd,stroke:#c33,color:#111 + class A,P,W editable + class R refused +``` + +Three properties fall out of this and they are the whole argument: + +- **The fork leaves the hot path.** The operator's render fidelity against Flux is + unimpeachable, because the operator renders with the same stock library Flux does. +- **The blast radius of the fork shrinks to one offline tool.** It is no longer linked into + the thing that writes to people's repositories. +- **The plan is a *hint*, never an authority.** See §2, which is the load-bearing section. + +## 2. The plan proposes; the oracle disposes + +The instinct that a precomputed plan is dangerous is correct, and the reason is staleness: +a plan describes how a repo rendered at commit *X*, and **our own writes produce commit +X+1**. Every write we make invalidates the plan that authorised it. + +That would be fatal — *if the plan were trusted*. It must not be. + +The oracle from [render-root-scoping.md](render-root-scoping.md) §3 already exists in the +design and, crucially, **needs no plan and no fork**: propose a source edit, re-render the +render root with stock kustomize, and require that the proposal reproduces the live object +exactly *and leaves every other object byte-identical*. That check is what the operator +already has to do anyway. + +So the plan only ever generates a *proposal*, and a wrong proposal is *caught*: + +| Plan state | Proposal | Oracle | Outcome | +|---|---|---|---| +| correct | right file, right entry | reproduces | **write** | +| stale / wrong | wrong file or wrong entry | does not reproduce | **refuse** | +| missing | fall back to the dye | either | write or refuse | + +**A bad plan degrades to a refusal, never to a corruption.** That converts staleness from a +correctness problem into an availability problem, and it is the single property that makes +this whole scheme safe to ship. It also means the plan does not have to be *perfect* — it +has to be *verifiable*, which is a far weaker and far more achievable requirement. + +This is the same reason [render-attribution.md](render-attribution.md) §5 insists that +attribution may be heuristic but verification may not. The plan is the heuristic. The +oracle is the verification. **Never let the plan be both.** + +## 3. Even so: fingerprint the inputs + +The oracle makes a stale plan safe. It does not make it *free* — every stale-plan proposal +costs a full re-render and ends in a refusal the user did not deserve. So the plan must be +able to say "I no longer describe this repo" without being run through the oracle to find +out. + +That means the plan is keyed by **a content hash of every input it depends on**: every file +in the render root's read scope, including the bases outside the root that the overlay +reaches into. Not the git commit — the *content* — because the operator writes to a branch +and the plan must remain valid for the files it did not touch. + +This is `go.sum`, and it fails the same way if you skip it: **a lockfile nobody verifies is +not a lockfile, it is a rumour.** A plan whose fingerprint does not match the repo it is +being applied to must be treated as absent (fall back to the dye), not as approximately +right. + +## 4. The tier must gate scope, not accuracy + +The commercial instinct — free tier gets the dye, paid tier gets the plan — is right in +outline and lethal in one specific formulation. + +**The trap.** The dye is *sound only for pure sinks* +([render-attribution.md](render-attribution.md) §3): it cannot attribute `newName`, it is +blind to an idempotent pin, and it says nothing about `patches:`. If the free tier *guesses* +in those cases while the paid tier *knows*, then correctness is the paid feature. The first +free-tier user whose base gets silently corrupted takes the paid tier's reputation with +them. You cannot sell "we write to the right file" as an upgrade from "we write to a file." + +**The fix, and it is one word.** Both tiers are **correct**. The paid tier is more +**capable**: + +| | Free (dye) | Paid (plan) | +|---|---|---| +| `images` / `replicas` pure sinks | **edits** | **edits** | +| idempotent pin, `newName` | **refuses** | **edits** | +| `patches:`, components | **refuses** | **edits** | +| ever writes the wrong file | **no** | **no** | + +What the customer buys is **a bigger support boundary, not a more accurate one.** The free +tier never lies; it says *"I cannot edit this."* The paid tier says *"I can."* That is a +defensible upsell precisely because it is the product's existing ethos — refusal is already +our honest answer, and [support-contract.md](support-contract.md) is built on it. The plan +turns refusals into edits. It must never turn refusals into guesses. + +A corollary worth stating, because it is a real constraint on the open-source operator: **the +operator must be fully correct with no plan present.** The plan is an enrichment. If the +operator's correctness depends on a file only paying customers get, the open-source project +is a trap, and it will be treated as one. + +## 5. What is in the plan + +Not the render. The **inverse** of the render — which is the thing that does not otherwise +exist ([generated-repo-map.md](generated-repo-map.md) §2, tier 4). + +```yaml +# renderplan.v1 +renderRoot: overlays/prod +inputs: # §3 — every file in the READ scope, not just the subtree + base/deployment.yaml: sha256:... + base/kustomization.yaml: sha256:... + overlays/prod/kustomization.yaml: sha256:... +objects: + - id: apps/v1/Deployment/prod/web + origin: base/deployment.yaml # exact, from kustomize + fields: + spec.replicas: + source: {kind: entry, file: overlays/prod/kustomization.yaml, stanza: replicas, index: 0} + spec.template.spec.containers[0].image: + source: {kind: entry, file: overlays/prod/kustomization.yaml, stanza: images, index: 0} + spec.template.spec.containers[1].image: + source: {kind: file, file: base/deployment.yaml} # no entry changed it +unattributable: # §6 — the plan MUST record its own gaps + - object: apps/v1/Deployment/prod/web + field: spec.template.metadata.labels.version + reason: changed-by-transformer-we-cannot-invert + transformer: PatchStrategicMergeTransformer +``` + +Two things about this schema are not negotiable. + +**It records what it could NOT attribute.** A plan that lists only its successes is +indistinguishable from a plan that is incomplete, and the operator cannot tell "this field +has no override" from "this field's override was not understood". The first is editable in +place; the second must be refused. `unattributable` is what keeps those apart. + +**Entry references are (file, stanza, index).** Not a transformer kind — kustomize builds +one transformer instance per entry, so the index is the only thing that identifies *which* +`images:` entry, and it is exactly what the observer emits. + +## 6. One artifact, three consumers + +This is why it is worth building once rather than three times. The plan is the serialized +tier-4 graph, and everything downstream is a *rendering* of it: + +| Consumer | Uses | +|---|---| +| **The writer** | `fields[].source` — route each changed field to a file or an entry (and refuse on `unattributable`). | +| **The diagram** ([generated-repo-map.md](generated-repo-map.md)) | the whole thing — this *is* the graph, and "renders graphically" is a viewer over this file, not a second pipeline. | +| **The metrics** | entries that appear in no `fields[].source` are **dead configuration**; `unattributable[]` grouped by transformer measures **the support boundary itself**, across every repo we see. | + +That last one is the sleeper. Every other question in these docs — *should we support +`patches:`? how common are components, really?* — is currently argued from fixtures and +intuition. `unattributable[]` answers it with a count from real repositories. + +## 7. Where does it live? + +Genuinely open, and the two options trade differently. + +**Committed to the customer's repo.** It is reviewable, it diffs in a pull request (*"this +change alters how your repo renders"*), and it is versioned alongside the content it +describes, which makes §3's fingerprint check natural. But our own writes churn it, and +there is a self-collision worth noticing: **a stray file in a GitTarget folder is currently +grounds for refusing the whole folder** ([acceptance-precision.md](acceptance-precision.md) +§1). We would be adding a file that trips our own acceptance gate. It must go on the inert +allowlist (`DefaultAllowlist`) in the same change, or we ship a product that refuses repos +because of a file it wrote itself. + +**Out of band** (object storage, a CR, the GitTarget status). No repo churn, no acceptance +collision — but invisible to the user, and it loses the "explain my repo in a PR" value that +is half the point. + +Leaning committed-in-repo, precisely *because* it is visible: the artifact that explains the +repo is worth more in the repo than in our database. + +## 8. Order of work + +1. **The oracle first, and unconditionally.** It is what makes every later step safe, it is + needed by the dye path anyway, and it needs no fork and no plan. Nothing else here should + be built before it. +2. **The plan schema + the fingerprint** (§3, §5), with `unattributable` from day one. +3. **The offline emitter** — the analyse run, linking the fork, producing the file. +4. **The operator reads it**, falling back to the dye when it is absent or its fingerprint + does not match. +5. **The diagram and the metrics**, which are then free (§6). + +## Still open + +- **What regenerates the plan after we write?** Our write invalidates the fingerprint of the + file we wrote. The cheapest honest answer is that the operator refuses the *next* edit to + that root until the analyse run reruns — correct, but a poor experience. The better answer + is probably that the operator can update the plan's fingerprint itself for a write whose + effect it fully understood (it just verified it with the oracle, after all), and only a + *foreign* change to the repo forces a full reanalysis. That is a nice property and it needs + proving, not asserting. +- **Does the plan cross the licence boundary cleanly?** The fork is Apache-2.0 (private use + is unrestricted; distributing a binary built from it requires the notices and a statement + of changes). The *plan* is our output and carries none of that. Running the fork as a + hosted analyse run and shipping only the file is the cleanest position, and it is worth + confirming with someone who is not me. +- **Is the plan per render root, or per repo?** Per root is simpler and matches the + fingerprint's read-scope. Per repo is what a diagram wants. Probably per root, with the + repo view being a join. diff --git a/internal/git/kustomize_oracle_test.go b/internal/git/kustomize_oracle_test.go new file mode 100644 index 00000000..3267c5fd --- /dev/null +++ b/internal/git/kustomize_oracle_test.go @@ -0,0 +1,165 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "os" + "path/filepath" + "testing" + + gogit "github.com/go-git/go-git/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" + "github.com/ConfigButler/gitops-reverser/internal/typeset" +) + +// flushEventsForTest is applyEventsViaPlanFlushWithMapper's sibling for the cases that are +// ABOUT the error: a refused flush returns one, and swallowing it with require.NoError +// would assert the opposite of the thing under test. +func flushEventsForTest( + t *testing.T, + writer *contentWriter, + worktree *gogit.Worktree, + mapper typeset.Lookup, + events ...Event, +) (bool, error) { + t.Helper() + w := &BranchWorker{contentWriter: writer, mapper: mapper} + return w.flushEventsToWorktree(context.Background(), worktree, "", events, nil) +} + +// Before a kustomize-governed write is committed, the repository is re-rendered WITH it +// and the result is required to be exactly the live object, with every other rendered +// object untouched. These are the tests for the refusal — the happy path is already +// covered by inplace_overrides_test.go, and a check that only ever says yes is not a +// check. +// +// See docs/design/support-boundary/render-attribution.md §5. + +// A base pinned at one tag, an overlay that shadows it, and a SECOND deployment the same +// images: entry also matches. Converging web's tag by editing the entry would drag api's +// image along with it — the entry is shared context. The oracle re-renders, sees api move, +// and refuses the whole write rather than committing a change to a resource nobody asked +// to change. +const sharedEntryKustomizationYAML = `apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +namespace: default +resources: + - apps/web.yaml + - apps/api.yaml +images: + - name: ghcr.io/example/shared + newTag: "1.0.0" +` + +func sharedImageDeploymentYAML(name string) string { + return `apiVersion: apps/v1 +kind: Deployment +metadata: + name: ` + name + ` +spec: + selector: + matchLabels: + app: ` + name + ` + template: + metadata: + labels: + app: ` + name + ` + spec: + containers: + - name: main + image: ghcr.io/example/shared:0.9.0 +` +} + +func seedSharedEntryWorktree(t *testing.T, root string) (string, string, string) { + t.Helper() + webPath := filepath.Join(root, "apps", "web.yaml") + apiPath := filepath.Join(root, "apps", "api.yaml") + kustPath := filepath.Join(root, "kustomization.yaml") + require.NoError(t, os.MkdirAll(filepath.Join(root, "apps"), 0o750)) + require.NoError(t, os.WriteFile(webPath, []byte(sharedImageDeploymentYAML("web")), 0o600)) + require.NoError(t, os.WriteFile(apiPath, []byte(sharedImageDeploymentYAML("api")), 0o600)) + require.NoError(t, os.WriteFile(kustPath, []byte(sharedEntryKustomizationYAML), 0o600)) + return webPath, apiPath, kustPath +} + +// sharedImageEvent is the live Deployment as the cluster holds it: the rendered form, +// which is what the source file plus the overlay's entry must reproduce. +func sharedImageEvent(name, image string) Event { + return Event{ + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{"name": name, "namespace": "default"}, + "spec": map[string]interface{}{ + "selector": map[string]interface{}{"matchLabels": map[string]interface{}{"app": name}}, + "template": map[string]interface{}{ + "metadata": map[string]interface{}{"labels": map[string]interface{}{"app": name}}, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{"name": "main", "image": image}, + }, + }, + }, + }, + }}, + Identifier: types.ResourceIdentifier{ + Group: "apps", Version: "v1", Resource: "deployments", Namespace: "default", Name: name, + }, + Operation: "UPDATE", + } +} + +// Routing web's new tag onto the entry would re-render api at that tag too, and api's live +// state says otherwise. The proposal is not isolated, so the flush is refused — loudly, as +// an AcceptanceRefusedError that reaches the GitTarget — and not one byte is written. +// +// A refusal, not a silent skip: a write that does not survive the re-render is one that +// would never converge, and absorbing it would leave the resource un-mirrored forever with +// nothing to show for it (render-attribution.md §7). +func TestPlanFlush_RefusesAWriteThatDragsASiblingAlong(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + webPath, apiPath, kustPath := seedSharedEntryWorktree(t, worktree.Filesystem.Root()) + + _, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), + sharedImageEvent("web", "ghcr.io/example/shared:2.0.0")) + + var refused *manifestanalyzer.AcceptanceRefusedError + require.ErrorAs(t, err, &refused, "the flush must be refused, and refused legibly") + assert.Contains(t, refused.Error(), "Deployment/api", + "the refusal must name the object the write would have dragged along") + assert.True(t, refused.AllIssuesOfKinds(manifestanalyzer.IssueRenderRefused), + "it is a write-boundary refusal, so it surfaces as WriteBoundaryRefused") + + assertFileBytes(t, kustPath, sharedEntryKustomizationYAML, + "the entry is shared context; editing it would move api too") + assertFileBytes(t, webPath, sharedImageDeploymentYAML("web"), "a refused flush writes nothing") + assertFileBytes(t, apiPath, sharedImageDeploymentYAML("api"), "and certainly leaves the sibling alone") +} + +// The same tag bump, but both deployments move together: now the entry edit reproduces +// BOTH live objects, nothing else drifts, and the write lands. This is the control — it is +// what proves the refusal above is discriminating rather than a blanket "no". +func TestPlanFlush_AllowsTheSharedEntryWriteWhenEverySiblingAgrees(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + webPath, apiPath, kustPath := seedSharedEntryWorktree(t, worktree.Filesystem.Root()) + + changed := applyEventsViaPlanFlushWithMapper(t, writer, worktree, deploymentMapper(), + sharedImageEvent("web", "ghcr.io/example/shared:2.0.0"), + sharedImageEvent("api", "ghcr.io/example/shared:2.0.0")) + + require.True(t, changed, "both siblings agree on the new tag; the entry edit is now isolated") + kust, err := os.ReadFile(kustPath) + require.NoError(t, err) + assert.Contains(t, string(kust), `newTag: "2.0.0"`, "the entry absorbs the tag both deployments now run") + assertFileBytes(t, webPath, sharedImageDeploymentYAML("web"), "the source manifests keep their bytes") + assertFileBytes(t, apiPath, sharedImageDeploymentYAML("api"), "the source manifests keep their bytes") +} diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index cc942296..eb278cf0 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -5,6 +5,7 @@ package git import ( "bytes" "context" + "errors" "fmt" "math" "os" @@ -74,6 +75,10 @@ type writeBatch struct { docLoc map[*manifestanalyzer.DocumentModel]manifestanalyzer.RecordRef contentByPath map[string][]byte buffers map[string]*fileBuffer + // intents records what each document this flush writes must render to, so the + // render precondition can tell a change the flush MEANT from one it merely caused. + // Anything not named here has to come out of the re-render untouched. + intents []manifestanalyzer.WriteIntent // policy is the GitTarget's declared new-file placement policy, consulted // only for a resource with no existing document. nil means no declared policy — // placement falls through to sibling inference and then the canonical path. @@ -356,6 +361,7 @@ func (wb *writeBatch) writeColdBundleMember( rebuilt = appendYAMLDocument(rebuilt, m.content) } wb.buffer(rel).current = rebuilt + wb.intend(markUnchecked(intentFor(event, rel, false), sensitive)) return upsertCreated, nil } @@ -384,6 +390,7 @@ func (wb *writeBatch) appendNewDocument(ctx context.Context, event Event, rel st } buf := wb.buffer(rel) buf.current = appendYAMLDocument(buf.current, content) + wb.intend(intentFor(event, rel, false)) return upsertCreated, nil } @@ -469,8 +476,16 @@ func (wb *writeBatch) applyFieldPatch(ctx context.Context, event Event) error { } assignments := event.FieldPatch.Assignments - if dm := wb.store.ByManifestIdentity[id]; dm != nil && dm.Overrides != nil { + dm := wb.store.ByManifestIdentity[id] + governed := dm != nil && dm.Overrides != nil + if governed { assignments = wb.routeGovernedFieldAssignments(ctx, event, dm, assignments) + // A routed scale changes only the kustomization entry, which still moves what this + // document renders to — so it must be declared, or the oracle would read its own + // intended write as collateral damage. It is UNCHECKED because a field patch carries + // a few audited assignments, never a whole object to compare the render against: the + // oracle can still prove the write disturbs nothing else, but not that it landed. + wb.intend(fieldPatchIntent(filePath, id, governed)) if len(assignments) == 0 { return nil } @@ -489,6 +504,9 @@ func (wb *writeBatch) applyFieldPatch(ctx context.Context, event Event) error { switch res.Mode { case manifestedit.EditPatched: buf.current = res.Content + if !governed { + wb.intend(fieldPatchIntent(filePath, id, false)) + } case manifestedit.EditNoChange, manifestedit.EditDeleted: // No-op: the audited value already matched (or, impossible here, a delete). case manifestedit.EditSkipped, manifestedit.EditWholeReplace: @@ -622,9 +640,154 @@ func (wb *writeBatch) patchExisting( if wb.applyOverrideEdits(ctx, event, overrideEdits) { outcome = upsertUpdated } + + // Declare what this document must render to. Attribution above decided WHERE the edit + // goes and is allowed to be wrong; the render precondition adjudicates it once the whole + // plan is known (see renderPrecondition). + // + // A GOVERNED document declares its intent even when its own bytes did not change, and + // that is not belt-and-braces — it is the difference between the oracle working and the + // oracle refusing perfectly good writes. An images: entry is shared: when two Deployments + // run the same image and are bumped together, the FIRST event's entry edit already moves + // what the second one renders to, so by the time the second is processed there is nothing + // left to write. Its render still moves, and it moves onto its own live state — that is + // the resource converging, not collateral damage, and only its declared intent says so. + if outcome == upsertUpdated || dm.Overrides != nil { + wb.intend(intentFor(event, filePath, dm.Overrides != nil)) + } return outcome, nil } +// renderPrecondition is the oracle, and it is a write-plan precondition like the three +// above it: it runs at the one moment the whole plan is known and before a single byte is +// touched, so a refusal aborts the flush and commits nothing. +// +// It only runs when the flush actually routed something through a kustomization. A repo +// with no override chain pays nothing, and a flush that changed no governed document has +// nothing for kustomize to adjudicate. +// +// A refusal is an AcceptanceRefusedError, which is the seam that carries it to the user as +// GitPathAccepted=False / Stalled=True with the file and object named. That is deliberate: +// render-attribution.md §7 is explicit that a proposal the renderer cannot vouch for +// "becomes a refused flush — that is the correct outcome and it must be reported, not +// absorbed." A resource we silently stop mirroring is the failure this path exists to +// prevent, so it must not be the failure this path introduces. +func (wb *writeBatch) renderPrecondition() error { + governed := false + for _, in := range wb.intents { + if in.Governed { + governed = true + break + } + } + if !governed { + return nil + } + + before := make([]manifestedit.FileContent, 0, len(wb.contentByPath)) + for _, path := range sortedContentKeys(wb.contentByPath) { + before = append(before, manifestedit.FileContent{Path: path, Content: wb.contentByPath[path]}) + } + + var refused *manifestanalyzer.RenderRefusedError + if err := manifestanalyzer.VerifyBatchRenders(before, wb.files(), wb.intents); err != nil { + if errors.As(err, &refused) { + issues := make([]manifestanalyzer.AcceptanceIssue, 0, len(refused.Reasons)) + for _, reason := range refused.Reasons { + issues = append(issues, manifestanalyzer.AcceptanceIssue{ + Kind: manifestanalyzer.IssueRenderRefused, + Message: reason, + }) + } + return &manifestanalyzer.AcceptanceRefusedError{Issues: issues} + } + return err + } + return nil +} + +// intend records what one document of this flush must render to, so the oracle can tell a +// change the flush MEANT from a change it merely caused. Everything not intended has to +// come out of the render untouched. +func (wb *writeBatch) intend(in manifestanalyzer.WriteIntent) { + if in.Kind == "" || in.Name == "" { + return // nothing addressable to check; the render comparison keys on kind+name + } + wb.intents = append(wb.intents, in) +} + +// intentFor builds the intent for an ordinary object-bearing write: the document must +// render to exactly the live object. +func intentFor(event Event, filePath string, governed bool) manifestanalyzer.WriteIntent { + desired := manifestreport.Project(event.Object) + return manifestanalyzer.WriteIntent{ + SourcePath: filePath, + Kind: desired.GetKind(), + Name: desired.GetName(), + Desired: desired, + Governed: governed, + } +} + +// unchecked marks a write whose rendered form cannot be predicted, so the oracle lets the +// object move without comparing it (but still holds the write to disturbing nothing else). +func markUnchecked(in manifestanalyzer.WriteIntent, unchecked bool) manifestanalyzer.WriteIntent { + if unchecked { + in.Unchecked = true + in.Desired = nil + } + return in +} + +// fieldPatchIntent declares a bounded field patch. It is always unchecked: the event carries +// a handful of audited assignments, not a whole object, so there is nothing to require the +// render to equal. +func fieldPatchIntent(filePath string, id manifestedit.Identity, governed bool) manifestanalyzer.WriteIntent { + return manifestanalyzer.WriteIntent{ + SourcePath: filePath, + Kind: id.Kind, + Name: id.Name, + Unchecked: true, + Governed: governed, + } +} + +// files is the batch's complete tree as the flush would leave it: the worktree bytes with +// every buffer folded over them, and deleted files removed. Sorted, so the render is +// reproducible. +func (wb *writeBatch) files() []manifestedit.FileContent { + byPath := make(map[string][]byte, len(wb.contentByPath)+len(wb.buffers)) + for path, content := range wb.contentByPath { + byPath[path] = content + } + for path, b := range wb.buffers { + if b.current == nil { + delete(byPath, path) // the flush deletes this file + continue + } + byPath[path] = b.current + } + paths := make([]string, 0, len(byPath)) + for path := range byPath { + paths = append(paths, path) + } + sort.Strings(paths) + out := make([]manifestedit.FileContent, 0, len(paths)) + for _, path := range paths { + out = append(out, manifestedit.FileContent{Path: path, Content: byPath[path]}) + } + return out +} + +func sortedContentKeys(m map[string][]byte) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} + // applyOverrideEdits folds routed override edits into their kustomization file // buffers, so they flush (and hit the .gittargetignore shadow precondition) // exactly like any other planned write. It reports whether any buffer changed. @@ -722,6 +885,11 @@ func (wb *writeBatch) writeWholeFile(ctx context.Context, event Event, rel strin } } buf.current = content + // A SENSITIVE document is written encrypted, so kustomize renders the SOPS ciphertext + // and no plaintext live object can ever equal it. Declare the write so the oracle does + // not read it as collateral damage, but mark it unchecked: we can still prove it + // disturbs nothing else, which is the half that protects other environments. + wb.intend(markUnchecked(intentFor(event, rel, false), wb.writer.isSensitiveIdentifier(event.Identifier))) if isNew { return upsertCreated, nil } @@ -747,6 +915,12 @@ func (wb *writeBatch) applyDelete(event Event) { if !ok { return } + wb.intend(manifestanalyzer.WriteIntent{ + SourcePath: target.filePath, + Kind: target.id.Kind, + Name: target.id.Name, + Removed: true, + }) res, _ := manifestedit.DeleteDocument(buf.current, idx) if res.FileEmpty { buf.current = nil @@ -841,6 +1015,9 @@ func (wb *writeBatch) flush(ctx context.Context, worktree *gogit.Worktree, root, if err := wb.fanInPrecondition(); err != nil { return false, err } + if err := wb.renderPrecondition(); err != nil { + return false, err + } logger := log.FromContext(ctx) changed := false for _, rel := range sortedBufferKeys(wb.buffers) { diff --git a/internal/manifestanalyzer/acceptance.go b/internal/manifestanalyzer/acceptance.go index 918442c4..9602aaf0 100644 --- a/internal/manifestanalyzer/acceptance.go +++ b/internal/manifestanalyzer/acceptance.go @@ -122,8 +122,20 @@ const ( // made explicit; the broader "any file shared by multiple render roots" generalization is // Per-render-root scoping would generalize this. IssueWriteFanIn IssueKind = "write-fan-in" + // IssueRenderRefused marks a planned write that kustomize itself will not vouch for: the + // flush was re-rendered with the write applied, and either the edited document did not + // come out as the live object, or the write moved an object it never set out to touch. + // + // It is the write-plan half of "attribution may be heuristic, verification may not" + // (docs/design/support-boundary/render-attribution.md §5). The projection is ALLOWED to + // guess which file an edit belongs in, precisely because this refuses the guess when the + // renderer disagrees. And it must refuse LOUDLY: a write that does not survive the + // re-render is one that would not converge — the entry overrides it straight back on the + // next render — so absorbing it would leave a resource silently un-mirrored forever, + // which is the exact failure this whole path exists to prevent. + IssueRenderRefused IssueKind = "kustomize-render-refused" - // A refusal made up purely of the two write-boundary kinds above surfaces as the GitTarget + // A refusal made up purely of the write-boundary kinds above surfaces as the GitTarget // reason WriteBoundaryRefused rather than the umbrella UnsupportedContent: the folder holds // nothing the operator cannot manage, the edit simply had nowhere safe to land. See the // watch package's gitPathRefusalReason. diff --git a/internal/manifestanalyzer/overrides_projection.go b/internal/manifestanalyzer/overrides_projection.go index 005566d5..d9b597f8 100644 --- a/internal/manifestanalyzer/overrides_projection.go +++ b/internal/manifestanalyzer/overrides_projection.go @@ -223,9 +223,13 @@ type slotPlan struct { // projectImages inverts the image chain for every container the live object and // the Git document share. It mutates out's container images to their // source-file form and returns the entry edits — or routes nothing (leaving the -// live values in out, today's write-through) when the inversion is unsafe: -// conflicting edits to one entry field, a removal an entry supplies, or a -// simulation that fails to reproduce live. +// live values in out) when the inversion is unsafe: conflicting edits to one entry +// field, or a removal an entry supplies. +// +// Nothing here is trusted. The proposal it produces is put to kustomize before it can +// become a commit (VerifyWriteProposal), so this only has to be a candidate that is +// usually right — and routing nothing is always a legal answer, because the re-render +// then adjudicates whatever the source document alone can carry. func projectImages( gitRaw map[string]interface{}, out *unstructured.Unstructured, @@ -252,7 +256,7 @@ func projectImages( plans = append(plans, plan) } edits, ok := collectConsistentEdits(plans) - if !ok || !simulateImageRender(plans, entries, edits) { + if !ok { return nil } for _, p := range plans { @@ -390,37 +394,6 @@ func collectConsistentEdits(plans []slotPlan) ([]OverrideEdit, bool) { return out, true } -// simulateImageRender verifies the whole inversion: with the edits applied to a -// copy of the entries, every planned source image must render exactly to its -// live image. Chained entries can interact (a newName edit can change which -// later entry matches), so this closes the loop the per-component logic cannot. -func simulateImageRender(plans []slotPlan, entries []ImageOverride, edits []OverrideEdit) bool { - sim := make([]ImageOverride, len(entries)) - copy(sim, entries) - for _, oe := range edits { - for i := range sim { - if sim[i].Source != oe.KustomizationPath || sim[i].Index != oe.Edit.EntryIndex { - continue - } - switch oe.Edit.Field { - case "newName": - sim[i].NewName = oe.Edit.Value - case "newTag": - sim[i].NewTag = oe.Edit.Value - case "digest": - sim[i].Digest = oe.Edit.Value - } - } - } - for _, p := range plans { - rendered, _ := renderImage(parseImageRef(p.fileImage), sim) - if rendered != p.live { - return false - } - } - return true -} - // replicaKinds are the kinds the builtin replica transformer touches. func isReplicaKind(kind string) bool { switch kind { diff --git a/internal/manifestanalyzer/render_verify.go b/internal/manifestanalyzer/render_verify.go new file mode 100644 index 00000000..d8aa2c6e --- /dev/null +++ b/internal/manifestanalyzer/render_verify.go @@ -0,0 +1,241 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "bytes" + "encoding/json" + "fmt" + "sort" + "strings" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" +) + +// This file is the oracle: before a commit that routes anything through a kustomization, +// we render the repository as it would stand AFTER the commit and refuse it unless the +// answer is exactly the one we intended. +// +// ATTRIBUTION MAY BE HEURISTIC. VERIFICATION MAY NOT. Deciding which file an edit belongs +// in is an inference, and this workstream is deliberately replacing one inference with a +// better one. But the check that says "and the result is right" must not be an inference at +// all, because a check that shares the blind spot of the thing it checks turns a wrong guess +// into a CONFIDENT wrong write. +// +// That is precisely what the deleted simulateImageRender did: it replayed OUR chain over the +// images OUR projection had planned, so a case our model got wrong, it got wrong twice — and +// agreed with itself. This cannot do that. It runs the library Flux renders with, over the +// exact bytes we are about to commit. +// +// It runs ONCE PER FLUSH, not once per resource, and that is a correctness requirement +// rather than an optimisation. An images: entry is shared context: converging one Deployment +// through it necessarily moves every other object the entry matches. Asked resource by +// resource, "did anything else move?" is yes for the first of two Deployments that share an +// image and are being bumped together — so a per-resource oracle would refuse a write that +// converges perfectly well. Only the whole batch knows that the sibling is moving because it, +// too, is being written to exactly where it now lives. +// +// See docs/design/support-boundary/render-attribution.md §5 and +// docs/design/support-boundary/render-root-scoping.md §3. + +// WriteIntent is one document a flush writes, and what the render must show for it +// afterwards. Everything the batch does NOT declare an intent for must come out of the +// render byte-for-byte unchanged — that is the blast-radius half of the oracle, and it is +// what makes it safe for the projection to guess. +type WriteIntent struct { + // SourcePath is the file holding the document, slash-relative to the scan root. + SourcePath string + // Kind and Name identify the document within that file. + Kind, Name string + // Desired is the live object the document must render to. It is the whole point of + // the check, and it is nil only when Removed or Unchecked says there is nothing to + // check against. + Desired *unstructured.Unstructured + // Removed marks a document the flush deletes: it must disappear from the render. + Removed bool + // Unchecked marks a write whose rendered form we cannot predict, so the object is + // permitted to move without being compared. There are exactly two: a SENSITIVE + // document (the file is SOPS-encrypted, so kustomize renders the ciphertext, which + // no plaintext live object can equal), and a bounded FIELD PATCH (the event carries + // a few assignments, never a whole object to compare against). + // + // Unchecked weakens the oracle for those documents, and it is stated rather than + // hidden: we can still prove that such a write disturbs nothing ELSE, which is the + // half that protects other people's environments. + Unchecked bool + // Governed marks a document whose write was routed through a kustomization override + // chain. These are the writes the oracle exists for, so one that turns out not to be + // rendered by any root at all is a contradiction, and refused rather than skipped. + Governed bool +} + +func (w WriteIntent) key() chainKey { + return chainKey{originPath: filepathToSlash(w.SourcePath), kind: w.Kind, name: w.Name} +} + +// RenderRefusedError is the oracle's verdict: the bytes the flush was about to commit do +// not render to the live cluster state, or they move something the flush never intended to +// touch. It aborts the flush — nothing is written — and it names the file and the object, +// because the correct outcome here is a REPORTED refusal, never a resource that is quietly +// not mirrored (render-attribution.md §7). +type RenderRefusedError struct { + // Reasons are the individual findings, sorted, so the message is stable across runs. + Reasons []string +} + +func (e *RenderRefusedError) Error() string { + return "kustomize render refused the write: " + strings.Join(e.Reasons, "; ") +} + +// VerifyBatchRenders re-renders every root of the subtree twice — as the flush found it, +// and as the flush would leave it — and proves both halves of the oracle: +// +// 1. every document the flush writes renders to exactly the live object, and +// 2. every object it does NOT write is byte-for-byte unchanged. +// +// (2) is not a nicety. A kustomization is shared context: an images: entry edited to +// converge one Deployment governs every other object it matches, and a base is rendered by +// every overlay above it. A proposal that fixes its own target and moves a second object has +// written a live value into a file another render root also reads — the one edit write-fan-in +// exists to forbid. +// +// before and after are complete file trees. The cost is two builds per render root, once per +// flush, and it is only paid when the flush routed something through a kustomization. +func VerifyBatchRenders(before, after []manifestedit.FileContent, intents []WriteIntent) error { + byKey := make(map[chainKey]WriteIntent, len(intents)) + for _, in := range intents { + byKey[in.key()] = in + } + seen := map[chainKey]struct{}{} + + var reasons []string + for _, root := range renderTargets(parseKustomizations(after)) { + was, err := renderRoot(before, root) + if err != nil { + // The tree did not build BEFORE we touched it. The acceptance gate refuses + // such a folder, so we should never be writing into one — but an unverifiable + // root is not a verified root, so say so rather than skip it. + reasons = append(reasons, fmt.Sprintf("render root %s did not build before the write: %v", root, err)) + continue + } + now, err := renderRoot(after, root) + if err != nil { + reasons = append(reasons, fmt.Sprintf("render root %s no longer builds with the write applied: %v", root, err)) + continue + } + reasons = append(reasons, compareRoot(root, byKey, seen, renderedByKey(was), renderedByKey(now))...) + } + + for _, in := range intents { + if _, hit := seen[in.key()]; in.Governed && !hit { + reasons = append(reasons, fmt.Sprintf( + "%s/%s in %s was routed through a kustomization override, but no render root renders it", + in.Kind, in.Name, in.SourcePath)) + } + } + + if len(reasons) == 0 { + return nil + } + sort.Strings(reasons) + return &RenderRefusedError{Reasons: reasons} +} + +// compareRoot checks one render root's before/after pair against the flush's intents. +func compareRoot( + root string, + intents map[chainKey]WriteIntent, + seen map[chainKey]struct{}, + was, now map[chainKey]renderedObject, +) []string { + var reasons []string + for key := range unionKeys(was, now) { + before, existed := was[key] + after, exists := now[key] + intent, intended := intents[key] + if intended { + seen[key] = struct{}{} + } + + switch { + case !intended: + // The blast radius. This object is nobody's target, so the flush has no + // business changing it — in this root or any other. + if !existed || !exists { + reasons = append(reasons, fmt.Sprintf( + "the write adds or removes %s/%s in render root %s, which it never set out to write", + key.kind, key.name, root)) + continue + } + if !sameObject(before.Object, after.Object) { + reasons = append(reasons, fmt.Sprintf( + "the write also changes what render root %s renders for %s/%s (from %s), "+ + "which it never set out to write", + root, key.kind, key.name, key.originPath)) + } + case intent.Removed: + if exists { + reasons = append(reasons, fmt.Sprintf( + "%s/%s was deleted, but render root %s still renders it", key.kind, key.name, root)) + } + case intent.Unchecked: + // A sensitive document or a bounded field patch: it is allowed to move, and + // there is nothing to compare it against. The other cases above still hold it + // to not disturbing anything else. + case !exists: + reasons = append(reasons, fmt.Sprintf( + "%s/%s was written, but render root %s no longer renders it", key.kind, key.name, root)) + case !sameObject(after.Object, intent.Desired): + reasons = append(reasons, fmt.Sprintf( + "in render root %s, %s/%s (from %s) does not render to the live object after the write", + root, key.kind, key.name, key.originPath)) + } + } + return reasons +} + +func unionKeys(a, b map[chainKey]renderedObject) map[chainKey]struct{} { + out := make(map[chainKey]struct{}, len(a)+len(b)) + for k := range a { + out[k] = struct{}{} + } + for k := range b { + out[k] = struct{}{} + } + return out +} + +// sameObject compares two rendered/live objects by canonical JSON. +// +// Canonical JSON rather than reflect.DeepEqual on purpose: kustomize hands numbers back as +// Go int where the API machinery uses int64 — measured, and it is why +// unstructured.NestedInt64 reports found=false on a rendered spec.replicas — and a +// comparison that called those two different would refuse every replica write there is. +// json.Marshal also sorts map keys, so this is key-order independent. +func sameObject(a, b *unstructured.Unstructured) bool { + if a == nil || b == nil { + return a == b + } + left, err := json.Marshal(a.Object) + if err != nil { + return false + } + right, err := json.Marshal(b.Object) + if err != nil { + return false + } + return bytes.Equal(left, right) +} + +// renderedByKey indexes a render by the document each object came from. The key carries the +// ORIGIN FILE, not just kind/name, so two same-named objects rendered from different source +// files stay distinct. +func renderedByKey(objects []renderedObject) map[chainKey]renderedObject { + out := make(map[chainKey]renderedObject, len(objects)) + for _, o := range objects { + out[chainKey{originPath: o.OriginPath, kind: o.Object.GetKind(), name: o.Object.GetName()}] = o + } + return out +} diff --git a/internal/watch/event_router.go b/internal/watch/event_router.go index 93ffa311..088c9764 100644 --- a/internal/watch/event_router.go +++ b/internal/watch/event_router.go @@ -269,8 +269,9 @@ func (r *EventRouter) handleScopedResyncError( // // - purely the .gittargetignore-shadows-a-write case (§4.3) — the unrecoverable footgun — // gets IgnoreShadowsManagedPath; -// - purely write-boundary violations (a planned write escaping spec.path, or an in-place -// edit of a file more than one render root reaches) gets WriteBoundaryRefused: the folder +// - purely write-boundary violations (a planned write escaping spec.path, an in-place edit +// of a file more than one render root reaches, or a write kustomize will not vouch for +// when the folder is re-rendered with it applied) gets WriteBoundaryRefused: the folder // content is fine, the *edit* had nowhere safe to land. // // Any other refusal, and any mix of shapes, keeps the umbrella UnsupportedContent. The strings @@ -281,7 +282,11 @@ func gitPathRefusalReason(refused *manifestanalyzer.AcceptanceRefusedError) stri switch { case refused.AllIssuesOfKinds(manifestanalyzer.IssueIgnoreShadowsManaged): return "IgnoreShadowsManagedPath" - case refused.AllIssuesOfKinds(manifestanalyzer.IssueWriteEscapesScope, manifestanalyzer.IssueWriteFanIn): + case refused.AllIssuesOfKinds( + manifestanalyzer.IssueWriteEscapesScope, + manifestanalyzer.IssueWriteFanIn, + manifestanalyzer.IssueRenderRefused, + ): return "WriteBoundaryRefused" default: return "UnsupportedContent" From 4595d8db7c87a1176d93d7c0075a688cef4a22a4 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 17:48:28 +0000 Subject: [PATCH 05/13] feat(kustomize)!: attribute override values with a dyed render, not a re-implementation Deletes the last of the hand-rolled kustomize: renderImage, imageSuppliers and isReplicaKind, the ~400 lines the write path used to decide which file an edit belongs in. The projection now inverts against what kustomize ACTUALLY renders, read off the renderer, instead of against our re-derivation of it. HOW IT WORKS. A render says web:2.0; it never says who supplied the 2.0, and kustomize keeps no field-level provenance anywhere, at any visibility level. So attribution cannot be READ out of it -- only inferred by questioning it. Write a unique nonce into every declared override entry, render the root a second time, and read the nonces off the output: wherever a dye lands, THAT entry supplied THAT field. Two builds per root, constant in the number of entries. WHY NOT LEAVE-ONE-OUT. Removing an entry and seeing what moves is blind exactly where it matters. A base at app:v1 under an overlay pinning newTag: v1 -- the state every repo is in the moment a release lands in both places -- moves NOTHING when the entry is removed, so the probe concludes the source file owns the tag, writes the user's next tag into the base, and the overlay overrides it straight back. Forever. Removal probes the VALUE, and values collide. A nonce nothing else can produce does not. Both cases are now tests (idempotent pin, and a tie between two entries). GUARDRAILS, all measured, none optional: - the nonce charset is a CORRECTNESS requirement. kustomize does not validate a tag, but it MATCHES on the image with a regex, so an out-of-charset dye leaves the image un-matchable and every later entry SILENTLY STOPS FIRING. Tag dyes stay in [a-zA-Z0-9_.{}-]; digest dyes carry the mandatory sha256: prefix. - newName is not a pure sink -- it is the join key for every later entry -- so it is dyed only when no other entry's name: matches it. And "matches" is asked of kustomize's own compiled pattern, not of string equality, because an entry name is a REGEX. - only a field an entry already DECLARES is dyed; injecting one would fabricate a supplier. - baseline first, then dye. A dyed build that fails where the real one succeeded means the dye hit something that is not a sink, and the answer is then NO ATTRIBUTION -- never a fallback to another heuristic. Nothing routes, and the oracle from the previous commit adjudicates whatever the source document alone can carry. THREE SHIPPED BUGS DIE WITH THE CODE THAT CAUSED THEM (see docs/UPGRADING.md): - B1: our image matcher was string equality; kustomize's is a regex over the whole image string. `- name: "ap."` matches `app` in kustomize and did not in us, so we believed the folder rendered one thing while it rendered another -- and wrote the difference into the source manifest, killing the entry. - B2: isReplicaKind listed Deployment, ReplicaSet, StatefulSet. kustomize's fieldspec also includes ReplicationController -- it says so in its own error message. A scale on an RC was written into the source document, where the transformer overrode it back. - B3: we collected ephemeralContainers (kustomize does NOT rewrite them) and missed volumes[].image.reference (kustomize DOES -- measured). Both directions mis-attributed. None of these are fixed. They are DELETED, along with the fieldspec we had no business keeping a second opinion about. The dye does not decide what kustomize touches; it reads where kustomize's own nonces came out. THE TEST NET IS REBUILT, which the signature change required and the change deserved. The 12 TestSplitDesired_* tests constructed (gitRaw, desired, overrides) BY HAND -- they asserted what we BELIEVED a folder renders to, which is the belief that was wrong twice in shipped code, so they could not have caught either bug and did not. Every case now builds a real tree, renders it with kustomize, reads the attribution off a dyed render, and drives the projection with the result. TestRenderImage_MatchesKustomizeOnTheHardCases is gone with its subject; the corpus differential is replaced by a stronger invariant across both corpora: AN IN-SYNC FOLDER MUST PROJECT TO A COMPLETE NO-OP -- which is exactly the property #231's digest/tag corruption violated. One more landmine confirmed while building it: a rendered object is not even a valid unstructured. kustomize hands numbers back as Go `int`, so DeepCopyJSON PANICS on one ("cannot deep copy int") and NestedInt64 reports found=false. The projection reads replica counts with a type switch, and the corpus test normalises through JSON. The corpus support baseline is byte-for-byte unchanged: this changes how attribution is derived, not which folders are supported. See docs/design/support-boundary/render-attribution.md sections 3, 6 and 7. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/git/plan_flush.go | 4 +- internal/manifestanalyzer/analyzer_test.go | 7 +- internal/manifestanalyzer/dye.go | 256 ++++++ internal/manifestanalyzer/dye_test.go | 158 ++++ .../kustomize_render_hostile_test.go | 9 +- .../kustomize_render_semantics_test.go | 114 +-- .../manifestanalyzer/kustomize_render_test.go | 135 ++-- internal/manifestanalyzer/override_chain.go | 27 +- internal/manifestanalyzer/overrides.go | 9 +- .../manifestanalyzer/overrides_attribution.go | 214 +++++ .../manifestanalyzer/overrides_projection.go | 402 +++++----- .../overrides_projection_test.go | 737 ++++++++++++------ internal/manifestanalyzer/render_verify.go | 5 +- internal/manifestanalyzer/store.go | 16 +- 14 files changed, 1461 insertions(+), 632 deletions(-) create mode 100644 internal/manifestanalyzer/dye.go create mode 100644 internal/manifestanalyzer/dye_test.go create mode 100644 internal/manifestanalyzer/overrides_attribution.go diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index eb278cf0..7661230d 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -613,9 +613,9 @@ func (wb *writeBatch) patchExisting( } projected := manifestreport.Project(desired) var overrideEdits []manifestanalyzer.OverrideEdit - if dm.Overrides != nil { + if dm.Rendered != nil { if gitRaw, parsed := gitDocRawObject(buf.current, idx); parsed { - projected, overrideEdits = manifestanalyzer.SplitDesiredForOverrides(gitRaw, projected, dm.Overrides) + projected, overrideEdits = manifestanalyzer.SplitDesiredForOverrides(gitRaw, projected, dm.Rendered) } } c := manifestedit.Comparison{ diff --git a/internal/manifestanalyzer/analyzer_test.go b/internal/manifestanalyzer/analyzer_test.go index 08edde13..baee943b 100644 --- a/internal/manifestanalyzer/analyzer_test.go +++ b/internal/manifestanalyzer/analyzer_test.go @@ -113,15 +113,18 @@ func TestAnalyze_Issues(t *testing.T) { IssueUnresolvedKRM: 0, IssueOutOfScope: 0, IssueUnsupportedKustomize: 0, - // Foreign-content, ignore-shadow, and the L1/L2 write-boundary refusals are + // Foreign-content, ignore-shadow, and the write-boundary refusals are // acceptance-gate / write-plan facts, not part of the structure-only Analyze report, - // so they never surface here. + // so they never surface here. IssueRenderRefused is the strongest case of that: it is + // a fact about a PROPOSED WRITE re-rendered with kustomize, which structure-only + // analysis has no live object to propose. IssueForeignFile: 0, IssueForeignSymlink: 0, IssueForeignSubmodule: 0, IssueIgnoreShadowsManaged: 0, IssueWriteEscapesScope: 0, IssueWriteFanIn: 0, + IssueRenderRefused: 0, } for kind, n := range want { if got := countIssues(rep, kind); got != n { diff --git a/internal/manifestanalyzer/dye.go b/internal/manifestanalyzer/dye.go new file mode 100644 index 00000000..c07cace7 --- /dev/null +++ b/internal/manifestanalyzer/dye.go @@ -0,0 +1,256 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "fmt" + "regexp" + "sort" + + kustypes "sigs.k8s.io/kustomize/api/types" + "sigs.k8s.io/yaml" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" +) + +// The dye: which override entry supplied this value? +// +// A render does not say. `kustomize build` returns `web:2.0`; it never says where the `2.0` +// came from, and there is no field-level provenance anywhere in kustomize, at any visibility +// level — the image filter rewrites the field and records nothing. So attribution cannot be +// READ out of kustomize. It can only be inferred by QUESTIONING it: perturb an input, render, +// see what moves. +// +// The obvious perturbation — remove an entry and see what changes — is measurably wrong, and +// wrong on the most ordinary configuration there is. A base at `app:v1` under an overlay +// declaring `newTag: v1` is the state every repo is in the moment a release lands in both +// places; remove the entry and NOTHING MOVES, so removal concludes the source file supplies +// the tag. Write the user's next tag into the base, and the overlay overrides it straight back +// on every reconcile, forever. The cause is structural: removal probes the VALUE, and values +// collide. +// +// The dye is that same idea with the flaw removed. Write a unique nonce into every override +// entry, render ONCE, and read the nonces off the output: wherever a dye lands, THAT entry +// supplied THAT field. Absence is indistinguishable from "someone else wrote the same value"; +// a nonce nothing else can produce is not. Ties resolve, the idempotent pin resolves, and the +// cost is one extra build per root rather than one per entry. +// +// See docs/design/support-boundary/render-attribution.md §3. + +// The nonce alphabet is not a style choice, it is a CORRECTNESS REQUIREMENT, and it is the +// one thing here that will silently ruin a render if it is got wrong. +// +// kustomize does not validate newTag or digest at all — a 200-character tag renders straight +// through. But it MATCHES on the image with a regex over the whole string +// (api/internal/image/image.go): +// +// "^" + name + "(:[a-zA-Z0-9_.{}-]*)?(@sha256:[a-zA-Z0-9_.{}-]*)?$" +// +// so a dye outside that charset leaves the image un-matchable and EVERY LATER ENTRY SILENTLY +// STOPS FIRING. No error; a different render. Measured: `newTag: zz/probe` renders, and kills +// the next entry. A digest without the mandatory `sha256:` prefix does the same. +const ( + // dyeTagPrefix keeps tag nonces inside [a-zA-Z0-9_.{}-]. + dyeTagPrefix = "grdye-t-" + // dyeNamePrefix is for newName. A name is not matched by the tag charset — it is the + // regex's literal prefix — but it must still be a plain image name: no ':' (a tag + // separator), no '@' (a digest separator), no '/' (a registry separator). + dyeNamePrefix = "grdye-n-" + // dyeDigestPrefix carries the sha256: the regex REQUIRES, and stays alphanumeric after it. + dyeDigestPrefix = "sha256:grdye" + // dyeReplicaBase is a reserved count. Measured: it renders through untouched, and no + // real deployment has two billion replicas. + dyeReplicaBase = int64(2_000_000_000) +) + +// dyeMark says what a nonce was minted for, so a dye read out of the tag position can be +// required to BE a tag dye rather than merely a nonce that happens to have landed there. +type dyeMark struct { + image *ImageOverride + replica *ReplicaOverride + field string +} + +// dyePlan is the counterfactual: the dyed kustomizations to render, and the table that reads +// the nonces back. One plan covers the whole scan, so it is built once and reused for every +// render root. +type dyePlan struct { + // replace is the dyed kustomization content, by slash path — the overlay handed to + // renderRootWith. + replace map[string][]byte + // byNonce maps a nonce back to the entry that carries it. + byNonce map[string]dyeMark + // byReplicaCount does the same for the reserved integer counts. + byReplicaCount map[int64]dyeMark + // namesDyed records whether newName was dyed. When a rename chain exists, it is not — + // see dyeingNamesIsSafe — and a name change is then simply unattributable, which means + // no entry edit, which means the oracle adjudicates whatever the source file can carry. + namesDyed bool +} + +// planDye mints a nonce for every override entry field that is DECLARED, and re-serialises +// each kustomization with the nonces in place. +// +// "Declared" matters: injecting a newTag into a newName-only entry would fabricate a supplier +// that does not exist, and the projection would then route a tag edit to an entry that never +// set a tag. +// +// The dye is applied to kustomize's own typed Kustomization and re-marshalled, exactly as +// withBuildMetadata already does, so YAML quoting is the encoder's problem rather than ours. +// It has to be: `newTag: y` is a YAML BOOLEAN, and kustomize's own Unmarshal rejects it. +func planDye(files []manifestedit.FileContent) *dyePlan { + plan := &dyePlan{ + replace: map[string][]byte{}, + byNonce: map[string]dyeMark{}, + byReplicaCount: map[int64]dyeMark{}, + namesDyed: dyeingNamesIsSafe(allImageOverrides(files)), + } + + next := 0 + mint := func() int { next++; return next } + + for _, f := range sortedKustomizationFiles(files) { + var k kustypes.Kustomization + if err := k.Unmarshal(f.Content); err != nil { + continue // an unparseable kustomization refuses the folder elsewhere; nothing to dye + } + k.FixKustomization() + path := filepathToSlash(f.Path) + + dyed := plan.dyeImages(&k, path, mint) + dyed = plan.dyeReplicas(&k, path, mint) || dyed + if !dyed { + continue + } + content, err := yaml.Marshal(&k) + if err != nil { + continue // cannot dye this file; its entries simply go unattributed + } + plan.replace[path] = content + } + return plan +} + +// dyeImages replaces every DECLARED images: field with a nonce, in place, and records what each +// nonce was minted for. It reports whether anything was dyed. +func (p *dyePlan) dyeImages(k *kustypes.Kustomization, path string, mint func() int) bool { + entries, ok := imageOverrides(k.Images, path) + if !ok || len(entries) != len(k.Images) { + return false + } + dyed := false + for i := range k.Images { + entry := &entries[i] + if entry.HasNewTag { + nonce := fmt.Sprintf("%s%04d", dyeTagPrefix, mint()) + p.byNonce[nonce] = dyeMark{image: entry, field: fieldNewTag} + k.Images[i].NewTag = nonce + dyed = true + } + if entry.HasDigest { + nonce := fmt.Sprintf("%s%04d", dyeDigestPrefix, mint()) + p.byNonce[nonce] = dyeMark{image: entry, field: fieldDigest} + k.Images[i].Digest = nonce + dyed = true + } + if entry.HasNewName && p.namesDyed { + nonce := fmt.Sprintf("%s%04d", dyeNamePrefix, mint()) + p.byNonce[nonce] = dyeMark{image: entry, field: fieldNewName} + k.Images[i].NewName = nonce + dyed = true + } + } + return dyed +} + +// dyeReplicas replaces every replicas: count with a reserved integer, in place. +func (p *dyePlan) dyeReplicas(k *kustypes.Kustomization, path string, mint func() int) bool { + entries, ok := replicaOverrides(k.Replicas, path) + if !ok || len(entries) != len(k.Replicas) { + return false + } + for i := range k.Replicas { + count := dyeReplicaBase + int64(mint()) + p.byReplicaCount[count] = dyeMark{replica: &entries[i], field: fieldCount} + k.Replicas[i].Count = count + } + return len(k.Replicas) > 0 +} + +// The entry fields the projection can write, and the dye can therefore attribute. +const ( + fieldNewName = "newName" + fieldNewTag = "newTag" + fieldDigest = "digest" + fieldCount = "count" +) + +// dyeingNamesIsSafe reports whether dyeing newName can be trusted in this tree. +// +// A dye is sound exactly when the dyed field is a PURE SINK — never an input to a matcher. +// newTag, digest and replicas[].count are sinks: nothing selects on them. newName IS NOT. It +// is the join key for every later entry: +// +// images: [{name: app, newName: renamed}, {name: renamed, newTag: "4.0"}] +// undyed -> renamed:4.0 +// newName dyed -> grdye-n-0001:v1 # entry 2 stopped matching. the render changed shape. +// +// The condition is exact and needs no model of kustomize's matching: dyeing a newName can only +// change a matching decision if some OTHER entry's name: matches it. And "matches" is asked of +// kustomize's own compiled pattern rather than of string equality, because an entry's name is a +// REGULAR EXPRESSION — `name: "rename."` matches `renamed` without being equal to it. +// +// Where no rename chain exists — essentially every real repository — names are dyed. Where one +// does, they are not, and a name change is simply not attributed. That is the correct fallback: +// no attribution, never a guess. +func dyeingNamesIsSafe(entries []ImageOverride) bool { + for i := range entries { + if !entries[i].HasNewName { + continue + } + for j := range entries { + if i == j { + continue + } + pattern, err := regexp.Compile(imageNamePattern(entries[j].Name)) + if err != nil { + return false // an uncompilable name refuses the folder anyway; do not dye into it + } + if pattern.MatchString(entries[i].NewName) { + return false + } + } + } + return true +} + +// allImageOverrides is every images: entry in the scan, which is the scope the rename-chain +// guard is asked over. Scoping it per render root would dye more, and the entries a root +// cannot reach cost nothing to be conservative about. +func allImageOverrides(files []manifestedit.FileContent) []ImageOverride { + var out []ImageOverride + for _, f := range sortedKustomizationFiles(files) { + var k kustypes.Kustomization + if err := k.Unmarshal(f.Content); err != nil { + continue + } + k.FixKustomization() + if entries, ok := imageOverrides(k.Images, filepathToSlash(f.Path)); ok { + out = append(out, entries...) + } + } + return out +} + +// sortedKustomizationFiles keeps nonce minting deterministic: the same tree always produces +// the same dyes, so a render is reproducible and a diff of two runs is empty. +func sortedKustomizationFiles(files []manifestedit.FileContent) []manifestedit.FileContent { + var out []manifestedit.FileContent + for _, f := range files { + if isKustomizationFile(f.Path) { + out = append(out, f) + } + } + sort.Slice(out, func(i, j int) bool { return out[i].Path < out[j].Path }) + return out +} diff --git a/internal/manifestanalyzer/dye_test.go b/internal/manifestanalyzer/dye_test.go new file mode 100644 index 00000000..f6719acd --- /dev/null +++ b/internal/manifestanalyzer/dye_test.go @@ -0,0 +1,158 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "regexp" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" +) + +// The dye's guardrails. Every one of these is a CORRECTNESS requirement rather than a +// preference, and every one was measured against kustomize rather than reasoned about. +// See docs/design/support-boundary/render-attribution.md §3 and §7. + +// The charset is the one that will silently ruin a render if it is got wrong. +// +// kustomize does not validate a tag or a digest — anything renders through. But it MATCHES on +// the whole image string with a regex, so a nonce outside that charset leaves the image +// un-matchable and EVERY LATER ENTRY SILENTLY STOPS FIRING. No error, just a different render, +// and an attribution that credits the wrong entry. +func TestPlanDye_NoncesStayInsideKustomizesMatchCharset(t *testing.T) { + // Exactly kustomize's own pattern (api/internal/image/image.go), for the image name "app". + matcher := regexp.MustCompile(imageNamePattern("app")) + + plan := planDye([]manifestedit.FileContent{ + {Path: "kustomization.yaml", Content: []byte( + "resources:\n - d.yaml\nimages:\n" + + " - name: app\n newTag: v1\n" + + " - name: app\n digest: sha256:abc\n")}, + }) + require.NotEmpty(t, plan.byNonce) + + for nonce, mark := range plan.byNonce { + var image string + switch mark.field { + case fieldNewTag: + image = "app:" + nonce + case fieldDigest: + image = "app@" + nonce + default: + continue + } + require.True(t, matcher.MatchString(image), + "the dyed image %q must still match kustomize's own pattern, or every later entry "+ + "silently stops firing", image) + } +} + +// A digest dye MUST carry the sha256: prefix. The pattern is `(@sha256:[...])?` — a bare nonce +// after the @ does not match, and the next entry goes quietly dead. Measured. +func TestPlanDye_DigestNonceCarriesTheMandatorySha256Prefix(t *testing.T) { + plan := planDye([]manifestedit.FileContent{ + {Path: "kustomization.yaml", Content: []byte( + "resources:\n - d.yaml\nimages:\n - name: app\n digest: sha256:abc\n")}, + }) + + found := false + for nonce, mark := range plan.byNonce { + if mark.field != fieldDigest { + continue + } + found = true + require.Regexp(t, `^sha256:[a-zA-Z0-9]+$`, nonce, + "a digest dye without the sha256: prefix disables every later entry") + } + require.True(t, found, "the digest entry must have been dyed") +} + +// Only a field the entry ALREADY DECLARES may be dyed. Injecting a newTag into a +// newName-only entry would fabricate a supplier that does not exist, and the projection would +// then route a tag edit onto an entry that never set a tag. +func TestPlanDye_OnlyDyesFieldsTheEntryDeclares(t *testing.T) { + plan := planDye([]manifestedit.FileContent{ + {Path: "kustomization.yaml", Content: []byte( + "resources:\n - d.yaml\nimages:\n - name: app\n newName: mirror/app\n")}, + }) + + for _, mark := range plan.byNonce { + require.Equal(t, fieldNewName, mark.field, + "the entry declares only newName, so only newName may carry a dye") + } +} + +// newName is NOT a pure sink — it is the join key for every later entry — so dyeing it can +// change which entries match and alter the render's shape. The guard is exact: a newName may be +// dyed unless some other entry's name: matches it. +func TestDyeingNamesIsSafe(t *testing.T) { + tests := []struct { + name string + entries []ImageOverride + safe bool + }{ + { + name: "no rename at all", + entries: []ImageOverride{ + {Name: "app", NewTag: "v2", HasNewTag: true}, + }, + safe: true, + }, + { + name: "a rename nothing else refers to", + entries: []ImageOverride{ + {Name: "app", NewName: "mirror/app", HasNewName: true}, + {Name: "other", NewTag: "v2", HasNewTag: true}, + }, + safe: true, + }, + { + name: "a rename CHAIN: a later entry keys off the new name", + entries: []ImageOverride{ + {Name: "app", NewName: "mirror/app", HasNewName: true}, + {Name: "mirror/app", NewTag: "v2", HasNewTag: true}, + }, + safe: false, + }, + { + // The guard must be asked of kustomize's compiled pattern, not of string + // equality: an entry's name is a REGULAR EXPRESSION, so `mirror/ap.` matches + // `mirror/app` without being equal to it. A string-equality guard would dye the + // name here, kill the second entry, and mis-attribute the tag. + name: "a rename chain joined by a REGEX, not by string equality", + entries: []ImageOverride{ + {Name: "app", NewName: "mirror/app", HasNewName: true}, + {Name: "mirror/ap.", NewTag: "v2", HasNewTag: true}, + }, + safe: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.safe, dyeingNamesIsSafe(tc.entries)) + }) + } +} + +// The dye must be reproducible: the same tree mints the same nonces every time, or two renders +// of an unchanged repository would disagree and the store would flap. +func TestPlanDye_IsDeterministic(t *testing.T) { + files := []manifestedit.FileContent{ + { + Path: "b/kustomization.yaml", + Content: []byte("resources:\n - d.yaml\nimages:\n - name: b\n newTag: v1\n"), + }, + { + Path: "a/kustomization.yaml", + Content: []byte("resources:\n - d.yaml\nimages:\n - name: a\n newTag: v1\n"), + }, + } + + first := planDye(files) + for range 5 { + require.Equal(t, first.replace, planDye(files).replace) + } +} diff --git a/internal/manifestanalyzer/kustomize_render_hostile_test.go b/internal/manifestanalyzer/kustomize_render_hostile_test.go index d7410153..a3a5ca92 100644 --- a/internal/manifestanalyzer/kustomize_render_hostile_test.go +++ b/internal/manifestanalyzer/kustomize_render_hostile_test.go @@ -38,10 +38,11 @@ func TestRenderRoot_ValidRegexImageNameStillBuilds(t *testing.T) { require.NoError(t, err) require.Len(t, rendered, 1) - slots := collectContainerSlots(rendered[0].Object.Object) + slots := collectImageSlots(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. + // Measured against kustomize: `ngin.` matches `nginx`. Our own matcher used to compare + // names for equality and did NOT (B1) — which is why attribution now reads kustomize's + // dyes instead of re-deciding what matched. require.Equal(t, "nginx:2.0", slots[0].image) } @@ -56,7 +57,7 @@ func TestRenderRootWith_RendersTheReplacementAndLeavesTheScanAlone(t *testing.T) require.NoError(t, err) require.Len(t, rendered, 1) - slots := collectContainerSlots(rendered[0].Object.Object) + slots := collectImageSlots(rendered[0].Object.Object) require.Len(t, slots, 1) require.Equal(t, "nginx:v3", slots[0].image, "the build must see the counterfactual") require.Contains(t, string(files[1].Content), "newTag: v2", "the scan must survive the probe unchanged") diff --git a/internal/manifestanalyzer/kustomize_render_semantics_test.go b/internal/manifestanalyzer/kustomize_render_semantics_test.go index 788bebd0..8b652afc 100644 --- a/internal/manifestanalyzer/kustomize_render_semantics_test.go +++ b/internal/manifestanalyzer/kustomize_render_semantics_test.go @@ -4,117 +4,19 @@ package manifestanalyzer import ( "fmt" - "testing" - - "github.com/stretchr/testify/require" "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" ) -// The layout corpus barely exercises images:, so on its own it cannot license -// deleting our re-implemented image transformer. This table does: it drives the -// cases the re-implementation actually has to get right — chained renames, digest -// precedence, a registry port that looks like a tag — through BOTH kustomize and -// renderImage, and requires them to agree byte for byte. +// TestRenderImage_MatchesKustomizeOnTheHardCases lived here: eleven hand-picked cases driven +// through BOTH kustomize and our own renderImage, required to agree. It is gone because +// renderImage is gone — there is no second opinion left to pin to the first. // -// Every row is a claim about kustomize's semantics that our code depends on. If -// kustomize ever changes one, this test fails instead of the operator silently -// writing a file that renders to something else. - -func TestRenderImage_MatchesKustomizeOnTheHardCases(t *testing.T) { - cases := []struct { - name string - source string // the container image in the source Deployment - images string // the images: block of the overlay kustomization - }{ - { - name: "newTag only", - source: "ghcr.io/org/web:1.0", - images: " - name: ghcr.io/org/web\n newTag: \"2.0\"\n", - }, - { - name: "newName only", - source: "ghcr.io/org/web:1.0", - images: " - name: ghcr.io/org/web\n newName: ghcr.io/org/web-hardened\n", - }, - { - name: "newName and newTag in one entry", - source: "ghcr.io/org/web:1.0", - images: " - name: ghcr.io/org/web\n newName: ghcr.io/org/hardened\n newTag: \"3.0\"\n", - }, - { - name: "digest replaces the tag", - source: "ghcr.io/org/web:1.0", - images: " - name: ghcr.io/org/web\n digest: sha256:abc123\n", - }, - { - // kustomize's own doc on types.Image: "If digest is present NewTag value - // is ignored." Our renderImage must not disagree. - name: "digest wins over newTag in the same entry", - source: "ghcr.io/org/web:1.0", - images: " - name: ghcr.io/org/web\n newTag: \"9.9\"\n digest: sha256:abc123\n", - }, - { - // The chain interaction that makes a naive inversion wrong: the first - // entry renames, and only then does the second entry's name match. - name: "a rename makes a later entry match", - source: "ghcr.io/org/web:1.0", - images: " - name: ghcr.io/org/web\n newName: ghcr.io/org/renamed\n" + - " - name: ghcr.io/org/renamed\n newTag: \"4.0\"\n", - }, - { - name: "the last matching entry wins", - source: "ghcr.io/org/web:1.0", - images: " - name: ghcr.io/org/web\n newTag: \"2.0\"\n" + - " - name: ghcr.io/org/web\n newTag: \"5.0\"\n", - }, - { - name: "an entry that matches nothing changes nothing", - source: "ghcr.io/org/web:1.0", - images: " - name: ghcr.io/org/other\n newTag: \"7.0\"\n", - }, - { - // The parseImageRef edge case: the colon in a registry port is not a tag - // separator. - name: "a registry port is not a tag", - source: "localhost:5000/org/web:1.0", - images: " - name: localhost:5000/org/web\n newTag: \"2.0\"\n", - }, - { - name: "an untagged source image", - source: "ghcr.io/org/web", - images: " - name: ghcr.io/org/web\n newTag: \"2.0\"\n", - }, - { - name: "a source image that already carries a digest", - source: "ghcr.io/org/web@sha256:oldoldold", - images: " - name: ghcr.io/org/web\n newTag: \"2.0\"\n", - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - files := imageFixture(tc.source, tc.images) - - // What kustomize actually renders. - rendered, err := renderRoot(files, ".") - require.NoError(t, err) - require.Len(t, rendered, 1) - slots := collectContainerSlots(rendered[0].Object.Object) - require.Len(t, slots, 1) - kustomizeSays := slots[0].image - - // What our re-implemented chain renders. - kusts := parseKustomizations(files) - doc := kusts["."] - require.NotNil(t, doc) - ours, _ := renderImage(parseImageRef(tc.source), doc.images) - - require.Equal(t, kustomizeSays, ours.String(), - "kustomize renders %q; renderImage renders %q", kustomizeSays, ours.String()) - }) - } -} +// It is worth recording why the table was never enough, because it is the argument for the +// whole workstream. Its rows were the cases we THOUGHT of. B1 — that an images: entry name is +// a regex, so `- name: "ap."` matches `app` — was not among them, and shipped. A table of +// cases we thought of is not a substitute for making kustomize the arbiter at runtime, which +// is what the dye now does. See docs/design/support-boundary/render-attribution.md §6. // imageFixture is a one-file render root: a Deployment with one container, plus a // kustomization carrying the images: block under test. diff --git a/internal/manifestanalyzer/kustomize_render_test.go b/internal/manifestanalyzer/kustomize_render_test.go index 29a6d2b4..f136e8df 100644 --- a/internal/manifestanalyzer/kustomize_render_test.go +++ b/internal/manifestanalyzer/kustomize_render_test.go @@ -4,6 +4,7 @@ package manifestanalyzer import ( "bytes" + "encoding/json" "os" "path/filepath" "strings" @@ -16,32 +17,42 @@ import ( "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" ) -// This is the differential test that licenses deleting the re-implemented -// transformers: for every kustomize render root in both corpora, the image our -// renderImage chain produces must be byte-for-byte the image kustomize produces. +// The corpus-wide invariant that licenses deleting the re-implemented transformers. // -// 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. +// This test used to render every corpus image through our own renderImage chain and require it +// to equal kustomize's. That comparison is gone with the code it compared — there is no second +// opinion left to check against the first. What replaces it is stronger, and it is the property +// the deleted code kept violating: +// +// AN IN-SYNC FOLDER MUST PROJECT TO A COMPLETE NO-OP. +// +// Take what kustomize renders and hand it back as the live object — the folder is by definition +// already converged — then run the projection. It must route NO entry edits and it must hand +// back the source document unchanged. Any disagreement between what we think a folder renders +// to and what it actually renders shows up here as a phantom edit or a rewritten source file, +// on every render root of every fixture in both corpora. +// +// That is exactly the shape of #231: a digest entry clears the tag, we thought it did not, and +// on a perfectly in-sync folder the projection "helpfully" rewrote the tag out of the source +// manifest. This test fails on that. The old one could not — it compared our belief to our +// belief. -func TestRenderRoot_ImagesAgreeWithKustomize(t *testing.T) { +func TestProjection_InSyncCorpusFolderIsANoOp(t *testing.T) { roots := allCorpusRenderRoots(t) require.NotEmpty(t, roots, "no render roots found — the test would prove nothing") - compared, skipped := 0, 0 + checked, skipped := 0, 0 for _, root := range roots { t.Run(root.name, func(t *testing.T) { rendered, err := renderRoot(root.files, root.dir) if err != nil { - // A folder we refuse (remote base, generators, patches, plugins) - // need not render: the gate refuses it and the writer never sees it. + // A folder we refuse (remote base, generators, patches, plugins) need not + // render: the gate refuses it and the writer never sees it. skipped++ t.Skipf("not renderable, and refused by the acceptance gate: %v", err) } + chains, _ := renderChains(root.files, parseKustomizations(root.files)) - 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 @@ -50,60 +61,84 @@ func TestRenderRoot_ImagesAgreeWithKustomize(t *testing.T) { if src == nil { continue // renamed by a transformer we refuse; not a supported shape } - chain, ambiguous := ourChainFor(chains, ro) - if ambiguous { - continue // we route nothing through it; there is no claim to check + attribution, ambiguous := ourAttributionFor(chains, ro) + if ambiguous || attribution == nil { + continue // nothing is routed through it, so there is no claim to check } - compared += assertImagesMatchKustomize(t, ro, src, chain) + assertInSyncIsANoOp(t, ro, src, attribution) + checked++ } }) } - t.Logf("compared %d rendered images against the hand-rolled chain (%d roots skipped as refused)", - compared, skipped) + t.Logf("checked %d rendered documents for no-op projection (%d roots skipped as refused)", + checked, skipped) } -// 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. -func assertImagesMatchKustomize( +// assertInSyncIsANoOp hands kustomize's own render back as the live object — the folder is by +// definition converged — and requires the projection to route nothing and to leave the source +// document's images exactly as they are. +func assertInSyncIsANoOp( t *testing.T, ro renderedObject, src *unstructured.Unstructured, - chain *KustomizeOverrides, -) int { + attribution *RenderedOverrides, +) { t.Helper() - var entries []ImageOverride - if chain != nil { - entries = chain.Images - } - ours := map[string]string{} - for _, slot := range collectContainerSlots(src.Object) { - got, _ := renderImage(parseImageRef(slot.image), entries) - ours[slot.key] = got.String() + where := ro.OriginPath + " " + ro.Object.GetKind() + "/" + ro.Object.GetName() + + out, edits := SplitDesiredForOverrides(src.Object, asLiveObject(t, ro.Object), attribution) + + require.Empty(t, edits, + "%s: the folder is already in sync, so nothing may be routed to an entry", where) + + for _, slot := range collectImageSlots(out.Object) { + want := sourceImageAt(src.Object, slot.key) + if want == "" { + continue // the live object has a slot the source does not; not our claim + } + require.Equal(t, want, slot.image, + "%s: an in-sync folder must hand back the SOURCE image untouched at %q", where, slot.key) } +} - compared := 0 - for _, slot := range collectContainerSlots(ro.Object.Object) { - got, known := ours[slot.key] - if !known { - continue +// asLiveObject turns a RENDERED object into the shape a live one actually has. +// +// This is not a formality. A rendered object is NOT a valid unstructured: kustomize hands +// numbers back as Go `int`, and DeepCopyJSON accepts only the JSON types, so +// unstructured.DeepCopy PANICS on one outright ("cannot deep copy int"). The API server hands +// out JSON, so a real live object has int64 — round-tripping through JSON is what makes this +// fixture faithful rather than merely non-crashing. +// +// The same landmine sits under any code that reads a number off a rendered object with the +// standard helpers: unstructured.NestedInt64 reports found=FALSE on a rendered spec.replicas, +// and silently gives you zero. See renderedReplicaCount, which is why the projection does not +// use it. +func asLiveObject(t *testing.T, rendered *unstructured.Unstructured) *unstructured.Unstructured { + t.Helper() + encoded, err := json.Marshal(rendered.Object) + require.NoError(t, err) + var obj map[string]interface{} + require.NoError(t, json.Unmarshal(encoded, &obj)) + return &unstructured.Unstructured{Object: obj} +} + +// sourceImageAt is the image the SOURCE document holds at a slot, or "" when it has none. +func sourceImageAt(src map[string]interface{}, key string) string { + for _, slot := range collectImageSlots(src) { + if slot.key == key { + return slot.image } - want := slot.image // kustomize's render is the expected truth - compared++ - require.Equal(t, want, got, - "%s container %s: kustomize renders %q, our chain renders %q", - ro.OriginPath, slot.key, want, got) } - return compared + return "" } -// 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( +// ourAttributionFor is what the store attributes to a document, and whether it was found +// ambiguous (reached by more than one render root with differing answers, which we refuse to +// route through). +func ourAttributionFor( chains map[chainKey]*overrideAssignment, ro renderedObject, -) (*KustomizeOverrides, bool) { +) (*RenderedOverrides, bool) { a := chains[chainKey{ originPath: ro.OriginPath, kind: ro.Object.GetKind(), @@ -115,7 +150,7 @@ func ourChainFor( if a.ambiguous() { return nil, true } - return a.overrides, false + return a.rendered, false } // sourceDocFor finds the document in the origin file that produced a rendered diff --git a/internal/manifestanalyzer/override_chain.go b/internal/manifestanalyzer/override_chain.go index 934f3515..081f91a0 100644 --- a/internal/manifestanalyzer/override_chain.go +++ b/internal/manifestanalyzer/override_chain.go @@ -146,6 +146,10 @@ func renderChains( out := map[chainKey]*overrideAssignment{} failed := map[string]string{} + // One dye plan for the whole scan, so the nonces are stable across roots and a base + // reached by two overlays is dyed identically in both. Building it costs no render. + plan := planDye(files) + for _, rootDir := range renderTargets(kusts) { rendered, err := renderRoot(files, rootDir) if err != nil { @@ -154,6 +158,13 @@ func renderChains( } continue } + // BASELINE FIRST, THEN DYE. The real render is what we ship; the dyed one is only + // ever a question we ask about it. A dyed build that fails where the real one + // succeeded means the dye perturbed something that is not a pure sink, and the + // answer is then no attribution — never a fallback to a second guess. + dyed, dyeErr := renderRootWith(files, rootDir, plan.replace) + attribution := attributeRoot(rendered, dyed, dyeErr, plan) + for _, ro := range rendered { if ro.OriginPath == "" { continue // a generated resource: it has no source document to edit @@ -163,7 +174,7 @@ func renderChains( kind: ro.Object.GetKind(), name: ro.Object.GetName(), } - record(out, key, chainOf(ro, kusts)) + record(out, key, chainOf(ro, kusts), attribution[key]) } } return out, failed @@ -219,23 +230,29 @@ func chainOf(ro renderedObject, kusts map[string]*kustomizationDoc) *KustomizeOv // 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) +func record(out map[chainKey]*overrideAssignment, key chainKey, ov *KustomizeOverrides, rd *RenderedOverrides) { + // The fingerprint now covers the ATTRIBUTION as well as the chain, which is the sharper + // question the fan-in check was always trying to ask: two roots agree only if they + // attribute the same field to the same entry AND render it to the same value. + fp := fingerprint(ov) + "\x03" + fingerprintRendered(rd) prev, seen := out[key] if !seen { out[key] = &overrideAssignment{ chainKeys: map[string]struct{}{fp: {}}, overrides: ov, + rendered: rd, anyOverrides: ov != nil, } return } if _, same := prev.chainKeys[fp]; same { - return // the same chain, reached twice; not an ambiguity + return // the same chain and the same attribution, 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 + // More than one distinct answer: route through none of them. + prev.overrides = nil + prev.rendered = nil } // fingerprint reduces a chain to a comparable string, so two roots reaching one diff --git a/internal/manifestanalyzer/overrides.go b/internal/manifestanalyzer/overrides.go index 7445a5b1..36ca8c8a 100644 --- a/internal/manifestanalyzer/overrides.go +++ b/internal/manifestanalyzer/overrides.go @@ -72,6 +72,7 @@ type KustomizeOverrides struct { type overrideAssignment struct { chainKeys map[string]struct{} overrides *KustomizeOverrides + rendered *RenderedOverrides anyOverrides bool } @@ -87,17 +88,17 @@ func resolveOverrides( loc manifestedit.Location, id manifestedit.Identity, assignments map[chainKey]*overrideAssignment, -) (*KustomizeOverrides, *manifestedit.Diagnostic) { +) (*KustomizeOverrides, *RenderedOverrides, *manifestedit.Diagnostic) { a := assignments[chainKey{ originPath: filepathToSlash(loc.Path), kind: id.Kind, name: id.Name, }] if a == nil { - return nil, nil + return nil, nil, nil } if a.ambiguous() { - return nil, &manifestedit.Diagnostic{ + return nil, nil, &manifestedit.Diagnostic{ Level: manifestedit.DiagWarning, Reason: reasonAmbiguousOverrides, Message: "multiple render roots reach this file with different images/replicas override chains; " + @@ -106,7 +107,7 @@ func resolveOverrides( DocumentIndex: loc.DocumentIndex, } } - return a.overrides, nil + return a.overrides, a.rendered, nil } // OverridesAmbiguousAt reports whether the store refused to route a kustomize override chain diff --git a/internal/manifestanalyzer/overrides_attribution.go b/internal/manifestanalyzer/overrides_attribution.go new file mode 100644 index 00000000..cfbdc318 --- /dev/null +++ b/internal/manifestanalyzer/overrides_attribution.go @@ -0,0 +1,214 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "math" + "sort" + "strconv" + "strings" +) + +// This file turns a pair of renders — the real one and the dyed one — into the answer the +// projection needs: for this document, what does kustomize render each image and replica +// count to, and WHICH ENTRY supplied it. +// +// Both halves come from the renderer. The VALUES are read off the real render, so what we +// believe a folder renders to is what kustomize says it renders to. The SUPPLIERS are read +// off the dyed render, so which entry owns a field is observed rather than re-derived. +// Nothing here re-implements a transformer, which is the whole point: the transformers we +// used to re-implement are where every shipped bug in this area came from. +// +// See docs/design/support-boundary/render-attribution.md §3. + +// RenderedOverrides is what kustomize renders one document to, plus the override entry +// behind each override-produced value. A nil supplier means THE SOURCE DOCUMENT supplies +// that value — so an edit to it belongs in the file, not in an entry. +type RenderedOverrides struct { + // Images is keyed by image slot (the container list path plus the container name), so + // the live object, the Git document and the render all address the same field. + Images map[string]RenderedImage + // Replicas is set only when the document actually renders a spec.replicas — which is + // kustomize's decision, not ours. + Replicas *RenderedReplicas +} + +// RenderedImage is one image slot: what it renders to, and who supplied each component. +type RenderedImage struct { + // Rendered is the image kustomize produces for this slot. + Rendered string + // Name, Tag and Digest are the entries supplying each component, or nil when the + // source document does. They are the dye's answer, and the reason renderImage is gone. + Name, Tag, Digest *ImageOverride +} + +// RenderedReplicas is the rendered spec.replicas and the entry that pinned it (nil when the +// source document supplies the count). +type RenderedReplicas struct { + Rendered int64 + Entry *ReplicaOverride +} + +// attributeRoot pairs one root's real render with its dyed render and reads the dyes off it. +// +// BASELINE FIRST, THEN DYE. If the dyed build fails where the real one succeeded, the dye has +// perturbed something that is not a pure sink — a replacements: block consuming the image as +// a SOURCE will do exactly this — and the honest answer is that we cannot attribute this root. +// The fallback is NO ATTRIBUTION, and it is never another heuristic: not renderImage, not +// leave-one-out, not "probably the last matching entry". Those are the silent-corruption paths +// this design exists to delete, and the moment the renderer says "I cannot tell you" is the +// worst possible moment to start guessing. With no attribution nothing routes to an entry, the +// proposal falls back to what the source document alone can carry, and the verification +// re-render adjudicates it — which, for a field an entry governs, means a refused flush. That +// is the correct outcome, and it is reported rather than absorbed. +// +// Objects are aligned BY POSITION, never by name: a generated name can carry a content hash +// that drifts between two builds. The keys are then required to agree at every position, so a +// misalignment refuses attribution instead of silently attributing one object's dyes to +// another. +func attributeRoot(plain, dyed []renderedObject, dyeErr error, plan *dyePlan) map[chainKey]*RenderedOverrides { + if dyeErr != nil || len(plain) != len(dyed) { + return nil + } + out := make(map[chainKey]*RenderedOverrides, len(plain)) + for i := range plain { + key := renderedKey(plain[i]) + if key != renderedKey(dyed[i]) { + return nil // the two builds disagree on what they rendered; attribute nothing + } + if plain[i].OriginPath == "" { + continue // generated: no source document to route an edit into + } + if attribution := readDyes(plain[i], dyed[i], plan); attribution != nil { + out[key] = attribution + } + } + return out +} + +func renderedKey(o renderedObject) chainKey { + return chainKey{originPath: o.OriginPath, kind: o.Object.GetKind(), name: o.Object.GetName()} +} + +// readDyes reads the nonces out of ONE document's image slots and replica count. +// +// Only the fields being attributed are read. The whole output is never grepped for a nonce: +// vars and replacements can carry a dyed value into args, env, or ConfigMap data, and a dye +// found there says nothing about who supplies the image. +func readDyes(plain, dyed renderedObject, plan *dyePlan) *RenderedOverrides { + out := &RenderedOverrides{Images: map[string]RenderedImage{}} + + dyedSlots := map[string]imageSlot{} + for _, s := range collectImageSlots(dyed.Object.Object) { + dyedSlots[s.key] = s + } + for _, slot := range collectImageSlots(plain.Object.Object) { + image := RenderedImage{Rendered: slot.image} + if probe, found := dyedSlots[slot.key]; found { + ref := parseImageRef(probe.image) + image.Name = markedImage(plan, ref.name, fieldNewName) + image.Tag = markedImage(plan, ref.tag, fieldNewTag) + image.Digest = markedImage(plan, ref.digest, fieldDigest) + } + out.Images[slot.key] = image + } + + // spec.replicas is read off the object rather than gated on a list of kinds we believe + // the transformer touches. kustomize's fieldspec is the authority: if a dyed count came + // out here, an entry governs this field, whatever the kind is. That is how + // ReplicationController — which our own isReplicaKind forgot — attributes for free. + if count, ok := renderedReplicaCount(plain.Object.Object); ok { + replicas := &RenderedReplicas{Rendered: count} + if dyedCount, found := renderedReplicaCount(dyed.Object.Object); found { + if mark, isDye := plan.byReplicaCount[dyedCount]; isDye && mark.field == fieldCount { + replicas.Entry = mark.replica + } + } + out.Replicas = replicas + } + + if len(out.Images) == 0 && out.Replicas == nil { + return nil + } + return out +} + +// markedImage looks a candidate nonce up, and requires it to have been minted FOR THE FIELD IT +// WAS FOUND IN. A tag dye surfacing in the name position is not attribution, it is a signal +// that something we do not model moved the value, so it is not treated as a supplier. +func markedImage(plan *dyePlan, value, field string) *ImageOverride { + if value == "" { + return nil + } + mark, found := plan.byNonce[value] + if !found || mark.field != field { + return nil + } + return mark.image +} + +// renderedReplicaCount reads spec.replicas off a RENDERED object. +// +// unstructured.NestedInt64 is deliberately not used, and this is a landmine rather than a +// preference: kustomize hands numbers back as Go `int`, so NestedInt64 returns found=FALSE on +// a rendered spec.replicas and the caller silently reads zero. Measured. +func renderedReplicaCount(obj map[string]interface{}) (int64, bool) { + spec, ok := obj["spec"].(map[string]interface{}) + if !ok { + return 0, false + } + switch n := spec["replicas"].(type) { + case int: + return int64(n), true + case int64: + return n, true + case float64: + if n != math.Trunc(n) { + return 0, false + } + return int64(n), true + default: + return 0, false + } +} + +// fingerprintRendered reduces an attribution to a comparable string, so two render roots +// reaching one document can be compared for agreement exactly as their chains are. This is the +// sharper question the fan-in check was always trying to ask: do two roots attribute this +// field to DIFFERENT entries? +func fingerprintRendered(rd *RenderedOverrides) string { + if rd == nil { + return "" + } + keys := make([]string, 0, len(rd.Images)) + for k := range rd.Images { + keys = append(keys, k) + } + sort.Strings(keys) + + var b strings.Builder + for _, k := range keys { + img := rd.Images[k] + b.WriteString(k) + b.WriteByte(0) + b.WriteString(img.Rendered) + b.WriteByte(0) + b.WriteString(entryRef(img.Name) + entryRef(img.Tag) + entryRef(img.Digest)) + b.WriteByte(1) + } + if rd.Replicas != nil { + b.WriteString(strconv.FormatInt(rd.Replicas.Rendered, 10)) + b.WriteByte(0) + if e := rd.Replicas.Entry; e != nil { + b.WriteString(e.Source + ":" + strconv.Itoa(e.Index)) + } + } + return b.String() +} + +func entryRef(e *ImageOverride) string { + if e == nil { + return "-;" + } + return e.Source + ":" + strconv.Itoa(e.Index) + ";" +} diff --git a/internal/manifestanalyzer/overrides_projection.go b/internal/manifestanalyzer/overrides_projection.go index d9b597f8..14c2a2f7 100644 --- a/internal/manifestanalyzer/overrides_projection.go +++ b/internal/manifestanalyzer/overrides_projection.go @@ -32,29 +32,38 @@ type OverrideEdit struct { Edit manifestedit.KustomizationEdit } -// SplitDesiredForOverrides maps the live desired object back through the -// override chain. It returns the object the source document should be compared -// against (a copy of desired with override-produced values restored to their -// source form) plus the entry edits for values whose supplier is an override -// entry. Anything it cannot route safely — a component removal an entry -// supplies, conflicting values for one entry field, or a simulated render that -// would not reproduce live — falls back to the unmodified live value -// (today's write-through), never to a guess. +// SplitDesiredForOverrides maps the live desired object back through what kustomize actually +// renders. It returns the object the source document should be compared against — a copy of +// desired with every override-produced value restored to its SOURCE form, so the file keeps +// its bytes — plus the entry edits for the values an override entry supplies. // -// gitRaw is the source document parsed as JSON-typed maps (sigs.k8s.io/yaml); -// desired is the sanitized projection the writer would otherwise compare. The -// returned object is always a copy; desired is never mutated. +// It is driven by RenderedOverrides, which carries both halves of the answer straight from +// the renderer: what each field renders to, and which entry supplied it (read off a dyed +// counterfactual build). Nothing in here re-implements a kustomize transformer, which is the +// entire point of this workstream — every shipped bug in this area came from the +// re-implementation, and all of them are deleted with it. +// +// Anything it cannot route safely — a component removal an entry supplies, a component a +// sibling entry clears, or two containers demanding different values for one entry field — +// routes NOTHING and leaves the live value in place. That is not a guess and not a fallback to +// another heuristic: the proposal then has to survive the verification re-render, which for a +// field an entry governs it will not, so it becomes a reported refusal rather than a commit +// that quietly never converges. +// +// gitRaw is the source document parsed as JSON-typed maps (sigs.k8s.io/yaml); desired is the +// sanitized projection the writer would otherwise compare. The returned object is always a +// copy; desired is never mutated. func SplitDesiredForOverrides( gitRaw map[string]interface{}, desired *unstructured.Unstructured, - ov *KustomizeOverrides, + rendered *RenderedOverrides, ) (*unstructured.Unstructured, []OverrideEdit) { - if ov == nil || desired == nil || gitRaw == nil { + if rendered == nil || desired == nil || gitRaw == nil { return desired, nil } out := desired.DeepCopy() - edits := projectImages(gitRaw, out, ov.Images) - edits = append(edits, projectReplicas(gitRaw, out, ov.Replicas)...) + edits := projectImages(gitRaw, out, rendered.Images) + edits = append(edits, projectReplicas(gitRaw, out, rendered.Replicas)...) return out, edits } @@ -93,65 +102,13 @@ func (r imageRef) String() string { return out } -// imageSuppliers records which override entry last supplied each component of a -// rendered image; nil means the source file supplies it. -type imageSuppliers struct { - name *ImageOverride - tag *ImageOverride - digest *ImageOverride -} - -// renderImage runs the override chain over a source image, kustomize-style: -// each entry whose name matches the image's CURRENT name rewrites the -// components it declares, in chain order. -// -// Tag and digest are MUTUALLY EXCLUSIVE, and this is the part that is easy to get -// wrong — we did. Quoting kustomize's own image transformer -// (filters/imagetag/updater.go, SetImageValue): -// -// // overriding tag or digest will replace both original tag and digest values -// case NewTag != "" && Digest != "": tag = NewTag; digest = Digest -// case NewTag != "": tag = NewTag; digest = "" -// case Digest != "": tag = ""; digest = Digest -// -// Setting the two components independently makes us believe a folder renders to -// `web:1.0@sha256:abc` where kustomize renders `web@sha256:abc`. The projection -// then reads the difference as a user removing the tag and rewrites the tag out of -// the source file — silent corruption, on every reconcile. Pinned against a real -// `kustomize build` by TestRenderImage_MatchesKustomizeOnTheHardCases. -func renderImage(src imageRef, entries []ImageOverride) (imageRef, imageSuppliers) { - cur := src - var sup imageSuppliers - for i := range entries { - e := &entries[i] - if e.Name != cur.name { - continue - } - if e.HasNewName { - cur.name = e.NewName - sup.name = e - } - switch { - case e.HasNewTag && e.HasDigest: - cur.tag, cur.digest = e.NewTag, e.Digest - sup.tag, sup.digest = e, e - case e.HasNewTag: - cur.tag, cur.digest = e.NewTag, "" - sup.tag, sup.digest = e, e - case e.HasDigest: - cur.tag, cur.digest = "", e.Digest - sup.tag, sup.digest = e, e - } - } - return cur, sup -} - -// containerSlot is one container-shaped list item holding an image, addressed -// by its list path plus container name so the live and Git objects align. -type containerSlot struct { +// imageSlot is one image-bearing field of an object, addressed by its list path plus the +// item's name so the live object, the Git document and the render all address the same one. +// set writes a new image back into whichever shape the field has. +type imageSlot struct { key string - item map[string]interface{} image string + set func(string) } func isContainerListKey(k string) bool { @@ -163,24 +120,38 @@ func isContainerListKey(k string) bool { } } -// collectContainerSlots walks the object for container lists at any depth, -// mirroring the builtin image transformer's generic traversal. Slots are sorted -// by key for deterministic edit output. -func collectContainerSlots(obj map[string]interface{}) []containerSlot { - var out []containerSlot +// collectImageSlots walks the object for every field that can hold an image. +// +// Which fields those are was MEASURED against kustomize, not derived from its fieldspecs, +// and the two surprises are both in here: +// +// - volumes[].image.reference — an OCI volume source. kustomize REWRITES it (measured), and +// the old collector did not look at it, so the rendered value was written back into the +// source document as if the user had typed it. +// - ephemeralContainers — kustomize does NOT rewrite them (measured), so no dye ever lands +// here and no entry is ever credited with the value. They are still collected, because the +// SOURCE document owns them and an edit to one belongs in the file. That is the dye doing +// the fieldspec's job: we no longer have to know which fields kustomize touches, only to +// look at where its dyes came out. +// +// Slots are sorted by key so edit output is deterministic. +func collectImageSlots(obj map[string]interface{}) []imageSlot { + var out []imageSlot var walk func(prefix string, v interface{}) walk = func(prefix string, v interface{}) { switch t := v.(type) { case map[string]interface{}: for k, val := range t { p := prefix + "/" + k - if isContainerListKey(k) { - if list, ok := val.([]interface{}); ok { - out = append(out, containerSlotsOf(p, list)...) - continue - } + list, isList := val.([]interface{}) + switch { + case isList && isContainerListKey(k): + out = append(out, containerImageSlots(p, list)...) + case isList && k == "volumes": + out = append(out, volumeImageSlots(p, list)...) + default: + walk(p, val) } - walk(p, val) } case []interface{}: for i, item := range t { @@ -193,8 +164,9 @@ func collectContainerSlots(obj map[string]interface{}) []containerSlot { return out } -func containerSlotsOf(listPath string, list []interface{}) []containerSlot { - var out []containerSlot +// containerImageSlots reads containers[].image — a plain string field. +func containerImageSlots(listPath string, list []interface{}) []imageSlot { + var out []imageSlot for _, item := range list { m, isMap := item.(map[string]interface{}) if !isMap { @@ -205,53 +177,86 @@ func containerSlotsOf(listPath string, list []interface{}) []containerSlot { if name == "" || !hasImage { continue } - out = append(out, containerSlot{key: listPath + "\x00" + name, item: m, image: image}) + out = append(out, imageSlot{ + key: listPath + "\x00" + name, + image: image, + set: func(v string) { m["image"] = v }, + }) + } + return out +} + +// volumeImageSlots reads volumes[].image.reference — a nested field, and the one the old +// collector missed. A volume with no image (a configMap or emptyDir) simply has no slot. +func volumeImageSlots(listPath string, list []interface{}) []imageSlot { + var out []imageSlot + for _, item := range list { + m, isMap := item.(map[string]interface{}) + if !isMap { + continue + } + name, _ := m["name"].(string) + img, hasImage := m["image"].(map[string]interface{}) + if name == "" || !hasImage { + continue + } + reference, hasReference := img["reference"].(string) + if !hasReference { + continue + } + out = append(out, imageSlot{ + key: listPath + "\x00" + name, + image: reference, + set: func(v string) { img["reference"] = v }, + }) } return out } -// slotPlan is the per-container outcome of the inversion: the image the source -// file should hold, the live image it must render to, and the entry edits that -// make the render true. +// slotPlan is the per-slot outcome of the inversion: the image the source file should +// hold, and the entry edits that make the render come out as live. type slotPlan struct { - slot containerSlot + slot imageSlot fileImage string - live imageRef edits []OverrideEdit } -// projectImages inverts the image chain for every container the live object and -// the Git document share. It mutates out's container images to their -// source-file form and returns the entry edits — or routes nothing (leaving the -// live values in out) when the inversion is unsafe: conflicting edits to one entry -// field, or a removal an entry supplies. +// projectImages inverts the image transformers for every slot the live object and the Git +// document share. It rewrites out's images to their SOURCE-FILE form and returns the entry +// edits — or routes nothing when the inversion is unsafe. // -// Nothing here is trusted. The proposal it produces is put to kustomize before it can -// become a commit (VerifyWriteProposal), so this only has to be a candidate that is -// usually right — and routing nothing is always a legal answer, because the re-render -// then adjudicates whatever the source document alone can carry. +// Nothing here re-derives what kustomize does any more. The rendered value comes from the +// renderer and the supplier comes from the dye, so the two questions that used to be answered +// by a hand-written transformer — "what does this folder render to" and "who supplied it" — +// are now both answered by kustomize. +// +// And nothing here is trusted. The proposal is put to kustomize before it can become a commit +// (VerifyBatchRenders), so this only has to be a candidate that is usually right. Routing +// nothing is always a legal answer: the proposal then falls back to whatever the source +// document alone can carry, and the re-render adjudicates it. func projectImages( gitRaw map[string]interface{}, out *unstructured.Unstructured, - entries []ImageOverride, + rendered map[string]RenderedImage, ) []OverrideEdit { - if len(entries) == 0 { + if len(rendered) == 0 { return nil } gitImages := map[string]string{} - for _, s := range collectContainerSlots(gitRaw) { + for _, s := range collectImageSlots(gitRaw) { gitImages[s.key] = s.image } var plans []slotPlan - for _, slot := range collectContainerSlots(out.Object) { - src, exists := gitImages[slot.key] - if !exists { + for _, slot := range collectImageSlots(out.Object) { + src, inGit := gitImages[slot.key] + render, isRendered := rendered[slot.key] + if !inGit || !isRendered { continue // a new container writes through; the supplier rule converges it later } - plan, routable := invertImage(slot, src, entries) + plan, routable := invertImage(slot, src, render) if !routable { - return nil // one unroutable container abandons routing for the object + return nil // one unroutable slot abandons routing for the whole object } plans = append(plans, plan) } @@ -260,22 +265,24 @@ func projectImages( return nil } for _, p := range plans { - p.slot.item["image"] = p.fileImage + p.slot.set(p.fileImage) } return edits } -// invertImage computes one container's source-file image and entry edits. -// routable is false when a live change cannot be expressed on the existing -// entries (a component removal whose supplier is an entry). -func invertImage(slot containerSlot, src string, entries []ImageOverride) (slotPlan, bool) { +// invertImage computes one slot's source-file image and entry edits, given what kustomize +// renders it to and which entry supplied each component. +// +// routable is false when a live change cannot be expressed on the entries that exist, which +// abandons routing for the whole object. +func invertImage(slot imageSlot, src string, render RenderedImage) (slotPlan, bool) { srcRef := parseImageRef(src) - rendered, sup := renderImage(srcRef, entries) + rendered := parseImageRef(render.Rendered) live := parseImageRef(slot.image) - plan := slotPlan{slot: slot, live: live} + + plan := slotPlan{slot: slot, fileImage: src} if rendered == live { - plan.fileImage = src - return plan, true + return plan, true // the folder already renders to live; the file keeps its bytes } newSrc := srcRef @@ -291,24 +298,26 @@ func invertImage(slot containerSlot, src string, entries []ImageOverride) (slotP }, }) } + if live.name != rendered.name { - if sup.name != nil { - route(sup.name, "newName", live.name) - } else { + switch { + case render.Name != nil: + route(render.Name, fieldNewName, live.name) + default: newSrc.name = live.name } } if live.tag != rendered.tag { - if !routeComponent(sup.tag, declaresNewTag, live.tag, + if !routeComponent(render.Tag, render.Digest, live.tag, func(v string) { newSrc.tag = v }, - func(e *ImageOverride, v string) { route(e, "newTag", v) }) { + func(e *ImageOverride, v string) { route(e, fieldNewTag, v) }) { return plan, false } } if live.digest != rendered.digest { - if !routeComponent(sup.digest, declaresDigest, live.digest, + if !routeComponent(render.Digest, render.Tag, live.digest, func(v string) { newSrc.digest = v }, - func(e *ImageOverride, v string) { route(e, "digest", v) }) { + func(e *ImageOverride, v string) { route(e, fieldDigest, v) }) { return plan, false } } @@ -316,40 +325,44 @@ func invertImage(slot containerSlot, src string, entries []ImageOverride) (slotP return plan, true } -// declaresNewTag / declaresDigest report whether an entry actually carries the key -// the writer would have to set. An entry can GOVERN a component without declaring -// it: a digest entry clears the tag, and a newTag entry clears the digest. -func declaresNewTag(e *ImageOverride) bool { return e.HasNewTag } -func declaresDigest(e *ImageOverride) bool { return e.HasDigest } - -// routeComponent decides where one changed image component (tag or digest) goes: -// into the source file when no entry supplies it, onto the supplying entry when -// that entry declares the key, or nowhere at all. It reports false when the change -// is unroutable, which abandons routing for the whole object (write-through). +// routeComponent decides where one changed image component (tag or digest) goes: onto the +// entry that supplies it, into the source file when no entry does, or nowhere at all. +// +// TAG AND DIGEST ARE MUTUALLY EXCLUSIVE IN KUSTOMIZE, and that is what `sibling` is for. From +// its own image transformer (filters/imagetag/updater.go, SetImageValue): +// +// case NewTag != "" && Digest != "": tag = NewTag; digest = Digest +// case NewTag != "": tag = NewTag; digest = "" // a tag entry CLEARS the digest +// case Digest != "": tag = ""; digest = Digest // a digest entry CLEARS the tag // -// The two unroutable cases are worth naming: +// So an entry can GOVERN a component it does not declare. When a digest entry has cleared the +// tag, no dye lands in the tag — nothing supplies it — but writing a tag into the source file +// would be wiped by the very next render. The dye cannot see that on its own; the sibling +// component's supplier is what reveals it, and it is the bug (#231) that corrupted real source +// files by rewriting a tag out of them. // -// - a REMOVAL of a component an entry supplies — there is no way to say "no tag" -// on an entry that sets one; and -// - a change to a component an entry governs but does not declare — a digest entry -// clears the tag, so setting a tag has no key to land in, and writing it into the -// source file would be undone by the very next render. +// The two unroutable cases: +// +// - a REMOVAL of a component an entry supplies — there is no way to say "no tag" on an +// entry that sets one; +// - a change to a component the SIBLING entry clears — nowhere to land, and the file would +// be overridden straight back. func routeComponent( - sup *ImageOverride, - declares func(*ImageOverride) bool, + supplier *ImageOverride, + sibling *ImageOverride, live string, setSource func(string), route func(*ImageOverride, string), ) bool { switch { - case sup == nil: - setSource(live) // the source file supplies it; the change flows into the file - case live == "": - return false - case !declares(sup): - return false + case supplier != nil && live == "": + return false // cannot express "no tag" on an entry that sets one + case supplier != nil: + route(supplier, live) + case sibling != nil: + return false // the sibling component's entry clears this one; the file cannot own it default: - route(sup, live) + setSource(live) // the source document supplies it; the change flows into the file } return true } @@ -394,31 +407,25 @@ func collectConsistentEdits(plans []slotPlan) ([]OverrideEdit, bool) { return out, true } -// replicaKinds are the kinds the builtin replica transformer touches. -func isReplicaKind(kind string) bool { - switch kind { - case "Deployment", "ReplicaSet", "StatefulSet": - return true - default: - return false - } -} - -// projectReplicas inverts the replicas transformer: when an entry pins this -// document's replica count, the source form of spec.replicas is restored on out -// (including its absence — the transformer creates the field) and a count edit -// is emitted only when live diverges from the pinned count. +// projectReplicas inverts the replica transformer: when an entry pins this document's replica +// count, the source form of spec.replicas is restored on out (including its ABSENCE — the +// transformer creates the field) and a count edit is emitted only when live diverges from the +// pinned count. +// +// There is no list of kinds here any more, and that is a bug fix rather than a tidy-up. We +// used to gate this on isReplicaKind — Deployment, ReplicaSet, StatefulSet — while kustomize's +// fieldspec is Deployment, ReplicaSet, StatefulSet AND ReplicationController. A scale on an RC +// governed by a replicas: entry was written into the source document, where the transformer +// overrode it right back: non-converging drift, silently, forever. The dye ends the argument: +// if a dyed count came out of this object, an entry governs the field, whatever the kind is. +// kustomize's fieldspec is the authority, and we no longer keep a second opinion about it. func projectReplicas( gitRaw map[string]interface{}, out *unstructured.Unstructured, - entries []ReplicaOverride, + rendered *RenderedReplicas, ) []OverrideEdit { - if len(entries) == 0 || !isReplicaKind(out.GetKind()) { - return nil - } - sup := replicaSupplier(entries, out.GetName()) - if sup == nil { - return nil + if rendered == nil || rendered.Entry == nil { + return nil // no entry supplies the count; a scale flows into the source document } liveCount, liveHas, err := unstructured.NestedInt64(out.Object, "spec", "replicas") if err != nil || !liveHas { @@ -426,55 +433,34 @@ func projectReplicas( } restoreSourceReplicas(gitRaw, out) - if liveCount == sup.Count { - return nil + if liveCount == rendered.Rendered { + return nil // the folder already renders to live } - return []OverrideEdit{{ - KustomizationPath: sup.Source, - Edit: manifestedit.KustomizationEdit{ - Section: manifestedit.KustomizationSectionReplicas, - EntryIndex: sup.Index, - EntryName: sup.Name, - Field: "count", - Value: strconv.FormatInt(liveCount, 10), - }, - }} + return []OverrideEdit{replicaCountEdit(rendered.Entry, liveCount)} } -// replicaSupplier is the last entry in the chain matching the document's name — -// the one whose count the render ends up with. -func replicaSupplier(entries []ReplicaOverride, name string) *ReplicaOverride { - var sup *ReplicaOverride - for i := range entries { - if entries[i].Name == name { - sup = &entries[i] - } - } - return sup -} - -// ReplicaCountEdit returns the entry edit that absorbs a live replica count for -// the document, when its override chain governs spec.replicas. The writer's -// field-patch path (the /scale subresource) uses it to route a scale to the -// kustomization entry instead of writing the count into the source manifest. +// ReplicaCountEdit returns the entry edit that absorbs a live replica count for the document, +// when a replicas: entry supplies spec.replicas. The writer's field-patch path (the /scale +// subresource) uses it to route a scale onto the entry instead of writing the count into the +// source manifest, where the transformer would override it back. func ReplicaCountEdit(dm *DocumentModel, count int64) (OverrideEdit, bool) { - if dm == nil || dm.Overrides == nil || !isReplicaKind(dm.ManifestIdentity.Kind) { - return OverrideEdit{}, false - } - sup := replicaSupplier(dm.Overrides.Replicas, dm.ManifestIdentity.Name) - if sup == nil { + if dm == nil || dm.Rendered == nil || dm.Rendered.Replicas == nil || dm.Rendered.Replicas.Entry == nil { return OverrideEdit{}, false } + return replicaCountEdit(dm.Rendered.Replicas.Entry, count), true +} + +func replicaCountEdit(entry *ReplicaOverride, count int64) OverrideEdit { return OverrideEdit{ - KustomizationPath: sup.Source, + KustomizationPath: entry.Source, Edit: manifestedit.KustomizationEdit{ Section: manifestedit.KustomizationSectionReplicas, - EntryIndex: sup.Index, - EntryName: sup.Name, - Field: "count", + EntryIndex: entry.Index, + EntryName: entry.Name, + Field: fieldCount, Value: strconv.FormatInt(count, 10), }, - }, true + } } // sourceReplicaCount reads spec.replicas off a source KRM document. This is a diff --git a/internal/manifestanalyzer/overrides_projection_test.go b/internal/manifestanalyzer/overrides_projection_test.go index 68983bd3..efcbddc5 100644 --- a/internal/manifestanalyzer/overrides_projection_test.go +++ b/internal/manifestanalyzer/overrides_projection_test.go @@ -3,14 +3,91 @@ package manifestanalyzer import ( + "fmt" "testing" + "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/yaml" "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" ) -func deploymentObj(image string, replicas *int64) map[string]interface{} { +// These tests drive the projection with GROUND TRUTH. +// +// They used to construct (gitRaw, desired, overrides) by hand, which meant they asserted what +// we BELIEVED a folder renders to. That is exactly the belief that was wrong — twice, in +// shipped code — so a test net woven out of it could not have caught either bug, and did not. +// +// Now every case builds a small real repository, renders it with kustomize, reads the +// attribution off a dyed counterfactual render, and drives the projection with the result. A +// row that disagrees with kustomize now fails here instead of silently corrupting a source +// file, and if kustomize ever changes its semantics these fail rather than the operator. + +// splitFixture is the whole harness: build the tree, ask kustomize what it renders and which +// entry supplied what, then invert the live object against that answer. +func splitFixture( + t *testing.T, + files []manifestedit.FileContent, + sourcePath string, + live map[string]interface{}, +) (*unstructured.Unstructured, []OverrideEdit) { + t.Helper() + desired := &unstructured.Unstructured{Object: live} + + assignments, failed := renderChains(files, parseKustomizations(files)) + require.Empty(t, failed, "the fixture must be a folder kustomize can build") + + assignment := assignments[chainKey{ + originPath: sourcePath, + kind: desired.GetKind(), + name: desired.GetName(), + }] + require.NotNil(t, assignment, "kustomize must render %s/%s from %s", + desired.GetKind(), desired.GetName(), sourcePath) + + var gitRaw map[string]interface{} + require.NoError(t, yaml.Unmarshal(contentOf(t, files, sourcePath), &gitRaw)) + + return SplitDesiredForOverrides(gitRaw, desired, assignment.rendered) +} + +func contentOf(t *testing.T, files []manifestedit.FileContent, path string) []byte { + t.Helper() + for _, f := range files { + if f.Path == path { + return f.Content + } + } + t.Fatalf("fixture has no file %q", path) + return nil +} + +func file(path, content string) manifestedit.FileContent { + return manifestedit.FileContent{Path: path, Content: []byte(content)} +} + +// deploymentSource is the Deployment as GIT holds it. +func deploymentSource(image string, replicas string) string { + if replicas != "" { + replicas = " replicas: " + replicas + "\n" + } + return fmt.Sprintf(`apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: +%s template: + spec: + containers: + - name: app + image: %s +`, replicas, image) +} + +// liveDeployment is the Deployment as the CLUSTER holds it — the rendered form, which is what +// the source file plus the entries have to reproduce. +func liveDeployment(image string, replicas *int64) map[string]interface{} { spec := map[string]interface{}{ "template": map[string]interface{}{ "spec": map[string]interface{}{ @@ -31,314 +108,476 @@ func deploymentObj(image string, replicas *int64) map[string]interface{} { } } -func desiredOf(obj map[string]interface{}) *unstructured.Unstructured { - u := &unstructured.Unstructured{Object: obj} - // Live objects carry int64 replicas; normalize the fixture. - if v, ok, _ := unstructured.NestedFieldNoCopy(obj, "spec", "replicas"); ok { - if n, isInt := v.(int); isInt { - _ = unstructured.SetNestedField(obj, int64(n), "spec", "replicas") - } - } - return u +func kustomizationWith(body string) string { + return "resources:\n - deployment.yaml\n" + body } -func desiredImage(t *testing.T, u *unstructured.Unstructured) string { +func imageOf(t *testing.T, u *unstructured.Unstructured) string { t.Helper() - slots := collectContainerSlots(u.Object) - if len(slots) != 1 { - t.Fatalf("want exactly one container slot, got %d", len(slots)) - } + slots := collectImageSlots(u.Object) + require.Len(t, slots, 1, "want exactly one image slot") return slots[0].image } -func imgEntry(name string, set map[string]string) ImageOverride { - e := ImageOverride{Source: "kustomization.yaml", Name: name} - if v, ok := set["newName"]; ok { - e.NewName, e.HasNewName = v, true - } - if v, ok := set["newTag"]; ok { - e.NewTag, e.HasNewTag = v, true - } - if v, ok := set["digest"]; ok { - e.Digest, e.HasDigest = v, true +func imagesEntry(body string) []manifestedit.FileContent { + return []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("ghcr.io/example/app:1.0.0", "")), + file("kustomization.yaml", kustomizationWith(body)), } - return e } -// TestSplitDesired_TagRoutedToEntry pins the core edit-through behavior: a live tag change -// produced by a newTag entry lands on the entry and the file keeps its bytes. +// The core of the edit-through: a live tag change produced by a newTag entry lands on the +// entry, and the source file keeps its bytes. func TestSplitDesired_TagRoutedToEntry(t *testing.T) { - git := deploymentObj("ghcr.io/example/app:1.0.0", nil) - desired := desiredOf(deploymentObj("ghcr.io/example/app:2.0.0", nil)) - ov := &KustomizeOverrides{Images: []ImageOverride{ - imgEntry("ghcr.io/example/app", map[string]string{"newTag": "1.5.0"}), - }} - - out, edits := SplitDesiredForOverrides(git, desired, ov) - if got := desiredImage(t, out); got != "ghcr.io/example/app:1.0.0" { - t.Errorf("file image = %q, want the source form restored", got) - } - if len(edits) != 1 { - t.Fatalf("want one entry edit, got %+v", edits) - } - e := edits[0] - if e.KustomizationPath != "kustomization.yaml" || - e.Edit != (manifestedit.KustomizationEdit{ + files := imagesEntry("images:\n - name: ghcr.io/example/app\n newTag: \"1.5.0\"\n") + + out, edits := splitFixture(t, files, "deployment.yaml", + liveDeployment("ghcr.io/example/app:2.0.0", nil)) + + require.Equal(t, "ghcr.io/example/app:1.0.0", imageOf(t, out), "the source form must be restored") + require.Len(t, edits, 1) + require.Equal(t, OverrideEdit{ + KustomizationPath: "kustomization.yaml", + Edit: manifestedit.KustomizationEdit{ Section: manifestedit.KustomizationSectionImages, EntryIndex: 0, EntryName: "ghcr.io/example/app", Field: "newTag", Value: "2.0.0", - }) { - t.Errorf("unexpected edit %+v", e) - } + }, + }, edits[0]) } -// TestSplitDesired_LiveMatchesRenderIsNoOp pins the write-through fix: live equal -// to the rendered value must restore the source form and route nothing, so the -// source file's "stale" tag is never overwritten. -func TestSplitDesired_LiveMatchesRenderIsNoOp(t *testing.T) { - git := deploymentObj("ghcr.io/example/app:1.0.0", nil) - desired := desiredOf(deploymentObj("ghcr.io/example/app:1.5.0", nil)) - ov := &KustomizeOverrides{Images: []ImageOverride{ - imgEntry("ghcr.io/example/app", map[string]string{"newTag": "1.5.0"}), - }} - - out, edits := SplitDesiredForOverrides(git, desired, ov) - if got := desiredImage(t, out); got != "ghcr.io/example/app:1.0.0" { - t.Errorf("file image = %q, want source form", got) +// THE IDEMPOTENT PIN — the case that killed leave-one-out probing, and the reason the dye +// exists. +// +// The source is already at v1 and the overlay pins v1: the state every repository is in the +// moment a release lands in both places. Removing the entry moves NOTHING, so a probe that +// asks "what changed?" concludes the source file supplies the tag — and writes the user's next +// tag into the base, where the overlay overrides it straight back, on every reconcile, forever. +// +// The dye is not fooled: a nonce nothing else can produce comes out of that field, so the +// entry is the supplier even though its value is identical to the source's. +func TestSplitDesired_IdempotentPinIsStillAttributedToTheEntry(t *testing.T) { + files := []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("app:v1", "")), + file("kustomization.yaml", kustomizationWith("images:\n - name: app\n newTag: v1\n")), } - if len(edits) != 0 { - t.Errorf("want no edits, got %+v", edits) + + out, edits := splitFixture(t, files, "deployment.yaml", liveDeployment("app:v2", nil)) + + require.Equal(t, "app:v1", imageOf(t, out), "the base must NOT absorb the tag; the entry owns it") + require.Len(t, edits, 1, "the tag must land on the entry, or it never converges") + require.Equal(t, "newTag", edits[0].Edit.Field) + require.Equal(t, "v2", edits[0].Edit.Value) +} + +// A TIE: two entries pinning the same tag. Removal cannot attribute either of them — drop +// one and the other still produces the same bytes. The dye reads the LAST writer straight off +// the output, because the two nonces are distinguishable even though the two values were not. +func TestSplitDesired_TieIsAttributedToTheLastWriter(t *testing.T) { + files := []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("app:v1", "")), + file("kustomization.yaml", kustomizationWith( + "images:\n - name: app\n newTag: v9\n - name: app\n newTag: v9\n")), } + + out, edits := splitFixture(t, files, "deployment.yaml", liveDeployment("app:v10", nil)) + + require.Equal(t, "app:v1", imageOf(t, out)) + require.Len(t, edits, 1) + require.Equal(t, 1, edits[0].Edit.EntryIndex, "the LAST matching entry is the one that renders") + require.Equal(t, "v10", edits[0].Edit.Value) } -// TestSplitDesired_UngovernedComponentPatchesFile: the matching entry declares -// only newName, so a tag change is file-supplied and flows into the source image -// while the name stays in its source form. +// Live equal to the rendered value is a full no-op: the source file's "stale" tag is dead text +// the entry shadows, and must not be overwritten. +func TestSplitDesired_LiveMatchesRenderIsNoOp(t *testing.T) { + files := imagesEntry("images:\n - name: ghcr.io/example/app\n newTag: \"1.5.0\"\n") + + out, edits := splitFixture(t, files, "deployment.yaml", + liveDeployment("ghcr.io/example/app:1.5.0", nil)) + + require.Equal(t, "ghcr.io/example/app:1.0.0", imageOf(t, out), "the source keeps its bytes") + require.Empty(t, edits) +} + +// The matching entry declares only newName, so the TAG is file-supplied: it flows into the +// source image while the name stays in its source form. func TestSplitDesired_UngovernedComponentPatchesFile(t *testing.T) { - git := deploymentObj("old/app:1.0.0", nil) - desired := desiredOf(deploymentObj("new/app:2.0.0", nil)) - ov := &KustomizeOverrides{Images: []ImageOverride{ - imgEntry("old/app", map[string]string{"newName": "new/app"}), - }} - - out, edits := SplitDesiredForOverrides(git, desired, ov) - if got := desiredImage(t, out); got != "old/app:2.0.0" { - t.Errorf("file image = %q, want source name with the live tag", got) - } - if len(edits) != 0 { - t.Errorf("tag is file-supplied here; want no edits, got %+v", edits) + files := []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("old/app:1.0.0", "")), + file("kustomization.yaml", kustomizationWith( + "images:\n - name: old/app\n newName: new/app\n")), } + + out, edits := splitFixture(t, files, "deployment.yaml", liveDeployment("new/app:2.0.0", nil)) + + require.Equal(t, "old/app:2.0.0", imageOf(t, out), "the source name stays; the live tag lands in the file") + require.Empty(t, edits, "no entry supplies the tag") } -// TestSplitDesired_NameChangeRoutedToNewName: a live name change whose supplier -// is a newName entry updates the entry. +// A live name change whose supplier is a newName entry updates the entry. func TestSplitDesired_NameChangeRoutedToNewName(t *testing.T) { - git := deploymentObj("old/app:1.0.0", nil) - desired := desiredOf(deploymentObj("mirror/app:1.0.0", nil)) - ov := &KustomizeOverrides{Images: []ImageOverride{ - imgEntry("old/app", map[string]string{"newName": "new/app"}), - }} - - out, edits := SplitDesiredForOverrides(git, desired, ov) - if got := desiredImage(t, out); got != "old/app:1.0.0" { - t.Errorf("file image = %q, want untouched source", got) - } - if len(edits) != 1 || edits[0].Edit.Field != "newName" || edits[0].Edit.Value != "mirror/app" { - t.Fatalf("want one newName edit to mirror/app, got %+v", edits) + files := []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("old/app:1.0.0", "")), + file("kustomization.yaml", kustomizationWith( + "images:\n - name: old/app\n newName: new/app\n")), } + + out, edits := splitFixture(t, files, "deployment.yaml", liveDeployment("mirror/app:1.0.0", nil)) + + require.Equal(t, "old/app:1.0.0", imageOf(t, out), "the source is untouched") + require.Len(t, edits, 1) + require.Equal(t, "newName", edits[0].Edit.Field) + require.Equal(t, "mirror/app", edits[0].Edit.Value) } -// TestSplitDesired_DigestRoutedToEntry: a live digest change whose supplier is a -// digest entry updates the entry; tag and name keep their source form. -// -// Note the live image carries NO tag. A digest entry replaces the tag as well as -// the digest (kustomize: "overriding tag or digest will replace both original tag -// and digest values"), so `app@sha256:...` is the only thing this folder can -// render. This test used to assert `app:1.0.0@sha256:ccc` — a live state kustomize -// cannot produce — because renderImage set the two components independently. +// A digest entry replaces the TAG as well as the digest — kustomize: "overriding tag or digest +// will replace both original tag and digest values" — so `app@sha256:...` is the only thing +// this folder can render, and the live object carries no tag. func TestSplitDesired_DigestRoutedToEntry(t *testing.T) { - git := deploymentObj("app:1.0.0", nil) - desired := desiredOf(deploymentObj("app@sha256:ccc", nil)) - ov := &KustomizeOverrides{Images: []ImageOverride{ - imgEntry("app", map[string]string{"digest": "sha256:bbb"}), - }} - - out, edits := SplitDesiredForOverrides(git, desired, ov) - if got := desiredImage(t, out); got != "app:1.0.0" { - t.Errorf("file image = %q, want untouched source (no digest)", got) - } - if len(edits) != 1 || - edits[0].Edit != (manifestedit.KustomizationEdit{ - Section: manifestedit.KustomizationSectionImages, EntryIndex: 0, - EntryName: "app", Field: "digest", Value: "sha256:ccc", - }) { - t.Fatalf("want one digest edit to sha256:ccc, got %+v", edits) + files := []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("app:1.0.0", "")), + file("kustomization.yaml", kustomizationWith( + "images:\n - name: app\n digest: sha256:bbb\n")), } + + out, edits := splitFixture(t, files, "deployment.yaml", liveDeployment("app@sha256:ccc", nil)) + + require.Equal(t, "app:1.0.0", imageOf(t, out), "the source keeps its tag") + require.Len(t, edits, 1) + require.Equal(t, manifestedit.KustomizationEdit{ + Section: manifestedit.KustomizationSectionImages, EntryIndex: 0, + EntryName: "app", Field: "digest", Value: "sha256:ccc", + }, edits[0].Edit) } -// TestSplitDesired_DigestEntryDoesNotStripTheSourceTag pins the corruption that a -// hand-written image transformer caused, so it cannot come back. +// The #231 corruption, pinned so it cannot come back. // -// Source `app:1.0.0`, an entry supplying only a digest. kustomize renders -// `app@sha256:bbb` — the digest REPLACES the tag. We used to believe the render was -// `app:1.0.0@sha256:bbb`, so on seeing the real live object (no tag) the projection -// concluded the user had removed the tag and rewrote `app:1.0.0` to `app` in the -// source file. On every reconcile, silently. -// -// The source document must come back untouched. +// Source `app:1.0.0`, an entry supplying only a digest. kustomize renders `app@sha256:bbb` — +// the digest REPLACES the tag. The hand-written transformer believed the render was +// `app:1.0.0@sha256:bbb`, so on seeing the real live object (no tag) it concluded the user had +// removed the tag, and rewrote `app:1.0.0` to `app` in the source file. On every reconcile, +// silently. Nothing about the source document may change here. func TestSplitDesired_DigestEntryDoesNotStripTheSourceTag(t *testing.T) { - git := deploymentObj("app:1.0.0", nil) - // What the folder actually renders to, mirrored back unchanged by the user. - desired := desiredOf(deploymentObj("app@sha256:bbb", nil)) - ov := &KustomizeOverrides{Images: []ImageOverride{ - imgEntry("app", map[string]string{"digest": "sha256:bbb"}), - }} - - out, edits := SplitDesiredForOverrides(git, desired, ov) - if got := desiredImage(t, out); got != "app:1.0.0" { - t.Errorf("source image = %q, want it untouched at app:1.0.0 — the tag must not be stripped", got) - } - if len(edits) != 0 { - t.Errorf("live matches the render, so nothing should be routed; got %+v", edits) + files := []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("app:1.0.0", "")), + file("kustomization.yaml", kustomizationWith( + "images:\n - name: app\n digest: sha256:bbb\n")), } + + out, edits := splitFixture(t, files, "deployment.yaml", liveDeployment("app@sha256:bbb", nil)) + + require.Equal(t, "app:1.0.0", imageOf(t, out), "the tag must NOT be stripped out of the source") + require.Empty(t, edits, "live already matches the render") } -// TestSplitDesired_RemovalUnroutable: live drops the digest an entry supplies; -// nothing can express that on the entry, so the whole object writes through. -func TestSplitDesired_RemovalUnroutable(t *testing.T) { - git := deploymentObj("app:1.0.0", nil) - desired := desiredOf(deploymentObj("app:1.0.0", nil)) - ov := &KustomizeOverrides{Images: []ImageOverride{ - imgEntry("app", map[string]string{"digest": "sha256:abc"}), - }} - - out, edits := SplitDesiredForOverrides(git, desired, ov) - if got := desiredImage(t, out); got != "app:1.0.0" { - t.Errorf("file image = %q, want the live value written through", got) - } - if len(edits) != 0 { - t.Errorf("want no edits on write-through, got %+v", edits) +// Live drops the digest an entry supplies. There is no way to say "no digest" on an entry that +// sets one, so nothing routes and the object writes through — where the oracle then adjudicates +// it, because a write-through of a governed field does not converge. +func TestSplitDesired_RemovalIsUnroutable(t *testing.T) { + files := []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("app:1.0.0", "")), + file("kustomization.yaml", kustomizationWith( + "images:\n - name: app\n digest: sha256:abc\n")), } + + out, edits := splitFixture(t, files, "deployment.yaml", liveDeployment("app:1.0.0", nil)) + + require.Equal(t, "app:1.0.0", imageOf(t, out), "the live value is written through") + require.Empty(t, edits) } -// TestSplitDesired_ConflictingContainersAbandonRouting: two containers governed -// by one entry cannot pin two different tags on it. +// Two containers governed by one entry cannot pin two different tags on it. func TestSplitDesired_ConflictingContainersAbandonRouting(t *testing.T) { - containers := func(tagA, tagB string) map[string]interface{} { - return map[string]interface{}{ - "apiVersion": "apps/v1", - "kind": "Deployment", - "metadata": map[string]interface{}{"name": "web"}, - "spec": map[string]interface{}{ - "template": map[string]interface{}{ - "spec": map[string]interface{}{ - "containers": []interface{}{ - map[string]interface{}{"name": "a", "image": "app:" + tagA}, - map[string]interface{}{"name": "b", "image": "app:" + tagB}, - }, + source := `apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + template: + spec: + containers: + - name: a + image: app:1.0.0 + - name: b + image: app:1.0.0 +` + files := []manifestedit.FileContent{ + file("deployment.yaml", source), + file("kustomization.yaml", kustomizationWith("images:\n - name: app\n newTag: \"1.5.0\"\n")), + } + live := map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{"name": "web"}, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{"name": "a", "image": "app:2.0.0"}, + map[string]interface{}{"name": "b", "image": "app:3.0.0"}, }, }, }, - } - } - git := containers("1.0.0", "1.0.0") - desired := desiredOf(containers("2.0.0", "3.0.0")) - ov := &KustomizeOverrides{Images: []ImageOverride{ - imgEntry("app", map[string]string{"newTag": "1.5.0"}), - }} - - out, edits := SplitDesiredForOverrides(git, desired, ov) - if len(edits) != 0 { - t.Fatalf("conflicting demands must abandon routing, got %+v", edits) - } - slots := collectContainerSlots(out.Object) - if slots[0].image != "app:2.0.0" || slots[1].image != "app:3.0.0" { - t.Errorf("write-through must keep the live values, got %q / %q", slots[0].image, slots[1].image) + }, } + + out, edits := splitFixture(t, files, "deployment.yaml", live) + + require.Empty(t, edits, "one entry cannot carry two different tags") + slots := collectImageSlots(out.Object) + require.Len(t, slots, 2) + require.Equal(t, "app:2.0.0", slots[0].image, "write-through keeps the live values") + require.Equal(t, "app:3.0.0", slots[1].image) } -// TestSplitDesired_ChainedEntriesCompose: a base renames the image, the parent -// pins the tag of the renamed image; a live tag change lands on the parent entry. +// A base renames the image and the overlay pins the tag OF THE RENAMED IMAGE. A live tag change +// lands on the overlay's entry. +// +// This is also the rename-chain guard doing its job: the overlay's name: matches the base's +// newName:, so dyeing newName would stop the overlay's entry matching and change the render's +// shape. Names therefore go undyed here — and the TAG is still attributed exactly, because a +// tag dye is a pure sink even inside a rename chain. func TestSplitDesired_ChainedEntriesCompose(t *testing.T) { - git := deploymentObj("app:1.0.0", nil) - desired := desiredOf(deploymentObj("mirror/app:9.9.9", nil)) - base := imgEntry("app", map[string]string{"newName": "mirror/app"}) - base.Source = "base/kustomization.yaml" - parent := imgEntry("mirror/app", map[string]string{"newTag": "2.0.0"}) - ov := &KustomizeOverrides{Images: []ImageOverride{base, parent}} - - out, edits := SplitDesiredForOverrides(git, desired, ov) - if got := desiredImage(t, out); got != "app:1.0.0" { - t.Errorf("file image = %q, want untouched source", got) - } - if len(edits) != 1 || edits[0].KustomizationPath != "kustomization.yaml" || - edits[0].Edit.Field != "newTag" || edits[0].Edit.Value != "9.9.9" { - t.Fatalf("want one newTag edit on the parent entry, got %+v", edits) + files := []manifestedit.FileContent{ + file("base/deployment.yaml", deploymentSource("app:1.0.0", "")), + file("base/kustomization.yaml", + "resources:\n - deployment.yaml\nimages:\n - name: app\n newName: mirror/app\n"), + file("kustomization.yaml", + "resources:\n - base\nimages:\n - name: mirror/app\n newTag: \"2.0.0\"\n"), + } + + out, edits := splitFixture(t, files, "base/deployment.yaml", + liveDeployment("mirror/app:9.9.9", nil)) + + require.Equal(t, "app:1.0.0", imageOf(t, out), "the source is untouched") + require.Len(t, edits, 1) + require.Equal(t, "kustomization.yaml", edits[0].KustomizationPath, "the OVERLAY's entry owns the tag") + require.Equal(t, "newTag", edits[0].Edit.Field) + require.Equal(t, "9.9.9", edits[0].Edit.Value) +} + +// B1: an images: entry's name: is a REGULAR EXPRESSION, and kustomize matches on it as one. +// Our matcher was string equality, so we believed `- name: "ap."` matched nothing while +// kustomize rewrote the image — and the projection then read the difference as a user edit and +// wrote the rendered value into the source manifest, killing the entry. The dye cannot make +// that mistake: it does not match anything, it reads where kustomize's own nonce came out. +func TestSplitDesired_RegexEntryNameIsAttributed(t *testing.T) { + files := []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("app:1.0.0", "")), + file("kustomization.yaml", kustomizationWith("images:\n - name: \"ap.\"\n newTag: \"1.5.0\"\n")), } + + out, edits := splitFixture(t, files, "deployment.yaml", liveDeployment("app:2.0.0", nil)) + + require.Equal(t, "app:1.0.0", imageOf(t, out), "the source keeps its bytes; the regex entry owns the tag") + require.Len(t, edits, 1, "kustomize matched this entry, so we must attribute to it") + require.Equal(t, "newTag", edits[0].Edit.Field) + require.Equal(t, "2.0.0", edits[0].Edit.Value) } -// TestSplitDesired_ReplicasRoutedToEntry: a pinned count absorbs the live scale; -// the file's replicas field is restored to its source form (absent here). +// A pinned count absorbs the live scale; the file's replicas field is restored to its source +// form — here, ABSENT, because the transformer creates the field. func TestSplitDesired_ReplicasRoutedToEntry(t *testing.T) { five := int64(5) - git := deploymentObj("app:1.0.0", nil) // source has no spec.replicas - desired := desiredOf(deploymentObj("app:1.0.0", &five)) - ov := &KustomizeOverrides{Replicas: []ReplicaOverride{ - {Source: "kustomization.yaml", Index: 0, Name: "web", Count: 3}, - }} - - out, edits := SplitDesiredForOverrides(git, desired, ov) - if _, has, _ := unstructured.NestedInt64(out.Object, "spec", "replicas"); has { - t.Errorf("source has no replicas field, so the desired-for-file must not either") - } - if len(edits) != 1 || - edits[0].Edit != (manifestedit.KustomizationEdit{ - Section: manifestedit.KustomizationSectionReplicas, EntryIndex: 0, - EntryName: "web", Field: "count", Value: "5", - }) { - t.Fatalf("want one count edit to 5, got %+v", edits) + files := []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("app:1.0.0", "")), // no spec.replicas + file("kustomization.yaml", kustomizationWith("replicas:\n - name: web\n count: 3\n")), } + + out, edits := splitFixture(t, files, "deployment.yaml", liveDeployment("app:1.0.0", &five)) + + _, has, _ := unstructured.NestedInt64(out.Object, "spec", "replicas") + require.False(t, has, "the source has no replicas field, so the desired-for-file must not either") + require.Len(t, edits, 1) + require.Equal(t, manifestedit.KustomizationEdit{ + Section: manifestedit.KustomizationSectionReplicas, EntryIndex: 0, + EntryName: "web", Field: "count", Value: "5", + }, edits[0].Edit) } -// TestSplitDesired_ReplicasMatchRestoresSource: live equals the pinned count; the -// source's own (stale) value is restored and nothing is routed. +// Live equals the pinned count: the source's own (stale) value is restored and nothing routes. func TestSplitDesired_ReplicasMatchRestoresSource(t *testing.T) { - one, three := int64(1), int64(3) - git := deploymentObj("app:1.0.0", &one) - desired := desiredOf(deploymentObj("app:1.0.0", &three)) - ov := &KustomizeOverrides{Replicas: []ReplicaOverride{ - {Source: "kustomization.yaml", Index: 0, Name: "web", Count: 3}, - }} - - out, edits := SplitDesiredForOverrides(git, desired, ov) - if got, _, _ := unstructured.NestedInt64(out.Object, "spec", "replicas"); got != 1 { - t.Errorf("desired-for-file replicas = %d, want the source's 1", got) + three := int64(3) + files := []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("app:1.0.0", "1")), + file("kustomization.yaml", kustomizationWith("replicas:\n - name: web\n count: 3\n")), } - if len(edits) != 0 { - t.Errorf("want no edits, got %+v", edits) + + out, edits := splitFixture(t, files, "deployment.yaml", liveDeployment("app:1.0.0", &three)) + + count, _, _ := unstructured.NestedInt64(out.Object, "spec", "replicas") + require.Equal(t, int64(1), count, "the source's own value is restored") + require.Empty(t, edits) +} + +// B2: kustomize's replica fieldspec is Deployment, ReplicaSet, StatefulSet AND +// ReplicationController. isReplicaKind listed three of the four, so a scale on an RC governed +// by a replicas: entry was written into the source document, where the transformer overrode it +// straight back — non-converging drift, silently, forever. +// +// There is no list of kinds any more. kustomize's fieldspec is the authority, and the dye +// reports what it did. +func TestSplitDesired_ReplicationControllerIsGoverned(t *testing.T) { + source := `apiVersion: v1 +kind: ReplicationController +metadata: + name: web +spec: + template: + spec: + containers: + - name: app + image: app:1.0.0 +` + files := []manifestedit.FileContent{ + file("rc.yaml", source), + file("kustomization.yaml", "resources:\n - rc.yaml\nreplicas:\n - name: web\n count: 3\n"), + } + live := map[string]interface{}{ + "apiVersion": "v1", + "kind": "ReplicationController", + "metadata": map[string]interface{}{"name": "web"}, + "spec": map[string]interface{}{ + "replicas": int64(7), + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{"name": "app", "image": "app:1.0.0"}, + }, + }, + }, + }, } + + out, edits := splitFixture(t, files, "rc.yaml", live) + + _, has, _ := unstructured.NestedInt64(out.Object, "spec", "replicas") + require.False(t, has, "the source has no replicas field; the entry supplies it") + require.Len(t, edits, 1, "kustomize DOES scale a ReplicationController, so the count must route") + require.Equal(t, "count", edits[0].Edit.Field) + require.Equal(t, "7", edits[0].Edit.Value) } -// TestSplitDesired_ReplicasIgnoresOtherKinds: the replica transformer only -// touches Deployment/ReplicaSet/StatefulSet. -func TestSplitDesired_ReplicasIgnoresOtherKinds(t *testing.T) { - git := map[string]interface{}{ - "apiVersion": "v1", "kind": "ConfigMap", - "metadata": map[string]interface{}{"name": "web"}, +// A document no replicas: entry names gets no dyed count, so nothing governs its spec.replicas +// and a live scale simply writes through into the file. +// +// The second workload is not padding: kustomize REFUSES to build a folder whose replicas: entry +// matches nothing ("resource with name other does not match a config with the following GVK +// [Deployment StatefulSet ReplicaSet ReplicationController]"). Which is also kustomize stating +// its own replica fieldspec out loud — all four kinds, the fourth being the one isReplicaKind +// forgot. +func TestSplitDesired_UnmatchedNameLeavesReplicasToTheFile(t *testing.T) { + other := `apiVersion: apps/v1 +kind: Deployment +metadata: + name: other +spec: + template: + spec: + containers: + - name: app + image: app:1.0.0 +` + files := []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("app:1.0.0", "1")), + file("other.yaml", other), + file("kustomization.yaml", + "resources:\n - deployment.yaml\n - other.yaml\nreplicas:\n - name: other\n count: 3\n"), } - desired := desiredOf(map[string]interface{}{ - "apiVersion": "v1", "kind": "ConfigMap", - "metadata": map[string]interface{}{"name": "web"}, - "spec": map[string]interface{}{"replicas": int64(5)}, - }) - ov := &KustomizeOverrides{Replicas: []ReplicaOverride{ - {Source: "kustomization.yaml", Index: 0, Name: "web", Count: 3}, - }} - - out, edits := SplitDesiredForOverrides(git, desired, ov) - if got, _, _ := unstructured.NestedInt64(out.Object, "spec", "replicas"); got != 5 { - t.Errorf("non-workload kinds are untouched, got replicas %d", got) + five := int64(5) + + out, edits := splitFixture(t, files, "deployment.yaml", liveDeployment("app:1.0.0", &five)) + + count, _, _ := unstructured.NestedInt64(out.Object, "spec", "replicas") + require.Equal(t, int64(5), count, "no entry names web, so the source file owns its count") + require.Empty(t, edits) +} + +// B3, both halves, measured against kustomize: +// +// - volumes[].image.reference IS rewritten by the image transformer. The old collector never +// looked at it, so the rendered value was mirrored back into the source document as though +// the user had typed it. It must now route to the entry like any other image. +// - ephemeralContainers are NOT rewritten. No dye lands there, so no entry is credited, and +// the change belongs in the source file — which is exactly where it goes. +func TestSplitDesired_VolumeImageRoutesAndEphemeralContainerDoesNot(t *testing.T) { + source := `apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + template: + spec: + containers: + - name: app + image: app:1.0.0 + ephemeralContainers: + - name: debug + image: app:1.0.0 + volumes: + - name: vol + image: + reference: app:1.0.0 +` + files := []manifestedit.FileContent{ + file("deployment.yaml", source), + file("kustomization.yaml", kustomizationWith("images:\n - name: app\n newTag: \"1.5.0\"\n")), + } + // Live: the container and the VOLUME render at 1.5.0 (kustomize rewrote both); the + // ephemeral container still renders at the source value. The user bumps the entry's tag to + // 2.0.0 and independently edits the ephemeral container. + live := map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{"name": "web"}, + "spec": map[string]interface{}{ + "template": map[string]interface{}{ + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{"name": "app", "image": "app:2.0.0"}, + }, + "ephemeralContainers": []interface{}{ + map[string]interface{}{"name": "debug", "image": "app:9.9.9"}, + }, + "volumes": []interface{}{ + map[string]interface{}{ + "name": "vol", + "image": map[string]interface{}{"reference": "app:2.0.0"}, + }, + }, + }, + }, + }, + } + + out, edits := splitFixture(t, files, "deployment.yaml", live) + + require.Len(t, edits, 1, "the container and the volume agree on 2.0.0, so one entry edit carries both") + require.Equal(t, "newTag", edits[0].Edit.Field) + require.Equal(t, "2.0.0", edits[0].Edit.Value) + + slots := map[string]string{} + for _, s := range collectImageSlots(out.Object) { + slots[s.key] = s.image } - if len(edits) != 0 { - t.Errorf("want no edits, got %+v", edits) + require.Contains(t, slots, "/spec/template/spec/volumes\x00vol") + require.Equal(t, "app:1.0.0", slots["/spec/template/spec/volumes\x00vol"], + "the volume image is entry-governed, so the source keeps its bytes") + require.Equal(t, "app:1.0.0", slots["/spec/template/spec/containers\x00app"], + "the container image is entry-governed, so the source keeps its bytes") + require.Equal(t, "app:9.9.9", slots["/spec/template/spec/ephemeralContainers\x00debug"], + "kustomize does not rewrite ephemeralContainers, so the SOURCE FILE owns this one") +} + +// A document no entry governs at all attributes nothing, and the whole object writes through. +func TestSplitDesired_NoOverridesWritesThrough(t *testing.T) { + files := []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("app:1.0.0", "")), + file("kustomization.yaml", "resources:\n - deployment.yaml\n"), } + + out, edits := splitFixture(t, files, "deployment.yaml", liveDeployment("app:2.0.0", nil)) + + require.Empty(t, edits) + require.Equal(t, "app:2.0.0", imageOf(t, out), "nothing governs it, so the live value lands in the file") } diff --git a/internal/manifestanalyzer/render_verify.go b/internal/manifestanalyzer/render_verify.go index d8aa2c6e..7541ec45 100644 --- a/internal/manifestanalyzer/render_verify.go +++ b/internal/manifestanalyzer/render_verify.go @@ -122,7 +122,10 @@ func VerifyBatchRenders(before, after []manifestedit.FileContent, intents []Writ } now, err := renderRoot(after, root) if err != nil { - reasons = append(reasons, fmt.Sprintf("render root %s no longer builds with the write applied: %v", root, err)) + reasons = append( + reasons, + fmt.Sprintf("render root %s no longer builds with the write applied: %v", root, err), + ) continue } reasons = append(reasons, compareRoot(root, byKey, seen, renderedByKey(was), renderedByKey(now))...) diff --git a/internal/manifestanalyzer/store.go b/internal/manifestanalyzer/store.go index 495785b2..12acdf46 100644 --- a/internal/manifestanalyzer/store.go +++ b/internal/manifestanalyzer/store.go @@ -184,6 +184,19 @@ type DocumentModel struct { // docs/design/support-boundary/finished/images-and-replicas-edit-through.md. Overrides *KustomizeOverrides + // Rendered is what kustomize ACTUALLY renders this document to, plus which override + // entry supplied each override-produced value — the values read off the real render, the + // suppliers read off a dyed counterfactual one. It is what the write-side projection + // inverts against, and it replaced ~400 lines that re-implemented kustomize's + // transformers in order to guess the same thing. + // + // Nil when no render root supplies a chain, when distinct roots disagree, or when the + // dyed build could not be trusted (see attributeRoot). Nil means NO ATTRIBUTION: the + // writer routes nothing to an entry, and the verification re-render adjudicates whatever + // the source document alone can carry. See + // docs/design/support-boundary/render-attribution.md. + Rendered *RenderedOverrides + // ResourceIdentity is the API-side identity (GVR + namespace + name). It is set // only when the injected GVK->GVR mapper resolves the document's GVK to a single // served, allowed resource; structure-only analysis (and any unresolved lookup) @@ -496,7 +509,7 @@ func (s *ManifestStore) materialize( if diag != nil { s.Diagnostics = append(s.Diagnostics, *diag) } - overrides, ovDiag := resolveOverrides(r.Location, r.Identity, ovAssignments) + overrides, rendered, ovDiag := resolveOverrides(r.Location, r.Identity, ovAssignments) if ovDiag != nil { s.Diagnostics = append(s.Diagnostics, *ovDiag) } @@ -510,6 +523,7 @@ func (s *ManifestStore) materialize( ManifestIdentity: identity, NamespaceSource: nsSource, Overrides: overrides, + Rendered: rendered, Editable: r.Editable && !r.Encrypted, Cause: causeFor(r), } From 82d03e78f90a2b1bcbcf65e6138bf14e23c28185 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 17:49:41 +0000 Subject: [PATCH 06/13] docs(kustomize): record the deleted bugs, and close the attribution ledger UPGRADING.md gets the entry B1, B2 and B3 are owed. All three shipped, and all three had the same shape: we believed a folder rendered one thing while kustomize rendered another, and the projection wrote the difference into the user's source manifest as though they had typed it. They arrive as deletions rather than fixes, which is the point. It also records the behaviour change that comes with them: a write routed through a kustomization is now re-rendered before it is committed, and one that does not reproduce the live object refuses the flush instead of landing. render-attribution.md marks section 7 done through stage 3, and records the two things building it corrected: - B4 is worse than the ledger said. A rendered object is not merely awkward to read a number off, it is not a valid unstructured at all: DeepCopyJSON PANICS on the Go int kustomize hands back. - the rename-chain guard has to use kustomize's own compiled regex, not the string equality section 3 proposed -- an entry name is a regex, so `mirror/ap.` matches `mirror/app` without equalling it, and an equality guard would dye the name, kill the next entry, and mis-attribute the tag. The ledger itself is kept as written rather than deleted, because how each bug was found is the whole argument for the method: not one came from reading kustomize's source. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/UPGRADING.md | 34 +++++++++++++++++++ .../support-boundary/render-attribution.md | 34 ++++++++++++++++++- 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index a6af7b73..15c0b836 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,40 @@ 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 — kustomize decides what it renders, and what it touched (next minor; bug fixes + behavior change) + +The write path no longer contains a re-implementation of kustomize's image and replica +transformers. It asks kustomize what a folder renders to, and — by rendering a second time +with a unique nonce written into every override entry — which entry supplied each value. +`renderImage`, `imageSuppliers`, `simulateImageRender` and `isReplicaKind` are deleted. + +**Three shipped bugs go with them.** Each was a case where we believed a folder rendered +one thing while kustomize rendered another, and the projection then wrote the difference +into your source manifest as though you had typed it there. + +| | If your repo has… | What was happening | +|---|---|---| +| **B1** | an `images:` entry whose `name:` is not a literal — `- name: "ap."`, `- name: ".*"`, `- name: app:v1` | A kustomization `name:` is a **regular expression**, and kustomize matches on it as one. Our matcher was string equality, so we thought the entry matched nothing while kustomize rewrote the image. We read the difference as a user edit and wrote the *rendered* value into the source manifest — which then no longer matched the entry, silently killing the override. | +| **B2** | a `replicas:` entry naming a **ReplicationController** | kustomize's replica fieldspec is `[Deployment, StatefulSet, ReplicaSet, ReplicationController]` — it says so in its own error message. Ours listed three of the four. A scale on an RC was written into the source document, where the transformer overrode it right back: non-converging drift, on every reconcile, forever. | +| **B3** | an OCI **`volumes[].image.reference`**, or an **`ephemeralContainers`** entry | kustomize rewrites volume image references (measured) and does **not** rewrite ephemeral containers (measured). We had it backwards on both: we never looked at volume images, so the rendered value was mirrored back into your source file; and we treated ephemeral containers as override-governed when the source file owns them. | + +**Nothing here needs migration** — these are fixes, and they make the operator stop +rewriting files it should have left alone. Check `git log` on your manifests if you want to +see whether a past reconcile touched an image or a replica count you did not change. + +**One behavior change comes with them.** A write routed through a kustomization is now +re-rendered with kustomize before it is committed, and must reproduce the live object +exactly while leaving every other rendered object untouched. A write that fails that check +**refuses the flush** — `GitPathAccepted=False` / `WriteBoundaryRefused`, naming the file +and the object — rather than being committed. + +This is deliberate, and it is the safe direction: a write that does not survive the +re-render is one the override entry overrides straight back on the next render, so +committing it would leave the resource permanently un-mirrored while looking like it +worked. If you see this refusal, the live state cannot be expressed in the repository as it +stands — most often because something we do not model (a `patches:` block, a +`replacements:` entry) owns the field. The refusal names 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 diff --git a/docs/design/support-boundary/render-attribution.md b/docs/design/support-boundary/render-attribution.md index e1098679..a2ab659b 100644 --- a/docs/design/support-boundary/render-attribution.md +++ b/docs/design/support-boundary/render-attribution.md @@ -1,12 +1,37 @@ # Render attribution: which override entry supplied this value? -> **design** — direction-setting; ships no code. Captured: 2026-07-14 +> **design** — direction-setting. 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 · +> [patching-kustomize.md](patching-kustomize.md) — **revises §4**: the fork is ~30 lines, and it is built · > [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) +> **Status: §7 is done — stages 1 through 3 are shipped.** `renderRootWith` is the +> counterfactual primitive; verification is a real re-render (`VerifyBatchRenders`), and +> `simulateImageRender` is gone; attribution is the dye (`dye.go`, +> `overrides_attribution.go`), and `renderImage`, `imageSuppliers` and `isReplicaKind` are +> gone with **B1, B2 and B3**. The 12 `TestSplitDesired_*` tests are rebuilt against real +> kustomize fixtures, and the corpus differential is now the stronger claim that **an in-sync +> folder projects to a complete no-op**. Stage 4 (chain → set) is not done. +> +> Two things the building corrected, both recorded below where they belong: +> +> - **B4 is worse than stated.** A rendered object is not merely awkward to read a number off +> — it is **not a valid `unstructured` at all**. `DeepCopyJSON` **panics** on one (*"cannot +> deep copy int"*), so a rendered object must be normalised through JSON before any +> apimachinery helper touches it. +> - **The rename-chain guard must use kustomize's regex, not string equality** (§3 proposed +> equality). An entry's `name:` is a regex, so `mirror/ap.` matches `mirror/app` without +> equalling it, and an equality guard would dye the name, kill the next entry, and +> mis-attribute the tag. +> +> §5's verdict — *attribution may be heuristic, verification may not* — survived contact +> intact, and it is what made the rest safe to delete. But see +> [patching-kustomize.md](patching-kustomize.md): the dye is now the **fallback** for an +> unpatched build, not the destination. + 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** — @@ -334,6 +359,13 @@ is checking for. ## 6. The bug ledger: what the probes found +> **All of it is closed.** P1 and C1 shipped in #232. **B1, B2 and B3 are not fixed — they are +> DELETED**, along with the `renderImage` / `isReplicaKind` / fieldspec code that caused them +> (see [UPGRADING.md](../../UPGRADING.md)). B4 was found to be worse than recorded and is +> handled by a type switch (`renderedReplicaCount`) plus a JSON normalisation in the tests. +> The ledger is kept as written, because *how* each was found is the argument for the method: +> not one came from reading kustomize's source. Every one came from asking it. + 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.** From c1b0c62d3a4271079a46088ac3e7b92c370624a4 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 18:04:19 +0000 Subject: [PATCH 07/13] docs(support-boundary): move the kustomize-fork docs out of this repo The three docs about the kustomize build-trace fork (patching-kustomize, generated-repo-map, render-plan-artifact) now live in the fork itself, under plans/ in ConfigButler/kustomize-tracer, next to the code they describe and the tests that verify them. They remain in this repository's history; this removes them going forward. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../support-boundary/generated-repo-map.md | 321 ----------------- .../support-boundary/patching-kustomize.md | 322 ------------------ .../support-boundary/render-plan-artifact.md | 237 ------------- 3 files changed, 880 deletions(-) delete mode 100644 docs/design/support-boundary/generated-repo-map.md delete mode 100644 docs/design/support-boundary/patching-kustomize.md delete mode 100644 docs/design/support-boundary/render-plan-artifact.md diff --git a/docs/design/support-boundary/generated-repo-map.md b/docs/design/support-boundary/generated-repo-map.md deleted file mode 100644 index ca548828..00000000 --- a/docs/design/support-boundary/generated-repo-map.md +++ /dev/null @@ -1,321 +0,0 @@ -# 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 deleted file mode 100644 index 45c65d3c..00000000 --- a/docs/design/support-boundary/patching-kustomize.md +++ /dev/null @@ -1,322 +0,0 @@ -# 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. - -> **Status: built and measured**, on branch `feat/build-trace-observer` in -> `external-sources/kustomize` (commit `290d04199`, on top of upstream `79bb1aa2b`). -> **638 lines added, 9 changed, across 11 files.** The full upstream `api` test suite passes: -> the only six failures are identical on the unpatched base commit (they need a container -> runtime, the network, or version stamping). See §9 for what the measurement changed. - -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) — an *additive* observer, not a change to the - annotation's semantics (§9). 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. - -## 9. What building it changed - -Three things the argument above got wrong, or only half-right. - -**The patch should be additive, not a semantics change.** §2 proposed making the transformer -annotation *mean* "changed". That was the wrong instinct: it rewrites goldens, it changes -`kubectl kustomize` output, and it turns a cheap patch into a standards fight (§7) — all to -deliver strictly *less* than the alternative. What shipped instead is a new -`krusty.Options.Observer`, called once per (transformer instance, resource) pair that the -transformer actually altered. It changes **no** existing behaviour, so the entire upstream -test suite passes untouched, and it carries the field paths and entry index that the -annotation could never carry anyway. **The annotation is left exactly as it is** — we simply -stop needing it. That also collapses §7's hardest objection: an additive option with no -golden churn is a far easier upstream conversation than a semantics change to an alpha -annotation vendored into `kubectl`. - -**The zero-cost gate is real, and it fell out for free.** An observer needs origins (they -carry the `ConfiguredIn` path), so setting one turns origin tracking on internally — and -`Kustomizer.Run` *already* strips the origin and transformer annotations unless -`buildMetadata` asked for them. So an observed build renders byte-identically to an -unobserved one with no extra work, and an unobserved build does no snapshotting and no -diffing at all (one nil check per transformer). §5 argued this was *safe*; it is in fact -*structural*, and it is now asserted by a test that fails loudly if it ever stops being -true (`TestBuildTraceDoesNotChangeTheBuild`). - -**Two facts only the running code produced:** - -- **Renames need a normalised key.** Matching a resource across a transformer by - `ResId.String()` does not work. `StorePreviousId` records a resource's *effective* - namespace (`default`), while `CurId` on an unnamespaced resource has *none* (`[noNs]`) — - so the id a resource had before a rename and the id it has after disagree on how to spell - "no namespace", and a `namePrefix` reads as a deletion plus a creation rather than a - rename. Normalising both sides through `EffectiveNamespace` fixes it. Nothing in the - source says this; the first test run did. -- **`ConfiguredIn` is relative to the build root, not the repo root.** An overlay's own - kustomization comes back as plain `kustomization.yaml`. Any consumer building several - roots must re-root these paths itself. - -And the headline claim of §3 now has a test rather than an argument behind it. For a base -pinned at `web:v1` with an overlay carrying `images: [web→v2, redis→6.2]` and -`replicas: [web→3]`, the observed trace is exactly two events: - -``` -kustomization.yaml ReplicaCountTransformer[0] Deployment/web spec.replicas: 1 -> 3 -kustomization.yaml ImageTagTransformer[0] Deployment/web spec.template.spec.containers[0].image: web:v1 -> web:v2 -``` - -The `Service` is not mentioned, though the annotation names both transformers on it. The -sidecar container is not mentioned. And `images[1]` — the **idempotent pin**, `redis:6.2` -over a base already at `redis:6.2` — produces **no event at all**, because it changed -nothing. That is the honest answer, it is the one leave-one-out probing structurally cannot -give ([render-attribution.md](render-attribution.md) §2), and it is the case that motivated -the dye. - -## 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-plan-artifact.md b/docs/design/support-boundary/render-plan-artifact.md deleted file mode 100644 index 5641a6fd..00000000 --- a/docs/design/support-boundary/render-plan-artifact.md +++ /dev/null @@ -1,237 +0,0 @@ -# The render plan: attribution as an artifact, not a capability - -> **design** — direction-setting; ships no code. Nothing it describes is supported today. -> Captured: 2026-07-14 -> Related: -> [patching-kustomize.md](patching-kustomize.md), -> [render-attribution.md](render-attribution.md), -> [render-root-scoping.md](render-root-scoping.md), -> [generated-repo-map.md](generated-repo-map.md), -> [support-contract.md](support-contract.md), -> [acceptance-precision.md](acceptance-precision.md) - -The fork ([patching-kustomize.md](patching-kustomize.md)) can tell us exactly which -kustomization entry supplied which field. But carrying a forked kustomize in the operator — -on the reconciliation hot path, in the same process that must render byte-identically to -the user's controller — is a cost we would rather not pay, and a `replace` directive does -not even survive being consumed as a library (§6 there). - -So don't. **Run the fork offline, once, and emit a file.** - -The operator links stock kustomize and reads the file. Attribution stops being something -the operator can *do* and becomes something it can *look up*. This doc is about what that -file has to contain to be safe, and about the one way of monetising it that does not -destroy the product. - -## 1. The shape - -```mermaid -flowchart LR - subgraph offline["analyse run — the fork lives HERE, and only here"] - A["patched kustomize
(buildtrace.Observer)"] --> P["render plan
(the artifact)"] - end - subgraph hot["operator — stock kustomize, no fork"] - P --> PROP["propose an edit"] - PROP --> OR["the oracle:
re-render, require the live
object reproduced exactly"] - OR -->|reproduces| W["write"] - OR -->|does not| R["refuse"] - end - - classDef editable fill:#dfd,stroke:#3a3,color:#111 - classDef refused fill:#fdd,stroke:#c33,color:#111 - class A,P,W editable - class R refused -``` - -Three properties fall out of this and they are the whole argument: - -- **The fork leaves the hot path.** The operator's render fidelity against Flux is - unimpeachable, because the operator renders with the same stock library Flux does. -- **The blast radius of the fork shrinks to one offline tool.** It is no longer linked into - the thing that writes to people's repositories. -- **The plan is a *hint*, never an authority.** See §2, which is the load-bearing section. - -## 2. The plan proposes; the oracle disposes - -The instinct that a precomputed plan is dangerous is correct, and the reason is staleness: -a plan describes how a repo rendered at commit *X*, and **our own writes produce commit -X+1**. Every write we make invalidates the plan that authorised it. - -That would be fatal — *if the plan were trusted*. It must not be. - -The oracle from [render-root-scoping.md](render-root-scoping.md) §3 already exists in the -design and, crucially, **needs no plan and no fork**: propose a source edit, re-render the -render root with stock kustomize, and require that the proposal reproduces the live object -exactly *and leaves every other object byte-identical*. That check is what the operator -already has to do anyway. - -So the plan only ever generates a *proposal*, and a wrong proposal is *caught*: - -| Plan state | Proposal | Oracle | Outcome | -|---|---|---|---| -| correct | right file, right entry | reproduces | **write** | -| stale / wrong | wrong file or wrong entry | does not reproduce | **refuse** | -| missing | fall back to the dye | either | write or refuse | - -**A bad plan degrades to a refusal, never to a corruption.** That converts staleness from a -correctness problem into an availability problem, and it is the single property that makes -this whole scheme safe to ship. It also means the plan does not have to be *perfect* — it -has to be *verifiable*, which is a far weaker and far more achievable requirement. - -This is the same reason [render-attribution.md](render-attribution.md) §5 insists that -attribution may be heuristic but verification may not. The plan is the heuristic. The -oracle is the verification. **Never let the plan be both.** - -## 3. Even so: fingerprint the inputs - -The oracle makes a stale plan safe. It does not make it *free* — every stale-plan proposal -costs a full re-render and ends in a refusal the user did not deserve. So the plan must be -able to say "I no longer describe this repo" without being run through the oracle to find -out. - -That means the plan is keyed by **a content hash of every input it depends on**: every file -in the render root's read scope, including the bases outside the root that the overlay -reaches into. Not the git commit — the *content* — because the operator writes to a branch -and the plan must remain valid for the files it did not touch. - -This is `go.sum`, and it fails the same way if you skip it: **a lockfile nobody verifies is -not a lockfile, it is a rumour.** A plan whose fingerprint does not match the repo it is -being applied to must be treated as absent (fall back to the dye), not as approximately -right. - -## 4. The tier must gate scope, not accuracy - -The commercial instinct — free tier gets the dye, paid tier gets the plan — is right in -outline and lethal in one specific formulation. - -**The trap.** The dye is *sound only for pure sinks* -([render-attribution.md](render-attribution.md) §3): it cannot attribute `newName`, it is -blind to an idempotent pin, and it says nothing about `patches:`. If the free tier *guesses* -in those cases while the paid tier *knows*, then correctness is the paid feature. The first -free-tier user whose base gets silently corrupted takes the paid tier's reputation with -them. You cannot sell "we write to the right file" as an upgrade from "we write to a file." - -**The fix, and it is one word.** Both tiers are **correct**. The paid tier is more -**capable**: - -| | Free (dye) | Paid (plan) | -|---|---|---| -| `images` / `replicas` pure sinks | **edits** | **edits** | -| idempotent pin, `newName` | **refuses** | **edits** | -| `patches:`, components | **refuses** | **edits** | -| ever writes the wrong file | **no** | **no** | - -What the customer buys is **a bigger support boundary, not a more accurate one.** The free -tier never lies; it says *"I cannot edit this."* The paid tier says *"I can."* That is a -defensible upsell precisely because it is the product's existing ethos — refusal is already -our honest answer, and [support-contract.md](support-contract.md) is built on it. The plan -turns refusals into edits. It must never turn refusals into guesses. - -A corollary worth stating, because it is a real constraint on the open-source operator: **the -operator must be fully correct with no plan present.** The plan is an enrichment. If the -operator's correctness depends on a file only paying customers get, the open-source project -is a trap, and it will be treated as one. - -## 5. What is in the plan - -Not the render. The **inverse** of the render — which is the thing that does not otherwise -exist ([generated-repo-map.md](generated-repo-map.md) §2, tier 4). - -```yaml -# renderplan.v1 -renderRoot: overlays/prod -inputs: # §3 — every file in the READ scope, not just the subtree - base/deployment.yaml: sha256:... - base/kustomization.yaml: sha256:... - overlays/prod/kustomization.yaml: sha256:... -objects: - - id: apps/v1/Deployment/prod/web - origin: base/deployment.yaml # exact, from kustomize - fields: - spec.replicas: - source: {kind: entry, file: overlays/prod/kustomization.yaml, stanza: replicas, index: 0} - spec.template.spec.containers[0].image: - source: {kind: entry, file: overlays/prod/kustomization.yaml, stanza: images, index: 0} - spec.template.spec.containers[1].image: - source: {kind: file, file: base/deployment.yaml} # no entry changed it -unattributable: # §6 — the plan MUST record its own gaps - - object: apps/v1/Deployment/prod/web - field: spec.template.metadata.labels.version - reason: changed-by-transformer-we-cannot-invert - transformer: PatchStrategicMergeTransformer -``` - -Two things about this schema are not negotiable. - -**It records what it could NOT attribute.** A plan that lists only its successes is -indistinguishable from a plan that is incomplete, and the operator cannot tell "this field -has no override" from "this field's override was not understood". The first is editable in -place; the second must be refused. `unattributable` is what keeps those apart. - -**Entry references are (file, stanza, index).** Not a transformer kind — kustomize builds -one transformer instance per entry, so the index is the only thing that identifies *which* -`images:` entry, and it is exactly what the observer emits. - -## 6. One artifact, three consumers - -This is why it is worth building once rather than three times. The plan is the serialized -tier-4 graph, and everything downstream is a *rendering* of it: - -| Consumer | Uses | -|---|---| -| **The writer** | `fields[].source` — route each changed field to a file or an entry (and refuse on `unattributable`). | -| **The diagram** ([generated-repo-map.md](generated-repo-map.md)) | the whole thing — this *is* the graph, and "renders graphically" is a viewer over this file, not a second pipeline. | -| **The metrics** | entries that appear in no `fields[].source` are **dead configuration**; `unattributable[]` grouped by transformer measures **the support boundary itself**, across every repo we see. | - -That last one is the sleeper. Every other question in these docs — *should we support -`patches:`? how common are components, really?* — is currently argued from fixtures and -intuition. `unattributable[]` answers it with a count from real repositories. - -## 7. Where does it live? - -Genuinely open, and the two options trade differently. - -**Committed to the customer's repo.** It is reviewable, it diffs in a pull request (*"this -change alters how your repo renders"*), and it is versioned alongside the content it -describes, which makes §3's fingerprint check natural. But our own writes churn it, and -there is a self-collision worth noticing: **a stray file in a GitTarget folder is currently -grounds for refusing the whole folder** ([acceptance-precision.md](acceptance-precision.md) -§1). We would be adding a file that trips our own acceptance gate. It must go on the inert -allowlist (`DefaultAllowlist`) in the same change, or we ship a product that refuses repos -because of a file it wrote itself. - -**Out of band** (object storage, a CR, the GitTarget status). No repo churn, no acceptance -collision — but invisible to the user, and it loses the "explain my repo in a PR" value that -is half the point. - -Leaning committed-in-repo, precisely *because* it is visible: the artifact that explains the -repo is worth more in the repo than in our database. - -## 8. Order of work - -1. **The oracle first, and unconditionally.** It is what makes every later step safe, it is - needed by the dye path anyway, and it needs no fork and no plan. Nothing else here should - be built before it. -2. **The plan schema + the fingerprint** (§3, §5), with `unattributable` from day one. -3. **The offline emitter** — the analyse run, linking the fork, producing the file. -4. **The operator reads it**, falling back to the dye when it is absent or its fingerprint - does not match. -5. **The diagram and the metrics**, which are then free (§6). - -## Still open - -- **What regenerates the plan after we write?** Our write invalidates the fingerprint of the - file we wrote. The cheapest honest answer is that the operator refuses the *next* edit to - that root until the analyse run reruns — correct, but a poor experience. The better answer - is probably that the operator can update the plan's fingerprint itself for a write whose - effect it fully understood (it just verified it with the oracle, after all), and only a - *foreign* change to the repo forces a full reanalysis. That is a nice property and it needs - proving, not asserting. -- **Does the plan cross the licence boundary cleanly?** The fork is Apache-2.0 (private use - is unrestricted; distributing a binary built from it requires the notices and a statement - of changes). The *plan* is our output and carries none of that. Running the fork as a - hosted analyse run and shipping only the file is the cleanest position, and it is worth - confirming with someone who is not me. -- **Is the plan per render root, or per repo?** Per root is simpler and matches the - fingerprint's read-scope. Per repo is what a diagram wants. Probably per root, with the - repo view being a join. From bbebddbcbd3a044b7a0438762384bcddeaa0280c Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 18:04:33 +0000 Subject: [PATCH 08/13] docs(support-boundary): repoint the links to the relocated fork docs render-attribution.md linked patching-kustomize.md, which now lives in the fork (ConfigButler/kustomize-tracer, plans/). Left as a plain reference so the link checker does not chase it into a repository it cannot see. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/design/support-boundary/render-attribution.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/design/support-boundary/render-attribution.md b/docs/design/support-boundary/render-attribution.md index a2ab659b..89c2a559 100644 --- a/docs/design/support-boundary/render-attribution.md +++ b/docs/design/support-boundary/render-attribution.md @@ -4,7 +4,7 @@ > 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 · -> [patching-kustomize.md](patching-kustomize.md) — **revises §4**: the fork is ~30 lines, and it is built · +> *patching-kustomize.md* (ConfigButler/kustomize-tracer, `plans/`) — **revises §4**: the fork is ~30 lines, and it is built · > [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) @@ -29,7 +29,7 @@ > > §5's verdict — *attribution may be heuristic, verification may not* — survived contact > intact, and it is what made the rest safe to delete. But see -> [patching-kustomize.md](patching-kustomize.md): the dye is now the **fallback** for an +> *patching-kustomize.md* (ConfigButler/kustomize-tracer, `plans/`): the dye is now the **fallback** for an > unpatched build, not the destination. The workstream that replaced our hand-written kustomize with the real one has one step From 8a71c37975d4cc8971ee89924f44207bc113e736 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 18:14:14 +0000 Subject: [PATCH 09/13] fix(kustomize): close the two holes an adversarial review found in the oracle Both were in the new code, and both are proved by a test that fails without the fix. 1. THE WRITE AND THE INTENT ARE DIFFERENT OBJECTS. createNew strips metadata.namespace out of the bytes it writes when the destination inherits its namespace from a kustomization's namespace: transformer -- correct, because the transformer puts it back, and every sibling in that directory follows the convention. But the three writers then built the WriteIntent from that same stripped object. The render HAS the namespace, so the intent demanded a render that did not, and the oracle refused a flush it had just planned perfectly: the new resource was lost, and so was any governed write batched with it. Resync replays the same batch, so it never recovered. patchExisting had it right all along -- it strips a LOCAL copy and hands the untouched event to the intent -- which is exactly the asymmetry that gave the bug away. Intent recording moves out of the three writers, which only ever see the mutated object, and up to the callers, which still hold both. This fires on the most ordinary kustomize layout there is: a per-env folder with a namespace: and an images: entry, plus any new resource. 2. A NEW RESOURCE COULD NOT REACH THE ORACLE AT ALL. Its intent was never Governed (Governed came from dm.Overrides -- a PRE-batch store document, which a new document by definition does not have), so a flush containing only new resources never turned the oracle on. And createNew does not route a new document's values onto an entry: it has no override chain yet. So a new Deployment whose image an existing images: entry matches was committed at its live tag, the entry rewrote it on the next render, and the mirror asserted a state the folder does not produce -- forever. Silently. That is precisely the failure this whole path exists to prevent, in the one write path the oracle could not see. It still does not route (that needs attribution for a document that does not exist yet), but it is now put in front of the oracle, which turns a silent non-converging commit into a reported refusal naming the file and the object. "We cannot express this here" is an answer. Quietly writing a lie is not. Turning the oracle on is now a separate question from WriteIntent.Governed, because the two are not the same claim: Governed additionally ASSERTS the document is rendered, and a new document is not entitled to that -- its resources: entry can legitimately fail to be added (a kustomization with no resources: sequence), leaving the file written but outside every render. Conflating them refused that case too, which the placement suite caught. Found by an adversarial review of the diff. It also cleared the dye itself: nonce collision, by-position alignment, and the tag/digest routing all held. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/git/kustomize_oracle_test.go | 141 +++++++++++++++++++++++++- internal/git/plan_flush.go | 116 ++++++++++++++++----- 2 files changed, 229 insertions(+), 28 deletions(-) diff --git a/internal/git/kustomize_oracle_test.go b/internal/git/kustomize_oracle_test.go index 3267c5fd..00e15183 100644 --- a/internal/git/kustomize_oracle_test.go +++ b/internal/git/kustomize_oracle_test.go @@ -4,8 +4,10 @@ package git import ( "context" + "io/fs" "os" "path/filepath" + "strings" "testing" gogit "github.com/go-git/go-git/v5" @@ -91,7 +93,7 @@ func seedSharedEntryWorktree(t *testing.T, root string) (string, string, string) // sharedImageEvent is the live Deployment as the cluster holds it: the rendered form, // which is what the source file plus the overlay's entry must reproduce. -func sharedImageEvent(name, image string) Event { +func sharedImageEvent(name, image string) Event { //nolint:unparam // image varies by intent, not by call today return Event{ Object: &unstructured.Unstructured{Object: map[string]interface{}{ "apiVersion": "apps/v1", @@ -144,6 +146,143 @@ func TestPlanFlush_RefusesAWriteThatDragsASiblingAlong(t *testing.T) { assertFileBytes(t, apiPath, sharedImageDeploymentYAML("api"), "and certainly leaves the sibling alone") } +// A NEW resource landing in a kustomize directory that supplies its namespace must not be +// read as collateral damage by the oracle. +// +// The bytes written for it deliberately OMIT metadata.namespace — the kustomization's +// namespace: transformer puts it back, and every sibling in that directory follows the same +// convention. But the render therefore HAS a namespace, so an intent built from the +// namespace-stripped object demands a render that does not, and the whole flush is refused: +// the new resource is lost, and so is the perfectly good governed write batched with it. +// Recovery never comes, because resync replays the same batch. +// +// The write and the intent are different objects. This is the test that says so. +func TestPlanFlush_NewResourceInANamespaceInheritingDirIsNotCollateralDamage(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + + // FLAT on purpose: the kustomization sits beside the manifests it lists, so a new + // document lands in the same directory AND gets a resources: entry — which is what puts + // it inside the render, where the oracle can see it. (With the manifests in a subfolder + // the new file is never added to resources:, so it is not rendered at all and this case + // cannot arise.) + kustPath := filepath.Join(root, "kustomization.yaml") + webPath := filepath.Join(root, "web.yaml") + require.NoError(t, os.WriteFile(webPath, []byte(sharedImageDeploymentYAML("web")), 0o600)) + require.NoError(t, os.WriteFile(kustPath, []byte(`apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +namespace: default +resources: + - web.yaml +images: + - name: ghcr.io/example/shared + newTag: "1.0.0" +`), 0o600)) + + changed, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), + // A governed write, which is what turns the oracle on... + sharedImageEvent("web", "ghcr.io/example/shared:2.0.0"), + // ...and a brand-new resource in the same namespace-inheriting directory. + newCacheDeploymentEvent("redis:7"), + ) + + require.NoError(t, err, "the flush must not be refused: both writes are exactly what we intended") + require.True(t, changed) + + kust, readErr := os.ReadFile(kustPath) + require.NoError(t, readErr) + assert.Contains(t, string(kust), `newTag: "2.0.0"`, "the governed write must still land") + assert.Contains(t, string(kust), "cache.yaml", "and the new document must be added to the render") + + created := findFileContaining(t, root, "name: cache") + require.NotEmpty(t, created, "the new resource must have been written") + assert.NotContains(t, created, "namespace: default", + "the directory supplies the namespace; the file must not repeat it") +} + +// findFileContaining returns the content of the first YAML file under root holding needle. +// The new document's placement is the placement rules' business, not this test's. +func findFileContaining(t *testing.T, root, needle string) string { + t.Helper() + var found string + require.NoError(t, filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() || found != "" || !strings.HasSuffix(path, ".yaml") { + return nil //nolint:nilerr // a walk error simply means nothing was found here + } + b, readErr := os.ReadFile(path) + if readErr == nil && strings.Contains(string(b), needle) { + found = string(b) + } + return nil + })) + return found +} + +// A NEW resource whose image an existing images: entry overrides cannot be mirrored by +// writing the live value into a new file: the entry rewrites it on the very next render, so +// the committed file asserts a state the folder does not produce, and it never converges. +// +// We do not route a new document's values onto an entry — it has no override chain yet, it +// did not exist when the store was built — so the honest answer is that this live state cannot +// be expressed in this repository as it stands. The oracle says so, names the file and the +// object, and writes nothing. Before, this was committed silently and wrongly. +func TestPlanFlush_RefusesANewResourceAnEntryWouldOverride(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + + require.NoError(t, os.WriteFile(filepath.Join(root, "web.yaml"), + []byte(sharedImageDeploymentYAML("web")), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(root, "kustomization.yaml"), []byte( + `apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +namespace: default +resources: + - web.yaml +images: + - name: ghcr.io/example/shared + newTag: "1.0.0" +`), 0o600)) + + // The new resource runs the image the entry matches, at a tag the entry does not produce. + _, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), + newCacheDeploymentEvent("ghcr.io/example/shared:2.0.0")) + + var refused *manifestanalyzer.AcceptanceRefusedError + require.ErrorAs(t, err, &refused, "the entry would override the new file; that must not be committed silently") + assert.Contains(t, refused.Error(), "Deployment/cache", "the refusal must name the object it cannot express") + + _, statErr := os.Stat(filepath.Join(root, "cache.yaml")) + assert.True(t, os.IsNotExist(statErr), "a refused flush writes nothing") +} + +// newCacheDeploymentEvent is a live Deployment with no document in Git yet. +func newCacheDeploymentEvent(image string) Event { + return Event{ + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{"name": "cache", "namespace": "default"}, + "spec": map[string]interface{}{ + "selector": map[string]interface{}{"matchLabels": map[string]interface{}{"app": "cache"}}, + "template": map[string]interface{}{ + "metadata": map[string]interface{}{"labels": map[string]interface{}{"app": "cache"}}, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{"name": "cache", "image": image}, + }, + }, + }, + }, + }}, + Identifier: types.ResourceIdentifier{ + Group: "apps", Version: "v1", Resource: "deployments", Namespace: "default", Name: "cache", + }, + Operation: "CREATE", + } +} + // The same tag bump, but both deployments move together: now the entry edit reproduces // BOTH live objects, nothing else drifts, and the write lands. This is the control — it is // what proves the refusal above is discriminating rather than a blanket "no". diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index 7661230d..7dc7baca 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -15,6 +15,7 @@ import ( "strings" gogit "github.com/go-git/go-git/v5" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/log" sigsyaml "sigs.k8s.io/yaml" @@ -79,6 +80,13 @@ type writeBatch struct { // render precondition can tell a change the flush MEANT from one it merely caused. // Anything not named here has to come out of the re-render untouched. intents []manifestanalyzer.WriteIntent + // putToKustomize records that this flush touched a kustomize render root — it edited a + // governed document, or placed a new one into a kustomization's resources:. It is what + // turns the oracle on, and it is deliberately NOT the same question as WriteIntent.Governed: + // that one additionally ASSERTS the document is rendered, which a new document is not + // entitled to claim (its resources: entry can legitimately fail to be added — see + // appendKustomizationResource — leaving the file written but outside every render). + putToKustomize bool // policy is the GitTarget's declared new-file placement policy, consulted // only for a resource with no existing document. nil means no declared policy — // placement falls through to sibling inference and then the canonical path. @@ -236,16 +244,39 @@ func (wb *writeBatch) applyEvent(ctx context.Context, event Event) error { // existing document is placed by createNew. It returns what it did to the bytes // (created / updated / no change). func (wb *writeBatch) applyUpsert(ctx context.Context, event Event) (upsertOutcome, error) { - if id, ok := manifestIdentity(event.Object); ok { - if dm := wb.store.ByManifestIdentity[id]; dm != nil { - filePath := wb.docLoc[dm].FilePath - if wb.writer.isSensitiveIdentifier(event.Identifier) { - return wb.writeWholeFile(ctx, event, filePath) - } - return wb.patchExisting(ctx, event, filePath, id, dm) - } + id, ok := manifestIdentity(event.Object) + if !ok { + return wb.createNew(ctx, event) + } + dm := wb.store.ByManifestIdentity[id] + if dm == nil { + return wb.createNew(ctx, event) + } + filePath := wb.docLoc[dm].FilePath + if !wb.writer.isSensitiveIdentifier(event.Identifier) { + return wb.patchExisting(ctx, event, filePath, id, dm) } - return wb.createNew(ctx, event) + return wb.rewriteSensitive(ctx, event, filePath) +} + +// rewriteSensitive re-encrypts a sensitive document wholesale at its existing path. +// +// Its intent is UNCHECKED: the file is SOPS ciphertext, so kustomize renders the encrypted +// blob and no plaintext live object can ever equal it. The oracle is told to expect this +// object to move without being able to say what to — while still holding the write to +// disturbing nothing else, which is the half that protects other environments. +func (wb *writeBatch) rewriteSensitive(ctx context.Context, event Event, filePath string) (upsertOutcome, error) { + outcome, err := wb.writeWholeFile(ctx, event, filePath) + if err == nil && wroteBytes(outcome) { + wb.intend(markUnchecked(intentFor(event.Object, filePath, false), true)) + } + return outcome, err +} + +// wroteBytes reports whether an upsert actually changed the worktree, which is the only +// case that owes the oracle an intent. +func wroteBytes(o upsertOutcome) bool { + return o == upsertCreated || o == upsertUpdated } // createNew resolves the placement of a resource with no existing document — @@ -274,6 +305,11 @@ func (wb *writeBatch) createNew(ctx context.Context, event Event) (upsertOutcome return upsertSkippedUnsafe, nil } + // The LIVE object, kept before the namespace strip below rewrites it. The bytes we write + // and the object the render must produce are not the same thing, and only this scope + // still holds both — see intentFor. + live := event.Object + if placement.Kustomization != nil { wb.appendKustomizationResource(ctx, event, placement) } @@ -288,6 +324,34 @@ func (wb *writeBatch) createNew(ctx context.Context, event Event) (upsertOutcome event.Object.SetNamespace("") } + outcome, err := wb.placeNewDocument(ctx, event, placement, sensitive) + if err != nil || !wroteBytes(outcome) { + return outcome, err + } + + // A new document that joins a kustomization's resources: list is INSIDE a render root, so + // the folder's images:/replicas: entries govern it from the moment it lands — and we do not + // route a new document's values onto an entry (it has no override chain yet; it did not + // exist when the store was built). So the live value goes into the file, and if an entry + // overrides it, the folder renders something else and the resource never converges. + // + // Declaring it governed puts it in front of the oracle, which turns that from a silent + // non-converging commit into a reported refusal naming the file and the object. It does not + // make the write work — that needs attribution for a document that does not exist yet — but + // "we cannot express this here" is an answer, and quietly writing a lie is not. + wb.putToKustomize = wb.putToKustomize || placement.Kustomization != nil + wb.intend(markUnchecked(intentFor(live, placement.Path, false), sensitive)) + return outcome, nil +} + +// placeNewDocument writes the new document at its resolved placement: appended to an existing +// accepted bundle, folded into a same-batch cold bundle, or as a file of its own. +func (wb *writeBatch) placeNewDocument( + ctx context.Context, + event Event, + placement manifestanalyzer.PlacementResult, + sensitive bool, +) (upsertOutcome, error) { if placement.Append { return wb.appendNewDocument(ctx, event, placement.Path) } @@ -361,7 +425,6 @@ func (wb *writeBatch) writeColdBundleMember( rebuilt = appendYAMLDocument(rebuilt, m.content) } wb.buffer(rel).current = rebuilt - wb.intend(markUnchecked(intentFor(event, rel, false), sensitive)) return upsertCreated, nil } @@ -390,7 +453,6 @@ func (wb *writeBatch) appendNewDocument(ctx context.Context, event Event, rel st } buf := wb.buffer(rel) buf.current = appendYAMLDocument(buf.current, content) - wb.intend(intentFor(event, rel, false)) return upsertCreated, nil } @@ -485,6 +547,7 @@ func (wb *writeBatch) applyFieldPatch(ctx context.Context, event Event) error { // intended write as collateral damage. It is UNCHECKED because a field patch carries // a few audited assignments, never a whole object to compare the render against: the // oracle can still prove the write disturbs nothing else, but not that it landed. + wb.putToKustomize = true wb.intend(fieldPatchIntent(filePath, id, governed)) if len(assignments) == 0 { return nil @@ -652,8 +715,11 @@ func (wb *writeBatch) patchExisting( // what the second one renders to, so by the time the second is processed there is nothing // left to write. Its render still moves, and it moves onto its own live state — that is // the resource converging, not collateral damage, and only its declared intent says so. + if dm.Overrides != nil { + wb.putToKustomize = true + } if outcome == upsertUpdated || dm.Overrides != nil { - wb.intend(intentFor(event, filePath, dm.Overrides != nil)) + wb.intend(intentFor(event.Object, filePath, dm.Overrides != nil)) } return outcome, nil } @@ -673,14 +739,7 @@ func (wb *writeBatch) patchExisting( // absorbed." A resource we silently stop mirroring is the failure this path exists to // prevent, so it must not be the failure this path introduces. func (wb *writeBatch) renderPrecondition() error { - governed := false - for _, in := range wb.intents { - if in.Governed { - governed = true - break - } - } - if !governed { + if !wb.putToKustomize { return nil } @@ -718,8 +777,16 @@ func (wb *writeBatch) intend(in manifestanalyzer.WriteIntent) { // intentFor builds the intent for an ordinary object-bearing write: the document must // render to exactly the live object. -func intentFor(event Event, filePath string, governed bool) manifestanalyzer.WriteIntent { - desired := manifestreport.Project(event.Object) +// +// It takes the LIVE object, not the event, and that distinction is load-bearing. createNew +// strips metadata.namespace out of the bytes it writes when the destination inherits its +// namespace from a kustomization's namespace: transformer — correct, because the transformer +// puts it back. But the render therefore HAS the namespace, so an intent built from the +// stripped object would demand that the render not have one, and the oracle would refuse a +// flush it had just planned perfectly. The bytes and the intent are different objects, and +// the caller is the only one that still holds both. +func intentFor(live *unstructured.Unstructured, filePath string, governed bool) manifestanalyzer.WriteIntent { + desired := manifestreport.Project(live) return manifestanalyzer.WriteIntent{ SourcePath: filePath, Kind: desired.GetKind(), @@ -885,11 +952,6 @@ func (wb *writeBatch) writeWholeFile(ctx context.Context, event Event, rel strin } } buf.current = content - // A SENSITIVE document is written encrypted, so kustomize renders the SOPS ciphertext - // and no plaintext live object can ever equal it. Declare the write so the oracle does - // not read it as collateral damage, but mark it unchecked: we can still prove it - // disturbs nothing else, which is the half that protects other environments. - wb.intend(markUnchecked(intentFor(event, rel, false), wb.writer.isSensitiveIdentifier(event.Identifier))) if isNew { return upsertCreated, nil } From 2ff3d3e990b2059ec737cc8b7ae8905df68212fb Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 18:34:49 +0000 Subject: [PATCH 10/13] fix(kustomize): deleting a resource also removes its resources: entry Registering a new resource in a kustomization's resources: list was only half the job. Nothing ever took the entry back out, so deleting the manifest left an entry naming a file that no longer exists -- and kustomize refuses to build over that: accumulating resources ... '/scan/bundle.yaml' doesn't exist The folder became undeployable, and the GitTarget was refused on the next reconcile. RemoveKustomizationResource is AppendKustomizationResource read backwards, with the same all-or-nothing semantics: idempotent, and a kustomization it cannot edit (multi-document, no resources: sequence, unparseable) is skipped with a diagnostic rather than having structure invented for it. THE ENTRY COMES OUT ONLY WHEN THE FILE ACTUALLY GOES. A file holding several documents survives the deletion of one of them, and its resources: entry must survive with it -- pulling it would un-deploy every OTHER resource in that file, which nobody asked to touch. That invariant is the first test, and it is one that bites: dropping the entry on any delete rather than only on the last one fails it immediately. Three unit tests at the writer (the multi-document guard, the last-document removal, and a delete batched with a governed write -- which without this could not build the counterfactual tree at all, so the oracle refused the whole flush), three at the editor, and an e2e that closes the round trip against the real controller: the placement spec creates a ConfigMap in a kustomize overlay and asserts the entry appears; the new spec deletes it, asserts the entry is gone, and then asks the cluster to `apply -k` the folder -- which is the whole point, and which a dangling entry fails. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/UPGRADING.md | 12 +- internal/git/kustomize_delete_test.go | 173 ++++++++++++++++++ internal/git/manifestedit/kustomization.go | 58 ++++++ .../git/manifestedit/kustomization_test.go | 59 ++++++ internal/git/plan_flush.go | 62 ++++++- test/e2e/new_file_placement_e2e_test.go | 38 ++++ 6 files changed, 396 insertions(+), 6 deletions(-) create mode 100644 internal/git/kustomize_delete_test.go diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 15c0b836..27aacd38 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -28,7 +28,17 @@ into your source manifest as though you had typed it there. rewriting files it should have left alone. Check `git log` on your manifests if you want to see whether a past reconcile touched an image or a replica count you did not change. -**One behavior change comes with them.** A write routed through a kustomization is now +**Deleting a resource now also removes its `resources:` entry.** Previously the manifest was +deleted and the entry was left behind, pointing at a file that no longer exists — which +kustomize refuses to build over (*"accumulating resources … doesn't exist"*), so the folder +became undeployable and the `GitTarget` was refused on the next reconcile. Registering the +entry when a resource is created was only half the job. + +The entry is removed **only when the file itself is actually gone**. A file holding several +documents survives the deletion of one of them, and its entry stays — pulling it would +un-deploy every other resource in that file. + +**One more behavior change.** A write routed through a kustomization is now re-rendered with kustomize before it is committed, and must reproduce the live object exactly while leaving every other rendered object untouched. A write that fails that check **refuses the flush** — `GitPathAccepted=False` / `WriteBoundaryRefused`, naming the file diff --git a/internal/git/kustomize_delete_test.go b/internal/git/kustomize_delete_test.go new file mode 100644 index 00000000..e2b71448 --- /dev/null +++ b/internal/git/kustomize_delete_test.go @@ -0,0 +1,173 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "os" + "path/filepath" + "testing" + + gogit "github.com/go-git/go-git/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/ConfigButler/gitops-reverser/internal/types" + "github.com/ConfigButler/gitops-reverser/internal/typeset" +) + +// Deleting a managed document is only half a delete when the file is named in a +// kustomization's resources:. A resources: entry pointing at a file that no longer exists is +// one kustomize refuses to build over — +// +// accumulating resources ... '/scan/api.yaml' doesn't exist +// +// — so the repository is left in a state no GitOps controller can deploy. +// +// The entry must come out. But ONLY when the file itself is actually gone: a file holding +// several documents survives the deletion of one of them, and pulling its resources: entry +// would then un-deploy every OTHER resource in it. That is the sharp edge here, and it is the +// first test below. + +const deleteKustomizationYAML = `apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +namespace: default +resources: + - web.yaml + - bundle.yaml +` + +// bundle.yaml holds TWO documents. Deleting one of them must leave the file — and therefore +// its resources: entry — in place, or the surviving document silently stops being deployed. +const deleteBundleYAML = `apiVersion: v1 +kind: ConfigMap +metadata: + name: keep-me +data: + a: "1" +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: delete-me +data: + b: "2" +` + +// seedDeleteWorktree stages the files in git, not merely on disk: removing a file the index +// has never seen fails with "entry not found", so a delete test that only wrote to the +// filesystem would be testing the fixture rather than the writer. +func seedDeleteWorktree(t *testing.T, worktree *gogit.Worktree, kustomization string) { + t.Helper() + seedPlacedManifest(t, worktree, "web.yaml", sharedImageDeploymentYAML("web")) + seedPlacedManifest(t, worktree, "bundle.yaml", deleteBundleYAML) + seedPlacedManifest(t, worktree, "kustomization.yaml", kustomization) +} + +func deleteConfigMapEvent(name string) Event { + return Event{ + Identifier: types.ResourceIdentifier{ + Version: "v1", Resource: "configmaps", Namespace: "default", Name: name, + }, + Operation: "DELETE", + } +} + +// mergedMapper resolves both kinds the delete-plus-governed-write flush needs. +func mergedMapper() typeset.Lookup { + return typeset.NewSnapshotRegistry(typeset.Snapshot{ + Entries: []typeset.Entry{ + { + GVK: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, + GVR: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, + Namespaced: true, + Allowed: true, + }, + { + GVK: schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, + GVR: schema.GroupVersionResource{Version: "v1", Resource: "configmaps"}, + Namespaced: true, + Allowed: true, + }, + }, + }) +} + +// THE ONE THAT MATTERS. bundle.yaml holds two ConfigMaps; one is deleted. The file still +// holds the other, so the file stays — and its resources: entry MUST stay with it. Removing +// the entry here would un-deploy keep-me, a resource nobody asked to touch. +func TestPlanFlush_DeletingOneDocumentOfAFileKeepsTheFileAndItsResourcesEntry(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + seedDeleteWorktree(t, worktree, deleteKustomizationYAML) + + changed, err := flushEventsForTest(t, writer, worktree, configMapMapper(), + deleteConfigMapEvent("delete-me")) + + require.NoError(t, err) + require.True(t, changed, "the document must have been removed") + + bundle, readErr := os.ReadFile(filepath.Join(root, "bundle.yaml")) + require.NoError(t, readErr, "the file must survive: it still holds another document") + assert.Contains(t, string(bundle), "name: keep-me", "the surviving document must be untouched") + assert.NotContains(t, string(bundle), "name: delete-me") + + assertFileBytes(t, filepath.Join(root, "kustomization.yaml"), deleteKustomizationYAML, + "the file still exists, so its resources: entry must NOT be removed — "+ + "pulling it would un-deploy keep-me") +} + +// The file's LAST document is deleted, so the file goes. Its resources: entry now names a +// file that does not exist, which kustomize refuses to build over — so the entry goes too, +// and the repository stays deployable. +func TestPlanFlush_DeletingTheLastDocumentAlsoRemovesTheResourcesEntry(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + seedDeleteWorktree(t, worktree, deleteKustomizationYAML) + + changed, err := flushEventsForTest(t, writer, worktree, configMapMapper(), + deleteConfigMapEvent("keep-me"), + deleteConfigMapEvent("delete-me"), + ) + + require.NoError(t, err) + require.True(t, changed) + + _, statErr := os.Stat(filepath.Join(root, "bundle.yaml")) + assert.True(t, os.IsNotExist(statErr), "the file held nothing else, so it is gone") + + kust, readErr := os.ReadFile(filepath.Join(root, "kustomization.yaml")) + require.NoError(t, readErr) + assert.NotContains(t, string(kust), "bundle.yaml", + "a resources: entry naming a file that no longer exists makes the folder unbuildable") + assert.Contains(t, string(kust), "web.yaml", "and every other entry is left exactly as it was") +} + +// The delete lands in the same flush as a governed write. Both must survive: without the +// entry removal the re-render cannot build the tree at all, and the oracle would refuse the +// whole flush — losing the delete AND the unrelated image bump. +func TestPlanFlush_DeleteAndGovernedWriteInOneFlush(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + seedDeleteWorktree(t, worktree, deleteKustomizationYAML+`images: + - name: ghcr.io/example/shared + newTag: "1.0.0" +`) + + changed, err := flushEventsForTest(t, writer, worktree, mergedMapper(), + sharedImageEvent("web", "ghcr.io/example/shared:2.0.0"), + deleteConfigMapEvent("keep-me"), + deleteConfigMapEvent("delete-me"), + ) + + require.NoError(t, err, "the tree must still build with the delete applied, or the oracle refuses everything") + require.True(t, changed) + + kust, readErr := os.ReadFile(filepath.Join(root, "kustomization.yaml")) + require.NoError(t, readErr) + assert.Contains(t, string(kust), `newTag: "2.0.0"`, "the governed write must land") + assert.NotContains(t, string(kust), "bundle.yaml", "and the dangling entry must be gone") +} diff --git a/internal/git/manifestedit/kustomization.go b/internal/git/manifestedit/kustomization.go index efd50da1..9d94eab1 100644 --- a/internal/git/manifestedit/kustomization.go +++ b/internal/git/manifestedit/kustomization.go @@ -178,6 +178,64 @@ func AppendKustomizationResource(path string, content []byte, entry string) (Edi return EditResult{Content: []byte(joinDocuments(docs)), Mode: EditPatched}, nil } +// RemoveKustomizationResource drops one entry from an existing kustomization.yaml's +// resources: sequence. It is AppendKustomizationResource's counterpart, and it exists for +// exactly the reason that one does, read backwards: a file named in resources: that no +// longer exists is a file kustomize refuses to build over — +// +// accumulating resources ... '/scan/apps/api.yaml' doesn't exist +// +// so deleting a managed document without removing its entry leaves a repository no GitOps +// controller can deploy. Deleting the manifest is only half the delete. +// +// It is idempotent (an entry that is not there is EditNoChange) and all-or-nothing in the +// same way as its sibling: a multi-document file, unparseable YAML, or a document with no +// resources: sequence skips the whole call with a diagnostic rather than inventing structure. +// An emptied sequence is left as an empty sequence — removing the key is not this function's +// call to make. +func RemoveKustomizationResource(path string, content []byte, entry string) (EditResult, []Diagnostic) { + skip := func(format string, args ...interface{}) (EditResult, []Diagnostic) { + return EditResult{Content: content, Mode: EditSkipped}, + []Diagnostic{diag(DiagWarning, Location{Path: path}, format, args...)} + } + + docs, idx, root, reason, ok := locateKustomizationDocument(path, content) + if !ok { + return skip("%s", reason) + } + target := docs[idx].body + + section := nodeMapGet(root, "resources") + if section == nil || section.Kind != yaml.SequenceNode { + return skip("kustomization %s has no resources sequence", path) + } + + kept := make([]*yaml.Node, 0, len(section.Content)) + removed := false + for _, item := range section.Content { + if item.Kind == yaml.ScalarNode && strings.TrimSpace(item.Value) == strings.TrimSpace(entry) { + removed = true + continue + } + kept = append(kept, item) + } + if !removed { + return EditResult{Content: content, Mode: EditNoChange}, nil + } + section.Content = kept + + encoded, err := encodeNode(root) + if err != nil { + return skip("kustomization %s: re-encode failed: %v", path, err) + } + body := reskinDocument(target, string(encoded)) + if body == target { + return EditResult{Content: content, Mode: EditNoChange}, nil + } + docs[idx].body = body + return EditResult{Content: []byte(joinDocuments(docs)), Mode: EditPatched}, nil +} + // setOverrideScalar writes the new value, keeping the value string-typed for the // image fields (the encoder quotes "1.29"-style values when the tag is !!str) and // integer-typed for count. An existing quoting style is kept; other styles reset diff --git a/internal/git/manifestedit/kustomization_test.go b/internal/git/manifestedit/kustomization_test.go index 520aeb3d..af35e422 100644 --- a/internal/git/manifestedit/kustomization_test.go +++ b/internal/git/manifestedit/kustomization_test.go @@ -183,6 +183,65 @@ func TestAppendKustomizationResource_IdempotentWhenAlreadyListed(t *testing.T) { } } +func TestRemoveKustomizationResource_DropsEntryPreservingHandAuthoring(t *testing.T) { + withExtra, _ := AppendKustomizationResource( + "kustomization.yaml", + []byte(kustomizationFixture), + "debug-toolbox.yaml", + ) + + res, diags := RemoveKustomizationResource("kustomization.yaml", withExtra.Content, "debug-toolbox.yaml") + if res.Mode != EditPatched { + t.Fatalf("Mode = %q, want patched (diags %+v)", res.Mode, diags) + } + got := string(res.Content) + if strings.Contains(got, "debug-toolbox.yaml") { + t.Errorf("the entry must be gone:\n%s", got) + } + // The hand-authoring, the surviving entry, and every unrelated section stay put. + for _, want := range []string{"# pin the app image here", "- deployment.yaml", "namespace: app", "count: 3"} { + if !strings.Contains(got, want) { + t.Errorf("want %q preserved in:\n%s", want, got) + } + } +} + +func TestRemoveKustomizationResource_IdempotentWhenNotListed(t *testing.T) { + res, _ := RemoveKustomizationResource("kustomization.yaml", []byte(kustomizationFixture), "never-there.yaml") + if res.Mode != EditNoChange { + t.Fatalf("Mode = %q, want no-change for an entry that is not listed", res.Mode) + } + if string(res.Content) != kustomizationFixture { + t.Errorf("a no-op must leave the bytes byte-identical") + } +} + +func TestRemoveKustomizationResource_RefusalsLeaveContentUntouched(t *testing.T) { + cases := []struct { + name string + content string + }{ + {"no resources sequence", "namespace: app\n"}, + {"resources is not a sequence", "resources: not-a-list\n"}, + {"multi-document file", kustomizationFixture + "---\nnamespace: other\n"}, + {"unparseable", "resources: [::\n"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + res, diags := RemoveKustomizationResource("kustomization.yaml", []byte(tc.content), "deployment.yaml") + if res.Mode != EditSkipped { + t.Fatalf("Mode = %q, want skipped", res.Mode) + } + if string(res.Content) != tc.content { + t.Errorf("a refused removal must leave the bytes untouched") + } + if len(diags) == 0 { + t.Errorf("a refusal must carry a diagnostic") + } + }) + } +} + func TestAppendKustomizationResource_RefusalsLeaveContentUntouched(t *testing.T) { cases := []struct { name string diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index 7dc7baca..e83ec8eb 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -227,7 +227,7 @@ func (wb *writeBatch) applyEvent(ctx context.Context, event Event) error { case event.IsFieldPatch(): return wb.applyFieldPatch(ctx, event) case event.Operation == "DELETE": - wb.applyDelete(event) + wb.applyDelete(ctx, event) return nil default: _, err := wb.applyUpsert(ctx, event) @@ -964,7 +964,7 @@ func (wb *writeBatch) writeWholeFile(ctx context.Context, event Event, rel strin // the same batch that shifted a multi-document file does not misdirect this one. // Removing the last document in a file marks it for deletion; otherwise the surviving // documents are kept byte-for-byte. -func (wb *writeBatch) applyDelete(event Event) { +func (wb *writeBatch) applyDelete(ctx context.Context, event Event) { target, found := wb.resolveDelete(event) if !found { return @@ -984,11 +984,63 @@ func (wb *writeBatch) applyDelete(event Event) { Removed: true, }) res, _ := manifestedit.DeleteDocument(buf.current, idx) - if res.FileEmpty { - buf.current = nil + if !res.FileEmpty { + buf.current = res.Content return } - buf.current = res.Content + // The file is gone. Anything still naming it in a resources: list now names a file that + // does not exist, and kustomize refuses to build over that — so deleting the manifest is + // only half the delete. + buf.current = nil + wb.dropKustomizationResource(ctx, event, target.filePath) +} + +// dropKustomizationResource removes the resources: entry naming a file this flush deleted, +// from every supported kustomization that lists it. +// +// It is the counterpart of appendKustomizationResource, and it fails the same way: a +// kustomization it cannot edit only loses its entry (logged), it does not abort the delete — +// the render precondition is what decides whether the resulting tree is committable. +func (wb *writeBatch) dropKustomizationResource(ctx context.Context, event Event, filePath string) { + for _, k := range sortedKustomizationPaths(wb.store.Kustomizations) { + info := wb.store.Kustomizations[k] + if info.Unsupported { + continue // never edit a kustomization we do not model + } + dir := path.Dir(info.Path) + for _, entry := range info.Resources { + if path.Clean(path.Join(dir, entry)) != filePath { + continue + } + wb.putToKustomize = true + buf := wb.buffer(info.Path) + if buf.current == nil { + continue // the kustomization is itself being deleted in this batch + } + res, diags := manifestedit.RemoveKustomizationResource(info.Path, buf.current, entry) + switch res.Mode { + case manifestedit.EditPatched: + buf.current = res.Content + log.FromContext(ctx).Info("Removed resources: entry for deleted file", + "kustomization", info.Path, "entry", entry, "resource", event.Identifier.String()) + case manifestedit.EditNoChange: + case manifestedit.EditSkipped, manifestedit.EditDeleted, manifestedit.EditWholeReplace: + log.FromContext(ctx).Info("Could not remove resources: entry for deleted file", + "kustomization", info.Path, "entry", entry, "resource", event.Identifier.String()) + logManifestDiagnostics(ctx, diags) + } + } + } +} + +// sortedKustomizationPaths keeps the edit order deterministic across reconciles. +func sortedKustomizationPaths(kusts map[string]*manifestanalyzer.KustomizationInfo) []string { + out := make([]string, 0, len(kusts)) + for dir := range kusts { + out = append(out, dir) + } + sort.Strings(out) + return out } // deleteTarget names the file and manifest identity a delete targets. The document diff --git a/test/e2e/new_file_placement_e2e_test.go b/test/e2e/new_file_placement_e2e_test.go index b0d2cfc7..3c745fbe 100644 --- a/test/e2e/new_file_placement_e2e_test.go +++ b/test/e2e/new_file_placement_e2e_test.go @@ -114,4 +114,42 @@ var _ = Describe("Manager New-File Placement", Label("manager", "new-file-placem By("✅ new resource placed inside the kustomize overlay and registered in resources:") }) + + // The round trip, and the half that used to be missing. Deleting the resource removes its + // file — and a resources: entry naming a file that no longer exists is one kustomize + // REFUSES to build over ("accumulating resources ... doesn't exist"), which would leave the + // repository in a state no GitOps controller can deploy. Registering the entry on create is + // only half the job if nothing ever takes it back out. + // + // Ordered: this deletes exactly the ConfigMap the spec above placed. + It("removes the resources: entry when the resource is deleted, leaving a folder that still builds", func() { + kustFullPath := filepath.Join(repo.CheckoutDir, gitPath, kustRepoPath) + newFileFullPath := filepath.Join(repo.CheckoutDir, gitPath, newFileRepoPath) + + By("deleting the ConfigMap from the cluster") + _, err := kubectlRunInNamespace(testNs, "delete", "configmap", newConfigMap) + Expect(err).NotTo(HaveOccurred(), "failed to delete the ConfigMap") + + By("verifying the file is gone AND its resources: entry went with it") + Eventually(func(g Gomega) { + pullLatestRepoState(g, repo.CheckoutDir) + + _, statErr := os.Stat(newFileFullPath) + g.Expect(os.IsNotExist(statErr)).To(BeTrue(), "the resource's file must be removed") + + kustBody := readRepoFile(g, kustFullPath) + g.Expect(kustBody).NotTo(ContainSubstring("- "+newFileRepoPath), + "a resources: entry pointing at a deleted file makes the folder unbuildable") + g.Expect(kustBody).To(ContainSubstring("- deployment.yaml"), + "and every other entry must be left exactly as it was") + }, 120*time.Second, 3*time.Second).Should(Succeed()) + + By("verifying kustomize can still build the folder — which is the whole point") + _, err = kubectlRunInNamespace(testNs, "apply", "-k", + filepath.Join(repo.CheckoutDir, gitPath), "--dry-run=server") + Expect(err).NotTo(HaveOccurred(), + "the overlay must still build after the delete; a dangling resources: entry would fail here") + + By("✅ deleted resource's file and its resources: entry both removed; the folder still builds") + }) }) From 8602f4271e2ee0b96d89d2a3af487fe857f95842 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 18:50:52 +0000 Subject: [PATCH 11/13] docs(kustomize): drop what the projection swap made untrue render-root-scoping.md still listed renderImage and isReplicaKind as "still there" with their divergences described in the present tense, and still planned the oracle as future work whose differential test was to be a comparison against simulateImageRender. All three are deleted, so those rows and that step now describe code that does not exist. render-attribution.md loses its "one step left" framing and the long status block: the workstream is done, so the doc is a record of how attribution was decided, not a plan. No content is removed that still says something true. The bug ledger stays exactly as written, because how each bug was found is the argument for the method. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../support-boundary/render-attribution.md | 67 +++++++------------ .../support-boundary/render-root-scoping.md | 30 +++++---- 2 files changed, 42 insertions(+), 55 deletions(-) diff --git a/docs/design/support-boundary/render-attribution.md b/docs/design/support-boundary/render-attribution.md index 89c2a559..8dc0fff9 100644 --- a/docs/design/support-boundary/render-attribution.md +++ b/docs/design/support-boundary/render-attribution.md @@ -8,43 +8,26 @@ > [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) -> **Status: §7 is done — stages 1 through 3 are shipped.** `renderRootWith` is the -> counterfactual primitive; verification is a real re-render (`VerifyBatchRenders`), and -> `simulateImageRender` is gone; attribution is the dye (`dye.go`, -> `overrides_attribution.go`), and `renderImage`, `imageSuppliers` and `isReplicaKind` are -> gone with **B1, B2 and B3**. The 12 `TestSplitDesired_*` tests are rebuilt against real -> kustomize fixtures, and the corpus differential is now the stronger claim that **an in-sync -> folder projects to a complete no-op**. Stage 4 (chain → set) is not done. +> **Status: shipped (§7 stages 1-3).** `renderRootWith` is the counterfactual primitive; +> verification is a real re-render (`VerifyBatchRenders`); attribution is the dye (`dye.go`, +> `overrides_attribution.go`). `renderImage`, `imageSuppliers`, `simulateImageRender` and +> `isReplicaKind` are deleted, and **B1, B2 and B3** with them. Stage 4 (chain to set) is not +> done. > -> Two things the building corrected, both recorded below where they belong: +> Building it corrected two things, both marked below: **B4 is worse than recorded** (a +> rendered object is not a valid `unstructured` at all; `DeepCopyJSON` *panics* on the Go +> `int` kustomize returns), and **the rename-chain guard must use kustomize's compiled regex, +> not the string equality §3 proposed** (an entry `name:` is a regex, so `mirror/ap.` matches +> `mirror/app` without equalling it). > -> - **B4 is worse than stated.** A rendered object is not merely awkward to read a number off -> — it is **not a valid `unstructured` at all**. `DeepCopyJSON` **panics** on one (*"cannot -> deep copy int"*), so a rendered object must be normalised through JSON before any -> apimachinery helper touches it. -> - **The rename-chain guard must use kustomize's regex, not string equality** (§3 proposed -> equality). An entry's `name:` is a regex, so `mirror/ap.` matches `mirror/app` without -> equalling it, and an equality guard would dye the name, kill the next entry, and -> mis-attribute the tag. -> -> §5's verdict — *attribution may be heuristic, verification may not* — survived contact -> intact, and it is what made the rest safe to delete. But see -> *patching-kustomize.md* (ConfigButler/kustomize-tracer, `plans/`): the dye is now the **fallback** for an -> unpatched build, not the destination. - -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. +> §5's verdict, *attribution may be heuristic, verification may not*, is what made the rest +> safe to delete. But see *patching-kustomize.md* (ConfigButler/kustomize-tracer, `plans/`): +> the dye is now the **fallback** for an unpatched build, not the destination. -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. +This document is the record of how attribution was decided: the approach we had settled on, +a second that is better, and a third that keeps getting proposed and cannot work. Each is +**measured against kustomize v0.21.1**, not argued from its source. §6 is the bug ledger the +measuring produced. --- @@ -134,11 +117,11 @@ collide.** Attribution by "what changed" is blind to a writer whose write is inv 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. +The `renderImage` this replaced got both right, because it attributed by *position in the +chain* (last matching entry wins) and never compared values. So Approach A was not merely +imperfect: **shipping it would have regressed behaviour that already worked**, in exchange +for deleting the code that made it work. Worth saying plainly, because the design was settled +before the measurement contradicted it. --- @@ -251,9 +234,9 @@ this field"**. A `patches:` entry (or a `replacements:` block) that clobbers the 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 +The code this replaced could not make that distinction. `simulateImageRender` "verified" the +inversion against a simulation that **shared its own blind spot**, so a value owned by a patch +was 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 diff --git a/docs/design/support-boundary/render-root-scoping.md b/docs/design/support-boundary/render-root-scoping.md index f3de515c..fcb292e0 100644 --- a/docs/design/support-boundary/render-root-scoping.md +++ b/docs/design/support-boundary/render-root-scoping.md @@ -39,17 +39,21 @@ code: > live object exactly — and leave every other object in the build byte-identical — refuse > the write.** +> **This shipped.** It is [`VerifyBatchRenders`](../../../internal/manifestanalyzer/render_verify.go), +> run as a write-plan precondition once per flush. See +> [render-attribution.md](render-attribution.md) §5. + 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. +`simulateImageRender`: 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 +work: `simulateImageRender` replayed **our re-implemented image chain, not kustomize**, and it +checked **only the images it planned** — never that the rest of the build is untouched. So it +was image-specific verification, and it shared a blind spot with the thing it verified: 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 +simulation and wrongly in reality. (It is deleted; the re-render replaced it.) 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. @@ -72,8 +76,8 @@ renderer** — and without one, neither half of the guarantee above is actually | 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`. | +| `renderImage` | a ~20-line reimplementation of kustomize's image transformer | **deleted.** It diverged from kustomize (its matcher was string equality where kustomize's is a *regex over the whole image string*). Attribution now reads a dyed render: [render-attribution.md](render-attribution.md) §3. | +| `isReplicaKind` | the replica transformer's fieldspec, hardcoded to three kinds | **deleted.** kustomize's fieldspec has four; it missed `ReplicationController`. There is no list of kinds any more. | | [`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 @@ -321,10 +325,10 @@ silently lost**. That is the gate on all of this, and it is the right one. 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. +3. ~~**The oracle**~~ — **done.** krusty, sandboxed, in the acceptance gate (#232) and in the + write-plan precondition (`VerifyBatchRenders`). The differential test against + `simulateImageRender` was overtaken: the simulation is deleted, and the corpus test that + replaced it makes the stronger claim that an in-sync folder projects to a no-op. 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. From 139ddc9088b4a299726072bd6256cf2793c8b7fa Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 19:09:23 +0000 Subject: [PATCH 12/13] fix(kustomize): address review on #233 -- verify every delete, and ask both questions in anyOverrides Three findings from the review. One was right, one was wrong but pointed at a real gap next to it, and one was a lint break. 1. anyOverrides only asked whether a CHAIN existed, never whether an ATTRIBUTION did. A document can render an image with no images:/replicas: entry governing it: the chain is nil, the attribution is not. Two roots disagreeing only there would diverge in the fingerprint while ambiguous() stayed false, so the document would be silently un-routed with no diagnostic and no fan-in refusal. Routing was already safe (record() nils the attribution on any divergence), but the guard was not. The divergence is not constructible today -- a nil chain means no image transformer ran, which means the rendered value IS the source value, which is the same in every root -- so this changes no current behaviour. It is fixed because the invariant is not obvious enough to leave resting on that argument. Test added, as asked. 2. The claim was that applyDelete never sets putToKustomize, so a delete could commit a dangling resources: entry. That is not so: dropKustomizationResource sets the flag BEFORE attempting the removal, so a removal that fails still puts the flush to the oracle. Measured: forcing RemoveKustomizationResource to fail refuses the flush with kustomize's own "'/scan/bundle.yaml' doesn't exist". But the instinct was right about the case beside it. A delete that does NOT empty the file never set the flag at all, so the rule was "verified only when our own bookkeeping says the file went" rather than "verified because it is inside a render root". The whole point of the oracle is that it does not take our word for anything, so the rule is now uniform: any delete of a document whose file is named in a resources: list goes to the oracle. The lookup that decides that is now one function shared with the entry removal, so the two cannot drift. 3. MD028: a bare blank line ended the blockquote instead of continuing it. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../support-boundary/render-attribution.md | 2 +- internal/git/kustomize_delete_test.go | 24 ++++++ internal/git/plan_flush.go | 85 ++++++++++++------- internal/manifestanalyzer/override_chain.go | 22 ++++- internal/manifestanalyzer/overrides_test.go | 42 +++++++++ 5 files changed, 141 insertions(+), 34 deletions(-) diff --git a/docs/design/support-boundary/render-attribution.md b/docs/design/support-boundary/render-attribution.md index 8dc0fff9..f275edc6 100644 --- a/docs/design/support-boundary/render-attribution.md +++ b/docs/design/support-boundary/render-attribution.md @@ -7,7 +7,7 @@ > *patching-kustomize.md* (ConfigButler/kustomize-tracer, `plans/`) — **revises §4**: the fork is ~30 lines, and it is built · > [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) - +> > **Status: shipped (§7 stages 1-3).** `renderRootWith` is the counterfactual primitive; > verification is a real re-render (`VerifyBatchRenders`); attribution is the dye (`dye.go`, > `overrides_attribution.go`). `renderImage`, `imageSuppliers`, `simulateImageRender` and diff --git a/internal/git/kustomize_delete_test.go b/internal/git/kustomize_delete_test.go index e2b71448..741d7f91 100644 --- a/internal/git/kustomize_delete_test.go +++ b/internal/git/kustomize_delete_test.go @@ -3,6 +3,7 @@ package git import ( + "context" "os" "path/filepath" "testing" @@ -145,6 +146,29 @@ func TestPlanFlush_DeletingTheLastDocumentAlsoRemovesTheResourcesEntry(t *testin assert.Contains(t, string(kust), "web.yaml", "and every other entry is left exactly as it was") } +// A delete inside a render root goes to the oracle, whether or not the file itself goes with +// it. So if the resources: entry is ever left dangling — by a kustomization we cannot edit, or +// by a future refactor that forgets to remove it — the re-render fails to build and the flush +// is refused, rather than committing a repository kustomize cannot deploy. +// +// Raised in review on #233. It is verified here rather than argued: the delete flush below +// runs the render precondition, which is what makes the dangling-entry case unreachable +// instead of merely unlikely. +func TestPlanFlush_DeleteInsideARenderRootIsVerified(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + seedDeleteWorktree(t, worktree, deleteKustomizationYAML) + + scan, err := scanWorktreeSubtree(root) + require.NoError(t, err) + batch := newWriteBatch(context.Background(), writer, configMapMapper(), scan, nil) + batch.applyDelete(context.Background(), deleteConfigMapEvent("delete-me")) + + assert.True(t, batch.putToKustomize, + "the deleted document's file is named in resources:, so the flush must be put to kustomize") +} + // The delete lands in the same flush as a governed write. Both must survive: without the // entry removal the re-render cannot build the tree at all, and the oracle would refuse the // whole flush — losing the delete AND the unrelated image bump. diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index e83ec8eb..5f550fba 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -983,6 +983,13 @@ func (wb *writeBatch) applyDelete(ctx context.Context, event Event) { Name: target.id.Name, Removed: true, }) + // Any delete inside a render root changes what that root renders, so it goes to the + // oracle — whether or not the file itself goes with it. Gating this on "the file was + // emptied" would be a rule about our own bookkeeping rather than about the render, and + // the whole point of the oracle is that it does not take our word for anything. + if len(wb.kustomizationsListing(target.filePath)) > 0 { + wb.putToKustomize = true + } res, _ := manifestedit.DeleteDocument(buf.current, idx) if !res.FileEmpty { buf.current = res.Content @@ -1002,44 +1009,60 @@ func (wb *writeBatch) applyDelete(ctx context.Context, event Event) { // kustomization it cannot edit only loses its entry (logged), it does not abort the delete — // the render precondition is what decides whether the resulting tree is committable. func (wb *writeBatch) dropKustomizationResource(ctx context.Context, event Event, filePath string) { - for _, k := range sortedKustomizationPaths(wb.store.Kustomizations) { - info := wb.store.Kustomizations[k] + for _, listing := range wb.kustomizationsListing(filePath) { + buf := wb.buffer(listing.kustomization) + if buf.current == nil { + continue // the kustomization is itself being deleted in this batch + } + res, diags := manifestedit.RemoveKustomizationResource(listing.kustomization, buf.current, listing.entry) + switch res.Mode { + case manifestedit.EditPatched: + buf.current = res.Content + log.FromContext(ctx).Info("Removed resources: entry for deleted file", + "kustomization", listing.kustomization, "entry", listing.entry, + "resource", event.Identifier.String()) + case manifestedit.EditNoChange: + case manifestedit.EditSkipped, manifestedit.EditDeleted, manifestedit.EditWholeReplace: + // The entry stays, and it now names a file that does not exist. We do not have to + // decide how bad that is: the render precondition rebuilds the tree and refuses + // the flush, because kustomize will not build over a missing resource. + log.FromContext(ctx).Info("Could not remove resources: entry for deleted file", + "kustomization", listing.kustomization, "entry", listing.entry, + "resource", event.Identifier.String()) + logManifestDiagnostics(ctx, diags) + } + } +} + +// resourceListing is one kustomization's resources: entry naming a given file. +type resourceListing struct { + kustomization string // the kustomization.yaml's own path + entry string // the entry text, relative to the kustomization's directory +} + +// kustomizationsListing returns every supported kustomization whose resources: names filePath, +// in deterministic order. It is the one definition of "this file is inside a render root", +// shared by the oracle's trigger and by the entry removal, so the two cannot drift apart. +func (wb *writeBatch) kustomizationsListing(filePath string) []resourceListing { + dirs := make([]string, 0, len(wb.store.Kustomizations)) + for dir := range wb.store.Kustomizations { + dirs = append(dirs, dir) + } + sort.Strings(dirs) + + var out []resourceListing + for _, dir := range dirs { + info := wb.store.Kustomizations[dir] if info.Unsupported { continue // never edit a kustomization we do not model } - dir := path.Dir(info.Path) + base := path.Dir(info.Path) for _, entry := range info.Resources { - if path.Clean(path.Join(dir, entry)) != filePath { - continue - } - wb.putToKustomize = true - buf := wb.buffer(info.Path) - if buf.current == nil { - continue // the kustomization is itself being deleted in this batch - } - res, diags := manifestedit.RemoveKustomizationResource(info.Path, buf.current, entry) - switch res.Mode { - case manifestedit.EditPatched: - buf.current = res.Content - log.FromContext(ctx).Info("Removed resources: entry for deleted file", - "kustomization", info.Path, "entry", entry, "resource", event.Identifier.String()) - case manifestedit.EditNoChange: - case manifestedit.EditSkipped, manifestedit.EditDeleted, manifestedit.EditWholeReplace: - log.FromContext(ctx).Info("Could not remove resources: entry for deleted file", - "kustomization", info.Path, "entry", entry, "resource", event.Identifier.String()) - logManifestDiagnostics(ctx, diags) + if path.Clean(path.Join(base, entry)) == filePath { + out = append(out, resourceListing{kustomization: info.Path, entry: entry}) } } } -} - -// sortedKustomizationPaths keeps the edit order deterministic across reconciles. -func sortedKustomizationPaths(kusts map[string]*manifestanalyzer.KustomizationInfo) []string { - out := make([]string, 0, len(kusts)) - for dir := range kusts { - out = append(out, dir) - } - sort.Strings(out) return out } diff --git a/internal/manifestanalyzer/override_chain.go b/internal/manifestanalyzer/override_chain.go index 081f91a0..03d4aef7 100644 --- a/internal/manifestanalyzer/override_chain.go +++ b/internal/manifestanalyzer/override_chain.go @@ -241,7 +241,7 @@ func record(out map[chainKey]*overrideAssignment, key chainKey, ov *KustomizeOve chainKeys: map[string]struct{}{fp: {}}, overrides: ov, rendered: rd, - anyOverrides: ov != nil, + anyOverrides: hasSomethingAtStake(ov, rd), } return } @@ -249,12 +249,30 @@ func record(out map[chainKey]*overrideAssignment, key chainKey, ov *KustomizeOve return // the same chain and the same attribution, reached twice; not an ambiguity } prev.chainKeys[fp] = struct{}{} - prev.anyOverrides = prev.anyOverrides || ov != nil + prev.anyOverrides = prev.anyOverrides || hasSomethingAtStake(ov, rd) // More than one distinct answer: route through none of them. prev.overrides = nil prev.rendered = nil } +// hasSomethingAtStake reports whether a root's view of a document carries anything an edit +// could be routed through, which is what makes a disagreement between two roots worth +// refusing rather than merely noting. +// +// It asks about the ATTRIBUTION as well as the chain. The chain alone is not the same +// question: an object can render with no images:/replicas: entry governing it at all (ov nil) +// and still carry a rendered image the projection reads (rd non-nil). Two roots that +// disagreed only there would diverge in the fingerprint while ambiguous() stayed false, so the +// document would be silently un-routed with no diagnostic and no fan-in refusal. +// +// That divergence is not constructible today — a nil chain means no image transformer ran, +// which means the rendered value IS the source value, which is the same in every root — so +// this changes no current behaviour. It is here because the invariant it protects is not +// obvious enough to leave resting on that argument. +func hasSomethingAtStake(ov *KustomizeOverrides, rd *RenderedOverrides) bool { + return ov != nil || rd != nil +} + // fingerprint reduces a chain to a comparable string, so two roots reaching one // document can be compared for agreement. func fingerprint(ov *KustomizeOverrides) string { diff --git a/internal/manifestanalyzer/overrides_test.go b/internal/manifestanalyzer/overrides_test.go index 545866c4..d3a02011 100644 --- a/internal/manifestanalyzer/overrides_test.go +++ b/internal/manifestanalyzer/overrides_test.go @@ -8,6 +8,8 @@ import ( "path/filepath" "testing" + "github.com/stretchr/testify/require" + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" "github.com/ConfigButler/gitops-reverser/internal/typeset" ) @@ -247,3 +249,43 @@ func hasOverrideAmbiguityDiag(store *ManifestStore) bool { } return false } + +// A document can render an image while NO images:/replicas: entry governs it: the chain is +// nil, but the attribution is not, because the projection still needs to know what the folder +// renders that slot to. The two are different questions, and anyOverrides has to ask both -- +// otherwise two roots disagreeing only on the attribution would diverge in the fingerprint +// while ambiguous() stayed false, leaving the document silently un-routed with no diagnostic +// and no fan-in refusal. +// +// Requested in review on #233. +func TestRecord_AttributionWithoutAChainIsStillSomethingAtStake(t *testing.T) { + files := []manifestedit.FileContent{ + {Path: "deployment.yaml", Content: []byte(`apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + template: + spec: + containers: + - name: app + image: app:v1 +`)}, + // No images:, no replicas: -- nothing governs this document. + {Path: "kustomization.yaml", Content: []byte("resources:\n - deployment.yaml\n")}, + } + + assignments, failed := renderChains(files, parseKustomizations(files)) + require.Empty(t, failed) + + a := assignments[chainKey{originPath: "deployment.yaml", kind: "Deployment", name: "web"}] + require.NotNil(t, a) + require.Nil(t, a.overrides, "no entry governs it, so there is no chain") + require.NotNil(t, a.rendered, "but the folder still renders its image, and the projection reads that") + require.True(t, a.anyOverrides, + "an attribution with no chain is still something an edit could be routed through") + + // And with nothing supplying the image, an edit to it flows into the source file. + require.Empty(t, a.rendered.Images["/spec/template/spec/containers\x00app"].Tag, + "no entry supplies the tag") +} From db3216d849831b45a556a4dbcc547f950688fdde Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 19:12:21 +0000 Subject: [PATCH 13/13] docs(support-boundary): the prompt for tolerating patches, with the ordering fact measured patches is the biggest refusal cause in the layout corpus (8, against namePrefix 6 and configMapGenerator 6), it refuses the whole GitTarget rather than the edit, and the dye shipped in #233 is the mechanism its milestone was waiting for. Carries the four things worth measuring rather than re-deriving, the load-bearing one being that patches run BEFORE the images:/replicas: transformers and the transformers WIN -- so a patch that sets a field an entry also governs is dead text. That is the fact that makes 'just edit the patch' wrong, and the reason the dye should be extended into the patch rather than the patch being read and reasoned about. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../next-prompt-simple-patches.md | 168 ++++++++++++++++++ 1 file changed, 168 insertions(+) create mode 100644 docs/design/support-boundary/next-prompt-simple-patches.md diff --git a/docs/design/support-boundary/next-prompt-simple-patches.md b/docs/design/support-boundary/next-prompt-simple-patches.md new file mode 100644 index 00000000..8b84f07c --- /dev/null +++ b/docs/design/support-boundary/next-prompt-simple-patches.md @@ -0,0 +1,168 @@ +# Prompt: tolerate `patches:`, and route a simple one + +Copy everything below the line into a fresh session. + +--- + +Continue the kustomize support-boundary workstream. The next thing is `patches:`, and it +comes in two halves that must not be confused: **tolerating a patch** (accept the folder, +mirror what it renders) and **authoring a patch** (write one from scratch). The first is the +job. The second stays deferred and this change must not drift into it. + +## Read first + +- `AGENTS.md`. +- The memory note `kustomize-renderer-workstream`. +- **`docs/design/support-boundary/render-root-scoping.md` §6** — this is the design. It is + titled "Why a patch still blocks the folder, and why it should not", and it already + separates the two gates you need: *renderable* (per folder) and *routable* (per object, + per field). +- `docs/design/support-boundary/render-attribution.md` §3 (the dye and its sink proof), §5 + (*attribution may be heuristic, verification may not*), §7 (the guardrails). + +## Where things stand + +PR #233 shipped the projection swap. Attribution is a **dyed render**: a nonce is written +into every declared override entry, the root is rendered a second time, and the entry a value +came from is read off the output. Verification is a **real re-render** (`VerifyBatchRenders`): +before any kustomize-governed flush is committed, the whole tree is rebuilt with the write +applied, every written document must reproduce its live object, and **every object the flush +did not write must come out byte-identical**. A proposal that fails refuses the flush and +names the file and the object. + +The re-implemented transformers (`renderImage`, `imageSuppliers`, `simulateImageRender`, +`isReplicaKind`) are gone. Nothing left in the write path models kustomize; it asks kustomize. + +## Why patches, and why now + +Counted across the layout corpus, `patches` is **the single biggest refusal cause**: 8 +occurrences, against `namePrefix` 6 and `configMapGenerator` 6. And it refuses the **whole +GitTarget**, not the edit. `flux-monorepo/apps/production`, whose patch touches replicas and +an env var, therefore also loses `images:`/`replicas:` edit-through, which the patch has +nothing to do with. + +`render-attribution.md` §3 names the blocker in as many words: *"Per-field refusal requires +per-field attribution. The dye is the mechanism that milestone is waiting for."* That +mechanism now exists. This is the milestone it unblocks. + +## Four things measured, not assumed. Do not re-derive them; do extend them. + +1. **A folder containing `patches:` builds fine.** kustomize renders it without complaint. + The refusal is *our fence*, not kustomize's. (Probe: add a `patches:` block to an + `imageFixture`-style tree and call `renderRoot`. No build failure.) + +2. **Patches run BEFORE `images:`/`replicas:`, and the transformers WIN.** Measured with a + patch and an entry both aimed at the same fields: + + ``` + patch says image app:patched, images: says newTag v2 -> renders app:v2 + patch says replicas 7, replicas: says count 3 -> renders 3 + ``` + + **A patch that sets a field an entry also governs is DEAD TEXT.** This is the fact that + makes "just edit the patch" wrong, and it is the first thing to get right. + +3. **The dye already answers both cases correctly, with no model of patches.** Where the entry + still matches, the dye lands and the value is attributed to the *entry* (the patch's value + is dead text). Where the patch changes the image *name*, the entry stops matching, no dye + lands, nothing is attributed, and the oracle refuses the write. Neither outcome required + knowing what a patch is. + +4. **A patch-owned field is currently unroutable, and fails safe.** No dye, so no attribution, + so nothing routes to an entry; the write falls back to the source document, the re-render + shows the patch overriding it, and the flush is refused. Wrong *outcome* for the user, + right *direction*: it refuses rather than corrupting. + +## The one thing not to get wrong + +**Do not read the patch and reason about who wins.** That is re-implementation wearing a +better hat, and (2) is exactly the ordering rule you would get wrong. The patch is a sparse +KRM document, so its fields are trivially readable, and that is precisely the trap: reading +them tells you what the patch *asks for*, never what the build *does*. + +**Ask kustomize.** Extend the dye to scalars inside the patch: write a nonce into the patch's +value, render, and see whether it survives to the output. If it does, the patch supplies that +field and an edit belongs in the patch. If it does not, something downstream overrode it, and +the dye that *did* land tells you who. Ordering, precedence and dead text all fall out for +free, and none of it has to be modelled. + +**The sink proof is not optional.** §3: a dye is sound only where the dyed value is a **pure +sink**, never an input to a matcher. Inside a patch this bites harder than it does for images: + +- **a list merge key is a selector, not a sink.** `containers[].name` is how strategic merge + decides *which* container to merge into. Dye it and the patch merges into the wrong element, + or creates a new one. Never dye a merge key. Enumerate the ones you rely on and say why each + is safe. +- `metadata.name` / `metadata.namespace` in the patch body are the object selector. Same rule. +- `$patch: delete` / `$patch: replace` directives change what the merge *means*. A dye near one + is not a sink. + +**Baseline first, then dye** (§7): if the dyed build errors where the real one did not, the dye +hit something that is not a sink. Fall back to **NO ATTRIBUTION**, never to another heuristic. + +## Order of work. Each stage is independently shippable; the risky one is last. + +1. **Tolerate.** Stop refusing the folder for `patches:`. It comes out of the unsupported set + for *acceptance*; it stays unsupported for *authoring*. Nothing routes to a patch yet, so a + patch-owned edit is refused by the oracle exactly as it is today, with a message that says + so. **Regenerate the corpus baseline and read it**: this is the stage whose whole point is + the rows that move, and it is where you find out what tolerating patches actually exposes + (`flux-monorepo` should go from Partial to more-accepted; look at what else it drags in). + *Ship this alone if the rest slips.* It converts 8 whole-folder refusals into per-field + ones. + +2. **Attribute.** Extend the dye to patch scalars, with the merge-key guard above. A field the + patch supplies is now attributable to `(patch file, field path)`. + +3. **Route.** A live change to a patch-supplied scalar edits the **patch document**, at the + same path, preserving its bytes and comments (this is `manifestedit` territory, the same + way `PatchKustomization` preserves hand-authoring). The oracle proves it or refuses it. + +4. **Refuse per field, not per folder.** Name the patch that owns the field and say that + authoring is not supported. This is the tier-2 accounting in + `unreflectable-edits-and-write-gating.md`. + +## Scope: what "simple" has to mean, and it must be enforced, not assumed + +Start with **one** shape and refuse the rest by name: + +- a **single** strategic-merge patch per object (two patches touching one field is a precedence + question you have not earned yet); +- **scalar** fields (`spec.replicas`, an env value, a resource limit). A change *inside a list* + is merge-key semantics and is a different problem; +- `patches:` with a **`path:`** to a sparse KRM document. Inline `patch: |` and **JSON6902** + (`op`/`from`/`value`) are not sparse KRM at all and should be refused explicitly rather than + fall through. + +Check what `FixKustomization` does with the deprecated `patchesStrategicMerge` and +`patchesJson6902` spellings before you write the parser: the analyzer already runs it +(`kustomization_parse.go`), and folded fields will arrive in `Patches` looking like something +they are not. **Measure it; do not assume.** + +## The test net + +The 12 `TestSplitDesired_*` tests now build a real tree, render it with kustomize, read the +attribution off a dyed render, and drive the projection with the result. Extend that harness; +do not build a second one. The corpus invariant to hold onto is +`TestProjection_InSyncCorpusFolderIsANoOp`: **an in-sync folder must project to a complete +no-op.** A patch that we mis-attribute will show up there as a phantom edit, across every +fixture in both corpora. + +Add the ordering fact (2) as a fixture. It is the one a future refactor will break. + +## Validation and delivery + +Full sequence per `AGENTS.md`: `task fmt` → `generate` → `manifests` → `vet` → `lint` → +`test` → `test-e2e` (needs Docker; check `docker info`; run sequentially). Regenerate +`task gitops-layouts-baseline` and **explain every row that moves** — in stage 1 the moving +rows are the deliverable. Branch off `main`, push, open a PR, and report the honest line +delta, including when it is unflattering. + +## How this workstream finds bugs + +Every stage of it has found a real, shipped bug, and not one came from reading kustomize's +source. They came from making kustomize the arbiter and probing it with a throwaway test: the +regex image matcher, the missing `ReplicationController`, the digest that silently clears the +tag, the `int` that panics `DeepCopyJSON`, and the patch-versus-transformer ordering above. + +**When you want to know what kustomize does, do not reason about it. Ask it.**