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/.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/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/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 new file mode 100644 index 00000000..84e6e8b2 --- /dev/null +++ b/docs/design/gitops-api/gittarget-granularity-and-cross-environment-edits.md @@ -0,0 +1,568 @@ +# GitTarget granularity, the write boundary, and cross-environment edits + +> 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), +> [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 *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: + 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 + 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 — never write outside the jail] + L1 -->|yes| L2{L2: is P's file consumed
by more than one render root?} + L2 -->|yes| RF[Refuse — never write through shared context] + L2 -->|no| OK[Write P] + 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 + 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. + +**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 + +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, 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 + +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* 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`, 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. + +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, 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 + +| 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 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 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. 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 + 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. 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. 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 +`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:** + +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. **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? +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..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 @@ -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 @@ -91,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: @@ -152,9 +158,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) @@ -172,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); @@ -422,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/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/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..7604b7a2 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,39 @@ 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. + 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 + } + + 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 +861,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 +902,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..8ad81af1 --- /dev/null +++ b/internal/git/git_path_refusal.go @@ -0,0 +1,77 @@ +// 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. +// +// 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 +// 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 + } + 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()) + if w.pathRefusal != nil { + w.pathRefusal(target, refused) + } + 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) + }) + } +} 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/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 new file mode 100644 index 00000000..5c082262 --- /dev/null +++ b/internal/git/write_boundary_precondition_test.go @@ -0,0 +1,241 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "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" +) + +// 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" +} + +// 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() + 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(f.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") +} + +// 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 5fabfe97..25af58a6 100644 --- a/internal/manifestanalyzer/acceptance.go +++ b/internal/manifestanalyzer/acceptance.go @@ -107,6 +107,26 @@ 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" + + // 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/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. 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) +} 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