diff --git a/.coverage-baseline b/.coverage-baseline index 427656bc..ddcc0c03 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -75.6 +75.8 diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 27aacd38..da2b4381 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,80 @@ 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:`, +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/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/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. --- 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. diff --git a/docs/design/support-boundary/render-root-scoping.md b/docs/design/support-boundary/render-root-scoping.md index fcb292e0..14c85e8b 100644 --- a/docs/design/support-boundary/render-root-scoping.md +++ b/docs/design/support-boundary/render-root-scoping.md @@ -85,22 +85,38 @@ 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**. -- **`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. +> **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 @@ -275,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/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..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 @@ -134,6 +139,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 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 + // 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 @@ -264,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/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..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,10 +59,23 @@ 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. + // 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 + // 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": {}, @@ -90,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 @@ -109,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() @@ -131,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 { @@ -138,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: @@ -252,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/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/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/source_form.go b/internal/manifestanalyzer/source_form.go new file mode 100644 index 00000000..dda21c4e --- /dev/null +++ b/internal/manifestanalyzer/source_form.go @@ -0,0 +1,327 @@ +// 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 + + ", 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 +// 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/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/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: 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