From b0e97e4d572a6db15c6c6f5992c0e403f3500709 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Thu, 10 Sep 2026 14:33:02 +0000 Subject: [PATCH 1/4] docs(preflight): make the write preflight an advisory gate, not a guarantee The proposal wanted two things that do not compose: no Git round-trip in the admission path, and failurePolicy: Fail with deny-on-cannot-assess. A snapshot you refuse to refresh is stale by construction, so a hard gate built on it promises a guarantee its own inputs cannot support. Pick the coherent half. Preflight answers from the working copy the branch worker already has, fast, and folds "cannot assess" toward allow: - The promise shrinks to "no involved target is KNOWN to be unable to publish this change". A denial is a finding; an allow is only the absence of one. - allowed=false carries a refusal; allowed=true plus AdmissionResponse.warnings carries "could not verify". There is no third verdict in the API, and warnings are the honest place to put the third state. - failurePolicy: Ignore with timeoutSeconds: 1, matching what /validate-all already ships. Fail was never available anyway: the handler is served by the operator's own pod, so Fail on core resources deadlocks the cluster at bootstrap. - A new no-Git-operations rule: no fetch, pull, push, commit, checkout or index write on the preflight path, enforced by handing it a read-only filesystem rather than by intending it. - A missing local copy is "cannot assess", allowed by default, with an install-level setting for operators who would rather fence the resource. Two tables now state what preflight can and cannot answer. The split is not arbitrary: repository shape does not change between one kubectl apply and the next, so shape-derived refusals do not decay, while everything lost to staleness was never knowable at admission time. Also records what the code already provides. writeBatch.flush runs its four preconditions on in-memory buffers before the byte-writing loop, so the read-only extraction is a split of one function, not a re-architecture; the earlier caution about calling the planner from admission was over-cautious. HA is dropped from scope, since the chart refuses replicaCount > 1. Co-Authored-By: Claude Opus 5 --- docs/design/git-write-preflight.md | 337 +++++++++++++++++++++++++---- 1 file changed, 289 insertions(+), 48 deletions(-) diff --git a/docs/design/git-write-preflight.md b/docs/design/git-write-preflight.md index b9252de8..6c5b7c52 100644 --- a/docs/design/git-write-preflight.md +++ b/docs/design/git-write-preflight.md @@ -1,14 +1,19 @@ -# Git write preflight: reject unpublishable edits at admission +# Git write preflight: warn or reject unpublishable edits at admission > Status: proposal, not implemented. This document recommends the next feature for the editing -> surface workflow. The two-refusal model and all-target acceptance rule are decided direction. -> API names and diagnostic shapes remain illustrative. +> surface workflow. The two-refusal model, the any-target refusal rule, and the advisory +> (fail-open) posture are decided direction. API names and diagnostic shapes remain illustrative. ## Recommendation Build a shared, read-only Git write preflight and expose it through an opt-in validating admission webhook. When the writer can establish that a proposed object change has no supported destination, -reject the request before persistence with a reason the author can act on. +reject the request before persistence with a reason the author can act on. When it cannot establish +that within its budget, allow the request and say so in a warning. + +**This is an advisory gate, not a guarantee.** It catches what is knowable from state the operator +already holds, quickly and without touching the network. It is deliberately not a proof that a +change will reach Git. See [What this gate is and is not](#what-this-gate-is-and-is-not). Prioritize this over broader Helm support or additional source transformations for the editing surface use case. It makes the existing supported surface easier to trust: an author learns that @@ -62,7 +67,7 @@ The supported model has two user-facing refusal surfaces: | Surface | Question | Failure behavior | |---|---|---| | Onboarding and renewed target validation | Can this target manage its repository scope? | Refuse the target and report its error. | -| Admission preflight | Can every involved target publish this complete object change? | Reject the API request before persistence, naming the blockers. | +| Admission preflight | Is any involved target known to be unable to publish this object change? | Reject the API request before persistence, naming the blockers; warn when the question could not be answered. | Write-time checks remain authoritative underneath both surfaces. They defend against stale admission decisions, direct Git changes, configuration changes, and operation paths outside @@ -84,16 +89,78 @@ Carry forward these principles: An admission rejection alone does not make a healthy target erroneous. Nothing changed in the cluster. A target error means its current repository or accepted state cannot be processed safely. -### Scope and promise +### What this gate is and is not + +The promise for a covered request is deliberately narrow: + +> No involved target is **known**, from state the operator already holds, to be unable to publish +> this change. + +That is not the same as "every target will publish it". Three properties of the deployment make the +stronger promise unbuyable at any acceptable price: + +- **Speed.** An admission handler sits in the synchronous path of every covered write. A gate that + fetches before answering makes every `kubectl apply` in the cluster as slow as a Git round-trip. +- **Availability.** A gate that must reach the forge to answer hands the forge a veto over cluster + edits. Hosted Git is not reliable enough to be placed in that position, and an operator whose + writes stop because GitHub is degraded is worse than one whose mirror is briefly behind. +- **Time of check to time of use.** Even a fresh answer is stale by the time the object persists. + Git can move underneath it, and a direct commit never traverses Kubernetes admission at all. + +So preflight answers from a local snapshot, without a network call, and accepts that it will be +wrong in one direction only: it may allow something that later fails to publish. It must never +reject something the writer would in fact have published. + +This shapes the whole design. **A denial is a finding; an allow is only the absence of one.** +Say so in user-facing documentation and keep the asymmetry visible in the messages themselves. -The promise for a covered request is: +What the gate still catches is the majority of real cases, because the expensive-to-know failures +are not the common ones. A base-owned field edit with no overlay patch authoring +([shape 8][shape8]) is computable from the local snapshot with no freshness requirement at all: +it depends on the repository's *shape*, which does not change between one `kubectl apply` and the +next. The cases lost to staleness are races, which no admission gate wins. -> Every involved target passed preflight for the complete proposed object change against the -> repository and configuration snapshots identified by the evaluation. +Acceptance therefore guarantees no future commit and no atomic publication across targets. +Onboarding acceptance is likewise revision-dependent: it is not a permanent certificate that every +future commit on the branch is supported. -Acceptance cannot guarantee a future commit or atomic publication across targets. Git can change -later and a push can fail. Onboarding acceptance is also revision-dependent: it is not a permanent -certificate that every future commit on the branch is supported. +### What preflight can and cannot answer + +State this plainly in user-facing documentation, because the value of an advisory gate depends +entirely on people knowing which half they are in. The dividing line is simple: preflight answers +questions about the **shape of the repository and the shape of the change**, both of which it can +read from the working copy it already has. It cannot answer questions about **the future or the +network**. + +Answerable from the offline copy, and therefore reliable: + +| Question | Raised by | +|---|---| +| Is this field owned by a base outside the target's write scope? | render attribution, [shape 8][shape8] | +| Would the planned write escape `spec.path`? | `pathScopePrecondition` | +| Would it write into context shared by more than one render root? | `fanInPrecondition` | +| Would it land on a path `.gittargetignore` shadows? | `ignoreShadowPrecondition` | +| Would the resulting render move an object this change did not intend? | `renderPrecondition` | +| Is this field supplied by a build transformer or owned by a tolerated patch? | source-form and patch checks | +| Is the folder structurally unsupported (undecodable build file, remote base)? | acceptance gate | +| Does the object resolve to no publishing destination at all? | target resolution | +| Would the delete patch collide with an existing file? | placement, once [step 2](#difficulty) lands | + +Not answerable, and never claimed: + +| Question | Why not | +|---|---| +| Will the push succeed? | Credentials, protected branches, and forge availability are all outside the snapshot, and reaching for them is what this design refuses to do. | +| Has Git moved since the worker last fetched? | The snapshot is exactly as fresh as the last fetch. A direct commit never passes through admission at all. | +| Will a concurrent batch change the answer? | The sibling-agreement case is a property of a whole flush; admission sees one request. | +| Is this target publishable right now? | A target with no local copy, or an unreported render epoch, is `Cannot assess` by construction. | +| Does a `/scale` or other subresource edit publish? | Not until the parent-object adapter exists. Until then it is excluded from coverage and said so. | +| Is encrypted or reduced-render content publishable? | Those checks are intentionally narrower; a verdict there states what was checked, not that everything was. | + +The useful property of this split is that the first table does not decay. Repository shape changes +when someone commits, not between one `kubectl apply` and the next, so a shape-derived refusal is +as true a second later as it was when computed. The answers lost to staleness live entirely in the +second table, where the honest response was never anything but "cannot know". Enable this feature explicitly for editing surfaces whose authored changes are intended to reach Git. Keep it off by default for live-cluster mirroring, audit capture, and discovery. With the gate @@ -111,12 +178,13 @@ Otherwise admission and publication will disagree as support evolves. ```mermaid flowchart TD - R[Admission request] --> S[Resolve all involved targets and immutable snapshots] - S --> P[Shared write planner and render verification] - P --> A[Every target publishable against its snapshot] + R[Admission request] --> S[Resolve involved targets against the local working copy] + S --> P[Shared write planner and render verification, in memory] + P --> A[Every target publishable] P --> D[Any refusal: deny with target reasons] - P --> U[Any indeterminate result: deny with context] + P --> U[Any indeterminate result: allow with a warning] A --> K[API may persist the object] + U --> K K --> W[Watch observes persisted state] W --> B[Writer plans the complete batch against current inputs] B --> V[Write boundaries and render verification] @@ -124,8 +192,10 @@ flowchart TD V -->|Refusal| X[Abort affected flush and report GitTarget error] ``` -The admission path must not commit, push, enqueue a publication, or advance watch state. A request -may still fail after this webhook approves it. Watch remains the source of persisted object state. +The admission path must not commit, push, fetch, enqueue a publication, or advance watch state. It +reads the working copy and plans in memory; see [The no-Git-operations +rule](#the-no-git-operations-rule). A request may still fail after this webhook approves it, and +watch remains the source of persisted object state. Candidate implementation seams: @@ -147,9 +217,45 @@ return is insufficient if an edit was skipped. Require evidence that the complet is represented, or is already present, under the applicable checks. Silent skips must become explicit refusals for this contract. -Evaluate against an immutable local snapshot. No Git fetch or push belongs in the admission request -path. Include enough snapshot identity to reproduce a decision: a Git revision alone may not -identify configuration changes or pending local state. +Include enough snapshot identity to reproduce a decision: a Git revision alone may not identify +configuration changes or pending local state. + +#### The no-Git-operations rule + +Preflight reads the **already checked-out worktree on disk** and nothing else. This is a hard +invariant, not a performance goal: + +> The preflight path performs no Git operation. No fetch, no pull, no push, no commit, no +> checkout, no index write. It reads files from the working copy the branch worker already has, +> and plans in memory. + +The current writer makes this cheap to honour, because everything before the byte-writing loop is +already of that shape. `scanRenderScope` walks the checked-out directory with ordinary filesystem +reads; `writeBatch` holds the resulting bytes in `contentByPath` and hydrates a `fileBuffer` per +touched path; every precondition then runs over those in-memory buffers. The one go-git call on the +path is a local `HEAD` read for the revision stamp, and it belongs to the layout report, which does +not run under preflight anyway. + +Enforce it rather than intending it. The preflight entry point should take the worktree as a +read-only filesystem, so a Git operation is not merely discouraged but unavailable to it, and a +test should assert that a preflight evaluation leaves the working copy and the index byte-identical. + +The consequence is deliberate: preflight's view of Git is exactly as fresh as the branch worker's +last fetch, and no fresher. It answers from what the operator already knows. + +#### When there is no local copy + +A target whose worktree has not been cloned yet, or whose clone is being replaced, has nothing to +plan against. That is `Cannot assess`, and by default the request is allowed with a warning naming +the target. + +Make the behavior configurable at install level (illustratively +`--preflight-on-unavailable=allow|deny`, defaulting to `allow`) for operators who would rather +fence the resource than admit an unverifiable edit. Document `deny` honestly: it converts every +cold start, every re-clone, and every target reconfiguration into a refusal window for covered +resources, and it is only safe alongside a `namespaceSelector` that keeps the operator's own +namespace and `kube-system` outside coverage. The default stays `allow` because an operator that +cannot see is not evidence that the author is wrong. ### Evaluation output @@ -159,7 +265,44 @@ Keep three distinct outcomes: |---|---| | Publishable | A supported candidate passes the applicable checks against this snapshot. | | Refused | The evaluation establishes a concrete publishing constraint violation. | -| Cannot assess | Required context is unavailable, incomplete, unsupported by preflight, or too costly to evaluate within its budget. This does not satisfy the gate. | +| Cannot assess | Required context is unavailable, incomplete, unsupported by preflight, or too costly to evaluate within its budget. | + +`Cannot assess` is the common case, not the exceptional one, and the design has to treat it that +way. A target whose render-vs-live epoch has not reported yet is already closed to live writes by +the [`RenderFidelityGate`][fidelity-gate] (`Unknown` until every scope reports clean, which is the +state at startup and after every watch-plan change). Preflight cannot assess such a target, and +under a fail-closed reading that ordinary, self-healing condition would have denied every covered +edit in the cluster until the scopes reported. Folding it to a warning is what makes the feature +survivable in a real cluster. + +### Rendering three outcomes onto a two-valued API + +`AdmissionResponse.allowed` is a `bool`. There is no third verdict, and no way to answer "don't +know" as a verdict. The third state is carried alongside the verdict instead, in +[`AdmissionResponse.warnings`][admission-response]: a list of strings the API server relays to the +client, which `kubectl` prints to stderr as `Warning:` while the request succeeds. + +| Outcome | `allowed` | Carried in | What the author sees | +|---|---|---|---| +| Publishable | `true` | nothing | Silence is the success case. | +| Refused | `false` | `status.message` | The request fails with the blocker and its source path. | +| Cannot assess | `true` | `warnings` | The write succeeds, with `Warning: could not verify …`. | + +This is the honest encoding: the operator says what it knows, and admits what it does not, without +converting ignorance into a refusal. + +Warning strings are budgeted. Keep each under 120 characters; the API server may truncate beyond +256, and may drop warnings when there are many. So a warning names the target and one reason code, +never a rendered diff. The full diagnostic goes to the handler's own logs and to +`AdmissionResponse.auditAnnotations`, which the API server records in the audit log under the +webhook's name. That is the durable trail for "how often could we not answer, and why", without +putting object names in metric labels. + +One limit worth stating plainly: with `failurePolicy: Ignore` (below), a handler that **times out** +produces no warning at all, because the API server never gets a response to relay. Answering +"cannot assess" quickly is therefore strictly better than answering late, and the budget in +[Request handling and availability](#request-handling-and-availability) exists to make the handler +give up early enough to still speak. Return structured diagnostics that can also serve the writer and future tooling: @@ -172,26 +315,35 @@ Return structured diagnostics that can also serve the writer and future tooling: Do not include Secret values or raw object bodies in error messages, logs, or metrics. A caller authorized to edit an object does not necessarily have permission to read all repository context. -## All involved targets must comply +## Any involved target may refuse For a covered request, resolve every target whose effective capture rules would publish the requested object change. Once enforcement applies to that edit, evaluate all its publishing destinations, including destinations whose own opt-in setting did not trigger enforcement. Do not choose the first match or ignore a refusing destination to obtain approval. -The decision is a conjunction: +The decision is three-valued, and `Cannot assess` folds toward allow: ```text -allow(request) = target resolution is complete - AND every involved target is Publishable +deny(request) = ANY involved target is Refused +warn(request) = no target is Refused AND any target is Cannot assess +allow(request) = every involved target is Publishable ``` | Target A | Target B | Admission result | |---|---|---| -| Publishable | Publishable | Allow the single API request. | +| Publishable | Publishable | Allow, silently. | | Publishable | Refused | Deny, naming B and its reason. | | Refused | Refused | Deny with both target reasons. | -| Publishable | Cannot assess | Deny, naming B's missing context. | +| Publishable | Cannot assess | Allow, with a warning naming B's missing context. | +| Refused | Cannot assess | Deny. A known blocker outranks an unknown. | + +A refusal from any single target denies the whole request, because the operator would not be able +to publish the change to that destination and the author should hear it now. But an unresolved or +unassessable destination is not evidence of a blocker, and treating it as one would put every +routine gap (a target still starting up, a render budget exceeded, a snapshot not yet warm) in +the path of an unrelated edit. That trade only makes sense for a gate claiming to be a guarantee, +and this one does not. An overlapping destination or conflicting writer claim is a configuration problem, not permission to run two writers against the same file. Existing ownership constraints must also pass. Targets @@ -212,8 +364,9 @@ team-a/checkout-prod passed preflight. The API change was not persisted. ``` An object outside explicitly enforced coverage proceeds through normal admission. Inside enforced -coverage, an unresolved publishing destination must be rejected rather than treated as a vacuous -success. The selector and configuration mechanism must make that distinction observable. +coverage, an unresolved publishing destination is a warning, not a silent success: the author is +told the operator could not see where the change was going. The selector and configuration +mechanism must make that distinction observable. ## Request handling and availability @@ -224,22 +377,52 @@ provide full edit prevention. Status and finalizer maintenance need explicit tre does not block cleanup or recovery. Subresource coverage is discussed in the walkthrough below. Support `kubectl apply --dry-run=server` through a side-effect-free handler with `sideEffects: None`. -Dry-run evaluates the same all-target rule and never starts publication. +Dry-run evaluates the same any-target rule and never starts publication. + +Use fail-open behavior for covered editing requests: `failurePolicy: Ignore` for invocation failure, +and an allow-with-warning for a target that cannot be assessed. Kubernetes distinguishes invocation +failure from explicit webhook denial; see [admission configuration][kubernetes-admission]. Only the +second is ours to shape. The first is the API server deciding what to do when we do not answer, +and `Ignore` is the only safe answer for a webhook whose backend is the operator's own pod. -Use fail-closed behavior for covered editing requests: `failurePolicy: Fail` for invocation failure -and explicit denial for a target that cannot be assessed. This follows the stronger rule that all -targets must pass. A warning followed by approval would not meet that rule. Kubernetes distinguishes -invocation failure from explicit webhook denial; see [admission configuration][kubernetes-admission]. +That last point is not a preference. The handler is served by the gitops-reverser pod itself, so a +`Fail` policy matching core resources deadlocks the cluster at bootstrap: the API server cannot +admit the pod that serves the webhook it must consult in order to admit the pod. The existing +`/validate-all` observer records the same constraint and the same resolution +([`config/webhook/validating-webhook.yaml`][validate-all-config]). A future `Fail` posture would +need, at minimum, a `namespaceSelector` excluding `kube-system` and the operator's own namespace, +and would still be buying a guarantee this design has already declined to sell. -This choice trades editing availability for prevention. During an outage, explain that preflight -could not be completed; do not describe the requested content as unsupported. Keep operator -configuration and webhook recovery operations outside content enforcement, with administrator -recovery instructions. Disabling the gate deliberately relaxes prevention but never the writer's -checks. The exact selectors and deployment controls remain implementation questions. +Follow the posture already shipped for `/validate-all`: `timeoutSeconds: 1`, `failurePolicy: +Ignore`. A slow or unreachable handler must cost a write nothing but a warning it may not even see. Bound execution time, render cost, and concurrency. No Git round-trip belongs in the admission -path. Budget exhaustion is “cannot assess” and denies a covered request. Measure supported layouts -before choosing limits; use distinct diagnostics for missing state, stale state, and render cost. +path. Budget exhaustion is “cannot assess”, which warns rather than denies. Measure supported +layouts before choosing limits; use distinct diagnostics for missing state, stale state, and render +cost. The dominant cost is the render precondition, which runs two kustomize builds over the +batch's in-memory content ([`renderPrecondition`][render-precondition]); measure it against the +layout corpus before fixing a budget, and skip it for a non-kustomize folder, where it does not run +at all. + +During an outage, explain that preflight could not be completed; do not describe the requested +content as unsupported. Keep operator configuration and webhook recovery operations outside content +enforcement. Disabling the gate deliberately relaxes advice but never the writer's checks. + +### Deployment shape: one replica, one writer + +The operator does not support HA today: the chart refuses `replicaCount > 1` +([`validate-replica-count.yaml`][replica-guard]), and leader election is declared by the +`WorkerManager` and watch `Manager` but never enabled on the manager. So there is exactly one pod, +holding every `BranchWorker` and every worktree, and the admission handler runs in that same +process. It answers from memory, with no hop. + +Preflight must not block HA later, and it does not: worker ownership is keyed per branch +(`BranchKey{RepoNamespace, RepoName, Branch}`, so many `GitTarget`s share one worker), which is the +unit any future ownership record has to use. When HA arrives, a pod that does not own the branch +answers `Cannot assess` and the request proceeds with a warning: the same degradation as a cold +snapshot, requiring no new correctness argument. Routing a question to the owning pod is then an +optimization that reduces warnings, not a prerequisite for the gate. Designing that routing is out +of scope here and belongs with the HA work itself. ## Single requests and coordinated changes @@ -470,8 +653,9 @@ including the case where R2 is discovered without any intervening API request. ### Availability and feedback -1. What deployment controls and administrator recovery procedure make fail-closed editing practical - without placing the operator's own repair operations behind the gate? +1. How does an editing surface that is not `kubectl` surface a warning? Warnings are ephemeral + (relayed to the client and stored nowhere), so a UI-driven editor has to read and display them + deliberately or the "cannot assess" signal is lost to its users entirely. 2. How does a caller distinguish a known refusal, a temporary inability to assess, and a later publishing failure? Which cases are worth retrying? 3. Which existing target conditions carry each late failure, and what full-scope revalidation @@ -483,12 +667,62 @@ including the case where R2 is discovered without any intervening API request. ## Delivery sequence and acceptance evidence -1. Extract and test a read-only preflight against an explicit snapshot. Establish planner parity +1. Extract and test a read-only preflight against the checked-out working copy, with a test that + proves it performs no Git operation and leaves the copy untouched. Establish planner parity with the writer for the initial supported operations. 2. Map late refusals and incomplete/skipped plans to target errors; verify resync recovery. 3. Add the opt-in validating endpoint and server-side dry-run behavior with bounded evaluation. 4. Demonstrate known acceptance, known refusal, and indeterminate behavior end to end. +### Difficulty + +The advisory posture removes most of what would have been hard. What remains is unevenly +distributed: one step is nearly free because of how the writer was already built, and one step is +the actual work. + +| Step | Size | Why | +|---|---|---| +| 1. Read-only preflight | **Small–Medium** | The split already exists. See below. | +| 2. Skipped plans become target errors | **Medium**, and the riskiest | Changes shipped write-path behavior. | +| 3. Endpoint, chart, coverage config | **Medium** | Mostly packaging, little of it novel. | +| 4. End-to-end evidence | **Medium** | Corpus and fixtures exist; the admission cases do not. | + +**Step 1 is small because the writer is already plan-then-flush.** `flushEventsToWorktree` builds +the entire change in memory (`writeBatch` accumulates `fileBuffer`s and never touches the +filesystem), and [`writeBatch.flush`][flush-planning] then runs four pure preconditions +(`ignoreShadowPrecondition`, `pathScopePrecondition`, `fanInPrecondition`, `renderPrecondition`) +*before* the byte-writing loop. Every refusal this feature wants to surface is raised in those four +calls, on in-memory state, with no I/O. So the extraction is: split `flush` into `verify()` and +`writeOut()`, and let preflight run everything up to `verify()`. That is a mechanical refactor of +one function, not a re-architecture. The earlier caution that the planner might not be callable +from admission turns out to be over-cautious: the seam was built in. + +Two real tasks remain in that step: suppress `scanLayout`, which publishes `GitTarget` status and +must not run on a read-only path; and decide how to read the worktree while the event loop may hold +`repoMu`. With a 1s timeout and allow-on-timeout, a bounded `TryLock` is sufficient: contention +becomes a warning, not a stall. + +**Step 2 is the risk, and it is not webhook work at all.** Making a skipped plan visible means +changing behavior that is deliberate today: +[`TestOverlayAuthors_DeletePatch_SkipsOnPathCollision`][placement-tests] asserts +`require.NoError(..., "a patch-path collision must be skipped, not error")`. Turning that into a +refusal can stall targets that are healthy now, with no admission involved. It deserves its own +change, its own release note, and its own soak. It is worth doing whether or not preflight ships, +because a front gate that only advises makes the authoritative rear gate more important, not +less. + +**Step 3 has no novel plumbing.** The admission server, cert wiring, and a second webhook entry all +exist; `/validate-all` already runs at `timeoutSeconds: 1` with `failurePolicy: Ignore`, which is +exactly the posture this feature wants. The new surface is chart packaging (`/validate-all` is +deliberately e2e-only and not in the chart), the opt-in coverage selector, and the rule set. The +one undecided piece is how coverage is expressed and kept consistent with effective +capture claims. + +The largest remaining uncertainty is not difficulty but budget: whether two kustomize builds over a +realistic repository fit inside a 1s admission timeout often enough to be useful. Measure that +against the layout corpus before step 3. If it does not fit, the feature still works: it warns +more often than it answers, which is a disappointing gate rather than a broken one. + Required evidence for the feature includes: - Shape 8's image edit passes preflight and produces the overlay declaration. @@ -498,8 +732,10 @@ Required evidence for the feature includes: future batch. - A Git or configuration change between admission and flush triggers fresh write verification. - Dry-run produces a decision without enqueueing work or advancing publishing state. -- All involved targets must pass; an unresolved target, unavailable snapshot, or timeout denies - covered requests with a clear reason. +- A refusal from any single involved target denies the request, naming that target. +- An unresolved target, unavailable snapshot, or exhausted budget allows the request and returns a + warning that names what could not be assessed, and never reports it as unsupported content. +- A handler that is unreachable, or slower than its timeout, costs the request nothing. - Unselected resources and excluded subresources behave according to the documented coverage. - A bad direct Git commit fails the affected target when observed, even without a new API edit. - A rejected request alone leaves a healthy target healthy. @@ -520,7 +756,7 @@ write checks and target errors. Per-edit residue accounting, partial reflection, `FullyReflected` conditions are dropped from this workstream. [Admission consent][consent] explores acknowledgment of broader effects. It remains deferred. -The all-target requirement grants no additional authority and introduces no consent bypass. +The any-target refusal rule grants no additional authority and introduces no consent bypass. [Where validation lives][validation-placement] argues for schema and CEL where possible and reconcile-time checks for cross-object rules. This proposal retains authoritative write-time @@ -533,8 +769,13 @@ remain separate workstreams. This feature makes existing publishing constraints point where an author can most easily respond. [kubernetes-admission]: https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/ +[admission-response]: https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#response [shape8]: ../../test/fixtures/layout-corpus/shapes/8-base-owned-field-edit/README.md [flush-planning]: ../../internal/git/plan_flush.go +[fidelity-gate]: ../../internal/git/render_fidelity_gate.go +[render-precondition]: ../../internal/git/plan_flush.go +[validate-all-config]: ../../config/webhook/validating-webhook.yaml +[replica-guard]: ../../charts/gitops-reverser/templates/validate-replica-count.yaml [override-projection]: ../../internal/manifestanalyzer/overrides_projection.go [render-verification]: ../../internal/manifestanalyzer/render_verify.go [namespace-admission]: ../../internal/webhook/watchrule_source_namespace_admission.go From 66cfa3d61e35090505c3fb305cdf9b320f53f01f Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 11 Sep 2026 07:20:40 +0000 Subject: [PATCH 2/4] fix(gittarget): report scoped path refusals --- docs/INDEX.md | 3 +- docs/TODO.md | 8 + docs/configuration.md | 33 +++ docs/design/git-write-preflight.md | 185 +++++++++++---- docs/design/gittarget-red-status-plan.md | 222 ++++++++++++++++++ internal/git/branch_worker.go | 44 +++- internal/git/branch_worker_split_test.go | 42 ++++ internal/git/git_path_refusal.go | 24 +- internal/git/git_path_refusal_test.go | 40 +++- internal/git/resync_flush.go | 16 ++ internal/git/resync_push_test.go | 4 +- internal/git/source_cell.go | 13 +- internal/git/types.go | 4 + .../git/write_boundary_precondition_test.go | 6 +- .../manifestanalyzer/acceptance_refusal.go | 87 ++++++- .../acceptance_refusal_test.go | 44 ++++ internal/watch/event_router.go | 8 +- internal/watch/event_router_test.go | 2 + internal/watch/git_path_acceptance.go | 51 +++- internal/watch/git_path_acceptance_test.go | 68 +++++- internal/watch/manager.go | 10 +- internal/watch/owner_test.go | 1 + internal/watch/target_watch.go | 17 +- test/e2e/unsupported_folder_e2e_test.go | 119 ++++++++-- 24 files changed, 941 insertions(+), 110 deletions(-) create mode 100644 docs/design/gittarget-red-status-plan.md create mode 100644 internal/manifestanalyzer/acceptance_refusal_test.go diff --git a/docs/INDEX.md b/docs/INDEX.md index 7050b555..3d007685 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -111,10 +111,11 @@ decided), **design, decided** (decision made, not built), **partly built**, **bu **deferred** (parked, kept as a decision record). The label is the first thing in the page, so you never have to read a proposal to find out it already shipped. -### Open — 15 pages +### Open — 16 pages | Doc | Open question | |---|---| +| [`gittarget-red-status-plan.md`](design/gittarget-red-status-plan.md) | **design, partly built.** The smaller alternative to Git write preflight: make a `GitTarget` turn red and explain why no commit was made, without moving Git checks into the admission webhook and without adding a second state source. Keeps the existing kstatus trio and domain conditions, ranks the most likely operator-facing failures, and has now shipped the first status-transition slice: refusal messages republish when details change, scoped refusals cannot be cleared by unrelated scope success, `BlockMessage()` carries stable issue kind/path/actor detail, and the unsupported-folder e2e covers Git-side break and recovery. | | [`open-asks-priority.md`](design/open-asks-priority.md) | **the work queue.** Merges three overlapping backlogs — the downstream consumer asks, the API-surface block left unbuilt by the status and configuration-model review, and the config-surface proposal (B1–B6) — into one ordered queue under four stated tests, and says where we deliberately do **not** do what was asked. The standing caveat narrowed once the layout model reversed: a Tier 2 entry belongs to postponed [#294](https://github.com/ConfigButler/gitops-reverser/issues/294) only if it breaks a `GitTarget` field, and everything else is independently schedulable. Makes one design call against what was asked: **delete Option C sibling inference** rather than ship an off-switch for it, because it let a human's edit to the repository change operator behavior with nothing in status recording the move. That deletion has shipped, and "what the deletion taught" records what building it found. **F9 is Tier 1**: the only item whose answer is unknown rather than whose work is unscheduled, and it gates planning the enum work | | [`proving-the-metric-surface.md`](design/proving-the-metric-surface.md) | **design, partly built.** What identity an operator needs to locate a failure, and what an e2e check has to do to be worth trusting. Written after a metric value that was declared, switched on, surfaced as a condition reason and counted turned out to have no producer at all, and the check added to catch that class then skipped whenever the counter read zero — so deleting every recording call left the suite green. Two rules follow: an assertion must not take its trigger from the metric under test, and no enum value may be reachable only through an injected fake. Carries a cardinality policy that separates bounded configuration identity from per-request identity, a table of which instrument families should own `source_cluster` directly rather than by join, and the PromQL reasoning for `increase()` versus `max_over_time` (neither applies mechanically: one cannot see an event before the first sample, the other cannot reconstruct totals across a reset). Steps 1–4 are built; step 5, diagnostics export, the `e2e_run` scrape label and a pinned-seed regression are not | | [`placement-visibility-and-declared-defaults.md`](design/placement-visibility-and-declared-defaults.md) | **design, half built.** The three questions the inference deletion left, decided and then built in two goes. PR #291 shipped the deletion and none of the eight items queued behind it; the residue was filed as [#295](https://github.com/ConfigButler/gitops-reverser/issues/295) — **which shipped in 0.42.1 via [#319](https://github.com/ConfigButler/gitops-reverser/pull/319) and is what reversed the layout model** — and [#296](https://github.com/ConfigButler/gitops-reverser/issues/296), whose largest half then shipped in [#326](https://github.com/ConfigButler/gitops-reverser/pull/326): `status.layout` became `status.placement`, and the ambiguous render root came with it as a refusal rather than a third counter outcome. The `declared` metric value has since split into `by_type`/`default` too, which was the one row with an external consumer because it changes a label a dashboard may select on. What still stands and is still unbuilt is the rest of the legibility set: the canonical path as a template constant, and `{kindLower}`, tracked on [#339](https://github.com/ConfigButler/gitops-reverser/issues/339) now that #296 is closed; the Event on the GitTarget was dropped. Its Question 2, a CRD default for `placement.default`, is **re-opened rather than superseded** and now lives as an open question in [`layout/model.md`](layout/model.md); the argument against it is legibility, since #319 removed the correctness objection | diff --git a/docs/TODO.md b/docs/TODO.md index ffadd6cc..1d1db4a8 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -4,6 +4,14 @@ This file is meant to track the smaller current backlog, not historical notes. ## Current backlog +- [ ] Finish making `GitTarget` turn red quickly and explain why no commit was made. + The plan is in [gittarget-red-status-plan.md](design/gittarget-red-status-plan.md). This is the + replacement direction for Git write preflight: no admission webhook Git work, no extra state + source, and no promise before etcd. The first slice has shipped: stale refusal messages now + republish, scoped refusal recovery is not over-broad, bounded refusal messages include stable + issue detail, and the unsupported-folder e2e covers Git-side break and recovery. Remaining work is + the broader reason/status contract table and any later durable restart behavior. + - [ ] **High availability (near-term priority).** Support running more than one controller replica; `replicaCount > 1` is currently hard-rejected by the chart. Redis/Valkey is already the shared store for watch resume cursors, command-author facts, and — since the fact-stream switchover — the diff --git a/docs/configuration.md b/docs/configuration.md index d5458726..6c05f789 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -489,6 +489,39 @@ The most useful status fields are: Use conditions for automation. +### Why a `GitTarget` is not committing + +Start with the target, then check the rules that feed it: + +```bash +kubectl get gittarget -A +kubectl describe gittarget -n +kubectl get watchrule,clusterwatchrule -A +``` + +A red `GitTarget` means the target itself cannot validate, run, or publish. `Ready=False` gives the +summary reason, while the domain conditions name the axis to fix: + +| Condition | What to check | +|---|---| +| `Validated=False` | The referenced `GitProvider`, branch policy, `ClusterProvider`, and namespace authorization. | +| `EncryptionConfigured=False` | The age/SOPS Secret and whether key generation is allowed. | +| `GitProviderReady=False` | Repository URL, credential Secret, host keys, and push permissions. | +| `ClusterProviderReady=False` or `SourceClusterReachable=False` | Source-cluster credentials and access. | +| `GitPathAccepted=False` | Repository files under `spec.path`; the message includes the issue kind, path, and actor when known. | +| `RenderMatchesLive=False` | Fields that no longer match the rendered source, often transformer-owned values. | + +A green `GitTarget` with a red `WatchRule` or `ClusterWatchRule` means the destination may be fine, +but the selected source objects are not being watched or authorized. Inspect the rule's `Ready`, +`GitTargetReady`, `SourceNamespaceAuthorized`, and `StreamsRunning` conditions. + +After fixing a Git-side folder problem, request a fresh read of the path: + +```bash +kubectl annotate gittarget -n \ + reconcile.configbutler.ai/requestedAt="$(date -Iseconds)" --overwrite +``` + The kstatus trio is also on Prometheus, so the same answer is alertable without a kubeconfig: `gitopsreverser_resource_condition{kind="GitTarget", type="Ready", status="False"} == 1` names every target that has not been accepted, and `reason` says which gate refused. The metric covers `Ready`, diff --git a/docs/design/git-write-preflight.md b/docs/design/git-write-preflight.md index 6c5b7c52..03894aca 100644 --- a/docs/design/git-write-preflight.md +++ b/docs/design/git-write-preflight.md @@ -4,6 +4,28 @@ > surface workflow. The two-refusal model, the any-target refusal rule, and the advisory > (fail-open) posture are decided direction. API names and diagnostic shapes remain illustrative. +## Purpose + +This feature is an opt-in fast feedback path for editing surfaces that expect their changes to be +reverse-written into a supported Git source. Its job is to answer one narrow question while the +author is still making the edit: + +> Can this requested object change be represented by the writer's currently supported source +> operations for every involved `GitTarget`? + +That makes it a structural allow/warn/disallow check. It is about known support boundaries: +base-owned fields that would need unsupported patch authoring, writes that would escape a target's +source scope, fields supplied by transformers, sibling objects that would move, ignored paths, +unsupported source features, and objects with no publishing destination. It is not about +race-proofing Git, policing concurrent editors, or proving the push will later succeed. Kubernetes +remains the accepted state of record; the writer follows that state and still performs +authoritative checks before committing. + +Use this gate where a user is editing a live object as a shortcut for producing Git they expect to +keep. Do not enable it merely because the operator observes a namespace. Live-cluster mirroring, +audit capture, and discovery should continue to accept API writes even when the mirror cannot +represent every field. + ## Recommendation Build a shared, read-only Git write preflight and expose it through an opt-in validating admission @@ -16,16 +38,30 @@ already holds, quickly and without touching the network. It is deliberately not change will reach Git. See [What this gate is and is not](#what-this-gate-is-and-is-not). Prioritize this over broader Helm support or additional source transformations for the editing -surface use case. It makes the existing supported surface easier to trust: an author learns that -a change cannot be saved while making it, instead of discovering a stalled target afterward. +surface use case. It makes the existing supported surface easier to trust: an author learns that a +live change cannot be reverse-written while making it, instead of discovering a stalled target +afterward. Call the operation **Git write preflight**. “Cannot be reverted” sounds like the operator cannot -undo a change. The question here is whether it can write the proposed change back to Git. +undo a change. “Cannot be reversed” is closer, but still ambiguous. The question here is whether +the writer can publish the proposed live-state change back to Git. Use validating admission because the decision is acceptance or rejection. The handler should preserve the author's requested object. Kubernetes runs validating admission after mutation; its [admission documentation][kubernetes-admission] describes the ordering and response contract. +### Implementation bar + +Implement this only if the first version stays inside the narrow contract above. It is worthwhile +when it gives a fast, local answer for common edits that would otherwise make the `GitTarget` turn +red after the user's command already succeeded. It is not worthwhile if it recreates admission or +audit as a second state-capture path, waits on the writer, performs Git work in the webhook path, or +claims coverage for subresources and coordinated batches it cannot faithfully model. + +The fallback remains valuable and simpler: make late structural failures turn the affected +`GitTarget` red quickly and clearly. Preflight exists to move some of those messages earlier for +editing surfaces, not to replace target status as the authoritative publication signal. + ## The user-visible problem Today Kubernetes can accept an object update that the publisher subsequently refuses. The author @@ -129,7 +165,7 @@ future commit on the branch is supported. State this plainly in user-facing documentation, because the value of an advisory gate depends entirely on people knowing which half they are in. The dividing line is simple: preflight answers questions about the **shape of the repository and the shape of the change**, both of which it can -read from the working copy it already has. It cannot answer questions about **the future or the +read from a branch filesystem it already has. It cannot answer questions about **the future or the network**. Answerable from the offline copy, and therefore reliable: @@ -144,7 +180,7 @@ Answerable from the offline copy, and therefore reliable: | Is this field supplied by a build transformer or owned by a tolerated patch? | source-form and patch checks | | Is the folder structurally unsupported (undecodable build file, remote base)? | acceptance gate | | Does the object resolve to no publishing destination at all? | target resolution | -| Would the delete patch collide with an existing file? | placement, once [step 2](#difficulty) lands | +| Would the delete patch collide with an existing file? | placement, once skipped-plan refusals land | Not answerable, and never claimed: @@ -162,6 +198,19 @@ when someone commits, not between one `kubectl apply` and the next, so a shape-d as true a second later as it was when computed. The answers lost to staleness live entirely in the second table, where the honest response was never anything but "cannot know". +### Where useful answers exist today + +The existing tests and layout corpus support a limited but useful first surface. These are writer +facts, not admission facts; the webhook still needs its own persistence and warning tests. + +| Source shape | Useful preflight answers | First-version limits | +|---|---|---| +| Flat and tree folders, namespaces serialized ([shapes 1 and 3][shape-readme]) | Placement, in-place edits, `pathScopePrecondition`, `.gittargetignore` shadow checks, and fan-in checks where a plan touches shared context. | No claim about a downstream deployer that later rewrites the object. | +| Namespace-free flat/tree folders ([shapes 2 and 4][shape-readme]) | The same file-placement and write-boundary answers, plus the existing one-source-namespace fence. | The external namespace supplier is intentionally unverifiable. | +| Single Kustomize folders ([shape 5][shape-readme]) | Resource registration, namespace inference, supported direct edits, and render verification. | Render cost may force `Cannot assess` under the admission budget. | +| Base/overlay and layered Kustomize folders ([shapes 6-8][shape-readme]) | Overlay-local new objects, image and replica override authoring, base-owned unsupported-field refusals, shared-entry sibling refusals, and unsupported source-form refusals. | Single-object admission cannot represent coordinated sibling changes; direct Git remains the batch route. | +| Field-patch-only events such as `/scale` | The writer can patch a parent Deployment when the watch/audit path supplies a field patch. | Admission does not see subresources in the current webhook shape. Exclude them until a parent-object adapter exists. | + Enable this feature explicitly for editing surfaces whose authored changes are intended to reach Git. Keep it off by default for live-cluster mirroring, audit capture, and discovery. With the gate off, the operator still refuses unsafe Git writes and reports target errors; it does not restrict @@ -178,7 +227,7 @@ Otherwise admission and publication will disagree as support evolves. ```mermaid flowchart TD - R[Admission request] --> S[Resolve involved targets against the local working copy] + R[Admission request] --> S[Resolve involved targets against a published read snapshot] S --> P[Shared write planner and render verification, in memory] P --> A[Every target publishable] P --> D[Any refusal: deny with target reasons] @@ -192,10 +241,10 @@ flowchart TD V -->|Refusal| X[Abort affected flush and report GitTarget error] ``` -The admission path must not commit, push, fetch, enqueue a publication, or advance watch state. It -reads the working copy and plans in memory; see [The no-Git-operations -rule](#the-no-git-operations-rule). A request may still fail after this webhook approves it, and -watch remains the source of persisted object state. +The admission path must not commit, push, fetch, enqueue a publication, advance watch state, or wait +for the writer. It reads a snapshot the branch worker already published and plans in memory; see +[The no-Git-operations rule](#the-no-git-operations-rule). A request may still fail after this +webhook approves it, and watch remains the source of persisted object state. Candidate implementation seams: @@ -208,6 +257,39 @@ Candidate implementation seams: These are starting points for a refactor, not a claim that the current planner can be called from admission without changes. Establish what state it reads and mutates before choosing an interface. +### Where the request lands + +The natural attachment point is the branch worker's normal write-planning boundary: the same place +where a watched change is converted into an in-memory write batch before a commit is created. +Admission should translate the API request into that planner's input and ask for a verdict; it +should not own a separate set of support rules. + +In other words, the webhook is the transport, not the implementation. The implementation belongs +beside commit planning, because that is where the operator already knows the target branch, +worktree, render scope, write boundaries, override projections, and exact refusal reasons. The +branch worker should publish a ready read snapshot outside admission; the webhook path must take no +writer lock, perform no refresh, and either evaluate immediately against that snapshot or return +`Cannot assess`. + +If contention proves material, the branch worker may own a refreshed read-only preflight view; the +cost is a second snapshot with its own staleness, so measure before adding it. + +### Why this is not a state source + +Do not put preflight back into the ingestion model. The project deliberately moved state capture to +watch streams because admission and audit sit at the wrong moment for final object truth: they see +the request, while defaulting, conversion, controllers, finalizers, and storage still decide what +eventually appears in etcd. Admission is still useful for this feature because the question is +narrower: whether the requested shape appears serializable by the supported writer. + +That means preflight never records desired state and never substitutes for the watch. It can deny a +known structural mismatch before persistence, or warn that it cannot answer quickly. Once the API +server accepts a request, only the watch stream supplies the object the writer follows. + +This constraint is also the guard against reintroducing the removed admission/audit machinery. A +preflight adapter may normalize an admission request enough to ask the writer's planner a question; +it must not become a parallel event format that the commit loop consumes. + ### Evaluation input The planner needs the resolved source identity and target, requested operation, old and proposed @@ -222,12 +304,15 @@ configuration changes or pending local state. #### The no-Git-operations rule -Preflight reads the **already checked-out worktree on disk** and nothing else. This is a hard -invariant, not a performance goal: +Preflight reads an **already-prepared, stable branch filesystem** and nothing else. Because the +webhook path must not take the writer lock, the active worktree is not safe to read while +`writeBatch.flush` may be writing files one by one. The branch worker must publish a stable read +view outside admission first: an immutable file snapshot, a read-only mirror, or a separate checkout +with an explicit refresh policy. This is a hard invariant, not a performance goal: > The preflight path performs no Git operation. No fetch, no pull, no push, no commit, no -> checkout, no index write. It reads files from the working copy the branch worker already has, -> and plans in memory. +> checkout, no index write. It reads files from a stable filesystem view the branch worker already +> published, and plans in memory. The current writer makes this cheap to honour, because everything before the byte-writing loop is already of that shape. `scanRenderScope` walks the checked-out directory with ordinary filesystem @@ -236,12 +321,17 @@ touched path; every precondition then runs over those in-memory buffers. The one path is a local `HEAD` read for the revision stamp, and it belongs to the layout report, which does not run under preflight anyway. -Enforce it rather than intending it. The preflight entry point should take the worktree as a -read-only filesystem, so a Git operation is not merely discouraged but unavailable to it, and a -test should assert that a preflight evaluation leaves the working copy and the index byte-identical. +Enforce it rather than intending it. The preflight entry point should take an already-published +read-only filesystem, so a Git operation is not merely discouraged but unavailable to it, and a test +should assert that a preflight evaluation leaves the active working copy, any preflight view, and +the index byte-identical. The consequence is deliberate: preflight's view of Git is exactly as fresh as the branch worker's -last fetch, and no fresher. It answers from what the operator already knows. +last published branch filesystem, and no fresher. It answers from what the operator already knows. + +A missing stable snapshot is `Cannot assess`, never a reason to inspect the mutating worktree +directly. Torn reads are worse than warnings because they can produce a confident refusal from a +repository state the writer would never have observed. #### When there is no local copy @@ -496,8 +586,9 @@ or per-edit residue lifecycle is introduced. ## Walkthrough: expected behavior and current evidence These cases assume an accepted base plus leaf-overlay layout, explicit gate coverage, and fresh -snapshots. “Allow” means every involved target passes the complete preflight. The webhook is still -unbuilt; linked tests establish current writer behavior, not completed admission behavior. +snapshots. “Allow, silently” means every involved target passes the complete preflight; “allow with +warning” is the `Cannot assess` path. The webhook is still unbuilt; linked tests establish current +writer behavior, not completed admission behavior. ### Image and replica changes on a base-owned Deployment @@ -512,8 +603,8 @@ Shape 8 provides the inspectable image input and expected Git patch. `kubectl scale` uses `/scale`. The [override tests][override-tests] cover the writer's scale field patch, but admission must reconstruct the parent object and resolve its targets to make the same assessment. A gate covering ordinary UPDATE alone must not claim it covers this command. Require -that adapter before advertising protected scaling; deny covered scale requests that cannot be -assessed instead of silently bypassing preflight. +that adapter before advertising protected scaling. Until then, exclude `/scale` from preflight +coverage and disclose the gap. ### Base-owned environment variable, resource limit, or probe @@ -604,9 +695,9 @@ For the same API image edit, let A have an isolated override and B have an overr unrelated Deployment. A passes, B refuses, and admission denies the whole object request. Neither publishes that rejected request. Neither healthy target enters error solely due to the denial. -If B instead lacks a usable snapshot, deny with “cannot assess B.” If B's current repository is -invalid, B independently reports its target error. Correcting B's repository and completing resync -allows a fresh request to pass, provided all involved targets then comply. +If B instead lacks a usable snapshot, allow with a warning: “cannot assess B.” If B's current +repository is invalid, B independently reports its target error. Correcting B's repository and +completing resync allows a fresh request to pass, provided all involved targets then comply. If both pass at R1 and a direct Git commit invalidates B at R2, B's write-time validation still fails and B enters error. A may already have published. Tests must make this asynchronous limit visible, @@ -617,7 +708,8 @@ including the case where R2 is discovered without any intervening API request. ### Authority and scope 1. How does request-to-target resolution prove that the involved set is complete, including remote - source identities and changing selectors? All involved targets must pass; that decision is settled. + source identities and changing selectors? The settled rule is any known refusal blocks the + request, while unknown target state folds to an allow-with-warning. 2. Who may enable the gate, and how are webhook selectors kept consistent with effective claims? Can users remove a label to leave coverage? 3. Where is the webhook installed when `ClusterProvider` points at a remote editing API, and how @@ -633,8 +725,8 @@ including the case where R2 is discovered without any intervening API request. affect false refusals, reproducibility, and concurrency? 3. How old may a snapshot be before the answer becomes “cannot assess”? What happens during startup or target reconfiguration? -4. Can planning and rendering run without mutating the manifest store or taking locks that delay - the publisher? What resource budget makes admission latency predictable? +4. Can planning and rendering run without mutating the manifest store or taking writer locks in the + webhook path? What resource budget makes admission latency predictable? 5. Do encrypted resources or other intentionally reduced render checks qualify for a publishable verdict, or require a narrower statement of what was checked? @@ -667,12 +759,14 @@ including the case where R2 is discovered without any intervening API request. ## Delivery sequence and acceptance evidence -1. Extract and test a read-only preflight against the checked-out working copy, with a test that - proves it performs no Git operation and leaves the copy untouched. Establish planner parity - with the writer for the initial supported operations. -2. Map late refusals and incomplete/skipped plans to target errors; verify resync recovery. -3. Add the opt-in validating endpoint and server-side dry-run behavior with bounded evaluation. -4. Demonstrate known acceptance, known refusal, and indeterminate behavior end to end. +1. Extract and test a read-only preflight planner, with a test that proves it performs no Git + operation and mutates no repository state. Establish planner parity with the writer for the + initial supported operations. +2. Publish a stable per-branch read snapshot outside admission. The webhook path must never wait on + the writer or read a worktree that can be mutating underneath it. +3. Map late refusals and incomplete/skipped plans to target errors; verify resync recovery. +4. Add the opt-in validating endpoint and server-side dry-run behavior with bounded evaluation. +5. Demonstrate known acceptance, known refusal, and indeterminate behavior end to end. ### Difficulty @@ -682,10 +776,11 @@ the actual work. | Step | Size | Why | |---|---|---| -| 1. Read-only preflight | **Small–Medium** | The split already exists. See below. | -| 2. Skipped plans become target errors | **Medium**, and the riskiest | Changes shipped write-path behavior. | -| 3. Endpoint, chart, coverage config | **Medium** | Mostly packaging, little of it novel. | -| 4. End-to-end evidence | **Medium** | Corpus and fixtures exist; the admission cases do not. | +| 1. Read-only preflight planner | **Small–Medium** | The flush split already exists. See below. | +| 2. Stable read snapshot | **Medium** | Needed for a lock-free webhook path; no current code publishes one. | +| 3. Skipped plans become target errors | **Medium**, and the riskiest | Changes shipped write-path behavior. | +| 4. Endpoint, chart, coverage config | **Medium** | Mostly packaging, little of it novel. | +| 5. End-to-end evidence | **Medium** | Corpus and fixtures exist; the admission cases do not. | **Step 1 is small because the writer is already plan-then-flush.** `flushEventsToWorktree` builds the entire change in memory (`writeBatch` accumulates `fileBuffer`s and never touches the @@ -697,12 +792,13 @@ calls, on in-memory state, with no I/O. So the extraction is: split `flush` into one function, not a re-architecture. The earlier caution that the planner might not be callable from admission turns out to be over-cautious: the seam was built in. -Two real tasks remain in that step: suppress `scanLayout`, which publishes `GitTarget` status and -must not run on a read-only path; and decide how to read the worktree while the event loop may hold -`repoMu`. With a 1s timeout and allow-on-timeout, a bounded `TryLock` is sufficient: contention -becomes a warning, not a stall. +Two real tasks remain around that step. First, suppress `scanLayout`, which publishes `GitTarget` +status and must not run on a read-only path. Second, publish a stable read snapshot the webhook can +use without waiting on the writer. That second task is real implementation work, not a free side +effect of the `flush` split. With a 1s timeout and allow-on-timeout, missing or stale snapshot +context becomes a warning, not a stall. -**Step 2 is the risk, and it is not webhook work at all.** Making a skipped plan visible means +**Step 3 is the risk, and it is not webhook work at all.** Making a skipped plan visible means changing behavior that is deliberate today: [`TestOverlayAuthors_DeletePatch_SkipsOnPathCollision`][placement-tests] asserts `require.NoError(..., "a patch-path collision must be skipped, not error")`. Turning that into a @@ -711,7 +807,7 @@ change, its own release note, and its own soak. It is worth doing whether or not because a front gate that only advises makes the authoritative rear gate more important, not less. -**Step 3 has no novel plumbing.** The admission server, cert wiring, and a second webhook entry all +**Step 4 has no novel plumbing.** The admission server, cert wiring, and a second webhook entry all exist; `/validate-all` already runs at `timeoutSeconds: 1` with `failurePolicy: Ignore`, which is exactly the posture this feature wants. The new surface is chart packaging (`/validate-all` is deliberately e2e-only and not in the chart), the opt-in coverage selector, and the rule set. The @@ -770,6 +866,7 @@ point where an author can most easily respond. [kubernetes-admission]: https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/ [admission-response]: https://kubernetes.io/docs/reference/access-authn-authz/extensible-admission-controllers/#response +[shape-readme]: ../../test/fixtures/layout-corpus/shapes/README.md [shape8]: ../../test/fixtures/layout-corpus/shapes/8-base-owned-field-edit/README.md [flush-planning]: ../../internal/git/plan_flush.go [fidelity-gate]: ../../internal/git/render_fidelity_gate.go diff --git a/docs/design/gittarget-red-status-plan.md b/docs/design/gittarget-red-status-plan.md new file mode 100644 index 00000000..e2c7ca65 --- /dev/null +++ b/docs/design/gittarget-red-status-plan.md @@ -0,0 +1,222 @@ +# Make GitTarget red when commits cannot happen + +> **design**: partly built. Index: [`../INDEX.md`](../INDEX.md) + +## Purpose + +This is the smaller alternative to Git write preflight. Instead of trying to predict an API write in +the admission path, make the existing `GitTarget` status answer the question operators actually have +after a change does not appear in Git: + +> Why did this target not commit, who can fix it, and what should I look at first? + +The watch stream remains the source of mirrored state. The writer remains the place that proves a +change can be represented in Git. The status surface should turn red after the writer refuses or +cannot run, without doing Git work in a webhook and without adding a second state-ingestion path. + +"Quickly" has a bounded meaning here, not an admission-path promise. A refusal report enqueues a +`GitTarget` reconcile through a best-effort channel. If that notification is dropped, status can stay +stale until the periodic requeue. A converged target can therefore take up to the steady requeue +interval to publish the new red state. That is still the right model: Kubernetes accepted the +object, the watch stream observed it, and the writer then reported whether Git could represent it. + +## Recommendation + +Keep the current kstatus shape: + +- `Ready=False`, `Reconciling=False`, `Stalled=True` for hard blocks that will not clear by waiting. +- `Ready=False`, `Reconciling=True`, `Stalled=False` for work that is still making progress. +- `Ready=True`, `Reconciling=False`, `Stalled=False` when the latest observed generation is healthy. + +Do not add a new phase field. Do not make the webhook participate. Improve the domain conditions that +already explain the summary: + +- `Validated` +- `EncryptionConfigured` +- `GitProviderReady` +- `ClusterProviderReady` +- `SourceClusterReachable` +- `StreamsRunning` +- `GitPathAccepted` +- `RenderMatchesLive` +- `LayoutResolved` + +The least invasive change is to make these conditions and messages complete enough that `kubectl get +gittarget -o wide` points at the right axis, and `kubectl describe gittarget` tells the operator what +to fix. + +Correctness comes before better prose. A more specific message is worse than today's message if it +can point at the first file that failed with that reason and never update when a different file fails +later. So the first implementation slice must fix the status transition semantics before enriching +`GitPathAccepted` messages. + +## Current constraints + +The existing surface is close, but four constraints matter for this plan: + +1. `GitPathAccepted` is target-level state held in memory by the watch manager. After a manager + restart the target reads accepted until a write or resync refuses again. +2. Acceptance-change notifications are best-effort. A full buffer drops the reconcile request and + the published status can stay stale until the periodic requeue. +3. `reportGitPathAcceptance` compares message changes as well as accepted flag and reason, so a new + first offender under the same reason republishes status. +4. Refusals remember their scope when the writer knows it. A successful per-scope resync clears only + a refusal raised by the same scope; an unrelated scope cannot make the target green. + +## Most likely situations + +| Likelihood | Situation | Existing signal | Desired operator answer | +|---|---|---|---| +| Very common | Referenced `GitProvider` is missing, unready, or the branch is not allowed. | `Validated=False` or `GitProviderReady=False`; `Ready=False`, `Stalled=True` or `Reconciling=True` depending on the dependency. | Name the provider, namespace, branch, and whether the fix is "create/fix the GitProvider" or "allow this branch". | +| Very common | No `ClusterProvider/default`, remote cluster credential broken, or this `GitTarget` namespace is not authorized to use the provider. | `Validated=False`, `ClusterProviderReady=False`, `SourceClusterReachable` condition. | Name the referenced ClusterProvider, whether it is missing, unready, unreachable, or refuses this target namespace. | +| Very common | Encryption is enabled but the age Secret is missing, malformed, or generation is disabled. | `EncryptionConfigured=False`; `Ready=False`, `Stalled=True`. | Name the Secret and exact missing/malformed field; say whether to create it or enable generation. | +| Common | Git folder contains unsupported or unsafe content. | `GitPathAccepted=False`, reason `UnsupportedContent`; writer commits nothing. | Name the first offending path and issue kind: duplicate identity, invalid YAML, non-KRM YAML, impure multi-doc file, mixed kustomization file, foreign file, symlink, submodule, or unsupported kustomize construct. | +| Common | `.gittargetignore` hides something the operator must write. | `GitPathAccepted=False`, reason `IgnoreShadowsManagedPath`. | Name the matching pattern and managed path; say to narrow the ignore rule or move the resource. | +| Common | A `GitTarget` path covers more than one kustomize render root. | `LayoutResolved=False`, reason `Ambiguous`; then write-time `GitPathAccepted=False`, reason `AmbiguousLayout` once a write needs placement. | Name the render roots and say to point the GitTarget at one leaf/root. `LayoutResolved` explains the shape but does not write the kstatus trio. | +| Common | A namespace-free target receives objects from more than one source namespace. | `GitPathAccepted=False`, reason `MultipleSourceNamespaces`. | Name the namespaces when available; say to split targets/rules or serialize namespaces. | +| Common | A placement template writes a new file outside the render root of a kustomize folder. | `GitPathAccepted=False`, reason `UnrenderedPlacement`; `status.placement` gives the root. | Name the resolved path and render root; say the file would not be rendered by the folder. | +| Less common but important | A planned edit would write outside `spec.path`. | `GitPathAccepted=False`, reason `WriteBoundaryRefused`. | Name the path and target scope; say this target reads shared context but only writes inside its path. | +| Less common but important | A file is reached by multiple render roots, so write-through would corrupt another environment. | `GitPathAccepted=False`, reason `WriteBoundaryRefused`. | Name the shared path and say write fan-in must be 1. | +| Less common but important | Kustomize render verification refuses the planned batch. | Pure `IssueRenderDoesNotMatchLive` goes to `RenderMatchesLive=False`; `IssueRenderRefused` goes to `GitPathAccepted=False`, reason `WriteBoundaryRefused`. | Separate "live no longer matches rendered tokens" from "the proposed write would not render correctly"; name the field/token/path when known. | +| Less common | The projection cannot place a field edit, usually a list without stable item identity. | `GitPathAccepted=False`, reason `WriteBoundaryRefused`. | Say this edit has no safe source-form representation and no user action can make that specific edit reversible today. | +| Operational | The branch worker cannot be wired for this target. | `Ready=False`, reason `WorkerUnavailable`; current code folds it into the kstatus trio. | Name the provider/branch wiring failure. | +| Operational, not target-red yet | The shared branch worker queue is full, or a background resync fails after enqueue. | Logs and metrics; queue and worker state are shared across targets. | Keep this in metrics/logs until durable queue or owner identity work can tie it to a target without lying. | +| Common trap, red elsewhere | A `WatchRule` or `ClusterWatchRule` is invalid, unauthorized, or not yet streaming. | Rule-level `Ready`, `GitTargetReady`, `SourceNamespaceAuthorized`, and `StreamsRunning`; the `GitTarget` may stay green. | Tell users to inspect rules whenever the target is green but no object is committing. | +| Normal, not red | Target is suspended. | `Ready=True`, reason `Suspended`. | Say no writes are attempted by request; keep placement scanning visible. | +| Normal, not red | No WatchRule selects the target. | `StreamsRunning=True`/`NoResolvedTypes`, `Ready=True`. | Say there is nothing to mirror, not that the target is broken. | +| Progress, not red | Initial replay or watch-plan settle is still running. | `Ready=False`, `Reconciling=True`, `Stalled=False`. | Say which axis is still progressing; avoid classifying it as a fault. | + +## Message contract + +Every red `GitTarget` should answer five questions in bounded prose. The condition message should be +one line, preferably under 240 characters and hard-capped at 512 characters before it is written to +status. Longer detail belongs in logs. + +1. **What stopped?** For example: `GitPathAccepted=False`, `Validated=False`, or + `EncryptionConfigured=False`. +2. **Where?** Name the object reference, branch/path, file path, document index, field path, or + source namespace when the code has it. +3. **Why was no commit made?** Use one reason string from the existing vocabulary. +4. **Who can act?** Reuse `AcceptanceIssue.Solvable` and `AcceptanceIssue.Actor`: + `repository-author`, `platform-operator`, or no actor when this release cannot support the case. +5. **What is the next check?** Point at the specific config or file to inspect, not a generic + troubleshooting page. + +Do not put per-event counts or last-attempt timestamps in status. Those belong in metrics. Status +should move on health transitions, spec/config changes, bounded rollups, or a changed refusal +message for an already-red reason. + +## Proposed implementation + +### Step 1: Pin the status and reason contract + +Add a table-driven unit test that constructs each existing hard-block condition and asserts the +resulting `Ready`, `Reconciling`, `Stalled`, domain condition reason, and message stem. This should +live near `internal/controller/gittarget_kstatus_test.go` and `internal/controller/gittarget_status_test.go`. + +Also pin the cross-package reason agreement. The reason strings are intentionally duplicated across +controller, watch, and manifest analyzer boundaries because import cycles prevent one shared +constant package today. A test should prove that every `manifestanalyzer.GitPathRefusalReason` +outcome is accepted by the `GitTarget` stalled-reason set and is represented in the controller +vocabulary. + +This first step should not change behavior. It makes the current user-facing surface visible. + +### Step 2: Fix acceptance state transitions + +Built: `reportGitPathAcceptance` compares `Message` as well as accepted flag and reason. That makes a +different offender under the same reason publish a fresh condition instead of leaving the first +message in place forever. + +Built: the implementation tracks refusal scope and clears only when the same scope succeeds. A +target-wide refusal still needs a target-wide proof, while a scoped refusal can recover through the +matching scoped resync. + +### Step 3: Improve refusal message rendering + +Built: split `AcceptanceRefusedError.Error()` from `BlockMessage()` for real. Tests assert `Error()` today, +so enriching status prose through `Error()` would churn unrelated caller expectations. + +Only one formatter is needed: both live and scoped-resync paths funnel through +`refused.BlockMessage()`. + +The formatter should keep the existing reason mapping, but include: + +- issue kind +- path and document index when present +- solvable/actor when present +- a short action hint derived from the issue kind + +Keep it bounded: first issue plus "`and N more`" is enough for status. Full detail can remain in +logs. If the cap truncates the hint, preserve kind, path, and actor before prose. + +### Step 4: Add missing red paths only where they are already proven + +Do not broaden support. Only surface failures the current code already knows: + +- live write-plan refusals +- scoped resync refusals +- render fidelity divergence +- worker wiring failure + +Do not add branch-worker queue-full or background-resync failure to `GitTarget` status in this +increment. Those paths are shared across targets or metrics-only today; forcing them onto one target +would be a diagnosis lie. + +### Step 5: Operator-facing documentation + +Add a short user doc section, probably in `docs/configuration.md`, called "Why a GitTarget is not +committing". It should say: + +```bash +kubectl get gittarget -A +kubectl describe gittarget -n +kubectl get watchrule,clusterwatchrule +``` + +Then map `Ready` reason and domain conditions to the likely fix. Also state the two places status can +be red: + +- A red `GitTarget` means the target itself cannot validate, run, or publish. +- A green `GitTarget` with a red `WatchRule` or `ClusterWatchRule` means the target may be fine, but + the selected source objects are not being watched or authorized. + +This is where most users will start, so it should be practical rather than architectural. + +### Step 6: Extend existing e2e coverage + +Do not add a new expensive e2e path just to retest refusal. Extend an existing refusal spec, such as +the unsupported-folder or foreign-content e2e, to assert the new message content. The existing tests +already prove `Ready=False`, `Stalled=True`, the domain reason, and no new Git commit. + +Also assert recovery there: start from a healthy target, push an unsupported Git-side change, request +a reconcile, observe `GitPathAccepted=False`, then undo the Git-side change, request another +reconcile, and observe the target and dependent rules recover. + +## Non-goals + +- No admission webhook preflight. +- No locks in the webhook path. +- No Git checkout, fetch, commit, or push from admission. +- No new state source beside the watch stream. +- No per-document list in `GitTarget.status`. +- No promise that an API write can be reversed before it reaches etcd. + +## Compatibility + +This plan is additive if it only changes condition messages, documents reason precedence, and adds +tests. A new status field would be additive at the CRD level, but it is not recommended for the first +increment because the existing condition set already has enough structure. + +Reason strings should remain stable where they already exist. If a new reason is unavoidable, add it +as a domain reason and cover it in kstatus tests before using it in controller output. + +Reason is also a metric label on `gitopsreverser_resource_condition`, so a new reason string is a new +time series. Prefer improving messages under an existing reason over minting another reason. + +## Open questions + +1. Should `AcceptanceIssue.Actor` remain status prose only, or should it become a tiny bounded status + field later? +2. Should acceptance refusal state survive manager restart, or is "red again on the next refused + write/resync" sufficient until the durable queue work exists? diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index dee12a6c..09090c9b 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -395,6 +395,7 @@ func (w *BranchWorker) EnqueueResync(request *ResyncRequest) bool { // for a newer snapshot. Swap in the newer request; the loop reads whatever // is current when the marker comes up. superseded := pending.request + request.RefreshRemote = request.RefreshRemote || superseded.RefreshRemote pending.request = request w.pendingResyncsMu.Unlock() // The superseded request's caller is waiting on its reply channel. Answer @@ -986,7 +987,7 @@ func (l *branchWorkerEventLoop) handleAtomicRequest(request *WriteRequest) { // 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) { + if l.w.reportPathRefusal(err, name, namespace, request.sourceCell()) { l.w.recordCommitFailure(commitFailureKindAtomic, commitFailureRefused) } else { l.w.recordCommitFailure(commitFailureKindAtomic, commitFailureError) @@ -1112,7 +1113,7 @@ func (l *branchWorkerEventLoop) finalizeOpenWindowWithReason(reason windowFinali // 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) { + if l.w.reportPathRefusal(err, targetName, targetNamespace, sourceCellForEvents(events)) { l.w.recordCommitFailure(commitFailureKindWindow, commitFailureRefused) } else { l.w.recordCommitFailure(commitFailureKindWindow, commitFailureError) @@ -1457,6 +1458,45 @@ func (w *BranchWorker) rebuildPendingWrites( return baseBranch, baseHash, nil } +func (w *BranchWorker) refreshRemoteAndRebuildPendingWrites(ctx context.Context, pendingWrites []PendingWrite) error { + w.repoMu.Lock() + defer w.repoMu.Unlock() + + if len(pendingWrites) == 0 { + return nil + } + + provider, err := w.getGitProvider(ctx) + if err != nil { + return fmt.Errorf("get GitProvider: %w", err) + } + + auth, err := getAuthFromSecret(ctx, w.Client, provider, w.sshHostKeys) + if err != nil { + return fmt.Errorf("resolve auth: %w", err) + } + + repoPath := w.repoPathForRemote(provider.Spec.URL) + repo, err := gogit.PlainOpen(repoPath) + if err != nil { + return fmt.Errorf("open repository: %w", err) + } + + pullReport, err := syncToRemoteFn(ctx, repo, plumbing.NewBranchReferenceName(w.Branch), auth) + if err != nil { + return fmt.Errorf("sync remote before replay: %w", err) + } + w.updateBranchMetadataFromPullReport(pullReport) + + rootBranch, rootHash, err := w.rebuildPendingWrites(repo, pendingWrites) + if err != nil { + return fmt.Errorf("rebuild pending writes: %w", err) + } + w.pushCycleRootBranch = rootBranch + w.pushCycleRootHash = rootHash + return nil +} + // tightenPendingPruneModes lowers every retained write's captured prune mode before replay. // Tightening only: see PruneMode.MoreRestrictiveOf for why loosening must NOT propagate. // diff --git a/internal/git/branch_worker_split_test.go b/internal/git/branch_worker_split_test.go index 16c0660a..ee9eab10 100644 --- a/internal/git/branch_worker_split_test.go +++ b/internal/git/branch_worker_split_test.go @@ -347,6 +347,48 @@ func TestPushPendingCommits_ReplaysOnConflict(t *testing.T) { assert.True(t, worker.pushCycleRootHash.IsZero()) } +func TestRefreshRemoteAndRebuildPendingWrites_ReplaysWithoutPushing(t *testing.T) { + worker, serverRepo, remoteURL := setupCommitPushSplitWorker(t) + + pendingWrite, err := worker.buildGroupedPendingWrite( + worker.ctx, + []Event{configMapEvent("from-operator", "alice", "team-a")}, + ) + require.NoError(t, err) + require.NoError(t, worker.commitPendingWrites([]PendingWrite{*pendingWrite}, false)) + pendingWrites := []PendingWrite{*pendingWrite} + + otherPath := filepath.Join(t.TempDir(), "other") + otherRepo, otherWorktree := initLocalRepo(t, otherPath, remoteURL, "main") + commitFileChange(t, otherWorktree, otherPath, "OUTSIDE.md", "from-other-actor\n") + require.NoError(t, otherRepo.Push(&git.PushOptions{ + RefSpecs: []config.RefSpec{config.RefSpec("refs/heads/main:refs/heads/main")}, + })) + + contendingRef, err := serverRepo.Reference(plumbing.NewBranchReferenceName("main"), true) + require.NoError(t, err) + + require.NoError(t, worker.refreshRemoteAndRebuildPendingWrites(worker.ctx, pendingWrites)) + + remoteRef, err := serverRepo.Reference(plumbing.NewBranchReferenceName("main"), true) + require.NoError(t, err) + assert.Equal(t, contendingRef.Hash(), remoteRef.Hash(), "refresh/replay must not publish retained writes") + assert.Equal(t, contendingRef.Hash(), worker.pushCycleRootHash, + "the replayed push cycle must be rooted at the fresh remote tip") + + localRepo, err := git.PlainOpen(worker.repoPathForRemote(remoteURL)) + require.NoError(t, err) + localRef, err := localRepo.Reference(plumbing.NewBranchReferenceName("main"), true) + require.NoError(t, err) + assert.Equal(t, pendingWrites[0].CommitSHA, localRef.Hash(), "the retained write carries its replayed SHA") + require.NotEqual(t, contendingRef.Hash(), localRef.Hash(), "the retained write is replayed locally") + + replayedCommit, err := localRepo.CommitObject(localRef.Hash()) + require.NoError(t, err) + require.Len(t, replayedCommit.ParentHashes, 1) + assert.Equal(t, contendingRef.Hash(), replayedCommit.ParentHashes[0]) +} + func TestPushPendingCommits_ReplayPreservesPendingWriteCommitOrder(t *testing.T) { worker, serverRepo, remoteURL := setupCommitPushSplitWorker(t) diff --git a/internal/git/git_path_refusal.go b/internal/git/git_path_refusal.go index e8e3bc3e..d82e26ca 100644 --- a/internal/git/git_path_refusal.go +++ b/internal/git/git_path_refusal.go @@ -27,7 +27,11 @@ func (w *BranchWorker) normalWritesAllowed(targetName, targetNamespace string) b // 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) +type PathRefusalReporter func( + target itypes.ResourceReference, + cell itypes.CellKey, + 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 @@ -40,11 +44,15 @@ type PathRefusalReporter func(target itypes.ResourceReference, refused *manifest // 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 { +// Recovery is the resync path's job: once the human fixes the Git path, the next successful resync +// for the same cell 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 string, + targetNamespace string, + cell itypes.CellKey, +) bool { var refused *manifestanalyzer.AcceptanceRefusedError if !errors.As(err, &refused) { return false @@ -58,9 +66,9 @@ func (w *BranchWorker) reportPathRefusal(err error, targetName, targetNamespace } target := itypes.NewResourceReference(targetName, targetNamespace) w.Log.Info("Live write refused: unsupported GitTarget path content", - "gitTarget", target.String(), "detail", refused.Error()) + "gitTarget", target.String(), "sourceCell", sourceCellForLog(cell), "detail", refused.Error()) if w.pathRefusal != nil { - w.pathRefusal(target, refused) + w.pathRefusal(target, cell, refused) } return true } diff --git a/internal/git/git_path_refusal_test.go b/internal/git/git_path_refusal_test.go index 289adcb0..1c32bfba 100644 --- a/internal/git/git_path_refusal_test.go +++ b/internal/git/git_path_refusal_test.go @@ -10,6 +10,7 @@ import ( "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/runtime/schema" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/types" @@ -28,21 +29,38 @@ func refusedError() error { } // 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) +type capturedRefusal struct { + target types.ResourceReference + cell types.CellKey +} + +func captureRefusals(w *BranchWorker) *[]capturedRefusal { + seen := &[]capturedRefusal{} + w.pathRefusal = func( + target types.ResourceReference, + cell types.CellKey, + _ *manifestanalyzer.AcceptanceRefusedError, + ) { + *seen = append(*seen, capturedRefusal{target: target, cell: cell}) } 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) { +func TestReportPathRefusal_ReportsAttributedRefusalWithSourceCell(t *testing.T) { w := &BranchWorker{Log: logr.Discard()} - seen := captureRefusals(w) + seen := &[]types.ResourceReference{} + cell := types.CellKeyFor(schema.GroupVersionResource{Version: "v1", Resource: "configmaps"}, "apps") + + w.pathRefusal = func( + target types.ResourceReference, + gotCell types.CellKey, + _ *manifestanalyzer.AcceptanceRefusedError, + ) { + *seen = append(*seen, target) + assert.Equal(t, cell, gotCell) + } - assert.True(t, w.reportPathRefusal(refusedError(), "podinfo-test", "team-a")) + assert.True(t, w.reportPathRefusal(refusedError(), "podinfo-test", "team-a", cell)) require.Len(t, *seen, 1) assert.Equal(t, types.NewResourceReference("podinfo-test", "team-a"), (*seen)[0]) } @@ -53,7 +71,7 @@ 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.False(t, w.reportPathRefusal(errors.New("remote hung up"), "podinfo-test", "team-a", types.CellKey{})) assert.Empty(t, *seen, "a transient write fault must not be reported as a Git path refusal") } @@ -70,7 +88,7 @@ func TestReportPathRefusal_UnattributableRefusalIsNotRecorded(t *testing.T) { w := &BranchWorker{Log: logr.Discard()} seen := captureRefusals(w) - assert.True(t, w.reportPathRefusal(refusedError(), c.name, c.ns), + assert.True(t, w.reportPathRefusal(refusedError(), c.name, c.ns, types.CellKey{}), "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) diff --git a/internal/git/resync_flush.go b/internal/git/resync_flush.go index df1b742f..579001ae 100644 --- a/internal/git/resync_flush.go +++ b/internal/git/resync_flush.go @@ -98,6 +98,22 @@ func (l *branchWorkerEventLoop) applyResync(req *ResyncRequest) { if !req.Heal { closedWindow = l.finalizeOpenWindowWithReason(windowFinalizeReasonResyncBeforeApply) } + if req.RefreshRemote { + if len(l.pendingWrites) > 0 { + if err := l.w.refreshRemoteAndRebuildPendingWrites(l.w.ctx, l.pendingWrites); err != nil { + l.w.Log.Error(err, "Failed to refresh remote before resync and replay pending writes", + "resources", len(req.Desired), + "gitTarget", req.GitTargetNamespace+"/"+req.GitTargetName, + "pendingWrites", len(l.pendingWrites)) + req.reply(ResyncResult{Err: fmt.Errorf("refresh remote before resync: %w", err)}) + return + } + } else if _, err := l.w.syncWithRemote(l.w.ctx); err != nil { + l.w.Log.Error(err, "Failed to refresh remote before resync", "resources", len(req.Desired)) + req.reply(ResyncResult{Err: fmt.Errorf("refresh remote before resync: %w", err)}) + return + } + } stats := &ResyncStats{} committed := false diff --git a/internal/git/resync_push_test.go b/internal/git/resync_push_test.go index e8d57ed1..60559ae8 100644 --- a/internal/git/resync_push_test.go +++ b/internal/git/resync_push_test.go @@ -63,7 +63,8 @@ func TestEnqueueResync_CoalescesSameScope(t *testing.T) { superseded := make(chan ResyncResult, 1) require.True(t, w.EnqueueResync(&ResyncRequest{ GitTargetNamespace: "ns", GitTargetName: "target", Revision: "1", - Result: superseded, + RefreshRemote: true, + Result: superseded, })) // The queue now has no free slot, yet a second request for the SAME scope is @@ -88,6 +89,7 @@ func TestEnqueueResync_CoalescesSameScope(t *testing.T) { require.NotNil(t, item.Resync) current := w.takePendingResync(item.Resync) assert.Equal(t, "2", current.Revision, "the marker runs the newest request for its scope") + assert.True(t, current.RefreshRemote, "coalescing must preserve a pending remote refresh") // The key is cleared, so the next resync for that scope queues a fresh marker. require.True(t, w.EnqueueResync(&ResyncRequest{ diff --git a/internal/git/source_cell.go b/internal/git/source_cell.go index ef3147f5..98548e86 100644 --- a/internal/git/source_cell.go +++ b/internal/git/source_cell.go @@ -56,9 +56,16 @@ func (r *WriteRequest) sourceCell() types.CellKey { if r == nil || len(r.Events) == 0 { return types.CellKey{} } - first := r.Events[0].SourceCell - for i := range r.Events { - if r.Events[i].SourceCell != first { + return sourceCellForEvents(r.Events) +} + +func sourceCellForEvents(events []Event) types.CellKey { + if len(events) == 0 { + return types.CellKey{} + } + first := events[0].SourceCell + for i := range events { + if events[i].SourceCell != first { return types.CellKey{} } } diff --git a/internal/git/types.go b/internal/git/types.go index 31c9c555..afcfc714 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -390,6 +390,10 @@ type ResyncRequest struct { // for idle recurs on every silence timeout, so it never starves. A first-sync backfill is NOT // a heal: it must establish initial state promptly. Heal bool + // RefreshRemote asks the worker to fetch/reset to the remote tip before evaluating the + // acceptance gate. Forced GitTarget rechecks use it because their trigger is often "I changed + // Git; look again", and the local checkout may still hold the refused revision. + RefreshRemote bool // SourceCell names the target-watch cell that gathered this snapshot. Zero for a // whole-GitTarget resync, which speaks for no single cell. Diagnostic only: nothing // filters the queue on it. See source_cell.go. diff --git a/internal/git/write_boundary_precondition_test.go b/internal/git/write_boundary_precondition_test.go index 2c2db4b5..2ccf0aaf 100644 --- a/internal/git/write_boundary_precondition_test.go +++ b/internal/git/write_boundary_precondition_test.go @@ -230,7 +230,11 @@ func TestEventLoop_LiveFanInRefusal_FailsGitTargetAndCommitsNothing(t *testing.T var refusals []*manifestanalyzer.AcceptanceRefusedError var refusedTargets []types.ResourceReference - worker.pathRefusal = func(target types.ResourceReference, refused *manifestanalyzer.AcceptanceRefusedError) { + worker.pathRefusal = func( + target types.ResourceReference, + _ types.CellKey, + refused *manifestanalyzer.AcceptanceRefusedError, + ) { refusedTargets = append(refusedTargets, target) refusals = append(refusals, refused) } diff --git a/internal/manifestanalyzer/acceptance_refusal.go b/internal/manifestanalyzer/acceptance_refusal.go index f7686bac..80672cc5 100644 --- a/internal/manifestanalyzer/acceptance_refusal.go +++ b/internal/manifestanalyzer/acceptance_refusal.go @@ -5,6 +5,7 @@ package manifestanalyzer import ( "fmt" "slices" + "strings" ) // AcceptanceRefusedError is the writer-facing error for a GitTarget folder the acceptance @@ -31,10 +32,90 @@ func (e *AcceptanceRefusedError) Error() string { first.Path, first.Message, len(e.Issues)-1) } +const acceptanceBlockMessageMax = 512 +const truncationSuffixLen = 3 + // BlockMessage returns a bounded, human-readable one-liner suitable for a GitTarget status -// condition / stream-block message. It is the same text as Error today, named separately so -// the surface intent is explicit at the call site. -func (e *AcceptanceRefusedError) BlockMessage() string { return e.Error() } +// condition / stream-block message. It keeps Error stable for callers while adding the +// machine-stable issue kind and, when known, the actor who can fix the folder. +func (e *AcceptanceRefusedError) BlockMessage() string { + if len(e.Issues) == 0 { + return e.Error() + } + + first := e.Issues[0] + parts := []string{"Git path refused"} + if first.Kind != "" { + parts = append(parts, "kind="+string(first.Kind)) + } + if first.Path != "" { + parts = append(parts, "path="+first.Path) + } + if first.DocumentIndex > 0 { + parts = append(parts, fmt.Sprintf("document=%d", first.DocumentIndex)) + } + if first.Solvable && first.Actor != ActorUnknown { + parts = append(parts, "actor="+string(first.Actor)) + } + + msg := strings.Join(parts, " ") + if first.Message != "" { + msg += ": " + first.Message + } + if hint := issueHint(first); hint != "" { + msg += "; " + hint + } + if len(e.Issues) > 1 { + msg += fmt.Sprintf(" (and %d more issue(s))", len(e.Issues)-1) + } + return capMessage(msg, acceptanceBlockMessageMax) +} + +func issueHint(issue AcceptanceIssue) string { + switch issue.Kind { + case IssueAmbiguousLayout: + return "point spec.path at one render root" + case IssueForeignFile, IssueForeignSymlink, IssueForeignSubmodule: + return "remove or move unmanaged content" + case IssueIgnoreShadowsManaged: + return "narrow .gittargetignore or move the managed resource" + case IssueMultipleSourceNamespaces: + return "split targets or serialize namespaces" + case IssueUnrenderedPlacement: + return "adjust placement so the file renders from the target root" + case IssueUnsupportedKustomize: + if issue.Solvable { + return "edit the unsupported kustomization feature" + } + return "use a supported source layout for this target" + case IssueWriteEscapesScope: + return "keep writes under spec.path" + case IssueDuplicate, + IssueImpureManagedFile, + IssueInvalidYAML, + IssueMixedFile, + IssueNonKRM, + IssueOutOfScope, + IssueRenderDoesNotMatchLive, + IssueRenderRefused, + IssueUnplaceableEdit, + IssueUnresolvedKRM, + IssueWriteFanIn: + return "" + default: + return "" + } +} + +func capMessage(msg string, limit int) string { + if len(msg) <= limit { + return msg + } + if limit <= truncationSuffixLen { + return msg[:limit] + } + return msg[:limit-truncationSuffixLen] + "..." +} // 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 diff --git a/internal/manifestanalyzer/acceptance_refusal_test.go b/internal/manifestanalyzer/acceptance_refusal_test.go new file mode 100644 index 00000000..022a2017 --- /dev/null +++ b/internal/manifestanalyzer/acceptance_refusal_test.go @@ -0,0 +1,44 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestAcceptanceRefusedError_BlockMessageIncludesStableDiagnosticFields(t *testing.T) { + refused := &AcceptanceRefusedError{ + Issues: []AcceptanceIssue{{ + Kind: IssueUnsupportedKustomize, + Path: "apps/kustomization.yaml", + Message: "uses remote base", + Solvable: true, + Actor: ActorRepositoryAuthor, + }}, + } + + got := refused.BlockMessage() + + assert.Contains(t, got, "kind=unsupported-kustomize") + assert.Contains(t, got, "path=apps/kustomization.yaml") + assert.Contains(t, got, "actor=repository-author") + assert.Contains(t, got, "uses remote base") + assert.Contains(t, got, "edit the unsupported kustomization feature") + assert.NotContains(t, refused.Error(), "kind=unsupported-kustomize", + "Error stays stable for callers that only need the coarse refusal") +} + +func TestAcceptanceRefusedError_BlockMessageIsBounded(t *testing.T) { + refused := &AcceptanceRefusedError{ + Issues: []AcceptanceIssue{{ + Kind: IssueForeignFile, + Path: "notes.txt", + Message: strings.Repeat("x", acceptanceBlockMessageMax), + }}, + } + + assert.LessOrEqual(t, len(refused.BlockMessage()), acceptanceBlockMessageMax) +} diff --git a/internal/watch/event_router.go b/internal/watch/event_router.go index 4d5dbdce..390c81f1 100644 --- a/internal/watch/event_router.go +++ b/internal/watch/event_router.go @@ -193,6 +193,7 @@ func (r *EventRouter) resolveWorkerForGitDest( // than the gather deletes managed documents outside it. heal marks a drift-correcting resync the // worker defers while a commit window is open. enqueued is false when the worker's queue was full // and dropped the request (its failure is still delivered on resultCh for the drain to record). +// refreshRemote asks the worker to fetch the latest remote tip before it inspects the folder. func (r *EventRouter) enqueueScopedResync( ctx context.Context, gitDest types.ResourceReference, @@ -201,6 +202,7 @@ func (r *EventRouter) enqueueScopedResync( desired []manifestanalyzer.DesiredResource, revision string, heal bool, + refreshRemote bool, ) (chan git.ResyncResult, bool, error) { worker, err := r.resolveWorkerForGitDest(ctx, gitDest) if err != nil { @@ -215,6 +217,7 @@ func (r *EventRouter) enqueueScopedResync( Scope: &scope, SourceCell: sourceCell, Heal: heal, + RefreshRemote: refreshRemote, Result: resultCh, }) return resultCh, enqueued, nil @@ -249,7 +252,7 @@ func (r *EventRouter) drainScopedResync( "gitDest", gitDest.String(), "cell", cell.String(), "created", result.Stats.Created, "updated", result.Stats.Updated, "deleted", result.Stats.Deleted) if r.WatchManager != nil { - r.WatchManager.MarkTargetGitPathAccepted(gitDest) + r.WatchManager.MarkTargetGitPathScopeAccepted(gitDest, cell) r.WatchManager.MarkTargetRenderFidelityScopeClean(gitDest, renderFidelityEpoch, cell) // Recorded for every applied resync, including the ones that retained nothing: zero // is the converged signal and is only meaningful if it is published as actively as a @@ -295,7 +298,8 @@ func (r *EventRouter) handleScopedResyncError( r.Log.Info("per-type "+kind+" refused: unsupported GitTarget path content", "gitDest", gitDest.String(), "cell", cell.String(), "detail", refused.Error()) if r.WatchManager != nil { - r.WatchManager.MarkTargetGitPathRefused(gitDest, gitPathRefusalReason(refused), refused.BlockMessage()) + r.WatchManager.MarkTargetGitPathScopeRefused( + gitDest, cell, gitPathRefusalReason(refused), refused.BlockMessage()) } return } diff --git a/internal/watch/event_router_test.go b/internal/watch/event_router_test.go index 6a418dc9..9bdb0736 100644 --- a/internal/watch/event_router_test.go +++ b/internal/watch/event_router_test.go @@ -104,6 +104,7 @@ func TestEnqueueScopedResync_ReportsMissingWorker(t *testing.T) { nil, "12", false, + false, ) require.Error(t, err) @@ -132,6 +133,7 @@ func TestEnqueueScopedResync_ReportsGoneGitTargetAsTerminal(t *testing.T) { nil, "12", false, + false, ) require.Error(t, err) diff --git a/internal/watch/git_path_acceptance.go b/internal/watch/git_path_acceptance.go index 75a1301c..673859d5 100644 --- a/internal/watch/git_path_acceptance.go +++ b/internal/watch/git_path_acceptance.go @@ -16,13 +16,14 @@ import ( // whether it was a live write or a background resync that hit it. func (m *Manager) ReportGitPathRefusal( gitDest types.ResourceReference, + cell types.CellKey, refused *manifestanalyzer.AcceptanceRefusedError, ) { if refused.AllIssuesOfKinds(manifestanalyzer.IssueRenderDoesNotMatchLive) { m.MarkTargetRenderFidelityDiverged(gitDest, renderFidelityDivergence(refused)) return } - m.MarkTargetGitPathRefused(gitDest, gitPathRefusalReason(refused), refused.BlockMessage()) + m.MarkTargetGitPathScopeRefused(gitDest, cell, gitPathRefusalReason(refused), refused.BlockMessage()) } // MarkTargetGitPathRefused records that the GitTarget path failed the structure-only @@ -39,9 +40,29 @@ func (m *Manager) MarkTargetGitPathRefused(gitDest types.ResourceReference, reas }) } -// MarkTargetGitPathAccepted clears any prior refusal for the GitTarget path. The steady-state -// resync calls it on every successful apply, so the fast path below has to be free: an already -// accepted path takes a pointer load and returns. +// MarkTargetGitPathScopeRefused records that a scoped writer pass failed the structure-only +// acceptance gate. The status remains target-level, but the remembered scope prevents an +// unrelated successful resync from clearing it. +func (m *Manager) MarkTargetGitPathScopeRefused( + gitDest types.ResourceReference, + cell types.CellKey, + reason string, + message string, +) { + status := GitPathAcceptanceStatus{ + Accepted: false, + Reason: reason, + Message: message, + } + if cell != (types.CellKey{}) { + status.RefusedCell = cell + status.RefusedCellSet = true + } + m.reportGitPathAcceptance(gitDest, status) +} + +// MarkTargetGitPathAccepted clears any prior unscoped refusal for the GitTarget path. Scoped +// resyncs should call MarkTargetGitPathScopeAccepted so they only clear the refusal they proved. func (m *Manager) MarkTargetGitPathAccepted(gitDest types.ResourceReference) { if prior, had := m.watchPlane().acceptance[gitDest.Key()]; had && prior.Accepted { return @@ -53,12 +74,30 @@ func (m *Manager) MarkTargetGitPathAccepted(gitDest types.ResourceReference) { }) } +// MarkTargetGitPathScopeAccepted clears a scoped refusal only when the successful resync covered +// the same cell that produced the refusal. A successful replay of secrets must not hide a still +// impossible configmaps path, because the GitTarget status is the operator's only durable clue. +func (m *Manager) MarkTargetGitPathScopeAccepted(gitDest types.ResourceReference, cell types.CellKey) { + prior, had := m.watchPlane().acceptance[gitDest.Key()] + if had && !prior.Accepted && cell != (types.CellKey{}) { + if !prior.RefusedCellSet || prior.RefusedCell != cell { + return + } + } + m.MarkTargetGitPathAccepted(gitDest) +} + func (m *Manager) reportGitPathAcceptance(gitDest types.ResourceReference, status GitPathAcceptanceStatus) { changed := m.mutateWatchPlane(func(s *watchPlaneState) bool { prior, had := s.acceptance[gitDest.Key()] // Compared before the write, so "changed" describes what an operator would see: newly - // refused, a different refusal reason, or a recovery. - if had && prior.Accepted == status.Accepted && prior.Reason == status.Reason { + // refused, a different refusal reason/message, or a recovery. + if had && + prior.Accepted == status.Accepted && + prior.Reason == status.Reason && + prior.Message == status.Message && + prior.RefusedCell == status.RefusedCell && + prior.RefusedCellSet == status.RefusedCellSet { return false } status.At = metav1.Now() diff --git a/internal/watch/git_path_acceptance_test.go b/internal/watch/git_path_acceptance_test.go index b589619a..5cbeb195 100644 --- a/internal/watch/git_path_acceptance_test.go +++ b/internal/watch/git_path_acceptance_test.go @@ -5,12 +5,14 @@ package watch import ( "strings" "testing" + "time" "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/event" "github.com/ConfigButler/gitops-reverser/internal/git" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" @@ -29,8 +31,9 @@ import ( func TestReportGitPathRefusal_SurfacesWriteBoundaryRefusal(t *testing.T) { mgr := &Manager{Log: logr.Discard()} gitDest := types.NewResourceReference("podinfo-test", "team-a") + cell := types.CellKeyFor(configmapsGVR, "apps") - mgr.ReportGitPathRefusal(gitDest, &manifestanalyzer.AcceptanceRefusedError{ + mgr.ReportGitPathRefusal(gitDest, cell, &manifestanalyzer.AcceptanceRefusedError{ Issues: []manifestanalyzer.AcceptanceIssue{{ Kind: manifestanalyzer.IssueWriteFanIn, Path: "base/deployment.yaml", @@ -43,6 +46,8 @@ func TestReportGitPathRefusal_SurfacesWriteBoundaryRefusal(t *testing.T) { 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.Equal(t, cell, gitPath.RefusedCell) + assert.True(t, gitPath.RefusedCellSet) assert.Empty(t, mgr.watchPlane().streams, "a Git path refusal must not mutate stream readiness") } @@ -52,7 +57,7 @@ func TestReportGitPathRefusal_ContentRefusalKeepsUmbrellaReason(t *testing.T) { mgr := &Manager{Log: logr.Discard()} gitDest := types.NewResourceReference("podinfo-test", "team-a") - mgr.ReportGitPathRefusal(gitDest, &manifestanalyzer.AcceptanceRefusedError{ + mgr.ReportGitPathRefusal(gitDest, types.CellKey{}, &manifestanalyzer.AcceptanceRefusedError{ Issues: []manifestanalyzer.AcceptanceIssue{{ Kind: manifestanalyzer.IssueForeignFile, Path: "notes.txt", @@ -65,6 +70,54 @@ func TestReportGitPathRefusal_ContentRefusalKeepsUmbrellaReason(t *testing.T) { assert.Equal(t, "UnsupportedContent", gitPath.Reason) } +func TestGitPathAcceptance_MessageChangePublishesStatus(t *testing.T) { + mgr := &Manager{Log: logr.Discard()} + events := mgr.GitPathEvents() + gitDest := types.NewResourceReference("podinfo-test", "team-a") + + mgr.MarkTargetGitPathRefused(gitDest, "UnsupportedContent", "Git path refused at a.yaml") + requireGitPathEvent(t, events) + + mgr.MarkTargetGitPathRefused(gitDest, "UnsupportedContent", "Git path refused at b.yaml") + requireGitPathEvent(t, events) + + gitPath := mgr.GitPathAcceptanceForGitTarget(gitDest) + assert.Equal(t, "Git path refused at b.yaml", gitPath.Message) +} + +func TestGitPathAcceptance_SiblingScopeSuccessDoesNotClearRefusal(t *testing.T) { + mgr := &Manager{Log: logr.Discard()} + gitDest := types.NewResourceReference("podinfo-test", "team-a") + configMaps := types.CellKeyFor(configmapsGVR, "apps") + secrets := types.CellKeyFor(schema.GroupVersionResource{Version: "v1", Resource: "secrets"}, "apps") + + mgr.MarkTargetGitPathScopeRefused(gitDest, configMaps, "UnsupportedContent", "configmaps refused") + mgr.MarkTargetGitPathScopeAccepted(gitDest, secrets) + + gitPath := mgr.GitPathAcceptanceForGitTarget(gitDest) + assert.False(t, gitPath.Accepted, "an unrelated successful scope must not clear the target refusal") + assert.Equal(t, configMaps, gitPath.RefusedCell) + + mgr.MarkTargetGitPathScopeAccepted(gitDest, configMaps) + assert.True(t, mgr.GitPathAcceptanceForGitTarget(gitDest).Accepted) +} + +func TestGitPathAcceptance_ScopedSuccessDoesNotClearUnscopedRefusal(t *testing.T) { + mgr := &Manager{Log: logr.Discard()} + gitDest := types.NewResourceReference("podinfo-test", "team-a") + configMaps := types.CellKeyFor(configmapsGVR, "apps") + + mgr.MarkTargetGitPathRefused(gitDest, "UnsupportedContent", "whole target refused") + mgr.MarkTargetGitPathScopeAccepted(gitDest, configMaps) + + assert.False(t, mgr.GitPathAcceptanceForGitTarget(gitDest).Accepted, + "a cell success must not clear a target-wide refusal") + + mgr.MarkTargetGitPathScopeAccepted(gitDest, types.CellKey{}) + assert.True(t, mgr.GitPathAcceptanceForGitTarget(gitDest).Accepted, + "a whole-target proof may clear an unscoped refusal") +} + // 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. @@ -150,7 +203,7 @@ func TestReportGitPathRefusal_RenderFidelityKeepsGitPathAccepted(t *testing.T) { manager.EventRouter = NewEventRouter(workerManager, manager, nil, logr.Discard()) target := types.NewResourceReference("podinfo-test", "team-a") - manager.ReportGitPathRefusal(target, &manifestanalyzer.AcceptanceRefusedError{ + manager.ReportGitPathRefusal(target, types.CellKey{}, &manifestanalyzer.AcceptanceRefusedError{ Issues: []manifestanalyzer.AcceptanceIssue{{ Kind: manifestanalyzer.IssueRenderDoesNotMatchLive, Field: "data.region", Token: "${REGION}", }}, @@ -162,6 +215,15 @@ func TestReportGitPathRefusal_RenderFidelityKeepsGitPathAccepted(t *testing.T) { assert.Equal(t, "RenderDoesNotMatchLive", fidelity.Reason) } +func requireGitPathEvent(t *testing.T, events <-chan event.GenericEvent) { + t.Helper() + select { + case <-events: + case <-time.After(time.Second): + t.Fatal("expected GitTarget reconcile event after acceptance status changed") + } +} + // TestMarkRenderFidelityScopeClean_NamesAResultTheGateWouldNotTake covers the branch that made // Failure A undiagnosable: the gate answers applied=false for a stale revision, an unknown scope // or an unknown target, and the caller used to discard that answer without a word — so a scope diff --git a/internal/watch/manager.go b/internal/watch/manager.go index 47515730..b0a97c95 100644 --- a/internal/watch/manager.go +++ b/internal/watch/manager.go @@ -225,10 +225,12 @@ type Manager struct { // GitPathAcceptanceStatus is the whole-target write-safety status for a GitTarget path. type GitPathAcceptanceStatus struct { - Accepted bool - Reason string - Message string - At metav1.Time + Accepted bool + Reason string + Message string + At metav1.Time + RefusedCell types.CellKey + RefusedCellSet bool } const ( diff --git a/internal/watch/owner_test.go b/internal/watch/owner_test.go index 0b9a2ad4..4a5f10e5 100644 --- a/internal/watch/owner_test.go +++ b/internal/watch/owner_test.go @@ -448,6 +448,7 @@ func TestOwner_AStreamOutlivesThePassThatStartedIt(t *testing.T) { map[targetWatchKey]string{}, map[types.CellKey]uint64{}, []types.CellKey{types.CellKeyFor(configmapsGVR, "apps")}, + false, ) require.Len(t, started, 1) streamCtx = started[0].ctx diff --git a/internal/watch/target_watch.go b/internal/watch/target_watch.go index e33e1457..70853d20 100644 --- a/internal/watch/target_watch.go +++ b/internal/watch/target_watch.go @@ -109,6 +109,10 @@ type targetWatchKey struct { type targetWatchStream struct { key targetWatchKey ops OperationSet + // refreshRemote asks the worker to inspect the remote tip before applying this stream's + // replay. It is set on forced GitTarget rechecks, where a human may have fixed or broken the + // folder directly in Git and the local checkout is the stale thing being tested. + refreshRemote bool // revision is the incarnation of this stream's CELL, issued by the render-fidelity gate // when the stream was started. It is captured at start, not read when a replay result is // ready: a cancelled stream that read the current revision on its way out would report its @@ -229,7 +233,7 @@ func (m *Manager) replaceGitTargetWatches( cells := cellsForWatchKeys(keys) m.resetTargetStreamStates(table.GitDest, cells, starting) revisions, fidelityChanged := m.reconcileTargetRenderFidelity(table.GitDest, cells, starting) - started := m.startTargetWatchStreams(ctx, set, keysByCell(keys), streams, specs, revisions, starting) + started := m.startTargetWatchStreams(ctx, set, keysByCell(keys), streams, specs, revisions, starting, force) m.retainTargetRetentionScopes(table.GitDest, streamRevisions(cells, revisions)) if fidelityChanged { @@ -278,6 +282,7 @@ func (m *Manager) startTargetWatchStreams( specs map[targetWatchKey]string, revisions map[types.CellKey]uint64, starting []types.CellKey, + refreshRemote bool, ) []startingTargetWatch { out := make([]startingTargetWatch, 0, len(starting)) for _, cell := range starting { @@ -290,9 +295,10 @@ func (m *Manager) startTargetWatchStreams( out = append(out, startingTargetWatch{ ctx: streamCtx, stream: targetWatchStream{ - key: watchKey, - ops: streams[watchKey], - revision: revisions[cell], + key: watchKey, + ops: streams[watchKey], + refreshRemote: refreshRemote, + revision: revisions[cell], }, }) } @@ -899,7 +905,8 @@ func (m *Manager) enqueueReplayResync( // writes on the strength of a snapshot the new plan never gathered. The gate already // ignores a superseded revision; capturing it at start is what makes it stale. resultCh, enqueued, err := m.EventRouter.enqueueScopedResync( - ctx, gitDest, resyncScopeForWatchKey(stream.key), stream.sourceCell(), desired, revision, false) + ctx, gitDest, resyncScopeForWatchKey(stream.key), stream.sourceCell(), desired, revision, false, + stream.refreshRemote) if err != nil { return err } diff --git a/test/e2e/unsupported_folder_e2e_test.go b/test/e2e/unsupported_folder_e2e_test.go index fa4e3e8d..3e61a0d6 100644 --- a/test/e2e/unsupported_folder_e2e_test.go +++ b/test/e2e/unsupported_folder_e2e_test.go @@ -61,23 +61,35 @@ var _ = Describe("Manager Unsupported Folder Refusal", Label("manager", "unsuppo cleanupNamespace(testNs) }) - It("refuses a hard-Kustomize path without writing", func() { - By("seeding the Git repository with a path that uses an unsupported Kustomize feature") - seedFolder := writeUnsupportedKustomizeFolder(testNs) - DeferCleanup(func() { _ = os.RemoveAll(seedFolder) }) - seedRenderedFolderIntoRepo(repo, testNs, seedFolder, gitPath) - - // Capture the seed commit before the GitTarget exists, so any later operator commit is - // detectable. The operator must add nothing on top of this. - var seedHead string + It("turns red after an unsupported Git-side commit, then recovers when it is undone", func() { + By("creating the live ConfigMap the supported source already contains") + _, _ = kubectlRunInNamespace(testNs, "delete", "configmap", "unsupported-sample", "--ignore-not-found=true") + _, err := kubectlRunInNamespace( + testNs, + "create", + "configmap", + "unsupported-sample", + "--from-literal=hello=world", + ) + Expect(err).NotTo(HaveOccurred(), "failed to create supported live ConfigMap") + DeferCleanup(func() { + _, _ = kubectlRunInNamespace(testNs, "delete", "configmap", "unsupported-sample", "--ignore-not-found=true") + }) + + By("seeding the Git repository with a supported Kustomize path") + supportedFolder := writeSupportedKustomizeFolder(testNs) + DeferCleanup(func() { _ = os.RemoveAll(supportedFolder) }) + seedRenderedFolderIntoRepo(repo, testNs, supportedFolder, gitPath) + + var supportedHead string Eventually(func(g Gomega) { - seedHead = remoteBranchHead(g, repo.CheckoutDir) - g.Expect(seedHead).NotTo(BeEmpty(), "the seed commit must be on the remote") + supportedHead = remoteBranchHead(g, repo.CheckoutDir) + g.Expect(supportedHead).NotTo(BeEmpty(), "the supported seed commit must be on the remote") }, 30*time.Second, 2*time.Second).Should(Succeed()) - By("creating the GitTarget, WatchRule, and ClusterWatchRule pointed at the unsupported path") + By("creating the GitTarget, WatchRule, and ClusterWatchRule pointed at the supported path") createGitTarget(destName, testNs, providerName, gitPath, "main") - err := applyFromTemplate("test/e2e/templates/manager/watchrule-configmap.tmpl", struct { + err = applyFromTemplate("test/e2e/templates/manager/watchrule-configmap.tmpl", struct { Name string Namespace string DestinationName string @@ -85,8 +97,28 @@ var _ = Describe("Manager Unsupported Folder Refusal", Label("manager", "unsuppo Expect(err).NotTo(HaveOccurred(), "failed to apply ConfigMap WatchRule") applyUnsupportedPathClusterWatchRule(clusterRuleName, testNs, destName) - By("the path is refused and the GitTarget is stalled") + By("the GitTarget and its rules start healthy") + waitForGitTargetGitPathAccepted(destName, testNs) + waitForRuleReady("watchrule", ruleName, testNs) + waitForRuleReady("clusterwatchrule", clusterRuleName, "") + + By("pushing an unsupported Kustomize change directly to Git") + unsupportedFolder := writeUnsupportedKustomizeFolder(testNs) + DeferCleanup(func() { _ = os.RemoveAll(unsupportedFolder) }) + seedRenderedFolderIntoRepo(repo, testNs, unsupportedFolder, gitPath) + var unsupportedHead string + Eventually(func(g Gomega) { + unsupportedHead = remoteBranchHead(g, repo.CheckoutDir) + g.Expect(unsupportedHead).NotTo(Equal(supportedHead), "the unsupported commit must reach the remote") + }, 30*time.Second, 2*time.Second).Should(Succeed()) + requestGitTargetReconcile(destName, testNs) + + By("the path is refused, the GitTarget is stalled, and the unsupported file is named") waitForGitTargetGitPathRefused(destName, testNs, "UnsupportedContent") + verifyResourceCondition("gittarget", destName, testNs, + "GitPathAccepted", "False", "UnsupportedContent", "kind=unsupported-kustomize") + verifyResourceCondition("gittarget", destName, testNs, + "GitPathAccepted", "False", "UnsupportedContent", "kustomization.yaml") By("the WatchRule and ClusterWatchRule surface the refused GitTarget dependency") waitForRuleBlockedByGitPath("watchrule", ruleName, testNs, "UnsupportedContent") @@ -95,13 +127,46 @@ var _ = Describe("Manager Unsupported Folder Refusal", Label("manager", "unsuppo By("the operator commits nothing on top of the unsupported path") Consistently(func(g Gomega) { g.Expect(remoteBranchHead(g, repo.CheckoutDir)). - To(Equal(seedHead), "the operator must not commit into a refused path") + To(Equal(unsupportedHead), "the operator must not commit into a refused path") }, 20*time.Second, 4*time.Second).Should(Succeed()) - By("unsupported Git path refused: GitTarget stalled, nothing written") + By("undoing the unsupported Git-side change and requesting a fresh reconcile") + seedRenderedFolderIntoRepo(repo, testNs, supportedFolder, gitPath) + requestGitTargetReconcile(destName, testNs) + + By("the GitTarget and its rules recover") + waitForGitTargetGitPathAccepted(destName, testNs) + waitForRuleReady("watchrule", ruleName, testNs) + waitForRuleReady("clusterwatchrule", clusterRuleName, "") + + By("unsupported Git path refusal recovered after the Git-side fix") }) }) +// writeSupportedKustomizeFolder renders a temp folder holding a kustomization.yaml plus the +// ConfigMap it references, using only the source shape the writer can safely manage. The spec +// creates a matching live ConfigMap before the target starts, so this seed is a no-op mirror rather +// than an unrelated prune setup. +func writeSupportedKustomizeFolder(namespace string) string { + GinkgoHelper() + + dir, err := os.MkdirTemp("", "gitops-reverser-e2e-supported-*") + Expect(err).NotTo(HaveOccurred(), "failed to create supported fixture directory") + + kustomization := "apiVersion: kustomize.config.k8s.io/v1beta1\n" + + "kind: Kustomization\n" + + "namespace: " + namespace + "\n" + + "resources:\n - cm.yaml\n" + configMap := "apiVersion: v1\n" + + "kind: ConfigMap\n" + + "metadata:\n name: unsupported-sample\n namespace: " + namespace + "\n" + + "data:\n hello: world\n" + + Expect(os.WriteFile(filepath.Join(dir, "kustomization.yaml"), []byte(kustomization), 0o600)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(dir, "cm.yaml"), []byte(configMap), 0o600)).To(Succeed()) + return dir +} + // writeUnsupportedKustomizeFolder renders a temp folder holding a kustomization.yaml that uses // an unsupported feature (a patches block) plus the ConfigMap it references. The operator // cannot map a patched render back to editable source documents, so the acceptance gate must @@ -145,6 +210,14 @@ func waitForGitTargetGitPathRefused(name, namespace, expectedReason string) { verifyResourceCondition("gittarget", name, namespace, "Stalled", "True", "", "", "150s") } +func waitForRuleReady(resourceType, name, namespace string) { + GinkgoHelper() + + verifyResourceCondition(resourceType, name, namespace, "GitTargetReady", "True", "", "", "150s") + verifyResourceCondition(resourceType, name, namespace, "Ready", "True", "", "", "150s") + verifyResourceCondition(resourceType, name, namespace, "Stalled", "False", "", "", "150s") +} + func waitForRuleBlockedByGitPath(resourceType, name, namespace, expectedReason string) { GinkgoHelper() @@ -162,6 +235,20 @@ func waitForRuleBlockedByGitPath(resourceType, name, namespace, expectedReason s verifyResourceCondition(resourceType, name, namespace, "Reconciling", "False", expectedReason, "stalled") } +func requestGitTargetReconcile(name, namespace string) { + GinkgoHelper() + + _, err := kubectlRunInNamespace( + namespace, + "annotate", + "gittarget", + name, + fmt.Sprintf("reconcile.configbutler.ai/requestedAt=%d", time.Now().UnixNano()), + "--overwrite", + ) + Expect(err).NotTo(HaveOccurred(), "failed to request GitTarget reconcile") +} + func applyUnsupportedPathClusterWatchRule(name, targetNamespace, targetName string) { GinkgoHelper() From 00749605e1b27c240aff6221fc482d6811d3f24a Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 11 Sep 2026 09:27:24 +0000 Subject: [PATCH 3/4] fix(git): gate credentialed http remotes --- charts/gitops-reverser/README.md | 1 + .../gitops-reverser/templates/deployment.yaml | 3 + charts/gitops-reverser/values.schema.json | 4 ++ charts/gitops-reverser/values.yaml | 3 + cmd/main.go | 13 +++-- cmd/main_audit_server_test.go | 10 ++++ cmd/main_chart_args_test.go | 8 +++ config/deployment.yaml | 2 + docs/configuration.md | 5 ++ docs/design/git-write-preflight.md | 8 ++- internal/controller/gitprovider_controller.go | 5 +- internal/git/branch_worker.go | 17 ++++-- internal/git/credentials.go | 48 ++++++++++++++- internal/git/credentials_test.go | 58 +++++++++++++++++++ internal/git/worker_manager.go | 11 ++++ internal/watch/git_path_acceptance.go | 36 ++++++++---- 16 files changed, 206 insertions(+), 26 deletions(-) diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index 504d7788..2a5de78d 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -176,6 +176,7 @@ nodeSelector: | `servers.audit.timeouts.idle` | Audit-server idle timeout | `60s` | | `servers.audit.tls.secretNameOverride` | Override Secret name for audit TLS cert/key | `-audit-server-cert` | | `controllerManager.additionalSensitiveResources` | Extra Secret-shaped resource types encrypted as `resource` or `group/resource` | `[]` | +| `controllerManager.allowInsecureGitHTTP` | Permit credentials with `http://` GitProvider URLs for trusted in-cluster development Git servers | `false` | | `auditService.type` | Service type for the dedicated audit Service | `NodePort` | | `auditService.nodePort` | Fixed NodePort for the audit Service when `auditService.type=NodePort` | `30444` | | `auditService.clusterIP` | Optional fixed ClusterIP for the dedicated audit Service | `""` | diff --git a/charts/gitops-reverser/templates/deployment.yaml b/charts/gitops-reverser/templates/deployment.yaml index bf29332e..cb50b80b 100644 --- a/charts/gitops-reverser/templates/deployment.yaml +++ b/charts/gitops-reverser/templates/deployment.yaml @@ -102,6 +102,9 @@ spec: {{- with .Values.controllerManager.additionalSensitiveResources }} - {{ printf "--additional-sensitive-resources=%s" (join "," .) | quote }} {{- end }} + {{- if .Values.controllerManager.allowInsecureGitHTTP }} + - --allow-insecure-git-http + {{- end }} {{- if .Values.logging.level }} - --zap-log-level={{ .Values.logging.level }} {{- end }} diff --git a/charts/gitops-reverser/values.schema.json b/charts/gitops-reverser/values.schema.json index 7183d670..59fc1385 100644 --- a/charts/gitops-reverser/values.schema.json +++ b/charts/gitops-reverser/values.schema.json @@ -99,6 +99,10 @@ "type": "array", "items": { "type": "string" }, "description": "Resource or group/resource, e.g. core.cozystack.io/tenantsecrets." + }, + "allowInsecureGitHTTP": { + "type": "boolean", + "description": "Permit credentials with http:// GitProvider URLs for trusted in-cluster development Git servers." } } }, diff --git a/charts/gitops-reverser/values.yaml b/charts/gitops-reverser/values.yaml index 73497591..09514c2a 100644 --- a/charts/gitops-reverser/values.yaml +++ b/charts/gitops-reverser/values.yaml @@ -61,6 +61,9 @@ controllerManager: # Extra Secret-shaped resources that must use the encrypted Git write path. # Entries are resource or group/resource, for example core.cozystack.io/tenantsecrets. additionalSensitiveResources: [] + # Permit credentials with http:// GitProvider URLs. This is only for trusted + # in-cluster development Git servers; production remotes should use HTTPS or SSH. + allowInsecureGitHTTP: false # cert-manager issuer shared by every certificate the chart mints. One self-signed CA # backs them all, so the issuer is genuinely cross-cutting and lives here; each server diff --git a/cmd/main.go b/cmd/main.go index 84e7a846..a61f6cc4 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -135,6 +135,7 @@ func main() { cfg.sensitiveResources, ) workerManager.SetSSHHostKeyConfig(cfg.sshHostKeys) + workerManager.SetCredentialTransportPolicy(cfg.credentialPolicy) fatalIfErr(mgr.Add(workerManager), "unable to add worker manager to manager") // Watch ingestion manager (placeholder, will get EventRouter set later) @@ -315,10 +316,11 @@ func main() { fatalIfErr(mgr.Add(watchMgr), "unable to add watch ingestion manager") if err := (&controller.GitProviderReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - SSHHostKeys: cfg.sshHostKeys, - Recorder: mgr.GetEventRecorderFor("gitprovider"), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + SSHHostKeys: cfg.sshHostKeys, + CredentialPolicy: cfg.credentialPolicy, + Recorder: mgr.GetEventRecorderFor("gitprovider"), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "GitProvider") os.Exit(1) @@ -453,6 +455,7 @@ type appConfig struct { branchBufferMaxBytes int64 sensitiveResources types.SensitiveResourcePolicy sshHostKeys git.SSHHostKeyConfig + credentialPolicy git.CredentialTransportPolicy // sourceClusterQPS / sourceClusterBurst bound the rate at which the operator talks to a // source cluster reached through a GitTarget.spec.kubeConfig. A remote is reached over a // network the in-cluster config is not, so it carries client-side throttling by default. @@ -635,6 +638,8 @@ func parseFlagsWithArgs(fs *flag.FlagSet, args []string) (appConfig, error) { fs.BoolVar(&cfg.sshHostKeys.AllowMissingKnownHosts, "insecure-allow-missing-known-hosts", false, "INSECURE, dev/throwaway clusters only: permit SSH when no host-key source produced any "+ "known_hosts at all. A present-but-unparseable known_hosts is always a hard error.") + fs.BoolVar(&cfg.credentialPolicy.AllowInsecureGitHTTP, "allow-insecure-git-http", false, + "INSECURE, dev/throwaway clusters only: permit credentials with http:// GitProvider URLs.") cfg.zapOpts = zap.Options{ // Production mode defaults to JSON encoding, which is easier for log processors to parse. Development: false, diff --git a/cmd/main_audit_server_test.go b/cmd/main_audit_server_test.go index a102c7f3..197bc3bd 100644 --- a/cmd/main_audit_server_test.go +++ b/cmd/main_audit_server_test.go @@ -52,6 +52,7 @@ func TestParseFlagsWithArgs_Defaults(t *testing.T) { assert.Equal(t, 3*time.Second, cfg.attributionGrace) assert.False(t, cfg.zapOpts.Development) assert.Equal(t, []string{"secrets"}, cfg.sensitiveResources.Entries()) + assert.False(t, cfg.credentialPolicy.AllowInsecureGitHTTP) } func TestParseFlagsWithArgs_AdmissionWebhookValues(t *testing.T) { @@ -85,6 +86,15 @@ func TestParseFlagsWithArgs_AuditUnsecure(t *testing.T) { assert.True(t, cfg.auditInsecure) } +func TestParseFlagsWithArgs_InsecureCredentialedGitHTTP(t *testing.T) { + fs := flag.NewFlagSet("test-insecure-git-http", flag.ContinueOnError) + + cfg, err := parseFlagsWithArgs(fs, []string{"--allow-insecure-git-http"}) + + require.NoError(t, err) + assert.True(t, cfg.credentialPolicy.AllowInsecureGitHTTP) +} + func TestParseFlagsWithArgs_CustomAuditValues(t *testing.T) { fs := flag.NewFlagSet("test-custom", flag.ContinueOnError) args := []string{ diff --git a/cmd/main_chart_args_test.go b/cmd/main_chart_args_test.go index e435bc5f..bf5cee0a 100644 --- a/cmd/main_chart_args_test.go +++ b/cmd/main_chart_args_test.go @@ -68,6 +68,7 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { wantAttribution bool wantAuditTLSCert string wantHTTP2 bool + wantGitHTTP bool }{ "chart defaults": { wantKeyPrefix: "gitops-reverser", @@ -118,6 +119,11 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { wantRedisAddr: "redis.example.com:6379", wantAttribution: true, }, + "credentialed git http opt-in": { + setValues: []string{"controllerManager.allowInsecureGitHTTP=true"}, + wantKeyPrefix: "gitops-reverser", + wantGitHTTP: true, + }, } for name, tc := range tests { @@ -142,6 +148,8 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { } require.Equal(t, tc.wantHTTP2, cfg.enableHTTP2, "servers.enableHTTP2 must reach the binary, and default to off") + require.Equal(t, tc.wantGitHTTP, cfg.credentialPolicy.AllowInsecureGitHTTP, + "controllerManager.allowInsecureGitHTTP must reach the binary") }) } } diff --git a/config/deployment.yaml b/config/deployment.yaml index a03127ea..cd5d9545 100644 --- a/config/deployment.yaml +++ b/config/deployment.yaml @@ -47,6 +47,8 @@ spec: # Dev/e2e only: the in-cluster Gitea SSH host key is not pinned, so permit SSH # when no known_hosts source exists. Never set this in production. - --insecure-allow-missing-known-hosts + # Dev/e2e only: the in-cluster Gitea serves Git over plain HTTP. Never set this in production. + - --allow-insecure-git-http command: - /manager env: diff --git a/docs/configuration.md b/docs/configuration.md index 6c05f789..fd67821d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -121,6 +121,11 @@ re-authoring it. The keys read for each auth method: Auth precedence is SSH key → HTTP basic → bearer token. Client certificates (mTLS), custom CA certificates, and GitHub App credentials are **not supported**. +Credentials are refused with `http://` GitProvider URLs by default. Use HTTPS or SSH for real +repositories. For trusted in-cluster development Git servers that intentionally serve plain HTTP, +start the manager with `--allow-insecure-git-http` or set +`controllerManager.allowInsecureGitHTTP=true` in the Helm chart. + > **A reused Secret needs write access.** Flux and Argo CD only *clone*, so their Git credentials are > often read-only (a read-only deploy key, a read-scoped token). GitOps Reverser **pushes** commits, > so a reused Secret's key or token must have **write** access on the repository; otherwise the diff --git a/docs/design/git-write-preflight.md b/docs/design/git-write-preflight.md index 03894aca..033a0f16 100644 --- a/docs/design/git-write-preflight.md +++ b/docs/design/git-write-preflight.md @@ -143,9 +143,11 @@ stronger promise unbuyable at any acceptable price: - **Time of check to time of use.** Even a fresh answer is stale by the time the object persists. Git can move underneath it, and a direct commit never traverses Kubernetes admission at all. -So preflight answers from a local snapshot, without a network call, and accepts that it will be -wrong in one direction only: it may allow something that later fails to publish. It must never -reject something the writer would in fact have published. +So preflight answers from a local snapshot, without a network call, and accepts that its guarantee +is snapshot-relative. It must not reject a request the writer would publish when evaluating that same +snapshot, but a stale published snapshot may still contain blockers that current inputs have already +removed. In that stale case the gate can falsely reject a request that a fresh writer evaluation +would publish. This shapes the whole design. **A denial is a finding; an allow is only the absence of one.** Say so in user-facing documentation and keep the asymmetry visible in the messages themselves. diff --git a/internal/controller/gitprovider_controller.go b/internal/controller/gitprovider_controller.go index 16116740..44533bbb 100644 --- a/internal/controller/gitprovider_controller.go +++ b/internal/controller/gitprovider_controller.go @@ -41,6 +41,9 @@ type GitProviderReconciler struct { // dev-only missing-key opt-out) for the connectivity check's credential read, so it matches // what the write path uses. SSHHostKeys gitpkg.SSHHostKeyConfig + + // CredentialPolicy controls explicit insecure opt-ins for Git credential transports. + CredentialPolicy gitpkg.CredentialTransportPolicy } // gitProviderLogFirsts keeps startup progress visible without turning every @@ -252,7 +255,7 @@ func (r *GitProviderReconciler) extractCredentials( if err != nil { return nil, err } - return cred.Options(), nil + return gitpkg.CredentialOptionsForProvider(gitProvider, cred, r.CredentialPolicy) } // extractCredential is extractCredentials before the options wrapper. go-git v6 credentials are diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index 09090c9b..93748784 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -92,6 +92,8 @@ type BranchWorker struct { // dev-only missing-key opt-out) for this worker's credential reads. Set by the WorkerManager // before Start, on the same goroutine the event loop reads it from. sshHostKeys SSHHostKeyConfig + // credentialPolicy controls explicit insecure opt-ins for Git credential transports. + credentialPolicy CredentialTransportPolicy // 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 @@ -680,7 +682,7 @@ func (w *BranchWorker) prepareBootstrapRepository( return "", fmt.Errorf("failed to get GitProvider: %w", err) } - auth, err := getAuthFromSecret(ctx, w.Client, provider, w.sshHostKeys) + auth, err := getAuthFromSecret(ctx, w.Client, provider, w.sshHostKeys, w.credentialPolicy) if err != nil { return "", fmt.Errorf("failed to get auth: %w", err) } @@ -1292,7 +1294,7 @@ func (w *BranchWorker) commitPendingWrites(pendingWrites []PendingWrite, hasPend // same cycle build on the local repo and never use auth, so re-reading the // credentials Secret here would be a wasted API GET per commit now that the // Secret cache is disabled. See docs/rbac.md §5. - auth, err := getAuthFromSecret(w.ctx, w.Client, provider, w.sshHostKeys) + auth, err := getAuthFromSecret(w.ctx, w.Client, provider, w.sshHostKeys, w.credentialPolicy) if err != nil { return fmt.Errorf("resolve auth: %w", err) } @@ -1372,7 +1374,7 @@ func (w *BranchWorker) runPushCycle(pendingWrites []PendingWrite) error { return fmt.Errorf("get GitProvider: %w", err) } - auth, err := getAuthFromSecret(w.ctx, w.Client, provider, w.sshHostKeys) + auth, err := getAuthFromSecret(w.ctx, w.Client, provider, w.sshHostKeys, w.credentialPolicy) if err != nil { return fmt.Errorf("resolve auth: %w", err) } @@ -1458,6 +1460,9 @@ func (w *BranchWorker) rebuildPendingWrites( return baseBranch, baseHash, nil } +// refreshRemoteAndRebuildPendingWrites moves the local checkout to the current remote tip, then +// replays retained pending writes on top of it without pushing. It is used by forced GitTarget +// rechecks so the acceptance gate evaluates the newest remote tree instead of a stale local clone. func (w *BranchWorker) refreshRemoteAndRebuildPendingWrites(ctx context.Context, pendingWrites []PendingWrite) error { w.repoMu.Lock() defer w.repoMu.Unlock() @@ -1471,7 +1476,7 @@ func (w *BranchWorker) refreshRemoteAndRebuildPendingWrites(ctx context.Context, return fmt.Errorf("get GitProvider: %w", err) } - auth, err := getAuthFromSecret(ctx, w.Client, provider, w.sshHostKeys) + auth, err := getAuthFromSecret(ctx, w.Client, provider, w.sshHostKeys, w.credentialPolicy) if err != nil { return fmt.Errorf("resolve auth: %w", err) } @@ -1877,7 +1882,7 @@ func (w *BranchWorker) syncWithRemote(ctx context.Context) (*PullReport, error) return nil, fmt.Errorf("failed to get GitProvider: %w", err) } - auth, err := getAuthFromSecret(ctx, w.Client, provider, w.sshHostKeys) + auth, err := getAuthFromSecret(ctx, w.Client, provider, w.sshHostKeys, w.credentialPolicy) if err != nil { return nil, fmt.Errorf("failed to get auth: %w", err) } @@ -1914,7 +1919,7 @@ func (w *BranchWorker) ensureRepositoryInitialized(ctx context.Context) error { return nil } - auth, err := getAuthFromSecret(ctx, w.Client, provider, w.sshHostKeys) + auth, err := getAuthFromSecret(ctx, w.Client, provider, w.sshHostKeys, w.credentialPolicy) if err != nil { return fmt.Errorf("failed to get auth: %w", err) } diff --git a/internal/git/credentials.go b/internal/git/credentials.go index 2f4dbc81..31b315c5 100644 --- a/internal/git/credentials.go +++ b/internal/git/credentials.go @@ -6,6 +6,8 @@ import ( "context" "errors" "fmt" + "net/url" + "strings" gitclient "github.com/go-git/go-git/v6/plumbing/client" gogithttp "github.com/go-git/go-git/v6/plumbing/transport/http" @@ -39,6 +41,13 @@ type SSHHostKeyConfig struct { AllowMissingKnownHosts bool } +// CredentialTransportPolicy controls unsafe Git credential transports. +type CredentialTransportPolicy struct { + // AllowInsecureGitHTTP permits credentials with http:// GitProvider URLs. It is intended for + // trusted in-cluster development Git servers only. + AllowInsecureGitHTTP bool +} + // getAuthFromSecret fetches the credentials Secret named by the GitProvider and resolves it into // go-git transport options. A GitProvider with no secretRef authenticates anonymously (public repos). func getAuthFromSecret( @@ -46,12 +55,47 @@ func getAuthFromSecret( k8sClient client.Client, provider *v1alpha3.GitProvider, hostKeys SSHHostKeyConfig, + transportPolicy CredentialTransportPolicy, ) ([]gitclient.Option, error) { cred, err := credentialFromSecret(ctx, k8sClient, provider, hostKeys) if err != nil { return nil, err } - return cred.Options(), nil + return CredentialOptionsForProvider(provider, cred, transportPolicy) +} + +// CredentialOptionsForProvider applies transport policy to a resolved credential. +func CredentialOptionsForProvider( + provider *v1alpha3.GitProvider, + cred Credential, + policy CredentialTransportPolicy, +) ([]gitclient.Option, error) { + if credentialed(cred) && gitProviderURLScheme(provider) == "http" && !policy.AllowInsecureGitHTTP { + return nil, errors.New("credentialed GitProvider URL must use https or ssh, not http") + } + + options := cred.Options() + if cred.Basic != nil || cred.Bearer != nil { + options = append(options, gitclient.WithRedirectPolicy(gitclient.FollowInitialRedirects)) + } + return options, nil +} + +// credentialed reports whether cred would send authentication material over a Git transport. +func credentialed(cred Credential) bool { + return cred.SSH != nil || cred.Basic != nil || cred.Bearer != nil +} + +// gitProviderURLScheme returns the normalized URL scheme or empty when it cannot be parsed. +func gitProviderURLScheme(provider *v1alpha3.GitProvider) string { + if provider == nil { + return "" + } + u, err := url.Parse(provider.Spec.URL) + if err != nil { + return "" + } + return strings.ToLower(u.Scheme) } // credentialFromSecret is getAuthFromSecret before the options wrapper: it returns the concrete @@ -122,7 +166,7 @@ func AuthFromSecretData( if err != nil { return nil, err } - return cred.Options(), nil + return CredentialOptionsForProvider(provider, cred, CredentialTransportPolicy{}) } // CredentialFromSecretData resolves a credential from an already-fetched Git credentials Secret, diff --git a/internal/git/credentials_test.go b/internal/git/credentials_test.go index 070dc4f5..fadb7b06 100644 --- a/internal/git/credentials_test.go +++ b/internal/git/credentials_test.go @@ -11,6 +11,7 @@ import ( "testing" "github.com/fluxcd/pkg/apis/meta" + gogithttp "github.com/go-git/go-git/v6/plumbing/transport/http" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" gossh "golang.org/x/crypto/ssh" @@ -358,3 +359,60 @@ func TestCredentialFromSecretData_AzureDevOpsPATForm(t *testing.T) { assert.Contains(t, err.Error(), "contains username but no password") }) } + +// TestCredentialOptionsForProvider_HTTPAuthSafety pins the transport boundary around HTTP +// credentials: authenticated network remotes must not use cleartext http://, and authenticated HTTP +// operations keep go-git's initial-only redirect policy explicit. +func TestCredentialOptionsForProvider_HTTPAuthSafety(t *testing.T) { + t.Run("credentialed http URL is rejected", func(t *testing.T) { + provider := &configv1alpha3.GitProvider{ + Spec: configv1alpha3.GitProviderSpec{URL: "http://example.com/repo.git"}, + } + cred := Credential{Basic: &gogithttp.BasicAuth{Username: "git", Password: "token"}} + + options, err := CredentialOptionsForProvider(provider, cred, CredentialTransportPolicy{}) + + require.Error(t, err) + assert.Nil(t, options) + assert.Contains(t, err.Error(), "must use https or ssh") + }) + + t.Run("anonymous http URL stays allowed", func(t *testing.T) { + provider := &configv1alpha3.GitProvider{ + Spec: configv1alpha3.GitProviderSpec{URL: "http://example.com/public.git"}, + } + + options, err := CredentialOptionsForProvider(provider, Credential{}, CredentialTransportPolicy{}) + + require.NoError(t, err) + assert.Nil(t, options) + }) + + t.Run("https HTTP auth pins redirect policy", func(t *testing.T) { + provider := &configv1alpha3.GitProvider{ + Spec: configv1alpha3.GitProviderSpec{URL: "https://example.com/repo.git"}, + } + cred := Credential{Bearer: &gogithttp.TokenAuth{Token: "token"}} + + options, err := CredentialOptionsForProvider(provider, cred, CredentialTransportPolicy{}) + + require.NoError(t, err) + assert.Len(t, options, 2) + }) + + t.Run("explicit opt-in allows credentialed http URL", func(t *testing.T) { + provider := &configv1alpha3.GitProvider{ + Spec: configv1alpha3.GitProviderSpec{URL: "http://git-http.git.svc/repo.git"}, + } + cred := Credential{Basic: &gogithttp.BasicAuth{Username: "git", Password: "token"}} + + options, err := CredentialOptionsForProvider( + provider, + cred, + CredentialTransportPolicy{AllowInsecureGitHTTP: true}, + ) + + require.NoError(t, err) + assert.Len(t, options, 2) + }) +} diff --git a/internal/git/worker_manager.go b/internal/git/worker_manager.go index b94ee421..76b91709 100644 --- a/internal/git/worker_manager.go +++ b/internal/git/worker_manager.go @@ -53,6 +53,9 @@ 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 + // credentialPolicy controls explicit insecure opt-ins for Git credential transports. Set + // once at startup (SetCredentialTransportPolicy) before any worker is created. + credentialPolicy CredentialTransportPolicy // 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 @@ -125,6 +128,13 @@ func (m *WorkerManager) SetSSHHostKeyConfig(cfg SSHHostKeyConfig) { m.sshHostKeys = cfg } +// SetCredentialTransportPolicy injects Git credential transport policy into every worker. +func (m *WorkerManager) SetCredentialTransportPolicy(policy CredentialTransportPolicy) { + m.mu.Lock() + defer m.mu.Unlock() + m.credentialPolicy = policy +} + // 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. @@ -207,6 +217,7 @@ func (m *WorkerManager) EnsureWorker( worker.mapper = m.mapper worker.clusterMapper = m.clusterMapper worker.sshHostKeys = m.sshHostKeys + worker.credentialPolicy = m.credentialPolicy worker.pathRefusal = m.pathRefusal worker.layoutReporter = m.layoutReporter worker.renderFidelityGate = m.renderFidelityGate diff --git a/internal/watch/git_path_acceptance.go b/internal/watch/git_path_acceptance.go index 673859d5..4c07428c 100644 --- a/internal/watch/git_path_acceptance.go +++ b/internal/watch/git_path_acceptance.go @@ -67,24 +67,40 @@ func (m *Manager) MarkTargetGitPathAccepted(gitDest types.ResourceReference) { if prior, had := m.watchPlane().acceptance[gitDest.Key()]; had && prior.Accepted { return } - m.reportGitPathAcceptance(gitDest, GitPathAcceptanceStatus{ - Accepted: true, - Reason: "GitPathAccepted", - Message: "GitTarget path accepted", - }) + m.reportGitPathAcceptance(gitDest, acceptedGitPathStatus()) } // MarkTargetGitPathScopeAccepted clears a scoped refusal only when the successful resync covered // the same cell that produced the refusal. A successful replay of secrets must not hide a still // impossible configmaps path, because the GitTarget status is the operator's only durable clue. func (m *Manager) MarkTargetGitPathScopeAccepted(gitDest types.ResourceReference, cell types.CellKey) { - prior, had := m.watchPlane().acceptance[gitDest.Key()] - if had && !prior.Accepted && cell != (types.CellKey{}) { - if !prior.RefusedCellSet || prior.RefusedCell != cell { - return + status := acceptedGitPathStatus() + changed := m.mutateWatchPlane(func(s *watchPlaneState) bool { + prior, had := s.acceptance[gitDest.Key()] + if had && !prior.Accepted && cell != (types.CellKey{}) { + if !prior.RefusedCellSet || prior.RefusedCell != cell { + return false + } + } + if had && prior.Accepted { + return false } + status.At = metav1.Now() + s.acceptance[gitDest.Key()] = status + return true + }) + if changed { + m.enqueueGitTargetReconcile(gitDest) + } +} + +// acceptedGitPathStatus is the canonical healthy GitPathAccepted condition payload. +func acceptedGitPathStatus() GitPathAcceptanceStatus { + return GitPathAcceptanceStatus{ + Accepted: true, + Reason: "GitPathAccepted", + Message: "GitTarget path accepted", } - m.MarkTargetGitPathAccepted(gitDest) } func (m *Manager) reportGitPathAcceptance(gitDest types.ResourceReference, status GitPathAcceptanceStatus) { From 54236a6680fbd4a35d5038373ad5d14f12b4bd6d Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 11 Sep 2026 09:53:41 +0000 Subject: [PATCH 4/4] test(e2e): allow http git in quickstart lanes --- hack/e2e/install-plain-manifests.sh | 8 ++++++++ test/e2e/quickstart_framework_e2e_test.go | 2 ++ 2 files changed, 10 insertions(+) diff --git a/hack/e2e/install-plain-manifests.sh b/hack/e2e/install-plain-manifests.sh index 9ae0f800..fae8dfa0 100755 --- a/hack/e2e/install-plain-manifests.sh +++ b/hack/e2e/install-plain-manifests.sh @@ -44,6 +44,14 @@ sed -i \ "s|--redis-addr=${DEFAULT_AUDIT_REDIS_ADDR}|--redis-addr=${E2E_AUDIT_REDIS_ADDR}|" \ "${tmpdir}/install.yaml" +# The public install bundle keeps credentialed HTTP Git disabled by default; the e2e fixtures +# intentionally use an in-cluster HTTP Gitea service. +if ! grep -q -- "--allow-insecure-git-http" "${tmpdir}/install.yaml"; then + perl -0pi -e \ + 's/(\n\s+- --redis-insecure\n)/$1 - --allow-insecure-git-http\n/' \ + "${tmpdir}/install.yaml" +fi + # Inject fixed ClusterIP for the controller Service perl -0pi -e \ "s/type: ClusterIP\n/type: ClusterIP\n clusterIP: ${E2E_CONTROLLER_SERVICE_CLUSTER_IP}\n/" \ diff --git a/test/e2e/quickstart_framework_e2e_test.go b/test/e2e/quickstart_framework_e2e_test.go index e85e524c..ff27e1b0 100644 --- a/test/e2e/quickstart_framework_e2e_test.go +++ b/test/e2e/quickstart_framework_e2e_test.go @@ -149,6 +149,8 @@ func (r *quickstartFrameworkRun) helmInstallReadmeQuickstart() { "--reuse-values", // The README's no-Redis default: empty addr runs configured-author. "--set", "queue.redis.addr=", + // The e2e repo is intentionally served by in-cluster HTTP Gitea. + "--set", "controllerManager.allowInsecureGitHTTP=true", "--set", "quickstart.enabled=true", "--set", fmt.Sprintf("quickstart.namespace=%s", r.namespace), // We created the namespace + git-creds above (README step 3, before install).