diff --git a/.coverage-baseline b/.coverage-baseline index 427656bca..6bc1e9f99 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -75.6 +75.9 diff --git a/api/v1alpha3/gittarget_types.go b/api/v1alpha3/gittarget_types.go index 9a35572c1..ec3a5fb18 100644 --- a/api/v1alpha3/gittarget_types.go +++ b/api/v1alpha3/gittarget_types.go @@ -178,6 +178,7 @@ type GitTargetStreamsStatus struct { // +kubebuilder:printcolumn:name="Reason",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].reason` // +kubebuilder:printcolumn:name="Streams",type=string,JSONPath=`.status.streams.summary` // +kubebuilder:printcolumn:name="GitPathAccepted",type=string,JSONPath=`.status.conditions[?(@.type=="GitPathAccepted")].status`,priority=1 +// +kubebuilder:printcolumn:name="RenderMatchesLive",type=string,JSONPath=`.status.conditions[?(@.type=="RenderMatchesLive")].status`,priority=1 // +kubebuilder:printcolumn:name="StreamsRunning",type=string,JSONPath=`.status.conditions[?(@.type=="StreamsRunning")].status`,priority=1 // +kubebuilder:printcolumn:name="Status",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].message`,priority=1 // +kubebuilder:printcolumn:name="Encryption",type=string,JSONPath=`.spec.encryption.provider`,priority=1 diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index dbdaac72c..0bc299138 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -37,6 +37,10 @@ spec: name: GitPathAccepted priority: 1 type: string + - jsonPath: .status.conditions[?(@.type=="RenderMatchesLive")].status + name: RenderMatchesLive + priority: 1 + type: string - jsonPath: .status.conditions[?(@.type=="StreamsRunning")].status name: StreamsRunning priority: 1 diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 27aacd380..de349513c 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,52 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## Unreleased — the build's output stops leaking into the build's input (next minor; bug fix + behavior change) + +**If your kustomization declares `labels:`, `commonLabels:`, `commonAnnotations:` or `namespace:`, +the operator has been writing those injected values into your source manifests.** Measured, on a +folder we accept today, with nothing changed in the cluster and nothing changed in the render: + +```yaml +# kustomization.yaml (yours) # deployment.yaml, after one reconcile (ours) +labels: metadata: + - pairs: labels: + env: prod env: prod # <- the OVERLAY's, absorbed into the BASE +commonAnnotations: annotations: + owner: platform owner: platform +``` + +The writer mirrors a live object into the file that produced it — but under kustomize that file is +not what the cluster runs, and mirroring the live object straight back writes the build's own +output into the build's input. Every reconcile of an unchanged folder produced a commit, and the +file was left wrong: delete the kustomization later and the injected values are now yours forever. +In a base shared by two overlays, the value baked in is **one environment's**. + +The fix needs no model of any transformer, and it is now the rule the writer follows: + +> **Where the live object and the render agree, the source keeps its bytes. Where they disagree, +> the user changed something, and that is what we write.** + +**Nothing needs migration** — this is a fix, and it makes the operator stop rewriting files it +should have left alone. If a past reconcile has already baked injected metadata into a manifest, +the operator will not remove it for you; remove it by hand and it will not come back. + +**Two behavior changes go with it.** + +*The re-render now runs for any document a kustomization produces*, not only for one an +`images:`/`replicas:` entry governs. A change to a field the build supplies (relabelling a live +object whose label a `labels:` block sets, say) cannot be expressed in the repository: the source +file cannot hold it, because the build would stamp its own value straight back. That write now +**refuses the flush** — `GitPathAccepted=False` / `WriteBoundaryRefused`, naming the file and the +object — where before it was committed and silently never converged. + +*A live change the projection cannot place is refused* (`unplaceable-edit`). It fires when the +build and the live object have **both** rewritten one list whose elements carry no unique `name:` +to pair them by — the source's `args:` rewritten by a patch, for example. There is no honest way +to say which of the source's bytes you meant to keep, and pairing the lists by position is +measurably wrong (kustomize *prepends* a container a patch adds), so the operator refuses rather +than guesses. + ## Unreleased — kustomize decides what it renders, and what it touched (next minor; bug fixes + behavior change) The write path no longer contains a re-implementation of kustomize's image and replica diff --git a/docs/design/support-boundary/README.md b/docs/design/support-boundary/README.md index 5c08547e1..3a7666fd4 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** · [render-fidelity-scenarios.md](render-fidelity-scenarios.md) — red-first fidelity fixtures + the folder-gate state matrix · [kustomize-token-writeback-explained.md](kustomize-token-writeback-explained.md) — teaching explainer: the `${...}` writeback problem, the tried simplifications, and the managedFields question · [renderer-abstraction-idea.md](renderer-abstraction-idea.md) — exploration: a pluggable renderer seam (`Owns` + blind spots), starting with FluxKustomize · [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 | @@ -120,6 +121,11 @@ rather than guessing. - **Kustomize `images:` / `replicas:` edit-through** — a live change produced by an override entry is written back to that entry, never through into the source manifest ([finished/images-and-replicas-edit-through.md](finished/images-and-replicas-edit-through.md)). +- **Render-fidelity token gate** — parsed `${...}` values are compared with live state before a + write. A mismatch refuses the operation and makes the independent + `RenderMatchesLive=False` condition close normal writes until a fresh complete watch epoch is clean + ([render-fidelity.md](render-fidelity.md)). Remote-Git revision detection and automatic recovery after + a Git repair remain unbuilt. - **Higher-level KRM documents** (Flux `HelmRelease`, Argo CD `Application`, KRO resources) mirror and edit exactly like core resources — the pipeline is kind-agnostic, and is pinned by a corpus plus a HelmRelease mirror+edit e2e diff --git a/docs/design/support-boundary/admission-consent.md b/docs/design/support-boundary/admission-consent.md new file mode 100644 index 000000000..081f3c759 --- /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 000000000..15a2a10ec --- /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/kustomize-token-writeback-explained.md b/docs/design/support-boundary/kustomize-token-writeback-explained.md new file mode 100644 index 000000000..02661a08a --- /dev/null +++ b/docs/design/support-boundary/kustomize-token-writeback-explained.md @@ -0,0 +1,244 @@ +# The `${...}` token writeback problem, explained + +> **explainer** — a teaching companion to the decided design in +> [render-fidelity.md](render-fidelity.md) and its fixtures in +> [render-fidelity-scenarios.md](render-fidelity-scenarios.md). Nothing here is new policy; +> this doc exists so the *why* is legible later without re-deriving it. Captured 2026-07-15. +> Related fact: [`../../facts/kustomize-never-emits-dollar-brace.md`](../../facts/kustomize-never-emits-dollar-brace.md). + +## The one-sentence problem + +We reverse live changes back into Git by comparing the live object to what our render +produces — but when the live object holds a value our render **did not** produce (a Flux +`postBuild` variable, an Argo override), naively writing that value back **destroys the +parameterisation it came from**. + +## The corruption, concretely + +A folder is managed by Flux. The source parameterises a region: + +```yaml +# git: configmap.yaml +data: + region: ${REGION} +``` + +Flux builds the folder, then runs `postBuild.substitute` **after** the build, replacing +`${REGION}` from a cluster ConfigMap. So the live object is: + +```yaml +# live (what the cluster runs) +data: + region: us-east +``` + +Now a user edits something unrelated on that object and we go to mirror it back. Our own +render of the folder still says `${REGION}` (our kustomize never touches a `${...}` token). +The naive writer compares: + +```text +git source: region = ${REGION} +our render: region = ${REGION} ← kustomize passed the token through untouched +live object: region = us-east ← Flux substituted it, out of our sight +``` + +It sees `source == our-render`, concludes *the user must have set `us-east`*, and writes +`us-east` into the file. **The `${REGION}` template is gone.** Worse, it looks fine: next +reconcile Flux substitutes again (now a no-op — the value is already literal), so the object +converges. The damage only surfaces later, when someone changes `REGION` and the file no +longer follows. + +```mermaid +flowchart LR + src["git source
region: ${REGION}"] --> ours["OUR kustomize build"] + ours --> og["our render
region: ${REGION}"] + src --> flux["FLUX: build + postBuild.substitute"] + cm[("cluster ConfigMap
REGION = us-east")] --> flux + flux --> live["live object
region: us-east"] + og -. "source == our-render,
so 'the user set us-east'" .-> bad["writes us-east into the file
❌ template destroyed"] + live -.-> bad + + classDef blind fill:#fdd,stroke:#c33,color:#111 + classDef ok fill:#dfd,stroke:#3a3,color:#111 + class cm,bad blind + class og ok +``` + +## Why "just run kustomize" does not fix this + +This is the most common misconception, so it is worth stating flatly. Running kustomize +(and the source-form projection built on it) solved a **different** leak: the one where +*our own render* introduces a value — a `commonLabels`/`images`/`replicas` transform makes +our render produce `env: prod`, and the writer would mirror `prod` back into a file that +only said `${ENV}`. Source-form fixes that: *where our render equals live, keep the source +bytes.* + +`postBuild` is not that. It runs in the **orchestrator**, **after** our build, on a token +our kustomize **passes through verbatim**. Our render of `${REGION}` is `${REGION}`; we +have no way, from the folder alone, to know it should be `us-east`. So: + +- our render **cannot** tell us the correct live value (we'd have to read the Flux object + and the cluster ConfigMap — the unbuilt "orchestrator awareness" fence), and +- the post-write oracle **cannot** catch the bad write either: it re-renders with *our* + kustomize, which also leaves `${REGION}` literal, so it agrees with the corruption. + +Running kustomize is blind to `postBuild` by construction. The fence covers exactly the gap +kustomize cannot see. + +## The fence: measure the render against the live object + +The discriminator we lack on disk we already hold at write time: **the live object is the +orchestrator's render.** So we do not predict the orchestrator's context — we observe its +output. + +> `diverges(render, live)` := the **render** carries a `${...}` token at a field where the +> live object holds a **different** value. + +On a divergence we refuse the write (nothing is committed) and raise the independent +`RenderMatchesLive=False` condition. We do **not** claim *what* caused it ("substituted" +would be a guess); we claim only the fact we can stand behind — *our render did not produce +this value* — under the reason `RenderDoesNotMatchLive`. + +```mermaid +flowchart TD + W["about to write a field back to source"] --> Q{"does our RENDER
equal live here?"} + Q -->|yes| K["SAFE — our render is what the cluster runs.
mirror / keep source as normal"] + Q -->|"no, and the render has a ${...} token here"| R["REFUSE — we did not produce
this live value; protect the token"] + Q -->|"no, and no token"| D["ordinary drift OR context skew —
the open general case (§5b)"] + + 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 +``` + +### Why key on the token at all? + +Because it is the one field-class where "different ⇒ dangerous" holds **for free**. +kustomize provably never emits or resolves a `${...}` token; an HPA never writes one; a +defaulting webhook never writes one. So a `${...}` in our *render output* is guaranteed to +be an unresolved input parameter — and if live differs there, the difference can only be +something resolving it out of band. On token fields the check has ~zero false positives, +**without** needing to solve the hard general question of "is this divergence context or +drift?" (see the managedFields section below). + +### Read the render, not the source + +Two small examples show why the comparison is against `dm.Rendered.Object` (or, for a plain +manifest, the Git document itself), never the raw source bytes: + +| Fixture | Source | Render | Live | Verdict | Reason | +|---|---|---|---|---|---| +| `label-overwrites-source-token` | `env: ${ENV}` | `env: prod` (a `labels:` transform overwrote it) | `prod` | **mirror** | render == live; a git-vs-live check would falsely refuse | +| `label-injects-render-token` | *(no `env`)* | `env: ${ENV}` (a `labels:` transform injected it) | `prod` | **refuse** | a token the source never had still reaches the cluster | + +And the "must still mirror" guardrails — where the token is genuinely literal because the +live object carries it verbatim too: + +| Fixture | Field | Live | Verdict | +|---|---|---|---| +| `literal-crd-description` | CRD schema `description: ${var:=default}` | same literal | **mirror** | +| `literal-kro-template` | `${schema.spec.replicas}` | same literal | **mirror** | +| `literal-nginx-config` | `nginx.conf: ...${host}...` | same literal | **mirror** | +| `native-dollar-paren` | `$(POD_IP)` | any value | **mirror** (parens are native syntax, outside the predicate) | +| `comment-only-token` | `# ${REGION}` in a comment | — | **mirror** (comments are not parsed values) | + +## Tried simplifications that did *not* work + +Recorded so they are not re-attempted. + +1. **A structural on-disk regex — "refuse any managed file containing `${...}`."** No live + comparison at all; cheapest possible. **Reverted — it broke CRD mirroring.** A CRD schema + `description` literally contains `${var:=default}` (the Flux Kustomization CRD documents + `postBuild` in its own schema), and a structural check cannot tell a *literal* token from a + *substituted* one. The acceptance gate is all-or-nothing over a folder, so one such CRD + refused every unrelated write in the folder. **Lesson: the discriminator — was this token + actually substituted? — is not on disk. You must look at the live object.** + +2. **git-vs-live instead of render-vs-live.** Comparing the *source* bytes to live seems + equivalent and simpler. It is wrong twice: it **falsely refuses** `label-overwrites-source-token` + (source `${ENV}` ≠ live `prod`, but the render is `prod` == live, so the folder is faithful), + and it **misses** `label-injects-render-token` (a token a `patches:`/`labels:` block injects + into the render that the source never had). The render is what the cluster actually gets, so + the render is what must be compared. + +3. **`task test-e2e 2>&1 | tail -N` to check the result.** A smaller lesson, but real: the + pipeline reports `tail`'s exit code (0), not the suite's — a **failing** suite reads as green. + Assert on the `Passed | Failed` summary line, or redirect to a file. (Easy to hit; it was hit + again during review of this very workstream.) + +## Could managedFields simplify this? + +Short answer: **managedFields does not make the token check simpler — it makes the token +check largely unnecessary, by enabling a more powerful check that subsumes it.** + +The token check is narrow on purpose. The token is a *free discriminator* for one question — +"is this divergence out-of-band context, or ordinary runtime drift?" — but it only answers +that question for fields that happen to carry a `${...}`. It is blind to divergences that +leave **no token**: an Argo `spec.source.kustomize.images` override, a `replicas` override, a +kustomize **version** difference. Those change the applied object with no `${...}` anywhere, +so the token gate never fires. + +The **general** fence (§5b of [render-fidelity.md](render-fidelity.md)) would catch all of +them: *before trusting our render as the baseline, require it to reproduce the live object for +every field the write does not deliberately change.* The reason it is not the fence today is a +single hard problem — **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 refuse nearly +every folder, not "a shade too eagerly." + +`managedFields` is the candidate discriminator that would make the general fence safe. Every +field records *which field manager last set it*: + +- A field owned by the **GitOps applier** (Flux `kustomize-controller`, Argo's controller) but + **differing from our render** ⇒ the applier had input we did not — `postBuild`, an override: + **render context we failed to reproduce ⇒ refuse.** +- A field owned by **`hpa`, `kubelet`, a defaulter, another controller** ⇒ **runtime drift ⇒ + ignore.** + +```mermaid +flowchart TD + F["live field differs from our render"] --> O{"managedFields owner?"} + O -->|"GitOps applier
(flux / argo controller)"| C["render context we did not reproduce
→ REFUSE (general fence)"] + O -->|"hpa / kubelet / defaulter / other"| R["ordinary runtime drift
→ ignore"] + + classDef bad fill:#fdd,stroke:#c33,color:#111 + classDef ok fill:#dfd,stroke:#3a3,color:#111 + class C bad + class R ok +``` + +If that holds, a diverged `${...}` token becomes just **one special case** of "the applier +owns a field our render did not produce," and the dedicated token predicate can retire in +favour of the general rule — which is arguably *simpler* (one ownership question, no token +regex, no list-pairing) **and** strictly more powerful (it also catches the tokenless Argo / +version-skew cases). + +So the honest framing: + +- managedFields is **not** a simplification *of* the token logic; the token logic is already + near-minimal for what it covers. +- managedFields is what lets you **replace** the token gate with a general render-vs-live gate + that covers everything the token gate does and more. +- It is filed as **"promising — measure it"**, not done, because ownership has sharp edges: + server-side vs client-side apply, shared/co-ownership, `force` conflicts, and whether the + live object's `managedFields` even survives our sanitization intact. It must be measured + against **real** Flux and Argo `managedFields`, not assumed — the same discipline that caught + the structural check: measure against real content, do not reason about it. + +Until then, the token gate is the safe down payment; the general fence is the endgame it is a +down payment on. + +## See also + +- [render-fidelity.md](render-fidelity.md) — the decided design (the fence, §5a/§5b, the state + machine, where it runs). +- [render-fidelity-scenarios.md](render-fidelity-scenarios.md) — the executable fixtures every + example above is drawn from. +- [orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md) — reading the Flux / + Argo object directly (the third, most complete fence). +- [renderer-abstraction-idea.md](renderer-abstraction-idea.md) — where a `FluxKustomize` + renderer that *models* `postBuild` would sit, turning "detect and refuse" into "reproduce + correctly." diff --git a/docs/design/support-boundary/next-prompt-render-matches-live-gate.md b/docs/design/support-boundary/next-prompt-render-matches-live-gate.md new file mode 100644 index 000000000..60ceb0e64 --- /dev/null +++ b/docs/design/support-boundary/next-prompt-render-matches-live-gate.md @@ -0,0 +1,181 @@ +# RenderMatchesLive gate: implementation record + +> **completed (2026-07-15)** — the predicate, fixture corpus, scoped epoch gate, worker enforcement, +> GitTarget condition, CRD print column, and direct Flux end-to-end validation are shipped. This document is +> retained as the original implementation brief, with corrections where its assumptions differed from +> the delivered runtime. + +## Delivered + +- Parsed render-vs-live `${...}` comparison for both plain and kustomize-governed documents. +- Refusal of both live-event and scoped-resync writes before Git bytes change. +- `RenderMatchesLive` state machine: `Unknown` and `False` close normal write windows; only every + current scope clean makes it `True`; stale results and later clean results cannot clear a divergence. +- Separate `RenderDoesNotMatchLive` reporting; it does not change `GitPathAccepted`. +- Fixture, gate, writer, watch, controller, CRD, lint, and unit coverage. +- A direct Flux `postBuild.substitute` end-to-end fixture: it observes the external render context, + stalls the GitTarget, and proves that neither the token nor a later live edit is written to Git. + +## Still open + +- An incoming remote Git revision does not yet refresh the local source and begin a new epoch. A Git + repair therefore does not automatically reopen a false gate. +- That recovery must be coupled to the retained-intent/orchestrator barrier in + [orchestrator-reconcile-trigger.md](orchestrator-reconcile-trigger.md), not added as an unsafe + periodic fetch. +- The general non-token fence (5b) remains future work. + +--- + +## Historical implementation brief + +The completed work implemented the **render-vs-live gate** — `RenderMatchesLive` — the fence that refuses to track a +folder whose live objects differ from our render because of context we cannot see (Flux `postBuild` +substitution, Argo `spec.source.kustomize` overrides, a divergent kustomize version). Build the +**token form (5a)**, which is the part we block on. This is the implementation of a design that is +already written and decided; do not re-derive it, and do not widen it. + +## Read first + +- `AGENTS.md`. +- The memory notes: `substitution-token-check-breaks-crds` (**the lesson — read it first**), + `kustomize-renderer-workstream`, `diagnose-e2e-via-controller-logs`. +- **`docs/design/support-boundary/render-fidelity.md`** — the whole design. §4 (the fence), §5a + (what to build), §6 (the two surfaces: per-write + the blocking condition), §7 (where it runs — + option A, a precondition on the reconcile-on-acceptance), §8 (what is deferred, and why 5b waits). +- **`docs/design/support-boundary/render-fidelity-scenarios.md`** — the red-first fixture corpus and + the folder-gate state traces. Implement those tests before production code. + +## Where things stand, and the one trap + +- #234 shipped `sourceForm` ([`internal/manifestanalyzer/source_form.go`](../../../internal/manifestanalyzer/source_form.go)): + where the live object and our render agree, the source keeps its bytes. It stops *our* render's + output leaking into the source — but **not context we do not render** (postBuild etc.). That gap is + this gate. +- A **structural** "refuse any managed doc containing `${...}`" acceptance check was **tried and + reverted**. It broke CRD mirroring: a CRD schema `description` carries literal `${var:=default}` + (the Flux Kustomization CRD documents postBuild in its own schema), and a structural check cannot + tell a *literal* token from a *substituted* one. **The discriminator is the live object, not the + disk.** Do not re-attempt anything structural. + +## The one thing not to get wrong + +The check is render-vs-**LIVE**, never a pattern on disk. A `${...}` token is dangerous **only** when +the live object holds a *different, resolved* value at that field. Where `live == our render` — a CRD +description, a KRO `${schema.spec.*}` template, an nginx/envsubst ConfigMap — the folder **mirrors +fine and must never be refused**. Write those "must still mirror" cases FIRST and keep them green; +they are the guardrail the reverted check failed. + +## What to build (5a, both surfaces of one predicate) + +The predicate, computed once: + +> `diverges(doc, live)` := the **RENDER** carries a `${...}` token at a field where the live object +> holds a **different** value — where the render is `dm.Rendered.Object` for a kustomize-governed +> document and the **Git document itself** for a plain manifest. + +**It must be render-vs-live, NOT git-vs-live.** kustomize does not *resolve* a `${...}` token, but it +does not *preserve* every token-bearing source field either: a supported `labels` / `commonLabels` +transform overwrites `metadata.labels[...]` via `SetEntry`, so a source `env: ${ENV}` under +`labels: {env: prod}` renders to `env: prod` — equal to live. A git-vs-live check would **falsely +refuse** that faithful folder; render-vs-live does not, and it also catches a token a `patches:` block +*injects* that the source never had. (See `docs/facts/kustomize-never-emits-dollar-brace.md` and +render-fidelity.md §5a.) + +Read **parsed values, not raw bytes**. Comments never enter parsed data. A CRD schema `description` +*is* a parsed scalar and must be compared normally: its literal token is safe because the render and +live value are equal, not because descriptions receive an exemption. The shipped token regex is +`\$\{[^{}]+\}`: any non-empty non-nested brace expression matches; `$(POD_IP)` (parens are native / +kustomize var syntax) and `${}` do not. + +**Do not over-claim the cause, and bias toward blocking.** A rendered token + a diverged live value +proves only that our render did not produce that value — it could be Flux postBuild, a direct live +edit, an admission mutation, or another controller. Refusal is safe regardless, which is why the +reason is `RenderDoesNotMatchLive` (a fact) not "substituted" (a guess). And the guiding bias: +**blocking a shade too soon is fine; failing to block is not.** A simple, slightly-over-eager token +gate is the right first cut — do not reach for cleverness to avoid the rare over-block. + +- **6a — per-write refusal.** In the write path, refuse a write that would overwrite a rendered token + with a diverged live value, aborting the flush. Mirror the existing `sourceFormRefusal` / + `SourceFormRefusedError` pattern. +- **6b — the blocking folder condition.** The same predicate ORed across the folder powers a distinct + `RenderMatchesLive` GitTarget condition. Do **not** reuse `GitPathAccepted`: that condition remains + the structure/write-boundary claim, whereas fidelity depends on current Git and live state. + + The shipped epoch state machine uses `(GVR, namespace)` scopes. A target-watch declaration or scope + replacement starts an epoch with every scope pending and `RenderMatchesLive=Unknown`; only every scope + clean makes it True; any divergence makes it False; stale results are ignored; and only a new, + complete epoch can clear False. Both Unknown and False block normal write windows. Resync remains + allowed while blocked so it can measure that epoch. A Git revision or arbitrary GitTarget generation + does **not** yet start an epoch. Beginning an epoch closes the worker gate; an already-open window is + discarded if it later finalizes while closed. The worker, not a status update, is the enforcement point. + This is target-wide: one scope that cannot complete its replay (for example, persistent missing RBAC) + leaves every normal write for that GitTarget closed. The watch retry loop can complete that same epoch + when access recovers; a fresh epoch is not required for a pending scope, only to clear a divergence. + + The implementation adds a dedicated issue kind + `RenderDoesNotMatchLive` reason. The condition + reports one representative `(field, token)`, while the write refusal also names the file. Scope + reduction and the parsed-field walk are deterministic, but the first diverging document within one + replay follows the API replay order and is not a stable cross-run representative. The status derives + from the same epoch state; no scoped-resync + success may unconditionally mark a target healthy. + +## Where it hooks (entry points, verified in the code) + +- **Per-write:** [`internal/git/plan_flush.go`](../../../internal/git/plan_flush.go) `patchExisting` + holds the Git doc (`gitDocRawObject(buf.current, idx)`), the desired/live projection, and `dm`. Use + **`dm.Rendered.Object` as the render** for a kustomize doc and the Git doc for a plain one; run + `diverges()` against the live projection; on divergence return the refusal (see `sourceFormRefusal` + for the shape). It fires on `patchExisting` — an existing *rendered* token being overwritten — which + is the corruption (a new doc goes through `createNew`, with no token yet to protect). +- **Resync and folder gate:** + [`internal/git/resync_flush.go`](../../../internal/git/resync_flush.go) + `applyResyncToWorktree` → `applyResyncPlan` → `applyUpsert` → `patchExisting`. The refusal fires + here during a scoped resync, so it must abort that resync before a flush. But that is only the + per-write half: begin and reduce the epoch at the watch/worker boundary, record each scope result + before the next queued target write can run, and leave regular writes closed until the reduction is + True. The current target watch scopes include namespace as well as GVR. +- **Enforcement and surfacing:** add the state owner that can atomically answer "may this target open a + write window?" from the branch worker, then project the same state to the controller as the new + condition. Update [`internal/manifestanalyzer/acceptance.go`](../../../internal/manifestanalyzer/acceptance.go) + with the issue kind, [`internal/watch/event_router.go`](../../../internal/watch/event_router.go) with + the dedicated reason, and the controller's condition/status derivation. Do not implement this as a + `MarkTargetGitPathAccepted` variant: that is last-result status, not a folder gate. + +## The test net + +- **Predicate fixtures (`internal/manifestanalyzer/testdata/render-fidelity/`)**: build the complete + red-first matrix in `render-fidelity-scenarios.md §2`, including CRD/KRO/nginx literals, comments, + `$(VAR)`, absent live fields, nested lists, a source token overwritten by labels, and a token injected + into the **render** by supported labels. The last two are non-negotiable render-not-source guardrails. +- **Gate state unit tests:** build the §3 epoch trace before wiring watches: pending scopes deny writes; + a later clean scope cannot erase a divergence; stale results are ignored; a complete explicitly + started epoch reopens the target; and a per-write divergence immediately closes it. +- **Writer/watch integration:** a substituted document refuses the resync with no commit; a clean event + queued behind it cannot open a write window; an existing uncommitted target window is discarded if it + later finalizes while a fresh epoch has the gate closed; and a full fresh recheck can recover. Assert the distinct `RenderMatchesLive=False` / + `RenderDoesNotMatchLive` status rather than `GitPathAccepted=False`. +- **Corpus:** regenerate `task gitops-layouts-baseline` and confirm **nothing moves** — the corpus has + no live objects, so nothing can be diverged. In particular the KRO row must **not** move this time + (it did under the reverted structural check; that is the difference between this fence and that one). +- **e2e (shipped):** the dedicated Flux `postBuild` fixture in + `test/e2e/fixtures/render-fidelity/` proves the live substitution stalls the target and preserves + the Git source form. The **CRD-lifecycle spec** remains the regression guard for the reverted + structural check. Run `task test-e2e` and **capture the full log** (`task test-e2e 2>&1 | tail -N` + reports `tail`'s exit code, not the suite's — a failing suite reads as green; assert on the + `Passed | Failed` summary line or redirect to a file). Docker required (`docker info`). + +## Validation and delivery + +The required sequence was run successfully: `task fmt` → `generate` → `manifests` → `vet` → `lint` → +`test` → `test-e2e` (sequential, with Docker available). The implementation was delivered on +`fix/kustomize-source-form-projection`; future work should begin from the current branch head rather +than relying on the historical branch/restack instructions. + +## How this workstream finds bugs + +**Measure against real content, not just the corpus.** The reverted structural check looked clean on +the corpus and broke on the real Flux CRD in e2e — because the corpus has no live objects and no +real-world CRD schemas. Before claiming a detection is clean, run it against the actual resources the +e2e installs. And the standing rule: when you want to know what the orchestrator does, do not reason +about it — measure our render against the live object. 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 000000000..1b0f781fb --- /dev/null +++ b/docs/design/support-boundary/orchestrator-reconcile-trigger.md @@ -0,0 +1,362 @@ +# The orchestrator reconcile trigger: revert a refusal, and order around origin drift + +> **design** — direction-setting; no orchestrator trigger or origin-drift barrier described here is +> implemented. `RenderMatchesLive` is shipped, but it deliberately stays closed after a Git repair +> until this document's safe remote-revision path exists. +> Captured: 2026-07-15; implementation status updated: 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**, +> [render-fidelity.md](render-fidelity.md) — the shipped gate whose automatic Git-repair recovery depends on this barrier, +> [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 replacement for the write correctness layers: the flush-time render oracle +([`VerifyBatchRenders`](../../../internal/manifestanalyzer/render_verify.go)) and the shipped +[`RenderMatchesLive`](render-fidelity.md) token fence remain the decision points. This document is +about closing the loop *fast* and *visibly* once a "no" has been decided, and about safely refreshing +Git after origin drift so a repaired render-fidelity gate can be measured again. + +--- + +## 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. + +### The operating model this assumes + +Three invariants from the existing pipeline underpin everything below. The barrier is a *consequence* +of them, not a new policy: + +- **The cluster leads.** The cluster is the editing surface; live watch events flow cluster → Git + ([bi-directional.md](../../bi-directional.md) — the reconciler as a *triggered applier*). Git is a + mirror of cluster intent, re-derivable at any time by the mark-and-sweep resync. Nothing in the + operator treats Git as authority *over* the cluster. +- **Git drift is a supported input, not an error.** Someone else pushing to the branch is expected and + handled, never rejected. The commit direction is already safe against it (below); what this document + adds is the missing *cluster*-side handling of that same drift. +- **Optimistic push, and the queue clears only on a landed push.** Cluster changes are committed + locally into a retained queue — `pendingWrites` — and pushed; **only a successful push clears that + queue**, and a failed push retains it for retry + ([`branch_worker.go`](../../../internal/git/branch_worker.go), `pushPending`). We do not pull-then-push + in steady state: we push optimistically and reconcile with the remote only when the push is rejected. + (The *first* commit of a fresh cycle does re-base the local worktree on the current remote tip first, + but the queue is empty at that point, so no intent is at stake — the re-pull/replay of *retained* + intent happens only on a rejected push.) + +### 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, then re-plan and re-commit the *retained pending writes* on top (the local commit objects +are discarded; the durable intent behind them is not), and re-push with an updated CAS. Because the +replay re-derives each commit from cluster intent rather than replaying opaque diffs, it can even +produce **no commit at all**: if the drift already carries the same content, the re-plan finds nothing +to change and the whole rebase collapses to a no-op. So we never clobber someone else's push, our own +intent survives, and a push conflict never manufactures a spurious commit. This path is covered by unit +tests that push a competing commit to the remote and assert the rebase resolves cleanly +(`TestBranchWorker_ConflictResolution`, `TestBranchWorker_ConcurrentOperations` in +[`git_operations_test.go`](../../../internal/git/git_operations_test.go)). + +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 —
holds live events back, replaying-gated FIFO + Rev->>Git: land pending intent — finalize the open window,
push pendingWrites rebased onto the new tip
(replay — a no-op if the drift already matches) + 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** (proactive origin-drift detection — see *The trigger*, below; the groundwork method +`SyncAndGetMetadata` exists but is dormant, uncalled today), 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 trigger: a Git push webhook, not lazy discovery + +Today the operator has **no proactive drift signal at all.** It notices a moved origin only at *push +time* — when `PushAtomic`'s compare-and-swap is rejected and `pushPendingCommits` rebases by replay — +and a push only happens after a *cluster* edit produces a commit. So a foreign push into an otherwise +quiet branch stays invisible until the next cluster edit collides with it: the conflict path *is* the +discovery mechanism. (`SyncAndGetMetadata` was meant to be a cached remote-drift check, but it is +dormant — nothing calls it, and the steady 5-minute reconcile never fetches the remote.) + +The better trigger is the one the orchestrator already consumes: the **Git host's push webhook.** The +same event that tells Flux/Argo "apply this revision now" is exactly the signal the operator needs — +"origin drifted, engage the barrier" — and it arrives at the same moment, so the operator can raise the +barrier *concurrently* with the orchestrator beginning its apply, rather than discovering the drift +mid-flood. This flips detection from lazy to proactive and **demotes the conflict/replay path from the +common case to a rarely-hit backstop.** Three properties make it a clean fit: + +- **It stays a backstop, never a correctness dependency — and must be tested as one.** Webhooks are + lost, delayed, misrouted, or **never configured at all**, and the operator cannot tell a webhook that + will never come from one that is merely late. So the CAS-rejection rebase-by-replay must remain the + correctness net, and **a kept e2e must exercise the drift-recovery path with the webhook switched + off** — proving a foreign push is still absorbed correctly on the next push attempt with no webhook in + play. If that test is ever allowed to lapse, a webhook regression turns silently into a data-loss bug. + (An optional periodic poll, reviving `SyncAndGetMetadata`, is a reasonable middle fallback, but it + replaces neither the backstop nor its test.) +- **It must suppress our own push.** The operator's own commit fires the very same webhook. The + receiver has to compare the webhook's new revision against the SHA it last pushed for that + `(provider, branch)`: equal ⇒ our own commit, ignore; different ⇒ foreign drift, engage. Without this + the operator would raise the barrier against every commit it makes. +- **Its routing is trivial — lighter than §4's.** A push webhook names `(repo, branch)`, and + BranchWorkers are already keyed by `(provider, branch)`, so the signal maps straight to the worker + with no path-level lookup. And *receiving* a drift notification is not a boundary *write*, so it + clears a much lower bar than the orchestrator-ownership claim §4 needs to *fire* the reconcile + trigger — though it is still a new inbound surface (a Service plus a shared secret to validate + signatures), opt-in like everything else here. + +### 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 **not-yet-remote** intent in two places: the **open commit window** (`openWindow` — +events coalesced in memory but not yet committed) and the **committed-but-unpushed queue** +(`pendingWrites`). Triggering the orchestrator reconcile *now* would apply the new origin over those +live edits on the cluster, erasing them before they reach Git. + +So the ordering must be: **land pending intent to Git first, *then* trigger the reconcile, *then* +resume.** "Landing" it is the ordinary finalize-then-push — the open window is finalized into a commit, +joins `pendingWrites`, and the whole queue is pushed, rebased onto the new tip by the replay above (and +a no-op if the drift already matches). This is only safe because the intent is durably recoverable: the +cluster is the source of truth and the mark-and-sweep resync re-derives it, so even a pod that dies +mid-barrier rebuilds the same intent on restart. The barrier's safety rests on that durability, stated +here as a precondition rather than discovered as a bug. + +A note on the commit window, because it is tempting to picture the barrier as "pause the window, reset, +replay, resume the same window." It is **not** that. The open window is in-memory event state, +*orthogonal* to the git worktree: the push-side reset/replay touches only `pendingWrites` and never the +window — which is exactly why a plain push conflict "just works" without any window ever being closed or +reopened; its eventual finalize simply commits on top of whichever tip the worktree now holds. The +barrier does not suspend and resume a window either. It *finalizes* the open one — closing it — before +the reconcile, and lets live events **open a fresh window** only after the barrier lifts. Holding those +live events back for the duration is the existing `replaying`-gated FIFO +([`target_watch.go`](../../../internal/watch/target_watch.go)) — the same mechanism that already +sequences a mark-and-sweep ahead of live events on watch (re)establishment. + +### What is proven, and what needs a test + +The commit-direction replay is proven by the unit tests named above. The *cluster-side* barrier this +document proposes is unbuilt, so nothing exercises it end to end yet. When it lands it needs an e2e that +pushes origin drift **while the operator holds uncommitted intent**, and asserts, in order: the intent +reaches Git rebased onto the new tip (a no-op when the drift already matches); the orchestrator +reconcile is triggered and awaited; and the resulting apply is absorbed as a resync no-op rather than +mirrored back as a fresh commit. Until then, treat "the whole thing just works" as a *design intent*, +not a tested guarantee. + +**Two drift e2es, both kept, both required — because the webhook is best-effort.** The webhook is an +accelerator that can fail or simply never be configured, so correctness cannot rest on it. Coverage must +split, and neither half may be allowed to lapse: + +1. **Webhook present (the fast path).** A foreign push fires the webhook; assert the barrier engages + promptly and the orchestrator's apply is absorbed as a no-op. +2. **Webhook absent (the backstop).** The *same* foreign push with **no webhook delivered**; assert the + operator still recovers on its next push — CAS rejection → rebase-by-replay → clean landing — with no + clobber and no spurious commit. + +The second is the one that must never rot: it is the guarantee that a missing or broken webhook degrades +gracefully to correct-but-slower, never to data loss. Keep it green as a required check for the life of +the feature. + +--- + +## 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. +- **Webhook delivery and trust** (the drift trigger, §3). Delivery is best-effort: how is a missed + webhook caught up — a poll fallback cadence (reviving `SyncAndGetMetadata`), or a fetch on the next + reconcile — so the CAS-replay backstop is not the *only* thing that ever notices a lost notification? + And how is the receiver authenticated per provider (shared secret, signature scheme) so a forged push + notification cannot make the operator barrier or replay on demand? diff --git a/docs/design/support-boundary/render-fidelity-scenarios.md b/docs/design/support-boundary/render-fidelity-scenarios.md new file mode 100644 index 000000000..8eba67f22 --- /dev/null +++ b/docs/design/support-boundary/render-fidelity-scenarios.md @@ -0,0 +1,143 @@ +# Render fidelity: red-first scenarios and fixtures + +> **design + implementation record** — the executable examples for +> [render-fidelity.md](render-fidelity.md). They are deliberately separate from the +> layout corpus: the layout corpus has repository bytes but no corresponding live +> objects, while fidelity is a render-**vs-live** claim. + +The predicate fixtures and state-machine tests below are implemented. This document keeps the +remaining worker-sequencing and Flux end-to-end cases as follow-up acceptance criteria. A hand-written +unit test that constructs only the happy path is not an adequate substitute: the two regressions this +fence exists to prevent are both plausible-looking shortcuts. + +## 1. Fixture boundary + +The implemented self-contained fixture suite is: + +```text +internal/manifestanalyzer/testdata/render-fidelity/ + / + git/ # the exact tracked tree; may contain kustomization.yaml + live.yaml # the sanitized live object(s), one or more YAML documents + want.yaml # condition result and the stable diagnostic field paths +``` + +The test renders `git/` with the same `renderRoot` path production uses; it does **not** +check in a second, hand-maintained `render.yaml`. `want.yaml` expresses only what the +test owns: + +```yaml +condition: "True" # or "False" +reason: "RenderMatchesLive" # or "RenderDoesNotMatchLive" +divergences: + - file: deployment.yaml + field: spec.template.spec.containers[app].env[REGION].value + token: ${REGION} +``` + +The field notation is diagnostic output, not an API for locating arbitrary edits. The +comparison itself walks parsed structured values and treats presence as significant: +a rendered token whose live field is absent or `null` is a divergence too. + +Do **not** add these cases to `test/fixtures/gitops-layouts/`. That corpus answers +whether a Git layout is structurally supported; it cannot answer whether a particular +orchestrator output matches its render. These fixtures are a live-pair corpus. + +## 2. Predicate fixtures — start here + +Each row is a committed fixture directory and a table-driven subtest. The first eight +must be red before `diverges(render, live)` exists. + +| Fixture | Git/render shape | Live shape | Expected | Why it is load-bearing | +|---|---|---|---|---| +| `plain-postbuild-token` | Plain Deployment env value `${REGION}` | `us-east` | `False`, one divergence | The corruption case: no kustomize document model is required. | +| `literal-crd-description` | CRD schema description `${var:=default}` | Same literal description | `True` | Regression for the reverted structural fence. A description is a parsed value, not an excluded field. | +| `literal-kro-template` | KRO template `${schema.spec.replicas}` | Same literal template | `True` | A non-CRD literal-token guardrail. | +| `literal-nginx-config` | ConfigMap data `${host}` | Same literal data | `True` | A template/config file that legitimately keeps a token. | +| `comment-only-token` | Comment contains `${REGION}`; values do not | Same object | `True` | Proves the walk reads parsed values, not YAML bytes. | +| `native-dollar-paren` | `$(POD_IP)` / unresolved `$(FOO)` value | Same or different literal value | `True` | Brace-only matching must not classify Kubernetes or kustomize vars. | +| `label-overwrites-source-token` | Source label `${ENV}`; supported `labels: {env: prod}` renders `prod` | `prod` | `True` | Proves the predicate reads the **render**, not the Git source. | +| `label-injects-render-token` | Source has no `env`; supported `labels: {env: ${ENV}}` injects it into the render | `prod` | `False`, one divergence | Proves a render-only token is found even though the source document lacks it. | +| `token-field-removed-live` | Rendered value `${REGION}` | Field absent or `null` | `False` | Presence is part of equality; a removal must not be mirrored over the token. | +| `nested-list-token` | Container `app`, env `REGION=${REGION}` | `REGION=us-east` | `False`, stable named-list path | Exercises a practical nested list and makes diagnostics usable. | + +The two label cases use constructs supported today. They must not be postponed behind +the later `patches:` work: a patch that injects a token is a useful future extension, +but labels already prove the same render-not-source property. + +## 3. Folder-gate state tests + +The condition must be tested separately from manifest rendering. Give the state machine +a small, pure API and table-drive the following trace. A scope is `(GVR, namespace)`; +the example uses `deployments.apps/default` and `configmaps/default`. + +| Step | Event | Expected condition | Writes allowed? | +|---:|---|---|---| +| 1 | Begin epoch `E1` with both scopes pending | `Unknown/Rechecking` | No | +| 2 | Deployment scope reports clean | `Unknown/Rechecking` | No | +| 3 | ConfigMap scope reports clean | `True/RenderMatchesLive` | Yes | +| 4 | Begin `E2` after a target-watch replacement; an existing window is discarded if it later finalizes while the gate is closed | `Unknown/Rechecking` | No | +| 5 | Deployment scope reports a `${REGION}` divergence | `False/RenderDoesNotMatchLive` | No | +| 6 | ConfigMap scope reports clean | Still `False` | No | +| 7 | A normal live write arrives | Still `False`; no window/commit | No | +| 8 | Stale clean result from `E1` arrives | Ignored; still `False` | No | +| 9 | Begin `E3` through an explicit fresh watch epoch; both scopes report clean | `True/RenderMatchesLive` | Yes | +| 10 | A steady-state write finds a divergence | Immediately `False` with a sample | No | + +The shipped tests cover this trace, including the zero-scope case. Once structural acceptance has passed, a target with +no active watch scopes is `True` by vacuous comparison; it cannot receive a normal live +write. This avoids leaving an otherwise idle target permanently `Unknown`. + +The test names should make the safety contract obvious: + +```text +TestRenderFidelityGate_RequiresEveryScopeInEpoch +TestRenderFidelityGate_IgnoresStaleEpochResult +TestRenderFidelityGate_PerWriteDivergenceClosesTarget +TestRenderFidelityGate_FullFreshEpochReopensAfterGitRepair +``` + +## 4. Writer and watch integration tests + +The shipped writer test proves that the same refusal blocks both a live write and a scoped-resync +write without changing the worktree. The following ordered worker trace remains to be added: + +1. Initial scoped resync finds `plain-postbuild-token` and creates **no** Git commit. +2. The worker records `RenderMatchesLive=False` before it processes the next queued + event for that target. +3. A clean-resource event queued behind the refusal cannot open a write window or + change a file. +4. An already-open, uncommitted window is discarded if it later finalizes while a fresh epoch has the + gate closed; the resync must not commit it before measuring the new epoch. +5. After a future remote-Git revision detector has safely started a fresh epoch, a complete replay of + the refreshed worktree reopens writes only after every scope passes. + +At the watch-manager layer, pin that a clean scoped resync does not overwrite another +scope's failed result. The existing `GitPathAccepted` tests are not enough: fidelity is +a separate condition and its result is a reduction over the epoch map, not the last +resync reply. + +## 5. End-to-end proof + +The dedicated fixture at `test/e2e/fixtures/render-fidelity/` is implemented and runs in the +regular `manager` E2E shard (not the Argo-only bi-directional lane). It uses a real Flux +`Kustomization`: the repository Deployment contains `${REGION}`, while +`postBuild.substitute` resolves it to `us-east` in the live object. Do not reuse a +render-root-scoping fixture: this fixture owns the render-vs-live pair directly. + +The e2e assertions are: + +1. The GitTarget reaches `RenderMatchesLive=False` with reason + `RenderDoesNotMatchLive`, `Ready=False`, and `Stalled=True`. +2. The Git file still contains `${REGION}` and no reverse-GitOps commit was created. +3. A subsequent live edit to an otherwise clean object is not mirrored while the gate + remains false. +4. **Future:** after a remote-Git revision detector refreshes a Git repair, a complete replay flips the + condition to `True` and normal writes resume. +5. The existing CRD-lifecycle e2e remains green: its literal `${var:=default}` schema + description must never fail the condition. + +The shipped gate is protected by synthetic predicate, writer, state, watch, controller, and direct +Flux end-to-end coverage. The test proves the actual external `postBuild` context rather than a +hand-constructed live object; recovery is deliberately not claimed until the revision detector exists. +The CRD lifecycle spec remains the regression guard against reviving the rejected structural check. diff --git a/docs/design/support-boundary/render-fidelity.md b/docs/design/support-boundary/render-fidelity.md new file mode 100644 index 000000000..b5958152a --- /dev/null +++ b/docs/design/support-boundary/render-fidelity.md @@ -0,0 +1,475 @@ +# Render fidelity: our render is not the orchestrator's + +> **design + implementation record** — the token form of this fence (5a), its write gate, +> condition, and fixture suite shipped on 2026-07-15. The general fence (5b), remote-Git +> revision detection, and the orchestrator reconcile barrier remain design work. +> Captured: 2026-07-15; implementation status updated: 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, +> [render-fidelity-scenarios.md](render-fidelity-scenarios.md) — the red-first fixture and gate-state matrix, +> [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. + +## Implementation status + +The shipped implementation is deliberately the narrow, safe token form (§5a): + +- `RenderTokenDivergences` walks parsed render values and compares every non-empty `${...}` scalar + with sanitized live state. It uses the kustomize render for governed documents and the parsed Git + document for plain manifests; comments and `$(...)` are outside the predicate. +- A divergence aborts both live-event and scoped-resync writes with + `IssueRenderDoesNotMatchLive`. No file is changed or committed by that refused operation. +- `RenderMatchesLive` is an independent three-state GitTarget condition. Its per-target gate keeps + normal writes closed while an epoch is `Unknown` or `False`; scoped resync remains allowed so it can + measure the current watch set. +- The fixture corpus covers literal CRD/KRO/ConfigMap tokens, comments, `$(...)`, missing live + fields, nested lists, and source-versus-render label transforms. The gate, writer, watch, and + controller seams have unit coverage. +- The direct Flux end-to-end fixture applies a real `postBuild.substitute` value, then proves the + resulting divergence stalls the GitTarget and preserves the unresolved source token and later live + edits. + +Not shipped: a remote-Git revision observer that starts a fresh fidelity epoch after someone changes +the source, the required retained-intent/orchestrator reconciliation barrier for doing that safely, +and the general non-token fence (§5b). Until the revision observer exists, a Git repair alone does +**not** automatically reopen a false gate. + +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, our render did not produce that value — refuse, do not +> guess *what* did.** + +The divergence proves only that our render is not the source of the live value; the *cause* — Flux +substitution, a direct live edit, an admission mutation, another controller — is neither knowable +here nor needed. We cannot faithfully reverse a value we did not render, so refusing is the safe +answer regardless, and `RenderDoesNotMatchLive` is a claim we can actually stand behind (where "must +have been substituted" would be a 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 render carries a token here"| R["REFUSE — the render did not produce
this live value; protect the token"] + 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: *when a write would replace a **rendered** value that still carries a +`${...}` token with a different live value, refuse it.* kustomize never creates or resolves a `${...}` +token ([the token fact](../../facts/kustomize-never-emits-dollar-brace.md)), so a token still present +in our **render output** came through verbatim from an input. If the live object holds a *different* +value there, our render did not produce that live value — and it does not matter what did (Flux +postBuild, a direct live edit, an admission mutation, another controller); we cannot reproduce the +live value from the source, so we refuse. + +**Compare the render, not the source.** kustomize does not *preserve* every token-bearing source +field: a supported `labels` / `commonLabels` transform overwrites `metadata.labels[...]` wholesale +(`SetEntry`), so a source `env: ${ENV}` under `labels: {env: prod}` renders to `env: prod` — no token, +and equal to live. Comparing the *source* to live would falsely refuse that faithful folder; comparing +the *render* to live does not. The render is `dm.Rendered.Object` for a kustomize-governed document +and the Git document itself for a plain one — and reading the render also catches a token a `patches:` +block *injects*, which a source scan would miss. + +It never refuses a folder whose render already equals live (the §4 table — CRD descriptions, KRO +templates, nginx ConfigMaps all pass). And where it *does* refuse, refusing is safe even when the +cause was a benign live edit: **we would rather block a folder a shade too eagerly than mirror a value +we cannot faithfully reverse.** Blocking a shade too soon is a nuisance; failing to block writes a +corrupted source file. + +### 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. And +*"block a shade too soon"* (§5a) does **not** rescue it here: nearly every live object drifts from +Git in some field, so a naive 5b would block nearly everything, not a shade too eagerly. +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**: keying on a token kustomize is +*guaranteed* never to produce means the only over-blocking it can do is on a field a template +already governs — narrow, and safe. + +--- + +## 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 the source representation of a **rendered** 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 corresponding **rendered** field still 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). + +Comments are not parsed fields and never participate. A CRD schema `description` **is** a parsed scalar, +so it participates exactly like any other scalar: it is safe when render and live both retain the literal +token, and it is deliberately refused if the live description differs. There is no kind- or field-name +exception; equality with live is the discriminator. + +### 6b. A GitTarget status you can read *before* you edit + +The same measurement, aggregated to the folder and surfaced as a standing **GitTarget condition** — +`RenderMatchesLive` — 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 +`RenderMatchesLive=False` condition tells you *this whole folder* has live values our render did not +produce (Flux postBuild, an Argo override, a direct live edit, admission mutation, or a divergent +version), so you learn it **up front, from status, before you waste an edit** — rather than one refusal +at a time. + +The current condition carries one representative `(field, token)` in its message; the per-write +refusal retains the file path as well. Scope reduction and the parsed-field walk are deterministic, +but the first diverging document in one scope follows API replay order, so the representative is not +a stable cross-run API. It is a sibling of the planned +`FullyReflected` condition in +[unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md): `FullyReflected` says *everything you edited was expressed*; +`RenderMatchesLive` says *our render matches what is running, so we can be trusted at all* — the more +fundamental of the two. It is recomputable when the watch manager begins a new epoch; steady-state +write refusals can close it, but cannot clear it. + +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. + +`RenderMatchesLive` is deliberately **separate from `GitPathAccepted`**. The latter remains the +structure/write-boundary claim (*can we parse and route this path safely?*). Fidelity is a live, +epoch-scoped claim (*do the currently replayed watch scopes match the local render?*). Conflating them +would let one clean scoped resync erase either a structural refusal or a still-diverging sibling scope. + +#### The folder-gate state machine + +The gate is a per-`GitTarget` data-plane state machine. Its unit of evidence is a watch **scope**: +`(GVR, namespace)`, not only a GVR. In the shipped implementation, a fidelity **epoch** is the +immutable set of active scopes installed by a target-watch declaration. It does not yet carry a Git +revision or independently detect source changes. Results from another epoch are stale and discarded. + +| Derived condition | Scope evidence for the current epoch | Normal live writes | +|---|---|---| +| `Unknown` / `Rechecking` | One or more active scopes are pending. A newly declared target starts here. | Deny | +| `False` / `RenderDoesNotMatchLive` | At least one completed scope found a rendered token whose live value differs. Expose one representative. | Deny | +| `True` / `RenderMatchesLive` | Every active scope completed cleanly for the same epoch. | Allow | + +The transitions are deliberately strict: + +1. The first target-watch declaration, or a replacement whose scope set changes, begins a new epoch. + It snapshots the scopes, marks each pending, sets `RenderMatchesLive=Unknown`, and closes normal + writes. This is **not** yet tied to every `GitTarget` generation or to an incoming Git revision. + Beginning an epoch does not enqueue a separate branch-worker control item; if an already-open window + later tries to finalize, the worker sees the closed gate and discards that window rather than + committing it. +2. Each scoped replay runs through the target's branch-worker FIFO. Its resync computes the predicate + for that scope and records `Clean` or `Diverged(sample)` **before** it replies and the worker accepts + the next queued write for that target. A stale `(epoch, scope)` result is ignored. +3. The condition is derived from the complete scope map, never from the last result: any `Diverged` + result makes it `False`; otherwise any `Pending` result keeps it `Unknown`; only all `Clean` makes it + `True`. +4. A steady-state per-write divergence immediately records `Diverged` for its scope, flips the target + `False`, refuses that write, and keeps later normal writes closed. A later clean result from one scope + cannot clear it. +5. A resync is allowed while the gate is `Unknown` or `False`, because it is how a new epoch is + measured. It evaluates before writing and commits nothing when it finds divergence. Recovery requires + a **new complete epoch** only from `False`; it is never inferred from one unrelated clean scope or a + status update. + +The denial is deliberately target-wide. One active scope that cannot finish replaying — for example, +because RBAC permanently forbids its GVR — leaves the whole target `Unknown`, so normal writes for its +otherwise healthy sibling scopes are denied too. The target-watch retry loop retries the same epoch; +once access recovers, that scope can report clean and reopen the target without a re-declaration. If +the scope is intentionally unavailable, an operator must repair access or remove/replace the scope. +This pending-scope case is distinct from `False`: only a fresh complete epoch can clear a recorded +divergence, and an incoming Git repair does not start one yet. + +The missing transition is intentional and visible: the controller does **not** yet observe an incoming +Git revision, refresh the source, and begin a complete epoch. Therefore a human Git repair cannot by +itself reopen a false gate. Adding that transition requires the retained-intent ordering barrier in +[orchestrator-reconcile-trigger.md](orchestrator-reconcile-trigger.md), so a source refresh cannot +discard or race an open live-edit window. The enforcement check already belongs in the branch worker, +not status projection: the worker rejects a `WriteRequest` before it opens a commit window. The +controller projects the same state as `Ready=False` / `Stalled=True` for `False`, and as +`Ready=False` / `Reconciling=True` for `Unknown`. + +This is deliberately strict. A `GitTarget` claims that a folder can be reverse-GitOps'd — live changes +captured faithfully back to Git. While the claim is unmeasured or false, the folder is not tracked for +writes. We can later loosen that to an audit-only mode if there is demand; we cannot undo a parameter we +silently replaced. + +### 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. Shipped implementation: where the comparison runs + +The blocking decision forces the timing: `RenderMatchesLive` is computed where the operator holds both +the Git content and the live objects, and **before the affected write** — because the first mirror of a +diverging folder is the corruption. That path is now implemented. + +### The one predicate, computed once, read by both surfaces + +Everything reduces to a single per-document test: + +> **diverges(doc) := the RENDER carries a `${...}` token at a field where the live object holds a +> different value** — where the render is `dm.Rendered.Object` for a kustomize-governed document and +> the Git document itself for a plain one. + +kustomize never creates or resolves a `${...}` token, so a token in the render output came from an +input; if the live object holds a *different* value there, our render did not produce it (whatever +did). Read the **render**, not the source: a transformer can overwrite a token-bearing source field +(§5a's `labels` case), and a `patches:` block can inject a token the source never had — the render is +what the cluster actually gets. 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 it runs + +**(A) Fold it into the reconcile that runs on acceptance — shipped.** When a folder is accepted, the +watch manager first starts the fidelity epoch, then each 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. The predicate runs as a **resync write 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 record the scope result in the epoch. A diverging scope aborts that resync's writes and +sets the aggregate condition `False`; a clean scope merely advances the epoch toward `True`. The gate +being `Unknown` before the first scope result is what prevents an early live event from opening a write +window. *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 — `AcceptRenderMatchesLive(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 diverging write (§6a) and flip `RenderMatchesLive=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 *"assumed to +match 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. + +**(A) is shipped.** 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 starts diverging 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 + +The state machine in §6b is the blocking mechanism: an initial or refreshed epoch closes normal writes +until every active `(GVR, namespace)` scope is clean, and a divergence keeps them closed. `GitPathAccepted` +is the independent structural gate; `RenderMatchesLive` is the live-fidelity gate. Both must be `True` +before the folder is writable. + +It is recomputable only when a complete fresh epoch begins. Today that happens when target watches are +installed or their scope set is replaced. Removing postBuild configuration or changing the source in Git +does **not** yet start that epoch automatically, so it does not automatically reopen writes. The planned +remote-revision transition must refresh safely behind the barrier in +[orchestrator-reconcile-trigger.md](orchestrator-reconcile-trigger.md); once that exists, a full clean +replay can reopen the gate without manual acknowledgement. + +--- + +## 8. Open questions + +- **5a is the gate we build; 5b is deferred** (decided). The token form (5a) is what + `RenderMatchesLive` blocks on — precise, cheap, and free of the runtime-drift false positive. The + general render-vs-live form (5b) is the complete answer (it also catches Argo overrides and version + skew) but must wait on the drift discriminator below before it is safe to block on. +- **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 fcb292e02..1d93d0a24 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 diff --git a/docs/design/support-boundary/renderer-abstraction-idea.md b/docs/design/support-boundary/renderer-abstraction-idea.md new file mode 100644 index 000000000..801db98d3 --- /dev/null +++ b/docs/design/support-boundary/renderer-abstraction-idea.md @@ -0,0 +1,276 @@ +# A renderer abstraction: idea and reasoning + +> **exploration — not decided, nothing shipped.** A design sketch for pulling the +> "what does the build do, and can a change be reflected safely?" knowledge behind a stable +> seam, so the rest of the code reasons against evidence and explicit refusals rather than +> scattered kustomize-specific branches. Captured 2026-07-15. Grew out of the render-fidelity work +> ([render-fidelity.md](render-fidelity.md), +> [kustomize-token-writeback-explained.md](kustomize-token-writeback-explained.md)). + +## The itch + +The kustomize model leaks across the codebase as ad-hoc branches — `dm.Rendered != nil`, +`putToKustomize`, `kustomizationsListing`, the override-split routing in +[`internal/git/plan_flush.go`](../../../internal/git/plan_flush.go), and more in +`internal/manifestanalyzer/` and `internal/watch/`. Each is a small, correct decision, but +collectively they mean "how kustomize behaves" is *diffuse*: to reason about one write you +consult knowledge spread over three packages. And there is a forward path — Helm, and +orchestrator-aware rendering (Flux `postBuild`, Argo `spec.source.kustomize`) — that today has +nowhere clean to live. + +The proposal: **name the seam.** Put the build's behaviour behind explicit results and plans, +with kustomize as the first implementation, so adding a renderer, or reasoning about the +current one, is local. This does *not* mean that every source format must implement one large +Go interface. + +## The key reframe: "render" is not one operation + +The word "render" hides at least four jobs. Lumping them behind a single `Render()` is where +these abstractions usually go wrong, because they abstract at very different quality: + +| # | Job | Kustomize | Abstracts cleanly? | +|---|---|---|---| +| 1 | **Forward render** — `inputs → []object` | `kustomize build` | **Yes.** Clean signature, obvious contract. | +| 2 | **Field ownership / inverse** — where does a live change belong in source so a re-render reproduces it? | images/replicas → entry; labels → transformer-owned; patch-injected → nobody | **Only as an evidence-backed plan.** An ownership enum alone loses needed context. | +| 3 | **Placement / wiring** — new-file location + editing `resources:` | `kustomization.yaml` | **Poorly.** Helm's model (`templates/` + `values.yaml`) shares no shape. | +| 4 | **External-input / blind-spot declaration** — what must be known to reproduce the build, and what is unsupported? | `${...}` is unresolved without postBuild inputs; version/flags may differ | **Yes, if operational.** A static list documents a gap; a render result must identify the missing input or refusal. | + +Job 1 is nearly free to abstract. Job 4 is cheap and valuable once it produces operational +requirements. Jobs 2 and 3 are the real design work, and the trap is an interface that makes +them *look* uniform when they are not: forcing Helm's placement through kustomize-shaped methods +it then stubs out is **worse** than today's explicit branches, because the branches are at least +honest about the coupling. + +## We are already at N ≈ 1.5 — the plain-manifests control case + +An important correction to "don't abstract from a single implementation": the plain-manifests +path is a useful **control case**. It is a degenerate input mode, not an independent source +model, and the `dm.Rendered != nil` branches are some of the polymorphism done by hand: + +| Interface job | Kustomize | **Plain manifest** | +|---|---|---| +| Forward render | `kustomize build` | **identity** — `render(x) = x` (the Git document itself) | +| Field ownership | transformer/entry-aware | **all `Source`** — every field is owned by the file | +| Placement / wiring | append to `resources:` | placement only, **no wiring** (no-op) | +| Blind spots | postBuild, overrides, version | **still non-empty** — a plain file inside a Flux Kustomization is *also* subject to `postBuild` | + +That last row is the subtle, important one: "plain" does **not** mean "no out-of-band context." +The token fence runs on plain documents too (`plain-postbuild-token`), because a plain manifest +under a Flux Kustomization can still be substituted. So plain is a renderer whose *forward* and +*ownership* jobs are trivial but whose *blind spots* are real. + +This materially strengthens the case: the seam is not speculative, it already exists as a +runtime `if`. But be honest about what plain proves and what it does not: + +- ✅ It exercises jobs **1 and 2** (the ownership seam) — plain is the identity/all-`Source` + end of that axis. +- ❌ It barely exercises job **3** (placement/wiring) and does **not** exercise a *different + source model* at all — a plain file is still "a YAML document," the same leaf kustomize edits. + +So we are at N ≈ 1.5, not N = 2-that-covers-everything. Enough to justify *naming* a render +result and projection seam; not enough to *freeze* an interface. Helm is the model with a +genuinely different source shape, and it is the honesty test the interface must eventually pass. + +## The seam must carry evidence, not just ownership + +Most of the branches that bother us ask a related question: *what source edit, if any, can +reproduce this desired live change?* Field ownership is useful vocabulary, but this is not a +static question of a rendered object and a field path. For Kustomize it can depend on: + +- the selected render root and the exact source-document bytes and index; +- renderer provenance and source-to-rendered-object mapping; +- the desired live value, not just the current rendered value; +- the current render's external inputs and version/flag identity; and +- counterfactual rendering to prove that the planned edit produces the requested result. + +One source document may also be reached by several render roots. If their required changes differ, +the answer is not `Source`: it is an ambiguity refusal. An `images:` entry is likewise not +merely the owner of a scalar field; it is a specific control-file edit selected by pattern and +then verified by rendering. Patches, generated names, and list reshaping can make the inverse +unsupported even though forward rendering succeeds. + +The shape to expose is therefore an **evidence-backed projection plan**: + +```text +Render(request, context snapshot) + -> RenderResult { objects, source mapping, provenance, render identity, + resolved inputs, unresolved requirements } + +PlanChange(render result, source document, desired live object) + -> EditPlan { source/control-file edits, explanation, verification inputs } + | Refusal { AmbiguousSource | UnsupportedInverse | MissingExternalInput | ... } +``` + +`Ownership` can remain part of the explanation (`Source`, `Transformer`, `OutOfBand`, or +`Unknown`), but it is not sufficient authority to write. The current source-form rule, +images/replicas edit-through, and render-fidelity gate become consumers of this richer result: + +- **source form** can preserve an untouched source scalar while an edit plan updates a + transformer entry; +- **images/replicas** can name the exact override entry and prove it by counterfactual render; +- **out-of-band or unknown** can require a live comparison, external-input resolution, or a + refusal. + +Root selection and source-graph analysis should remain explicit rather than becoming hidden +methods on the renderer. They are format-specific policy and are already needed to enforce the +write fan-in boundary. + +```mermaid +flowchart TD + subgraph seam["evidence-backed decision"] + Q["Render result + desired change"] + end + Q --> S["Source → edit in place, then verify"] + Q --> T["Transformer → plan control-file edit, then verify"] + Q --> B["Out-of-band → resolve context or compare to live"] + Q --> U["Unknown / ambiguous → refusal"] + + subgraph impls["renderers supply different evidence"] + P["plain: everything Source"] + K["kustomize: transformers, entries, ${...} tokens"] + FK["flux-kustomize: + postBuild, substituteFrom"] + H["helm: values, templates, conditionals — different shape"] + end + impls -.-> Q +``` + +A sketch of **separate capabilities** (illustrative, **not** prescriptive): + +```go +type RenderEngine interface { + Render(req RenderRequest, snapshot ContextSnapshot) (RenderResult, error) +} + +type ChangeProjector interface { + PlanChange(result RenderResult, change DesiredChange) (EditPlan, *Refusal) +} + +// Optional: a format-specific capability, not a method every renderer must fake. +type NewObjectPlacer interface { + PlanNewObject(obj Object, layout SourceLayout) (EditPlan, *Refusal) +} +``` + +`RenderResult` must include enough evidence to explain and reproduce its answer: selected root, +source identity, render provenance, renderer/version/flag identity, and the identities and +resource versions of external inputs. It must never expose Secret values in status or logs. +`MissingExternalInput` and `Unsupported` are results, not hidden implementation failures. + +This makes the design tension visible: `plain` and `kustomize` are answerable today; +`flux-kustomize` needs a declared context snapshot; and Helm's new-object and inverse semantics +must be answered on paper *before* any interface is frozen. + +## Cases the seam must preserve + +The abstraction earns its keep only if the following behaviours stay explicit and testable. +These are contract cases, not merely implementation examples. + +| Case | Required result | +|---|---| +| Plain YAML, no external context | A direct source edit may be planned and verified. | +| Kustomize label overwrites a source `${TOKEN}` | If the *rendered* value equals live, mirror it; source text alone is not the oracle. | +| Kustomize label injects a rendered `${TOKEN}` which differs from live | Refuse; rendering revealed a fidelity mismatch. | +| `images:` / `replicas:` override | Plan an edit to the selected override entry, preserve the source field, and counterfactually re-render. | +| Patch/list reshaping or generated-name ambiguity | Return `UnsupportedInverse`/`AmbiguousSource`; never guess an indexed source edit. | +| One source reached by conflicting render roots | Refuse because write fan-in is not one, even if each individual render succeeds. | +| Flux `postBuild` / `substituteFrom` | Require a stable context snapshot. If an input is inaccessible, unresolved, or changes during the operation, refuse rather than render with a partial view. | +| Admission/runtime mutation after apply | Keep the render-vs-live verification fence; a renderer cannot claim ownership of mutation it cannot reproduce. | + +The existing [render-fidelity scenarios](render-fidelity-scenarios.md) are the initial corpus for +the second and third rows. New renderer work should add equivalent contract fixtures before it +changes write authority. + +## The strongest motivation: FluxKustomize / ArgoKustomize + +This is more than pluggability, and it is why the abstraction is worth doing even if Helm never +ships. The whole render-fidelity problem is that `applied = f(repo, orchestrator-context)` while +we compute `g(repo)`, and the token gate is a *detector* for `g ≠ f`. A `FluxKustomizeRenderer` +that reads the Flux Kustomization and applies `postBuild`/`substituteFrom` attempts to make +**`g` closer to `f`** by modelling the context — the "orchestrator awareness / interpreter model" +third fence the design already reserves +([orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md)). + +It does not make the fidelity gate disappear. First we must establish that the live object belongs +to one particular Flux Kustomization and snapshot. Overlapping orchestrators, unreadable +`substituteFrom` inputs, unsupported plugins or version semantics, and admission/runtime mutation +remain independent reasons to withhold a write. An orchestrator-aware renderer can become a +stronger input to the gate; it cannot relax the gate until its projected edit survives the same +counterfactual verification. + +Be clear-eyed about why it is not a free drop-in: + +- **It is not a pure function of the folder.** `substituteFrom` reads cluster ConfigMaps and + Secrets, so rendering needs a selected orchestrator binding and an immutable context snapshot, + not an unrestricted mutable `ClusterContext`. The snapshot identifies every external input and + its resource version, has least-privilege reads, and is rechecked before writing. (Same reason + the fidelity check cannot run at the offline structure-only gate.) +- **Version/flag fidelity.** Flux and Argo run *their* kustomize version with *their* flags. A + Go library linked into this controller cannot magically use the exact version embedded in a + running Flux controller. The design needs an explicit support matrix, or a deliberate way to + evaluate the orchestrator's own semantics; the interface only gives that responsibility a home. +- **The inverse must be context-aware too.** If live `us-east` came from a `postBuild` var, the + correct edit-through may be "edit the substitution ConfigMap" or "you cannot edit this here," + not "write the source file." A forward model that reports "no divergence" while the reverse + still writes to the wrong place is worse than refusing. + +## Recommendation and sequencing + +1. **Extract a result type, not a broad interface, without changing behaviour.** Make the + existing Kustomize renderer's current contract explicit: render provenance, source mapping, + counterfactual replacement, ambiguity, and existing refusal reasons. Keep outputs and write + decisions byte-identical. This is a locality win with no new authority. +2. **Pin those facts with contract fixtures.** Preserve the cases above, including rendered-not- + source token checks, override routing, fan-in refusal, and unsupported inverse cases. A future + implementation is conforming only if it keeps the same refusals as well as the same successes. +3. **Add a read-only Flux context resolver before a Flux writer.** Resolve one Flux binding into a + redacted, versioned snapshot and shadow-compare its render with the existing fidelity oracle. + On missing evidence, retain today's refusal. Do not grant it edit-through authority yet. +4. **Add context-aware projection only with counterfactual verification.** A planned source or + control-file edit must re-render, against the same snapshot, to the intended object. Recheck + snapshot identity before committing. Otherwise refuse. +5. **Only then judge Helm.** With render, projection, and context seams tested, write Helm's + new-object and inverse semantics on paper. If it needs different capabilities, add them; do + not force it to stub kustomize-shaped methods. +6. **Do not freeze interfaces until Flux exists in code and Helm exists on paper.** Freezing from + Kustomize plus the degenerate plain case alone is how Kustomize's shape becomes "the" shape. + +## Risks to hold in view + +- **Premature abstraction from a degenerate N.** Plain validates the ownership axis, not the + source-model axis. Treat the interface as provisional until a non-degenerate second renderer + stresses jobs 3 and 4. +- **Leaky abstraction.** An interface that hosts Helm only by having Helm stub kustomize methods + is a step backward. The forcing function is: *could a new renderer implement this without + lying?* +- **Mutable context is a correctness and security boundary.** Reads of a substitution Secret or + ConfigMap must be minimal, snapshotted, and evidenced without exposing values. Rendering with a + different set of inputs from the one used to verify a write is a time-of-check/time-of-use bug. +- **A successful forward render is not write authority.** The inverse can remain ambiguous or + unsupported. Keep refusal as the default until a planner identifies an edit and a + counterfactual render verifies it. +- **Deleting load-bearing knowledge.** Some kustomize `if`s encode real semantics — a `labels` + transform overwrites `metadata.labels` wholesale via `SetEntry`; a strategic-merge patch + *prepends* a container, so you cannot align lists by position (see `IssueUnplaceableEdit`). + The goal is to **relocate** that knowledge into the `KustomizeRenderer` behind a stable + question, **not** to erase it. If the abstraction deletes the knowledge, it deletes the + correctness. + +## Bottom line + +Yes to a seam — but its centre of gravity is **evidence-backed projection and explicit +blindness/refusal**, not `Render` alone and not a static `Owns` enum. `Render` is the easy 20%; +source mapping, inverse planning, context identity, and counterfactual verification are the 80% +that make the machinery safe to extend. Start by naming the current Kustomize result contract, +then let a read-only `FluxKustomize` experiment teach the context seam before it receives any +write authority or anything is frozen. + +## See also + +- [render-fidelity.md](render-fidelity.md) — the independent render-vs-live fence that remains + the verification oracle while modelled and live semantics can differ. +- [kustomize-token-writeback-explained.md](kustomize-token-writeback-explained.md) — the concrete + problem a `FluxKustomize` renderer would model away. +- [orchestrator-knowledge-boundary.md](orchestrator-knowledge-boundary.md) — reading the Flux / + Argo object; the `TransformedOutOfBand` claim these renderers would emit. +- [finished/images-and-replicas-edit-through.md](finished/images-and-replicas-edit-through.md) — + the source-form / projection machinery an evidence-backed edit planner would generalise. diff --git a/docs/facts/kustomize-never-emits-dollar-brace.md b/docs/facts/kustomize-never-emits-dollar-brace.md new file mode 100644 index 000000000..9ad7afafd --- /dev/null +++ b/docs/facts/kustomize-never-emits-dollar-brace.md @@ -0,0 +1,85 @@ +# Kustomize never emits a `${...}` token, verified from source + +> **reference** — durable background. Index: [`../INDEX.md`](../INDEX.md) +> +> Established by **reading the kustomize source code** in `external-sources/kustomize` at commit +> `6a1560da2` (`git describe`: `kustomize/v5.8.1-62-g6a1560da2`), plus executing the +> token regex against a table of vectors. These are source-derived + test-verified +> facts, not observations from a live cluster. + +## Why this matters + +The render-fidelity fence ([`RenderMatchesLive`](../design/support-boundary/render-fidelity.md)) uses a +`${...}` token as the lever to spot a value **our render did not produce**. Two facts make it reliable: + +- kustomize can neither create nor resolve a `${...}` token (facts 1–2 below), so any `${...}` left in + a render **output** was carried in verbatim from an *input* — a source document, or the kustomization + itself (e.g. a `patches:` block) — never minted by kustomize. +- So if a rendered field still holds a token but the **live** value there has *diverged*, our render + did not produce that live value. **What did is not knowable here — Flux `postBuild`, a direct live + edit, an admission mutation, another controller — and it does not need to be:** we cannot reproduce + the live value from the source, so the honest action is to refuse (`RenderDoesNotMatchLive`), not to + guess a cause. ("Must have been substituted" would be the guess.) + +**The fence must read the render output, not the source.** kustomize does not *preserve* every +token-bearing source field: a supported `labels` / `commonLabels` transform overwrites +`metadata.labels[...]` wholesale via `SetEntry` (`api/filters/labels/labels.go`). So a source +`metadata.labels.env: ${ENV}` under `labels: {env: prod}` renders to `env: prod` — no token, equal to +a live `prod`. Comparing the *source* to live would falsely refuse that faithful folder; comparing the +*render* (`dm.Rendered.Object`, or the Git document itself for a plain manifest) does not. + +## The three facts + +1. **Kustomize's own variable syntax is `$(...)`, not `${...}`.** The variable engine + `api/filters/refvar/expand.go` (under `external-sources/kustomize`) + is hard-coded to `operator='$'`, `referenceOpener='('`, `referenceCloser=')'`. In + `tryReadVariableName`, a `$` followed by `{` hits the `default` branch and is returned + **verbatim** (`isVar=false`). So kustomize never *parses or resolves* `${...}` — it + passes it straight through. There is also **no** `os.Expand` / `os.ExpandEnv` on + resource content anywhere in `api`/`plugin`/`kyaml`. + +2. **Kustomize never *originates* a `${...}` token.** A sweep of `api` + `plugin` + + `kyaml` for any string literal, raw string, or `Sprintf` building `${` in non-test, + non-comment Go returns **zero** hits (the only `${...}` in the tree are code comments + describing plugin-path templates). Every builtin generator/transformer is a + concat/copy operation, not a templating engine. `HelmChartInflationGenerator` merely + `exec.Command`s the `helm` binary, so any `${...}` in its output is chart-authored + source content. + +3. **Read parsed field *values*, not raw bytes.** Parsing removes comments, so a + `${var}` in `# commentary` never enters the predicate. A CRD schema `description` + is a real parsed scalar and **must** enter it: the literal `${var:=default}` that + broke CRD mirroring is safe because the rendered and live descriptions are equal, + not because descriptions receive a structural exemption. The live comparison, not + a field-name exception, avoids that false positive. + +## Token regex — shipped implementation + +`internal/manifestanalyzer/render_fidelity.go` uses `\$\{[^{}]+\}`. The gate treats every non-empty, +non-nested brace expression as a token; it deliberately excludes only `${}` and `$(...)`. This is +broader than the historical substitution regex, but remains safe under the same render-vs-live rule: +a match blocks only when the rendered scalar differs from live. + +Executed against these vectors: + +| String | Matches | Expected | Note | +|---|---|---|---| +| `${cluster_domain}` | ✅ | ✅ | plain token | +| `${schema.spec.replicas}` | ✅ | ✅ | dotted path | +| `${var:=default}` | ✅ | ✅ | Flux default-value syntax — caught | +| `prefix-${x}-suffix` | ✅ | ✅ | token mid-string | +| `$(POD_IP)` | ❌ | ❌ | parens, not braces | +| `$(kustomize_leftover_var)` | ❌ | ❌ | unresolved kustomize var — correctly ignored | +| `${}` | ❌ | ❌ | empty | +| `${ spaced }` | ✅ | ✅ | non-empty brace expression | +| `# a comment with ${var}` | ✅ | — | regex alone can't tell; fact 3 handles it | + +All rows matched expectation. + +## One nuance + +`$(POD_IP)` is not *only* "native Kubernetes env expansion" — `$(...)` is **also** +kustomize's own vars syntax, and an unresolved kustomize var is re-emitted as `$(FOO)` +(via `MakePrimitiveReplacer` → `syntaxWrap`). This strengthens the fence rather than +weakening it: a brace-only regex ignores leftover kustomize vars too, so it will not +false-positive on them. diff --git a/internal/controller/constants.go b/internal/controller/constants.go index 0f172b024..4d022875e 100644 --- a/internal/controller/constants.go +++ b/internal/controller/constants.go @@ -36,6 +36,8 @@ const ( ConditionTypeStreamsRunning = "StreamsRunning" // ConditionTypeGitPathAccepted indicates whether the GitTarget path is safe to materialize. ConditionTypeGitPathAccepted = "GitPathAccepted" + // ConditionTypeRenderMatchesLive indicates whether every current render scope agrees with live. + ConditionTypeRenderMatchesLive = "RenderMatchesLive" // ConditionTypeGitTargetReady indicates whether the referenced GitTarget is ready for writes. ConditionTypeGitTargetReady = "GitTargetReady" // ConditionTypeStreamsReady is a source-compatibility alias for StreamsRunning. diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index f74203334..f16b8e571 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -41,6 +41,7 @@ const ( GitTargetConditionValidated = "Validated" GitTargetConditionEncryptionConfigured = "EncryptionConfigured" GitTargetConditionGitPathAccepted = ConditionTypeGitPathAccepted + GitTargetConditionRenderMatchesLive = ConditionTypeRenderMatchesLive // GitTargetConditionStreamsRunning is the source data-plane axis: True when every tracked type's // watch has crossed its replay watermark or resumed from a durable cursor. GitTargetConditionStreamsRunning = ConditionTypeStreamsRunning @@ -80,7 +81,10 @@ const ( // planned write escaping spec.path (L1), or an in-place edit of a source file more than // one kustomize render root reaches (L2, write-fan-in > 1). Nothing was committed. The // string must stay in sync with the watch package's gitPathRefusalReason. - GitTargetReasonWriteBoundaryRefused = "WriteBoundaryRefused" + GitTargetReasonWriteBoundaryRefused = "WriteBoundaryRefused" + GitTargetReasonRenderMatchesLive = "RenderMatchesLive" + GitTargetReasonRenderDoesNotMatchLive = "RenderDoesNotMatchLive" + GitTargetReasonRenderRechecking = "Rechecking" GitTargetReadyReasonValidationFailed = "ValidationFailed" GitTargetReadyReasonEncryptionNotConfigured = "EncryptionNotConfigured" @@ -198,6 +202,9 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( Reason: GitTargetReasonGitPathAccepted, Message: "GitTarget path accepted", } + renderFidelity := watch.RenderFidelityStatus{ + State: "True", Reason: GitTargetReasonRenderMatchesLive, Message: "Every rendered token matches live", + } if r.EventRouter != nil && r.EventRouter.WatchManager != nil { gitDest := types.NewResourceReference(target.Name, target.Namespace).WithUID(string(target.UID)) if declareErr := r.EventRouter.WatchManager.DeclareForGitTarget( @@ -211,15 +218,17 @@ func (r *GitTargetReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } streams = r.EventRouter.WatchManager.StreamSummaryForGitTarget(gitDest) gitPath = r.EventRouter.WatchManager.GitPathAcceptanceForGitTarget(gitDest) + renderFidelity = r.EventRouter.WatchManager.RenderFidelityForGitTarget(gitDest) target.Status.Streams = gitTargetStreamsStatus(streams) - streamsSettling = streamsSettling || !streams.StreamsRunning() || !gitPath.Accepted + streamsSettling = streamsSettling || !streams.StreamsRunning() || !gitPath.Accepted || + renderFidelity.State == "Unknown" } else { streams = noResolvedStreamsSummary() target.Status.Streams = gitTargetStreamsStatus(streams) streamsSettling = true } - r.applyDataPlaneConditions(&target, streams, gitPath) + r.applyDataPlaneConditions(&target, streams, gitPath, renderFidelity) if err := r.updateStatusWithRetry(ctx, &target); err != nil { return ctrl.Result{}, err @@ -352,6 +361,13 @@ func (r *GitTargetReconciler) setBlockedDataPlane(target *configbutleraiv1alpha3 GitTargetStreamsRunningReasonNotReady, "Blocked by control-plane gate; streams not evaluated", ) + r.setCondition( + target, + GitTargetConditionRenderMatchesLive, + metav1.ConditionUnknown, + GitTargetReasonRenderRechecking, + "Blocked by control-plane gate; render fidelity not evaluated", + ) } func (r *GitTargetReconciler) setGitPathAcceptedUnknown( @@ -380,10 +396,13 @@ func (r *GitTargetReconciler) applyDataPlaneConditions( target *configbutleraiv1alpha3.GitTarget, streams watch.StreamSummary, gitPath watch.GitPathAcceptanceStatus, + renderFidelity watch.RenderFidelityStatus, ) { - d := deriveGitTargetDataPlaneStatus(streams, gitPath) + d := deriveGitTargetDataPlaneStatusWithRenderFidelity(streams, gitPath, renderFidelity) r.setCondition(target, GitTargetConditionStreamsRunning, d.StreamsStatus, d.StreamsReason, d.StreamsMessage) r.setCondition(target, GitTargetConditionGitPathAccepted, d.GitPathStatus, d.GitPathReason, d.GitPathMessage) + r.setCondition(target, GitTargetConditionRenderMatchesLive, d.RenderFidelityStatus, + d.RenderFidelityReason, d.RenderFidelityMessage) r.setCondition(target, GitTargetConditionReady, d.ReadyStatus, d.ReadyReason, d.ReadyMessage) r.setCondition( target, @@ -396,21 +415,24 @@ func (r *GitTargetReconciler) applyDataPlaneConditions( } type gitTargetDataPlaneDecision struct { - StreamsStatus metav1.ConditionStatus - StreamsReason string - StreamsMessage string - GitPathStatus metav1.ConditionStatus - GitPathReason string - GitPathMessage string - ReadyStatus metav1.ConditionStatus - ReadyReason string - ReadyMessage string - ReconcilingStatus metav1.ConditionStatus - ReconcilingReason string - ReconcilingMessage string - StalledStatus metav1.ConditionStatus - StalledReason string - StalledMessage string + StreamsStatus metav1.ConditionStatus + StreamsReason string + StreamsMessage string + GitPathStatus metav1.ConditionStatus + GitPathReason string + GitPathMessage string + RenderFidelityStatus metav1.ConditionStatus + RenderFidelityReason string + RenderFidelityMessage string + ReadyStatus metav1.ConditionStatus + ReadyReason string + ReadyMessage string + ReconcilingStatus metav1.ConditionStatus + ReconcilingReason string + ReconcilingMessage string + StalledStatus metav1.ConditionStatus + StalledReason string + StalledMessage string } func deriveGitTargetDataPlaneStatus( @@ -511,6 +533,66 @@ func deriveGitTargetDataPlaneStatus( } } +// deriveGitTargetDataPlaneStatusWithRenderFidelity layers the independent render-vs-live gate +// over the existing source-stream and Git-path decisions. A divergence stalls the target without +// changing GitPathAccepted; an incomplete epoch is progress, not failure. +func deriveGitTargetDataPlaneStatusWithRenderFidelity( + streams watch.StreamSummary, + gitPath watch.GitPathAcceptanceStatus, + renderFidelity watch.RenderFidelityStatus, +) gitTargetDataPlaneDecision { + decision := deriveGitTargetDataPlaneStatus(streams, gitPath) + decision.RenderFidelityStatus = metav1.ConditionTrue + decision.RenderFidelityReason = GitTargetReasonRenderMatchesLive + decision.RenderFidelityMessage = "Every rendered token matches live" + + switch renderFidelity.State { + case git.RenderFidelityFalse: + decision.RenderFidelityStatus = metav1.ConditionFalse + decision.RenderFidelityReason = GitTargetReasonRenderDoesNotMatchLive + decision.RenderFidelityMessage = renderFidelity.Message + case git.RenderFidelityUnknown: + decision.RenderFidelityStatus = metav1.ConditionUnknown + decision.RenderFidelityReason = GitTargetReasonRenderRechecking + decision.RenderFidelityMessage = renderFidelity.Message + case git.RenderFidelityTrue: + } + if renderFidelity.Reason != "" { + decision.RenderFidelityReason = renderFidelity.Reason + } + if decision.RenderFidelityMessage == "" { + decision.RenderFidelityMessage = "Waiting for render-vs-live verification" + } + + if !gitPath.Accepted || streams.Blocked > 0 || !streams.StreamsRunning() { + return decision + } + switch renderFidelity.State { + case git.RenderFidelityFalse: + decision.ReadyStatus = metav1.ConditionFalse + decision.ReadyReason = decision.RenderFidelityReason + decision.ReadyMessage = decision.RenderFidelityMessage + decision.ReconcilingStatus = metav1.ConditionFalse + decision.ReconcilingReason = decision.RenderFidelityReason + decision.ReconcilingMessage = "Reconciliation is stalled" + decision.StalledStatus = metav1.ConditionTrue + decision.StalledReason = decision.RenderFidelityReason + decision.StalledMessage = decision.RenderFidelityMessage + case git.RenderFidelityUnknown: + decision.ReadyStatus = metav1.ConditionFalse + decision.ReadyReason = decision.RenderFidelityReason + decision.ReadyMessage = decision.RenderFidelityMessage + decision.ReconcilingStatus = metav1.ConditionTrue + decision.ReconcilingReason = decision.RenderFidelityReason + decision.ReconcilingMessage = decision.RenderFidelityMessage + decision.StalledStatus = metav1.ConditionFalse + decision.StalledReason = ReasonProgressing + decision.StalledMessage = "Reconciliation is making progress" + case git.RenderFidelityTrue: + } + return decision +} + func (r *GitTargetReconciler) ensureEventStream( target *configbutleraiv1alpha3.GitTarget, providerNS string, diff --git a/internal/controller/gittarget_status_test.go b/internal/controller/gittarget_status_test.go index 63c10afb7..589eedc3b 100644 --- a/internal/controller/gittarget_status_test.go +++ b/internal/controller/gittarget_status_test.go @@ -125,11 +125,12 @@ func TestApplyDataPlaneConditions_SetsKstatusTrio(t *testing.T) { r.applyDataPlaneConditions(target, watch.StreamSummary{ Total: 1, Ready: 1, Reason: watch.StreamReasonAllStreamsReady, Message: "1/1 streams running", - }, watch.GitPathAcceptanceStatus{Accepted: true}) + }, watch.GitPathAcceptanceStatus{Accepted: true}, watch.RenderFidelityStatus{State: "True"}) require.True(t, isConditionTrue(target.Status.Conditions, GitTargetConditionReady)) require.True(t, isConditionTrue(target.Status.Conditions, GitTargetConditionStreamsRunning)) require.True(t, isConditionTrue(target.Status.Conditions, GitTargetConditionGitPathAccepted)) + require.True(t, isConditionTrue(target.Status.Conditions, GitTargetConditionRenderMatchesLive)) require.False(t, isConditionTrue(target.Status.Conditions, GitTargetConditionReconciling)) require.False(t, isConditionTrue(target.Status.Conditions, GitTargetConditionStalled)) } @@ -144,4 +145,28 @@ func TestSetBlockedDataPlane_MarksUnknownAndPending(t *testing.T) { require.NotNil(t, streamsRunning) assert.Equal(t, metav1.ConditionUnknown, streamsRunning.Status) assert.Equal(t, GitTargetStreamsRunningReasonNotReady, streamsRunning.Reason) + renderMatchesLive := conditionByType(target.Status.Conditions, GitTargetConditionRenderMatchesLive) + require.NotNil(t, renderMatchesLive) + assert.Equal(t, metav1.ConditionUnknown, renderMatchesLive.Status) +} + +func TestDeriveGitTargetDataPlaneStatusWithRenderFidelity(t *testing.T) { + streams := watch.StreamSummary{ + Total: 1, Ready: 1, Reason: watch.StreamReasonAllStreamsReady, Message: "1/1 streams running", + } + gitPath := watch.GitPathAcceptanceStatus{Accepted: true} + + unknown := deriveGitTargetDataPlaneStatusWithRenderFidelity( + streams, gitPath, watch.RenderFidelityStatus{State: "Unknown", Reason: "Rechecking", Message: "waiting"}) + assert.Equal(t, metav1.ConditionUnknown, unknown.RenderFidelityStatus) + assert.Equal(t, metav1.ConditionTrue, unknown.ReconcilingStatus) + assert.Equal(t, metav1.ConditionFalse, unknown.StalledStatus) + + diverged := deriveGitTargetDataPlaneStatusWithRenderFidelity( + streams, gitPath, + watch.RenderFidelityStatus{State: "False", Reason: "RenderDoesNotMatchLive", Message: "${REGION}"}) + assert.Equal(t, metav1.ConditionFalse, diverged.RenderFidelityStatus) + assert.Equal(t, metav1.ConditionFalse, diverged.ReadyStatus) + assert.Equal(t, metav1.ConditionTrue, diverged.StalledStatus) + assert.Equal(t, GitTargetReasonRenderDoesNotMatchLive, diverged.StalledReason) } diff --git a/internal/controller/stream_status.go b/internal/controller/stream_status.go index 4ada154dc..21e612dd3 100644 --- a/internal/controller/stream_status.go +++ b/internal/controller/stream_status.go @@ -126,6 +126,7 @@ func gitTargetReadyReasonIsStalled(reason string) bool { GitTargetReasonUnsupportedContent, GitTargetReasonIgnoreShadowsManagedPath, GitTargetReasonWriteBoundaryRefused, + GitTargetReasonRenderDoesNotMatchLive, GitTargetReadyReasonValidationFailed, GitTargetReadyReasonEncryptionNotConfigured, GitTargetReadyReasonWorkerUnavailable, diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index e2a12aba6..212436456 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -90,6 +90,11 @@ type BranchWorker struct { // by the WorkerManager before Start; a nil reporter only drops the status transition. pathRefusal PathRefusalReporter + // renderFidelityGate closes normal live writes while a target's current render-vs-live + // epoch is pending or divergent. Resync remains allowed so it can measure and repair Git. + // Set by WorkerManager before Start, alongside pathRefusal. + renderFidelityGate *RenderFidelityGate + // Event processing eventQueue chan WorkItem ctx context.Context @@ -690,13 +695,23 @@ func (l *branchWorkerEventLoop) handleQueueItem(item WorkItem) { if item.Request == nil { return } - if item.Request.CommitMode == CommitModeAtomic { + targetName, targetNamespace := atomicRefusalTarget(item.Request) + if !l.w.normalWritesAllowed(targetName, targetNamespace) { + l.w.Log.V(1).Info("Dropping atomic write while render fidelity is not established", + "gitTarget", targetNamespace+"/"+targetName) + return + } l.handleAtomicRequest(item.Request) return } for _, event := range item.Request.Events { + if !l.w.normalWritesAllowed(event.GitTargetName, event.GitTargetNamespace) { + l.w.Log.V(1).Info("Dropping live event while render fidelity is not established", + "gitTarget", event.GitTargetNamespace+"/"+event.GitTargetName) + continue + } if l.openWindow != nil && !l.openWindow.canAppend(event) { // Log the identity that broke the window so an unexpected split is // diagnosable: the common cause is an incoming event whose author is empty @@ -870,6 +885,12 @@ func (l *branchWorkerEventLoop) finalizeOpenWindowWithMessage(reason windowFinal if effectiveMessage == "" { effectiveMessage = l.openWindow.pendingMessage } + if !l.w.normalWritesAllowed(targetName, targetNamespace) { + l.w.Log.V(1).Info("Discarding open window while render fidelity is not established", + "reason", string(reason), "gitTarget", targetNamespace+"/"+targetName) + l.dropOpenWindow(pendingCR, errors.New("render fidelity gate is closed")) + return false + } l.w.Log.Info("Finalizing open commit window", "reason", string(reason), diff --git a/internal/git/git_path_refusal.go b/internal/git/git_path_refusal.go index 8ad81af1d..e8e3bc3e8 100644 --- a/internal/git/git_path_refusal.go +++ b/internal/git/git_path_refusal.go @@ -9,6 +9,13 @@ import ( itypes "github.com/ConfigButler/gitops-reverser/internal/types" ) +func (w *BranchWorker) normalWritesAllowed(targetName, targetNamespace string) bool { + if w.renderFidelityGate == nil || targetName == "" || targetNamespace == "" { + return true + } + return w.renderFidelityGate.AllowsWrites(itypes.NewResourceReference(targetName, targetNamespace)) +} + // PathRefusalReporter surfaces a refused write plan to the layer that owns GitTarget // status. A refusal is not a transient write fault: the acceptance gate or a write-boundary // precondition aborted the flush before any byte was written, nothing was committed, and only diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index 5f550fba1..119f41dc6 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -674,12 +674,17 @@ 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 { + var fidelity *renderFidelityRefusedError + if errors.As(err, &fidelity) { + return upsertNoChange, renderFidelityRefusal(filePath, id, fidelity) } + // 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 +720,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 +737,80 @@ 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 plain document uses its parsed Git object as its render. A kustomize document uses the +// DocumentModel's local render. In both cases, a rendered ${...} value that differs in live is +// refused before source-form projection can write the live expansion back into Git. +func projectThroughKustomize( + projected *unstructured.Unstructured, + content []byte, + idx int, + dm *manifestanalyzer.DocumentModel, +) (*unstructured.Unstructured, []manifestanalyzer.OverrideEdit, error) { + gitRaw, parsed := gitDocRawObject(content, idx) + if !parsed { + return projected, nil, nil + } + rendered := gitRaw + if dm.Rendered != nil { + rendered = dm.Rendered.Object + } + if divergences := manifestanalyzer.RenderTokenDivergences(rendered, projected.Object); len(divergences) > 0 { + return nil, nil, &renderFidelityRefusedError{Divergences: divergences} + } + if dm.Rendered == nil { + return projected, nil, nil + } + return manifestanalyzer.SplitDesiredForOverrides(gitRaw, projected, dm.Rendered) +} + +// renderFidelityRefusedError travels from the projection seam to patchExisting, where the file +// and object identity are available to make a normal write-boundary refusal. +type renderFidelityRefusedError struct { + Divergences []manifestanalyzer.RenderDivergence +} + +func (e *renderFidelityRefusedError) Error() string { + return "rendered token does not match live" +} + +// 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), + }}, + } +} + +func renderFidelityRefusal( + filePath string, + id manifestedit.Identity, + fidelity *renderFidelityRefusedError, +) error { + issues := make([]manifestanalyzer.AcceptanceIssue, 0, len(fidelity.Divergences)) + for _, divergence := range fidelity.Divergences { + issues = append(issues, manifestanalyzer.AcceptanceIssue{ + Kind: manifestanalyzer.IssueRenderDoesNotMatchLive, + Path: filePath, + Field: divergence.Field, + Token: divergence.Token, + Message: fmt.Sprintf("%s/%s in %s: rendered token %q at %s does not match live", + id.Kind, id.Name, filePath, divergence.Token, divergence.Field), + }) + } + return &manifestanalyzer.AcceptanceRefusedError{Issues: issues} +} + // 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/render_fidelity_gate.go b/internal/git/render_fidelity_gate.go new file mode 100644 index 000000000..ca1774845 --- /dev/null +++ b/internal/git/render_fidelity_gate.go @@ -0,0 +1,237 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "sort" + "sync" + + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// RenderFidelityScope is one independently replayed target-watch scope. Namespace is part of the +// key: a namespaced GitTarget can watch the same GVR in more than one namespace. +type RenderFidelityScope struct { + GVR schema.GroupVersionResource + Namespace string +} + +func (s RenderFidelityScope) key() string { + return s.GVR.String() + "|" + s.Namespace +} + +// RenderFidelityState is the three-state result of a complete render-vs-live epoch. +type RenderFidelityState string + +const ( + RenderFidelityUnknown RenderFidelityState = "Unknown" + RenderFidelityTrue RenderFidelityState = "True" + RenderFidelityFalse RenderFidelityState = "False" +) + +// RenderFidelityStatus is the target-level reduction of all scope results in one epoch. +// Unknown means the current epoch has not observed every scope; callers must not write live +// events while it is Unknown or False. +type RenderFidelityStatus struct { + Epoch uint64 + State RenderFidelityState + Reason string + Message string + Divergence *manifestanalyzer.RenderDivergence + ScopeCount int + CleanScopes int +} + +type renderFidelityScopeResult struct { + clean bool + finished bool + divergence *manifestanalyzer.RenderDivergence +} + +type renderFidelityTargetState struct { + epoch uint64 + scopes map[string]renderFidelityScopeResult +} + +// RenderFidelityGate is the concurrency-safe ownership point for the RenderMatchesLive state +// machine. A fresh epoch closes writes until every current scope reports clean. A single +// divergence latches False for that epoch; a later success from another scope cannot reopen it. +type RenderFidelityGate struct { + mu sync.RWMutex + targets map[string]renderFidelityTargetState +} + +// NewRenderFidelityGate creates an empty gate. Targets absent from it remain writable for +// backwards-compatible callers until their watch manager begins an epoch. +func NewRenderFidelityGate() *RenderFidelityGate { + return &RenderFidelityGate{targets: map[string]renderFidelityTargetState{}} +} + +// Begin starts a new epoch for target and replaces the complete scope set. It returns Unknown +// when scopes are pending, or True for the vacuous zero-scope case. +func (g *RenderFidelityGate) Begin( + target types.ResourceReference, + scopes []RenderFidelityScope, +) RenderFidelityStatus { + g.mu.Lock() + defer g.mu.Unlock() + if g.targets == nil { + g.targets = map[string]renderFidelityTargetState{} + } + state := g.targets[target.Key()] + state.epoch++ + state.scopes = make(map[string]renderFidelityScopeResult, len(scopes)) + for _, scope := range scopes { + state.scopes[scope.key()] = renderFidelityScopeResult{} + } + g.targets[target.Key()] = state + return reduceRenderFidelity(state) +} + +// RecordScopeClean records a completed clean result. It ignores stale epochs and results for a +// scope the current watch set no longer contains, returning applied=false in either case. +func (g *RenderFidelityGate) RecordScopeClean( + target types.ResourceReference, + epoch uint64, + scope RenderFidelityScope, +) (RenderFidelityStatus, bool) { + return g.recordScope(target, epoch, scope, nil) +} + +// RecordScopeDivergence records a render-vs-live mismatch for one completed scope. It latches +// the target False until Begin starts a newer epoch. +func (g *RenderFidelityGate) RecordScopeDivergence( + target types.ResourceReference, + epoch uint64, + scope RenderFidelityScope, + divergence manifestanalyzer.RenderDivergence, +) (RenderFidelityStatus, bool) { + return g.recordScope(target, epoch, scope, &divergence) +} + +func (g *RenderFidelityGate) recordScope( + target types.ResourceReference, + epoch uint64, + scope RenderFidelityScope, + divergence *manifestanalyzer.RenderDivergence, +) (RenderFidelityStatus, bool) { + g.mu.Lock() + defer g.mu.Unlock() + state, found := g.targets[target.Key()] + if !found || state.epoch != epoch { + return RenderFidelityStatus{}, false + } + result, found := state.scopes[scope.key()] + if !found { + return RenderFidelityStatus{}, false + } + // False is sticky within one epoch. A later clean replay from the same scope may be a retry + // of an older snapshot; only Begin is allowed to clear a divergence. + if result.divergence != nil && divergence == nil { + return reduceRenderFidelity(state), true + } + result.finished = true + result.clean = divergence == nil + result.divergence = divergence + state.scopes[scope.key()] = result + g.targets[target.Key()] = state + return reduceRenderFidelity(state), true +} + +// Fail closes a target immediately when a steady-state write discovers a divergence. It does not +// invent a successful scope result, so recovery still requires a complete fresh epoch. +func (g *RenderFidelityGate) Fail( + target types.ResourceReference, + divergence manifestanalyzer.RenderDivergence, +) RenderFidelityStatus { + g.mu.Lock() + defer g.mu.Unlock() + if g.targets == nil { + g.targets = map[string]renderFidelityTargetState{} + } + state, found := g.targets[target.Key()] + if !found { + state = renderFidelityTargetState{epoch: 1, scopes: map[string]renderFidelityScopeResult{"write": { + finished: true, divergence: &divergence, + }}} + } else { + state.scopes["write"] = renderFidelityScopeResult{finished: true, divergence: &divergence} + } + g.targets[target.Key()] = state + return reduceRenderFidelity(state) +} + +// Status returns the current status. An unregistered target is treated as True so adding the gate +// does not change callers that have no target watch lifecycle. +func (g *RenderFidelityGate) Status(target types.ResourceReference) RenderFidelityStatus { + if g == nil { + return renderFidelityReadyStatus(0, 0, 0) + } + g.mu.RLock() + defer g.mu.RUnlock() + state, found := g.targets[target.Key()] + if !found { + return renderFidelityReadyStatus(0, 0, 0) + } + return reduceRenderFidelity(state) +} + +// AllowsWrites reports whether a target may accept a normal live or atomic write. Resync work is +// deliberately not gated here: it is how the current epoch measures and repairs the Git tree. +func (g *RenderFidelityGate) AllowsWrites(target types.ResourceReference) bool { + return g.Status(target).State == RenderFidelityTrue +} + +// Forget removes a deleted GitTarget's state. +func (g *RenderFidelityGate) Forget(target types.ResourceReference) { + g.mu.Lock() + defer g.mu.Unlock() + delete(g.targets, target.Key()) +} + +func reduceRenderFidelity(state renderFidelityTargetState) RenderFidelityStatus { + keys := make([]string, 0, len(state.scopes)) + for key := range state.scopes { + keys = append(keys, key) + } + sort.Strings(keys) + clean := 0 + for _, key := range keys { + result := state.scopes[key] + if !result.finished { + continue + } + if result.divergence != nil { + sample := *result.divergence + return RenderFidelityStatus{ + Epoch: state.epoch, + State: RenderFidelityFalse, + Reason: "RenderDoesNotMatchLive", + Message: "Rendered token " + sample.Token + " at " + sample.Field + " does not match live", + Divergence: &sample, + ScopeCount: len(state.scopes), CleanScopes: clean, + } + } + if result.clean { + clean++ + } + } + if clean != len(state.scopes) { + return RenderFidelityStatus{ + Epoch: state.epoch, State: RenderFidelityUnknown, Reason: "Rechecking", + Message: "Waiting for every render scope in the current epoch", ScopeCount: len(state.scopes), + CleanScopes: clean, + } + } + return renderFidelityReadyStatus(state.epoch, len(state.scopes), clean) +} + +func renderFidelityReadyStatus(epoch uint64, scopes, clean int) RenderFidelityStatus { + return RenderFidelityStatus{ + Epoch: epoch, State: RenderFidelityTrue, Reason: "RenderMatchesLive", + Message: "Every rendered token matches live", ScopeCount: scopes, CleanScopes: clean, + } +} diff --git a/internal/git/render_fidelity_gate_test.go b/internal/git/render_fidelity_gate_test.go new file mode 100644 index 000000000..e1c85ef69 --- /dev/null +++ b/internal/git/render_fidelity_gate_test.go @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +func TestRenderFidelityGate_RequiresEveryScopeInEpoch(t *testing.T) { + gate := NewRenderFidelityGate() + target := types.NewResourceReference("apps", "default") + deployment := fidelityScope("apps", "deployments") + configMap := fidelityScope("", "configmaps") + + status := gate.Begin(target, []RenderFidelityScope{deployment, configMap}) + assert.Equal(t, RenderFidelityUnknown, status.State) + assert.False(t, gate.AllowsWrites(target)) + + status, applied := gate.RecordScopeClean(target, status.Epoch, deployment) + require.True(t, applied) + assert.Equal(t, RenderFidelityUnknown, status.State) + assert.False(t, gate.AllowsWrites(target)) + + status, applied = gate.RecordScopeClean(target, status.Epoch, configMap) + require.True(t, applied) + assert.Equal(t, RenderFidelityTrue, status.State) + assert.True(t, gate.AllowsWrites(target)) +} + +func TestRenderFidelityGate_IgnoresStaleEpochResult(t *testing.T) { + gate := NewRenderFidelityGate() + target := types.NewResourceReference("apps", "default") + scope := fidelityScope("apps", "deployments") + + first := gate.Begin(target, []RenderFidelityScope{scope}) + second := gate.Begin(target, []RenderFidelityScope{scope}) + _, applied := gate.RecordScopeClean(target, first.Epoch, scope) + assert.False(t, applied) + assert.Equal(t, RenderFidelityUnknown, gate.Status(target).State) + + status, applied := gate.RecordScopeClean(target, second.Epoch, scope) + require.True(t, applied) + assert.Equal(t, RenderFidelityTrue, status.State) +} + +func TestRenderFidelityGate_PerWriteDivergenceClosesTarget(t *testing.T) { + gate := NewRenderFidelityGate() + target := types.NewResourceReference("apps", "default") + scope := fidelityScope("apps", "deployments") + status := gate.Begin(target, []RenderFidelityScope{scope}) + _, applied := gate.RecordScopeClean(target, status.Epoch, scope) + require.True(t, applied) + + status = gate.Fail(target, manifestanalyzer.RenderDivergence{Field: "data.region", Token: "${REGION}"}) + assert.Equal(t, RenderFidelityFalse, status.State) + assert.Equal(t, "RenderDoesNotMatchLive", status.Reason) + assert.False(t, gate.AllowsWrites(target)) +} + +func TestRenderFidelityGate_FullFreshEpochReopensAfterGitRepair(t *testing.T) { + gate := NewRenderFidelityGate() + target := types.NewResourceReference("apps", "default") + deployment := fidelityScope("apps", "deployments") + configMap := fidelityScope("", "configmaps") + + first := gate.Begin(target, []RenderFidelityScope{deployment, configMap}) + _, applied := gate.RecordScopeDivergence(target, first.Epoch, deployment, + manifestanalyzer.RenderDivergence{Field: "data.region", Token: "${REGION}"}) + require.True(t, applied) + _, applied = gate.RecordScopeClean(target, first.Epoch, configMap) + require.True(t, applied) + assert.Equal(t, RenderFidelityFalse, gate.Status(target).State) + + second := gate.Begin(target, []RenderFidelityScope{deployment, configMap}) + assert.Equal(t, RenderFidelityUnknown, second.State) + _, applied = gate.RecordScopeClean(target, second.Epoch, deployment) + require.True(t, applied) + _, applied = gate.RecordScopeClean(target, second.Epoch, configMap) + require.True(t, applied) + assert.Equal(t, RenderFidelityTrue, gate.Status(target).State) + assert.True(t, gate.AllowsWrites(target)) +} + +func TestRenderFidelityGate_ClosedEpochCannotOpenLiveWindow(t *testing.T) { + gate := NewRenderFidelityGate() + target := types.NewResourceReference("apps", "default") + scope := fidelityScope("apps", "deployments") + status := gate.Begin(target, []RenderFidelityScope{scope}) + worker := &BranchWorker{ + contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), + renderFidelityGate: gate, + branchBufferMaxBytes: DefaultBranchBufferMaxBytes, + } + loop := newBranchWorkerEventLoop(worker, DefaultCommitWindow) + event := Event{GitTargetName: target.Name, GitTargetNamespace: target.Namespace, Operation: "UPDATE"} + + loop.handleQueueItem(WorkItem{Request: &WriteRequest{Events: []Event{event}, CommitMode: CommitModePerEvent}}) + assert.Nil(t, loop.openWindow, "Unknown must block a new live window") + + _, applied := gate.RecordScopeClean(target, status.Epoch, scope) + require.True(t, applied) + loop.handleQueueItem(WorkItem{Request: &WriteRequest{Events: []Event{event}, CommitMode: CommitModePerEvent}}) + assert.NotNil(t, loop.openWindow, "a complete clean epoch reopens normal writes") +} + +func fidelityScope(group, resource string) RenderFidelityScope { + return RenderFidelityScope{ + GVR: schema.GroupVersionResource{Group: group, Version: "v1", Resource: resource}, Namespace: "default", + } +} diff --git a/internal/git/render_fidelity_test.go b/internal/git/render_fidelity_test.go new file mode 100644 index 000000000..93668bd31 --- /dev/null +++ b/internal/git/render_fidelity_test.go @@ -0,0 +1,95 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "os" + "path/filepath" + "testing" + + gogit "github.com/go-git/go-git/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +const postBuildTokenManifest = `apiVersion: v1 +kind: ConfigMap +metadata: + name: region + namespace: default +data: + value: ${REGION} +` + +func postBuildTokenEvent() Event { + return Event{ + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{"name": "region", "namespace": "default"}, + "data": map[string]interface{}{"value": "us-east"}, + }}, + Identifier: types.ResourceIdentifier{ + Version: "v1", Resource: "configmaps", Namespace: "default", Name: "region", + }, + Operation: "UPDATE", + } +} + +func seedPostBuildTokenManifest(t *testing.T, root string) string { + t.Helper() + path := filepath.Join(root, "configmap.yaml") + require.NoError(t, os.WriteFile(path, []byte(postBuildTokenManifest), 0o600)) + return path +} + +// Both mutation paths reach the same boundary. In particular, resync must not be a back door +// that turns an externally resolved ${REGION} value into a source-file edit. +func TestRenderFidelityRefusal_BlocksLiveAndResyncWrites(t *testing.T) { + for _, test := range []struct { + name string + run func(worker *BranchWorker, worktree *gogit.Worktree) error + }{ + { + name: "live event", + run: func(worker *BranchWorker, worktree *gogit.Worktree) error { + _, err := worker.flushEventsToWorktree( + context.Background(), worktree, "", []Event{postBuildTokenEvent()}, nil) + return err + }, + }, + { + name: "scoped resync", + run: func(worker *BranchWorker, worktree *gogit.Worktree) error { + _, _, err := worker.applyResyncToWorktree( + context.Background(), worktree, "", + []manifestanalyzer.DesiredResource{{ + Resource: postBuildTokenEvent().Identifier, + Object: postBuildTokenEvent().Object, + }}, nil, nil) + return err + }, + }, + } { + t.Run(test.name, func(t *testing.T) { + worktree := newWorktreeForTest(t) + path := seedPostBuildTokenManifest(t, worktree.Filesystem.Root()) + worker := &BranchWorker{ + contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), + mapper: configMapMapper(), + } + + err := test.run(worker, worktree) + var refused *manifestanalyzer.AcceptanceRefusedError + require.ErrorAs(t, err, &refused) + assert.True(t, refused.AllIssuesOfKinds(manifestanalyzer.IssueRenderDoesNotMatchLive)) + assert.Contains(t, refused.Error(), "${REGION}") + assertFileBytes(t, path, postBuildTokenManifest, "a fidelity refusal must not change Git") + }) + } +} diff --git a/internal/git/source_form_test.go b/internal/git/source_form_test.go new file mode 100644 index 000000000..0ca6ff04b --- /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/git/worker_manager.go b/internal/git/worker_manager.go index 21889cfcb..61e8ea606 100644 --- a/internal/git/worker_manager.go +++ b/internal/git/worker_manager.go @@ -46,6 +46,10 @@ type WorkerManager struct { // once at startup (SetPathRefusalReporter) before any worker is created; nil in the // CLI and in tests that do not assert on the status transition. pathRefusal PathRefusalReporter + + // renderFidelityGate is shared by every worker and the watch manager. It is created with the + // manager so a target's state survives workers being recreated for the same branch. + renderFidelityGate *RenderFidelityGate } // NewWorkerManager creates a new worker manager. @@ -66,9 +70,18 @@ func NewWorkerManager( branchBufferMaxBytes: branchBufferMaxBytes, sensitiveResources: sensitiveResources, workers: make(map[BranchKey]*BranchWorker), + renderFidelityGate: NewRenderFidelityGate(), } } +// RenderFidelityGate returns the manager-wide target gate used by branch workers. The gate is +// safe for concurrent watch and worker access. +func (m *WorkerManager) RenderFidelityGate() *RenderFidelityGate { + m.mu.RLock() + defer m.mu.RUnlock() + return m.renderFidelityGate +} + // SetMapper injects the GVK->GVR resolver used by every worker's store scan. It is // called once at startup, before any GitTarget registers a worker, so each worker // created by EnsureWorker carries it. @@ -153,6 +166,7 @@ func (m *WorkerManager) EnsureWorker( worker.mapper = m.mapper worker.sshHostKeys = m.sshHostKeys worker.pathRefusal = m.pathRefusal + worker.renderFidelityGate = m.renderFidelityGate if err := worker.Start(m.ctx); err != nil { return fmt.Errorf("failed to start worker for %s: %w", key.String(), err) diff --git a/internal/manifestanalyzer/acceptance.go b/internal/manifestanalyzer/acceptance.go index 9602aaf0d..fedfe1514 100644 --- a/internal/manifestanalyzer/acceptance.go +++ b/internal/manifestanalyzer/acceptance.go @@ -134,6 +134,22 @@ 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" + // IssueRenderDoesNotMatchLive marks a rendered ${...} value whose corresponding live field is + // absent or different. It is a runtime fidelity refusal, distinct from structural Git-path + // acceptance and from a proposed write that fails the post-write render oracle. + IssueRenderDoesNotMatchLive IssueKind = "render-does-not-match-live" + // 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 diff --git a/internal/manifestanalyzer/analyzer.go b/internal/manifestanalyzer/analyzer.go index a1e41808b..fa6820f11 100644 --- a/internal/manifestanalyzer/analyzer.go +++ b/internal/manifestanalyzer/analyzer.go @@ -147,6 +147,10 @@ type AcceptanceIssue struct { Path string `json:"path"` DocumentIndex int `json:"documentIndex"` Message string `json:"message"` + // Field and Token add structured diagnostics for a render-fidelity refusal. They are empty + // for structural acceptance issues, which predate the render-vs-live gate. + Field string `json:"field,omitempty"` + Token string `json:"token,omitempty"` } // Report is the full result of analyzing a tree. diff --git a/internal/manifestanalyzer/analyzer_test.go b/internal/manifestanalyzer/analyzer_test.go index baee943b5..d9d52b979 100644 --- a/internal/manifestanalyzer/analyzer_test.go +++ b/internal/manifestanalyzer/analyzer_test.go @@ -105,14 +105,15 @@ func TestAnalyze_Issues(t *testing.T) { // the acceptance gate's mapping-aware kinds never appear here, so they are 0. All // kinds are listed so the exhaustive linter guards future additions. want := map[IssueKind]int{ - IssueDuplicate: 1, - IssueNonKRM: 1, - IssueInvalidYAML: 1, - IssueImpureManagedFile: 0, - IssueMixedFile: 0, - IssueUnresolvedKRM: 0, - IssueOutOfScope: 0, - IssueUnsupportedKustomize: 0, + IssueDuplicate: 1, + IssueNonKRM: 1, + IssueInvalidYAML: 1, + IssueImpureManagedFile: 0, + IssueMixedFile: 0, + IssueUnresolvedKRM: 0, + IssueOutOfScope: 0, + IssueUnsupportedKustomize: 0, + IssueRenderDoesNotMatchLive: 0, // Foreign-content, ignore-shadow, and the write-boundary refusals are // acceptance-gate / write-plan facts, not part of the structure-only Analyze report, // so they never surface here. IssueRenderRefused is the strongest case of that: it is @@ -125,6 +126,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 65907abca..30c0ed1af 100644 --- a/internal/manifestanalyzer/kustomization_parse.go +++ b/internal/manifestanalyzer/kustomization_parse.go @@ -58,10 +58,11 @@ func supportedKustomizationFields() map[string]struct{} { "Images": {}, "Replicas": {}, - // Tolerated exactly as before this change, so it stays a refactor. These - // inject metadata into rendered objects and therefore leak into mirrored - // source files as drift — a known defect, tracked separately, deliberately - // not altered here. + // These inject metadata into every rendered object. They used to leak into mirrored + // source files as drift — the writer mirrored the live object, injected labels and + // all, back into the file the overlay renders. That is fixed at the source: the + // projection leaves every field the BUILD supplies to the build (sourceForm), so an + // injected label is no longer something the file can absorb. "CommonLabels": {}, "Labels": {}, "CommonAnnotations": {}, diff --git a/internal/manifestanalyzer/kustomize_render_semantics_test.go b/internal/manifestanalyzer/kustomize_render_semantics_test.go index 8b652afca..6c5d4a748 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 f136e8df0..321ac91f7 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 cfbdc318d..8a1f0c086 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 14c2a2f7e..71db7e273 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 efcbddc50..e8cba0d10 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/render_fidelity.go b/internal/manifestanalyzer/render_fidelity.go new file mode 100644 index 000000000..9ea19af72 --- /dev/null +++ b/internal/manifestanalyzer/render_fidelity.go @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "regexp" + "sort" + "strconv" +) + +var renderTokenPattern = regexp.MustCompile(`\$\{[^{}]+\}`) + +// RenderDivergence identifies a rendered ${...} value that is not present unchanged in the +// sanitized live object. Field is a human-readable structured path; Token is the first token in +// the rendered scalar, retained so the refusal tells an operator why the field is protected. +type RenderDivergence struct { + Field string + Token string +} + +// RenderTokenDivergences returns every rendered ${...} scalar whose matching live field is absent +// or differs. It walks parsed object values, never YAML source bytes: comments therefore do not +// participate, and a token kustomize overwrote in the render is not considered. Only ${...} is a +// token; native Kubernetes $(...) syntax is deliberately outside this predicate. +func RenderTokenDivergences(rendered, live map[string]interface{}) []RenderDivergence { + var out []RenderDivergence + walkRenderTokens(rendered, live, "", &out) + return out +} + +func walkRenderTokens(rendered, live interface{}, field string, out *[]RenderDivergence) { + switch rendered := rendered.(type) { + case string: + match := renderTokenPattern.FindString(rendered) + if match == "" { + return + } + liveValue, livePresent := live.(string) + if !livePresent || liveValue != rendered { + *out = append(*out, RenderDivergence{Field: field, Token: match}) + } + case map[string]interface{}: + liveMap, _ := live.(map[string]interface{}) + keys := make([]string, 0, len(rendered)) + for key := range rendered { + keys = append(keys, key) + } + sort.Strings(keys) + for _, key := range keys { + liveValue := interface{}(nil) + if liveMap != nil { + liveValue = liveMap[key] + } + walkRenderTokens(rendered[key], liveValue, joinRenderFidelityField(field, key), out) + } + case []interface{}: + walkRenderTokenList(rendered, live, field, out) + } +} + +func walkRenderTokenList(rendered []interface{}, live interface{}, field string, out *[]RenderDivergence) { + liveList, _ := live.([]interface{}) + renderedByName, renderedNamed := renderFidelityNamedList(rendered) + liveByName, liveNamed := renderFidelityNamedList(liveList) + if renderedNamed && liveNamed { + for _, item := range rendered { + name := renderFidelityName(item) + walkRenderTokens(renderedByName[name], liveByName[name], renderFidelityListField(field, name), out) + } + return + } + for index, item := range rendered { + var liveItem interface{} + if index < len(liveList) { + liveItem = liveList[index] + } + walkRenderTokens(item, liveItem, renderFidelityListField(field, strconv.Itoa(index)), out) + } +} + +func renderFidelityNamedList(items []interface{}) (map[string]interface{}, bool) { + if len(items) == 0 { + return map[string]interface{}{}, true + } + byName := make(map[string]interface{}, len(items)) + for _, item := range items { + name := renderFidelityName(item) + if name == "" { + return nil, false + } + if _, duplicate := byName[name]; duplicate { + return nil, false + } + byName[name] = item + } + return byName, true +} + +func renderFidelityName(value interface{}) string { + item, ok := value.(map[string]interface{}) + if !ok { + return "" + } + name, _ := item["name"].(string) + return name +} + +func joinRenderFidelityField(field, key string) string { + if field == "" { + return key + } + return field + "." + key +} + +func renderFidelityListField(field, item string) string { + return field + "[" + item + "]" +} diff --git a/internal/manifestanalyzer/render_fidelity_test.go b/internal/manifestanalyzer/render_fidelity_test.go new file mode 100644 index 000000000..b6d7f031e --- /dev/null +++ b/internal/manifestanalyzer/render_fidelity_test.go @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "os" + "path/filepath" + "sort" + "testing" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/yaml" +) + +type renderFidelityWant struct { + Condition string `yaml:"condition"` + Reason string `yaml:"reason"` + Divergences []struct { + Field string `yaml:"field"` + Token string `yaml:"token"` + } `yaml:"divergences"` +} + +// TestRenderTokenDivergences_Fixtures keeps the render-vs-live contract in an isolated corpus. +// The fixtures deliberately do not use the broader layout corpus: each one owns both the Git +// tree and its sanitized live object, which is the only honest input to this predicate. +func TestRenderTokenDivergences_Fixtures(t *testing.T) { + root := filepath.Join("testdata", "render-fidelity") + entries, err := os.ReadDir(root) + require.NoError(t, err) + + for _, entry := range entries { + if !entry.IsDir() { + continue + } + name := entry.Name() + t.Run(name, func(t *testing.T) { + fixtureRoot := filepath.Join(root, name) + want := readRenderFidelityWant(t, filepath.Join(fixtureRoot, "want.yaml")) + rendered := renderFidelityFixture(t, filepath.Join(fixtureRoot, "git")) + live := readRenderFidelityObject(t, filepath.Join(fixtureRoot, "live.yaml")) + + got := RenderTokenDivergences(rendered, live) + if want.Condition == "True" { + require.Equal(t, "RenderMatchesLive", want.Reason) + require.Empty(t, got) + return + } + require.Equal(t, "False", want.Condition) + require.Equal(t, "RenderDoesNotMatchLive", want.Reason) + require.Len(t, got, len(want.Divergences)) + for index, expected := range want.Divergences { + require.Equal(t, expected.Field, got[index].Field) + require.Equal(t, expected.Token, got[index].Token) + } + }) + } +} + +func readRenderFidelityWant(t *testing.T, path string) renderFidelityWant { + t.Helper() + content, err := os.ReadFile(path) + require.NoError(t, err) + var want renderFidelityWant + require.NoError(t, yaml.Unmarshal(content, &want)) + return want +} + +func renderFidelityFixture(t *testing.T, root string) map[string]interface{} { + t.Helper() + files := readYAMLTree(t, root) + for _, file := range files { + if filepath.Base(file.Path) != "kustomization.yaml" && filepath.Base(file.Path) != "kustomization.yml" { + continue + } + rendered, err := renderRoot(files, ".") + require.NoError(t, err) + require.Len(t, rendered, 1, "each render-fidelity fixture owns exactly one object") + return rendered[0].Object.Object + } + sort.Slice(files, func(i, j int) bool { return files[i].Path < files[j].Path }) + require.Len(t, files, 1, "a plain render-fidelity fixture has one manifest") + return readRenderFidelityObject(t, filepath.Join(root, files[0].Path)) +} + +func readRenderFidelityObject(t *testing.T, path string) map[string]interface{} { + t.Helper() + content, err := os.ReadFile(path) + require.NoError(t, err) + var object map[string]interface{} + require.NoError(t, yaml.Unmarshal(content, &object)) + return (&unstructured.Unstructured{Object: object}).Object +} diff --git a/internal/manifestanalyzer/source_form.go b/internal/manifestanalyzer/source_form.go new file mode 100644 index 000000000..dda21c4e3 --- /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 000000000..24ec5d486 --- /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/testdata/render-fidelity/comment-only-token/git/configmap.yaml b/internal/manifestanalyzer/testdata/render-fidelity/comment-only-token/git/configmap.yaml new file mode 100644 index 000000000..162abbb81 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/comment-only-token/git/configmap.yaml @@ -0,0 +1,7 @@ +# ${REGION} is a comment; it is not a rendered value. +apiVersion: v1 +kind: ConfigMap +metadata: + name: comments +data: + region: us-east diff --git a/internal/manifestanalyzer/testdata/render-fidelity/comment-only-token/live.yaml b/internal/manifestanalyzer/testdata/render-fidelity/comment-only-token/live.yaml new file mode 100644 index 000000000..37b9dc830 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/comment-only-token/live.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: comments +data: + region: us-east diff --git a/internal/manifestanalyzer/testdata/render-fidelity/comment-only-token/want.yaml b/internal/manifestanalyzer/testdata/render-fidelity/comment-only-token/want.yaml new file mode 100644 index 000000000..5eae5eadc --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/comment-only-token/want.yaml @@ -0,0 +1,2 @@ +condition: "True" +reason: "RenderMatchesLive" diff --git a/internal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/git/configmap.yaml b/internal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/git/configmap.yaml new file mode 100644 index 000000000..58452e12d --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/git/configmap.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: labels +data: + value: unchanged diff --git a/internal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/git/kustomization.yaml b/internal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/git/kustomization.yaml new file mode 100644 index 000000000..115b63fea --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/git/kustomization.yaml @@ -0,0 +1,7 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +resources: + - configmap.yaml +labels: + - pairs: + env: ${ENV} diff --git a/internal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/live.yaml b/internal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/live.yaml new file mode 100644 index 000000000..85c4216bb --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/live.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: labels + labels: + env: prod +data: + value: unchanged diff --git a/internal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/want.yaml b/internal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/want.yaml new file mode 100644 index 000000000..623269357 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/label-injects-render-token/want.yaml @@ -0,0 +1,5 @@ +condition: "False" +reason: "RenderDoesNotMatchLive" +divergences: + - field: metadata.labels.env + token: ${ENV} diff --git a/internal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/git/configmap.yaml b/internal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/git/configmap.yaml new file mode 100644 index 000000000..8e0dde58f --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/git/configmap.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: labels + labels: + env: ${ENV} +data: + value: unchanged diff --git a/internal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/git/kustomization.yaml b/internal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/git/kustomization.yaml new file mode 100644 index 000000000..dab137434 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/git/kustomization.yaml @@ -0,0 +1,7 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +resources: + - configmap.yaml +labels: + - pairs: + env: prod diff --git a/internal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/live.yaml b/internal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/live.yaml new file mode 100644 index 000000000..85c4216bb --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/live.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: labels + labels: + env: prod +data: + value: unchanged diff --git a/internal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/want.yaml b/internal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/want.yaml new file mode 100644 index 000000000..5eae5eadc --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/label-overwrites-source-token/want.yaml @@ -0,0 +1,2 @@ +condition: "True" +reason: "RenderMatchesLive" diff --git a/internal/manifestanalyzer/testdata/render-fidelity/literal-crd-description/git/crd.yaml b/internal/manifestanalyzer/testdata/render-fidelity/literal-crd-description/git/crd.yaml new file mode 100644 index 000000000..c6cd875cc --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/literal-crd-description/git/crd.yaml @@ -0,0 +1,18 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: widgets.example.io +spec: + group: example.io + names: + kind: Widget + plural: widgets + scope: Namespaced + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + description: "${var:=default}" diff --git a/internal/manifestanalyzer/testdata/render-fidelity/literal-crd-description/live.yaml b/internal/manifestanalyzer/testdata/render-fidelity/literal-crd-description/live.yaml new file mode 100644 index 000000000..c6cd875cc --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/literal-crd-description/live.yaml @@ -0,0 +1,18 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: widgets.example.io +spec: + group: example.io + names: + kind: Widget + plural: widgets + scope: Namespaced + versions: + - name: v1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + description: "${var:=default}" diff --git a/internal/manifestanalyzer/testdata/render-fidelity/literal-crd-description/want.yaml b/internal/manifestanalyzer/testdata/render-fidelity/literal-crd-description/want.yaml new file mode 100644 index 000000000..5eae5eadc --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/literal-crd-description/want.yaml @@ -0,0 +1,2 @@ +condition: "True" +reason: "RenderMatchesLive" diff --git a/internal/manifestanalyzer/testdata/render-fidelity/literal-kro-template/git/resourcegraphdefinition.yaml b/internal/manifestanalyzer/testdata/render-fidelity/literal-kro-template/git/resourcegraphdefinition.yaml new file mode 100644 index 000000000..103365018 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/literal-kro-template/git/resourcegraphdefinition.yaml @@ -0,0 +1,15 @@ +apiVersion: kro.run/v1alpha1 +kind: ResourceGraphDefinition +metadata: + name: web +spec: + schema: + apiVersion: v1alpha1 + kind: Web + spec: + replicas: integer | default=1 + resources: + - id: deployment + template: + spec: + replicas: ${schema.spec.replicas} diff --git a/internal/manifestanalyzer/testdata/render-fidelity/literal-kro-template/live.yaml b/internal/manifestanalyzer/testdata/render-fidelity/literal-kro-template/live.yaml new file mode 100644 index 000000000..103365018 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/literal-kro-template/live.yaml @@ -0,0 +1,15 @@ +apiVersion: kro.run/v1alpha1 +kind: ResourceGraphDefinition +metadata: + name: web +spec: + schema: + apiVersion: v1alpha1 + kind: Web + spec: + replicas: integer | default=1 + resources: + - id: deployment + template: + spec: + replicas: ${schema.spec.replicas} diff --git a/internal/manifestanalyzer/testdata/render-fidelity/literal-kro-template/want.yaml b/internal/manifestanalyzer/testdata/render-fidelity/literal-kro-template/want.yaml new file mode 100644 index 000000000..5eae5eadc --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/literal-kro-template/want.yaml @@ -0,0 +1,2 @@ +condition: "True" +reason: "RenderMatchesLive" diff --git a/internal/manifestanalyzer/testdata/render-fidelity/literal-nginx-config/git/configmap.yaml b/internal/manifestanalyzer/testdata/render-fidelity/literal-nginx-config/git/configmap.yaml new file mode 100644 index 000000000..1ec5fdbb1 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/literal-nginx-config/git/configmap.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: nginx +data: + nginx.conf: "proxy_set_header Host ${host};" diff --git a/internal/manifestanalyzer/testdata/render-fidelity/literal-nginx-config/live.yaml b/internal/manifestanalyzer/testdata/render-fidelity/literal-nginx-config/live.yaml new file mode 100644 index 000000000..1ec5fdbb1 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/literal-nginx-config/live.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: nginx +data: + nginx.conf: "proxy_set_header Host ${host};" diff --git a/internal/manifestanalyzer/testdata/render-fidelity/literal-nginx-config/want.yaml b/internal/manifestanalyzer/testdata/render-fidelity/literal-nginx-config/want.yaml new file mode 100644 index 000000000..5eae5eadc --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/literal-nginx-config/want.yaml @@ -0,0 +1,2 @@ +condition: "True" +reason: "RenderMatchesLive" diff --git a/internal/manifestanalyzer/testdata/render-fidelity/native-dollar-paren/git/configmap.yaml b/internal/manifestanalyzer/testdata/render-fidelity/native-dollar-paren/git/configmap.yaml new file mode 100644 index 000000000..4afd0d982 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/native-dollar-paren/git/configmap.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: native-vars +data: + endpoint: "$(POD_IP):$(PORT)" diff --git a/internal/manifestanalyzer/testdata/render-fidelity/native-dollar-paren/live.yaml b/internal/manifestanalyzer/testdata/render-fidelity/native-dollar-paren/live.yaml new file mode 100644 index 000000000..20cf52b38 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/native-dollar-paren/live.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: native-vars +data: + endpoint: "$(POD_IP):9090" diff --git a/internal/manifestanalyzer/testdata/render-fidelity/native-dollar-paren/want.yaml b/internal/manifestanalyzer/testdata/render-fidelity/native-dollar-paren/want.yaml new file mode 100644 index 000000000..5eae5eadc --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/native-dollar-paren/want.yaml @@ -0,0 +1,2 @@ +condition: "True" +reason: "RenderMatchesLive" diff --git a/internal/manifestanalyzer/testdata/render-fidelity/nested-list-token/git/deployment.yaml b/internal/manifestanalyzer/testdata/render-fidelity/nested-list-token/git/deployment.yaml new file mode 100644 index 000000000..f92659de3 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/nested-list-token/git/deployment.yaml @@ -0,0 +1,13 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: list-token +spec: + template: + spec: + containers: + - name: app + image: example/app + env: + - name: REGION + value: ${REGION} diff --git a/internal/manifestanalyzer/testdata/render-fidelity/nested-list-token/live.yaml b/internal/manifestanalyzer/testdata/render-fidelity/nested-list-token/live.yaml new file mode 100644 index 000000000..5ed563ec8 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/nested-list-token/live.yaml @@ -0,0 +1,13 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: list-token +spec: + template: + spec: + containers: + - name: app + image: example/app + env: + - name: REGION + value: us-east diff --git a/internal/manifestanalyzer/testdata/render-fidelity/nested-list-token/want.yaml b/internal/manifestanalyzer/testdata/render-fidelity/nested-list-token/want.yaml new file mode 100644 index 000000000..595ff6f30 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/nested-list-token/want.yaml @@ -0,0 +1,5 @@ +condition: "False" +reason: "RenderDoesNotMatchLive" +divergences: + - field: spec.template.spec.containers[app].env[REGION].value + token: ${REGION} diff --git a/internal/manifestanalyzer/testdata/render-fidelity/plain-postbuild-token/git/deployment.yaml b/internal/manifestanalyzer/testdata/render-fidelity/plain-postbuild-token/git/deployment.yaml new file mode 100644 index 000000000..f465afe4d --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/plain-postbuild-token/git/deployment.yaml @@ -0,0 +1,13 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + template: + spec: + containers: + - name: app + image: example/web + env: + - name: REGION + value: ${REGION} diff --git a/internal/manifestanalyzer/testdata/render-fidelity/plain-postbuild-token/live.yaml b/internal/manifestanalyzer/testdata/render-fidelity/plain-postbuild-token/live.yaml new file mode 100644 index 000000000..6f5e0307b --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/plain-postbuild-token/live.yaml @@ -0,0 +1,13 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: web +spec: + template: + spec: + containers: + - name: app + image: example/web + env: + - name: REGION + value: us-east diff --git a/internal/manifestanalyzer/testdata/render-fidelity/plain-postbuild-token/want.yaml b/internal/manifestanalyzer/testdata/render-fidelity/plain-postbuild-token/want.yaml new file mode 100644 index 000000000..595ff6f30 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/plain-postbuild-token/want.yaml @@ -0,0 +1,5 @@ +condition: "False" +reason: "RenderDoesNotMatchLive" +divergences: + - field: spec.template.spec.containers[app].env[REGION].value + token: ${REGION} diff --git a/internal/manifestanalyzer/testdata/render-fidelity/token-field-removed-live/git/configmap.yaml b/internal/manifestanalyzer/testdata/render-fidelity/token-field-removed-live/git/configmap.yaml new file mode 100644 index 000000000..71b01a90a --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/token-field-removed-live/git/configmap.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: missing +data: + region: ${REGION} diff --git a/internal/manifestanalyzer/testdata/render-fidelity/token-field-removed-live/live.yaml b/internal/manifestanalyzer/testdata/render-fidelity/token-field-removed-live/live.yaml new file mode 100644 index 000000000..f60702872 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/token-field-removed-live/live.yaml @@ -0,0 +1,5 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: missing +data: {} diff --git a/internal/manifestanalyzer/testdata/render-fidelity/token-field-removed-live/want.yaml b/internal/manifestanalyzer/testdata/render-fidelity/token-field-removed-live/want.yaml new file mode 100644 index 000000000..f0fe8cf44 --- /dev/null +++ b/internal/manifestanalyzer/testdata/render-fidelity/token-field-removed-live/want.yaml @@ -0,0 +1,5 @@ +condition: "False" +reason: "RenderDoesNotMatchLive" +divergences: + - field: data.region + token: ${REGION} diff --git a/internal/watch/event_router.go b/internal/watch/event_router.go index 088c9764e..8a89d6e26 100644 --- a/internal/watch/event_router.go +++ b/internal/watch/event_router.go @@ -213,12 +213,13 @@ func (r *EventRouter) drainScopedResync( gitDest types.ResourceReference, key targetWatchKey, kind string, + renderFidelityEpoch uint64, resultCh chan git.ResyncResult, ) { select { case result := <-resultCh: if result.Err != nil { - r.handleScopedResyncError(gitDest, key, kind, result.Err) + r.handleScopedResyncError(gitDest, key, kind, renderFidelityEpoch, result.Err) return } r.Log.V(1).Info("per-type "+kind+" applied", @@ -226,6 +227,7 @@ func (r *EventRouter) drainScopedResync( "created", result.Stats.Created, "updated", result.Stats.Updated, "deleted", result.Stats.Deleted) if r.WatchManager != nil { r.WatchManager.MarkTargetGitPathAccepted(gitDest) + r.WatchManager.MarkTargetRenderFidelityScopeClean(gitDest, renderFidelityEpoch, key) } // Count an applied per-type RECONCILE as a completed GitTarget reconcile so the // per-pod counter advances after a restart — the drain signal the restart-reconcile @@ -248,10 +250,20 @@ func (r *EventRouter) handleScopedResyncError( gitDest types.ResourceReference, key targetWatchKey, kind string, + renderFidelityEpoch uint64, err error, ) { var refused *manifestanalyzer.AcceptanceRefusedError if errors.As(err, &refused) { + if refused.AllIssuesOfKinds(manifestanalyzer.IssueRenderDoesNotMatchLive) { + r.Log.Info("per-type "+kind+" found a render-vs-live divergence", + "gitDest", gitDest.String(), "gvr", key.GVR.String(), "detail", refused.Error()) + if r.WatchManager != nil { + r.WatchManager.MarkTargetRenderFidelityScopeDiverged( + gitDest, renderFidelityEpoch, key, renderFidelityDivergence(refused)) + } + return + } r.Log.Info("per-type "+kind+" refused: unsupported GitTarget path content", "gitDest", gitDest.String(), "gvr", key.GVR.String(), "detail", refused.Error()) if r.WatchManager != nil { @@ -263,6 +275,15 @@ func (r *EventRouter) handleScopedResyncError( r.recordBackgroundResyncFailure(gitDest) } +func renderFidelityDivergence(refused *manifestanalyzer.AcceptanceRefusedError) manifestanalyzer.RenderDivergence { + for _, issue := range refused.Issues { + if issue.Kind == manifestanalyzer.IssueRenderDoesNotMatchLive { + return manifestanalyzer.RenderDivergence{Field: issue.Field, Token: issue.Token} + } + } + return manifestanalyzer.RenderDivergence{} +} + // gitPathRefusalReason picks the GitTarget status reason for a refused path. Two refusal // shapes are distinct enough to name, because they tell an operator something the umbrella // reason does not: @@ -270,9 +291,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 +308,7 @@ func gitPathRefusalReason(refused *manifestanalyzer.AcceptanceRefusedError) stri manifestanalyzer.IssueWriteEscapesScope, manifestanalyzer.IssueWriteFanIn, manifestanalyzer.IssueRenderRefused, + manifestanalyzer.IssueUnplaceableEdit, ): return "WriteBoundaryRefused" default: diff --git a/internal/watch/event_router_test.go b/internal/watch/event_router_test.go index 4e4f76fbe..65ab260ec 100644 --- a/internal/watch/event_router_test.go +++ b/internal/watch/event_router_test.go @@ -124,6 +124,7 @@ func TestDrainScopedResync_CompletesSuccessfulResult(t *testing.T) { types.NewResourceReference("team-a-config", "team-a"), targetWatchKey{GVR: configmapsGVR}, "reconcile", + 0, resultCh, ) close(done) @@ -156,7 +157,7 @@ func TestDrainScopedResync_RefusalMarksGitPathRefused(t *testing.T) { resultCh := make(chan git.ResyncResult, 1) resultCh <- git.ResyncResult{Err: fmt.Errorf("execute pending writes: %w", refusal)} - router.drainScopedResync(gitDest, key, "reconcile", resultCh) + router.drainScopedResync(gitDest, key, "reconcile", 0, resultCh) gitPath := mgr.GitPathAcceptanceForGitTarget(gitDest) diff --git a/internal/watch/git_path_acceptance.go b/internal/watch/git_path_acceptance.go index 896efbf6b..aa364d764 100644 --- a/internal/watch/git_path_acceptance.go +++ b/internal/watch/git_path_acceptance.go @@ -18,6 +18,10 @@ func (m *Manager) ReportGitPathRefusal( gitDest types.ResourceReference, refused *manifestanalyzer.AcceptanceRefusedError, ) { + if refused.AllIssuesOfKinds(manifestanalyzer.IssueRenderDoesNotMatchLive) { + m.MarkTargetRenderFidelityDiverged(gitDest, renderFidelityDivergence(refused)) + return + } m.MarkTargetGitPathRefused(gitDest, gitPathRefusalReason(refused), refused.BlockMessage()) } diff --git a/internal/watch/git_path_acceptance_test.go b/internal/watch/git_path_acceptance_test.go index 72b165734..47bd9a87d 100644 --- a/internal/watch/git_path_acceptance_test.go +++ b/internal/watch/git_path_acceptance_test.go @@ -68,3 +68,56 @@ func TestReportGitPathRefusal_SatisfiesWorkerManagerReporter(t *testing.T) { var reporter git.PathRefusalReporter = (&Manager{Log: logr.Discard()}).ReportGitPathRefusal assert.NotNil(t, reporter) } + +func TestRenderFidelityStatus_ReducesCurrentEpochScopes(t *testing.T) { + workerManager := git.NewWorkerManager(nil, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + manager := &Manager{Log: logr.Discard()} + manager.EventRouter = NewEventRouter(workerManager, manager, nil, logr.Discard()) + target := types.NewResourceReference("podinfo-test", "team-a") + deployment := targetWatchKey{GVR: configmapsGVR, Namespace: "apps"} + other := targetWatchKey{GVR: configmapsGVR, Namespace: "ops"} + + manager.targetWatchesMu.Lock() + manager.beginTargetRenderFidelityEpochLocked(target, []targetWatchKey{deployment, other}) + epoch := manager.targetRenderFidelity[target.Key()].Epoch + manager.targetWatchesMu.Unlock() + + manager.MarkTargetRenderFidelityScopeClean(target, epoch, deployment) + assert.Equal(t, git.RenderFidelityUnknown, manager.RenderFidelityForGitTarget(target).State) + manager.MarkTargetRenderFidelityScopeDiverged(target, epoch, other, + manifestanalyzer.RenderDivergence{Field: "data.region", Token: "${REGION}"}) + assert.Equal(t, git.RenderFidelityFalse, manager.RenderFidelityForGitTarget(target).State) + + manager.MarkTargetRenderFidelityScopeClean(target, epoch, other) + assert.Equal(t, git.RenderFidelityFalse, manager.RenderFidelityForGitTarget(target).State, + "a later clean result cannot overwrite the failed scope in the same epoch") + + manager.targetWatchesMu.Lock() + manager.beginTargetRenderFidelityEpochLocked(target, []targetWatchKey{deployment, other}) + freshEpoch := manager.targetRenderFidelity[target.Key()].Epoch + manager.targetWatchesMu.Unlock() + manager.MarkTargetRenderFidelityScopeClean(target, epoch, deployment) + assert.Equal(t, git.RenderFidelityUnknown, manager.RenderFidelityForGitTarget(target).State, + "a stale result from the previous epoch must be ignored") + manager.MarkTargetRenderFidelityScopeClean(target, freshEpoch, deployment) + manager.MarkTargetRenderFidelityScopeClean(target, freshEpoch, other) + assert.Equal(t, git.RenderFidelityTrue, manager.RenderFidelityForGitTarget(target).State) +} + +func TestReportGitPathRefusal_RenderFidelityKeepsGitPathAccepted(t *testing.T) { + workerManager := git.NewWorkerManager(nil, logr.Discard(), 0, types.SensitiveResourcePolicy{}) + manager := &Manager{Log: logr.Discard()} + manager.EventRouter = NewEventRouter(workerManager, manager, nil, logr.Discard()) + target := types.NewResourceReference("podinfo-test", "team-a") + + manager.ReportGitPathRefusal(target, &manifestanalyzer.AcceptanceRefusedError{ + Issues: []manifestanalyzer.AcceptanceIssue{{ + Kind: manifestanalyzer.IssueRenderDoesNotMatchLive, Field: "data.region", Token: "${REGION}", + }}, + }) + + assert.True(t, manager.GitPathAcceptanceForGitTarget(target).Accepted) + fidelity := manager.RenderFidelityForGitTarget(target) + assert.Equal(t, git.RenderFidelityFalse, fidelity.State) + assert.Equal(t, "RenderDoesNotMatchLive", fidelity.Reason) +} diff --git a/internal/watch/manager.go b/internal/watch/manager.go index 7433865dd..1ca6048c5 100644 --- a/internal/watch/manager.go +++ b/internal/watch/manager.go @@ -23,6 +23,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" + "github.com/ConfigButler/gitops-reverser/internal/git" "github.com/ConfigButler/gitops-reverser/internal/rulestore" "github.com/ConfigButler/gitops-reverser/internal/telemetry" "github.com/ConfigButler/gitops-reverser/internal/types" @@ -157,6 +158,10 @@ type Manager struct { // targetGitPathAcceptance is the target-side acceptance surface. It is keyed by // GitTarget and projected into GitTarget status as GitPathAccepted. targetGitPathAcceptance map[string]GitPathAcceptanceStatus + // targetRenderFidelity is the projected state of the shared worker gate. It keeps the + // target-watch epoch observable without making a last successful scoped resync overwrite a + // sibling scope's divergence. + targetRenderFidelity map[string]git.RenderFidelityStatus // gitPathEventsCh carries a GenericEvent for a GitTarget whenever its GitPath acceptance // state TRANSITIONS, so the GitTarget controller re-projects GitPathAccepted promptly diff --git a/internal/watch/render_fidelity_gate.go b/internal/watch/render_fidelity_gate.go new file mode 100644 index 000000000..497e9fb72 --- /dev/null +++ b/internal/watch/render_fidelity_gate.go @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// RenderFidelityStatus is the GitTarget render-vs-live condition state shared with the writer. +type RenderFidelityStatus = git.RenderFidelityStatus + +func (m *Manager) fidelityGate() *git.RenderFidelityGate { + if m.EventRouter == nil || m.EventRouter.WorkerManager == nil { + return nil + } + return m.EventRouter.WorkerManager.RenderFidelityGate() +} + +func renderFidelityScopes(keys []targetWatchKey) []git.RenderFidelityScope { + scopes := make([]git.RenderFidelityScope, 0, len(keys)) + for _, key := range keys { + scopes = append(scopes, git.RenderFidelityScope{GVR: key.GVR, Namespace: key.Namespace}) + } + return scopes +} + +// beginTargetRenderFidelityEpochLocked replaces the target's scope set. targetWatchesMu must be +// held. The returned bool tells the caller to enqueue a status refresh after releasing the lock. +func (m *Manager) beginTargetRenderFidelityEpochLocked( + target types.ResourceReference, + keys []targetWatchKey, +) bool { + gate := m.fidelityGate() + if gate == nil { + return false + } + status := gate.Begin(target, renderFidelityScopes(keys)) + if m.targetRenderFidelity == nil { + m.targetRenderFidelity = map[string]git.RenderFidelityStatus{} + } + prior, had := m.targetRenderFidelity[target.Key()] + m.targetRenderFidelity[target.Key()] = status + return !had || renderFidelityStatusChanged(prior, status) +} + +// RenderFidelityEpochForGitTarget returns the epoch a replay result must carry. A zero epoch +// means no shared gate is wired, so callers preserve the legacy data path. +func (m *Manager) RenderFidelityEpochForGitTarget(target types.ResourceReference) uint64 { + m.targetWatchesMu.Lock() + defer m.targetWatchesMu.Unlock() + return m.targetRenderFidelity[target.Key()].Epoch +} + +// RenderFidelityForGitTarget returns the latest condition projection. Missing state means the +// target has not installed watches yet and remains writable for compatibility. +func (m *Manager) RenderFidelityForGitTarget(target types.ResourceReference) RenderFidelityStatus { + gate := m.fidelityGate() + if gate == nil { + return git.RenderFidelityStatus{State: git.RenderFidelityTrue, Reason: "RenderMatchesLive", + Message: "Every rendered token matches live"} + } + return gate.Status(target) +} + +// MarkTargetRenderFidelityScopeClean records one complete clean replay result from the current +// epoch. A stale cancellation tail is ignored by the gate and cannot reopen a failed target. +func (m *Manager) MarkTargetRenderFidelityScopeClean( + target types.ResourceReference, + epoch uint64, + key targetWatchKey, +) { + gate := m.fidelityGate() + if gate == nil || epoch == 0 { + return + } + status, applied := gate.RecordScopeClean( + target, + epoch, + git.RenderFidelityScope{GVR: key.GVR, Namespace: key.Namespace}, + ) + if applied { + m.recordRenderFidelityStatus(target, status) + } +} + +// MarkTargetRenderFidelityScopeDiverged records a replay refusal caused by a rendered token. +func (m *Manager) MarkTargetRenderFidelityScopeDiverged( + target types.ResourceReference, + epoch uint64, + key targetWatchKey, + divergence manifestanalyzer.RenderDivergence, +) { + gate := m.fidelityGate() + if gate == nil || epoch == 0 { + return + } + status, applied := gate.RecordScopeDivergence( + target, epoch, git.RenderFidelityScope{GVR: key.GVR, Namespace: key.Namespace}, divergence) + if applied { + m.recordRenderFidelityStatus(target, status) + } +} + +// MarkTargetRenderFidelityDiverged closes normal writes immediately when a live window hits the +// same boundary outside a scoped replay. A fresh watch epoch is the only recovery route. +func (m *Manager) MarkTargetRenderFidelityDiverged( + target types.ResourceReference, + divergence manifestanalyzer.RenderDivergence, +) { + gate := m.fidelityGate() + if gate == nil { + return + } + m.recordRenderFidelityStatus(target, gate.Fail(target, divergence)) +} + +func (m *Manager) recordRenderFidelityStatus(target types.ResourceReference, status git.RenderFidelityStatus) { + m.targetWatchesMu.Lock() + if m.targetRenderFidelity == nil { + m.targetRenderFidelity = map[string]git.RenderFidelityStatus{} + } + prior, had := m.targetRenderFidelity[target.Key()] + m.targetRenderFidelity[target.Key()] = status + changed := !had || renderFidelityStatusChanged(prior, status) + m.targetWatchesMu.Unlock() + if changed { + m.enqueueGitPathChange(target) + } +} + +func renderFidelityStatusChanged(before, after git.RenderFidelityStatus) bool { + return before.Epoch != after.Epoch || before.State != after.State || before.Reason != after.Reason || + before.Message != after.Message +} + +func (m *Manager) dropTargetRenderFidelityLocked(target types.ResourceReference) { + delete(m.targetRenderFidelity, target.Key()) + if gate := m.fidelityGate(); gate != nil { + gate.Forget(target) + } +} diff --git a/internal/watch/target_watch.go b/internal/watch/target_watch.go index 578026272..8e43c7792 100644 --- a/internal/watch/target_watch.go +++ b/internal/watch/target_watch.go @@ -136,7 +136,11 @@ func (m *Manager) replaceGitTargetWatches( ) } } + fidelityChanged := m.beginTargetRenderFidelityEpochLocked(table.GitDest, keys) m.targetWatchesMu.Unlock() + if fidelityChanged { + m.enqueueGitPathChange(table.GitDest) + } log := m.Log.WithName("target-watch").WithValues("gitDest", table.GitDest.String()) for _, watchKey := range keys { @@ -196,6 +200,7 @@ func (m *Manager) forgetGitTargetWatches(gitDest types.ResourceReference) { } m.dropTargetStreamStateLocked(gitDest) m.dropTargetGitPathAcceptanceLocked(gitDest) + m.dropTargetRenderFidelityLocked(gitDest) } func targetWatchSpecs(table WatchedTypeTable) map[targetWatchKey]string { @@ -270,8 +275,14 @@ func (m *Manager) runTargetWatch( key targetWatchKey, ops OperationSet, ) { + // A target-watch declaration defines the fidelity epoch. Its first session must replay even + // when a durable cursor exists: a replacement can add a sibling scope, and resuming an unchanged + // scope would otherwise leave that scope pending in the new epoch forever. Later reconnects may + // resume from their cursors because they stay within the same declaration and epoch. + resumeFromCursor := false for ctx.Err() == nil { - err := m.targetWatchReplayAndStream(ctx, log, gitDest, key, ops) + err := m.targetWatchReplayAndStream(ctx, log, gitDest, key, ops, resumeFromCursor) + resumeFromCursor = true if ctx.Err() != nil { return } @@ -292,9 +303,10 @@ func (m *Manager) targetWatchReplayAndStream( gitDest types.ResourceReference, key targetWatchKey, ops OperationSet, + resumeFromCursor bool, ) error { cursorExpired := false - if cursor, ok := m.lookupTargetWatchCursor(ctx, gitDest, key); ok { + if cursor, ok := m.lookupTargetWatchCursor(ctx, gitDest, key); resumeFromCursor && ok { err := m.targetWatchResumeAndStream(ctx, log, gitDest, key, ops, cursor) if !errors.Is(err, errTargetWatchExpired) { return err @@ -565,7 +577,9 @@ func (m *Manager) enqueueReplayResync( if m.EventRouter == nil { return nil } - resultCh, enqueued, err := m.EventRouter.enqueueScopedResync(ctx, gitDest, key.GVR, desired, revision, false) + epoch := m.RenderFidelityEpochForGitTarget(gitDest) + resultCh, enqueued, err := m.EventRouter.enqueueScopedResync( + ctx, gitDest, key.GVR, desired, revision, false) if err != nil { return err } @@ -576,7 +590,7 @@ func (m *Manager) enqueueReplayResync( // The key (GVR + namespace) is threaded to the drain for diagnostics. A refused // Git path acceptance is target-level state, so the drain records GitPathAccepted=False rather // than mutating this stream's watch readiness. - go m.EventRouter.drainScopedResync(gitDest, key, "reconcile", resultCh) + go m.EventRouter.drainScopedResync(gitDest, key, "reconcile", epoch, resultCh) log.V(1).Info("target replay resync enqueued", "gitDest", gitDest.String(), "gvr", key.GVR.String(), "revision", revision, "count", len(desired)) return nil diff --git a/internal/watch/target_watch_test.go b/internal/watch/target_watch_test.go index 65eb389f0..f3c97729b 100644 --- a/internal/watch/target_watch_test.go +++ b/internal/watch/target_watch_test.go @@ -65,8 +65,10 @@ func TestReplaceGitTargetWatches_ReusesUnchangedSetAndRestartsOnSpecChange(t *te defer cancel() opened := make(chan openedWatch, 4) + store := &fakeWatchCursorStore{rv: "41", ok: true} manager := &Manager{ - Log: logr.Discard(), + Log: logr.Discard(), + WatchCursorStore: store, targetWatchOpen: func( _ context.Context, _ schema.GroupVersionResource, @@ -78,6 +80,7 @@ func TestReplaceGitTargetWatches_ReusesUnchangedSetAndRestartsOnSpecChange(t *te return fw, nil }, } + manager.rememberGitTargetUID(gitDest.WithUID("uid-1")) first := WatchedTypeTable{ GitDest: gitDest, @@ -106,6 +109,10 @@ func TestReplaceGitTargetWatches_ReusesUnchangedSetAndRestartsOnSpecChange(t *te require.NoError(t, manager.replaceGitTargetWatches(ctx, changed)) restarted := receiveOpenedWatch(t, opened) assert.Equal(t, "apps", restarted.namespace) + assert.True(t, *restarted.opts.SendInitialEvents, + "a target-watch replacement must replay the existing scope for its new fidelity epoch") + assert.Empty(t, restarted.opts.ResourceVersion, + "the first session of a replacement must not resume an old epoch from a durable cursor") } func TestRouteLiveTargetWatchEvent_ForwardsObjectEventsAsCommitter(t *testing.T) { @@ -347,6 +354,7 @@ func TestTargetWatchReplayAndStream_ReturnsWhenContextCancels(t *testing.T) { types.NewResourceReference("target", "default"), targetWatchKey{GVR: configmapsGVR, Namespace: "apps"}, nil, + false, ) }() @@ -400,6 +408,7 @@ func TestTargetWatchReplayAndStream_FallsBackWhenReplayWatchIsForbidden(t *testi gitDest, targetWatchKey{GVR: configmapsGVR, Namespace: "apps"}, nil, + true, ) }() @@ -460,6 +469,7 @@ func TestTargetWatchReplayAndStream_ResumesFromStoredCursor(t *testing.T) { gitDest, targetWatchKey{GVR: configmapsGVR, Namespace: "apps"}, nil, + true, ) }() @@ -631,7 +641,7 @@ func TestTargetWatchReplayAndStream_ExpiredCursorFallsBackToFreshReplay(t *testi go func() { done <- manager.targetWatchReplayAndStream( ctx, logr.Discard(), gitDest, - targetWatchKey{GVR: configmapsGVR, Namespace: "apps"}, nil, + targetWatchKey{GVR: configmapsGVR, Namespace: "apps"}, nil, true, ) }() diff --git a/test/e2e/fixtures/render-fidelity/deployment.yaml b/test/e2e/fixtures/render-fidelity/deployment.yaml new file mode 100644 index 000000000..b8bb41b62 --- /dev/null +++ b/test/e2e/fixtures/render-fidelity/deployment.yaml @@ -0,0 +1,22 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: render-fidelity-postbuild + labels: + app.kubernetes.io/part-of: render-fidelity +spec: + replicas: 1 + selector: + matchLabels: + app: render-fidelity-postbuild + template: + metadata: + labels: + app: render-fidelity-postbuild + spec: + containers: + - name: app + image: nginx:1.27 + env: + - name: REGION + value: ${REGION} diff --git a/test/e2e/fixtures/render-fidelity/kustomization.yaml b/test/e2e/fixtures/render-fidelity/kustomization.yaml new file mode 100644 index 000000000..aafea77c7 --- /dev/null +++ b/test/e2e/fixtures/render-fidelity/kustomization.yaml @@ -0,0 +1,5 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +namespace: __E2E_NAMESPACE__ +resources: + - deployment.yaml diff --git a/test/e2e/render_fidelity_e2e_test.go b/test/e2e/render_fidelity_e2e_test.go new file mode 100644 index 000000000..726752989 --- /dev/null +++ b/test/e2e/render_fidelity_e2e_test.go @@ -0,0 +1,255 @@ +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +const ( + renderFidelityFixtureRoot = "test/e2e/fixtures/render-fidelity" + renderFidelityGitPath = "e2e/render-fidelity" + renderFidelityTimeout = 120 * time.Second +) + +// This is the real external-render-context proof for RenderMatchesLive. Flux postBuild resolves +// ${REGION} in the live Deployment, while gitops-reverser's local Kustomize render must retain the +// source token. The target therefore stalls and must not write either the expansion or a later, +// unrelated live change back to Git. It intentionally does not claim remote-Git repair works: the +// revision detector needed to start that fresh epoch is not implemented yet. +var _ = Describe("Manager Render Fidelity", Label("manager", "render-fidelity", "flux"), Ordered, func() { + var run renderFidelityRun + + BeforeAll(func() { + testNs := testNamespaceFor("manager-render-fidelity") + _, _ = kubectlRun("create", "namespace", testNs) + + repo := SetupRepo( + resolveE2EContext(), + testNs, + fmt.Sprintf("e2e-render-fidelity-%d", GinkgoRandomSeed()), + ) + _, err := kubectlRunInNamespace(testNs, "apply", "-f", repo.SecretsYAML) + Expect(err).NotTo(HaveOccurred(), "failed to apply Git credentials") + applySOPSAgeKeyToNamespace(testNs) + + run = newRenderFidelityRun(testNs, repo) + run.checkout.assertCheckoutReady() + }) + + AfterAll(func() { + run.cleanup() + }) + + It("refuses a real Flux postBuild expansion and preserves the Git source form", func() { + By("seeding the dedicated Kustomize source fixture") + fixture := renderInPlaceFixtureFolder(renderFidelityFixtureRoot, run.testNs) + DeferCleanup(func() { _ = os.RemoveAll(fixture) }) + seedRenderedFolderIntoRepo(run.repo, run.testNs, fixture, renderFidelityGitPath) + + seedHead := remoteBranchHead(Default, run.repo.CheckoutDir) + Expect(seedHead).NotTo(BeEmpty(), "the source fixture must be committed before Flux reads it") + + By("letting Flux resolve the postBuild variable in the live Deployment") + run.applyFluxGitRepository() + run.applyFluxKustomization() + run.reconcileFluxKustomization() + run.waitForFluxPostBuildValue() + + By("watching that path with gitops-reverser") + createReadyGitProvider(run.providerName, run.testNs, run.repo.GitSecretHTTP, run.repo.RepoURLHTTP) + createValidatedGitTarget(run.targetName, run.testNs, run.providerName, renderFidelityGitPath) + run.applyDeploymentWatchRule() + + By("reporting the render-vs-live divergence as a stalled target") + verifyResourceCondition( + "gittarget", run.targetName, run.testNs, + "RenderMatchesLive", "False", "RenderDoesNotMatchLive", "${REGION}", "150s", + ) + verifyResourceCondition( + "gittarget", run.targetName, run.testNs, + "Ready", "False", "RenderDoesNotMatchLive", "${REGION}", "150s", + ) + verifyResourceCondition( + "gittarget", run.targetName, run.testNs, + "Stalled", "True", "RenderDoesNotMatchLive", "${REGION}", "150s", + ) + waitForStreamsRunning(run.targetName, run.testNs) + + By("preserving the unresolved token and creating no reverse-GitOps commit") + run.consistentlyExpectSourceForm(seedHead) + + By("rejecting a later live edit while the gate remains closed") + _, err := kubectlRunInNamespace( + run.testNs, + "patch", + "deployment", + run.deploymentName, + "--type=merge", + "--patch={\"spec\":{\"replicas\":2}}", + ) + Expect(err).NotTo(HaveOccurred(), "failed to patch the live Deployment") + run.consistentlyExpectSourceForm(seedHead) + verifyResourceCondition( + "gittarget", run.targetName, run.testNs, + "RenderMatchesLive", "False", "RenderDoesNotMatchLive", "${REGION}", + ) + + By("Flux postBuild divergence stalled the target without changing its source tree") + }) +}) + +type renderFidelityRun struct { + testNs string + repo *RepoArtifacts + + checkout gitCheckout + + deploymentName string + providerName string + targetName string + watchRuleName string + + fluxSecretName string + fluxGitRepositoryName string + fluxKustomizationName string +} + +func newRenderFidelityRun(testNs string, repo *RepoArtifacts) renderFidelityRun { + id := strconv.FormatInt(time.Now().UnixNano(), 10) + return renderFidelityRun{ + testNs: testNs, + repo: repo, + checkout: newGitCheckout(repo, testNs), + deploymentName: "render-fidelity-postbuild", + providerName: fmt.Sprintf("render-fidelity-provider-%s", id), + targetName: fmt.Sprintf("render-fidelity-target-%s", id), + watchRuleName: fmt.Sprintf("render-fidelity-watchrule-%s", id), + fluxSecretName: fmt.Sprintf("render-fidelity-auth-%s", id), + fluxGitRepositoryName: fmt.Sprintf("render-fidelity-repo-%s", id), + fluxKustomizationName: fmt.Sprintf("render-fidelity-kustomization-%s", id), + } +} + +func (r renderFidelityRun) applyFluxGitRepository() { + username, password := r.checkout.readGitCredentialSecretDataBase64() + Expect(applyFromTemplate("test/e2e/templates/bi-directional/flux-gitrepository-http.tmpl", struct { + Namespace string + SecretName string + Name string + RepoURL string + Branch string + Interval string + Username string + Password string + }{ + Namespace: "flux-system", + SecretName: r.fluxSecretName, + Name: r.fluxGitRepositoryName, + RepoURL: r.repo.RepoURLHTTP, + Branch: "main", + Interval: "30m", + Username: username, + Password: password, + }, "flux-system")).To(Succeed(), "failed to apply Flux GitRepository") +} + +func (r renderFidelityRun) applyFluxKustomization() { + manifest := fmt.Sprintf(`apiVersion: kustomize.toolkit.fluxcd.io/v1 +kind: Kustomization +metadata: + name: %s + namespace: flux-system +spec: + interval: 30m + timeout: 2m + path: ./%s + prune: true + sourceRef: + kind: GitRepository + name: %s + postBuild: + substitute: + REGION: us-east +`, r.fluxKustomizationName, renderFidelityGitPath, r.fluxGitRepositoryName) + _, err := kubectlRunWithStdin("flux-system", manifest, "apply", "-f", "-") + Expect(err).NotTo(HaveOccurred(), "failed to apply Flux Kustomization") +} + +func (r renderFidelityRun) reconcileFluxKustomization() { + args := []string{ + "reconcile", "kustomization", r.fluxKustomizationName, + "-n", "flux-system", "--with-source", "--timeout", "90s", + } + if context := strings.TrimSpace(kubectlContext()); context != "" { + args = append([]string{"--context", context}, args...) + } + command := exec.Command("flux", args...) + output, err := command.CombinedOutput() + Expect(err).NotTo(HaveOccurred(), "flux %s: %s", strings.Join(args, " "), strings.TrimSpace(string(output))) +} + +func (r renderFidelityRun) waitForFluxPostBuildValue() { + Eventually(func(g Gomega) { + value, err := kubectlRunInNamespace( + r.testNs, + "get", + "deployment", + r.deploymentName, + "-o=jsonpath={.spec.template.spec.containers[0].env[0].value}", + ) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(value).To(Equal("us-east")) + }, renderFidelityTimeout, resourceConditionPollInterval).Should(Succeed()) +} + +func (r renderFidelityRun) applyDeploymentWatchRule() { + manifest := fmt.Sprintf(`apiVersion: configbutler.ai/v1alpha3 +kind: WatchRule +metadata: + name: %s + namespace: %s +spec: + targetRef: + kind: GitTarget + name: %s + rules: + - apiGroups: ["apps"] + apiVersions: ["v1"] + resources: ["deployments"] +`, r.watchRuleName, r.testNs, r.targetName) + _, err := kubectlRunWithStdin(r.testNs, manifest, "apply", "-f", "-") + Expect(err).NotTo(HaveOccurred(), "failed to apply Deployment WatchRule") +} + +func (r renderFidelityRun) consistentlyExpectSourceForm(seedHead string) { + Consistently(func(g Gomega) { + g.Expect(remoteBranchHead(g, r.repo.CheckoutDir)).To(Equal(seedHead), recentCommitDiagnostics( + r.repo.CheckoutDir, renderFidelityGitPath, + )) + pullLatestRepoState(g, r.repo.CheckoutDir) + content := readRepoFile(g, filepath.Join(r.repo.CheckoutDir, renderFidelityGitPath, "deployment.yaml")) + g.Expect(content).To(ContainSubstring("value: ${REGION}")) + g.Expect(content).To(ContainSubstring("replicas: 1")) + g.Expect(content).NotTo(ContainSubstring("replicas: 2")) + }, 20*time.Second, 2*time.Second).Should(Succeed()) +} + +func (r renderFidelityRun) cleanup() { + cleanupWatchRule(r.watchRuleName, r.testNs) + cleanupGitTarget(r.targetName, r.testNs) + cleanupNamespacedResource(r.testNs, "gitprovider", r.providerName) + cleanupNamespacedResource("flux-system", "kustomization", r.fluxKustomizationName) + cleanupNamespacedResource("flux-system", "gitrepository", r.fluxGitRepositoryName) + cleanupNamespacedResource("flux-system", "secret", r.fluxSecretName) + cleanupNamespace(r.testNs) +}