From c32f5259ff4a1f69098c905b38359f4d3178024f Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Thu, 9 Jul 2026 13:03:03 +0000 Subject: [PATCH 1/5] docs(gitops-api): GitTarget granularity, write boundary & cross-env edits (Track 1) Records the Track-1 design decisions: the two-layer write boundary (L1 write-scope jail + L2 fan-in=1), the GitTarget granularity fork (Option A for launch, C kept open for shared-defaults editing, B rejected as the operator write model), and cross-environment editing (product promotion now, base-as- variant later and only for shared defaults). Includes the who-renders split for floating/external sources and mermaid overviews + action-to-output examples. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...granularity-and-cross-environment-edits.md | 427 ++++++++++++++++++ 1 file changed, 427 insertions(+) create mode 100644 docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md diff --git a/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md b/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md new file mode 100644 index 00000000..d850b15b --- /dev/null +++ b/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md @@ -0,0 +1,427 @@ +# GitTarget granularity, the write boundary, and cross-environment edits + +> Status: direction-setting / options (no code change; opens two forks that +> feed F2 and gate the Track-1 write-boundary hardening) +> Captured: 2026-07-09 +> Related: +> [README.md](README.md), +> [kustomize-support-boundary-and-product-model.md](kustomize-support-boundary-and-product-model.md) +> (§4 invariant, §5 overlay model, §9 three arrows), +> [unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md), +> [../gitpath-foreign-content-stringency.md](../gitpath-foreign-content-stringency.md) + +## Purpose + +Two questions surfaced while scoping F2 that the existing docs do not settle, +and both change what the Track-1 write-boundary hardening should build: + +1. **Granularity** — is a `GitTarget` allowed at an *overlay* subfolder + (`overlays/test`), or only at the *app root* (`apps/podinfo`)? "Manage the + higher level as one thing" resolves some problems and creates others. +2. **Cross-environment edits** — the model today makes "change one thing in + every environment at once" (bump a version everywhere) deliberately + impossible for the operator (§9). People *will* want it. What is the honest + answer? + +This doc records the inputs already settled, lays out each fork as concrete +options with diagrams, and — because the user asked for it — walks a fixed set +of actions through each option so the expected output is never in doubt. + +## Settled inputs (so the options below are grounded) + +These are decided; they frame the forks but are not reopened here. + +- **The write boundary is two layers.** (§1 below.) **L1** — every path the + operator writes is inside the target's write scope (a filesystem jail). + **L2** — within the reachable graph, never write a file consumed by more than + one render root (fan-in = 1). Both are only *emergent* in today's code; making + them explicit and tested is the Track-1 prerequisite. +- **The admission webhook is a fail-open accelerator, not a correctness layer.** + It rejects unsavable edits at `kubectl apply` time for immediate, + *atomic* feedback, but it is opt-in, intent-mode-only, and `failurePolicy: + Ignore`. Correctness rests on L1 (base is never written even if the webhook is + off) plus the Tier-2 unreflected-set accounting + ([unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md)). +- **Floating / external sources split by *who renders*.** + - *An external control plane renders it* — Flux `Kustomization`, Argo CD + `Application`, Flux `HelmRelease`, KRO. These are opaque **intent** KRM to + us; we edit their fields in place, whether the referenced source is pinned + (`v1.2.3`) or floating (`>=1.0.0`, `main`, `latest`). The render is the + user's responsibility. **Accept.** + - *We render it* — a local `kustomization.yaml`'s own `resources:` / `bases:`. + Here the operator is the inverter, so a remote or floating source makes the + render non-deterministic and the round-trip unprovable. **Refuse the + folder.** This is not new refusal surface (remote bases and `helmCharts` are + already refused); it is the crisp *reason* and a commitment not to relax it. + +## Decision (2026-07-09) + +Both forks below are **decided — this is the user's call**, recorded here: + +- **Granularity: Option A for launch; keep the way open for Option C; reject + Option B as the operator write model.** A `GitTarget` is a *write partition* — + one overlay = environment = watch scope = write scope. B is rejected not only + on the L1-vs-L2 safety point but because, *even if L2 were perfect*, one target + spanning test/acceptance/production muddies authorization, audit, status, and + session lifecycle. "Manage the app as one thing" is a **product grouping** + concern (an aggregate/app concept the product layer can add later over N + targets), never a reason to widen an operator target across environments. +- **Cross-environment editing: product-layer promotion now; Option C later, and + only for *shared defaults* — not as the answer to "edit every environment."** + C (base-as-variant) edits the shared default/template and reaches only the + overlays that do **not** override that field; the honest "set every + environment's effective value" stays the product-layer Git operation (§3a). + Promotion and factor-into-base remain distinct verbs (§3/§4). 3c rejected. + +The rest of this doc keeps the full option analysis so the decision stays +legible; §2 and §3 mark the chosen paths. + +## 1. The write boundary, precisely + +Two nested guarantees. Keeping them distinct is what lets the granularity fork +in §2 be a real choice rather than a muddle. + +```mermaid +flowchart TD + W[Planned write to path P for object O] --> L1{L1: is P inside
this target's write scope?} + L1 -->|no| RJ[Refuse the flush — never write outside the jail] + L1 -->|yes| L2{L2: is P's file consumed
by more than one render root?} + L2 -->|yes| RF[Refuse this edit — record in the unreflected set] + L2 -->|no| OK[Write P] + RJ --> REP[Report: GitPathAccepted=False / FullyReflected=False] + RF --> REP +``` + +- **L1 is a filesystem fact.** Cheap (`filepath.Rel` prefix test), robust, and + independent of how well we model the render graph. It is what makes "the base + is read-only" true *by construction* when the base sits outside the write + scope. +- **L2 is a graph fact.** It catches the case L1 cannot: a file *inside* the + write scope that two render roots both consume (the `diamond-images` shape). +- **Read scope ⊋ write scope, always.** The operator must *read* shared context + (a base reached via `../../base`) to know which file supplied each live value, + but it *writes* only inside the jail. "Reading is fine, writing is not" is + literally L1: reads may leave the write scope; writes never do. +- **Overlap between targets is a *write-scope* question only.** Two overlays + sharing one read-only base is legal; their write scopes (the overlay dirs) + must stay disjoint. Today's overlap check already compares `spec.path` + (write scope) and treats siblings as non-overlapping — so it must not later + fold the wider read scope into ownership. + +**Today (Track-1 gap):** L1 holds only because write paths are always built +relative to the write root, and L2 is not enforced at all — an ambiguous +override chain *warns and writes through*, and the only thing preventing a +shared-file clobber is a coincidental namespace-ambiguity block. Track 1 turns +both into explicit, tested preconditions (L1 alongside the existing +`.gittargetignore` shadow check; L2 as a refusal instead of a write-through). + +## 2. Fork one — GitTarget granularity + +Given the reference layout: + +```text +apps/podinfo/ +├── base/ # no namespace; overlays inject it +└── overlays/ + ├── test/ # namespace: podinfo-test + ├── acceptance/ # namespace: podinfo-acc + └── production/ # namespace: podinfo-prod +``` + +there are three coherent ways to place `GitTarget`s over it. + +### Option A — Fine: one target per overlay (base is a read-only neighbour) + +The §5 design as written. Each overlay is its own target; the base sits +*outside* every write scope and is reached only for reading. + +```mermaid +flowchart TB + subgraph repo["apps/podinfo/"] + B["base/
(no owner — read-only)"] + subgraph ovs["overlays/"] + T["overlays/test/"] + AC["overlays/acceptance/"] + P["overlays/production/"] + end + end + GTt["GitTarget test → ns podinfo-test
write ⊆ overlays/test"] ==> T + GTa["GitTarget acc → ns podinfo-acc
write ⊆ overlays/acceptance"] ==> AC + GTp["GitTarget prod → ns podinfo-prod
write ⊆ overlays/production"] ==> P + T -. "read ../../base" .-> B + AC -. "read ../../base" .-> B + P -. "read ../../base" .-> B +``` + +- **Base read-only enforced by L1** (it is outside every write scope — the + strong, filesystem guarantee). +- Clean identity: environment = GitTarget = namespace = write scope = RBAC + scope. This is what §8/§9 lean on — "propose to test, read-only on prod" is a + namespace RoleBinding, and a session branch is naturally single-environment. +- **Cost:** F2 must teach the analyzer to *follow* `../../base` for reading + (today that reference is dropped). More `GitTarget` objects; onboarding emits + several per app. + +### Option B — Coarse: one target at the app root + +"Manage the higher level as one thing." One `GitTarget` owns the whole +`apps/podinfo` subtree, base included. + +```mermaid +flowchart TB + GT["GitTarget podinfo
write ⊆ apps/podinfo (whole subtree)"] + subgraph repo["apps/podinfo/ (one owner)"] + B["base/
(inside write scope —
read-only by L2 fan-in ONLY)"] + T["overlays/test/"] + AC["overlays/acceptance/"] + P["overlays/production/"] + end + GT ==> repo +``` + +- **What it resolves:** read scope = write scope = the owned subtree, so there + is no "read wider than write" machinery and no `../../base` escape to follow. + The overlap check stays trivially one-owner-per-app. +- **What it costs:** the base now sits *inside* the write scope, so the *only* + thing keeping it read-only is **L2** — the graph fan-in rule we just confirmed + is emergent and buggy today. The weaker guarantee becomes load-bearing exactly + where the blast radius is highest ("an edit in test writes base → changes + prod"). It also dissolves the clean identity: one target spans three + namespaces, its watch scope is all of them, and a session branch can mix + environments — which muddies RBAC, promotion, and session lifecycle. + +### Option C — Fine + base-as-variant + +Option A, plus the base is itself a target: a render root hydrated into its own +synthetic namespace. This is the enabler for the cross-environment fork (§3). + +```mermaid +flowchart TB + subgraph repo["apps/podinfo/"] + B["base/"] + T["overlays/test/"] + AC["overlays/acceptance/"] + P["overlays/production/"] + end + GTb["GitTarget base → ns podinfo-base
write ⊆ base/"] ==> B + GTt["GitTarget test → ns podinfo-test
write ⊆ overlays/test"] ==> T + GTa["GitTarget acc → ns podinfo-acc"] ==> AC + GTp["GitTarget prod → ns podinfo-prod"] ==> P + T -. read .-> B + AC -. read .-> B + P -. read .-> B +``` + +- **Base read-only from every overlay (L1), writable only through its own + target.** `base/` is `test`'s neighbour (fan-in > 1 across overlays → never + written from an overlay), but it is the *base* target's own render root + (fan-in = 1 there → writable). The write jail per target is unchanged; + ownership stays disjoint. +- **Nuance:** `base/` has no namespace, so the base target must inject a + synthetic one (`podinfo-base`) for the intent cluster and strip it on + write-back — which is exactly the existing "inherited namespaces are kept out + of file bytes on write" behaviour, reused. + +### Comparison + +| | A — per overlay | B — app root | C — A + base variant | +|---|---|---|---| +| Base kept read-only by | **L1** (filesystem) | **L2** (graph) ⚠ | **L1** | +| Read wider than write | yes (`../../base`) | no | yes | +| Identity env=target=ns | clean | broken (target spans envs) | clean | +| Session branch scope | one environment | can mix environments | one environment | +| RBAC per environment | namespace RoleBinding | coarse (whole app) | namespace RoleBinding | +| Onboarding output | N targets/app | 1 target/app | N+1 targets/app | +| "Edit all envs at once" | product-layer only (§3a) | tempting but unsafe | edit base variant (§3b) | +| New F2 machinery | follow `../../base` reads | none | follow reads + base variant | + +### Decision — Option A now, C kept open, B rejected + +**This is the chosen path (2026-07-09):** Option A for launch, Option C as an +additive later step (shared-defaults editing, §3b), and **B rejected as the +operator write model**. + +The decisive safety point stands: B makes the *weaker*, currently broken +guarantee (L2) the only thing between "edit test" and "change prod", where A and +C keep the base read-only by L1 — the filesystem guarantee Track 1 is already +hardening. But the case against B **does not even need L2**: a `GitTarget` should +be a **write partition**. Even with a perfect L2, one target spanning +test/acceptance/production muddies four things a per-overlay target keeps clean — +authorization (RBAC per namespace), audit (who changed which environment), status +(per-environment `Ready`/`FullyReflected`), and session lifecycle (a session +branch is one environment). "Manage the app as one thing" is a *product grouping* +concern — an aggregate/app concept the product layer can add over N targets +later — not a reason to make the operator's write unit span environments. + +A is also where the code and docs already point: the product model fixes +environment = GitTarget = watch scope = write scope with shared read-only bases +([kustomize-support-boundary-and-product-model.md §5](kustomize-support-boundary-and-product-model.md#L208)), +and the repo-walker already classifies an out-of-subtree base as a +forward-looking F2 gap (`overlay-fan-out-needs-f2`), not permanent unsupported +structure +([repowalk.go](../../../internal/manifestanalyzer/repowalk.go#L37)). A is the +direction of travel; this decision commits to it. + +**Onboarding UX is not a reason to pick B.** A produces N targets per app, but +object count is a product-presentation problem, not an API-shape problem: the +product layer groups the N targets as one app in its UI. Do not let "one object +is tidier" pull the write model into B. + +## 3. Fork two — "edit N environments at once" + +Today the operator cannot write the base, so "bump the image everywhere" has no +operator path — it is a product-layer Git computation (§9). That is correct as a +*safety* stance and wrong as a *product* stance: it is a top-three user request. +Three ways to answer it. + +### 3a. Product-layer Git operation (status quo, §9) + +The user bumps the tag in `test`; the operator lands the one-line +`overlays/test/kustomization.yaml` change; the product offers **promote** / +**factor into base** — a pure Git→Git copy/diff that opens a PR. The operator +never writes the base. + +- **Pro:** operator stays minimal; base edits are ordinary reviewed Git changes. +- **Con:** "all at once" is a product feature that must exist, and until it does + the answer is "edit three files." Not an *operator* capability. + +### 3b. Base-as-variant (Option C): edit the shared default explicitly + +The base is hydrated into a synthetic `podinfo-base` namespace that is a +**virtual editing surface, not a deployable environment**. Editing the object +*there* writes `base/` once — it changes the shared **default/template**. This is +**not** "edit every environment": the change reaches only the overlays that do +**not** override that field. If overlays carry an `images:` override, editing +`base/`'s image does not move them at all. + +```mermaid +sequenceDiagram + autonumber + participant U as User / product UI + participant IC as Intent cluster ns podinfo-base + participant R as Reverser base GitTarget + participant G as Git base/ + participant F as GitOps controllers + U->>IC: set image on the base variant object + IC-->>R: watch event in ns podinfo-base + R->>G: write base/ (fan-in = 1, base is this target's render root) + Note over G: PR then merge changes the shared default, reaching only non-overriding overlays + G-->>F: new main, overlays re-render base, effective change only where not overridden +``` + +- **What it is good for:** editing a *shared default* as a first-class, safe + gesture. Fan-in = 1 holds because `base/` is this target's own render root, and + the edit is explicit (a change in `podinfo-base`), never inferred from + per-environment observations. +- **What it is *not*:** the answer to "set every environment's effective value." + For any field an overlay overrides, the base edit is shadowed — the honest + every-environment change is the product-layer promotion of §3a. C edits + defaults; promotion sets effective values. +- **RBAC / blast radius:** writing `podinfo-base` changes the default under *all* + non-overriding environments at once, so it is not just another environment + edit — it needs its own **"global/defaults editor"** permission, distinct from + per-environment write access. +- **Intent-only semantics to design:** the synthetic base namespace is a virtual + surface with no workloads and is never a deploy target. Bases that inject + *multiple* namespaces, and **cluster-scoped** resources living in the base, + need explicit handling before C can ship — which namespace the virtual surface + uses, and how cluster-scoped objects are edited. + +### 3c. Multi-consumer collapse (rejected) + +The operator writes the base when it observes the *identical* edit in all N +overlays. **Reject:** it forces the operator to wait for and correlate N +environments before writing, it is racy, and a single-environment observation +plus a logic slip silently writes the base — reintroducing exactly the +"edit test changed prod" failure L1 exists to prevent. It also violates fan-in = +1 by design. Listed only to mark it considered and closed. + +### Decision — product promotion now, C later for shared defaults + +**Chosen (2026-07-09): 3a is the launch answer; 3b (Option C) comes later and +only for shared-defaults editing; 3c rejected.** The honest "set every +environment's effective value" is the product-layer Git operation — it falls out +of work already done (a tag bump in one overlay is a one-line diff to copy across +overlays). Base-as-variant is added when editing a shared *default* is worth the +virtual-namespace handling, and it is never sold as "edit everywhere." + +Keep **two distinct verbs** and never conflate them: + +- **Promotion** — copy an *effective* environment change across overlays + (product-layer Git → Git). This is how "make every environment 6.6.1" is done. +- **Factor into base** — refactor a shared *default* into `base/` (Option C's + surface, or a product refactor). This changes the template, not necessarily + every effective value. + +## 4. Worked examples: action → expected output + +Fixed action set, acting in `podinfo-test` unless noted, launch scope (F2+F4, +no F3). "Unreflected" = recorded in the unreflected set, `FullyReflected=False`, +reverted by hydration in intent mode +([unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md)). + +### Common to all options + +| Action | Expected output | +|---|---| +| `kubectl set image deploy/podinfo podinfo=…:6.6.1` | `images:` entry in `overlays/test/kustomization.yaml` | +| `kubectl scale deploy/podinfo --replicas=5` | `replicas:` entry in `overlays/test/kustomization.yaml` | +| `kubectl apply -f new-cronjob.yaml` (test-only) | new `overlays/test/cronjob.yaml` + `resources:` entry | +| edit an env var on the **base-owned** Deployment | **unreflected** (no destination until F3); webhook rejects at apply time if enabled | +| edit a `HelmRelease` chart version `6.0.0 → 6.1.0` (floating range or pinned) | in-place edit of the `HelmRelease` document — **accepted** (control plane renders it) | +| a `kustomization.yaml` gains `resources: [github.com/org/repo//base?ref=main]` | **folder refused** (`GitPathAccepted=False`) — we render kustomize; a remote/floating source is non-invertible | + +### Where the options differ — "bump image to 6.6.1 in *all* environments" + +| Option | Expected output of "bump everywhere" | +|---|---| +| **A** (per overlay) | not an operator action — product **promote** copies the one-line change into each overlay's `kustomization.yaml`, one PR | +| **B** (app root) | *tempting* to write `base/`, but that is the unsafe path — must still be refused/kept to promotion, so B buys nothing here while weakening L1→L2 | +| **C** (base variant) | editing `base/` changes the shared *default* only — but overlays override `images:`, so it does **not** bump those environments. "Every environment effective value" is still product promotion (A's answer). C fits editing a *default*, not this action | + +### Where the options differ — "how is base kept read-only" + +| Option | If the operator ever computed a write into `base/deployment.yaml` from a `podinfo-test` edit | +|---|---| +| **A** | impossible — `base/` is outside `overlays/test` (L1 refuses before planning) | +| **B** | possible in principle — only L2 fan-in stops it, and L2 is the emergent/buggy layer ⚠ | +| **C** | impossible from the test target (L1); the *base* target may write `base/`, but only from a `podinfo-base` edit | + +## 5. Consequences for the ladder and Track 1 + +- **Granularity is decided (A), which fixes the Track-1 investment.** Because A + keeps the base read-only by **L1**, Track 1 builds **L1 as an explicit + precondition** (the strong, cheap guarantee) and treats **L2 as a refusal** + (never write-through a multi-consumer file) — it never leans on L2 to protect + the base. +- **F2 scope gains one concrete capability under A/C:** follow `../../base` for + *reading* (today dropped), while the write jail stays at `spec.path`. Much of + the read-scope / render-root / out-of-subtree-base logic already exists + read-only in the F8 repo-walker and can be promoted into the live analyzer. +- **Cross-environment editing is decided:** product promotion at launch; Option + C (base-as-variant) later and only for editing *shared defaults*, not as the + "every environment" answer. Neither reopens the fan-in invariant. +- **Floating-source rule needs no new code, only docs + a test:** the + who-renders split is already how the gate behaves; state it in the support + contract and pin it with a corpus case (a `HelmRelease` with a floating range: + accepted; a `kustomization.yaml` with a remote base: refused). + +## 6. Decisions and remaining open items + +**Decided (2026-07-09) — the user's call:** + +1. **Granularity — Option A for launch, C kept open, B rejected** as the operator + write model. A `GitTarget` is a write partition (§2 Decision). +2. **Cross-environment edits — product promotion now, Option C later and only for + shared defaults**; 3c rejected. Promotion and factor-into-base stay distinct + verbs (§3 Decision). + +**Still open:** + +3. **Write-up placement** — fold the §1 L1/L2 model back into + [kustomize-support-boundary-and-product-model.md §4](kustomize-support-boundary-and-product-model.md) + (one canonical invariant statement), or keep §4 as the short invariant and let + this doc own the two-layer detail? +4. **Option C sub-questions (deferred with C):** the synthetic base namespace's + handling of multi-namespace bases and cluster-scoped resources, and the + separate "global/defaults editor" RBAC role (§3b). From 0a05a40d81dcaec82a0e635f8807c720839eb9f1 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Thu, 9 Jul 2026 13:03:03 +0000 Subject: [PATCH 2/5] feat(git): enforce L1 write-scope and L2 fan-in write-boundary preconditions Make the two write-boundary invariants explicit and tested rather than emergent, before any byte is written (Track 1 prerequisite for F2): - L1 (pathScopePrecondition): every planned write stays inside the GitTarget write scope; an absolute or ".."-escaping path refuses the flush (IssueWriteEscapesScope). Reads may reach shared context (../../base); writes never leave the scope. Defense-in-depth alongside ignoreShadowPrecondition. - L2 (fanInPrecondition): a planned in-place edit of a source file more than one kustomize render path reaches with override entries at stake (the diamond hole) is now refused (IssueWriteFanIn) instead of falling back to write- through. It fires only on an actual planned write, so the legitimate base- sharing layout (base doc is NamespaceNone, never dirty) is not refused; F2 render-root scoping generalizes the check. Store-side signal via ManifestStore.OverridesAmbiguousAt, derived from build-time diagnostics. Both surface as the umbrella UnsupportedContent GitTarget reason (message names the path + cause). Unit + end-to-end tests; unit coverage 75.2% -> 75.3%. Co-Authored-By: Claude Opus 4.8 (1M context) --- .coverage-baseline | 2 +- internal/git/plan_flush.go | 92 +++++++++++++ .../git/write_boundary_precondition_test.go | 130 ++++++++++++++++++ internal/manifestanalyzer/acceptance.go | 15 ++ internal/manifestanalyzer/analyzer_test.go | 7 +- internal/manifestanalyzer/overrides.go | 19 +++ internal/manifestanalyzer/overrides_test.go | 21 +++ 7 files changed, 283 insertions(+), 3 deletions(-) create mode 100644 internal/git/write_boundary_precondition_test.go diff --git a/.coverage-baseline b/.coverage-baseline index 903ec006..3c2d8494 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -75.2 +75.3 diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index 0e7fdca7..23724d2d 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -11,6 +11,7 @@ import ( "path" "path/filepath" "sort" + "strings" gogit "github.com/go-git/go-git/v5" "sigs.k8s.io/controller-runtime/pkg/log" @@ -822,9 +823,23 @@ func currentDocIndex(filePath string, content []byte, id manifestedit.Identity) // operator can no longer see) is never reached — the flush is refused and the GitTarget // fails before the file exists. func (wb *writeBatch) flush(ctx context.Context, worktree *gogit.Worktree, root, base string) (bool, error) { + // Write-plan preconditions run before any byte is touched, so a violation aborts the + // whole flush and commits nothing (each reuses the existing "refusal aborts before a file + // is written" seam). They enforce, at the one moment the planned paths are known, the two + // write-boundary invariants the operator must never break: the .gittargetignore shadow + // guard (§4.3), the L1 write-scope jail (writes stay inside spec.path), and the L2 + // write-fan-in = 1 rule (never write a live change through into context shared by more + // than one render root). See + // docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md §1. if err := wb.ignoreShadowPrecondition(); err != nil { return false, err } + if err := wb.pathScopePrecondition(); err != nil { + return false, err + } + if err := wb.fanInPrecondition(); err != nil { + return false, err + } logger := log.FromContext(ctx) changed := false for _, rel := range sortedBufferKeys(wb.buffers) { @@ -882,6 +897,83 @@ func (wb *writeBatch) ignoreShadowPrecondition() error { return &manifestanalyzer.AcceptanceRefusedError{Issues: issues} } +// pathScopePrecondition enforces the L1 write-boundary invariant: every planned write stays +// inside the GitTarget write scope (spec.path). It tests each created/edited (dirty) or +// removed (deleted) buffer path and refuses the whole flush — one IssueWriteEscapesScope per +// offender — if the base-relative path is absolute or escapes the subtree via "..". Reading +// shared context outside the scope is legitimate (the analyzer follows ../../base); writing +// outside it never is. Planned paths are base-relative by construction today (scan Rel plus +// ".."-free placement validation), so this is defense-in-depth made explicit and tested, +// symmetric to ignoreShadowPrecondition: it never write-then-detects. +func (wb *writeBatch) pathScopePrecondition() error { + var issues []manifestanalyzer.AcceptanceIssue + for _, rel := range sortedBufferKeys(wb.buffers) { + buf := wb.buffers[rel] + if !buf.dirty() && !buf.deleted() { + continue + } + if writePathEscapesScope(rel) { + issues = append(issues, manifestanalyzer.AcceptanceIssue{ + Kind: manifestanalyzer.IssueWriteEscapesScope, + Path: rel, + Message: fmt.Sprintf( + "planned write path %q escapes the GitTarget write scope: the operator only ever writes "+ + "inside spec.path (reads may reach shared context such as ../../base, writes never leave it)", + rel), + }) + } + } + if len(issues) == 0 { + return nil + } + return &manifestanalyzer.AcceptanceRefusedError{Issues: issues} +} + +// writePathEscapesScope reports whether a base-relative planned write path would land outside +// the GitTarget subtree — an empty path (no destination), an absolute path, or one whose +// cleaned form still climbs above the base with "..". +func writePathEscapesScope(rel string) bool { + if rel == "" || path.IsAbs(rel) { + return true + } + clean := path.Clean(rel) + return clean == ".." || strings.HasPrefix(clean, "../") +} + +// fanInPrecondition enforces the L2 write-boundary invariant: never write a live change +// through into a source file more than one kustomize render path reaches with override +// entries at stake (write-fan-in > 1). It refuses the whole flush — one IssueWriteFanIn per +// offending path — when a dirty/deleted buffer targets a file the store flagged +// reasonAmbiguousOverrides. This replaces the former warn-and-write-through fallback with a +// refusal, so the fan-in guarantee no longer depends on the emergent side effect of namespace +// ambiguity blocking the match. It fires only on an actual planned write, so the legitimate +// base-sharing layout (a base doc reached by distinct overlays is NamespaceNone and never +// dirty) is not refused — F2 render-root scoping generalizes the check. +func (wb *writeBatch) fanInPrecondition() error { + var issues []manifestanalyzer.AcceptanceIssue + for _, rel := range sortedBufferKeys(wb.buffers) { + buf := wb.buffers[rel] + if !buf.dirty() && !buf.deleted() { + continue + } + if wb.store.OverridesAmbiguousAt(rel) { + issues = append(issues, manifestanalyzer.AcceptanceIssue{ + Kind: manifestanalyzer.IssueWriteFanIn, + Path: rel, + Message: fmt.Sprintf( + "planned write to %q would edit in place a source file that more than one kustomize render "+ + "path reaches with override entries at stake (write-fan-in must be 1); refusing rather than "+ + "writing the change through into context shared by multiple render roots", + rel), + }) + } + } + if len(issues) == 0 { + return nil + } + return &manifestanalyzer.AcceptanceRefusedError{Issues: issues} +} + // writeAndStageFile writes a file's bytes to disk (creating parent directories) and // stages it in the worktree. func writeAndStageFile(worktree *gogit.Worktree, worktreePath, fullPath string, content []byte) error { diff --git a/internal/git/write_boundary_precondition_test.go b/internal/git/write_boundary_precondition_test.go new file mode 100644 index 00000000..35e04c99 --- /dev/null +++ b/internal/git/write_boundary_precondition_test.go @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// The write-boundary preconditions (Track 1) make two invariants explicit and tested rather +// than emergent, before any byte is written. See +// docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md §1: +// - L1: every planned write stays inside the GitTarget write scope (spec.path). +// - L2: never write a live change through into a source file more than one render root +// reaches with override entries at stake (write-fan-in must be 1). + +// TestWritePathEscapesScope pins the L1 containment predicate: only an absolute, empty, or +// "..".-escaping base-relative path leaves the write scope; nested paths and a "../" that +// cleans back inside stay in. +func TestWritePathEscapesScope(t *testing.T) { + cases := []struct { + rel string + want bool + }{ + {"default/pods/web.yaml", false}, + {"a/b/c.yaml", false}, + {"a/../b.yaml", false}, // cleans to b.yaml, still inside + {"", true}, + {"/etc/passwd", true}, + {"..", true}, + {"../escape.yaml", true}, + {"../../base/deployment.yaml", true}, + {"a/../../escape.yaml", true}, + } + for _, c := range cases { + if got := writePathEscapesScope(c.rel); got != c.want { + t.Errorf("writePathEscapesScope(%q) = %v, want %v", c.rel, got, c.want) + } + } +} + +// TestPathScopePrecondition_RefusesEscapingWrite proves the L1 write-plan precondition: a +// planned write inside the scope is fine, and one whose path escapes the subtree refuses the +// whole flush with IssueWriteEscapesScope, naming the path — before any byte is written. +func TestPathScopePrecondition_RefusesEscapingWrite(t *testing.T) { + wb := &writeBatch{buffers: map[string]*fileBuffer{}} + wb.buffers["default/cm.yaml"] = &fileBuffer{rel: "default/cm.yaml", current: []byte("a")} + require.NoError(t, wb.pathScopePrecondition(), "a write inside the scope must be allowed") + + wb.buffers["../escape.yaml"] = &fileBuffer{rel: "../escape.yaml", current: []byte("b")} + kinds := refusalIssueKinds(t, wb.pathScopePrecondition()) + assert.Contains(t, kinds, manifestanalyzer.IssueWriteEscapesScope, + "a write escaping spec.path must refuse the flush") +} + +// diamondDeploymentYAML is the shared base document a single render root reaches two ways +// (through overlays a and b) with differing images entries — the ambiguity the writer must +// never resolve by writing through into the shared file. +const diamondDeploymentYAML = `apiVersion: apps/v1 +kind: Deployment +metadata: + name: web + namespace: default +spec: + selector: + matchLabels: + app: web + template: + metadata: + labels: + app: web + spec: + containers: + - name: podinfo + image: ghcr.io/example/podinfo:6.3.0 +` + +// diamondOverlayKust builds an overlay kustomization that references the shared base and +// pins the podinfo image to newTag — a and b differ, so the two chains reaching base conflict. +func diamondOverlayKust(newTag string) string { + return "resources:\n - ../base\nimages:\n - name: ghcr.io/example/podinfo\n newTag: \"" + newTag + "\"\n" +} + +// seedDiamond writes a minimal single-root diamond: root → a → base and root → b → base, +// where a and b carry differing images entries so base/deployment.yaml is reached by two +// distinct override chains (write-fan-in > 1). +func seedDiamond(t *testing.T, root string) { + t.Helper() + files := map[string]string{ + "kustomization.yaml": "resources:\n - a\n - b\n", + "a/kustomization.yaml": diamondOverlayKust("1.0.0"), + "b/kustomization.yaml": diamondOverlayKust("2.0.0"), + "base/kustomization.yaml": "resources:\n - deployment.yaml\n", + "base/deployment.yaml": diamondDeploymentYAML, + } + for rel, content := range files { + full := filepath.Join(root, filepath.FromSlash(rel)) + require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o750)) + require.NoError(t, os.WriteFile(full, []byte(content), 0o600)) + } +} + +// TestFanInPrecondition_RefusesAmbiguousOverrideWriteThrough proves the L2 write-boundary +// invariant end to end: a live image bump on the diamond's shared Deployment would fall back +// to a write-through into base/deployment.yaml — a file two render paths reach with override +// entries at stake. The flush must refuse (IssueWriteFanIn) before writing, and the shared +// source file must keep its bytes, rather than the former warn-and-write-through behaviour. +func TestFanInPrecondition_RefusesAmbiguousOverrideWriteThrough(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + seedDiamond(t, root) + + w := &BranchWorker{contentWriter: writer, mapper: deploymentMapper()} + _, err := w.flushEventsToWorktree(context.Background(), worktree, "", + []Event{overridesDeploymentEvent("ghcr.io/example/podinfo:9.9.9", 3)}, nil) + assert.Contains(t, refusalIssueKinds(t, err), manifestanalyzer.IssueWriteFanIn, + "an ambiguous-override write-through must be refused, not written through") + + assertFileBytes(t, filepath.Join(root, "base", "deployment.yaml"), diamondDeploymentYAML, + "a refused fan-in write must leave the shared source file untouched") +} diff --git a/internal/manifestanalyzer/acceptance.go b/internal/manifestanalyzer/acceptance.go index 5fabfe97..3224d7ee 100644 --- a/internal/manifestanalyzer/acceptance.go +++ b/internal/manifestanalyzer/acceptance.go @@ -107,6 +107,21 @@ const ( // write-plan precondition — an ignore pattern matching a planned write/edit/delete path. // It surfaces as the GitTarget reason IgnoreShadowsManagedPath. IssueIgnoreShadowsManaged IssueKind = "ignore-shadows-managed" + // IssueWriteEscapesScope marks a planned write whose path escapes the GitTarget write + // scope (spec.path) — an absolute or ".."-escaping destination. It is the write-plan half + // of the L1 write-boundary invariant: the operator reads shared context outside the scope + // but never writes outside it. Enforced by the writer's pathScopePrecondition; today it is + // defense-in-depth (planned write paths are base-relative by construction), made explicit + // and tested per + // docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md §1. + IssueWriteEscapesScope IssueKind = "write-escapes-scope" + // IssueWriteFanIn marks a planned in-place edit of a source file that more than one + // kustomize render path reaches with override entries at stake (write-fan-in > 1). Writing + // the change through would corrupt what another render root renders, so the flush is + // refused instead of falling back to write-through. It is the L2 write-boundary invariant + // made explicit; the broader "any file shared by multiple render roots" generalization is + // F2 render-root scoping. + IssueWriteFanIn IssueKind = "write-fan-in" ) // Allowlist is the set of build-directive files that are retained on disk but never diff --git a/internal/manifestanalyzer/analyzer_test.go b/internal/manifestanalyzer/analyzer_test.go index 4bf6f377..08edde13 100644 --- a/internal/manifestanalyzer/analyzer_test.go +++ b/internal/manifestanalyzer/analyzer_test.go @@ -113,12 +113,15 @@ func TestAnalyze_Issues(t *testing.T) { IssueUnresolvedKRM: 0, IssueOutOfScope: 0, IssueUnsupportedKustomize: 0, - // Foreign-content and ignore-shadow refusals are acceptance-gate facts, not part of - // the structure-only Analyze report, so they never surface here. + // Foreign-content, ignore-shadow, and the L1/L2 write-boundary refusals are + // acceptance-gate / write-plan facts, not part of the structure-only Analyze report, + // so they never surface here. IssueForeignFile: 0, IssueForeignSymlink: 0, IssueForeignSubmodule: 0, IssueIgnoreShadowsManaged: 0, + IssueWriteEscapesScope: 0, + IssueWriteFanIn: 0, } for kind, n := range want { if got := countIssues(rep, kind); got != n { diff --git a/internal/manifestanalyzer/overrides.go b/internal/manifestanalyzer/overrides.go index f7acfd02..e0e02eaf 100644 --- a/internal/manifestanalyzer/overrides.go +++ b/internal/manifestanalyzer/overrides.go @@ -363,3 +363,22 @@ func resolveOverrides( } return a.overrides, nil } + +// OverridesAmbiguousAt reports whether the store refused to route a kustomize override chain +// for a document in the file at the given base-relative (slash) path, because more than one +// render path reaches it with override entries at stake (reasonAmbiguousOverrides). It is the +// store-side signal for the writer's write-fan-in precondition: editing such a file in place +// would write a live change through into source context shared by multiple render roots — the +// one edit the write-fan-in = 1 invariant forbids — so the flush is refused rather than +// corrupting what another root renders. Derived from the build-time diagnostics the store +// already carries, so it needs no extra per-file state. +func (s *ManifestStore) OverridesAmbiguousAt(rel string) bool { + want := filepathToSlash(rel) + for i := range s.Diagnostics { + d := s.Diagnostics[i] + if d.Reason == reasonAmbiguousOverrides && filepathToSlash(d.Path) == want { + return true + } + } + return false +} diff --git a/internal/manifestanalyzer/overrides_test.go b/internal/manifestanalyzer/overrides_test.go index e06d93be..3b57cc72 100644 --- a/internal/manifestanalyzer/overrides_test.go +++ b/internal/manifestanalyzer/overrides_test.go @@ -93,6 +93,27 @@ func TestKustomizeOverridesCorpus_AmbiguousImages(t *testing.T) { } } +// TestOverridesAmbiguousAt pins the store-side signal the writer's write-fan-in precondition +// consults: the shared document a diamond reaches two ways reports ambiguous, a build +// directive / unknown path does not, and a store with no ambiguous chain never reports it. +func TestOverridesAmbiguousAt(t *testing.T) { + diamond := corpusStore(t, "unsupported/diamond-images") + if !diamond.OverridesAmbiguousAt("base/deployment.yaml") { + t.Errorf("the diamond's shared base/deployment.yaml must report an ambiguous override chain") + } + if diamond.OverridesAmbiguousAt("base/kustomization.yaml") { + t.Errorf("a build directive is not an ambiguous managed write path") + } + if diamond.OverridesAmbiguousAt("no/such/file.yaml") { + t.Errorf("an unknown path must not report ambiguity") + } + + clean := corpusStore(t, "supported/images-overlay") + if clean.OverridesAmbiguousAt("base/deployment.yaml") { + t.Errorf("a store with no ambiguous chain must never report ambiguity") + } +} + // TestKustomizeOverridesNestedBaseIsNotARoot pins the render-root rule: a base // referenced by another kustomization is not walked as its own root, so the // nested layout yields ONE chain (base+parent composed), not two conflicting ones. From defc6e841df5a9a0f30ddd250bd7c324cf6064e0 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Thu, 9 Jul 2026 13:38:09 +0000 Subject: [PATCH 3/5] chore(deps): bump go 1.26.5 and x/crypto v0.54.0 to clear GO-2026-5932/GO-2026-5856 - golang.org/x/crypto v0.53.0 -> v0.54.0 drops the unmaintained openpgp package from the module graph, clearing GO-2026-5932. govulncheck and `go mod why` confirm our code neither imports nor calls it (go-git already uses the ProtonMail/go-crypto fork). - go 1.26.4 -> 1.26.5 (go.mod, Dockerfile, .devcontainer/Dockerfile, test/mutationlab/Dockerfile; base-image digests re-pinned to the 1.26.5 index) fixes the reachable crypto/tls ECH privacy leak GO-2026-5856. - k8s.io/utils and transitive golang.org/x/{mod,sync,sys,term,text,tools} refreshed by go mod tidy. govulncheck: 0 vulnerabilities affecting our code. lint/vet/unit tests green under go1.26.5. Co-Authored-By: Claude Opus 4.8 (1M context) --- .devcontainer/Dockerfile | 2 +- Dockerfile | 2 +- go.mod | 18 +++++++++--------- go.sum | 32 ++++++++++++++++---------------- test/mutationlab/Dockerfile | 2 +- 5 files changed, 28 insertions(+), 28 deletions(-) diff --git a/.devcontainer/Dockerfile b/.devcontainer/Dockerfile index 9e788b5c..ada23b5d 100644 --- a/.devcontainer/Dockerfile +++ b/.devcontainer/Dockerfile @@ -2,7 +2,7 @@ # Stage 1: CI base image with essential build tools # Pinned by digest (Scorecard "pinned dependencies"); Dependabot's docker # ecosystem keeps version + digest current together. -FROM golang:1.26.4-bookworm@sha256:b305420a68d0f229d91eb3b3ed9e519fcf2cf5461da4bef997bf927e8c0bfd2b AS ci +FROM golang:1.26.5-bookworm@sha256:18aedc16aa19b3fd7ded7245fc14b109e054d65d22ed53c355c899582bbb2113 AS ci # Avoid warnings by switching to noninteractive ENV DEBIAN_FRONTEND=noninteractive diff --git a/Dockerfile b/Dockerfile index 8d2489f4..de83c291 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,7 +1,7 @@ # Build the manager binary # Base images are pinned by digest (Scorecard "pinned dependencies"); # Dependabot's docker ecosystem keeps version + digest current together. -FROM golang:1.26.4@sha256:f96cc555eb8db430159a3aa6797cd5bae561945b7b0fe7d0e284c63a3b291609 AS builder +FROM golang:1.26.5@sha256:079e59808d2d252516e27e3f3a9c003740dee7f75e55aa71528766d52bcfc16a AS builder # Automatic platform arguments provided by Docker BuildKit ARG TARGETOS diff --git a/go.mod b/go.mod index 42d2555f..137fdc7c 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module github.com/ConfigButler/gitops-reverser -go 1.26.4 +go 1.26.5 require ( filippo.io/age v1.3.1 @@ -21,13 +21,13 @@ require ( go.opentelemetry.io/otel/metric v1.44.0 go.opentelemetry.io/otel/sdk/metric v1.44.0 go.uber.org/zap v1.28.0 - golang.org/x/crypto v0.53.0 + golang.org/x/crypto v0.54.0 gopkg.in/yaml.v3 v3.0.1 k8s.io/api v0.36.2 k8s.io/apimachinery v0.36.2 k8s.io/apiserver v0.36.2 k8s.io/client-go v0.36.2 - k8s.io/utils v0.0.0-20260507154919-ff6756f316d2 + k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 sigs.k8s.io/cli-utils v0.37.2 sigs.k8s.io/controller-runtime v0.24.1 sigs.k8s.io/yaml v1.6.0 @@ -111,15 +111,15 @@ require ( go.yaml.in/yaml/v2 v2.4.4 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/exp v0.0.0-20260508232706-74f9aab9d74a // indirect - golang.org/x/mod v0.36.0 // indirect + golang.org/x/mod v0.37.0 // indirect golang.org/x/net v0.56.0 // indirect golang.org/x/oauth2 v0.36.0 // indirect - golang.org/x/sync v0.21.0 // indirect - golang.org/x/sys v0.46.0 // indirect - golang.org/x/term v0.44.0 // indirect - golang.org/x/text v0.38.0 // indirect + golang.org/x/sync v0.22.0 // indirect + golang.org/x/sys v0.47.0 // indirect + golang.org/x/term v0.45.0 // indirect + golang.org/x/text v0.40.0 // indirect golang.org/x/time v0.15.0 // indirect - golang.org/x/tools v0.45.0 // indirect + golang.org/x/tools v0.47.0 // indirect gomodules.xyz/jsonpatch/v2 v2.5.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20260511170946-3700d4141b60 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20260511170946-3700d4141b60 // indirect diff --git a/go.sum b/go.sum index 8179ae80..e4430a95 100644 --- a/go.sum +++ b/go.sum @@ -278,38 +278,38 @@ go.yaml.in/yaml/v2 v2.4.4/go.mod h1:gMZqIpDtDqOfM0uNfy0SkpRhvUryYH0Z6wdMYcacYXQ= go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= -golang.org/x/crypto v0.53.0 h1:QZ4Muo8THX6CizN2vPPd5fBGHyogrdK9fG4wLPFUsto= -golang.org/x/crypto v0.53.0/go.mod h1:DNLU434OwVakk9PzuwV8w62mAJpRJL3vsgcfp4Qnsio= +golang.org/x/crypto v0.54.0 h1:YLIA59K4fiNzHzjnZt2tUJQjQtUWfWbeHBqKtk3eScw= +golang.org/x/crypto v0.54.0/go.mod h1:KWL8ny2AZdGR2cWmzeHrp2azQPGogOv+HeQaVEXC2dk= golang.org/x/exp v0.0.0-20260508232706-74f9aab9d74a h1:+3jdDGGB8NGb1Zktc737jlt3/A5f6UlwSzmvqUuufxw= golang.org/x/exp v0.0.0-20260508232706-74f9aab9d74a/go.mod h1:d2fgXJLVs4dYDHUk5lwMIfzRzSrWCfGZb0ZqeLa/Vcw= -golang.org/x/mod v0.36.0 h1:JJjpVx6myfUsUdAzZuOSTTmRE0PfZeNWzzvKrP7amb4= -golang.org/x/mod v0.36.0/go.mod h1:moc6ELqsWcOw5Ef3xVprK5ul/MvtVvkIXLziUOICjUQ= +golang.org/x/mod v0.37.0 h1:vF1DjpVEshcIqoEaauuHebaLk1O1forxjxBaVn884JQ= +golang.org/x/mod v0.37.0/go.mod h1:m8S8VeM9r4dzDwjrKO0a1sZP3YjeMamRRlD+fmR2Q/0= golang.org/x/net v0.0.0-20211112202133-69e39bad7dc2/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= golang.org/x/net v0.56.0 h1:Rw8j/hFzGvJUZwNBXnAtf5sVDVt+65SK2C7IxCxZt5o= golang.org/x/net v0.56.0/go.mod h1:D3Ku6r+V6JROoZK144D2XfMHFcMq/0zSfLelVTCFKec= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= -golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= -golang.org/x/sync v0.21.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= +golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= +golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/sys v0.0.0-20191026070338-33540a1f6037/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210124154548-22da62e12c0c/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= -golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/sys v0.47.0 h1:o7XGOvZQCADBQQ4Y7VNq2dRWQR7JmOUW8Kxx4ZsNgWs= +golang.org/x/sys v0.47.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= -golang.org/x/term v0.44.0 h1:0rLvDRCtNj0gZkyIXhCyOb2OAzEhLVqc4B+hrsBhrmc= -golang.org/x/term v0.44.0/go.mod h1:7ze4MdzUzLXpSAoFP1H0bOI9aXDqveSvatT5vKcFh2Y= +golang.org/x/term v0.45.0 h1:NwWyBmoJCbfTHpxrWoZ9C6/VxOf7ic219I8xZZFdrf0= +golang.org/x/term v0.45.0/go.mod h1:9aqxs0blBcrm/n0L9QW0aRVD+ktan8ssZromtqJC43w= golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= -golang.org/x/text v0.38.0 h1:sXmwo9DwP3OK9EZ7PqAdaooSGozfl/3a6/xJcbzPRhE= -golang.org/x/text v0.38.0/go.mod h1:YXZt3QhHUKYT53r2lLKFIVi6Ao1jdzrTR/KQ09qyxF4= +golang.org/x/text v0.40.0 h1:Ub2Z6/xjgF1WrYQz2nuITOEegKFtiIy+rieRJ5lHZKs= +golang.org/x/text v0.40.0/go.mod h1:hpnzDAfGV753zIKo+wk3u1bVKCGPbrnF7+7LBF/UHVY= golang.org/x/time v0.15.0 h1:bbrp8t3bGUeFOx08pvsMYRTCVSMk89u4tKbNOZbp88U= golang.org/x/time v0.15.0/go.mod h1:Y4YMaQmXwGQZoFaVFk4YpCt4FLQMYKZe9oeV/f4MSno= golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= -golang.org/x/tools v0.45.0 h1:18qN3FAooORvApf5XjCXgsuayZOEtXf6JK18I3+ONa8= -golang.org/x/tools v0.45.0/go.mod h1:LuUGqqaXcXMEFEruIVJVm5mgDD8vww/z/SR1gQ4uE/0= +golang.org/x/tools v0.47.0 h1:7Kn5x/d1svx/PzryTsqeoZN4TZwqeH5pGWjefhLi/1Q= +golang.org/x/tools v0.47.0/go.mod h1:dFHnyTvFWY212G+h7ZY4Vsp/K3U4/7W9TyVaAul8uCA= gomodules.xyz/jsonpatch/v2 v2.5.0 h1:JELs8RLM12qJGXU4u/TO3V25KW8GreMKl9pdkk14RM0= gomodules.xyz/jsonpatch/v2 v2.5.0/go.mod h1:AH3dM2RI6uoBZxn3LVrfvJ3E0/9dG4cSrbuBJT4moAY= gonum.org/v1/gonum v0.17.0 h1:VbpOemQlsSMrYmn7T2OUvQ4dqxQXU+ouZFQsZOx50z4= @@ -354,8 +354,8 @@ k8s.io/kube-openapi v0.0.0-20260512234627-ef417d054102 h1:xs2ux1MvyrOdfKwS3vuFWr k8s.io/kube-openapi v0.0.0-20260512234627-ef417d054102/go.mod h1:V/QaCUYDa+0QpcHhVVc5l99Uz56wEMEXBSj9oCDkNDY= k8s.io/streaming v0.36.2 h1:NSKthPPg9UFSKsRauVJUVGH2Dvn8fhKmY4qrMkw/p98= k8s.io/streaming v0.36.2/go.mod h1:z6fV3D+NVkoeqRMtWwlUZK6U17SY/LqNzOxWL6GyR/s= -k8s.io/utils v0.0.0-20260507154919-ff6756f316d2 h1:wU4tMEhLGgIbLvXQb1cfN+EcM0wf7zC6CPF+C79jroc= -k8s.io/utils v0.0.0-20260507154919-ff6756f316d2/go.mod h1:xDxuJ0whA3d0I4mf/C4ppKHxXynQ+fxnkmQH0vTHnuk= +k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3 h1:jVkFFVfXdXP74B/zbO3hM3hpSFD0xvhQ5U686DPurkE= +k8s.io/utils v0.0.0-20260707023825-cf1189d6abe3/go.mod h1:M2s5JB1lIYP3jzZdorPLHXIPJzt9vv2muW5a6L9DtNM= sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.34.0 h1:hSfpvjjTQXQY2Fol2CS0QHMNs/WI1MOSGzCm1KhM5ec= sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.34.0/go.mod h1:Ve9uj1L+deCXFrPOk1LpFXqTg7LCFzFso6PA48q/XZw= sigs.k8s.io/cli-utils v0.37.2 h1:GOfKw5RV2HDQZDJlru5KkfLO1tbxqMoyn1IYUxqBpNg= diff --git a/test/mutationlab/Dockerfile b/test/mutationlab/Dockerfile index 5301627d..55fb13d0 100644 --- a/test/mutationlab/Dockerfile +++ b/test/mutationlab/Dockerfile @@ -1,6 +1,6 @@ # Mutation-capture lab image. Built and deployed only by `task lab-e2e`; it is # never part of the product image, the Helm chart, or the default CI lane. -FROM golang:1.26.4 AS builder +FROM golang:1.26.5 AS builder ARG TARGETOS ARG TARGETARCH From 4c05fb683bcb7755fa8b2d1c12c9f0fc2819d689 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Thu, 9 Jul 2026 14:37:46 +0000 Subject: [PATCH 4/5] feat(git): surface refused live writes as GitPathAccepted=False/WriteBoundaryRefused MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A write-plan refusal on a live-event path was logged and dropped. The resync path carries its refusal back on a result channel the router inspects, but a live commit window is finalized on a timer with no channel to return anything on, so an ambiguous write was correctly prevented while the GitTarget stayed healthy — the user's apply appeared to take effect and Git never moved. Both live paths were affected: the open window and the atomic request. Give the branch worker a PathRefusalReporter hook, installed by the watch manager, and call it from both paths when errors.As recovers an AcceptanceRefusedError. Every other error keeps its existing write-fault handling. Recovery stays with the resync path: a live write that happens to avoid the offending file proves nothing about the rest of the subtree. Name the two write-boundary refusals rather than hiding them behind the umbrella UnsupportedContent: a refusal composed purely of write-escapes-scope and/or write-fan-in issues now reports WriteBoundaryRefused, which is in the controller's stalled-reason set. UnsupportedContent means the folder holds content we cannot manage; WriteBoundaryRefused means the folder is fine and the edit had nowhere safe to land. AllIssuesOfKind becomes variadic AllIssuesOfKinds to express "all issues are among this set". Tests cover the path end to end, since the gap was that nothing connected the preconditions to the user-visible surface: a real diamond repo driven through the real event loop (reporter fires, branch tip unmoved), the watch mapping to GitPathAccepted=False, and the controller mapping to Stalled=True. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/main.go | 6 + ...granularity-and-cross-environment-edits.md | 168 +++++++++++++++--- ...mize-support-boundary-and-product-model.md | 31 ++-- internal/controller/gittarget_controller.go | 8 + internal/controller/gittarget_status_test.go | 20 +++ internal/controller/stream_status.go | 1 + internal/git/branch_worker.go | 83 +++++---- internal/git/git_path_refusal.go | 46 +++++ internal/git/worker_manager.go | 15 ++ .../git/write_boundary_precondition_test.go | 137 ++++++++++++-- internal/manifestanalyzer/acceptance.go | 5 + .../manifestanalyzer/acceptance_refusal.go | 20 ++- internal/watch/event_router.go | 29 ++- internal/watch/event_router_test.go | 16 ++ internal/watch/git_path_acceptance.go | 13 ++ internal/watch/git_path_acceptance_test.go | 70 ++++++++ 16 files changed, 573 insertions(+), 95 deletions(-) create mode 100644 internal/git/git_path_refusal.go create mode 100644 internal/watch/git_path_acceptance_test.go diff --git a/cmd/main.go b/cmd/main.go index 527e5b17..1d6c01e4 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -157,6 +157,12 @@ func main() { // The registry is a stable pointer the watch manager refreshes in place. workerManager.SetMapper(watchMgr.TypeRegistry()) + // Give the workers a way to surface a refused live write plan. Live events are committed + // off a timer with no result channel, so without this a refusal (acceptance gate or a + // write-boundary precondition) would abort the commit and leave the GitTarget looking + // healthy; the resync path already reports its own refusals through the router. + workerManager.SetPathRefusalReporter(watchMgr.ReportGitPathRefusal) + // WatchRule controller (with WatchManager reference for dynamic reconciliation) fatalIfErr((&controller.WatchRuleReconciler{ Client: mgr.GetClient(), diff --git a/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md b/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md index d850b15b..681b0fd6 100644 --- a/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md +++ b/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md @@ -1,7 +1,11 @@ # GitTarget granularity, the write boundary, and cross-environment edits -> Status: direction-setting / options (no code change; opens two forks that -> feed F2 and gate the Track-1 write-boundary hardening) +> Status: direction-setting / options. The two forks are decided (§2 → **A**, +> §3 → **product promotion**), and the Track-1 write-boundary hardening they +> gate has **shipped**: L1 and L2 are enforced as write-plan preconditions in +> `internal/git/plan_flush.go` and surface as the GitTarget reason +> `WriteBoundaryRefused` (§1). What remains open is F2 render-root scoping and +> the divergence notification sketched in §6. > Captured: 2026-07-09 > Related: > [README.md](README.md), @@ -34,8 +38,8 @@ These are decided; they frame the forks but are not reopened here. - **The write boundary is two layers.** (§1 below.) **L1** — every path the operator writes is inside the target's write scope (a filesystem jail). **L2** — within the reachable graph, never write a file consumed by more than - one render root (fan-in = 1). Both are only *emergent* in today's code; making - them explicit and tested is the Track-1 prerequisite. + one render root (fan-in = 1). Both *were* only emergent; Track 1 made them + explicit write-plan preconditions, checked before any byte is written. - **The admission webhook is a fail-open accelerator, not a correctness layer.** It rejects unsavable edits at `kubectl apply` time for immediate, *atomic* feedback, but it is opt-in, intent-mode-only, and `failurePolicy: @@ -108,12 +112,48 @@ flowchart TD (write scope) and treats siblings as non-overlapping — so it must not later fold the wider read scope into ownership. -**Today (Track-1 gap):** L1 holds only because write paths are always built -relative to the write root, and L2 is not enforced at all — an ambiguous -override chain *warns and writes through*, and the only thing preventing a -shared-file clobber is a coincidental namespace-ambiguity block. Track 1 turns -both into explicit, tested preconditions (L1 alongside the existing -`.gittargetignore` shadow check; L2 as a refusal instead of a write-through). +**Before Track 1:** L1 held only because write paths happened to be built +relative to the write root, and L2 was not enforced at all — an ambiguous +override chain *warned and wrote through*, and the only thing preventing a +shared-file clobber was a coincidental namespace-ambiguity block. + +**Now (Track 1, shipped):** both are write-plan preconditions in +`writeBatch.flush`, evaluated at the one moment every planned path is known and +before any byte is touched, alongside the existing `.gittargetignore` shadow +check: + +| Layer | Precondition | Refusal issue | +|---|---|---| +| L1 | `pathScopePrecondition` — no planned write is absolute or climbs out of `spec.path` with `..` | `write-escapes-scope` | +| L2 | `fanInPrecondition` — no planned write edits a file more than one render path reaches with override entries at stake | `write-fan-in` | + +A violation aborts the whole flush, commits nothing, and fails the GitTarget +with `GitPathAccepted=False` / `Stalled=True`, reason **`WriteBoundaryRefused`** +— distinct from the umbrella `UnsupportedContent`, because the folder is not +malformed: the *edit* had nowhere safe to land. This holds on both write paths. +The resync path carries the refusal back on its result channel; the live-event +path has no result channel (a commit window is finalized on a timer), so the +branch worker reports it through a `GitPathRefusalReporter` hook the watch +manager installs. Without that hook a refused live write would be prevented +correctly but silently, which is the worse failure: the user's `kubectl apply` +appears to have taken effect and Git never moves. + +Refusal is *target-level and immediate*, not per-edit. That is the only surface +that exists today, and it is the honest one: the offending shape is a property of +the folder's render graph, so every subsequent edit that touches the shared file +will hit it too. Recovery is left to the resync path — once a human fixes the +layout, the next successful per-type resync clears `GitPathAccepted` back to +true. A live write never clears it, because a live write that happens to avoid +the offending file proves nothing about the rest of the subtree. When the Tier-2 +per-edit unreflected-set accounting +([unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md)) +exists, the *individual dropped edit* belongs there; the target-level condition +still belongs here. + +L1 stays *defense-in-depth*: planned write paths are base-relative by +construction today, so the check should never fire — but it is the invariant the +base-is-read-only guarantee rests on under granularity option **A**, so it is +asserted rather than assumed. ## 2. Fork one — GitTarget granularity @@ -183,12 +223,16 @@ flowchart TB is no "read wider than write" machinery and no `../../base` escape to follow. The overlap check stays trivially one-owner-per-app. - **What it costs:** the base now sits *inside* the write scope, so the *only* - thing keeping it read-only is **L2** — the graph fan-in rule we just confirmed - is emergent and buggy today. The weaker guarantee becomes load-bearing exactly - where the blast radius is highest ("an edit in test writes base → changes - prod"). It also dissolves the clean identity: one target spans three - namespaces, its watch scope is all of them, and a session branch can mix - environments — which muddies RBAC, promotion, and session lifecycle. + thing keeping it read-only is **L2** — the graph fan-in rule, which is now + enforced but is still the weaker guarantee: it depends on modelling the render + graph correctly, where L1 is a path check that cannot be wrong. That weaker + guarantee would become load-bearing exactly where the blast radius is highest + ("an edit in test writes base → changes prod"), and any future fan-in blind + spot (a shared file with no override entries at stake — see §5) is a + corruption rather than a refusal. It also dissolves the clean identity: one + target spans three namespaces, its watch scope is all of them, and a session + branch can mix environments — which muddies RBAC, promotion, and session + lifecycle. ### Option C — Fine + base-as-variant @@ -384,16 +428,24 @@ reverted by hydration in intent mode | Option | If the operator ever computed a write into `base/deployment.yaml` from a `podinfo-test` edit | |---|---| | **A** | impossible — `base/` is outside `overlays/test` (L1 refuses before planning) | -| **B** | possible in principle — only L2 fan-in stops it, and L2 is the emergent/buggy layer ⚠ | +| **B** | possible in principle — only L2 fan-in stops it, and L2 is the graph-modelling layer, not the path check ⚠ | | **C** | impossible from the test target (L1); the *base* target may write `base/`, but only from a `podinfo-base` edit | ## 5. Consequences for the ladder and Track 1 -- **Granularity is decided (A), which fixes the Track-1 investment.** Because A - keeps the base read-only by **L1**, Track 1 builds **L1 as an explicit - precondition** (the strong, cheap guarantee) and treats **L2 as a refusal** +- **Granularity is decided (A), which fixed the Track-1 investment.** Because A + keeps the base read-only by **L1**, Track 1 built **L1 as an explicit + precondition** (the strong, cheap guarantee) and turned **L2 into a refusal** (never write-through a multi-consumer file) — it never leans on L2 to protect - the base. + the base. Both shipped; see §1. +- **L2's remaining blind spot is F2's job.** `fanInPrecondition` fires on the + signal the store already carries: a file more than one render path reaches + *with override entries at stake*. A file shared by two render roots with no + competing `images:`/`replicas:` chain is not flagged — under layout A it is + also never dirty (a base doc reached by distinct overlays is `NamespaceNone` + and never matches a live object), so nothing is written. Generalizing the check + to "any file reachable from more than one render root" is F2 render-root + scoping, and it is what would be required before layout B could be offered. - **F2 scope gains one concrete capability under A/C:** follow `../../base` for *reading* (today dropped), while the write jail stays at `spec.path`. Much of the read-scope / render-root / out-of-subtree-base logic already exists @@ -406,7 +458,72 @@ reverted by hydration in intent mode contract and pin it with a corpus case (a `HelmRelease` with a floating range: accepted; a `kustomization.yaml` with a remote base: refused). -## 6. Decisions and remaining open items +## 6. Reflecting an edit as a *local override* — and telling the user they diverged + +The write boundary refuses an edit that would land in shared context. But most +edits that *want* to land there have a legal destination one level up: the +overlay's own `kustomization.yaml`. This is the reflection path F1 already +implements, and it is worth naming explicitly, because it is the reason the L2 +refusal is a narrow rule rather than a broad one. + +Bump `podinfo` to `9.9.9` in the `test` namespace. The container image lives in +`base/deployment.yaml`, which `prod` also renders — writing it there is exactly +the edit L2 forbids. Instead the operator edits the overlay's `images:` entry: + +```text +apps/podinfo/ +├── base/deployment.yaml # image: podinfo:6.3.0 ← untouched, prod still renders this +└── overlays/test/kustomization.yaml + images: + - name: podinfo + newTag: "9.9.9" # ← the write lands HERE; kustomize gives it precedence +``` + +The write stays inside `spec.path` (L1 holds), the shared file is never touched +(L2 holds), and the render is correct: kustomize applies `images:` *after* the +base is loaded, so the overlay entry wins. `replicas:` behaves the same way. +Field-level edits that no override entry can express (an env var, a resource +limit) have no such destination — they are the F3 patch case, and until F3 they +are refused or accounted as unreflected +([unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md)). + +**The cost: the divergence is invisible.** After that write, `test` is pinned to +`9.9.9` and no longer tracks whatever `base` says. A later bump of the base image +to `6.4.0` silently does nothing for `test` — the overlay entry shadows it. The +override *is* the intent, so this is correct behavior, not a bug. But it is a +fact the user should learn when the override is created, not months later when +they wonder why a base bump did not reach an environment. + +Two shapes are worth distinguishing: + +| Shape | What happened | Notable? | +|---|---|---| +| **Override updated** | an `images:` entry already existed for this image; the operator changed `newTag` | no — the overlay already diverged; the user is editing their own pin | +| **Override created** | no entry existed; the overlay rendered the base value, and the operator has now pinned it | **yes** — this is the moment the environment stops tracking base | + +**Proposal (not built; own F-item).** On the *create* transition, surface the +divergence at the point it becomes true. Three candidate surfaces, cheapest +first, and they are not exclusive: + +1. **Commit-message trailer** on the commit that adds the entry — e.g. + `Overlay-Diverged: images/podinfo base=6.3.0 overlay=9.9.9`. Free, durable, + reviewable, lands in the product PR where a human is already looking. This is + the one to build first. +2. **Kubernetes Event** on the GitTarget (`reason: OverlayDiverged`), so the + divergence is visible to `kubectl describe` and to anything watching events. +3. **Status** — rejected as the primary surface. A `GitTarget` condition is + target-scoped and level-triggered; divergence is per-resource and per-edit, so + it would either flap or accumulate unbounded. The **unreflected-set + accounting** in + [unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md) + is the right home for anything per-edit and durable, once it exists. + +Note the deliberate asymmetry with §1: a *refused* write is an error the user +must fix, so it fails the GitTarget. A *divergent* write is a correct write whose +consequence the user should know about, so it is a notification. Conflating the +two would make the common, healthy overlay edit look like a failure. + +## 7. Decisions and remaining open items **Decided (2026-07-09) — the user's call:** @@ -418,10 +535,13 @@ reverted by hydration in intent mode **Still open:** -3. **Write-up placement** — fold the §1 L1/L2 model back into +3. **Divergence notification (§6)** — build the commit-message trailer on the + "override created" transition? Own F-item; nothing depends on it shipping with + the write boundary. +4. **Write-up placement** — fold the §1 L1/L2 model back into [kustomize-support-boundary-and-product-model.md §4](kustomize-support-boundary-and-product-model.md) (one canonical invariant statement), or keep §4 as the short invariant and let this doc own the two-layer detail? -4. **Option C sub-questions (deferred with C):** the synthetic base namespace's +5. **Option C sub-questions (deferred with C):** the synthetic base namespace's handling of multi-namespace bases and cluster-scoped resources, and the separate "global/defaults editor" RBAC role (§3b). diff --git a/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md b/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md index 72ec84b8..cb32928e 100644 --- a/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md +++ b/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md @@ -1,9 +1,12 @@ # Kustomize support boundary and product model -> Status: direction-setting (no code change; feeds F2/F3 design) +> Status: direction-setting; feeds F2/F3 design. The §4 fan-in invariant is no +> longer emergent — it ships as a write-plan refusal (see §1 and +> [gittarget-granularity-and-cross-environment-edits.md §1](gittarget-granularity-and-cross-environment-edits.md)). > Captured: 2026-07-06 > Related: > [README.md](README.md), +> [gittarget-granularity-and-cross-environment-edits.md](gittarget-granularity-and-cross-environment-edits.md), > [finished/f1-images-replicas-edit-through.md](finished/f1-images-replicas-edit-through.md), > [../manifest/contextual-namespace-and-kustomize-folder-editing.md](../manifest/contextual-namespace-and-kustomize-folder-editing.md), > [../unsupported-folder-refusal-plan.md](../unsupported-folder-refusal-plan.md), @@ -61,13 +64,15 @@ Two nuances found while auditing the current gate same "dead text shadowed by a transformer" pathology F1 fixed for `images:`. A future projection-side subtraction is the F1-style fix; until then the support statement should name the limitation. -- **The fan-out fallback is safe only emergently.** Ambiguous override chains - (`ambiguous-images`, `diamond-images` corpora) emit a *warning* and fall - back to plain write-through. Write-through into a file consumed by two - render roots is the one edit that must never happen. Today the parallel - namespace ambiguity (`NamespaceNone`) prevents the live-object match in - practice, so no write occurs — but that safety is a side effect, not a - stated rule. See [§4](#4-the-invariant). +- **The fan-out fallback is now an explicit refusal.** Ambiguous override chains + (`ambiguous-images`, `diamond-images` corpora) still emit a warning at store + build time, but a *planned write* into such a file is refused before any byte + is written: write-through into a file consumed by two render roots is the one + edit that must never happen, and it no longer depends on the coincidental + namespace ambiguity (`NamespaceNone`) that used to block the live-object match. + The refusal fails the GitTarget with reason `WriteBoundaryRefused`. See + [§4](#4-the-invariant) and + [gittarget-granularity-and-cross-environment-edits.md §1](gittarget-granularity-and-cross-environment-edits.md). ## 2. Supported layouts: an allowlist, not field caveats @@ -152,9 +157,13 @@ This single sentence: - explains the `diamond-images` refusal (two paths from one root); - decides the multi-environment product questions in §9 (the operator cannot "add to all environments" *by design*); -- must be promoted from today's emergent behavior to an explicit, tested rule - **before** the F2/F4 launch unit ships (see the fan-out fallback nuance in - §1). +- **has been promoted from emergent behavior to an explicit, tested rule** — a + write-plan precondition that refuses the flush (`WriteBoundaryRefused`) rather + than writing through. It is paired with the filesystem jail (writes never leave + `spec.path`); the two layers are specified in + [gittarget-granularity-and-cross-environment-edits.md §1](gittarget-granularity-and-cross-environment-edits.md). + Generalizing it from "a file two override chains reach" to "any file two render + roots reach" is F2 render-root scoping. ## 5. The overlay model (F2+F4 at launch, F3 completes it) diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index 50b5a531..96a757fd 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -73,6 +73,14 @@ const ( // written and the GitTarget is failed with this reason. The string must stay in sync with // the watch package's gitPathRefusalReason. GitTargetReasonIgnoreShadowsManagedPath = "IgnoreShadowsManagedPath" + // GitTargetReasonWriteBoundaryRefused is the reason for a write the operator refused + // because it had nowhere safe to land, rather than because the folder holds content the + // operator cannot manage + // (docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md §1): a + // 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" GitTargetReadyReasonValidationFailed = "ValidationFailed" GitTargetReadyReasonEncryptionNotConfigured = "EncryptionNotConfigured" diff --git a/internal/controller/gittarget_status_test.go b/internal/controller/gittarget_status_test.go index 458058de..63c10afb 100644 --- a/internal/controller/gittarget_status_test.go +++ b/internal/controller/gittarget_status_test.go @@ -81,6 +81,26 @@ func TestDeriveGitTargetDataPlaneStatus(t *testing.T) { wantStreams: metav1.ConditionTrue, wantStalledReason: GitTargetReasonUnsupportedContent, }, + { + // A write-boundary refusal is not "this folder holds content we cannot manage"; + // it is "this edit had nowhere safe to land". It must carry its own reason all + // the way through to Stalled, so an operator can tell the two apart. + name: "write boundary refused", + streams: watch.StreamSummary{ + Total: 1, Ready: 1, Reason: watch.StreamReasonAllStreamsReady, Message: "1/1 streams running", + }, + gitPath: watch.GitPathAcceptanceStatus{ + Accepted: false, + Reason: GitTargetReasonWriteBoundaryRefused, + Message: "Git path refused at base/deployment.yaml: write-fan-in must be 1", + }, + wantReady: metav1.ConditionFalse, + wantReconciling: metav1.ConditionFalse, + wantStalled: metav1.ConditionTrue, + wantGitPath: metav1.ConditionFalse, + wantStreams: metav1.ConditionTrue, + wantStalledReason: GitTargetReasonWriteBoundaryRefused, + }, } for _, tt := range tests { diff --git a/internal/controller/stream_status.go b/internal/controller/stream_status.go index 115c1799..4ada154d 100644 --- a/internal/controller/stream_status.go +++ b/internal/controller/stream_status.go @@ -125,6 +125,7 @@ func gitTargetReadyReasonIsStalled(reason string) bool { GitTargetReasonTargetConflict, GitTargetReasonUnsupportedContent, GitTargetReasonIgnoreShadowsManagedPath, + GitTargetReasonWriteBoundaryRefused, GitTargetReadyReasonValidationFailed, GitTargetReadyReasonEncryptionNotConfigured, GitTargetReadyReasonWorkerUnavailable, diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index eec060d8..826ee1c7 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -84,6 +84,12 @@ type BranchWorker struct { // before Start, on the same goroutine the event loop reads it from. sshHostKeys SSHHostKeyConfig + // pathRefusal surfaces a refused write plan as GitTarget GitPathAccepted=False. The + // live-event paths have no result channel to carry the refusal back, so without it a + // refused live write would abort the commit and leave the GitTarget looking healthy. Set + // by the WorkerManager before Start; a nil reporter only drops the status transition. + pathRefusal PathRefusalReporter + // Event processing eventQueue chan WorkItem ctx context.Context @@ -686,30 +692,7 @@ func (l *branchWorkerEventLoop) handleQueueItem(item WorkItem) { } if item.Request.CommitMode == CommitModeAtomic { - // Atomic batches bypass the commit window, but not the retained push - // lifecycle. Finalize any open live work first so arrival order is - // preserved, then append the atomic write to pendingWrites and let the - // normal cooldown-driven push path decide when to publish. - l.finalizeOpenWindowWithReason(windowFinalizeReasonAtomicBeforeApply) - // Finalizing the window opened an idle boundary: a heal parked behind that window - // arrived BEFORE this atomic, so drain it here to keep arrival order (window, then heal, - // then atomic) rather than letting the atomic overtake it. - l.applyDeferredHeals() - - pendingWrite, err := l.w.buildAtomicPendingWrite(l.w.ctx, item.Request) - if err != nil { - l.w.Log.Error(err, "Failed to build atomic pending write", "events", len(item.Request.Events)) - return - } - - if err := l.w.commitPendingWrites([]PendingWrite{*pendingWrite}, len(l.pendingWrites) > 0); err != nil { - l.w.Log.Error(err, "Atomic commit failed; dropping request", "events", len(item.Request.Events)) - return - } - - l.pendingWrites = append(l.pendingWrites, *pendingWrite) - l.pendingWritesBytes += pendingWrite.ByteSize - l.maybeSchedulePush() + l.handleAtomicRequest(item.Request) return } @@ -768,6 +751,38 @@ func (l *branchWorkerEventLoop) handleQueueItem(item WorkItem) { } } +// handleAtomicRequest applies one atomic write request. Atomic batches bypass the commit +// window, but not the retained push lifecycle: any open live work is finalized first so +// arrival order is preserved, then the atomic write joins pendingWrites and the normal +// cooldown-driven push path decides when to publish. +func (l *branchWorkerEventLoop) handleAtomicRequest(request *WriteRequest) { + l.finalizeOpenWindowWithReason(windowFinalizeReasonAtomicBeforeApply) + // Finalizing the window opened an idle boundary: a heal parked behind that window + // arrived BEFORE this atomic, so drain it here to keep arrival order (window, then heal, + // then atomic) rather than letting the atomic overtake it. + l.applyDeferredHeals() + + pendingWrite, err := l.w.buildAtomicPendingWrite(l.w.ctx, request) + if err != nil { + l.w.Log.Error(err, "Failed to build atomic pending write", "events", len(request.Events)) + return + } + + if err := l.w.commitPendingWrites([]PendingWrite{*pendingWrite}, len(l.pendingWrites) > 0); err != nil { + // A refused write plan is surfaced as a GitTarget status transition rather than + // logged as a write fault; nothing was committed either way, so the request is + // dropped in both cases. + if !l.w.reportPathRefusal(err, request.GitTargetName, request.GitTargetNamespace) { + l.w.Log.Error(err, "Atomic commit failed; dropping request", "events", len(request.Events)) + } + return + } + + l.pendingWrites = append(l.pendingWrites, *pendingWrite) + l.pendingWritesBytes += pendingWrite.ByteSize + l.maybeSchedulePush() +} + func (l *branchWorkerEventLoop) handleShutdown() { l.w.Log.Info("Handling shutdown, finalizing open window and pushing pending commits") l.finalizeOpenWindowWithReason(windowFinalizeReasonShutdown) @@ -845,7 +860,8 @@ func (l *branchWorkerEventLoop) finalizeOpenWindowWithMessage(reason windowFinal l.stopCommitTimer() events := l.openWindow.orderedEvents() windowAuthor := l.openWindow.Author - windowTarget := l.openWindow.GitTargetNamespace + "/" + l.openWindow.GitTarget + targetName, targetNamespace := l.openWindow.GitTarget, l.openWindow.GitTargetNamespace + windowTarget := targetNamespace + "/" + targetName pendingCR := l.openWindow.pendingCR // Message precedence (§6.4.2): explicit override, else the attached // CommitRequest message, else the generated grouped-commit message (empty). @@ -885,11 +901,18 @@ func (l *branchWorkerEventLoop) finalizeOpenWindowWithMessage(reason windowFinal batch := []PendingWrite{*pendingWrite} hasPendingCommits := len(l.pendingWrites) > 0 if err := l.w.commitPendingWrites(batch, hasPendingCommits); err != nil { - l.w.Log.Error(err, "Commit failed; dropping open window", - "reason", string(reason), - "windowAuthor", windowAuthor, - "windowTarget", windowTarget, - "events", len(events)) + // A refused write plan (acceptance gate or write-boundary precondition) committed + // nothing and needs a human to fix the Git path, so it is surfaced as + // GitPathAccepted=False instead of being logged as a transient write fault. The + // window is dropped either way — the events are already lost to the failed flush, + // and the next resync re-derives them. + if !l.w.reportPathRefusal(err, targetName, targetNamespace) { + l.w.Log.Error(err, "Commit failed; dropping open window", + "reason", string(reason), + "windowAuthor", windowAuthor, + "windowTarget", windowTarget, + "events", len(events)) + } l.dropOpenWindow(pendingCR, fmt.Errorf("commit failed: %w", err)) return false } diff --git a/internal/git/git_path_refusal.go b/internal/git/git_path_refusal.go new file mode 100644 index 00000000..d346f3cc --- /dev/null +++ b/internal/git/git_path_refusal.go @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "errors" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + itypes "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// 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 +// a human editing the Git path can clear it — so it must reach the user as +// GitPathAccepted=False / Stalled=True rather than being logged and dropped. +// +// The resync path already carries its refusal back on ResyncResult.Err, where the watch layer +// classifies it. The live-event paths have no result channel — a window is finalized on a +// timer, and its failure used to be logged and dropped — so they report through this hook +// instead. The watch Manager supplies it (WorkerManager.SetPathRefusalReporter), which is +// why the reason mapping lives there and not here. +type PathRefusalReporter func(target itypes.ResourceReference, refused *manifestanalyzer.AcceptanceRefusedError) + +// reportPathRefusal classifies a failed live commit. When the error is (or wraps) an +// AcceptanceRefusedError it hands the refusal to the configured reporter and returns true, so +// the caller can log it as a refusal rather than an unexpected write fault. Every other error +// returns false and keeps its existing handling. +// +// Recovery is the resync path's job: once the human fixes the Git path, the next successful +// per-type resync calls MarkTargetGitPathAccepted and clears the condition. A live write never +// clears it, because a live write that happens to avoid the offending file proves nothing about +// the rest of the subtree. +func (w *BranchWorker) reportPathRefusal(err error, targetName, targetNamespace string) bool { + var refused *manifestanalyzer.AcceptanceRefusedError + if !errors.As(err, &refused) { + return false + } + target := itypes.NewResourceReference(targetName, targetNamespace) + w.Log.Info("Live write refused: unsupported GitTarget path content", + "gitTarget", target.String(), "detail", refused.Error()) + if w.pathRefusal != nil { + w.pathRefusal(target, refused) + } + return true +} diff --git a/internal/git/worker_manager.go b/internal/git/worker_manager.go index 389a36af..21889cfc 100644 --- a/internal/git/worker_manager.go +++ b/internal/git/worker_manager.go @@ -41,6 +41,11 @@ type WorkerManager struct { // sshHostKeys configures SSH host-key resolution for every worker's credential reads. Set // once at startup (SetSSHHostKeyConfig) before any worker is created. sshHostKeys SSHHostKeyConfig + + // pathRefusal reports a refused live write plan to the GitTarget status surface. Set + // 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 } // NewWorkerManager creates a new worker manager. @@ -81,6 +86,15 @@ func (m *WorkerManager) SetSSHHostKeyConfig(cfg SSHHostKeyConfig) { m.sshHostKeys = cfg } +// SetPathRefusalReporter injects the hook every worker calls when a live write plan is +// refused, so the refusal reaches GitTarget status instead of being logged and dropped. Like +// SetMapper, it is called once at startup before any worker is created. +func (m *WorkerManager) SetPathRefusalReporter(reporter PathRefusalReporter) { + m.mu.Lock() + defer m.mu.Unlock() + m.pathRefusal = reporter +} + // RegisterTarget ensures a worker exists for the target's (provider, branch) // and registers the target with that worker. // This is called by GitTarget controller when a target becomes Ready. @@ -138,6 +152,7 @@ func (m *WorkerManager) EnsureWorker( // exists) is race-free. worker.mapper = m.mapper worker.sshHostKeys = m.sshHostKeys + worker.pathRefusal = m.pathRefusal 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/git/write_boundary_precondition_test.go b/internal/git/write_boundary_precondition_test.go index 35e04c99..5c082262 100644 --- a/internal/git/write_boundary_precondition_test.go +++ b/internal/git/write_boundary_precondition_test.go @@ -7,10 +7,17 @@ import ( "os" "path/filepath" "testing" + "time" + gogit "github.com/go-git/go-git/v5" + "github.com/go-git/go-git/v5/config" + "github.com/go-git/go-git/v5/plumbing" + "github.com/go-git/go-git/v5/plumbing/object" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/types" ) @@ -89,22 +96,26 @@ func diamondOverlayKust(newTag string) string { return "resources:\n - ../base\nimages:\n - name: ghcr.io/example/podinfo\n newTag: \"" + newTag + "\"\n" } -// seedDiamond writes a minimal single-root diamond: root → a → base and root → b → base, -// where a and b carry differing images entries so base/deployment.yaml is reached by two -// distinct override chains (write-fan-in > 1). +// diamondFiles is a minimal single-root diamond: root → a → base and root → b → base, where a +// and b carry differing images entries so base/deployment.yaml is reached by two distinct +// override chains (write-fan-in > 1). Ordered so seeding it into Git produces stable commits. +func diamondFiles() []struct{ rel, content string } { + return []struct{ rel, content string }{ + {"kustomization.yaml", "resources:\n - a\n - b\n"}, + {"a/kustomization.yaml", diamondOverlayKust("1.0.0")}, + {"b/kustomization.yaml", diamondOverlayKust("2.0.0")}, + {"base/kustomization.yaml", "resources:\n - deployment.yaml\n"}, + {"base/deployment.yaml", diamondDeploymentYAML}, + } +} + +// seedDiamond writes the diamond into a worktree on disk. func seedDiamond(t *testing.T, root string) { t.Helper() - files := map[string]string{ - "kustomization.yaml": "resources:\n - a\n - b\n", - "a/kustomization.yaml": diamondOverlayKust("1.0.0"), - "b/kustomization.yaml": diamondOverlayKust("2.0.0"), - "base/kustomization.yaml": "resources:\n - deployment.yaml\n", - "base/deployment.yaml": diamondDeploymentYAML, - } - for rel, content := range files { - full := filepath.Join(root, filepath.FromSlash(rel)) + for _, f := range diamondFiles() { + full := filepath.Join(root, filepath.FromSlash(f.rel)) require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o750)) - require.NoError(t, os.WriteFile(full, []byte(content), 0o600)) + require.NoError(t, os.WriteFile(full, []byte(f.content), 0o600)) } } @@ -128,3 +139,103 @@ func TestFanInPrecondition_RefusesAmbiguousOverrideWriteThrough(t *testing.T) { assertFileBytes(t, filepath.Join(root, "base", "deployment.yaml"), diamondDeploymentYAML, "a refused fan-in write must leave the shared source file untouched") } + +// seedDiamondInRemote pushes the diamond to a remote branch as one commit. It clones once +// rather than reusing simulateClientCommitOnDisk per file, because the diamond spans +// subdirectories and that helper does not create parent directories. +func seedDiamondInRemote(t *testing.T, remoteURL, branch string) { + t.Helper() + clientPath := filepath.Join(t.TempDir(), "seed") + repo, worktree := initLocalRepo(t, clientPath, remoteURL, branch) + for _, f := range diamondFiles() { + full := filepath.Join(clientPath, filepath.FromSlash(f.rel)) + require.NoError(t, os.MkdirAll(filepath.Dir(full), 0o750)) + require.NoError(t, os.WriteFile(full, []byte(f.content), 0o600)) + _, err := worktree.Add(f.rel) + require.NoError(t, err) + } + _, err := worktree.Commit("seed diamond", &gogit.CommitOptions{ + Author: &object.Signature{Name: "Client", Email: "client@example.com", When: time.Now()}, + }) + require.NoError(t, err) + require.NoError(t, repo.Push(&gogit.PushOptions{ + RefSpecs: []config.RefSpec{config.RefSpec("refs/heads/" + branch + ":refs/heads/" + branch)}, + })) +} + +// diamondGitTarget is the GitTarget the live-path test's events name, rooted at the repo root +// so the whole diamond is inside its write scope. +func diamondGitTarget(providerName, branch string) *configv1alpha3.GitTarget { + return &configv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "podinfo-test", Namespace: "default"}, + Spec: configv1alpha3.GitTargetSpec{ + ProviderRef: configv1alpha3.GitProviderReference{Name: providerName}, + Branch: branch, + Path: "", + }, + } +} + +// TestEventLoop_LiveFanInRefusal_FailsGitTargetAndCommitsNothing closes the live-path gap: a +// live event window whose flush trips a write-boundary precondition is finalized off a timer, +// with no result channel to carry the refusal back to the router. It must still reach the user +// as a GitTarget refusal — reported through the worker's PathRefusalReporter, which the +// watch Manager maps to GitPathAccepted=False / Stalled=True — and it must leave the branch +// exactly where it was: an ambiguous write is prevented, never half-applied. +func TestEventLoop_LiveFanInRefusal_FailsGitTargetAndCommitsNothing(t *testing.T) { + tempDir := t.TempDir() + remotePath := filepath.Join(tempDir, "remote.git") + createBareRepo(t, remotePath) + remoteURL := "file://" + remotePath + seedDiamondInRemote(t, remoteURL, "main") + seededHash := branchHash(t, remotePath, "main") + + worker, err := newTestBranchWorker(remoteURL, "test-repo", "main", diamondGitTarget("test-repo", "main")) + require.NoError(t, err) + worker.mapper = deploymentMapper() + + var refusals []*manifestanalyzer.AcceptanceRefusedError + var refusedTargets []types.ResourceReference + worker.pathRefusal = func(target types.ResourceReference, refused *manifestanalyzer.AcceptanceRefusedError) { + refusedTargets = append(refusedTargets, target) + refusals = append(refusals, refused) + } + + loop := newBranchWorkerEventLoop(worker, time.Hour) + loop.lastPushAt = time.Now() // keep the (irrelevant) push out of this test + defer loop.stopTimers() + + event := overridesDeploymentEvent("ghcr.io/example/podinfo:9.9.9", 3) + event.UserInfo = UserInfo{Username: "alice"} + event.GitTargetName = "podinfo-test" + event.GitTargetNamespace = "default" + loop.handleQueueItem(WorkItem{Request: &WriteRequest{Events: []Event{event}, CommitMode: CommitModePerEvent}}) + require.NotNil(t, loop.openWindow, "the live event must open a commit window") + + assert.False(t, loop.finalizeOpenWindow(), "a refused write plan must not produce a pending write") + assert.Empty(t, loop.pendingWrites, "a refused flush must retain nothing to push") + assert.Nil(t, loop.openWindow, "the refused window must be dropped, not retried forever") + + require.Len(t, refusals, 1, "a live write-boundary refusal must be reported exactly once") + assert.Equal(t, types.NewResourceReference("podinfo-test", "default"), refusedTargets[0], + "the refusal must name the GitTarget whose window was refused") + assert.Contains(t, refusalIssueKinds(t, refusals[0]), manifestanalyzer.IssueWriteFanIn) + + // The GitTarget fails, and Git is untouched: no commit was created on the worker's local + // branch, so nothing could ever be pushed. + localHead, err := gogit.PlainOpen(worker.repoPathForRemote(remoteURL)) + require.NoError(t, err) + head, err := localHead.Head() + require.NoError(t, err) + assert.Equal(t, seededHash, head.Hash(), "a refused live write must create no commit") +} + +// branchHash reads a branch tip straight out of a repository on disk. +func branchHash(t *testing.T, repoPath, branch string) plumbing.Hash { + t.Helper() + repo, err := gogit.PlainOpen(repoPath) + require.NoError(t, err) + ref, err := repo.Reference(plumbing.NewBranchReferenceName(branch), true) + require.NoError(t, err) + return ref.Hash() +} diff --git a/internal/manifestanalyzer/acceptance.go b/internal/manifestanalyzer/acceptance.go index 3224d7ee..25af58a6 100644 --- a/internal/manifestanalyzer/acceptance.go +++ b/internal/manifestanalyzer/acceptance.go @@ -122,6 +122,11 @@ const ( // made explicit; the broader "any file shared by multiple render roots" generalization is // F2 render-root scoping. IssueWriteFanIn IssueKind = "write-fan-in" + + // A refusal made up purely of the two write-boundary kinds above surfaces as the GitTarget + // reason WriteBoundaryRefused rather than the umbrella UnsupportedContent: the folder holds + // nothing the operator cannot manage, the edit simply had nowhere safe to land. See the + // watch package's gitPathRefusalReason. ) // Allowlist is the set of build-directive files that are retained on disk but never diff --git a/internal/manifestanalyzer/acceptance_refusal.go b/internal/manifestanalyzer/acceptance_refusal.go index c576169e..f7125596 100644 --- a/internal/manifestanalyzer/acceptance_refusal.go +++ b/internal/manifestanalyzer/acceptance_refusal.go @@ -2,7 +2,10 @@ package manifestanalyzer -import "fmt" +import ( + "fmt" + "slices" +) // AcceptanceRefusedError is the writer-facing error for a GitTarget folder the acceptance // gate refused. It carries every issue so the surface (GitTarget status / a blocked stream) @@ -33,17 +36,18 @@ func (e *AcceptanceRefusedError) Error() string { // the surface intent is explicit at the call site. func (e *AcceptanceRefusedError) BlockMessage() string { return e.Error() } -// AllIssuesOfKind reports whether the refusal is composed entirely of one issue kind. The -// surface uses it to pick a precise status reason: a refusal that is purely -// IssueIgnoreShadowsManaged is the unrecoverable .gittargetignore-shadows-a-write case -// (§4.3) and deserves its own reason, whereas any mix falls back to the umbrella -// UnsupportedContent. An empty issue set returns false. -func (e *AcceptanceRefusedError) AllIssuesOfKind(kind IssueKind) bool { +// AllIssuesOfKinds reports whether every issue in the refusal is one of the given kinds. The +// surface uses it to pick a precise status reason: a refusal made up purely of +// IssueIgnoreShadowsManaged is the unrecoverable .gittargetignore-shadows-a-write case (§4.3), +// and one made up purely of the write-boundary kinds (IssueWriteEscapesScope, IssueWriteFanIn) +// is a refused write-boundary violation — each deserves its own reason, whereas any mix falls +// back to the umbrella UnsupportedContent. An empty issue set returns false. +func (e *AcceptanceRefusedError) AllIssuesOfKinds(kinds ...IssueKind) bool { if len(e.Issues) == 0 { return false } for _, iss := range e.Issues { - if iss.Kind != kind { + if !slices.Contains(kinds, iss.Kind) { return false } } diff --git a/internal/watch/event_router.go b/internal/watch/event_router.go index 09dfb648..93ffa311 100644 --- a/internal/watch/event_router.go +++ b/internal/watch/event_router.go @@ -263,18 +263,29 @@ func (r *EventRouter) handleScopedResyncError( r.recordBackgroundResyncFailure(gitDest) } -// gitPathRefusalReason picks the GitTarget status reason for a refused path. A refusal made -// up purely of the .gittargetignore-shadows-a-write case (§4.3) gets its own terminal reason -// IgnoreShadowsManagedPath so an operator can tell the unrecoverable footgun apart from any -// other unsupported content; every other refusal keeps the umbrella UnsupportedContent. The -// strings mirror the controller's GitTargetReason* constants (the watch package cannot import -// controller without a cycle), and BOTH are members of the controller's stalled-reason set, -// so either way the GitTarget is surfaced as Stalled=True / kstatus Failed. +// 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: +// +// - purely the .gittargetignore-shadows-a-write case (§4.3) — the unrecoverable footgun — +// gets IgnoreShadowsManagedPath; +// - purely write-boundary violations (a planned write escaping spec.path, or an in-place +// edit of a file more than one render root reaches) gets WriteBoundaryRefused: the folder +// 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 +// controller without a cycle), and all three are members of the controller's stalled-reason +// set, so every refusal surfaces as Stalled=True / kstatus Failed. func gitPathRefusalReason(refused *manifestanalyzer.AcceptanceRefusedError) string { - if refused.AllIssuesOfKind(manifestanalyzer.IssueIgnoreShadowsManaged) { + switch { + case refused.AllIssuesOfKinds(manifestanalyzer.IssueIgnoreShadowsManaged): return "IgnoreShadowsManagedPath" + case refused.AllIssuesOfKinds(manifestanalyzer.IssueWriteEscapesScope, manifestanalyzer.IssueWriteFanIn): + return "WriteBoundaryRefused" + default: + return "UnsupportedContent" } - return "UnsupportedContent" } // RegisterGitTargetEventStream registers a GitTargetEventStream with the router. diff --git a/internal/watch/event_router_test.go b/internal/watch/event_router_test.go index 98372694..4e4f76fb 100644 --- a/internal/watch/event_router_test.go +++ b/internal/watch/event_router_test.go @@ -172,6 +172,14 @@ func TestGitPathRefusalReason(t *testing.T) { Path: ".gittargetignore", } foreign := manifestanalyzer.AcceptanceIssue{Kind: manifestanalyzer.IssueForeignFile, Path: "notes.txt"} + fanIn := manifestanalyzer.AcceptanceIssue{ + Kind: manifestanalyzer.IssueWriteFanIn, + Path: "base/deployment.yaml", + } + escape := manifestanalyzer.AcceptanceIssue{ + Kind: manifestanalyzer.IssueWriteEscapesScope, + Path: "../escape.yaml", + } cases := []struct { name string @@ -181,6 +189,14 @@ func TestGitPathRefusalReason(t *testing.T) { {"pure shadow refusal", []manifestanalyzer.AcceptanceIssue{shadow}, "IgnoreShadowsManagedPath"}, {"foreign content refusal", []manifestanalyzer.AcceptanceIssue{foreign}, "UnsupportedContent"}, {"mixed refusal falls back", []manifestanalyzer.AcceptanceIssue{shadow, foreign}, "UnsupportedContent"}, + {"write fan-in refusal", []manifestanalyzer.AcceptanceIssue{fanIn}, "WriteBoundaryRefused"}, + {"write scope-escape refusal", []manifestanalyzer.AcceptanceIssue{escape}, "WriteBoundaryRefused"}, + {"both write-boundary kinds", []manifestanalyzer.AcceptanceIssue{fanIn, escape}, "WriteBoundaryRefused"}, + { + "write boundary mixed with content falls back", + []manifestanalyzer.AcceptanceIssue{fanIn, foreign}, + "UnsupportedContent", + }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { diff --git a/internal/watch/git_path_acceptance.go b/internal/watch/git_path_acceptance.go index 7d87a69c..896efbf6 100644 --- a/internal/watch/git_path_acceptance.go +++ b/internal/watch/git_path_acceptance.go @@ -5,9 +5,22 @@ package watch import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/types" ) +// ReportGitPathRefusal records a write plan the branch worker refused on a live-event path, +// where no result channel carries the error back to the router. It is installed on the +// WorkerManager (git.GitPathRefusalReporter) at startup, and applies the same reason mapping +// the resync path uses, so a refusal reaches the user as GitPathAccepted=False / Stalled=True +// whether it was a live write or a background resync that hit it. +func (m *Manager) ReportGitPathRefusal( + gitDest types.ResourceReference, + refused *manifestanalyzer.AcceptanceRefusedError, +) { + m.MarkTargetGitPathRefused(gitDest, gitPathRefusalReason(refused), refused.BlockMessage()) +} + // MarkTargetGitPathRefused records that the GitTarget path failed the structure-only // acceptance gate. The refusal is target-wide, not stream-specific. func (m *Manager) MarkTargetGitPathRefused(gitDest types.ResourceReference, reason, message string) { diff --git a/internal/watch/git_path_acceptance_test.go b/internal/watch/git_path_acceptance_test.go new file mode 100644 index 00000000..72b16573 --- /dev/null +++ b/internal/watch/git_path_acceptance_test.go @@ -0,0 +1,70 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + + "github.com/ConfigButler/gitops-reverser/internal/git" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// The live-event write path has no result channel: a commit window is finalized on a timer, so +// a refused write plan used to be logged and dropped, leaving the GitTarget looking healthy +// while its edit was silently prevented. ReportGitPathRefusal is the hook the branch worker +// calls instead, and it must produce the same GitPathAccepted=False transition the resync path +// produces (TestDrainScopedResync_RefusalMarksGitPathRefused pins that side). + +// TestReportGitPathRefusal_SurfacesWriteBoundaryRefusal proves a live write-boundary refusal +// reaches the GitTarget status surface with the specific WriteBoundaryRefused reason, naming +// the file the operator refused to write through. +func TestReportGitPathRefusal_SurfacesWriteBoundaryRefusal(t *testing.T) { + mgr := &Manager{Log: logr.Discard()} + gitDest := types.NewResourceReference("podinfo-test", "team-a") + + mgr.ReportGitPathRefusal(gitDest, &manifestanalyzer.AcceptanceRefusedError{ + Issues: []manifestanalyzer.AcceptanceIssue{{ + Kind: manifestanalyzer.IssueWriteFanIn, + Path: "base/deployment.yaml", + Message: "more than one kustomize render path reaches it", + }}, + }) + + gitPath := mgr.GitPathAcceptanceForGitTarget(gitDest) + assert.False(t, gitPath.Accepted, "a refused live write must mark the target Git path unaccepted") + assert.Equal(t, "WriteBoundaryRefused", gitPath.Reason, + "a pure write-boundary refusal must not hide behind the umbrella UnsupportedContent reason") + assert.Contains(t, gitPath.Message, "base/deployment.yaml", "the refusal must name the offending file") + assert.Empty(t, mgr.targetStreamStates, "a Git path refusal must not mutate stream readiness") +} + +// TestReportGitPathRefusal_ContentRefusalKeepsUmbrellaReason pins the fallback: a live refusal +// that is not purely a write-boundary violation still surfaces, under UnsupportedContent. +func TestReportGitPathRefusal_ContentRefusalKeepsUmbrellaReason(t *testing.T) { + mgr := &Manager{Log: logr.Discard()} + gitDest := types.NewResourceReference("podinfo-test", "team-a") + + mgr.ReportGitPathRefusal(gitDest, &manifestanalyzer.AcceptanceRefusedError{ + Issues: []manifestanalyzer.AcceptanceIssue{{ + Kind: manifestanalyzer.IssueForeignFile, + Path: "notes.txt", + Message: "foreign file", + }}, + }) + + gitPath := mgr.GitPathAcceptanceForGitTarget(gitDest) + assert.False(t, gitPath.Accepted) + assert.Equal(t, "UnsupportedContent", gitPath.Reason) +} + +// TestReportGitPathRefusal_SatisfiesWorkerManagerReporter is a compile-time proof that the +// Manager method can be installed as the branch workers' refusal hook, so the live-path wiring +// in cmd/main.go cannot drift out of shape unnoticed. +func TestReportGitPathRefusal_SatisfiesWorkerManagerReporter(t *testing.T) { + var reporter git.PathRefusalReporter = (&Manager{Log: logr.Discard()}).ReportGitPathRefusal + assert.NotNil(t, reporter) +} From 8e4afd62aa28eb864da3611fee3681887524123a Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Thu, 9 Jul 2026 15:01:43 +0000 Subject: [PATCH 5/5] docs: last updates --- docs/design/gitops-api/README.md | 19 ++- .../f1-images-replicas-edit-through.md | 7 + ...granularity-and-cross-environment-edits.md | 53 ++++--- ...mize-support-boundary-and-product-model.md | 48 ++++--- internal/git/branch_worker.go | 3 +- internal/git/git_path_refusal.go | 31 +++++ internal/git/git_path_refusal_test.go | 130 ++++++++++++++++++ 7 files changed, 254 insertions(+), 37 deletions(-) create mode 100644 internal/git/git_path_refusal_test.go diff --git a/docs/design/gitops-api/README.md b/docs/design/gitops-api/README.md index fa22d702..77557d22 100644 --- a/docs/design/gitops-api/README.md +++ b/docs/design/gitops-api/README.md @@ -110,10 +110,13 @@ The scoping move that keeps this launchable: **F2 + F4 are day-one Kustomize support; F3 is the deferred hard part.** Adding overlay-local KRM and bumping governed versions do not need patch authoring. A per-environment edit of a base-owned *field* (the `kubectl set env` case) has no destination -until F3 — at launch it is honestly reported as unreflected and reverted by -hydration +until F3. Today such an edit is *prevented* — the write-boundary preconditions +refuse it and fail the GitTarget (`WriteBoundaryRefused`), so it is never written +into the base. Turning that target-level refusal into a per-edit report, +reverted by hydration and never silently lost, is the **designed but unbuilt** +unreflected-set accounting ([unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md)), -never silently lost. Tier-2 metrics on how often users hit that wall are +a launch prerequisite. Tier-2 metrics on how often users hit that wall are exactly what prices F3. ## Feature ladder @@ -150,6 +153,16 @@ base"), and the mirror-mode vs. intent-cluster topology — live in surface with terminal `Pushed=True` + SHA. - Refusals: unsupported kustomize features, duplicate identities, impure or foreign content — refuse-first, never mis-edit. +- The two-layer **write boundary**, enforced as write-plan preconditions before + any byte is written: **L1** — no write leaves `spec.path` (reads may, writes + never); **L2** — no in-place edit of a source file that more than one kustomize + render path reaches with override entries at stake (write-fan-in = 1). A + violation aborts the whole flush, commits nothing, and fails the GitTarget with + `GitPathAccepted=False` / reason `WriteBoundaryRefused` — on the live-event path + as well as on resync. Specified in + [gittarget-granularity-and-cross-environment-edits.md §1](gittarget-granularity-and-cross-environment-edits.md). + The refusal is target-level; the per-edit `FullyReflected` accounting that would + name each dropped edit is designed and unbuilt. - Higher-level KRM documents (Flux `HelmRelease`, Argo CD `Application`, KRO resources) mirror and edit exactly like core resources — the pipeline is kind-agnostic, now pinned by F7's corpus + HelmRelease e2e diff --git a/docs/design/gitops-api/finished/f1-images-replicas-edit-through.md b/docs/design/gitops-api/finished/f1-images-replicas-edit-through.md index 9d754564..2a20f116 100644 --- a/docs/design/gitops-api/finished/f1-images-replicas-edit-through.md +++ b/docs/design/gitops-api/finished/f1-images-replicas-edit-through.md @@ -66,6 +66,13 @@ component is applied to its supplier: Distinct chains from multiple roots emit an `ambiguous-kustomize-overrides` diagnostic and fall back to today's write-through (no behavior regression; F2's render-root scoping resolves this case properly). + + > **Superseded (Track-1 write boundary).** The write-through fallback is gone. + > A planned write into a file flagged `ambiguous-kustomize-overrides` now + > refuses the flush (`write-fan-in`) and fails the GitTarget with + > `WriteBoundaryRefused`; nothing is committed. See + > [../gittarget-granularity-and-cross-environment-edits.md §1](../gittarget-granularity-and-cross-environment-edits.md). + > F2 render-root scoping still generalizes the check. 3. **Acceptance tightens only for garbage.** A kustomization whose `images:` or `replicas:` value is present but not structurally parseable (not a list of maps, missing `name`, non-string image fields, non-integer count) is diff --git a/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md b/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md index 681b0fd6..84e6e8b2 100644 --- a/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md +++ b/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md @@ -43,9 +43,12 @@ These are decided; they frame the forks but are not reopened here. - **The admission webhook is a fail-open accelerator, not a correctness layer.** It rejects unsavable edits at `kubectl apply` time for immediate, *atomic* feedback, but it is opt-in, intent-mode-only, and `failurePolicy: - Ignore`. Correctness rests on L1 (base is never written even if the webhook is - off) plus the Tier-2 unreflected-set accounting - ([unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md)). + Ignore`. Correctness rests entirely on the L1/L2 write-plan preconditions + below: with the webhook off, a write that would leave the jail or touch shared + context is still refused before any byte is written. The Tier-2 unreflected-set + accounting ([unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md)) + is **designed but unbuilt**; it adds per-edit *reporting* on top, and is not + what keeps the base safe. - **Floating / external sources split by *who renders*.** - *An external control plane renders it* — Flux `Kustomization`, Argo CD `Application`, Flux `HelmRelease`, KRO. These are opaque **intent** KRM to @@ -88,14 +91,23 @@ in §2 be a real choice rather than a muddle. ```mermaid flowchart TD W[Planned write to path P for object O] --> L1{L1: is P inside
this target's write scope?} - L1 -->|no| RJ[Refuse the flush — never write outside the jail] + L1 -->|no| RJ[Refuse — never write outside the jail] L1 -->|yes| L2{L2: is P's file consumed
by more than one render root?} - L2 -->|yes| RF[Refuse this edit — record in the unreflected set] + L2 -->|yes| RF[Refuse — never write through shared context] L2 -->|no| OK[Write P] - RJ --> REP[Report: GitPathAccepted=False / FullyReflected=False] + RJ --> REP["Abort the whole flush, commit nothing
GitPathAccepted=False, Stalled=True
reason: WriteBoundaryRefused"] RF --> REP ``` +Note the granularity of that refusal, because it is the thing most easily +misread. A violation aborts the **entire flush**, not just the offending edit: +nothing is committed, and the failure is recorded **once, on the GitTarget**, not +per-edit. There is no per-edit record of the dropped change today — the +`FullyReflected` condition and the unreflected set are designed +([unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md)) +and **not built**. Wherever this doc says an edit is "unreflected," read it as +*the designed future behavior*; what ships today is the hard refusal above. + - **L1 is a filesystem fact.** Cheap (`filepath.Rel` prefix test), robust, and independent of how well we model the render graph. It is what makes "the base is read-only" true *by construction* when the base sits outside the write @@ -285,14 +297,16 @@ flowchart TB additive later step (shared-defaults editing, §3b), and **B rejected as the operator write model**. -The decisive safety point stands: B makes the *weaker*, currently broken -guarantee (L2) the only thing between "edit test" and "change prod", where A and -C keep the base read-only by L1 — the filesystem guarantee Track 1 is already -hardening. But the case against B **does not even need L2**: a `GitTarget` should +The decisive safety point stands: B makes the *weaker* guarantee (L2 — enforced +now, but only as good as our model of the render graph) the only thing between +"edit test" and "change prod", where A and C keep the base read-only by L1, the +filesystem guarantee that cannot be wrong. But the case against B **does not even +need L2**: a `GitTarget` should be a **write partition**. Even with a perfect L2, one target spanning test/acceptance/production muddies four things a per-overlay target keeps clean — authorization (RBAC per namespace), audit (who changed which environment), status -(per-environment `Ready`/`FullyReflected`), and session lifecycle (a session +(per-environment `Ready`, and the planned per-edit `FullyReflected`), and session +lifecycle (a session branch is one environment). "Manage the app as one thing" is a *product grouping* concern — an aggregate/app concept the product layer can add over N targets later — not a reason to make the operator's write unit span environments. @@ -399,10 +413,16 @@ Keep **two distinct verbs** and never conflate them: ## 4. Worked examples: action → expected output -Fixed action set, acting in `podinfo-test` unless noted, launch scope (F2+F4, -no F3). "Unreflected" = recorded in the unreflected set, `FullyReflected=False`, -reverted by hydration in intent mode +Fixed action set, acting in `podinfo-test` unless noted, at the **future** launch +scope (F2+F4, no F3) — this table describes where the model is headed, not what +today's binary does. + +"Unreflected" here means the *designed* Tier-2 outcome: recorded in the +unreflected set, `FullyReflected=False`, reverted by hydration in intent mode ([unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md)). +None of that is built. Until it is, an edit with no legal destination either +never matches a source document (nothing happens) or trips a write-boundary +precondition and is refused outright (§1). ### Common to all options @@ -483,8 +503,9 @@ The write stays inside `spec.path` (L1 holds), the shared file is never touched (L2 holds), and the render is correct: kustomize applies `images:` *after* the base is loaded, so the overlay entry wins. `replicas:` behaves the same way. Field-level edits that no override entry can express (an env var, a resource -limit) have no such destination — they are the F3 patch case, and until F3 they -are refused or accounted as unreflected +limit) have no such destination — they are the F3 patch case. Today they are +simply refused when they reach a write-boundary precondition; the honest per-edit +report of *what was dropped* is the unbuilt Tier-2 accounting ([unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md)). **The cost: the divergence is invisible.** After that write, `test` is pinned to diff --git a/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md b/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md index cb32928e..4233a9c0 100644 --- a/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md +++ b/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md @@ -96,8 +96,9 @@ layout 3 launches in its **F2+F4 scope** — per-overlay `namespace` + new overlay-local KRM added to that overlay's `resources:`. The day-to-day use cases (add something to test, bump a version, edit an overlay-local object) need exactly that slice. Per-environment edits of base-owned *fields* -are the deferred hard part (F3) and are honestly reported as unreflected until -then. See the launch path in [README.md](README.md). +are the deferred hard part (F3); today they are refused rather than written into +the base, and reporting each one as unreflected is the unbuilt Tier-2 accounting +(§4). See the launch path in [README.md](README.md). Explicitly **out of scope**, and worth saying in user docs: @@ -181,12 +182,23 @@ The third row is the honesty condition on shipping F2+F4 first: an arbitrary field edit on a base-owned object has no legal destination without patch authoring (in-place would be the base). Day-one Kustomize support therefore covers the common slice — overlay entries, overlay-local documents, and adding -overlay-local KRM to `resources:` — **and must ship with the per-edit -`FullyReflected` accounting**, so an -out-of-scope edit is reported and reverted, never silently lost. That is a -scoped promise, not a gap: the launch use cases (add to test, bump a -version) never hit the third row, and tier-2 metrics on how often real -users *do* hit it are what price F3. +overlay-local KRM to `resources:`. + +Two mechanisms cover what falls outside that slice, and they must not be +conflated: + +| | Status | Granularity | Surface | +|---|---|---|---| +| **Write-boundary refusal** (L1/L2) | **shipped** | aborts the whole flush; commits nothing | `GitPathAccepted=False`, `Stalled=True`, reason `WriteBoundaryRefused` | +| **Per-edit `FullyReflected` accounting** | **designed, unbuilt** | records the individual dropped edit; reverted by hydration | planned `FullyReflected` condition + unreflected set | + +Today an out-of-scope edit is *prevented* — refused before any byte is written — +but the operator is told at the target level, not per edit. Making the third row +"reported and reverted, never silently lost" is the unbuilt accounting, and it is +a **prerequisite for the F2/F4 launch unit**, not something this branch delivers. +That remains a scoped promise rather than a gap: the launch use cases (add to +test, bump a version) never hit the third row, and tier-2 metrics on how often +real users *do* hit it are what price F3. F3's scope stays narrow and safe: the operator only creates/updates patches **it authored** (scalar fields, one patch file per object per overlay); @@ -431,15 +443,17 @@ diff instead of an archaeology exercise. selection, remote branch cleanup, a quiescence condition) — whose priority intent mode raises from "ergonomics" to "product prerequisite." - **F2+F4 ship at launch; F3 completes them.** Render-root scoping, overlay - new-file placement, `resources:` entry creation, and the unreflected-set - accounting are an honest launch unit scoped to adds, bumps, and - overlay-local edits; the first `kubectl set env` against a base-owned - object in test is *reported and reverted*, not silently lost. F3 turns - that report into a reflected edit, priced by how often the report fires. -- **Prerequisite hardening (pre-F2/F4):** make the write-fan-in-=-1 - invariant (§4) explicit and tested — the ambiguity fallback must provably - never write-through into a multi-consumer file, rather than being blocked as - a side effect of namespace ambiguity. + new-file placement, `resources:` entry creation, and the (still unbuilt) + unreflected-set accounting are an honest launch unit scoped to adds, bumps, and + overlay-local edits; once the accounting exists, the first `kubectl set env` + against a base-owned object in test is *reported and reverted* rather than + merely refused, and never silently lost. F3 turns that report into a reflected + edit, priced by how often the report fires. +- **Prerequisite hardening (pre-F2/F4): done.** The write-fan-in-=-1 invariant + (§4) is explicit and tested — the ambiguity fallback provably never writes + through into a multi-consumer file, instead of being blocked as a side effect + of namespace ambiguity. It refuses the flush and fails the GitTarget + (`WriteBoundaryRefused`), paired with the L1 filesystem jail. - **F4 moves a boundary:** overlay new-object placement needs `resources:` entry *creation*, which F1 explicitly excluded. The exclusion was per-F1 policy, not architecture; F4's design should own it. diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index 826ee1c7..7604b7a2 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -772,7 +772,8 @@ func (l *branchWorkerEventLoop) handleAtomicRequest(request *WriteRequest) { // A refused write plan is surfaced as a GitTarget status transition rather than // logged as a write fault; nothing was committed either way, so the request is // dropped in both cases. - if !l.w.reportPathRefusal(err, request.GitTargetName, request.GitTargetNamespace) { + name, namespace := atomicRefusalTarget(request) + if !l.w.reportPathRefusal(err, name, namespace) { l.w.Log.Error(err, "Atomic commit failed; dropping request", "events", len(request.Events)) } return diff --git a/internal/git/git_path_refusal.go b/internal/git/git_path_refusal.go index d346f3cc..8ad81af1 100644 --- a/internal/git/git_path_refusal.go +++ b/internal/git/git_path_refusal.go @@ -27,6 +27,12 @@ type PathRefusalReporter func(target itypes.ResourceReference, refused *manifest // the caller can log it as a refusal rather than an unexpected write fault. Every other error // returns false and keeps its existing handling. // +// An unattributable refusal (either half of the target reference empty) is logged loudly and +// NOT reported: the acceptance map is keyed by "namespace/name", so an empty half would file +// the refusal under a key no GitTarget ever reads, and every unattributable refusal would +// collide on that one key. Refusing to guess keeps a silent mis-attribution from looking like +// a healthy target elsewhere. +// // Recovery is the resync path's job: once the human fixes the Git path, the next successful // per-type resync calls MarkTargetGitPathAccepted and clears the condition. A live write never // clears it, because a live write that happens to avoid the offending file proves nothing about @@ -36,6 +42,13 @@ func (w *BranchWorker) reportPathRefusal(err error, targetName, targetNamespace if !errors.As(err, &refused) { return false } + if targetName == "" || targetNamespace == "" { + w.Log.Error(err, "Live write refused but no GitTarget could be attributed; "+ + "the refusal is NOT surfaced in status", + "gitTargetName", targetName, "gitTargetNamespace", targetNamespace, + "detail", refused.Error()) + return true + } target := itypes.NewResourceReference(targetName, targetNamespace) w.Log.Info("Live write refused: unsupported GitTarget path content", "gitTarget", target.String(), "detail", refused.Error()) @@ -44,3 +57,21 @@ func (w *BranchWorker) reportPathRefusal(err error, targetName, targetNamespace } return true } + +// atomicRefusalTarget names the GitTarget an atomic request writes for. Request-level target +// metadata is authoritative when set — buildAtomicPendingWrite resolves it and stamps it onto +// every event — but it fills that metadata only when the request carries it, leaving requests +// whose events name their own target. So fall back to the events rather than reporting a +// refusal against an empty reference. Returns the target's (name, namespace), both empty when +// nothing names a target — which reportPathRefusal treats as unattributable. +func atomicRefusalTarget(request *WriteRequest) (string, string) { + if request.GitTargetName != "" && request.GitTargetNamespace != "" { + return request.GitTargetName, request.GitTargetNamespace + } + for _, event := range request.Events { + if event.GitTargetName != "" && event.GitTargetNamespace != "" { + return event.GitTargetName, event.GitTargetNamespace + } + } + return "", "" +} diff --git a/internal/git/git_path_refusal_test.go b/internal/git/git_path_refusal_test.go new file mode 100644 index 00000000..289adcb0 --- /dev/null +++ b/internal/git/git_path_refusal_test.go @@ -0,0 +1,130 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "errors" + "fmt" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// refusedError is a write-plan refusal wrapped exactly as commitPendingWrites wraps it, so +// these tests also pin errors.As recovery through the wrap. +func refusedError() error { + return fmt.Errorf("execute pending writes: %w", &manifestanalyzer.AcceptanceRefusedError{ + Issues: []manifestanalyzer.AcceptanceIssue{{ + Kind: manifestanalyzer.IssueWriteFanIn, + Path: "base/deployment.yaml", + Message: "write-fan-in must be 1", + }}, + }) +} + +// captureRefusals installs a recording reporter on a bare worker. +func captureRefusals(w *BranchWorker) *[]types.ResourceReference { + seen := &[]types.ResourceReference{} + w.pathRefusal = func(target types.ResourceReference, _ *manifestanalyzer.AcceptanceRefusedError) { + *seen = append(*seen, target) + } + return seen +} + +// TestReportPathRefusal_ReportsAttributedRefusal is the happy path: a wrapped refusal with a +// fully named target reaches the reporter and tells the caller not to log a write fault. +func TestReportPathRefusal_ReportsAttributedRefusal(t *testing.T) { + w := &BranchWorker{Log: logr.Discard()} + seen := captureRefusals(w) + + assert.True(t, w.reportPathRefusal(refusedError(), "podinfo-test", "team-a")) + require.Len(t, *seen, 1) + assert.Equal(t, types.NewResourceReference("podinfo-test", "team-a"), (*seen)[0]) +} + +// TestReportPathRefusal_PassesThroughNonRefusal proves a plain write fault is left to its +// existing error handling rather than being swallowed as a refusal. +func TestReportPathRefusal_PassesThroughNonRefusal(t *testing.T) { + w := &BranchWorker{Log: logr.Discard()} + seen := captureRefusals(w) + + assert.False(t, w.reportPathRefusal(errors.New("remote hung up"), "podinfo-test", "team-a")) + assert.Empty(t, *seen, "a transient write fault must not be reported as a Git path refusal") +} + +// TestReportPathRefusal_UnattributableRefusalIsNotRecorded pins the guard: the acceptance map +// is keyed by "namespace/name", so a half-empty reference would file the refusal under a key no +// GitTarget reads, and every unattributable refusal would collide there. Refusing to guess is +// safer than mis-attributing — the caller is still told it was a refusal, not a write fault. +func TestReportPathRefusal_UnattributableRefusalIsNotRecorded(t *testing.T) { + for _, c := range []struct{ name, ns string }{ + {"", ""}, + {"podinfo-test", ""}, + {"", "team-a"}, + } { + w := &BranchWorker{Log: logr.Discard()} + seen := captureRefusals(w) + + assert.True(t, w.reportPathRefusal(refusedError(), c.name, c.ns), + "an unattributable refusal is still a refusal, not a write fault") + assert.Empty(t, *seen, + "a refusal with an incomplete target reference must never be recorded (%q/%q)", c.ns, c.name) + } +} + +// TestAtomicRefusalTarget_PrefersRequestThenEvents proves the atomic path names the right +// GitTarget. buildAtomicPendingWrite stamps request-level metadata onto every event, but only +// when the request carries it — a request whose events name their own target would otherwise +// have its refusal reported against an empty reference. +func TestAtomicRefusalTarget_PrefersRequestThenEvents(t *testing.T) { + eventFor := func(name, namespace string) Event { + return Event{GitTargetName: name, GitTargetNamespace: namespace} + } + + cases := []struct { + name string + request *WriteRequest + wantName string + wantNamespace string + }{ + { + name: "request-level metadata wins", + request: &WriteRequest{ + GitTargetName: "from-request", GitTargetNamespace: "team-a", + Events: []Event{eventFor("from-event", "team-b")}, + }, + wantName: "from-request", wantNamespace: "team-a", + }, + { + name: "falls back to the first event that names a target", + request: &WriteRequest{ + Events: []Event{eventFor("", ""), eventFor("from-event", "team-b")}, + }, + wantName: "from-event", wantNamespace: "team-b", + }, + { + name: "a half-set request falls back rather than reporting a partial reference", + request: &WriteRequest{ + GitTargetName: "from-request", + Events: []Event{eventFor("from-event", "team-b")}, + }, + wantName: "from-event", wantNamespace: "team-b", + }, + { + name: "nothing names a target", + request: &WriteRequest{Events: []Event{eventFor("", "")}}, + }, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + name, namespace := atomicRefusalTarget(c.request) + assert.Equal(t, c.wantName, name) + assert.Equal(t, c.wantNamespace, namespace) + }) + } +}