From cb522474555116d2a48c43ad514a13e2ce1fbc83 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 20:55:41 +0000 Subject: [PATCH 1/5] fix(kustomize): stop writing the build's own output into the build's input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The writer mirrors a live object into the file that produced it. Under kustomize that file is not what the cluster runs, and mirroring the live object straight back writes the BUILD'S OUTPUT into the build's INPUT. Measured, on a folder we accept today, with nothing changed in the cluster and nothing changed in the render: a kustomization declaring `labels:` + `commonAnnotations:` and nothing else commits the overlay's `env: prod` and `owner: platform` into the base manifest, on the first reconcile of an unchanged folder. Every reconcile of an in-sync folder produced a commit, and the file was left wrong. In a base shared by two overlays, the value baked in is one environment's. The fix is one rule, and it models no transformer: WHERE THE LIVE OBJECT AND THE RENDER AGREE, THE SOURCE KEEPS ITS BYTES. WHERE THEY DISAGREE, THE USER CHANGED SOMETHING, AND THAT IS WHAT WE WRITE. Agreement means the build already produces exactly what the cluster runs, so the source is — by construction — what produced it, and there is nothing to write. Disagreement is the user's edit: it is routed to an images:/replicas: entry when the dye says one supplies the field, and written through to the source otherwise — where, if the build owns the field, the re-render refuses the flush. Because it needs no model of labels, of namespace, or of a patch, it closes all of them at once. It is also the gate on tolerating `patches:` at all: a patched base would otherwise absorb one environment's values, and no re-render can catch that (the patch re-imposes its value, so the render comes out identical). Two behavior changes go with it: - the re-render now runs for ANY document a render root produces, not only one an override chain governs. A change to a build-supplied field in a folder with no images:/replicas: entries used to be committed and silently never converge; it is now a reported refusal. - a live change the projection cannot place is refused (`unplaceable-edit`): the build and the user both rewrote one list whose elements carry no unique `name:` to pair them by. Pairing by position is not a conservative guess, it is measurably wrong — kustomize PREPENDS a container a patch adds. The corpus no-op invariant now compares the WHOLE document, not just its images, which is how a projection that quietly rewrote every field we had not modelled passed it for as long as it did: 8 documents checked before, 53 now. --- .coverage-baseline | 2 +- docs/UPGRADING.md | 46 +++ .../support-boundary/render-root-scoping.md | 15 + internal/git/plan_flush.go | 61 +++- internal/git/source_form_test.go | 179 ++++++++++ internal/manifestanalyzer/acceptance.go | 12 + internal/manifestanalyzer/analyzer_test.go | 1 + .../manifestanalyzer/kustomization_parse.go | 9 +- .../kustomize_render_semantics_test.go | 72 ++++ .../manifestanalyzer/kustomize_render_test.go | 39 ++- .../manifestanalyzer/overrides_attribution.go | 81 ++++- .../manifestanalyzer/overrides_projection.go | 61 +++- .../overrides_projection_test.go | 4 +- internal/manifestanalyzer/source_form.go | 326 ++++++++++++++++++ internal/manifestanalyzer/source_form_test.go | 303 ++++++++++++++++ internal/watch/event_router.go | 8 +- 16 files changed, 1159 insertions(+), 60 deletions(-) create mode 100644 internal/git/source_form_test.go create mode 100644 internal/manifestanalyzer/source_form.go create mode 100644 internal/manifestanalyzer/source_form_test.go diff --git a/.coverage-baseline b/.coverage-baseline index 427656bc..219e3542 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -75.6 +75.7 diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 27aacd38..de349513 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,52 @@ 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 — the build's output stops leaking into the build's input (next minor; bug fix + behavior change) + +**If your kustomization declares `labels:`, `commonLabels:`, `commonAnnotations:` or `namespace:`, +the operator has been writing those injected values into your source manifests.** Measured, on a +folder we accept today, with nothing changed in the cluster and nothing changed in the render: + +```yaml +# kustomization.yaml (yours) # deployment.yaml, after one reconcile (ours) +labels: metadata: + - pairs: labels: + env: prod env: prod # <- the OVERLAY's, absorbed into the BASE +commonAnnotations: annotations: + owner: platform owner: platform +``` + +The writer mirrors a live object into the file that produced it — but under kustomize that file is +not what the cluster runs, and mirroring the live object straight back writes the build's own +output into the build's input. Every reconcile of an unchanged folder produced a commit, and the +file was left wrong: delete the kustomization later and the injected values are now yours forever. +In a base shared by two overlays, the value baked in is **one environment's**. + +The fix needs no model of any transformer, and it is now the rule the writer follows: + +> **Where the live object and the render agree, the source keeps its bytes. Where they disagree, +> the user changed something, and that is what we write.** + +**Nothing needs migration** — this is a fix, and it makes the operator stop rewriting files it +should have left alone. If a past reconcile has already baked injected metadata into a manifest, +the operator will not remove it for you; remove it by hand and it will not come back. + +**Two behavior changes go with it.** + +*The re-render now runs for any document a kustomization produces*, not only for one an +`images:`/`replicas:` entry governs. A change to a field the build supplies (relabelling a live +object whose label a `labels:` block sets, say) cannot be expressed in the repository: the source +file cannot hold it, because the build would stamp its own value straight back. That write now +**refuses the flush** — `GitPathAccepted=False` / `WriteBoundaryRefused`, naming the file and the +object — where before it was committed and silently never converged. + +*A live change the projection cannot place is refused* (`unplaceable-edit`). It fires when the +build and the live object have **both** rewritten one list whose elements carry no unique `name:` +to pair them by — the source's `args:` rewritten by a patch, for example. There is no honest way +to say which of the source's bytes you meant to keep, and pairing the lists by position is +measurably wrong (kustomize *prepends* a container a patch adds), so the operator refuses rather +than guesses. + ## Unreleased — kustomize decides what it renders, and what it touched (next minor; bug fixes + behavior change) The write path no longer contains a re-implementation of kustomize's image and replica diff --git a/docs/design/support-boundary/render-root-scoping.md b/docs/design/support-boundary/render-root-scoping.md index fcb292e0..2fcba9b1 100644 --- a/docs/design/support-boundary/render-root-scoping.md +++ b/docs/design/support-boundary/render-root-scoping.md @@ -90,6 +90,7 @@ 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**. + (`vars` was moved off the tolerated set by #229 and now refuses the folder.) - **`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. @@ -101,6 +102,20 @@ incorrectly, for the transformers we called benign. A renderer replaces three policies with one. +> **The leak is fixed, and the fix is one rule, not three policies.** +> [`sourceForm`](../../../internal/manifestanalyzer/source_form.go): *where the live object and +> the render agree, the source keeps its bytes; where they disagree, the user changed something, +> and that is what we write.* Agreement means the build already produces exactly what the cluster +> runs, so the source is — by construction — what produced it, and there is nothing to write. It +> needs no model of `commonLabels`, of `namespace`, or of a patch, which is precisely why it +> closes all three at once and is what makes §6 possible at all. +> +> The leak was measured before it was fixed, and it was not theoretical: a folder declaring +> `labels:` + `commonAnnotations:` and nothing else committed the overlay's `env: prod` into the +> base manifest on the first reconcile of an *unchanged* folder. The corpus no-op invariant now +> compares the **whole document** rather than only its images, which is how a projection that +> quietly rewrote every field we had not modelled passed that test for as long as it did. + --- ## 3. The oracle diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index 5f550fba..a89a2b55 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -674,12 +674,13 @@ func (wb *writeBatch) patchExisting( desired = desired.DeepCopy() desired.SetNamespace("") } - projected := manifestreport.Project(desired) - var overrideEdits []manifestanalyzer.OverrideEdit - if dm.Rendered != nil { - if gitRaw, parsed := gitDocRawObject(buf.current, idx); parsed { - projected, overrideEdits = manifestanalyzer.SplitDesiredForOverrides(gitRaw, projected, dm.Rendered) - } + projected, overrideEdits, err := projectThroughKustomize( + manifestreport.Project(desired), buf.current, idx, dm) + if err != nil { + // The projection could not place the edit. Refusing the whole flush is the point: the + // alternative is to write the live object through and silently absorb the build's own + // output into the file that feeds it. + return upsertNoChange, sourceFormRefusal(filePath, id, err) } c := manifestedit.Comparison{ Git: gitDoc, @@ -715,7 +716,15 @@ func (wb *writeBatch) patchExisting( // what the second one renders to, so by the time the second is processed there is nothing // left to write. Its render still moves, and it moves onto its own live state — that is // the resource converging, not collateral damage, and only its declared intent says so. - if dm.Overrides != nil { + // + // The oracle is armed for ANY document a render root produces, not only one an override + // chain governs, and the difference is a hole rather than a refinement. The source form + // leaves a field the build supplies to the source file — but where the live object and the + // render DISAGREE the user has changed something, and that change is written through. If a + // transformer or a patch owns that field it will be overridden right back, and the write + // never converges. Only the re-render can see that, and until now it did not run at all + // unless an images:/replicas: entry happened to exist somewhere in the chain. + if dm.Rendered != nil { wb.putToKustomize = true } if outcome == upsertUpdated || dm.Overrides != nil { @@ -724,6 +733,44 @@ func (wb *writeBatch) patchExisting( return outcome, nil } +// projectThroughKustomize turns the live projection into the SOURCE FORM of it: the object the +// file should hold once everything the build supplies is left to the build, plus the entry edits +// for the values an images:/replicas: entry supplies. +// +// A document no render root produces (dm.Rendered nil), or one whose Git bytes will not parse, +// passes straight through: there is no build standing between the file and the cluster, so the +// live projection IS what the file should hold. +func projectThroughKustomize( + projected *unstructured.Unstructured, + content []byte, + idx int, + dm *manifestanalyzer.DocumentModel, +) (*unstructured.Unstructured, []manifestanalyzer.OverrideEdit, error) { + if dm.Rendered == nil { + return projected, nil, nil + } + gitRaw, parsed := gitDocRawObject(content, idx) + if !parsed { + return projected, nil, nil + } + return manifestanalyzer.SplitDesiredForOverrides(gitRaw, projected, dm.Rendered) +} + +// sourceFormRefusal turns a projection that could not place an edit into the same reported +// refusal every other write-boundary violation surfaces as: GitPathAccepted=False / Stalled=True, +// naming the file and the object. It is not an internal error — the folder is fine and the +// operator is fine; the EDIT had nowhere honest to land, and saying so is the whole contract. +func sourceFormRefusal(filePath string, id manifestedit.Identity, err error) error { + return &manifestanalyzer.AcceptanceRefusedError{ + Issues: []manifestanalyzer.AcceptanceIssue{{ + Kind: manifestanalyzer.IssueUnplaceableEdit, + Path: filePath, + Message: fmt.Sprintf("%s/%s in %s: %v", + id.Kind, id.Name, filePath, err), + }}, + } +} + // renderPrecondition is the oracle, and it is a write-plan precondition like the three // above it: it runs at the one moment the whole plan is known and before a single byte is // touched, so a refusal aborts the flush and commits nothing. diff --git a/internal/git/source_form_test.go b/internal/git/source_form_test.go new file mode 100644 index 00000000..0ca6ff04 --- /dev/null +++ b/internal/git/source_form_test.go @@ -0,0 +1,179 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// THE BUILD'S OUTPUT MUST NOT BECOME THE BUILD'S INPUT. +// +// The writer mirrors a live object into the file that produced it. Under kustomize that file is +// not what the cluster runs, and mirroring the live object straight back wrote the OVERLAY's +// values into the BASE — measured, on a folder the operator accepts today, with nothing changed +// in the cluster and nothing changed in the render. +// +// These pin it at the commit, which is where it bit. + +// labelledKustomizationYAML injects metadata into every object it renders. It is a supported +// folder: `labels` and `commonAnnotations` are on the modelled list, and there is no patch and no +// generator anywhere near it. +const labelledKustomizationYAML = `apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +namespace: default +resources: + - apps/deployment.yaml +labels: + - pairs: + env: prod + includeSelectors: false +commonAnnotations: + owner: platform +` + +const labelledDeploymentYAML = `apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + selector: + matchLabels: + app: web + template: + metadata: + labels: + app: web + spec: + containers: + - name: podinfo + image: ghcr.io/example/podinfo:6.3.0 # hand-authored, and it stays that way +` + +// labelledLiveDeployment is the object as the CLUSTER holds it — which is the RENDER: the overlay's +// label and annotation are on it, because kustomize put them there before Flux applied it. +func labelledLiveDeployment(labelValue string) Event { + return Event{ + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "apps/v1", + "kind": "Deployment", + "metadata": map[string]interface{}{ + "name": "web", + "namespace": "default", + "labels": map[string]interface{}{"env": labelValue}, + "annotations": map[string]interface{}{"owner": "platform"}, + }, + "spec": map[string]interface{}{ + "selector": map[string]interface{}{"matchLabels": map[string]interface{}{"app": "web"}}, + "template": map[string]interface{}{ + "metadata": map[string]interface{}{ + "labels": map[string]interface{}{"app": "web"}, + "annotations": map[string]interface{}{"owner": "platform"}, + }, + "spec": map[string]interface{}{ + "containers": []interface{}{ + map[string]interface{}{"name": "podinfo", "image": "ghcr.io/example/podinfo:6.3.0"}, + }, + }, + }, + }, + }}, + Identifier: types.ResourceIdentifier{ + Group: "apps", Version: "v1", Resource: "deployments", Namespace: "default", Name: "web", + }, + Operation: "UPDATE", + } +} + +func seedLabelledWorktree(t *testing.T, root string) (string, string) { + t.Helper() + deployPath := filepath.Join(root, "apps", "deployment.yaml") + kustPath := filepath.Join(root, "kustomization.yaml") + require.NoError(t, os.MkdirAll(filepath.Dir(deployPath), 0o750)) + require.NoError(t, os.WriteFile(deployPath, []byte(labelledDeploymentYAML), 0o600)) + require.NoError(t, os.WriteFile(kustPath, []byte(labelledKustomizationYAML), 0o600)) + return deployPath, kustPath +} + +// The folder is in sync: the live object IS the render. So the flush must do NOTHING. +// +// It used to write `env: prod` and `owner: platform` into the base manifest — the overlay's +// metadata, absorbed into the file the overlay renders, as if the author had typed it. Every +// reconcile of an unchanged folder produced a commit, and the file was left wrong: remove the +// kustomization later and the drift is permanent. +func TestPlanFlush_InjectedMetadataIsNeverWrittenIntoTheSourceManifest(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + deployPath, kustPath := seedLabelledWorktree(t, worktree.Filesystem.Root()) + + changed, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), + labelledLiveDeployment("prod")) + + require.NoError(t, err) + assert.False(t, changed, "the live object is exactly what the folder renders: there is nothing to write") + assertFileBytes(t, deployPath, labelledDeploymentYAML, + "the overlay's labels and annotations belong to the BUILD, not to the file it renders") + assertFileBytes(t, kustPath, labelledKustomizationYAML, "and the kustomization is untouched") +} + +// A field the build does NOT supply is still the user's, and still lands in the file. The fix +// must not turn a kustomize folder into a read-only one. +func TestPlanFlush_UngovernedFieldStillLandsInTheSourceManifest(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + deployPath, _ := seedLabelledWorktree(t, worktree.Filesystem.Root()) + + event := labelledLiveDeployment("prod") + containers, _, err := unstructured.NestedSlice(event.Object.Object, "spec", "template", "spec", "containers") + require.NoError(t, err) + containers[0].(map[string]interface{})["image"] = "ghcr.io/example/podinfo:6.5.0" + require.NoError(t, unstructured.SetNestedSlice( + event.Object.Object, containers, "spec", "template", "spec", "containers")) + + changed, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), event) + require.NoError(t, err) + require.True(t, changed) + + deploy, err := os.ReadFile(deployPath) + require.NoError(t, err) + assert.Contains(t, string(deploy), "ghcr.io/example/podinfo:6.5.0", + "no images: entry governs the tag, so the change belongs in the file") + assert.Contains(t, string(deploy), "# hand-authored, and it stays that way", + "and it is an in-place edit, so the comment survives") + assert.NotContains(t, string(deploy), "env: prod", + "the injected label still has no business in the source file") +} + +// And a change to a field the BUILD supplies is refused, not absorbed. The user relabels the live +// Deployment; no entry can express it and the source file cannot hold it — the overlay would +// stamp `env: prod` straight back on the next render. The re-render sees exactly that and refuses +// the flush, which is the reported outcome the design demands over a write that never converges. +// +// The oracle now runs for ANY document a render root produces. It used to run only when an +// images:/replicas: entry existed somewhere in the chain — and this folder has none, so this write +// went through unchecked and silently failed to converge, forever. +func TestPlanFlush_RefusesAChangeToABuildSuppliedField(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + deployPath, kustPath := seedLabelledWorktree(t, worktree.Filesystem.Root()) + + _, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), + labelledLiveDeployment("staging")) + + var refused *manifestanalyzer.AcceptanceRefusedError + require.ErrorAs(t, err, &refused, "the flush must be refused, and refused legibly") + assert.Contains(t, refused.Error(), "Deployment/web", "the refusal names the object") + assert.True(t, refused.AllIssuesOfKinds(manifestanalyzer.IssueRenderRefused), + "the renderer is what refused it, so it surfaces as WriteBoundaryRefused") + + assertFileBytes(t, deployPath, labelledDeploymentYAML, "a refused flush writes nothing") + assertFileBytes(t, kustPath, labelledKustomizationYAML, "and touches no kustomization") +} diff --git a/internal/manifestanalyzer/acceptance.go b/internal/manifestanalyzer/acceptance.go index 9602aaf0..6f8ee4e8 100644 --- a/internal/manifestanalyzer/acceptance.go +++ b/internal/manifestanalyzer/acceptance.go @@ -134,6 +134,18 @@ const ( // next render — so absorbing it would leave a resource silently un-mirrored forever, // which is the exact failure this whole path exists to prevent. IssueRenderRefused IssueKind = "kustomize-render-refused" + // IssueUnplaceableEdit marks a live change the projection could not place in the source + // document: the BUILD and the USER both rewrote one list, and its elements carry no unique + // name: to pair the source's with the render's by (SourceFormRefusedError). + // + // The alternative to refusing is aligning the two lists by position, and that is not a + // conservative guess — it is measurably wrong: kustomize's strategic merge PREPENDS a + // container a patch adds, so the source's first element is not the render's first element in + // exactly the case where it matters. Writing one element's fields into another is the kind of + // corruption no re-render can catch, because the patch re-imposes its own values and the + // render comes out identical either way. So this refusal is not the oracle being cautious; it + // is the one place the oracle cannot see, and it must fail loudly instead. + IssueUnplaceableEdit IssueKind = "unplaceable-edit" // A refusal made up purely of the write-boundary kinds above surfaces as the GitTarget // reason WriteBoundaryRefused rather than the umbrella UnsupportedContent: the folder holds diff --git a/internal/manifestanalyzer/analyzer_test.go b/internal/manifestanalyzer/analyzer_test.go index baee943b..b1ef9563 100644 --- a/internal/manifestanalyzer/analyzer_test.go +++ b/internal/manifestanalyzer/analyzer_test.go @@ -125,6 +125,7 @@ func TestAnalyze_Issues(t *testing.T) { IssueWriteEscapesScope: 0, IssueWriteFanIn: 0, IssueRenderRefused: 0, + IssueUnplaceableEdit: 0, } for kind, n := range want { if got := countIssues(rep, kind); got != n { diff --git a/internal/manifestanalyzer/kustomization_parse.go b/internal/manifestanalyzer/kustomization_parse.go index 65907abc..30c0ed1a 100644 --- a/internal/manifestanalyzer/kustomization_parse.go +++ b/internal/manifestanalyzer/kustomization_parse.go @@ -58,10 +58,11 @@ func supportedKustomizationFields() map[string]struct{} { "Images": {}, "Replicas": {}, - // Tolerated exactly as before this change, so it stays a refactor. These - // inject metadata into rendered objects and therefore leak into mirrored - // source files as drift — a known defect, tracked separately, deliberately - // not altered here. + // These inject metadata into every rendered object. They used to leak into mirrored + // source files as drift — the writer mirrored the live object, injected labels and + // all, back into the file the overlay renders. That is fixed at the source: the + // projection leaves every field the BUILD supplies to the build (sourceForm), so an + // injected label is no longer something the file can absorb. "CommonLabels": {}, "Labels": {}, "CommonAnnotations": {}, diff --git a/internal/manifestanalyzer/kustomize_render_semantics_test.go b/internal/manifestanalyzer/kustomize_render_semantics_test.go index 8b652afc..6c5d4a74 100644 --- a/internal/manifestanalyzer/kustomize_render_semantics_test.go +++ b/internal/manifestanalyzer/kustomize_render_semantics_test.go @@ -4,6 +4,9 @@ package manifestanalyzer import ( "fmt" + "testing" + + "github.com/stretchr/testify/require" "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" ) @@ -41,3 +44,72 @@ spec: {Path: "kustomization.yaml", Content: []byte(kustomization)}, } } + +// PATCHES RUN FIRST, AND THE TRANSFORMERS WIN. +// +// This is the fact that makes "the patch asks for it, so edit the patch" wrong, and it is the one +// a future refactor will break. A patch that sets a field an images:/replicas: entry also governs +// is DEAD TEXT: kustomize applies the patch, then the transformers overwrite it, and the value the +// user reads in the patch file is not the value the cluster runs. +// +// It cannot be established by reading the patch — reading it tells you what the patch ASKS for, +// never what the build DOES — so it is pinned here as a render, against the library that decides +// it. The transformations annotation says the same thing in the same breath: PatchTransformer +// runs, and then the two transformers do. +func TestRender_ATransformerOverridesAPatchOnTheSameField(t *testing.T) { + files := []manifestedit.FileContent{ + {Path: "deployment.yaml", Content: []byte(`apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + replicas: 1 + template: + spec: + containers: + - name: app + image: ghcr.io/example/app:1.0.0 +`)}, + {Path: "patch.yaml", Content: []byte(`apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + replicas: 7 + template: + spec: + containers: + - name: app + image: ghcr.io/example/app:patched +`)}, + {Path: "kustomization.yaml", Content: []byte(`resources: + - deployment.yaml +patches: + - path: patch.yaml +images: + - name: ghcr.io/example/app + newTag: 2.0.0 +replicas: + - name: web + count: 3 +`)}, + } + + rendered, err := renderRoot(files, ".") + require.NoError(t, err) + require.Len(t, rendered, 1, "a patch is a build input, not a resource: it renders no object of its own") + + object := rendered[0].Object.Object + require.Equal(t, "ghcr.io/example/app:2.0.0", + nestedOf(t, object, "spec", "template", "spec", "containers", "0", "image"), + "the images: entry wins; the patch's :patched never reaches the cluster") + require.Equal(t, 3, nestedOf(t, object, "spec", "replicas"), + "the replicas: entry wins; the patch's 7 never reaches the cluster") + + var order []string + for _, tr := range rendered[0].TransformedBy { + order = append(order, tr.Kind) + } + require.Equal(t, []string{"PatchTransformer", "ReplicaCountTransformer", "ImageTagTransformer"}, order, + "kustomize itself says which ran when, and the patch ran first") +} diff --git a/internal/manifestanalyzer/kustomize_render_test.go b/internal/manifestanalyzer/kustomize_render_test.go index f136e8df..321ac91f 100644 --- a/internal/manifestanalyzer/kustomize_render_test.go +++ b/internal/manifestanalyzer/kustomize_render_test.go @@ -86,19 +86,30 @@ func assertInSyncIsANoOp( t.Helper() where := ro.OriginPath + " " + ro.Object.GetKind() + "/" + ro.Object.GetName() - out, edits := SplitDesiredForOverrides(src.Object, asLiveObject(t, ro.Object), attribution) + out, edits, err := SplitDesiredForOverrides(src.Object, asLiveObject(t, ro.Object), attribution) + require.NoError(t, err, "%s: an in-sync folder needs no edit placed at all", where) require.Empty(t, edits, "%s: the folder is already in sync, so nothing may be routed to an entry", where) - for _, slot := range collectImageSlots(out.Object) { - want := sourceImageAt(src.Object, slot.key) - if want == "" { - continue // the live object has a slot the source does not; not our claim - } - require.Equal(t, want, slot.image, - "%s: an in-sync folder must hand back the SOURCE image untouched at %q", where, slot.key) - } + // THE WHOLE DOCUMENT, not just its images. The claim is that an in-sync folder projects to a + // complete no-op, and checking only the image slots is how a projection that quietly rewrote + // every other field — an injected label, a patched CPU request — passed this test while + // corrupting source files. There is nothing special about the image: it is simply the field + // we happened to model. + require.Equal(t, normaliseForCompare(t, src.Object), normaliseForCompare(t, out.Object), + "%s: an in-sync folder must hand the SOURCE document back untouched", where) +} + +// normaliseForCompare puts a document through JSON so the three number types in play (kustomize's +// int, the API machinery's int64, the YAML decoder's float64) compare as the numbers they are. +func normaliseForCompare(t *testing.T, obj map[string]interface{}) map[string]interface{} { + t.Helper() + encoded, err := json.Marshal(obj) + require.NoError(t, err) + var out map[string]interface{} + require.NoError(t, json.Unmarshal(encoded, &out)) + return out } // asLiveObject turns a RENDERED object into the shape a live one actually has. @@ -122,16 +133,6 @@ func asLiveObject(t *testing.T, rendered *unstructured.Unstructured) *unstructur return &unstructured.Unstructured{Object: obj} } -// sourceImageAt is the image the SOURCE document holds at a slot, or "" when it has none. -func sourceImageAt(src map[string]interface{}, key string) string { - for _, slot := range collectImageSlots(src) { - if slot.key == key { - return slot.image - } - } - return "" -} - // ourAttributionFor is what the store attributes to a document, and whether it was found // ambiguous (reached by more than one render root with differing answers, which we refuse to // route through). diff --git a/internal/manifestanalyzer/overrides_attribution.go b/internal/manifestanalyzer/overrides_attribution.go index cfbdc318..8a1f0c08 100644 --- a/internal/manifestanalyzer/overrides_attribution.go +++ b/internal/manifestanalyzer/overrides_attribution.go @@ -3,6 +3,9 @@ package manifestanalyzer import ( + "crypto/sha256" + "encoding/hex" + "encoding/json" "math" "sort" "strconv" @@ -25,6 +28,14 @@ import ( // behind each override-produced value. A nil supplier means THE SOURCE DOCUMENT supplies // that value — so an edit to it belongs in the file, not in an entry. type RenderedOverrides struct { + // Object is the whole object kustomize renders this document to. It is what the live + // object is compared against, and it is the reason the writer can tell a value the BUILD + // supplied from a value the USER set without modelling a single transformer: where the + // live object and the render agree, the source keeps its bytes (see sourceForm). + // + // It is JSON-normalised, because a rendered object is not valid unstructured: kustomize + // hands numbers back as Go `int`, which makes DeepCopyJSON panic outright. + Object map[string]interface{} // Images is keyed by image slot (the container list path plus the container name), so // the live object, the Git document and the render all address the same field. Images map[string]RenderedImage @@ -90,13 +101,23 @@ func renderedKey(o renderedObject) chainKey { return chainKey{originPath: o.OriginPath, kind: o.Object.GetKind(), name: o.Object.GetName()} } -// readDyes reads the nonces out of ONE document's image slots and replica count. +// readDyes reads the nonces out of ONE document's image slots and replica count, and keeps the +// rendered object itself. // -// Only the fields being attributed are read. The whole output is never grepped for a nonce: -// vars and replacements can carry a dyed value into args, env, or ConfigMap data, and a dye -// found there says nothing about who supplies the image. +// Only the fields being attributed are read FOR ATTRIBUTION. The whole output is never grepped +// for a nonce: vars and replacements can carry a dyed value into args, env, or ConfigMap data, +// and a dye found there says nothing about who supplies the image. +// +// The rendered object is kept whole, though, and that is a different question from attribution: +// the writer does not need to know WHO supplied a field in order to know THAT the build did — +// it only has to compare the live object against the render (see sourceForm). So a document with +// no image slot and no replica count still gets a RenderedOverrides: it has no entry to route +// to, but the build may still have written into it, and the source must be protected from that. func readDyes(plain, dyed renderedObject, plan *dyePlan) *RenderedOverrides { - out := &RenderedOverrides{Images: map[string]RenderedImage{}} + out := &RenderedOverrides{ + Object: jsonNormalised(plain.Object.Object), + Images: map[string]RenderedImage{}, + } dyedSlots := map[string]imageSlot{} for _, s := range collectImageSlots(dyed.Object.Object) { @@ -127,7 +148,26 @@ func readDyes(plain, dyed renderedObject, plan *dyePlan) *RenderedOverrides { out.Replicas = replicas } - if len(out.Images) == 0 && out.Replicas == nil { + if out.Object == nil { + return nil // an object we cannot even normalise is one we must not compare against + } + return out +} + +// jsonNormalised round-trips a RENDERED object through JSON. +// +// It is not tidiness. A rendered object is not a valid unstructured: kustomize hands numbers +// back as Go `int`, and DeepCopyJSON accepts only the JSON types, so unstructured.DeepCopy +// PANICS on one outright ("cannot deep copy int"). The API server hands out JSON, so the live +// object this is compared against has int64 — and the comparison has to be made between two +// values a JSON encoder agrees on, not between two Go types that happen to hold the same number. +func jsonNormalised(obj map[string]interface{}) map[string]interface{} { + encoded, err := json.Marshal(obj) + if err != nil { + return nil + } + var out map[string]interface{} + if err := json.Unmarshal(encoded, &out); err != nil { return nil } return out @@ -176,10 +216,39 @@ func renderedReplicaCount(obj map[string]interface{}) (int64, bool) { // reaching one document can be compared for agreement exactly as their chains are. This is the // sharper question the fan-in check was always trying to ask: do two roots attribute this // field to DIFFERENT entries? +// +// It covers the whole RENDERED OBJECT as well as the entries, and that is load-bearing rather +// than thorough. Two overlays that render one base document to different objects — one patching +// an env var, the other not — agree on every images:/replicas: entry and would fingerprint +// identically. The writer would then compare a live object from one environment against the +// other environment's render and call the difference a user edit. Asking whether the two roots +// RENDER THE SAME OBJECT is the question that has to be asked, and the answer marks the document +// ambiguous: no attribution, and the fan-in precondition refuses any write to it. func fingerprintRendered(rd *RenderedOverrides) string { if rd == nil { return "" } + var b strings.Builder + b.WriteString(hashObject(rd.Object)) + b.WriteByte('\x03') + b.WriteString(fingerprintEntries(rd)) + return b.String() +} + +// hashObject digests a rendered object. json.Marshal sorts map keys, so it is key-order +// independent, and the digest keeps the fingerprint small enough to be a map key. +func hashObject(obj map[string]interface{}) string { + encoded, err := json.Marshal(obj) + if err != nil { + return "\x00unhashable" + } + sum := sha256.Sum256(encoded) + return hex.EncodeToString(sum[:]) +} + +// fingerprintEntries reduces the per-field ATTRIBUTION — which entry supplied which value — to a +// comparable string. +func fingerprintEntries(rd *RenderedOverrides) string { keys := make([]string, 0, len(rd.Images)) for k := range rd.Images { keys = append(keys, k) diff --git a/internal/manifestanalyzer/overrides_projection.go b/internal/manifestanalyzer/overrides_projection.go index 14c2a2f7..71db7e27 100644 --- a/internal/manifestanalyzer/overrides_projection.go +++ b/internal/manifestanalyzer/overrides_projection.go @@ -33,15 +33,21 @@ type OverrideEdit struct { } // SplitDesiredForOverrides maps the live desired object back through what kustomize actually -// renders. It returns the object the source document should be compared against — a copy of -// desired with every override-produced value restored to its SOURCE form, so the file keeps -// its bytes — plus the entry edits for the values an override entry supplies. +// renders. It returns the object the source document should be compared against — the SOURCE +// FORM of the live state, so the file keeps every byte the build supplied — plus the entry edits +// for the values an override entry supplies. // -// It is driven by RenderedOverrides, which carries both halves of the answer straight from -// the renderer: what each field renders to, and which entry supplied it (read off a dyed -// counterfactual build). Nothing in here re-implements a kustomize transformer, which is the -// entire point of this workstream — every shipped bug in this area came from the -// re-implementation, and all of them are deleted with it. +// It is two rules, and neither models a transformer: +// +// 1. WHERE THE LIVE OBJECT AND THE RENDER AGREE, THE SOURCE KEEPS ITS BYTES (sourceForm). The +// build already produces what the cluster runs, so the source is by construction what +// produced it. This is what stops the writer mirroring the build's own output back into the +// build's input — an injected label, a patched CPU request — and it needs to know nothing +// about labels or patches to do it. +// 2. WHERE THEY DISAGREE, THE USER CHANGED SOMETHING. If an images:/replicas: entry supplies +// that field — which the dye says, read off a counterfactual render — the change is routed +// to the ENTRY and the source keeps its bytes there too. Otherwise it is written through to +// the source document. // // Anything it cannot route safely — a component removal an entry supplies, a component a // sibling entry clears, or two containers demanding different values for one entry field — @@ -50,6 +56,10 @@ type OverrideEdit struct { // field an entry governs it will not, so it becomes a reported refusal rather than a commit // that quietly never converges. // +// The one thing it refuses outright is a list the build and the user BOTH changed whose elements +// cannot be paired by name (*SourceFormRefusedError): there is no honest way to say which of the +// source's bytes the user meant to keep, and aligning by position is measurably wrong. +// // gitRaw is the source document parsed as JSON-typed maps (sigs.k8s.io/yaml); desired is the // sanitized projection the writer would otherwise compare. The returned object is always a // copy; desired is never mutated. @@ -57,14 +67,18 @@ func SplitDesiredForOverrides( gitRaw map[string]interface{}, desired *unstructured.Unstructured, rendered *RenderedOverrides, -) (*unstructured.Unstructured, []OverrideEdit) { +) (*unstructured.Unstructured, []OverrideEdit, error) { if rendered == nil || desired == nil || gitRaw == nil { - return desired, nil + return desired, nil, nil } - out := desired.DeepCopy() - edits := projectImages(gitRaw, out, rendered.Images) - edits = append(edits, projectReplicas(gitRaw, out, rendered.Replicas)...) - return out, edits + source, err := sourceForm(gitRaw, rendered.Object, desired.Object) + if err != nil { + return nil, nil, err + } + out := &unstructured.Unstructured{Object: source} + edits := projectImages(gitRaw, desired, out, rendered.Images) + edits = append(edits, projectReplicas(gitRaw, desired, out, rendered.Replicas)...) + return out, edits, nil } // imageRef is an image reference split into its three overridable components. @@ -236,7 +250,7 @@ type slotPlan struct { // document alone can carry, and the re-render adjudicates it. func projectImages( gitRaw map[string]interface{}, - out *unstructured.Unstructured, + live, out *unstructured.Unstructured, rendered map[string]RenderedImage, ) []OverrideEdit { if len(rendered) == 0 { @@ -246,18 +260,27 @@ func projectImages( for _, s := range collectImageSlots(gitRaw) { gitImages[s.key] = s.image } + // The slot is READ off the live object and WRITTEN on the source form: they are two + // different documents now, and the whole point of this step is that the image the user set + // does not have to end up in the file the source form is built from. + outSlots := map[string]imageSlot{} + for _, s := range collectImageSlots(out.Object) { + outSlots[s.key] = s + } var plans []slotPlan - for _, slot := range collectImageSlots(out.Object) { + for _, slot := range collectImageSlots(live.Object) { src, inGit := gitImages[slot.key] render, isRendered := rendered[slot.key] - if !inGit || !isRendered { + target, inSource := outSlots[slot.key] + if !inGit || !isRendered || !inSource { continue // a new container writes through; the supplier rule converges it later } plan, routable := invertImage(slot, src, render) if !routable { return nil // one unroutable slot abandons routing for the whole object } + plan.slot = target plans = append(plans, plan) } edits, ok := collectConsistentEdits(plans) @@ -421,13 +444,13 @@ func collectConsistentEdits(plans []slotPlan) ([]OverrideEdit, bool) { // kustomize's fieldspec is the authority, and we no longer keep a second opinion about it. func projectReplicas( gitRaw map[string]interface{}, - out *unstructured.Unstructured, + live, out *unstructured.Unstructured, rendered *RenderedReplicas, ) []OverrideEdit { if rendered == nil || rendered.Entry == nil { return nil // no entry supplies the count; a scale flows into the source document } - liveCount, liveHas, err := unstructured.NestedInt64(out.Object, "spec", "replicas") + liveCount, liveHas, err := unstructured.NestedInt64(live.Object, "spec", "replicas") if err != nil || !liveHas { return nil } diff --git a/internal/manifestanalyzer/overrides_projection_test.go b/internal/manifestanalyzer/overrides_projection_test.go index efcbddc5..e8cba0d1 100644 --- a/internal/manifestanalyzer/overrides_projection_test.go +++ b/internal/manifestanalyzer/overrides_projection_test.go @@ -49,7 +49,9 @@ func splitFixture( var gitRaw map[string]interface{} require.NoError(t, yaml.Unmarshal(contentOf(t, files, sourcePath), &gitRaw)) - return SplitDesiredForOverrides(gitRaw, desired, assignment.rendered) + out, edits, err := SplitDesiredForOverrides(gitRaw, desired, assignment.rendered) + require.NoError(t, err, "the projection must be able to place the edit") + return out, edits } func contentOf(t *testing.T, files []manifestedit.FileContent, path string) []byte { diff --git a/internal/manifestanalyzer/source_form.go b/internal/manifestanalyzer/source_form.go new file mode 100644 index 00000000..5524ab96 --- /dev/null +++ b/internal/manifestanalyzer/source_form.go @@ -0,0 +1,326 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "bytes" + "encoding/json" + "sort" + + "k8s.io/apimachinery/pkg/runtime" +) + +// The source form: what the SOURCE DOCUMENT should hold, given what the cluster holds. +// +// The writer mirrors a live object into the file that produced it. Under kustomize that file +// is not what the cluster runs — the build stands between them — and mirroring the live object +// straight back writes THE BUILD'S OWN OUTPUT into the build's INPUT. Measured, on features we +// accept today: +// +// kustomization.yaml: labels: [{pairs: {env: prod}}] · commonAnnotations: {owner: platform} +// deployment.yaml gains, on the next reconcile: labels: {env: prod}, annotations: {owner: platform} +// +// Nothing changed in the cluster and nothing changed in the render; the source file simply +// absorbed the overlay's metadata as if the author had typed it. Do it once and the file is +// wrong; remove the kustomization later and the drift is permanent. The same shape, with a +// patch instead of a label, silently rewrites a base with one environment's values — which is +// why this is the gate on tolerating patches at all. +// +// The rule needs no model of any transformer, and it is the whole of this file: +// +// WHERE THE LIVE OBJECT AND THE RENDER AGREE, THE SOURCE KEEPS ITS BYTES. +// WHERE THEY DISAGREE, THE USER CHANGED SOMETHING, AND THAT IS WHAT WE WRITE. +// +// Agreement means the build already produces exactly what the cluster runs, so whatever the +// source says at that field is — by construction — the thing that produced it. Keeping it is +// not a heuristic; changing it would be writing a value we did not get from the user. +// +// Disagreement is the user's edit. It is written through to the source, and if a transformer or +// a patch owns that field the render will not reproduce it — which the render precondition +// (VerifyBatchRenders) catches, and refuses. So the two halves compose: this file makes an +// in-sync folder a no-op, and the oracle adjudicates everything else. +// +// See docs/design/support-boundary/render-root-scoping.md §6 and render-attribution.md §5. + +// node is one field of a document, and whether it is there at all. Absence is a value here: +// "the build injected this key and the source does not carry it" is the most common thing this +// file has to say, and it can only be said if absence travels with the node. +type node struct { + value interface{} + present bool +} + +// absent is the zero node: the field is not in this view of the document. +func absent() node { return node{} } + +// SourceFormRefusedError is the projection giving up rather than guessing. The build and the +// live object BOTH changed one list, and the source's elements cannot be paired with the +// render's, so there is no way to say which of the source's bytes the user meant to keep. +// +// Refusing is the only safe answer. Guessing an alignment writes one element's fields into +// another, and index alignment is not a safe guess: kustomize's strategic merge PREPENDS a +// container a patch adds (measured — the render is [sidecar, app] over a source of [app]), so +// source[0] is not render[0] in the one case where it matters most. +type SourceFormRefusedError struct { + // Field is the path of the list that could not be aligned, as a user would read it. + Field string +} + +func (e *SourceFormRefusedError) Error() string { + return "cannot place the edit: the build and the live object both changed " + e.Field + + ", and its elements carry no unique name: to pair the source with the render by" +} + +// sourceForm computes the document the source file should hold: src, carrying the user's +// changes, never the build's. +// +// src is the document as Git holds it, rendered is what kustomize renders THAT document to, and +// live is the sanitized live object. All three describe one object, and every value in the +// result comes from src or from live — never from rendered, which is an output and must not +// become an input. +// +// A result that is not a document at all — which would mean the live object is not a mapping — +// falls back to live: there is nothing to restore, and today's write-through is what the rest of +// the writer already expects. +func sourceForm(src, rendered, live map[string]interface{}) (map[string]interface{}, error) { + out, err := restore( + node{value: src, present: src != nil}, + node{value: rendered, present: rendered != nil}, + node{value: live, present: live != nil}, + "", + ) + if err != nil { + return nil, err + } + if !out.present { + return live, nil + } + obj, isDocument := deepCopyValue(out.value).(map[string]interface{}) + if !isDocument { + return live, nil + } + return obj, nil +} + +// restore is the rule, applied to one field. +func restore(src, rendered, live node, field string) (node, error) { + switch { + case equalNode(live, rendered): + // The build already produces what the cluster runs. Whatever the source says here IS + // what produced it, so the source keeps its bytes — or its ABSENCE, which is how an + // injected label stays out of a file that never declared one. + return src, nil + case equalNode(src, rendered): + // The build does not touch this field, so there is nothing standing between the source + // and the cluster: the live value is the user's, and it is written through. This is + // also what keeps the change a no-op for every document no transformer rewrites. + return live, nil + } + + // Both moved it. Decompose, so that a build-supplied field sitting inside a subtree the + // user did change is still left to the source — the ordinary case of an image bump on a + // container whose resources a patch pins. + if renderedMap, ok := asMap(rendered); ok { + if liveMap, isMap := asMap(live); isMap { + if srcMap, srcIsMap := asMap(src); srcIsMap || !src.present { + return restoreMap(srcMap, renderedMap, liveMap, field) + } + } + } + if renderedList, ok := asList(rendered); ok { + if liveList, isList := asList(live); isList { + if srcList, srcIsList := asList(src); srcIsList || !src.present { + return restoreList(srcList, renderedList, liveList, field) + } + } + } + + // A scalar the build and the user both wrote, or a field whose very shape changed. The + // user's value goes in, and if the build owns the field the render will not reproduce it — + // the render precondition refuses the flush and names the file. A guess here would be a + // silent non-converging write; a write-through is a reported refusal. + return live, nil +} + +// restoreMap applies the rule key by key, over every key any of the three views carries. +func restoreMap(src, rendered, live map[string]interface{}, field string) (node, error) { + out := make(map[string]interface{}, len(live)) + for _, key := range unionKeys3(src, rendered, live) { + got, err := restore(at(src, key), at(rendered, key), at(live, key), join(field, key)) + if err != nil { + return absent(), err + } + if got.present { + out[key] = deepCopyValue(got.value) + } + } + return node{value: out, present: true}, nil +} + +// restoreList applies the rule element by element, pairing the three lists BY NAME. +// +// Name is the only pairing available, and it has to be verified rather than assumed: kustomize +// prepends a container a patch adds, so the source's element 0 is not the render's element 0. +// Where any of the three lists is not a set of uniquely-named maps, nothing is paired and the +// edit is refused (SourceFormRefusedError) — never aligned by position. +// +// The source's ORDER is the file's order, so it is the output's order: an element the user adds +// is appended, and an element the BUILD adds is dropped (its source node is absent, and the rule +// returns absence). +func restoreList(src, rendered, live []interface{}, field string) (node, error) { + srcByName, srcNamed := byName(src) + renderedByName, renderedNamed := byName(rendered) + liveByName, liveNamed := byName(live) + if !srcNamed || !renderedNamed || !liveNamed { + return absent(), &SourceFormRefusedError{Field: field} + } + + out := make([]interface{}, 0, len(live)) + emit := func(name string, srcNode node) error { + got, err := restore(srcNode, named(renderedByName, name), named(liveByName, name), join(field, name)) + if err != nil { + return err + } + if got.present { + out = append(out, deepCopyValue(got.value)) + } + return nil + } + + for _, element := range src { + if err := emit(nameOf(element), node{value: element, present: true}); err != nil { + return absent(), err + } + } + for _, element := range live { + if name := nameOf(element); !hasName(srcByName, name) { + if err := emit(name, absent()); err != nil { + return absent(), err + } + } + } + return node{value: out, present: true}, nil +} + +// byName indexes a list by its elements' name: field, reporting false unless EVERY element is a +// map carrying a non-empty, unique name. A list that fails this cannot be paired across the +// three views, and the edit is refused rather than aligned by position. +func byName(list []interface{}) (map[string]interface{}, bool) { + out := make(map[string]interface{}, len(list)) + for _, element := range list { + name := nameOf(element) + if name == "" { + return nil, false + } + if _, duplicate := out[name]; duplicate { + return nil, false + } + out[name] = element + } + return out, true +} + +func nameOf(element interface{}) string { + m, isMap := element.(map[string]interface{}) + if !isMap { + return "" + } + name, _ := m["name"].(string) + return name +} + +func named(byName map[string]interface{}, name string) node { + value, found := byName[name] + return node{value: value, present: found} +} + +func hasName(byName map[string]interface{}, name string) bool { + _, found := byName[name] + return found +} + +func at(m map[string]interface{}, key string) node { + if m == nil { + return absent() + } + value, found := m[key] + return node{value: value, present: found} +} + +func asMap(n node) (map[string]interface{}, bool) { + if !n.present { + return nil, false + } + m, ok := n.value.(map[string]interface{}) + return m, ok +} + +func asList(n node) ([]interface{}, bool) { + if !n.present { + return nil, false + } + l, ok := n.value.([]interface{}) + return l, ok +} + +// equalNode compares two views of one field. Absence is a value: two absences are equal, and an +// absence never equals a present field. +// +// Canonical JSON, not reflect.DeepEqual, and that is a landmine rather than a preference: the +// three views carry a number in three different Go types. kustomize hands back `int`, the API +// machinery uses `int64`, and sigs.k8s.io/yaml decodes the source file into `float64` — so +// DeepEqual calls an unchanged replica count changed, and every source file in the corpus would +// be rewritten by a projection that believed the user had scaled it. +func equalNode(a, b node) bool { + if a.present != b.present { + return false + } + if !a.present { + return true + } + left, err := json.Marshal(a.value) + if err != nil { + return false + } + right, err := json.Marshal(b.value) + if err != nil { + return false + } + return bytes.Equal(left, right) +} + +// unionKeys3 is every key any of the three views carries, sorted, so the walk is deterministic. +func unionKeys3(a, b, c map[string]interface{}) []string { + seen := make(map[string]struct{}, len(a)+len(b)+len(c)) + for _, m := range []map[string]interface{}{a, b, c} { + for key := range m { + seen[key] = struct{}{} + } + } + out := make([]string, 0, len(seen)) + for key := range seen { + out = append(out, key) + } + sort.Strings(out) + return out +} + +// deepCopyValue copies a node out of the source document or the live object, so the result +// shares no structure with either and the caller may edit it freely (the image slots are set on +// it straight afterwards). +// +// It is only ever handed a value from src or live, never from the render — which is the whole +// point of the rule, and also why it is safe: a RENDERED object is not valid JSON-typed +// unstructured (kustomize's `int` makes runtime.DeepCopyJSONValue panic outright), while src +// and live both come out of a JSON decoder. +func deepCopyValue(value interface{}) interface{} { + return runtime.DeepCopyJSONValue(value) +} + +// join builds the field path a refusal names, as a user would read it. +func join(field, key string) string { + if field == "" { + return key + } + return field + "." + key +} diff --git a/internal/manifestanalyzer/source_form_test.go b/internal/manifestanalyzer/source_form_test.go new file mode 100644 index 00000000..24ec5d48 --- /dev/null +++ b/internal/manifestanalyzer/source_form_test.go @@ -0,0 +1,303 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/yaml" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" +) + +// The source form, driven by ground truth: build a real tree, render it with kustomize, and hand +// the render back as the live object. The folder is then converged BY CONSTRUCTION, so anything +// the projection wants to write into the source file is a phantom — a value the build produced, +// being mirrored back into the build's own input. +// +// That is not a hypothetical failure. Two of these fixtures fail on the code as it stood: one +// with a feature we ACCEPT today (labels/commonAnnotations), one with the patches this workstream +// is about. They are the same bug. + +// sourceDoc is the one document these fixtures manage: the base the overlay renders. +const sourceDoc = "deployment.yaml" + +// renderedAsLive is the whole trick: kustomize's own output, in the shape a live object has. The +// folder is then converged by construction, and anything the projection writes is a phantom. +func renderedAsLive(t *testing.T, files []manifestedit.FileContent) map[string]interface{} { + t.Helper() + rendered, err := renderRoot(files, ".") + require.NoError(t, err, "the fixture must be a folder kustomize can build") + for _, ro := range rendered { + if ro.OriginPath == sourceDoc { + return asLiveObject(t, ro.Object).Object + } + } + t.Fatalf("kustomize rendered nothing from %s", sourceDoc) + return nil +} + +// requireSourceUntouched is the no-op claim: the projected document IS the source document. +func requireSourceUntouched(t *testing.T, files []manifestedit.FileContent, out *unstructured.Unstructured) { + t.Helper() + var src map[string]interface{} + require.NoError(t, yaml.Unmarshal(contentOf(t, files, sourceDoc), &src)) + require.Equal(t, normaliseForCompare(t, src), normaliseForCompare(t, out.Object), + "an in-sync folder must hand the source document back byte-for-byte") +} + +// THE SHIPPED BUG. labels: and commonAnnotations: are on the supported list today, and they inject +// metadata into every rendered object. Mirroring the live object straight back wrote that metadata +// into the source file, as if the author had typed it — measured, on an accepted folder, with +// nothing changed in the cluster and nothing changed in the render. +func TestSourceForm_InjectedMetadataStaysOutOfTheSource(t *testing.T) { + files := []manifestedit.FileContent{ + file(sourceDoc, deploymentSource("ghcr.io/example/app:1.0.0", "1")), + file("kustomization.yaml", `resources: + - deployment.yaml +labels: + - pairs: + env: prod + includeSelectors: false +commonAnnotations: + owner: platform +images: + - name: ghcr.io/example/app + newTag: 2.0.0 +`), + } + out, edits := splitFixture(t, files, sourceDoc, + renderedAsLive(t, files)) + + require.Empty(t, edits) + requireSourceUntouched(t, files, out) + require.NotContains(t, out.GetLabels(), "env", "the overlay's label is the BUILD's, not the file's") + require.NotContains(t, out.GetAnnotations(), "owner") +} + +// The same bug with a patch instead of a label, which is the one that matters: the value the +// overlay pins is one ENVIRONMENT's, and baking it into the base rewrites what every other +// environment starts from. The render is identical afterwards, so no re-render can catch it — +// only refusing to write it can. +func TestSourceForm_PatchedFieldStaysOutOfTheSource(t *testing.T) { + files := patchedFolder() + out, edits := splitFixture(t, files, sourceDoc, + renderedAsLive(t, files)) + + require.Empty(t, edits) + requireSourceUntouched(t, files, out) + require.Equal(t, "50m", cpuRequestOf(t, out.Object), "the patch's 200m is the overlay's value") +} + +// A field no transformer and no patch touches is the user's, and it is written through exactly as +// before. This is the half of the rule that keeps the change a no-op for every ordinary document. +func TestSourceForm_UngovernedFieldIsWrittenThrough(t *testing.T) { + files := patchedFolder() + live := renderedAsLive(t, files) + setNested(t, live, int64(9080), "spec", "template", "spec", "containers", "0", "ports", "0", "containerPort") + + out, edits := splitFixture(t, files, sourceDoc, live) + + require.Empty(t, edits) + require.Equal(t, int64(9080), nestedOf(t, out.Object, + "spec", "template", "spec", "containers", "0", "ports", "0", "containerPort"), + "a port nothing in the build touches is the user's edit, and belongs in the file") + require.Equal(t, "50m", cpuRequestOf(t, out.Object), + "and the patched CPU is still the overlay's, in the same container the user did edit") +} + +// The composition that has to work, and the reason the rule cannot stop at whole subtrees: the +// user bumps an image that an images: ENTRY governs, inside a container whose CPU a PATCH governs. +// The tag goes to the entry, the CPU stays in the overlay, and the source file keeps every byte. +func TestSourceForm_ImageBumpRoutesAndLeavesThePatchedFieldAlone(t *testing.T) { + files := patchedFolder() + live := renderedAsLive(t, files) + setNested(t, live, "ghcr.io/example/app:3.0.0", "spec", "template", "spec", "containers", "0", "image") + + out, edits := splitFixture(t, files, sourceDoc, live) + + require.Len(t, edits, 1, "the tag is supplied by the entry, so the edit belongs on the entry") + require.Equal(t, "3.0.0", edits[0].Edit.Value) + require.Equal(t, fieldNewTag, edits[0].Edit.Field) + requireSourceUntouched(t, files, out) +} + +// A container the PATCH adds is not the source's, and it must not be written into it. This is +// where aligning the source with the render by POSITION would corrupt the file rather than merely +// leak into it: kustomize PREPENDS the added container, so the source's element 0 is the render's +// element 1 (measured). +func TestSourceForm_BuildAddedContainerIsNotBakedIntoTheSource(t *testing.T) { + files := []manifestedit.FileContent{ + file(sourceDoc, deploymentSource("ghcr.io/example/app:1.0.0", "1")), + file("patch.yaml", `apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + template: + spec: + containers: + - name: sidecar + image: ghcr.io/example/sidecar:1.0.0 +`), + file("kustomization.yaml", `resources: + - deployment.yaml +patches: + - path: patch.yaml +images: + - name: ghcr.io/example/app + newTag: 2.0.0 +`), + } + live := renderedAsLive(t, files) + setNested(t, live, "ghcr.io/example/app:3.0.0", "spec", "template", "spec", "containers", "1", "image") + + out, edits := splitFixture(t, files, sourceDoc, live) + + require.Len(t, edits, 1, "the app's tag still routes to its entry") + require.Equal(t, "3.0.0", edits[0].Edit.Value) + requireSourceUntouched(t, files, out) + containers := nestedOf(t, out.Object, "spec", "template", "spec", "containers") + require.Len(t, containers, 1, "the sidecar belongs to the overlay; the base never declared it") +} + +// And where the pairing cannot be made, the edit is REFUSED rather than guessed. args: is a list +// of scalars: if the build rewrote it and the user rewrote it too, there is no honest way to say +// which of the source's elements survive. +func TestSourceForm_UnpairableListRefusesTheEdit(t *testing.T) { + files := []manifestedit.FileContent{ + file(sourceDoc, `apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + template: + spec: + containers: + - name: app + image: ghcr.io/example/app:1.0.0 + args: ["--from-the-base"] +`), + file("patch.yaml", `apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + template: + spec: + containers: + - name: app + args: ["--from-the-patch"] +`), + file("kustomization.yaml", "resources:\n - deployment.yaml\npatches:\n - path: patch.yaml\n"), + } + live := renderedAsLive(t, files) + setNested(t, live, []interface{}{"--the-user-changed-it"}, + "spec", "template", "spec", "containers", "0", "args") + + var gitRaw map[string]interface{} + require.NoError(t, yaml.Unmarshal(contentOf(t, files, sourceDoc), &gitRaw)) + chains, _ := renderChains(files, parseKustomizations(files)) + rendered := chains[chainKey{originPath: sourceDoc, kind: "Deployment", name: "web"}].rendered + + _, _, err := SplitDesiredForOverrides(gitRaw, &unstructured.Unstructured{Object: live}, rendered) + + var refused *SourceFormRefusedError + require.ErrorAs(t, err, &refused) + require.Contains(t, refused.Field, "args") +} + +// patchedFolder is the shape tolerating patches would newly accept: one root, one base document, +// one strategic-merge patch that pins a CPU request, and an images: entry over the top. +func patchedFolder() []manifestedit.FileContent { + return []manifestedit.FileContent{ + file(sourceDoc, `apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + replicas: 1 + template: + spec: + containers: + - name: app + image: ghcr.io/example/app:1.0.0 + ports: + - containerPort: 8080 + resources: + requests: + cpu: 50m +`), + file("patch.yaml", `apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + template: + spec: + containers: + - name: app + resources: + requests: + cpu: 200m +`), + file("kustomization.yaml", `resources: + - deployment.yaml +patches: + - path: patch.yaml +images: + - name: ghcr.io/example/app + newTag: 2.0.0 +`), + } +} + +func cpuRequestOf(t *testing.T, obj map[string]interface{}) string { + t.Helper() + value := nestedOf(t, obj, "spec", "template", "spec", "containers", "0", "resources", "requests", "cpu") + cpu, ok := value.(string) + require.True(t, ok, "cpu request is %T", value) + return cpu +} + +// nestedOf walks maps and lists alike: a numeric path element indexes a list. +func nestedOf(t *testing.T, obj interface{}, path ...string) interface{} { + t.Helper() + current := obj + for _, step := range path { + switch node := current.(type) { + case map[string]interface{}: + current = node[step] + case []interface{}: + i := listIndex(t, step) + require.Less(t, i, len(node), "index %s is past the end of the list", step) + current = node[i] + default: + t.Fatalf("cannot walk %q into a %T", step, current) + } + } + return current +} + +func setNested(t *testing.T, obj map[string]interface{}, value interface{}, path ...string) { + t.Helper() + parent := nestedOf(t, obj, path[:len(path)-1]...) + last := path[len(path)-1] + switch node := parent.(type) { + case map[string]interface{}: + node[last] = value + case []interface{}: + node[listIndex(t, last)] = value + default: + t.Fatalf("cannot set %q on a %T", last, parent) + } +} + +func listIndex(t *testing.T, step string) int { + t.Helper() + require.Len(t, step, 1, "list index %q must be a single digit in these fixtures", step) + require.True(t, step[0] >= '0' && step[0] <= '9', "list index %q is not a digit", step) + return int(step[0] - '0') +} diff --git a/internal/watch/event_router.go b/internal/watch/event_router.go index 088c9764..33558661 100644 --- a/internal/watch/event_router.go +++ b/internal/watch/event_router.go @@ -270,9 +270,10 @@ func (r *EventRouter) handleScopedResyncError( // - purely the .gittargetignore-shadows-a-write case (§4.3) — the unrecoverable footgun — // gets IgnoreShadowsManagedPath; // - purely write-boundary violations (a planned write escaping spec.path, an in-place edit -// of a file more than one render root reaches, or a write kustomize will not vouch for -// when the folder is re-rendered with it applied) gets WriteBoundaryRefused: the folder -// content is fine, the *edit* had nowhere safe to land. +// of a file more than one render root reaches, a write kustomize will not vouch for when the +// folder is re-rendered with it applied, or an edit the projection could not place in the +// source document) gets WriteBoundaryRefused: the folder content is fine, the *edit* had +// nowhere safe to land. // // Any other refusal, and any mix of shapes, keeps the umbrella UnsupportedContent. The strings // mirror the controller's GitTargetReason* constants (the watch package cannot import @@ -286,6 +287,7 @@ func gitPathRefusalReason(refused *manifestanalyzer.AcceptanceRefusedError) stri manifestanalyzer.IssueWriteEscapesScope, manifestanalyzer.IssueWriteFanIn, manifestanalyzer.IssueRenderRefused, + manifestanalyzer.IssueUnplaceableEdit, ): return "WriteBoundaryRefused" default: From a841de8800f1702888d65c40386d42539b30190f Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 15 Jul 2026 04:47:24 +0000 Subject: [PATCH 2/5] docs(kustomize): address review on #234 -- finish the refusal sentence, fix the vars contradiction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SourceFormRefusedError message (surfaced to the user via WriteBoundaryRefused) and its IssueUnplaceableEdit doc comment both cut off mid-clause; complete them. And §2's vars bullet claimed the folder is accepted today while a parenthetical said it now refuses -- rewrite the passage in past tense (both leaks it cites are now closed, by #229 and by sourceForm). --- .../support-boundary/render-root-scoping.md | 29 ++++++++++--------- internal/manifestanalyzer/acceptance.go | 4 +-- internal/manifestanalyzer/source_form.go | 3 +- 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/docs/design/support-boundary/render-root-scoping.md b/docs/design/support-boundary/render-root-scoping.md index 2fcba9b1..1d93d0a2 100644 --- a/docs/design/support-boundary/render-root-scoping.md +++ b/docs/design/support-boundary/render-root-scoping.md @@ -85,20 +85,21 @@ reimplementation** — a list of everything we chose not to re-derive. That is w [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**. - (`vars` was moved off the tolerated set by #229 and now refuses the folder.) -- **`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. +The fence had a cost we were already paying, and it was not the cost we thought (both examples +are now closed — see the note below — but they are what the argument rests on): + +- **`vars` was not on the deny-list.** A source document containing `$(SOME_VAR)` renders to a + substituted value, and mirroring that live object wrote the *substituted* value over the + `$(VAR)` in the source: silent corruption, in a folder we accepted. #229 moved `vars` off the + tolerated set, so it now refuses the folder. +- **`labels` / `commonLabels` / `annotations` were classed as benign.** They inject metadata + into every rendered object, and mirroring baked it into the source file as drift — the + metadata-transformer leak, live in supported folders until `sourceForm` (below) closed it. + +So the inversion problem is not something overlay support introduces. **We already had it, and +handled 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. diff --git a/internal/manifestanalyzer/acceptance.go b/internal/manifestanalyzer/acceptance.go index 6f8ee4e8..d4b4c404 100644 --- a/internal/manifestanalyzer/acceptance.go +++ b/internal/manifestanalyzer/acceptance.go @@ -135,8 +135,8 @@ const ( // which is the exact failure this whole path exists to prevent. IssueRenderRefused IssueKind = "kustomize-render-refused" // IssueUnplaceableEdit marks a live change the projection could not place in the source - // document: the BUILD and the USER both rewrote one list, and its elements carry no unique - // name: to pair the source's with the render's by (SourceFormRefusedError). + // document: the BUILD and the USER both rewrote one list whose elements carry no unique + // name to pair the source's with the render's by (see SourceFormRefusedError). // // The alternative to refusing is aligning the two lists by position, and that is not a // conservative guess — it is measurably wrong: kustomize's strategic merge PREPENDS a diff --git a/internal/manifestanalyzer/source_form.go b/internal/manifestanalyzer/source_form.go index 5524ab96..dda21c4e 100644 --- a/internal/manifestanalyzer/source_form.go +++ b/internal/manifestanalyzer/source_form.go @@ -68,7 +68,8 @@ type SourceFormRefusedError struct { func (e *SourceFormRefusedError) Error() string { return "cannot place the edit: the build and the live object both changed " + e.Field + - ", and its elements carry no unique name: to pair the source with the render by" + ", whose elements carry no unique name to pair the source with the render by, " + + "so pairing them by position could place one element's edit onto another" } // sourceForm computes the document the source file should hold: src, carrying the user's From d4b3332f3c5779052d81d7ce3be43d9e57b337c2 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Wed, 15 Jul 2026 07:59:52 +0000 Subject: [PATCH 3/5] docs(support-boundary): design work alongside the render-fidelity implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bring the support-boundary design docs onto this PR — they are the design that sits next to this real implementation, so they belong together rather than in a separate PR: render-fidelity.md (our render is not the orchestrator's; the render-vs-live fence, the blocking RenderFaithful condition, and where to implement it), admission-consent.md, orchestrator-reconcile-trigger.md, and the kpt-and-krm-functions orientation note, plus their README index rows. Consolidated from PR #237, now closed. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/design/support-boundary/README.md | 3 +- .../support-boundary/admission-consent.md | 211 ++++++++++ .../support-boundary/kpt-and-krm-functions.md | 331 ++++++++++++++++ .../orchestrator-reconcile-trigger.md | 250 ++++++++++++ .../support-boundary/render-fidelity.md | 360 ++++++++++++++++++ 5 files changed, 1154 insertions(+), 1 deletion(-) create mode 100644 docs/design/support-boundary/admission-consent.md create mode 100644 docs/design/support-boundary/kpt-and-krm-functions.md create mode 100644 docs/design/support-boundary/orchestrator-reconcile-trigger.md create mode 100644 docs/design/support-boundary/render-fidelity.md diff --git a/docs/design/support-boundary/README.md b/docs/design/support-boundary/README.md index 5c08547e..abecb1b0 100644 --- a/docs/design/support-boundary/README.md +++ b/docs/design/support-boundary/README.md @@ -13,9 +13,10 @@ | Topic | Docs | |---|---| | **The boundary** | [support-contract.md](support-contract.md) — the single statement · [kustomize-support-boundary.md](kustomize-support-boundary.md) — field taxonomy + layout allowlist · [gittarget-granularity-and-cross-environment-edits.md](gittarget-granularity-and-cross-environment-edits.md) — **the write boundary; the one home of fan-in = 1** | +| **Renderers & provenance** | [render-attribution.md](render-attribution.md) — attribution and verification · [render-root-scoping.md](render-root-scoping.md) — render roots and the oracle · [render-fidelity.md](render-fidelity.md) — **our render is not the orchestrator's; refuse where they diverge** · [kpt-and-krm-functions.md](kpt-and-krm-functions.md) — how Kpt packages, setters, and KRM functions may fit safely | | **Orchestrators & expansion** | [orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md) — renderability vs ownership; claims about paths · [expansion-boundary-and-corpus-organisation.md](expansion-boundary-and-corpus-organisation.md) — provenance; ApplicationSet vs ResourceSet; Helm · [`../../facts/expansion-provenance-markers.md`](../../facts/expansion-provenance-markers.md) — **the measured markers** · [argocd-bi-directional.md](argocd-bi-directional.md) — why `selfHeal` is incompatible | | **Documents & secrets** | [resource-capability-model.md](resource-capability-model.md) — what may I do to this document · [write-only-encrypted-secrets.md](write-only-encrypted-secrets.md) — SOPS · [sealed-secrets-and-external-secrets.md](sealed-secrets-and-external-secrets.md) | -| **Edits with no home** | [unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md) | +| **Edits with no home** | [unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md) — tier-1/2/3 accounting · [admission-consent.md](admission-consent.md) — say yes to a blast-radius refusal · [orchestrator-reconcile-trigger.md](orchestrator-reconcile-trigger.md) — revert a refusal / order around origin drift | | **Discovery (read-only)** | [repo-discovery-and-onboarding-scan.md](repo-discovery-and-onboarding-scan.md) | | **Shipped** | [finished/images-and-replicas-edit-through.md](finished/images-and-replicas-edit-through.md) · [finished/higher-level-krm-documents.md](finished/higher-level-krm-documents.md) | | **Evidence** | [`test/fixtures/gitops-layouts/`](../../../test/fixtures/gitops-layouts/) — the corpus of real-world repo shapes, and the generated behavioural baseline beside it | diff --git a/docs/design/support-boundary/admission-consent.md b/docs/design/support-boundary/admission-consent.md new file mode 100644 index 00000000..081f3c75 --- /dev/null +++ b/docs/design/support-boundary/admission-consent.md @@ -0,0 +1,211 @@ +# Admission consent: a blast-radius refusal you can say yes to + +> **design** — direction-setting; ships no code. Nothing it describes is supported today. +> Captured: 2026-07-15 +> Related: +> [README.md](README.md), +> [unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md) — **the tier-1/2/3 model this extends; tier 3 is the admission gate**, +> [gittarget-granularity-and-cross-environment-edits.md](gittarget-granularity-and-cross-environment-edits.md) — the write boundary; fan-in = 1; base read-only by L1, +> [render-attribution.md](render-attribution.md) §5 — attribution may be heuristic, verification may not, +> [orchestrator-reconcile-trigger.md](orchestrator-reconcile-trigger.md) — **the sibling half: what reverts a refusal that lands anyway**, +> [support-contract.md](support-contract.md) + +This is one half of a two-part design. This half is about turning a *refusal* into a *yes* at the +moment of the edit; the other half — [the reconcile trigger](orchestrator-reconcile-trigger.md) — is +about making the *outcome* of a "no" prompt and visible. They compose, but they are separate topics +and separate documents. + +Today a write the operator cannot place is refused, correctly, but the refusal is **binary and +mute**. The user who edits a base shared by prod and staging is told "no" — at flush time, on a +`GitTarget` condition they may not be watching — with no way to say *"yes, I know it changes both; +that is what I meant."* Some refusals should never be lift­able. But that one should, and this +document is about the line between them. + +--- + +## 1. The refusal is right, but there is no "yes" + +An edit to a shared base is refused because fan-in = 1 makes shared context read-only +([gittarget-granularity-and-cross-environment-edits.md](gittarget-granularity-and-cross-environment-edits.md)): +changing one file would change what more than one environment renders. That rule exists because the +user *usually* did not mean to change every environment at once. + +But sometimes they did. "Bump the base image for all environments" is an ordinary, legitimate +intent. Today it has no expression: the operator refuses it exactly as it refuses an accidental +cross-environment edit, because it cannot tell the two apart. The missing thing is not a weaker +rule — it is a way for the user to **declare which one this is.** + +That declaration has to happen where the user is, and synchronously, which is why this rides the +**tier-3 admission gate** already designed in +[unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md) (opt-in per +GitTarget, `failurePolicy: Ignore`, `--dry-run=server` preflight). Tier 3 as written only *rejects* +an unsavable write. This adds the "yes." + +--- + +## 2. Two kinds of "no", and only one is negotiable + +Everything turns on splitting refusals into two piles, because consent is safe for exactly one. + +```mermaid +flowchart TD + N["the operator says NO"] --> Q{which kind of no?} + Q -->|"the render does not reproduce
what you asked, or moves an
object you never touched"| C["CORRECTNESS
consent cannot lift it —
no annotation changes physics"] + Q -->|"the edit does exactly what you asked,
but the file is a base shared by
more than one environment (fan-in > 1)"| P["POLICY / blast radius
consent CAN lift it —
'yes, change every environment'"] + classDef bad fill:#fdd,stroke:#c33,color:#111 + classDef good fill:#dfd,stroke:#3a3,color:#111 + class C bad + class P good +``` + +| | **Correctness refusal** | **Policy refusal** | +|---|---|---| +| The operator is saying | "this does not reproduce what you asked, or corrupts an object you did not touch" | "this does what you asked, but the blast radius is bigger than you may realize" | +| Example | the field is owned by a patch/transformer; the render does not converge; an unpairable list | the file is a **base shared by prod and staging**; the edit changes *both* | +| Decided by | the render oracle (`VerifyBatchRenders`) | the write-boundary policy (L2 fan-in = 1) | +| Consent lifts it? | **Never.** | **Yes.** | + +The correctness pile is the render oracle, and it is absolute: a dyed render plus a real re-render +either reproduce the live object and disturb nothing unintended, or they do not +([render-attribution.md §5](render-attribution.md)). No annotation makes a non-converging write +converge. + +The policy pile is different — it is refused for the user's protection, not for physics. That is the +pile consent unlocks. + +--- + +## 3. Consent is declared intent, not a bypass + +The tempting model is a `force: true` that skips a check. That is exactly wrong, and the correct +model is already in the code. + +The oracle ([`render_verify.go`](../../../internal/manifestanalyzer/render_verify.go)) checks two +things against a set of **`WriteIntent`s**: every intended document renders to its live object, and +**every object the batch did *not* declare an intent for comes out byte-identical**. A fan-in +refusal is really the second clause firing: editing the shared base also moves `Deployment/web` in +staging, staging was never an intent, so the write "moves something it never set out to write" and +is refused. + +**Consent adds the sibling object as an intent.** When the user acknowledges "this changes prod and +staging," the operator promotes staging's object from the *must-be-untouched* set into the +*intended-to-change* set, and the **same oracle** runs, unchanged, over the larger intent — and it +still has to pass: the base edit must render to the acknowledged live state in *both* environments +and disturb nothing else. + +```mermaid +flowchart LR + E["edit a shared base"] --> O{"oracle: does anything
UNINTENDED move?"} + O -->|"staging/web moves,
and it is not an intent"| R["REFUSE — fan-in"] + R -->|"user acknowledges
the named consequence"| I["staging/web becomes
a DECLARED intent"] + I --> O2{"oracle: does the base edit render
to live in BOTH, and disturb
nothing else?"} + O2 -->|yes| W["write the base"] + O2 -->|no| R2["REFUSE — correctness
(consent cannot lift this)"] + + classDef bad fill:#fdd,stroke:#c33,color:#111 + classDef good fill:#dfd,stroke:#3a3,color:#111 + class R,R2 bad + class W,I good +``` + +So consent does not remove a check. It **re-labels collateral as intent**, and the correctness gate +verifies the whole expanded intent exactly as before. There is no second code path, no bypass, and +no way for consent to wave through a write that does not actually converge. + +### The token is scoped to a consequence, not to "off" + +The acknowledgement must name *what* is being consented to, so it can never become a standing "ignore +safety" flag: + +- The operator computes the consequence — the concrete set of `(object, environment)` pairs the + write would move — and reduces it to a **content hash**. +- Consent carries that hash. The operator honours it only when the *current* computed consequence + hashes to the same value. Change the base differently, or let the tree drift, and the old + acknowledgement no longer matches → the write is refused again, naming the *new* consequence to + acknowledge. + +This is the same discipline the dye uses for attribution: consent to a **named, verified** fact, not +to a mood. + +### Where the token lives + +Two surfaces, both already "near the actor" (the write-gating doc's fourth principle) and both +already understood by the pipeline: + +- **An annotation on the edited object** (`configbutler.ai/acknowledge-consequence: `) travels + with the `kubectl apply`, so consent is expressed in the same breath as the edit. It must be + **stripped before mirroring**, exactly as [`sanitize`](../../../internal/sanitize/types.go) already + strips orchestrator bookkeeping keys — a consent token is operator control data, never content + that belongs in Git. +- **A field on the `CommitRequest`** — the object a caller already polls for `Pushed` + `status.sha` + — is the natural home for *session-scoped* consent ("everything in this save window may touch the + base"), and it composes with the `FullyReflected` condition tier 2 puts there. + +### The edge consent must not cross: authorization + +Consent lifts *"did you realize,"* never *"are you allowed."* A base shared across environments in +**one** GitTarget's authorization scope is fine — the acking user already owns all of it. But a base +shared across **different** GitTargets (different RBAC, different tenants) is the case +[gittarget-granularity-and-cross-environment-edits.md](gittarget-granularity-and-cross-environment-edits.md) +forecloses on purpose: a user who can edit prod must not change staging by editing the base when they +may have no rights to staging. Consent from the prod editor cannot manufacture authority over +staging. So consent unlocks a shared-base edit **only within a single authorization scope**; across +scopes it stays refused, and the route remains Option C (base-as-variant with its own GitTarget and +RBAC). This boundary is the same class of "no" as correctness — not lift­able by annotation. + +--- + +## 4. The admission surface + +Consent needs a synchronous surface, and admission is the only one: it is the sole point where the +user's intent can be accepted or rejected **whole, before persistence** — the property the +write-gating doc names as the real argument for the gate. The infrastructure exists (the operator +already runs admission webhooks, +[`validate_operator_types_handler.go`](../../../internal/webhook/validate_operator_types_handler.go)), +and tier 3 already specifies it. Consent adds one branch: + +```mermaid +flowchart TD + A["kubectl apply (edit)"] --> W{"admission: single-object
source-form + oracle preview"} + W -->|"clean"| OK["allow — writes normally"] + W -->|"correctness refusal"| DENY["deny: name the field
(no token offered)"] + W -->|"policy refusal,
no matching ack"| ASK["deny: name the consequence
+ its ack token"] + W -->|"policy refusal,
ack matches"| OKC["allow — record the ack
as a declared intent"] + ASK -.->|"user re-applies
with the token"| W + + classDef bad fill:#fdd,stroke:#c33,color:#111 + classDef good fill:#dfd,stroke:#3a3,color:#111 + class DENY,ASK bad + class OK,OKC good +``` + +Two honest limits, both already answered by the tier-3 design: + +- **Admission sees one request; the oracle needs the batch.** The webhook can only run a + *single-object preview* of the oracle (project this one object to source form, re-render, read the + blast radius). It can be wrong — staleness, or cross-object batch effects it cannot see. That is + tolerable because the gate is **fail-open and advisory**: a wrong *deny* is a retry, and a wrong + *allow* is caught at flush. +- **Consent granted at admission does not bind the flush.** This is the load-bearing safety line. The + flush-time oracle re-verifies the *whole batch* against the declared intents — including the + consented ones — and still refuses if the consented set does not actually converge. So a stale or + mistaken admission-time "yes" cannot cause a bad write. Worst case it lands, the flush refuses it, + and the [reconcile trigger](orchestrator-reconcile-trigger.md) reverts it — the same safety net + that catches an edit made while the gate was disabled entirely. + +--- + +## 5. Open questions + +- **Surface and granularity.** Object annotation (per-edit, stripped like the sanitize deny-list) + vs. `CommitRequest` field (per-window, already polled) vs. both. Per-consequence-hash scoping is + the recommendation; a per-GitTarget "base edits allowed" mode is the blunt alternative, probably + too blunt. +- **Does consent-to-edit-a-base earn a place in the support contract?** It is a genuinely different + operation from per-environment patch authoring ("I mean the base" vs. "I mean this overlay"), and + [support-contract.md](support-contract.md) should say so explicitly rather than leave it implied by + the fan-in refusal being liftable. +- **What computes the consequence at admission time?** The single-object preview needs the same + source-form projection the writer uses, evaluated against a store snapshot. Its cost and staleness + are the tier-3 concerns; consent adds the requirement that the *hash* it produces is stable across + the preview and the eventual flush, or the token will spuriously fail to match. diff --git a/docs/design/support-boundary/kpt-and-krm-functions.md b/docs/design/support-boundary/kpt-and-krm-functions.md new file mode 100644 index 00000000..15a2a10e --- /dev/null +++ b/docs/design/support-boundary/kpt-and-krm-functions.md @@ -0,0 +1,331 @@ +# Kpt and KRM functions: a safe place in the reverse-GitOps model + +> **design** — an orientation note. Captured: 2026-07-14. +> +> Related: [render attribution](render-attribution.md), +> [render-root scoping](render-root-scoping.md), +> [the support contract](support-contract.md), and the +> `kustomize-tracer` plans (ConfigButler/kustomize-tracer, `plans/` — a local checkout under +> `external-sources/`, not tracked here). + +Kpt and KRM functions have a useful place in GitOps Reverser, but they do not make a +generic rendered object reversible. The product's central question remains: + +> Given a live field change, is there exactly one writable place in Git that can +> produce it again without changing anything else? + +Kustomize, Kpt, a CI hydration job, and an Argo or Flux controller are all possible +parts of the answer. They are not interchangeable. The renderer we use to attribute +and verify a write must be the renderer that produced the objects the delivery system +applies. + +## 1. The common model + +Every supported configuration system fits the same pipeline. The stages in green are +capabilities, not promises: an unfamiliar transform may stop at the red refusal. + +```mermaid +flowchart LR + G["Git intent\nfiles, Kustomization, Kptfile"] + R["actual renderer\nplain YAML / kustomize / kpt hydration"] + L["live KRM object\nuser changes a field"] + A["attribute governance\nsource field, override, setter, or unknown"] + P["propose the smallest\nsource edit"] + V["verify the whole\nrender scope"] + C["commit to Git"] + X["report refusal\nno write"] + + G --> R --> L --> A --> P --> V + V -->|target matches and\nother output is unchanged| C + A -->|no unique writable home| X + V -->|mismatch or blast radius| X + + classDef safe fill:#dfd,stroke:#3a3,color:#111 + classDef refuse fill:#fdd,stroke:#c33,color:#111 + class A,P,V,C safe + class X refuse +``` + +The current Kustomize work already implements the important safety half. A candidate +write is rendered against the exact post-write bytes; the intended object must match +the live object and every object outside the write batch must be unchanged. This is the +**oracle**. Attribution can improve over time, but it must never become the only proof +that a write is safe. + +This explains two rules that apply equally to Kustomize and Kpt: + +1. **Causation is not governance.** A transform that made no visible change can still + override a later source edit. An idempotent Kustomize `images:` entry and an + idempotent KRM function parameter are both "invisible and armed." +2. **A render result is not a source map.** It says what exists after rendering, not + which exact configuration field a user should edit to reproduce one value. + +## 2. Why Kustomize is the first renderer + +Kustomize is unusually tractable because its configuration language has named entries +such as `images[0]`, `replicas[0]`, and `patches[0]`. The tracer work observes the +before/after state around each transformer entry, so it can say both *which entry* and +*which field* changed. + +```yaml +# overlays/prod/kustomization.yaml +resources: + - ../../base + +images: + - name: ghcr.io/acme/web + newTag: "2.4.1" + +patches: + - path: production-tuning.yaml + target: + kind: Deployment + name: web +``` + +```yaml +# overlays/prod/production-tuning.yaml +apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + replicas: 3 +``` + +For `containers[name=web].image`, an `images[0]` event can route the edit to +`kustomization.yaml`. For `spec.replicas`, a patch may be the governing input. The +important result is not "always edit the patch"; Kustomize ordering can make a patch +dead text when a later `replicas:` entry wins. The renderer, rather than a hand-written +ordering model, decides that. + +The two complementary attribution techniques are: + +| Question | Technique | Result | +|---|---|---| +| What actually changed this field in this build? | Observer around each transformer entry | exact causal field events and lineage | +| Which configured input will win even when it agrees with its source? | Counterfactual dye | governance of safe, dyeable values | + +Neither is permission to write. Both feed the oracle, which checks the proposed edit over +the whole render root and its read scope. Fields changed by unobserved mechanisms—such +as generator hashes and reference rewrites—are **unattributable**, not source-owned by +default. + +## 3. What Kpt adds + +Kpt is package-centred rather than Kustomization-centred. A `Kptfile` can declare an +upstream package, package metadata, inventory, and a function pipeline. A function takes +KRM resources plus configuration, then emits KRM resources. This produces three useful +ideas for this project. + +### Packages are render roots + +A Kpt package can be treated as another candidate unit in repo discovery, much as a +Kustomize render root is today. Its read scope includes the package and any declared +inputs that its actual hydration process reads. Its write scope must still remain inside +the `GitTarget` path. + +This is only meaningful when Kpt hydration is in the delivery path. There are two +materially different repository shapes: + +```mermaid +flowchart TB + subgraph authoring["Kpt used only while authoring"] + D1["dry Kpt package"] --> CI["developer or CI: kpt fn render"] --> W["wet YAML committed for Flux/Argo"] + W --> F1["Flux or Argo applies YAML"] + end + + subgraph delivery["Kpt hydration is the delivery renderer"] + D2["dry Kpt package"] --> K["delivery job/controller: kpt fn render"] --> F2["apply rendered KRM"] + end + + classDef note fill:#fff4cc,stroke:#b8860b,color:#111 + class CI,K note +``` + +In the first shape, GitOps Reverser sees the wet YAML as its ordinary Git source; it +cannot safely reverse through an unobserved CI step into the dry package. In the second, +Kpt is part of the renderer and needs a dedicated renderer adapter, provenance rules, and +the same post-write verification as Kustomize. + +### Setters are an explicit inverse contract + +The most promising Kpt concept is the setter. It marks a resource value with the name of +the parameter that controls it. That is much better than inferring ownership from a final +rendered value. + +```yaml +# deployment.yaml +apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + replicas: 2 # kpt-set: ${web-replicas} + template: + spec: + containers: + - name: web + image: ghcr.io/acme/web:2.4.1 # kpt-set: ghcr.io/acme/web:${web-tag} +``` + +```yaml +# setters.yaml -- function configuration, not an applied workload +apiVersion: v1 +kind: ConfigMap +metadata: + name: setters + annotations: + config.kubernetes.io/local-config: "true" +data: + web-replicas: "2" + web-tag: "2.4.1" +``` + +```yaml +# Kptfile +apiVersion: kpt.dev/v1 +kind: Kptfile +pipeline: + mutators: + - image: ghcr.io/kptdev/krm-functions-catalog/apply-setters:v0.2 + configPath: setters.yaml +``` + +An edit to the live Deployment's replicas can propose changing +`setters.yaml:data.web-replicas`, rather than overwriting the annotated Deployment +field. This is an **existing-setter** capability only. Creating comments, creating a +setter configuration, or guessing that equal values share a parameter is authoring and +must be a separate future feature. + +A setter is shared context. If `web-tag` controls ten resources, changing it for one +resource is valid only when the intended batch includes all ten resulting changes. The +whole-render oracle makes this check mechanical. + +### Function pipelines are a renderer extension point, not an inverse API + +Kpt's pipeline has useful structure: ordered mutators, validators, per-function +configuration, and selectors. It does **not** turn an arbitrary function into a reversible +operation. The function can be a container image or an executable, and selectors can fan +one configuration value out to many resources. + +```yaml +# This is renderable only under a deliberately approved runner policy. +apiVersion: kpt.dev/v1 +kind: Kptfile +pipeline: + mutators: + - image: ghcr.io/example/organisation-policy@sha256:REPLACE_WITH_PINNED_DIGEST + configMap: + team: payments + selectors: + - kind: Deployment + labels: + app.kubernetes.io/part-of: checkout +``` + +Even if the output has `metadata.labels.team: payments`, that does not identify a +source-field write location, prove that the function is deterministic, or prove it did +not change another object. Treat this form as **renderable read-only context** until a +specific function contract is supported. + +## 4. Support levels for Kpt and KRM functions + +The right boundary is additive and per capability, rather than "Kpt supported" or +"Kpt refused." + +```mermaid +flowchart TD + A["Kptfile or KRM function found"] --> B{"Does this process\nproduce deployed objects?"} + B -->|No / unknown| R1["Do not reverse through it\nedit deployed Git source only"] + B -->|Yes| C{"Runner is pinned, local/sandboxed,\nand reproducible?"} + C -->|No| R2["Read-only or refuse\narbitrary execution/network"] + C -->|Yes| D{"Existing explicit setter\nor supported function contract?"} + D -->|Existing setter| E["Propose setter-config edit\nthen verify whole render"] + D -->|Known function, proven sinks| F["Counterfactual attribution\nroute only proven fields"] + D -->|No| R3["Renderable, but field is\nunroutable: report/refuse edit"] + + classDef safe fill:#dfd,stroke:#3a3,color:#111 + classDef refuse fill:#fdd,stroke:#c33,color:#111 + class E,F safe + class R1,R2,R3 refuse +``` + +| Level | Example | Operator behaviour | +|---|---|---| +| 0 — metadata only | `Kptfile` without a pipeline in a raw-YAML delivery repo | Ignore it as a deployment transform; preserve it as package metadata. | +| 1 — observable but unroutable | A known pipeline renders the object, but no explicit ownership exists | Render for comparison; report the field as unreflectable; do not write. | +| 2 — explicit route | Existing `apply-setters` comment and config | Edit the named setter value; verify every render output. | +| 3 — narrow function contract | A pinned, allowlisted function with a tested sink-only configuration field | Dye or otherwise perturb the configuration, count fan-out, and write only after verification. | +| never generic | Arbitrary container/exec function, network access, unpinned tag, opaque generated output | No source routing. Refuse the edit or operate only on a separately committed rendered artifact. | + +The KRM Functions Catalog is therefore a source of **candidate contracts**, not a list to +enable. `apply-setters` is a strong candidate because it declares the inverse coordinate. +Bulk transforms such as `set-labels`, `set-namespace`, `set-image`, Starlark, Helm +rendering, or generic search-and-replace should begin at level 1. Each needs independent +evidence about determinism, selector fan-out, safe-to-dye values, and a comment-preserving +write location before it can move higher. + +## 5. Extending the render-plan idea + +The Kustomize tracer's proposed render plan is a useful *shape* that can eventually cover +more than Kustomize. It is an inverse-build artifact: a lookup hint for routing, never +permission to write. + +```yaml +apiVersion: gitopsreverser.io/v1alpha1 +kind: RenderPlan +renderer: + type: kpt + packageRoot: apps/web + pipelineFingerprint: sha256:REPLACE_WITH_REAL_CONTENT_HASH +inputs: + Kptfile: sha256:REPLACE_WITH_REAL_CONTENT_HASH + deployment.yaml: sha256:REPLACE_WITH_REAL_CONTENT_HASH + setters.yaml: sha256:REPLACE_WITH_REAL_CONTENT_HASH +objects: + - id: apps/v1/Deployment/default/web + origin: deployment.yaml + fields: + spec.replicas: + source: + kind: setter + file: setters.yaml + path: data.web-replicas + function: apply-setters +unattributable: + - object: apps/v1/Deployment/default/web + field: metadata.labels.team + reason: function-contract-not-supported +``` + +The fingerprint prevents a plan made for old inputs from posing as current knowledge. A +matching plan can propose the setter edit; a missing, stale, or incomplete plan causes a +fallback to live counterfactual attribution where that is safe, otherwise a refusal. In +all cases the post-write render remains decisive. + +This artifact could eventually serve three consumers without creating three analyses: + +- **writer:** field-to-source proposals and clear refusals; +- **repository map:** render roots/packages, inputs, and ownership edges; and +- **metrics:** counts of `unattributable` fields and the transforms that cause them. + +## 6. Practical sequencing + +Do not let Kpt exploration interrupt the current Kustomize support-boundary work. +`patches:` is the immediate, measured opportunity: make folders renderable first, move +refusals to individual unroutable fields, then add tightly bounded attribution and routing. + +After that, the smallest useful Kpt slice is: + +1. Detect `Kptfile` during repo discovery and report whether a pipeline is present. +2. Record whether the deployment path actually executes Kpt. Do not assume that it does. +3. Support **existing setters only** for a locally reproducible, pinned `apply-setters` + pipeline, with a fixture that proves single-field and fan-out behaviour. +4. Reuse the existing full-batch oracle over the Kpt render scope. +5. Add one function contract at a time, only when its safe inputs and writeback coordinate + are explicit and covered by counterfactual tests. + +The desired outcome is not universal transformation support. It is a growing set of +honest answers: *this live field belongs to this source coordinate and we can prove the +round-trip*, or *this renderer produced the field but we cannot safely write it back.* diff --git a/docs/design/support-boundary/orchestrator-reconcile-trigger.md b/docs/design/support-boundary/orchestrator-reconcile-trigger.md new file mode 100644 index 00000000..9f3f3da1 --- /dev/null +++ b/docs/design/support-boundary/orchestrator-reconcile-trigger.md @@ -0,0 +1,250 @@ +# The orchestrator reconcile trigger: revert a refusal, and order around origin drift + +> **design** — direction-setting; ships no code. Nothing it describes is supported today. +> Captured: 2026-07-15 +> Related: +> [README.md](README.md), +> [../../bi-directional.md](../../bi-directional.md) — **the user-facing model this expands: the reconciler as a *triggered applier***, +> [argocd-bi-directional.md](argocd-bi-directional.md) — why `selfHeal` must be off, and why that means nothing reverts a refused edit, +> [orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md) — **the ownership model this rides on; it is the first *write* action built on it**, +> [admission-consent.md](admission-consent.md) — the sibling half: deciding *whether* a write happens, +> [gittarget-granularity-and-cross-environment-edits.md](gittarget-granularity-and-cross-environment-edits.md) + +This is one half of a two-part design. [The other half](admission-consent.md) decides *whether* a +write happens, at admission time. This half is about the operator gaining **one new outward action — +asking the GitOps orchestrator (Flux/Argo) to reconcile now** — and the two places it is needed: +reverting a refused edit promptly, and ordering our processing behind an incoming origin change. + +It is not a correctness layer. The one correctness gate stays where it is — the flush-time render +oracle ([`VerifyBatchRenders`](../../../internal/manifestanalyzer/render_verify.go)). This is about +closing the loop *fast* and *visibly* once a "no" has been decided. + +--- + +## 1. The gap: a triggered applier that is never triggered on the two cases that need it + +[bi-directional.md](../../bi-directional.md) already establishes the model: the reconciler is not an +always-on loop, it is a **triggered applier**. After the operator commits, it triggers the reconciler +to apply *that exact commit* and waits for the SHA — and because the applied revision equals the +committed revision, the loop is closed. The guide draws it as a steady-state loop: + +```mermaid +flowchart LR + api(["API / operator edit"]) --> cluster[("Cluster")] + cluster -- watch --> gr["GitOps Reverser"] + gr -- "commit + push" --> git[("Git")] + git -- "push webhook" --> recon["Flux / Argo CD
(selfHeal OFF)"] + recon -- "apply fresh commit" --> cluster +``` + +That loop only closes on the **happy path**, where a commit is produced and a push fires the webhook. +Two cases fall straight through it, and they are exactly the ones where the operator must trigger the +reconciler *directly* rather than via a push that never happens: + +```mermaid +flowchart LR + api(["API / operator edit"]) --> cluster[("Cluster")] + cluster -- watch --> gr["GitOps Reverser"] + gr -- "commit + push (write placed)" --> git[("Git")] + git -- "push webhook" --> recon["Flux / Argo CD
(selfHeal OFF)"] + recon -- "apply fresh commit" --> cluster + + gr -. "REFUSED: no commit, no push, no webhook —
trigger reconcile to REVERT the live edit (§2)" .-> recon + other(["someone else pushes"]) --> git + git -. "ORIGIN DRIFT: trigger + wait, then process events (§3)" .-> recon + + classDef trig fill:#ffd,stroke:#cc3,color:#111 + class gr,git trig +``` + +The two dotted arrows are the whole of this document: the reconcile trigger, and where it sits. + +--- + +## 2. Use one: revert a refused edit + +### Why it is necessary, not merely nice + +With Argo `selfHeal: false` — *required* for bi-directional editing, because with it on Argo reverts +the live edit sub-second from cached Git and thrashes against us +([argocd-bi-directional.md](argocd-bi-directional.md)) — **nothing reverts a refused edit.** It is +`OutOfSync` until a human intervenes: the push webhook never fires (there was no commit, no push), and +the poll re-resolves an *unchanged* Git and does nothing. Under Flux it is milder but the same shape — +reverted only on the next interval reconcile. So triggering a reconcile is not a speed-up; for Argo it +is the *only* thing that ever reverts a refused edit. + +And the operator is uniquely entitled to do it. [argocd-bi-directional.md](argocd-bi-directional.md) +names the missing ingredient precisely: a system that can *distinguish authorized drift from +unauthorized*, which Argo cannot. **At refusal time the operator has exactly that signal** — the +refused edit is, by construction, the drift with no home in Git. So an operator-triggered reconcile of +a refused edit is the **targeted, per-write substitute for the blanket self-heal the operator had to +switch off.** + +```mermaid +sequenceDiagram + actor User + participant K8s as Cluster + participant Rev as GitOps Reverser + participant Git + participant Recon as Flux / Argo CD + + User->>K8s: edit a base-owned field + K8s-->>Rev: watch event + Note over Rev: flush oracle REFUSES —
no legal destination in Git + Rev-->>K8s: GitPathAccepted=False
(async — the User already got 200 OK) + Note over Git: nothing committed → no push → no webhook + Note over Recon: selfHeal off → it will NOT revert drift on its own + Rev->>Recon: TRIGGER reconcile of the governing object + Recon->>Git: fetch current revision (unchanged) + Recon->>K8s: re-apply desired state → the refused edit is reverted + Recon-->>Rev: reports revision applied + Note over Rev,K8s: loop closed in seconds, instead of never +``` + +Compare this to the guide's happy-path triggered-applier sequence: the shape is identical, but the +trigger fires on a **refusal** (nothing was committed) instead of on a commit, and its job is to +*revert* the drift rather than to *apply* a fresh commit. + +### What "trigger" means, per orchestrator + +The [orchestrator-knowledge-boundary](orchestrator-knowledge-boundary.md) rule is absolute: **never +depend on the `argoproj` or `fluxcd` Go modules.** The trigger is a patch on an object, matched by +group+kind over `unstructured`. But the two orchestrators differ, and the difference matters: + +| | **Flux** | **Argo CD** | +|---|---|---| +| Trigger | patch `reconcile.fluxcd.io/requestedAt` on the `Kustomization` | see below | +| Does it revert drift? | **Yes, cleanly** — Flux server-side-applies desired state, which reverts drift as a side effect | **Not with a plain refresh.** `argocd.argoproj.io/refresh` re-reads Git and re-compares, but with `selfHeal` off it only marks `OutOfSync` — it does **not** revert | +| To actually revert | (nothing more) | requires a **sync operation** — a deliberate, one-shot self-heal of the specific refused object | + +So Flux is a one-annotation, correct-outcome trigger. Argo needs the stronger action — a sync — which +writes to the cluster and must therefore be an explicitly granted authority, scoped so it can only +ever revert the specific refused object, never sync the whole app. + +--- + +## 3. Use two: reconcile-before-process, as an ordering barrier + +The same trigger answers a different, subtler problem — **origin moved under us** — and here it is a +*barrier*, not a revert. + +### What is, and isn't, already handled + +The commit direction is already safe against a moved remote. `PushAtomic` +([`git_atomic_push.go`](../../../internal/git/git_atomic_push.go)) is a compare-and-swap, never a +force-push; if the remote advanced, the push is rejected and `pushPendingCommits` +([`branch_worker.go`](../../../internal/git/branch_worker.go)) **rebases by replay** — hard-reset to +the new tip, re-plan and re-commit the retained pending writes on top, re-push with an updated CAS. So +we never clobber someone else's push, and our own intent survives. + +What is **not** handled is the *cluster* side. When origin gains new desired state, the orchestrator is +about to apply it, producing a flood of watch events. Two problems follow: + +1. **The reconcile echo.** Those events came *from* Git via the orchestrator. If the operator processes + them as user intent and mirrors them back, it is round-tripping the orchestrator's own apply into + Git — noise at best, a fight at worst. +2. **The stale baseline.** The operator's model of "what this folder renders to" was computed against + the *old* tree, so its refusal/attribution decisions for the transitional state can be wrong until + the cluster reflects the new origin. + +### The barrier + +The fix is an ordering rule: **on origin drift, trigger the orchestrator to reconcile Git → cluster, +wait for it, and only then resume processing live events.** After the reconcile, the operator's own +mark-and-sweep resync absorbs the orchestrator's apply as a **no-op against the new tree**, instead of +mirroring it as intent. + +```mermaid +sequenceDiagram + actor Other as Someone else + participant Git + participant Rev as GitOps Reverser + participant Recon as Flux / Argo CD + participant K8s as Cluster + + Other->>Git: push new desired state (origin drifts) + Note over Rev: detects the remote moved + Rev->>Git: land pending intent first
(rebase onto the new tip — §3, the hazard) + Rev->>Recon: TRIGGER reconcile Git → cluster, and WAIT + Recon->>Git: fetch the new revision + Recon->>K8s: apply the new desired state + Recon-->>Rev: reports the new revision applied + Note over Rev: only NOW resume live events —
the orchestrator's apply is absorbed
as a resync no-op, not mirrored back +``` + +This rides machinery that already exists. There is already a *reconcile-before-process* barrier: on +watch (re)establishment the operator enqueues a scoped mark-and-sweep ahead of live events, gated by +the `replaying` flag, all on one FIFO so order is preserved +([`target_watch.go`](../../../internal/watch/target_watch.go), +[`resync_flush.go`](../../../internal/git/resync_flush.go)). Two additions turn it into what we need: a +new **trigger** (origin-drift detection, from the existing cached remote-drift check `SyncAndGetMetadata`), +and a stronger **wait** (the barrier's "reconcile" step now waits for the *orchestrator* to apply +Git → cluster, not only the operator's own sweep). + +> **Terminology, because "reconcile" is overloaded and this doc would mislead without saying so.** +> There are two. The **orchestrator reconcile** (Flux/Argo applies Git → cluster) is what this +> document triggers and waits on. The operator's internal **resync** (a mark-and-sweep that rebuilds +> the Git-side model from the cluster, cluster → Git) is what runs *after* the barrier to absorb the +> apply. Where this doc means the internal one, it says "resync." + +### The hazard: pending intent vs. the reconcile that overwrites it + +There is a real ordering trap, drawn as the first `Rev->>Git` step above. When origin drifts, the +operator may hold **uncommitted** pending intent — live edits captured in the open commit window but +not yet pushed. Triggering the orchestrator reconcile *now* would apply the new origin over those live +edits on the cluster, erasing them before they reach Git. + +That is only safe because the intent is **durably captured** (the open window / `pendingWrites`) and +the commit side already rebases it onto a moved origin. So the ordering must be: **land pending intent +to Git first (rebased onto the new tip), *then* trigger the reconcile, *then* resume.** If capture +were not durable, the reconcile would eat the user's edit — so the barrier's safety rests on the +durability the pipeline already has, stated here as a precondition rather than discovered as a bug. + +--- + +## 4. The prerequisite: this is the first *write* on the ownership model + +The operator today **cannot name the Flux/Argo object that deploys a GitTarget's path.** A GitTarget +knows only `(provider, branch, path)`; there is no field, no lookup, and no code that reaches an +orchestrator object (confirmed across `internal/`, `api/`). That capability is designed but unbuilt: +[orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md) proposes per-orchestrator +*interpreters* (`internal/gitops/{flux,argocd}`) emitting **claims about paths** — e.g. +`RenderRootFor{path, by}` naming the object that renders a folder. + +The reconcile trigger is the **first write action** built on that model, which until now is a purely +read/claim vocabulary. It needs one new claim — *"object O reconciles path P"* — and the ability to +patch O. So this feature does not stand alone: **it is gated on the ownership interpreters landing +first.** Stated as a dependency, not smuggled as an assumption. + +--- + +## 5. The boundary: opt-in, and never on a mirror + +Patching another controller's object, and (on Argo) issuing a sync, are boundary crossings. They are +off by default and enabled per GitTarget, alongside the tier-3 write gate — e.g. a +`spec.reconcileTrigger: Off | OnRefusal | OnDrift` knob. Two hard rules: + +- **Never on a cluster the operator merely mirrors.** As with the write gate, we do not get to drive + someone's orchestrator because our mirror is lossy. Only where the cluster is an *editing surface* + whose changes are meant to flow to Git. +- **Absent orchestrator ⇒ no-op, not error.** If no interpreter claims the path, there is nothing to + trigger; the operator falls back to today's behavior (report and wait). The trigger is an + accelerator layered on top. + +--- + +## 6. Open questions + +- **The Argo sync decision.** Reverting a refused edit needs a *sync operation*, not just a refresh — + a deliberate one-shot self-heal. Is issuing it an authority the operator should hold, and how is it + scoped so it can only ever revert the specific refused object, never sync the whole app? +- **Wait semantics for the barrier.** How long to wait for the orchestrator to finish; timeout and + fallback (resume anyway, or stay blocked?); how to observe "done" without depending on the + orchestrator's Go types (its status conditions over `unstructured` — e.g. + `Kustomization.status.lastAppliedRevision`, `Application.status.sync.revision`). +- **How confident must the ownership claim be before we *write* on it?** A wrong "object O reconciles + path P" claim triggers the wrong controller. This is a higher bar than a claim used only to *read*. +- **Interaction with the happy path.** When a commit *does* land, `bi-directional.md`'s webhook already + triggers the apply. Does the operator still issue an explicit trigger (and wait for the SHA, closing + the handshake the guide describes), or defer to the webhook? Likely: trigger only where the webhook + cannot help — refusal and drift — and let the push webhook cover the happy path. diff --git a/docs/design/support-boundary/render-fidelity.md b/docs/design/support-boundary/render-fidelity.md new file mode 100644 index 00000000..36e61c8e --- /dev/null +++ b/docs/design/support-boundary/render-fidelity.md @@ -0,0 +1,360 @@ +# Render fidelity: our render is not the orchestrator's + +> **design** — direction-setting; ships no code. Nothing it describes is supported today. +> Captured: 2026-07-15 +> Related: +> [README.md](README.md), +> [render-root-scoping.md](render-root-scoping.md) §3 — the version-skew caveat this generalises, +> [render-attribution.md](render-attribution.md) §5 — *attribution may be heuristic, verification may not*, and the "shared blind spot" failure, +> [orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md) — reading the Flux/Argo object; the `TransformedOutOfBand` claim, +> [gittarget-granularity-and-cross-environment-edits.md](gittarget-granularity-and-cross-environment-edits.md), +> [finished/images-and-replicas-edit-through.md](finished/images-and-replicas-edit-through.md) + +We run `kustomize build` on the folder. Flux and Argo run kustomize on the folder **plus a layer +of context that is not in the folder** — so the object the cluster runs is not the object our +render produces, and every guarantee we make by rendering is only as good as that gap being empty. + +This document names the gap, records a fence that looked obvious and was **wrong** (and why), and +proposes the fence that is right: **measure our render against the live object, and refuse where +they disagree.** It is the same discipline as the rest of this workstream — do not reason about +what the renderer does, ask it — applied one level up: do not assume our render is the +orchestrator's, *check* it. + +--- + +## 1. The context the folder does not hold + +Grounded against the vendored trees, not assumed: + +- **Argo CD** keeps a *second kustomize layer in the Application object*, not the repo: + `spec.source.kustomize` overrides `images`, `replicas`, `patches`, `components`, `nameSuffix`, + `namePrefix`, `commonLabels`, `commonAnnotations`, `namespace`, and even the kustomize **`version`** + (measured in the vendored `argo-cd/pkg/apis/application/v1alpha1/types.go:723`, under + `external-sources/`). A folder we render to `X`, Argo can apply as `Y` — with the very features we + build edit-through around (`images`, `replicas`, `patches`) supplied from a place we never read. +- **Flux** runs `postBuild.substitute` / `substituteFrom` **after** the build, replacing `${var}` + tokens from cluster ConfigMaps/Secrets (measured in the vendored `flux2/internal/build/build.go:631`, + `kustomize.SubstituteVariables`), plus `targetNamespace` and object-level `patches`/`images` on the + Kustomization. +- **Both** run *their* kustomize version with *their* build flags (Argo's `kustomize.buildOptions`), + possibly through a config-management plugin. + +So `applied = f(repo, orchestrator-object, their-kustomize)`, while we compute `g(repo, our-defaults)`. +`g = f` only when the orchestrator adds no context. That is common — but not guaranteed, and **we +cannot tell from the repo which case we are in.** + +```mermaid +flowchart LR + repo[("Git folder")] --> ours["OUR kustomize build
pinned version, default flags"] + ours --> og["our render: g(repo)"] + repo --> orch["THE ORCHESTRATOR
kustomize + postBuild substitute
+ Application overrides + their version"] + ctx["context NOT in the folder:
Flux postBuild vars, Argo spec.source.kustomize,
build flags, kustomize version"] --> orch + orch --> live["live object: f(repo, context)"] + og -. "we ASSUME g = f
(and only sometimes it is)" .-> live + + classDef blind fill:#fdd,stroke:#c33,color:#111 + class ctx blind +``` + +This is not a new admission. [render-root-scoping.md §3](render-root-scoping.md) already concedes a +*version*-skew caveat — the guarantee is only *"this renders to what you edited, under the kustomize +we pinned."* This document generalises it: **it is not only the version that can differ, it is the +whole render context**, and version is the *least* likely axis to bite. + +--- + +## 2. Why it bites, and why the source-form fix does not close it + +The [source-form projection](finished/images-and-replicas-edit-through.md) stops **our** render's +output leaking into the source: where the live object and *our* render agree, the source keeps its +bytes. But context we do **not** render makes `live ≠ our-render` for a reason that is *not a user +edit* — and the projection has no way to tell the two apart. It reads the divergence as an edit and +writes the orchestrator's value into the source. + +Concretely, with Flux `postBuild`: + +```text +git source: env REGION = ${REGION} +our render: env REGION = ${REGION} (kustomize never touches a ${...} token) +live object: env REGION = us-east (Flux substituted it from a cluster ConfigMap) +``` + +The projection sees source == our-render (`${REGION}`) but live differs, concludes the user set +`us-east`, and **writes `us-east` into the source — destroying the `${REGION}` parameterisation.** +Next reconcile, Flux substitutes again (now a no-op, the value is already literal), so it *looks* +converged while the template is gone; change `REGION` later and the file no longer follows. + +And the oracle does not catch it. [`VerifyBatchRenders`](../../../internal/manifestanalyzer/render_verify.go) +re-renders the write with **our** kustomize, which also leaves `${REGION}` literal — so it agrees +with the corrupt write. This is precisely the failure +[render-attribution.md §5](render-attribution.md) warns about — *a verification that shares the blind +spot of the thing it verifies* — one level up: **our whole renderer shares the orchestrator's blind +spot.** + +--- + +## 3. What we learned: the structural `${...}` check is the wrong fence + +The obvious fence is structural and cheap, and it was tried (and reverted): refuse, at the +acceptance gate, any managed document whose values carry a `${...}` token. It is wrong, and the way +it is wrong is the reason the right fence looks the way it does — so it is recorded here rather than +quietly dropped. + +**`${...}` is ambiguous, and the repo cannot disambiguate it.** The same token is, with equal +frequency: + +- **literal, and safe to mirror** — a CRD schema `description` (the Flux Kustomization CRD documents + postBuild with `${var:=default}` *in its own schema text*), a KRO `${schema.spec.*}` template, an + nginx or envsubst ConfigMap. In every one of these the **live object carries the token verbatim + too** — nothing substitutes it — so `live == our-render` and there is no risk at all. +- **substituted, and dangerous** — the Flux `postBuild` case of §2, where live is `us-east` and our + render kept `${REGION}`. + +A structural check fires on both, and the measurement was unambiguous: **it broke CRD mirroring +outright.** The acceptance gate is all-or-nothing over a folder, so a single CRD whose description +merely *mentions* `${var:=default}` refused the entire folder — every unrelated write with it. A +folder of ordinary CRDs (Flux, cert-manager, prometheus-operator all ship `${}` in their schemas) +became unmanageable. This is not "over-refuse a little and be right"; it is breaking a core feature. + +**The lesson is not "narrow the regex."** No structural refinement helps, because the +discriminator — *was this token actually substituted?* — **is not in the repo.** A CRD's +`${var:=default}` and a Deployment's substituted `${REGION}` are identical on disk; they differ only +in whether the *cluster's* copy still holds the token. The fence therefore cannot be structural, and +cannot live at the structure-only acceptance gate. + +> A smaller lesson, recorded so it is not relearned: `task test-e2e 2>&1 | tail -N` reports `tail`'s +> exit status (0), not the suite's — a failing suite read as green. Capture the full log, or assert +> on the summary line. + +--- + +## 4. The right fence: measure against the live object + +The discriminator we lack on disk, we already hold at write time: **the live object *is* the +orchestrator's render.** It is what the orchestrator actually applied — postBuild, overrides, its +kustomize version, all of it. So we need not *predict* the orchestrator's context; we can *observe +its output*. + +> **Our edit-through is sound exactly where our render equals the live object at the fields we did +> not set out to change. Where it does not, the orchestrator did something we cannot see — refuse, +> do not guess.** + +This is the workstream's own method, one level up. The dye measures *which entry supplies a value*; +the oracle measures *whether a write reproduces the live object*; this measures *whether our render +is even the right baseline to reason from*. And it is **precise where the structural check was +blunt** — same tokens, opposite verdicts, each correct because it is read off the live object rather +than guessed from disk: + +```mermaid +flowchart TD + W["about to write a field back to source"] --> Q{"does our render equal
the LIVE object here?"} + Q -->|"yes"| K["SAFE — our render is what the cluster runs.
keep source / write as normal"] + Q -->|"no, and the source carries a substitution token here"| R["REFUSE — out-of-band substitution
would destroy the token on write"] + Q -->|"no, and no token (general case)"| D["context skew OR ordinary runtime drift —
the open discriminator (§5b, §8)"] + + classDef good fill:#dfd,stroke:#3a3,color:#111 + classDef bad fill:#fdd,stroke:#c33,color:#111 + classDef open fill:#ffd,stroke:#cc3,color:#111 + class K good + class R bad + class D open +``` + +| document | token | our render vs live | structural check | render-vs-live | +|---|---|---|---|---| +| Flux CRD, `${var:=default}` in a description | yes | **equal** (live has it too) | ❌ refused (wrong) | ✅ mirror | +| KRO RGD, `${schema.spec.*}` template | yes | **equal** | ❌ refused (wrong) | ✅ mirror | +| nginx ConfigMap, `${host}`, no postBuild | yes | **equal** | ❌ refused (wrong) | ✅ mirror | +| Deployment env `${REGION}`, Flux postBuild | yes | **differ** (`us-east`) | ✅ refused | ✅ refuse | + +--- + +## 5. Two shapes of the fence, and which to build first + +### 5a. The precise instance — refuse a write over a diverged token + +The cheapest correct fence, and the one with **no false positives**: *when a write would replace a +source value that still carries a `${...}` token with a different live value, refuse it.* This is the +render-vs-live rule scoped to the one case we can be certain of. kustomize provably never touches a +`${...}` token, so **our render always keeps it**; if the live object has a *different* value there, +something out of band changed it — not the user, not our build. It catches the Flux +postBuild / envsubst class, per field, and it fires on none of the literal-token documents in §4's +table, because there the live value still *is* the token. + +It applies to **any** mirrored document, not only kustomize-governed ones: for a plain folder our +"render" is the source itself, so the rule reduces to *"live differs from a source token ⇒ refuse,"* +which is exactly right for a plain folder Flux deploys with postBuild. + +### 5b. The general version — our render must reproduce live for the untouched set + +The broader fence catches more than tokens — Argo `spec.source.kustomize` overrides, version skew, +anything that makes the applied object differ: *before trusting our render as the baseline, require +it to reproduce the live object for every field the write does not deliberately change.* + +It is strictly more powerful and strictly harder, for one honest reason: **a live object legitimately +drifts from Git for reasons that are not out-of-band render context.** An HPA changed `replicas`; a +defaulting webhook filled a field; another controller populated something. A naive *"live ≠ render ⇒ +refuse"* would abort a flush because an HPA scaled a Deployment — which is not ours to police. +Distinguishing *context skew* from *ordinary runtime drift* is the open problem of the general fence +(§8), and it is why **5a is the right thing to build first**: it sidesteps the problem entirely by +keying on a token kustomize is *guaranteed* never to produce. + +--- + +## 6. Two surfaces of the one measurement + +The measurement — *does our render equal the live object?* — needs the live object, so it runs at +**reconcile time**, where the operator holds both the Git content and the watched live state. That +rules out only the **structure-only** acceptance gate (the CLI scan, the initial dry validation), +which has no cluster to look at — the trap the reverted structural check fell into. It does **not** +rule out the operator, which has live state in hand on every reconcile. And once the measurement runs +there, the answer is worth exposing two different ways. + +### 6a. A per-write refusal (§5) + +Point-in-time, per field: a write that would overwrite a source token whose live value diverged is +refused, in the family of `WriteBoundaryRefused`, naming the file, field, and token. This is the guard +that stops the corruption at the moment it would happen. It sits beside the source-form projection — +that decides *keep-source vs write-live* per field; this turns a *write-live* into a refusal when the +field it would overwrite holds a token the live object no longer has. It is per field, per object, so +one diverged token refuses one write, never a whole folder (the failure that broke CRD mirroring). + +### 6b. A GitTarget status you can read *before* you edit + +The same measurement, aggregated to the folder and surfaced as a standing **GitTarget condition** — +e.g. `RenderFaithful` — answers a more fundamental question than any single write does: + +> **Do we even have a chance of tracking this folder?** + +Because if our render does not match what the cluster runs, *nothing* we do on the folder is +trustworthy — not the mirror, not edit-through, not the refusal decisions themselves — since all of +them reason from a baseline that is wrong. A per-write refusal tells you *this edit* could not land; a +`RenderFaithful=False` condition tells you *this whole folder* is deployed with context we cannot +reproduce (Flux postBuild, Argo `spec.source.kustomize`, a divergent version), so you learn it **up +front, from status, before you waste an edit** — rather than one refusal at a time. + +It carries a bounded sample of the diverging `(file, field)` pairs, in the style of the +`FullyReflected` condition in +[unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md), and it is a +sibling of that condition: `FullyReflected` says *everything you edited was expressed*; +`RenderFaithful` says *our render matches what is running, so we can be trusted at all* — the more +fundamental of the two. It is recomputable: the mark-and-sweep resync rebuilds it from scratch, +steady-state events keep it current. + +This is exactly what the reverted structural check was reaching for and could not have — a +folder-level *"can we track this?"* verdict. It failed because it tried to answer from the **disk**; +the same question, answered from the **live object**, is both correct and precisely the up-front +signal a user wants. + +**And the condition blocks.** A `GitTarget` *is* the claim that a folder can be reverse-GitOps'd — +live changes captured faithfully back to Git. If our render is not equal to what the cluster runs +that claim is void: we cannot reverse a state we cannot reproduce. So `RenderFaithful=False` gates +adoption exactly as a structural refusal does — the folder is **not tracked**, `Ready=False`, with a +reason naming the diverging fields — not a soft warning beside a folder we quietly mishandle. This is +deliberately the strict choice, and it can be loosened later (mirror for audit, refuse only the +writes) *if* a demand for tracking read-only-but-unfaithful folders proves out. Strict first, because +the failure it prevents is silent corruption, and loosening a gate is reversible where a shipped +corruption is not. + +### How it composes with the oracle + +`VerifyBatchRenders` checks our render reproduces live **after** a write, sharing our render's blind +spot. The fidelity measurement checks our render reproduces live **before** it, at the fields we are +not touching — catching the blind spot the oracle cannot. + +--- + +## 7. Implementation: where the comparison runs + +The blocking decision forces the timing: `RenderFaithful` must be computed where the operator holds +both the Git content and the live objects, and **before any write** — because the first mirror of an +unfaithful folder *is* the corruption. That point already exists, which is why the shape you +suggested is the right one. + +### The one predicate, computed once, read by both surfaces + +Everything reduces to a single per-document test: + +> **unfaithful(doc) := our render of the Git document carries a `${...}` token at a field where the +> live object holds a resolved value.** + +kustomize never emits a `${...}` token, so our render preserves the ones the source carries; if the +live object has a *different, resolved* value there, something out of band produced it. The per-write +refusal (§6a) is this predicate on the one document a write touches; the folder condition (§6b) is the +same predicate ORed across the folder. Build it once and read it twice. + +It is the **token** form (5a) we block on, not the general render≠live form (5b), and that is a +*requirement* of blocking rather than a shortcut: 5b would flag an HPA that scaled a Deployment, and +blocking a folder for ordinary runtime drift would refuse to track a folder that is perfectly fine. A +`${...}` token — which kustomize provably never produces and an HPA never introduces — has no such +false positive. 5b, and the `managedFields` discriminator it needs, is the follow-on (§8). + +### Where to run it — three ways + +**(A) Fold it into the reconcile that runs on acceptance — recommended, and the shape you described.** +When a folder is accepted, the watch opens with `SendInitialEvents` and enqueues a scoped +**mark-and-sweep resync ahead of any live event** (the `replaying` barrier; +[`target_watch.go`](../../../internal/watch/target_watch.go), +[`resync_flush.go`](../../../internal/git/resync_flush.go)). That resync already scans the whole +subtree *and* replays the live objects — the one moment both halves are in hand and nothing has been +written. Add the predicate as a **resync precondition**, beside the write-boundary ones: render the +roots (already done for the oracle), walk each rendered object against its live counterpart for a +token divergence, and if any document is unfaithful, **abort the resync's writes and set +`RenderFaithful=False`**. That same abort is what stops the corruption — an unfaithful resync would +otherwise mirror `us-east` over `${REGION}` on the spot. *Cost:* one field-walk on top of a render we +already do — milliseconds. + +**(B) A distinct live-aware acceptance layer.** Keep structure-only `Accept` as it is (fast, no +cluster), and add a *second* gate — `AcceptRenderFaithful(store, liveObjects)` — that runs at the same +resync moment but is named and tested as its own function. Behaviourally this is (A); the difference is +packaging. Worth it only if the seam buys clarity: structure-only acceptance answers *"can we parse +and route this?"*, the fidelity gate answers *"does our render match reality?"*, and separate pure +functions keep each independently testable. + +**(C) Derive the folder verdict purely from per-write refusals.** Don't compute a folder pass at all — +refuse each unfaithful write (§6a) and flip `RenderFaithful=False` the first time one is refused for +this reason. Simplest, and it keeps 6a and 6b in one code path — but it makes the folder *"faithful +until proven otherwise, one write at a time"*, so a user only learns it is untrackable **after** +attempting an edit. That is exactly the up-front property 6b exists to give, so (C) delivers 6a and +loses the point of 6b. + +**Recommended: (A).** It is where the reads already happen, it runs before any write — so it both sets +the verdict and prevents the corruption in one step — and it produces the up-front folder answer. (B) +is (A) with a cleaner seam, a reasonable refinement. (C) is the fallback that keeps only the per-write +half. Whichever computes the folder pass, the steady-state per-write check (§6a) still runs between +resyncs, so a folder that becomes unfaithful later (postBuild added after adoption) flips the +condition the moment the first such write is refused — 6a keeps 6b current. + +### How it blocks, and how it clears + +`RenderFaithful` is a GitTarget condition. False means the folder is **not tracked**: the resync +commits nothing, the branch worker opens no window for it, and `Ready=False` carries a reason +(`RenderNotFaithful`) with a bounded sample of the diverging `(file, field, token)`. It sits second to +`GitPathAccepted` — structure-only acceptance is the first gate (*parseable, routable?*), +`RenderFaithful` the second (*does our render match reality?*), and a folder must pass both. It is +**recomputable and self-healing**: rebuilt from scratch on every resync, so removing the postBuild +config — or moving the tokens out of the tracked subtree — flips it back to True on the next reconcile +with no manual acknowledgement. + +--- + +## 8. Open questions + +- **5a vs 5b, and sequencing.** 5a (token) is precise and cheap — build first. 5b (general + render-vs-live) is the complete answer but needs the runtime-drift discriminator before it is safe. +- **The runtime-drift discriminator.** Can we separate "an HPA changed replicas" from "postBuild + changed an env" without hand-written per-field policy? **`managedFields` looks promising and should + be measured**: a field owned by the GitOps controller's apply is render context; a field owned by + `hpa`, `kubelet`, or a defaulter is drift. If that holds, 5b becomes safe. +- **Argo overrides leave no token.** `spec.source.kustomize.images` produces no `${}`; only 5b — or + orchestrator awareness — catches it. Is it acceptable to catch the token class first and the + Application-override class later, or do they need to land together? +- **Orchestrator awareness as the third, most complete fence.** Reading the Flux Kustomization / Argo + Application (the [interpreter model](orchestrator-knowledge-boundary.md)) would let us *know* + postBuild/overrides are configured and refuse — or eventually model — them directly, emitting the + `TransformedOutOfBand` claim that doc already reserves. It is the most work; 5a is the same + protection for the substitution class **without** reading the orchestrator's object. +- **Version skew.** Neither fence fully catches a pure version difference that changes a render + subtly but touches nothing we compare; [render-root-scoping.md §3](render-root-scoping.md)'s "pin + to the version Flux ships" stays the mitigation. 5b *does* catch a version difference that actually + moves an untouched object. From 4a255a1055d8711525cf4dfe51e4e9065c9a8194 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 21:15:48 +0000 Subject: [PATCH 4/5] feat(kustomize): tolerate a patch, without authoring one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `patches:` block used to refuse the whole GitTarget. Not the edit — the TARGET. A folder whose patch pins a replica count also lost images:/replicas: edit-through, which the patch has nothing to do with. It is the single biggest refusal cause in the layout corpus. A strategic-merge patch named by `path:` is now READ-ONLY BUILD CONTEXT: - the folder is accepted, and what it renders is mirrored; - the patch FILE is retained, never managed. This is the part nothing else in the store would have got right: a sparse patch IS a KRM document, so materialised it would be indexed as a manifest, matched to a live object, mirrored over with a whole Deployment, or swept as an orphan nothing in the cluster answers to. That a patch is not a resource is not our claim — it is the RENDER's: a patch file never appears as a rendered object's origin; - images:/replicas: edit-through works in a patched folder, exactly as anywhere; - an edit to a field the PATCH owns is refused per OBJECT, not per folder. Tolerating a patch is not authoring one. Nothing is ever written into a patch file. Exactly one shape is tolerated; the rest refuse BY NAME, because "unsupported" is not something a user can act on and "your patch is inline, and we can only read one from a file" is: patches-inline (which is also where an inline JSON6902 op list arrives, since it decodes into the same field), patches-json6902 (a path to an op list — a YAML sequence, not a sparse KRM document), patches-outside-tree. MEASURED, NOT ASSUMED: FixKustomization folds `bases` into resources and `imageTags` into images, but it does NOT fold `patchesStrategicMerge` or `patchesJson6902` into `Patches`. They keep refusing under their own names, and a test pins that so a kustomize bump cannot silently widen what we accept. THE CORPUS SAYS SOMETHING THE PLAN DID NOT EXPECT, and it is the deliverable here: tolerating patches accepts ZERO new candidates. Every patched overlay in the corpus also reads a base from OUTSIDE its own folder, so `patches` was masking the real refusal. flux-monorepo/apps/{staging,production} now report overlay-fan-out-unsupported — the verdict render-root-scoping.md §5 records as never having been observed, because refused-structural always fired first and hid it. Render-root scoping, not patches, is the single blocker on the corpus's most tractable layout. The mechanism itself is proven by fixtures at both levels: a self-contained patched root is accepted, its image bump routes to the entry, its in-sync state is a no-op, and its patch-owned scale is refused. --- .coverage-baseline | 2 +- docs/UPGRADING.md | 28 +++ .../support-boundary/render-root-scoping.md | 23 ++ .../support-boundary/support-contract.md | 5 +- internal/git/patches_test.go | 129 +++++++++++ internal/manifestanalyzer/acceptance.go | 14 +- .../manifestanalyzer/kustomization_parse.go | 183 +++++++++++++-- internal/manifestanalyzer/overrides_test.go | 5 +- internal/manifestanalyzer/patches_test.go | 219 ++++++++++++++++++ internal/manifestanalyzer/scan_repo.go | 20 +- internal/manifestanalyzer/store.go | 108 +++++++-- test/fixtures/gitops-layouts/support-today.md | 16 +- 12 files changed, 689 insertions(+), 63 deletions(-) create mode 100644 internal/git/patches_test.go create mode 100644 internal/manifestanalyzer/patches_test.go diff --git a/.coverage-baseline b/.coverage-baseline index 219e3542..ddcc0c03 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -75.7 +75.8 diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index de349513..da2b4381 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,34 @@ 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 `patches:` block no longer refuses the folder (next minor; more folders accepted) + +A kustomization declaring `patches:` used to refuse the whole `GitTarget`. Not the edit — the +**target**. A folder whose patch touched a replica count also lost `images:`/`replicas:` +edit-through, which the patch had nothing to do with. + +A patch is now **tolerated as read-only build context**: + +- the folder is **accepted**, and what it renders is mirrored; +- the patch file is **retained, never managed** — it is a build input, not a manifest. (It is a KRM + document, so without this the operator would index it as one: match a live object to it, mirror a + whole Deployment over the sparse patch, or sweep it away as an orphan.) +- `images:` / `replicas:` edit-through works in a patched folder exactly as it does anywhere else; +- an edit to a field **the patch owns** is refused *per object* — `WriteBoundaryRefused`, naming the + file and the object — because authoring a patch is still not supported. + +**Tolerating a patch is not authoring one.** Nothing is ever written into a patch file. + +Exactly one shape is tolerated. The rest are refused **by name**, so the message says what to fix: + +| Shape | Verdict | +|---|---| +| `patches: [{path: patch.yaml}]` — a sparse KRM document inside the tree | **tolerated** | +| `patches: [{patch: "..."}]` — inline (including an inline JSON6902 op list) | refused: `patches-inline` | +| `patches: [{path: json-patch.yaml}]` where the file is an `op`/`path`/`value` list | refused: `patches-json6902` | +| a `path:` naming no file in the tree, or escaping it | refused: `patches-outside-tree` | +| `patchesStrategicMerge:`, `patchesJson6902:` (deprecated spellings) | refused under their own names | + ## Unreleased — the build's output stops leaking into the build's input (next minor; bug fix + behavior change) **If your kustomization declares `labels:`, `commonLabels:`, `commonAnnotations:` or `namespace:`, diff --git a/docs/design/support-boundary/render-root-scoping.md b/docs/design/support-boundary/render-root-scoping.md index 1d93d0a2..14c85e8b 100644 --- a/docs/design/support-boundary/render-root-scoping.md +++ b/docs/design/support-boundary/render-root-scoping.md @@ -291,6 +291,29 @@ Three things the corpus makes visible that no design doc had said out loud: ## 6. Why a patch still blocks the folder — and why it should not +> **Shipped, and the corpus said something the plan did not expect.** +> +> A `patches:` entry naming a strategic-merge document by `path:` is now **tolerated**: the folder +> is accepted, the render is mirrored, the patch file is retained as read-only build context (never +> managed, never mirrored over, never swept), and nothing is routed into it. Inline patches, +> JSON6902, and paths outside the tree are refused **by name**. `images:`/`replicas:` edit-through +> works in a patched folder, which is the point: a patch on a replica count has nothing to do with +> an image tag, and refusing the folder refused both. +> +> **It accepted zero new candidates in the corpus, and that is the finding.** Every patched overlay +> in the corpus *also* reads a base from outside its own folder, so `patches` was **masking the real +> refusal**. `flux-monorepo/apps/{staging,production}` now report +> `overlay-fan-out-unsupported` — the verdict §5 records as *never having been observed*, because +> `refused-structural` always fired first and hid it. The single blocker on the corpus's most +> tractable layout is therefore §4 (render-root scoping), not patches. +> +> The prerequisite named at the end of this section turned out to be a different one, and a +> load-bearing one: **the writer was mirroring the build's own output back into the build's input** +> (§2). A patched base would have absorbed one environment's values, and *no re-render can catch +> that* — the patch re-imposes its value, so the render comes out identical either way. That is +> fixed first ([`sourceForm`](../../../internal/manifestanalyzer/source_form.go)), and tolerating +> patches without it would have been silent corruption. + 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. diff --git a/docs/design/support-boundary/support-contract.md b/docs/design/support-boundary/support-contract.md index d8f6a260..f0d3a377 100644 --- a/docs/design/support-boundary/support-contract.md +++ b/docs/design/support-boundary/support-contract.md @@ -80,7 +80,10 @@ templates, and multi-input ResourceSet templates alike. It is specified once, in | kustomize `resources`, `namespace`, `images`, `replicas` | **Editable** | invertible; `images`/`replicas` edit-through is shipped | | kustomize base + un-fancy overlays | **Designed, not shipped** | render-root scoping; the base stays read-only context | | kustomize base shared by >1 overlay, edited in place | **Refused** | fan-in > 1 | -| kustomize `patches*`, generators, `components`, `namePrefix`/`nameSuffix`, remote bases | **Refused** | non-invertible; the only safe route would be patches the operator authored itself | +| kustomize `patches:` — a strategic-merge document named by `path:` | **Tolerated, not authored** | the folder is accepted and the render is mirrored; the patch is read-only build context. An edit to a field the patch OWNS is refused per object, not per folder | +| kustomize `patches:` — inline, JSON6902, or a path outside the tree | **Refused** | not a sparse KRM document we can read; refused by name | +| kustomize `patchesStrategicMerge`, `patchesJson6902` (deprecated spellings) | **Refused** | kustomize does not fold them into `patches:` (measured), so they refuse under their own names | +| kustomize generators, `components`, `namePrefix`/`nameSuffix`, remote bases | **Refused** | non-invertible | | kustomize `helmCharts:` (inflation) | **Refused** | we never render a chart | | A Helm chart (`Chart.yaml` + `templates/` + `values.yaml` + `crds/`) | **Planned: skipped as a unit** | this needs chart-folder detection; today `scan-repo` can report an incidental `crds/` directory as accepted | | Helm knobs on a `HelmRelease` / `Application` (chart version, inline values, parameters) | **Editable** | the Helm surface people actually use — see below | diff --git a/internal/git/patches_test.go b/internal/git/patches_test.go new file mode 100644 index 00000000..95837cae --- /dev/null +++ b/internal/git/patches_test.go @@ -0,0 +1,129 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// TOLERATE, DON'T AUTHOR — at the commit, which is where it has to be true. +// +// A folder with a patch is now ADOPTED, and the two edit-through channels work in it exactly as +// they do anywhere else: that is the whole point, because a patch on a replica count has nothing +// to do with an image tag, and refusing the folder refused both. +// +// What is still refused is AUTHORING: an edit to a field the patch owns has nowhere to land, and +// the re-render says so rather than committing a write the build overrides straight back. + +// The patch pins spec.replicas. The images: entry supplies the tag. They are unrelated, and until +// now the patch refused the whole GitTarget and took the tag with it. +const patchedKustomizationYAML = `apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +namespace: default +resources: + - apps/deployment.yaml +patches: + - path: apps/replicas-patch.yaml +images: + - name: ghcr.io/example/podinfo + newTag: "6.4.0" +` + +const replicasPatchYAML = `# The production overlay pins the replica count. It is a PARTIAL object. +apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + replicas: 5 +` + +func seedPatchedWorktree(t *testing.T, root string) (string, string, string) { + t.Helper() + deployPath := filepath.Join(root, "apps", "deployment.yaml") + patchPath := filepath.Join(root, "apps", "replicas-patch.yaml") + kustPath := filepath.Join(root, "kustomization.yaml") + require.NoError(t, os.MkdirAll(filepath.Dir(deployPath), 0o750)) + require.NoError(t, os.WriteFile(deployPath, []byte(overridesDeploymentYAML), 0o600)) + require.NoError(t, os.WriteFile(patchPath, []byte(replicasPatchYAML), 0o600)) + require.NoError(t, os.WriteFile(kustPath, []byte(patchedKustomizationYAML), 0o600)) + return deployPath, patchPath, kustPath +} + +// THE MILESTONE, in one test: a folder carrying a patch is adopted, and an image bump still routes +// to the images: entry. The patch is read-only context — it is not touched, and it is not read. +func TestPlanFlush_PatchedFolderStillRoutesAnImageBumpToTheEntry(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + deployPath, patchPath, kustPath := seedPatchedWorktree(t, worktree.Filesystem.Root()) + + // The live object is what the folder renders: the patch's 5 replicas, the entry's 6.4.0 tag — + // with the tag bumped to 6.5.0, which is the user's edit. + changed, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), + overridesDeploymentEvent("ghcr.io/example/podinfo:6.5.0", 5)) + require.NoError(t, err) + require.True(t, changed) + + kust, err := os.ReadFile(kustPath) + require.NoError(t, err) + assert.Contains(t, string(kust), `newTag: "6.5.0"`, "the tag belongs on the entry, patch or no patch") + + assertFileBytes(t, deployPath, overridesDeploymentYAML, + "the source manifest keeps its bytes: neither the tag nor the patched replica count lands here") + assertFileBytes(t, patchPath, replicasPatchYAML, + "and the patch is READ-ONLY context — tolerating it is not authoring it") +} + +// The folder is in sync. The patch says 5 replicas and the cluster runs 5. Nothing to do — and in +// particular the base manifest must not absorb the overlay's 5, which is the corruption that made +// tolerating patches unsafe until the projection stopped writing what the build supplies. +func TestPlanFlush_PatchedFolderInSyncIsANoOp(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + deployPath, patchPath, kustPath := seedPatchedWorktree(t, worktree.Filesystem.Root()) + + changed, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), + overridesDeploymentEvent("ghcr.io/example/podinfo:6.4.0", 5)) + + require.NoError(t, err) + assert.False(t, changed, "the live object is exactly what the folder renders") + assertFileBytes(t, deployPath, overridesDeploymentYAML, + "the patch's replica count is the OVERLAY's, and it must not be baked into the base") + assertFileBytes(t, patchPath, replicasPatchYAML, "nor may the patch itself be rewritten") + assertFileBytes(t, kustPath, patchedKustomizationYAML, "nor the kustomization") +} + +// AND AUTHORING IS STILL REFUSED. Scaling the Deployment is an edit to a field the PATCH owns: +// there is no replicas: entry, the source file cannot hold it (the patch would stamp 5 back on the +// next render), and writing a patch from scratch is not supported. +// +// So the flush is refused, loudly, naming the object — instead of committing a write that would +// leave the resource permanently un-mirrored while looking like it worked. That is the same +// refusal a patch-owned field got before this change; what has changed is that it is now the EDIT +// that is refused, not the whole GitTarget. +func TestPlanFlush_RefusesAnEditToAFieldThePatchOwns(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + deployPath, patchPath, kustPath := seedPatchedWorktree(t, worktree.Filesystem.Root()) + + _, err := flushEventsForTest(t, writer, worktree, deploymentMapper(), + overridesDeploymentEvent("ghcr.io/example/podinfo:6.4.0", 9)) + + var refused *manifestanalyzer.AcceptanceRefusedError + require.ErrorAs(t, err, &refused, "a patch-owned edit must be refused, and refused legibly") + assert.Contains(t, refused.Error(), "Deployment/web", "the refusal names the object") + assert.True(t, refused.AllIssuesOfKinds(manifestanalyzer.IssueRenderRefused), + "the renderer is what refused it, so it surfaces as WriteBoundaryRefused") + + assertFileBytes(t, deployPath, overridesDeploymentYAML, "a refused flush writes nothing") + assertFileBytes(t, patchPath, replicasPatchYAML, "and above all does not try to author the patch") + assertFileBytes(t, kustPath, patchedKustomizationYAML, "and touches no kustomization") +} diff --git a/internal/manifestanalyzer/acceptance.go b/internal/manifestanalyzer/acceptance.go index d4b4c404..3ad895a0 100644 --- a/internal/manifestanalyzer/acceptance.go +++ b/internal/manifestanalyzer/acceptance.go @@ -87,9 +87,14 @@ const ( IssueOutOfScope IssueKind = "out-of-scope" // IssueUnsupportedKustomize marks a retained kustomization.yaml that uses a feature // the contextual-namespace writer cannot map back to editable source documents - // (generators / patches / components / helm / replacements / transformers / - // name(pre|suf)fix / remote bases). The folder is refused rather than written, - // because the operator cannot take responsibility for content produced this way. + // (generators / components / helm / replacements / transformers / name(pre|suf)fix / + // remote bases). The folder is refused rather than written, because the operator cannot + // take responsibility for content produced this way. + // + // `patches:` is NOT on that list any more: a strategic-merge patch named by path is + // tolerated as read-only build context (the render is mirrored, the patch file is never + // managed, and nothing is routed into it). The shapes we cannot read still refuse by name — + // an inline patch, a JSON6902 op list, a path outside the tree. IssueUnsupportedKustomize IssueKind = "unsupported-kustomize" // IssueForeignFile marks a non-YAML regular file under spec.path that matches no // recognized role — the operator-exclusive subtree refuses content it cannot manage @@ -276,7 +281,8 @@ 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), " + + "(generators/components/helm/replacements/transformers/namePrefix/nameSuffix/remote bases, " + + "or a patch that is not a strategic-merge document named by path), " + "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)", diff --git a/internal/manifestanalyzer/kustomization_parse.go b/internal/manifestanalyzer/kustomization_parse.go index 30c0ed1a..ccf02ea8 100644 --- a/internal/manifestanalyzer/kustomization_parse.go +++ b/internal/manifestanalyzer/kustomization_parse.go @@ -8,6 +8,7 @@ import ( "strings" kustypes "sigs.k8s.io/kustomize/api/types" + "sigs.k8s.io/yaml" "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" ) @@ -58,6 +59,18 @@ func supportedKustomizationFields() map[string]struct{} { "Images": {}, "Replicas": {}, + // TOLERATED, NOT AUTHORED. A patch is read-only context: kustomize applies it, we + // mirror what it renders, and nothing is ever routed INTO it. That is a weaker claim + // than the four above, and it is the whole of what "tolerate" means — see + // patchRefusals for the shapes that are still refused by name. + // + // It is only safe because the projection leaves every field the BUILD supplies to the + // build (sourceForm): a patched base is no longer something the writer can absorb one + // environment's values into. Tolerating patches without that is silent corruption, and + // no re-render can catch it — the patch re-imposes its value, so the render comes out + // identical either way. + "Patches": {}, + // These inject metadata into every rendered object. They used to leak into mirrored // source files as drift — the writer mirrored the live object, injected labels and // all, back into the file the overlay renders. That is fixed at the source: the @@ -91,11 +104,39 @@ const ( featureMalformedReplicas = "malformed-replicas" ) +// The patch shapes that are refused BY NAME. `patches:` is tolerated in exactly one shape — a +// `path:` to a sparse KRM document inside the scanned tree — and everything else says so rather +// than falling through into a folder we would then mishandle. +// +// The three of them are not arbitrary. Each is a different kind of thing wearing the same key: +// +// - an INLINE patch is bytes in the kustomization, so there is no document to retain as build +// context and no file an authoring step could ever edit; +// - a JSON6902 patch is not a sparse KRM document at all — it is a list of `op`/`path`/`value` +// operations, and a file full of them would otherwise be indexed as a broken manifest; +// - a path leaving the scanned tree is a file we never read, so we cannot know what it does. +// +// The deprecated spellings need no entry here, and that was MEASURED rather than assumed: +// FixKustomization folds `bases` into `resources` and `imageTags` into `images`, but it does NOT +// fold `patchesStrategicMerge` or `patchesJson6902` into `Patches`. They stay in their own fields, +// land outside supportedKustomizationFields, and refuse the folder under their own names — which +// is what we want, and which a kustomize bump could change. TestParse_DeprecatedPatchSpellings +// pins it. +const ( + featurePatchInline = "patches-inline" + featurePatchJSON6902 = "patches-json6902" + featurePatchOutsideTree = "patches-outside-tree" +) + // parseKustomization decodes one kustomization.yaml and reports every feature the // operator does not model, sorted. An empty slice means the file is fully modelled. // The doc is returned even when unsupported: callers keep it (so it never acts as a // namespace source) rather than dropping it. -func parseKustomization(content []byte, path string) (*kustomizationDoc, []string) { +// +// tree is the scanned file set, needed because a `patches:` entry names a FILE and what that file +// holds decides whether we can tolerate it. A nil tree means the caller has no file set, and every +// patch path is then refused as unreadable rather than assumed benign. +func parseKustomization(content []byte, path string, tree map[string][]byte) (*kustomizationDoc, []string) { doc := &kustomizationDoc{path: path} // Unmarshal then FixKustomization is exactly what kustomize's own loader does @@ -110,7 +151,8 @@ func parseKustomization(content []byte, path string) (*kustomizationDoc, []strin var k kustypes.Kustomization if err := k.Unmarshal(content); err != nil { doc.unsupported = true - return doc, []string{featureUnparseable} + doc.features = []string{featureUnparseable} + return doc, doc.features } k.FixKustomization() @@ -132,6 +174,10 @@ func parseKustomization(content []byte, path string) (*kustomizationDoc, []strin if doc.replicas, ok = replicaOverrides(k.Replicas, path); !ok { features[featureMalformedReplicas] = struct{}{} } + doc.patches = patchPaths(k.Patches, slashDir(path), tree) + for _, refusal := range patchRefusals(k.Patches, slashDir(path), tree) { + features[refusal] = struct{}{} + } out := make([]string, 0, len(features)) for f := range features { @@ -139,9 +185,99 @@ func parseKustomization(content []byte, path string) (*kustomizationDoc, []strin } sort.Strings(out) doc.unsupported = len(out) > 0 + doc.features = out return doc, out } +// patchRefusals names every patch entry the operator will not tolerate. See the feature constants +// for why each shape is its own answer rather than a generic "unsupported". +func patchRefusals(entries []kustypes.Patch, dir string, tree map[string][]byte) []string { + var out []string + for _, entry := range entries { + switch { + case strings.TrimSpace(entry.Patch) != "": + // Inline bytes, and this is also where an inline JSON6902 op list arrives — + // `patches: [{patch: "- op: replace ...", target: {...}}]` decodes into exactly + // this field, so refusing Patch outright refuses both spellings at once. + out = append(out, featurePatchInline) + case !patchFileIsSparseKRM(entry.Path, dir, tree): + // The file is missing, unreadable, escapes the tree, or is not a KRM document — + // a JSON6902 op list being the shape that most looks like a patch and least is one. + out = append(out, patchPathRefusal(entry.Path, dir, tree)) + } + } + return out +} + +// patchPathRefusal distinguishes "we cannot read that file" from "that file is not a patch we can +// tolerate", because they are different things for a user to fix. +func patchPathRefusal(entryPath, dir string, tree map[string][]byte) string { + if resolvePatchPath(entryPath, dir, tree) == "" { + return featurePatchOutsideTree + } + return featurePatchJSON6902 +} + +// patchPaths is the set of files this kustomization reads as patches — build inputs, never +// resources. They are retained outside the managed model exactly as kustomization.yaml is: a +// strategic-merge patch IS a KRM document, so nothing else stops the store from indexing it as a +// manifest, mirroring a live object over it, or sweeping it away as an orphan. +// +// That a patch produces no object of its own is not our claim — it is the RENDER's: a patch file +// never appears as a rendered object's origin. TestRetain_PatchFileIsNeverARenderOrigin pins it. +func patchPaths(entries []kustypes.Patch, dir string, tree map[string][]byte) []string { + var out []string + for _, entry := range entries { + if strings.TrimSpace(entry.Patch) != "" { + continue // inline: no file to retain, and refused anyway + } + if !patchFileIsSparseKRM(entry.Path, dir, tree) { + continue // refused; retaining it would hide the very file the refusal names + } + out = append(out, resolvePatchPath(entry.Path, dir, tree)) + } + sort.Strings(out) + return out +} + +// patchFileIsSparseKRM reports whether the file a patches: entry names is one we can tolerate: a +// readable document inside the scanned tree carrying an apiVersion and a kind. +// +// A sparse strategic-merge patch is a KRM document with most of its fields missing, so apiVersion +// + kind is the whole test — the fields it does carry are the patch. A JSON6902 op list decodes as +// a YAML SEQUENCE, so it fails this and is refused by name. +func patchFileIsSparseKRM(entryPath, dir string, tree map[string][]byte) bool { + resolved := resolvePatchPath(entryPath, dir, tree) + if resolved == "" { + return false + } + var doc struct { + APIVersion string `json:"apiVersion"` + Kind string `json:"kind"` + } + if err := yaml.Unmarshal(tree[resolved], &doc); err != nil { + return false + } + return doc.APIVersion != "" && doc.Kind != "" +} + +// resolvePatchPath resolves a patches: entry's path against the kustomization's own directory, +// returning "" when it is empty, remote, escapes the scanned tree, or names no file in it. +func resolvePatchPath(entryPath, dir string, tree map[string][]byte) string { + entryPath = strings.TrimSpace(entryPath) + if entryPath == "" || isRemoteResource(entryPath) { + return "" + } + resolved := cleanJoin(dir, entryPath) + if resolved == "" { + return "" + } + if _, found := tree[resolved]; !found { + return "" + } + return resolved +} + // kustomizationDecodeError returns kustomize's own decode error for a file it // cannot build, or "" when it decodes. It is what makes an `unparseable` refusal // actionable: "your kustomization.yaml is a Flux Kustomization CR" and "resources: @@ -253,30 +389,47 @@ func trimmedEntries(lists ...[]string) []string { // parseKustomizations reads every kustomization.yaml into a kustomizationDoc keyed // by its directory. An unparseable kustomization, or one using an unsupported // feature, is kept but marked unsupported so it never acts as a namespace source. +// +// It is the ONE place a kustomization is judged, and it is file-aware because it has to be: a +// `patches:` entry names a file, and what that file holds — a sparse KRM document, or a JSON6902 +// op list, or nothing at all — is what decides whether the folder can be tolerated. Every consumer +// (the acceptance gate, the repo scan, the namespace walk) reads the doc this produces, so no two +// of them can drift on what "unsupported" means. func parseKustomizations(files []manifestedit.FileContent) map[string]*kustomizationDoc { + tree := contentByPath(files) out := map[string]*kustomizationDoc{} for _, f := range files { if !isKustomizationFile(f.Path) { continue } - doc, _ := parseKustomization(f.Content, filepathToSlash(f.Path)) + doc, _ := parseKustomization(f.Content, filepathToSlash(f.Path), tree) out[slashDir(f.Path)] = doc } return out } -// kustomizationUsesUnsupportedFeature reports whether a kustomization.yaml uses a -// feature outside the modelled subset — the predicate the acceptance gate uses to -// refuse the folder at the retention site. -func kustomizationUsesUnsupportedFeature(content []byte) bool { - _, features := parseKustomization(content, "") - return len(features) > 0 +// contentByPath indexes the scan by slash path, so a kustomization can be judged against the +// files it names. +func contentByPath(files []manifestedit.FileContent) map[string][]byte { + out := make(map[string][]byte, len(files)) + for _, f := range files { + out[filepathToSlash(f.Path)] = f.Content + } + return out } -// unsupportedKustomizeFeatures names the features a kustomization declares that the -// operator does not model, for the repo scan's per-candidate refusal detail. It is -// the same parse the acceptance gate runs, so the two cannot drift. -func unsupportedKustomizeFeatures(content []byte) []string { - _, features := parseKustomization(content, "") - return features +// patchFilesOf is every file any kustomization in the scan reads as a patch. They are build +// inputs, and the store retains them outside the managed model rather than treating a sparse +// patch as a manifest it may mirror over or sweep away. +func patchFilesOf(kusts map[string]*kustomizationDoc) map[string]struct{} { + out := map[string]struct{}{} + for _, doc := range kusts { + if doc.unsupported { + continue // a refused kustomization's patches are not build context, they are the refusal + } + for _, path := range doc.patches { + out[path] = struct{}{} + } + } + return out } diff --git a/internal/manifestanalyzer/overrides_test.go b/internal/manifestanalyzer/overrides_test.go index d3a02011..3726c3c8 100644 --- a/internal/manifestanalyzer/overrides_test.go +++ b/internal/manifestanalyzer/overrides_test.go @@ -227,8 +227,9 @@ func TestKustomizationOverrideParsing(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - if got := kustomizationUsesUnsupportedFeature([]byte(tc.content)); got != tc.unsupported { - t.Errorf("kustomizationUsesUnsupportedFeature = %v, want %v", got, tc.unsupported) + _, features := parseKustomization([]byte(tc.content), "kustomization.yaml", nil) + if got := len(features) > 0; got != tc.unsupported { + t.Errorf("unsupported = %v (%v), want %v", got, features, tc.unsupported) } }) } diff --git a/internal/manifestanalyzer/patches_test.go b/internal/manifestanalyzer/patches_test.go new file mode 100644 index 00000000..81e58481 --- /dev/null +++ b/internal/manifestanalyzer/patches_test.go @@ -0,0 +1,219 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" +) + +// TOLERATING A PATCH IS NOT AUTHORING ONE, and these are the two halves of that sentence. +// +// Tolerate: the folder is accepted, the render is mirrored, and the patch file is read-only build +// context — never a manifest, never mirrored over, never swept. +// +// Author: still refused. Nothing is routed INTO a patch, so an edit to a field a patch owns has +// nowhere to land, and the re-render refuses the flush rather than writing a value the build will +// override straight back. + +const simplePatchYAML = `apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + replicas: 4 +` + +// patchedTree is the shape this milestone accepts: one base, one strategic-merge patch named by +// path, nothing else. +func patchedTree(kustomization string) []manifestedit.FileContent { + return []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("ghcr.io/example/app:1.0.0", "1")), + file("patch.yaml", simplePatchYAML), + file("kustomization.yaml", kustomization), + } +} + +func featuresOf(t *testing.T, files []manifestedit.FileContent) []string { + t.Helper() + doc := parseKustomizations(files)["."] + require.NotNil(t, doc, "the fixture must carry a kustomization at its root") + return doc.features +} + +// The one shape we tolerate: a path to a sparse KRM document inside the tree. +func TestPatches_ASimpleStrategicMergePatchIsTolerated(t *testing.T) { + files := patchedTree("resources:\n - deployment.yaml\npatches:\n - path: patch.yaml\n") + + require.Empty(t, featuresOf(t, files), "a patch by path is read-only context, not a refusal") + + store := BuildStoreFromFiles(context.Background(), files, nil, WriterAllowlist()) + require.True(t, AcceptStructureOnly(store).Accepted, "and the folder is adopted") +} + +// Every other shape is refused BY NAME. "unsupported" on its own is not something a user can act +// on; "your patch is inline, and we can only read one from a file" is. +func TestPatches_EveryOtherShapeIsRefusedByName(t *testing.T) { + cases := []struct { + name string + files []manifestedit.FileContent + refusal string + }{{ + name: "inline strategic-merge patch", + files: patchedTree(`resources: + - deployment.yaml +patches: + - patch: | + apiVersion: apps/v1 + kind: Deployment + metadata: + name: web + spec: + replicas: 4 +`), + refusal: featurePatchInline, + }, { + // A JSON6902 patch hides inside the SAME key as a strategic merge, and decodes into the + // same struct. Refusing an inline `patch:` outright is what catches it. + name: "inline JSON6902 op list", + files: patchedTree(`resources: + - deployment.yaml +patches: + - target: + kind: Deployment + name: web + patch: |- + - op: replace + path: /spec/replicas + value: 4 +`), + refusal: featurePatchInline, + }, { + // And by path it is a YAML SEQUENCE, not a KRM document — so it carries no apiVersion and + // no kind, and it would otherwise be indexed as a broken manifest. + name: "JSON6902 op list by path", + files: []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("ghcr.io/example/app:1.0.0", "1")), + file("patch.yaml", "- op: replace\n path: /spec/replicas\n value: 4\n"), + file("kustomization.yaml", `resources: + - deployment.yaml +patches: + - path: patch.yaml + target: + kind: Deployment + name: web +`), + }, + refusal: featurePatchJSON6902, + }, { + name: "a path naming no file in the tree", + files: patchedTree("resources:\n - deployment.yaml\npatches:\n - path: nowhere.yaml\n"), + refusal: featurePatchOutsideTree, + }, { + name: "a path escaping the scanned tree", + files: patchedTree("resources:\n - deployment.yaml\npatches:\n - path: ../shared/patch.yaml\n"), + refusal: featurePatchOutsideTree, + }} + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, []string{tc.refusal}, featuresOf(t, tc.files)) + + store := BuildStoreFromFiles(context.Background(), tc.files, nil, WriterAllowlist()) + require.False(t, AcceptStructureOnly(store).Accepted, "and the folder is refused") + }) + } +} + +// MEASURED, NOT ASSUMED. FixKustomization folds `bases` into `resources` and `imageTags` into +// `images` — so it is easy to believe it folds the deprecated patch spellings into `patches` too, +// and to then tolerate them by accident. It does not: they stay in their own fields, land outside +// the modelled set, and refuse the folder under their own names. +// +// This is the test that fails if a kustomize bump ever starts folding them, which would otherwise +// silently widen what we accept to shapes we have never looked at. +func TestPatches_DeprecatedSpellingsAreNotFoldedIntoPatches(t *testing.T) { + for _, tc := range []struct { + name string + body string + refusal string + }{{ + name: "patchesStrategicMerge", + body: "patchesStrategicMerge:\n - patch.yaml\n", + refusal: "patchesStrategicMerge", + }, { + name: "patchesJson6902", + body: `patchesJson6902: + - target: + group: apps + version: v1 + kind: Deployment + name: web + path: patch.yaml +`, + refusal: "patchesJson6902", + }} { + t.Run(tc.name, func(t *testing.T) { + files := patchedTree("resources:\n - deployment.yaml\n" + tc.body) + require.Equal(t, []string{tc.refusal}, featuresOf(t, files), + "the deprecated spelling must refuse under its OWN name, not arrive disguised as patches:") + }) + } +} + +// A PATCH FILE IS A BUILD INPUT, NOT A MANIFEST. +// +// It is a KRM document — apiVersion, kind, metadata.name — so every other part of the store would +// happily treat it as one: index it, match a live object to it, mirror a whole Deployment over the +// sparse patch, or sweep it away as an orphan nothing in the cluster answers to. It is retained +// instead, exactly as kustomization.yaml is. +func TestPatches_ThePatchFileIsRetainedNotManaged(t *testing.T) { + files := patchedTree("resources:\n - deployment.yaml\npatches:\n - path: patch.yaml\n") + store := BuildStoreFromFiles(context.Background(), files, nil, WriterAllowlist()) + + require.NotContains(t, store.FilesByPath, "patch.yaml", + "a managed patch file is a live object waiting to be written over it") + require.Contains(t, store.FilesByPath, "deployment.yaml", "the base is still managed") + + var retained []string + for _, rd := range store.Retained { + retained = append(retained, rd.Location.Path) + require.Empty(t, rd.Identity.Name, + "a patch must not be retained WITH an identity: that is the mixed-file refusal, "+ + "and it would refuse the folder for containing exactly what it should") + } + require.ElementsMatch(t, []string{"kustomization.yaml", "patch.yaml"}, retained) +} + +// And that a patch is not a resource is not OUR claim — it is the RENDER's. kustomize produces one +// object here, from the base; the patch file never appears as an origin. If that ever stopped +// being true, retaining it would be hiding a resource. +func TestPatches_ThePatchFileIsNeverARenderOrigin(t *testing.T) { + files := patchedTree("resources:\n - deployment.yaml\npatches:\n - path: patch.yaml\n") + + rendered, err := renderRoot(files, ".") + require.NoError(t, err) + require.Len(t, rendered, 1) + require.Equal(t, "deployment.yaml", rendered[0].OriginPath) +} + +// A duplicate of the base, dressed as a patch, is still refused — retention follows the +// kustomization's `patches:` entries, not a filename convention, so nothing can smuggle a manifest +// out of the managed set by being called a patch. +func TestPatches_AnUnreferencedPatchLookingFileIsStillAManifest(t *testing.T) { + files := []manifestedit.FileContent{ + file("deployment.yaml", deploymentSource("ghcr.io/example/app:1.0.0", "1")), + file("patch.yaml", simplePatchYAML), // named by nothing + file("kustomization.yaml", "resources:\n - deployment.yaml\n"), + } + store := BuildStoreFromFiles(context.Background(), files, nil, WriterAllowlist()) + + require.Contains(t, store.FilesByPath, "patch.yaml", + "nothing reads this file as a patch, so it is what it looks like: a manifest") + require.False(t, AcceptStructureOnly(store).Accepted, + "and it duplicates the base's identity, which is refused") +} diff --git a/internal/manifestanalyzer/scan_repo.go b/internal/manifestanalyzer/scan_repo.go index dc414c57..f3573f0c 100644 --- a/internal/manifestanalyzer/scan_repo.go +++ b/internal/manifestanalyzer/scan_repo.go @@ -199,7 +199,7 @@ func scanRepoFS(ctx context.Context, fsys fs.FS) RepoReport { return RepoReport{ Candidates: candidates, - Summary: summarize(candidates, fsys, kustContent), + Summary: summarize(candidates, fsys, kusts), } } @@ -224,7 +224,7 @@ func classifyRenderRoot( c.AcceptedByOperator = false c.RefusalReasons = []RefusalReason{{ Code: ReasonRefusedStructural, - Detail: refusedStructuralDetail(kustContent[rootDir]), + Detail: refusedStructuralDetail(kusts[rootDir], kustContent[rootDir]), }} c.Resources = countResources(store, rootDir, rendered) return c @@ -440,9 +440,13 @@ func overlayFanOutDetail(base string, kusts map[string]*kustomizationDoc) string } // refusedStructuralDetail names the specific unsupported kustomize features so the -// refusal is actionable, not a bare "refused". -func refusedStructuralDetail(content []byte) string { - features := unsupportedKustomizeFeatures(content) +// refusal is actionable, not a bare "refused". The features come off the parsed doc, which is +// the same judgement the acceptance gate reads, so the scan and the operator cannot drift. +func refusedStructuralDetail(doc *kustomizationDoc, content []byte) string { + var features []string + if doc != nil { + features = doc.features + } if len(features) == 0 { return "kustomization uses an unsupported feature the operator cannot map back to editable source" } @@ -592,7 +596,7 @@ func detectOverlaps(candidates []RepoCandidate) { // signal read from the repo's top-level directories. Unsupported constructs are // recomputed from each refused-structural candidate's kustomization bytes, so the // summary shares one source of truth with the per-candidate detail. -func summarize(candidates []RepoCandidate, fsys fs.FS, kustContent map[string][]byte) RepoSummary { +func summarize(candidates []RepoCandidate, fsys fs.FS, kusts map[string]*kustomizationDoc) RepoSummary { s := RepoSummary{CandidatesByLayout: map[Layout]int{}} constructs := map[string]struct{}{} for _, c := range candidates { @@ -602,8 +606,8 @@ func summarize(candidates []RepoCandidate, fsys fs.FS, kustContent map[string][] } else { s.Refused++ } - if c.Layout == LayoutRefusedStructural { - for _, f := range unsupportedKustomizeFeatures(kustContent[c.Path]) { + if doc := kusts[c.Path]; c.Layout == LayoutRefusedStructural && doc != nil { + for _, f := range doc.features { constructs[f] = struct{}{} } } diff --git a/internal/manifestanalyzer/store.go b/internal/manifestanalyzer/store.go index 12acdf46..9ded07fc 100644 --- a/internal/manifestanalyzer/store.go +++ b/internal/manifestanalyzer/store.go @@ -400,23 +400,13 @@ func buildStore( Kustomizations: kustomizationInfos(kusts), } - // inv.Records are exactly the KRM documents (editable or not), in stable scan - // order (path, then document index), so each managed file's Documents slice is - // built in document order and first-occurrence-wins is deterministic. - hasNamedRecord := map[string]bool{} - for _, r := range inv.Records { - if allowlist.Allows(r.Location.Path) { - // A named KRM record inside an allowlisted build-directive file (a managed - // resource hiding in kustomization.yaml). We must not silently un-manage it, - // so retain it WITH its identity for the mixed-file refusal; never materialise. - hasNamedRecord[r.Location.Path] = true - store.Retained = append(store.Retained, RetainedDocument{ - Location: r.Location, Identity: r.Identity, GVK: gvkOf(r.Identity), - }) - continue - } - store.materialize(ctx, r, lookup, nsAssignments, ovAssignments) - } + hasNamedRecord := store.materializeRecords(ctx, inv.Records, materializeInputs{ + lookup: lookup, + allowlist: allowlist, + patchFiles: patchFilesOf(kusts), + nsAssignments: nsAssignments, + ovAssignments: ovAssignments, + }) // Record every allowlisted file with no named record as a whole-file retention, // so it is known to acceptance (and shown) but never becomes a FileModel. @@ -428,10 +418,11 @@ func buildStore( // 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)] + doc := kusts[slashDir(f.Path)] store.Retained = append(store.Retained, RetainedDocument{ Location: manifestedit.Location{Path: f.Path}, Unsupported: isKustomizationFile(f.Path) && - (kustomizationUsesUnsupportedFeature(f.Content) || buildFailed), + ((doc != nil && doc.unsupported) || buildFailed), }) } } @@ -495,6 +486,63 @@ func (s *ManifestStore) DocumentLocations() map[*DocumentModel]RecordRef { return documentLocations(s) } +// materializeInputs are the scan-wide facts every record is judged against. +type materializeInputs struct { + lookup typeset.Lookup + allowlist Allowlist + // patchFiles are the files some kustomization reads as a patch. + patchFiles map[string]struct{} + nsAssignments map[string]namespaceAssignment + ovAssignments map[chainKey]*overrideAssignment +} + +// materializeRecords sorts every KRM document into one of three fates — retained as a build +// directive, retained as a patch, or materialised as a managed manifest — and returns the files +// that were retained rather than managed. +// +// records arrive in stable scan order (path, then document index), so each managed file's +// Documents slice is built in document order and first-occurrence-wins is deterministic. +// +// A PATCH FILE IS A BUILD INPUT, NOT A MANIFEST, and nothing else in the store would know that: +// a strategic-merge patch IS a KRM document. Materialised, it would be indexed as a manifest, +// matched to a live object, mirrored over (a whole Deployment written where a sparse patch used to +// be), or swept as an orphan when nothing in the cluster answers to it. It is retained exactly as +// kustomization.yaml is: known, never managed. +func (s *ManifestStore) materializeRecords( + ctx context.Context, + records []manifestedit.DocumentRecord, + in materializeInputs, +) map[string]bool { + retained := map[string]bool{} + for _, r := range records { + switch { + case in.allowlist.Allows(r.Location.Path): + // A named KRM record inside an allowlisted build-directive file (a managed + // resource hiding in kustomization.yaml). We must not silently un-manage it, + // so retain it WITH its identity for the mixed-file refusal; never materialise. + retained[r.Location.Path] = true + s.Retained = append(s.Retained, RetainedDocument{ + Location: r.Location, Identity: r.Identity, GVK: gvkOf(r.Identity), + }) + case isPatchFile(in.patchFiles, r.Location.Path): + // A patch's identity is NOT retained, and that is the difference from a resource + // hiding in a kustomization: this document is not a resource that must be refused, + // it is a patch doing exactly its job. Naming it would refuse the folder as a mixed + // file for holding precisely what it is supposed to hold. One retention per file, + // however many documents the patch carries. + if !retained[r.Location.Path] { + s.Retained = append(s.Retained, RetainedDocument{ + Location: manifestedit.Location{Path: r.Location.Path}, + }) + } + retained[r.Location.Path] = true + default: + s.materialize(ctx, r, in.lookup, in.nsAssignments, in.ovAssignments) + } + } + return retained +} + // materialize adds one managed KRM record to the store: its FileModel, the GVK // index, the resolved mapping, and the first-occurrence-wins identity indexes. func (s *ManifestStore) materialize( @@ -687,12 +735,18 @@ func kustomizationInfos(kusts map[string]*kustomizationDoc) map[string]*Kustomiz // a namespace source). See the "Kustomize subset proposal" in // docs/spec/contextual-namespace-and-kustomize-folder-editing.md. type kustomizationDoc struct { - path string // kustomization file path (slash) - namespace string // the namespace: transformer value - resources []string // resources + bases entries, raw and relative to the file's dir - images []ImageOverride // parsed images: entries, in listed order - replicas []ReplicaOverride // parsed replicas: entries, in listed order - unsupported bool // uses generators/patches/components/remote bases/name(pre|suf)fix/... + path string // kustomization file path (slash) + namespace string // the namespace: transformer value + resources []string // resources + bases entries, raw and relative to the file's dir + images []ImageOverride // parsed images: entries, in listed order + replicas []ReplicaOverride // parsed replicas: entries, in listed order + // patches are the files this kustomization reads as strategic-merge patches, resolved against + // the scan root. They are BUILD INPUTS, not resources: retained outside the managed model, so + // no live object is ever mirrored over one and no sweep ever deletes one. + patches []string + // features names every unmodelled feature, sorted — the refusal, in the user's own words. + features []string + unsupported bool // uses generators/components/remote bases/name(pre|suf)fix/an unauthorable patch/... } // kustomizeNamespaceAssignments walks each supported kustomization as a render root and @@ -861,6 +915,12 @@ func filepathToSlash(filePath string) string { return strings.ReplaceAll(filePath, "\\", "/") } +// isPatchFile reports whether a scanned file is read by some kustomization as a patch. +func isPatchFile(patchFiles map[string]struct{}, filePath string) bool { + _, found := patchFiles[filepathToSlash(filePath)] + return found +} + func isKustomizationFile(filePath string) bool { switch path.Base(filepathToSlash(filePath)) { case "kustomization.yaml", "kustomization.yml": diff --git a/test/fixtures/gitops-layouts/support-today.md b/test/fixtures/gitops-layouts/support-today.md index e61d06be..b1046ded 100644 --- a/test/fixtures/gitops-layouts/support-today.md +++ b/test/fixtures/gitops-layouts/support-today.md @@ -26,12 +26,12 @@ Reading rules: |---|---:|---|---:|---:|---|---|---| | 1-desired-state/argocd-app-of-apps | 0 | All reported candidates accepted | 4 | 0 | plain=4 | - | None | | 1-desired-state/argocd-plain | 0 | Partial | 1 | 1 | plain=2 | - | non-krm-yaml: ci-metadata.yaml: YAML is not a Kubernetes manifest | -| 1-desired-state/flux-monorepo | 0 | Partial | 4 | 2 | kustomize-single=4, refused-structural=2 | patches | refused-structural: kustomization uses unsupported feature(s): patches
refused-structural: kustomization uses unsupported feature(s): patches | +| 1-desired-state/flux-monorepo | 0 | Partial | 4 | 2 | kustomize-overlay=2, kustomize-single=4 | - | overlay-fan-out-unsupported: base "apps/base/frontend" is read from outside this folder's subtree and is shared by 2 render root(s); render-root scoping required
overlay-fan-out-unsupported: base "apps/base/frontend" is read from outside this folder's subtree and is shared by 2 render root(s); render-root scoping required | | 1-desired-state/repo-per-environment | 0 | Partial | 6 | 3 | plain=9 | - | foreign-file: .gitignore: foreign file .gitignore is not a managed manifest; remove it or name it in .gittargetignore
foreign-file: .gitignore: foreign file .gitignore is not a managed manifest; remove it or name it in .gittargetignore
foreign-file: .gitignore: foreign file .gitignore is not a managed manifest; remove it or name it in .gittargetignore | | 2-rendered/argocd-external-helm | 0 | Partial | 2 | 1 | plain=3 | - | non-krm-yaml: values.yaml: YAML is not a Kubernetes manifest | | 2-rendered/helm-chart | 0 | All reported candidates accepted | 1 | 0 | plain=1 | - | None | | 2-rendered/helm-environment-values | 0 | All reported candidates accepted | 1 | 0 | plain=1 | - | None | -| 2-rendered/kustomize-overlays | 0 | Partial | 1 | 3 | kustomize-single=1, refused-structural=3 | configMapGenerator, namePrefix, nameSuffix, patches, remote-base, secretGenerator | refused-structural: kustomization uses unsupported feature(s): remote-base
refused-structural: kustomization uses unsupported feature(s): configMapGenerator, nameSuffix, patches, secretGenerator
refused-structural: kustomization uses unsupported feature(s): configMapGenerator, namePrefix, patches | +| 2-rendered/kustomize-overlays | 0 | Partial | 1 | 3 | kustomize-single=1, refused-structural=3 | configMapGenerator, namePrefix, nameSuffix, remote-base, secretGenerator | refused-structural: kustomization uses unsupported feature(s): remote-base
refused-structural: kustomization uses unsupported feature(s): configMapGenerator, nameSuffix, secretGenerator
refused-structural: kustomization uses unsupported feature(s): configMapGenerator, namePrefix | | 2-rendered/rendered-manifests | 0 | Partial | 3 | 2 | plain=3, refused-structural=2 | namePrefix | refused-structural: kustomization uses unsupported feature(s): namePrefix
refused-structural: kustomization uses unsupported feature(s): namePrefix | | 3-expanded/argocd-applicationset-directories | 0 | All reported candidates accepted | 5 | 0 | plain=5 | - | None | | 3-expanded/argocd-applicationset-files | 0 | No reported candidates accepted | 0 | 1 | plain=1 | - | non-krm-yaml: chart/Chart.yaml: YAML is not a Kubernetes manifest
foreign-file: chart/templates/_helpers.tpl: foreign file chart/templates/_helpers.tpl is not a managed manifest; remove it or name it in .gittargetignore
non-krm-yaml: chart/templates/deployment.yaml: YAML is not a Kubernetes manifest
non-krm-yaml: chart/templates/service.yaml: YAML is not a Kubernetes manifest
+5 more | @@ -68,12 +68,12 @@ Unsupported constructs: `none`. Fleet root: `false`. ## 1-desired-state/flux-monorepo Reported rc `0`. Accepted `4`, refused `2`. -Unsupported constructs: `patches`. Fleet root: `false`. +Unsupported constructs: `none`. Fleet root: `false`. | Candidate | Layout | Accepted today | Namespace | rendered/editable/non-KRM | Refusal reasons | |---|---|---|---|---|---| -| `apps/production` | `refused-structural` | false | `production` | 2/0/0 | refused-structural: kustomization uses unsupported feature(s): patches | -| `apps/staging` | `refused-structural` | false | `staging` | 2/0/0 | refused-structural: kustomization uses unsupported feature(s): patches | +| `apps/production` | `kustomize-overlay` | false | `production` | 2/0/0 | overlay-fan-out-unsupported: base "apps/base/frontend" is read from outside this folder's subtree and is shared by 2 render root(s); render-root scoping required | +| `apps/staging` | `kustomize-overlay` | false | `staging` | 2/0/0 | overlay-fan-out-unsupported: base "apps/base/frontend" is read from outside this folder's subtree and is shared by 2 render root(s); render-root scoping required | | `clusters/production` | `kustomize-single` | true | `flux-system` | 7/7/0 | none | | `clusters/staging` | `kustomize-single` | true | `flux-system` | 7/7/0 | none | | `infrastructure/configs` | `kustomize-single` | true | `-` | 1/1/0 | none | @@ -128,14 +128,14 @@ Unsupported constructs: `none`. Fleet root: `false`. ## 2-rendered/kustomize-overlays Reported rc `0`. Accepted `1`, refused `3`. -Unsupported constructs: `configMapGenerator, namePrefix, nameSuffix, patches, remote-base, secretGenerator`. Fleet root: `false`. +Unsupported constructs: `configMapGenerator, namePrefix, nameSuffix, remote-base, secretGenerator`. Fleet root: `false`. | Candidate | Layout | Accepted today | Namespace | rendered/editable/non-KRM | Refusal reasons | |---|---|---|---|---|---| | `apps/backend/base` | `kustomize-single` | true | `-` | 2/2/0 | none | | `apps/backend/overlays/production` | `refused-structural` | false | `backend-production` | 0/0/0 | refused-structural: kustomization uses unsupported feature(s): remote-base | -| `apps/frontend/overlays/production` | `refused-structural` | false | `frontend-production` | 2/0/3 | refused-structural: kustomization uses unsupported feature(s): configMapGenerator, nameSuffix, patches, secretGenerator | -| `apps/frontend/overlays/staging` | `refused-structural` | false | `frontend-staging` | 2/0/1 | refused-structural: kustomization uses unsupported feature(s): configMapGenerator, namePrefix, patches | +| `apps/frontend/overlays/production` | `refused-structural` | false | `frontend-production` | 2/0/3 | refused-structural: kustomization uses unsupported feature(s): configMapGenerator, nameSuffix, secretGenerator | +| `apps/frontend/overlays/staging` | `refused-structural` | false | `frontend-staging` | 2/0/1 | refused-structural: kustomization uses unsupported feature(s): configMapGenerator, namePrefix | ## 2-rendered/rendered-manifests From 77fcf738c795ab2f7e8323c19f306ae18ee744c9 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Tue, 14 Jul 2026 21:16:41 +0000 Subject: [PATCH 5/5] docs(kustomize): drop what tolerating patches made untrue The prompt's stage 1 shipped, and three of its premises did not survive contact with the measurement: the gate was the projection rather than the deny-list, FixKustomization does not fold the deprecated spellings, and tolerating patches accepted zero new corpus candidates because every patched overlay in the corpus is really blocked on render-root scoping. --- .../next-prompt-simple-patches.md | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/docs/design/support-boundary/next-prompt-simple-patches.md b/docs/design/support-boundary/next-prompt-simple-patches.md index 8b84f07c..0fe6b73e 100644 --- a/docs/design/support-boundary/next-prompt-simple-patches.md +++ b/docs/design/support-boundary/next-prompt-simple-patches.md @@ -1,5 +1,26 @@ # Prompt: tolerate `patches:`, and route a simple one +> **Stage 1 shipped (#235), and stage 0 turned out to exist (#234).** Read this before using the +> prompt below, because three of its premises are now wrong: +> +> - **The gate was not the deny-list, it was the projection.** The writer was mirroring the build's +> own output back into the build's input — measured on `labels:`/`commonAnnotations:`, which we +> accept *today*. A patched base would have absorbed one environment's values, and **no re-render +> catches that** (the patch re-imposes its value, so the render is identical either way). Fixed +> first: *where the live object and the render agree, the source keeps its bytes* +> ([`sourceForm`](../../../internal/manifestanalyzer/source_form.go)). +> - **`FixKustomization` does NOT fold the deprecated spellings** into `Patches` (the prompt below +> suspects it does). Measured; pinned by a test. +> - **Tolerating patches accepted zero new corpus candidates.** Every patched overlay in the corpus +> *also* reads a base from outside its own folder, so `patches` was masking the real refusal. +> `flux-monorepo` now reports `overlay-fan-out-unsupported`. **Render-root scoping, not patches, +> is the blocker on the corpus's best layout.** +> +> Also measured, and load-bearing for stage 3: kustomize's strategic merge **prepends** a container +> a patch adds, so aligning a source list with the render by *position* is wrong, not merely +> fragile. Stages 2–4 (attribute a patch scalar, route into the patch, per-field accounting) are +> still open, and the sections below still describe them correctly. + Copy everything below the line into a fresh session. ---