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