diff --git a/docs/INDEX.md b/docs/INDEX.md index f48d9695..415a9d60 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -134,39 +134,41 @@ 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 (27 pages) +### Open (29 pages) | Doc | Open question | |---|---| -| [`branch-worker-event-model.md`](design/branch-worker-event-model.md) | **proposal; its in-process fixes are built in #412 and #413.** Publication retry, Git operation deadlines, intake pause and resume, and publication status are done. Worker ownership, FIFO inputs, save and deadline semantics, and the transition boundary for later durability, which is next. Journal storage and rollout belong to the HA plan. | +| [`branch-worker-event-model.md`](design/branch-worker-event-model.md) | **proposal; its in-process fixes are built in #412 and #413.** Publication retry, Git operation deadlines, intake pause and resume, and publication status are done. Open: worker ownership, FIFO inputs, save and deadline semantics, and the transition boundary for later durability, which is deferred: restart recovery stays in memory for now (decided 2026-10-28). Journal storage and rollout belong to the HA plan. | | [`gittarget-branch-worker-pending-writes.md`](design/gittarget-branch-worker-pending-writes.md) | **built in #413.** Ordered pending writes, one materializer and one retry schedule; a safe admission boundary for the watch; Git call deadlines; intake pause and resume; publication status. Holds the recovery contract for an outage, replay limits, and the acceptance scenarios. | -| [`event-pipeline-overview.md`](design/event-pipeline-overview.md) | **snapshot, 2026-10-04.** Diagrams of the pipeline as built: watch streams and their replay/resume rules, the three meanings of "replay", the branch worker's inputs, the pending writes' lifecycle, retry and intake, a save end to end, what is an event today, and an honest list of what is still missing. | -| [`gittarget-parent-branch.md`](design/gittarget-parent-branch.md) | **merged in #407.** `spec.parentBranch`: a target stays on standby until a write needs a branch, then creates its write branch from the parent's tip as checked at publication. The contract as built, the defect it started from, prior art and naming. Open: parent per `GitProvider.status.branches` entry. | +| [`gittarget-parent-branch.md`](design/gittarget-parent-branch.md) | **merged in #407; the field moved to `GitBranch` in #429.** `GitBranch.spec.parentBranch`: a branch stays on standby until a write needs it, then is created from the parent's tip as checked at publication. The contract as built, the defect it started from, prior art and naming. Open: reporting the parent per branch in status. | | [`gittarget-parent-hardening.md`](design/gittarget-parent-hardening.md) | **done in #407.** Parent changes on a live worker, an empty repository gaining a ref, an unresolved default branch, recovery that cannot strand work, and the refused upload, without adding a pull to the fast path; plus the two recovery fixes from the review at `93120caf`. Records the merge boundary and the next separate PRs. | -| [`gittarget-parent-empty-repository.md`](design/gittarget-parent-empty-repository.md) | **deferred.** An empty repository's default branch: the server reports it over protocol v2 but go-git discards it (upstream fix first); then a separate policy decision on which branch the first commit may create. Assuming `main` was considered and rejected. | +| [`gittarget-parent-empty-repository.md`](design/gittarget-parent-empty-repository.md) | **deferred.** An empty repository's default branch: the server reports it over protocol v2, and go-git discarded it at alpha.5. The pinned beta.1 keeps it at the transport layer (`RemoteRefs.Unborn`), so the go-git half may be unblocked (see [TODO](TODO.md)); which branch the first commit may create stays a separate policy decision. Assuming `main` was considered and rejected. | | [`gittarget-parent-observation.md`](design/gittarget-parent-observation.md) | **deferred.** Keep the parent visible after the write branch exists: availability first, ancestry only after measuring how often a shallow checkout can answer it. Informational; authorizes no branch mutation. | | [`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 | +| [`open-asks-priority.md`](design/open-asks-priority.md) | **stale; [`TODO.md`](TODO.md) is the work queue.** Merged 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. B2, B4, F6 and F10 have shipped, and F9 is probably moot since #333 deleted the `ClusterWatchRule` `scope` field. Still the record of one design call: **Option C sibling inference was deleted** rather than given an off-switch, and "what the deletion taught" records what building it found | | [`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 | | [`target-watch-plan.md`](design/target-watch-plan.md) | **partly built.** The diff is built and applied; removal semantics are not. The companion to [`watch-manager-ownership.md`](design/watch-manager-ownership.md): the ownership page says WHO applies a plan, this one says WHAT a plan is and what changing it may touch. A resource collection — group, resource, namespace, deliberately no served version — is the one identity the watch stream, the render-fidelity scope and the mark-and-sweep boundary all agree on, because a key that does not round-trip to the scope it sweeps under is the class of error that deletes user data. The plan is diffed into `keep`/`start`/`restart`/`stop` and applied per collection, so adding one WatchRule stops re-initializing every unrelated collection into a queue shared with other tenants; a `restart` is a served-version change, which is why the version is spec DATA rather than identity. Readiness and the fidelity revision are per scope, so a KEPT collection holds the result its own initialization produced rather than being asked to prove itself again over an unrelated edit. `stop` never touches files — removal is a Git-side sweep under the target's existing `spec.prune.mode`, not a watch-layer delete. "Cut at the producer" is the accepted consequence: nothing fences the queue, so a deselected collection may leave a short tail of writes, bounded by the queue and converged afterwards. Still open: the `stop` classification wants a settled `TypeRemoved` from `typeset` (see TODO), and removal on INTENT is undecided. | -| [`watches-labels-simplification.md`](design/watches-labels-simplification.md) | **step 2, implemented.** After the collection rename and operation-filter removal, native per-rule `objectSelector` (#146). Kubernetes is authoritative: `Always` compares selected identities with all managed Git identities in the type/namespace scope, without comparing Git labels. Full support includes supported Kustomize layouts from the first release, plus label-exit attribution (now selector-agnostic, see the attribution spec) and ordered producer replacement. | -| [`label-selection-follow-ups.md`](design/label-selection-follow-ups.md) | **design, open.** What the label-selection review rounds left open, with the evidence for each: real deletions in a selected collection wait the full 3s attribution grace (the watch cannot tell them from a label exit, measured), the resync fence that stops overlapping collections from coalescing (kept on purpose), same-second overlap ties, and one store rebuild per object coming back. Also lists what was fixed on the branch | -| [`attribution-removal-wait-options.md`](design/attribution-removal-wait-options.md) | a removal now waits for evidence about the DELETION rather than accepting the object's last write, which stopped it naming whoever last edited the object as the author of a deletion they did not perform. Enumerates the eight situations a resolution can be in and shows the cost is concentrated in exactly one: a removal for which no delete fact will ever arrive (a graceful pod delete, a status-only removal, a type the audit policy skips) spends the whole grace to return the answer it had at t=0, measured at ~3.1s against ~70ms when evidence is present. Prices five options against that, and recommends a per-route watermark — stop waiting once the fact stream has demonstrably moved past this event — over a second timeout flag whose right value lives in the API server's config rather than ours. Open: the decision, and how common the case is outside the e2e suite | -| [`concurrent-author-windows.md`](design/concurrent-author-windows.md) | **proposal, not built.** A branch worker holds one commit window at a time, so a second person's edit closes the first person's window and splits their change into two commits. Proposes a window per GitTarget and author, open side by side only when the writes' conflict footprints (the Git files they may change, kept separate from the per-object coalescing key) are proven disjoint; anything unproven stays serial. Every close trigger keeps today's exceptions (a push hint and an apply flush leave a save's window alone), windows close in opening order, and commit order becomes close order. Tested against an independent reference model on every commit, and footprint proofs tied to a tree revision and recomputed when it moves, and shipped in five stages behind a startup flag. Follow-ups in `window-field-ownership-and-apply-hold.md` | -| [`window-field-ownership-and-apply-hold.md`](design/window-field-ownership-and-apply-hold.md) | **proposal, not built.** Two separate follow-ups to concurrent author windows. Part 1: a window owns the fields it changed, the way an SSA field manager does, so two authors on one object split by field. It reopens the decided `manifestedit-field-ownership-spike.md` and argues the use case it asks for; every field-owned commit writes only owned fields, and convergence is a separate, explicitly attributed repair that compares Git with a desired view (withheld applies undone), never the live object; experimental until that is accepted. Part 2: an incoming Git commit that is provably disjoint from the open windows (rendered, against the last applied revision) holds the apply instead of closing them, with an absolute deadline of its own; it needs a fetch on a busy branch and brings back a bounded form of the wait S1 removed. The next candidate if measurements justify it. Lists each part's potential problems | +| [`label-selection-follow-ups.md`](design/label-selection-follow-ups.md) | **design, mostly resolved.** What the label-selection review rounds left open, with the evidence for each. Items 1 and 2 are resolved; left are same-second overlap ties (the lower name wins, kept on purpose) and one store rebuild per object coming back | +| [`attribution-removal-wait-options.md`](design/attribution-removal-wait-options.md) | **design, open.** A removal waits for evidence about the DELETION rather than accepting the object's last write, which stopped it naming whoever last edited the object as the author of a deletion they did not perform. Enumerates the eight situations a resolution can be in and shows the cost is concentrated in one: a removal for which no delete fact will ever arrive (a graceful pod delete, a status-only removal, a type the audit policy skips) spends the whole grace to return the answer it had at t=0, measured at ~3.1s against ~70ms when evidence is present. Prices the options against that. Open: build option F, the per-(route, type) breaker, then measure before option C | +| [`concurrent-author-windows.md`](design/concurrent-author-windows.md) | **built** in #463, #467, #469, #470 and #476, with e2e in #478; no startup flag. A window per GitTarget and author, open side by side only when the writes' conflict footprints (the Git files they may change, kept separate from the per-object coalescing key) are proven disjoint against a tree revision and proven again when it moves; anything unproven stays serial. Every close trigger keeps its exceptions (a push hint and an apply flush leave a save's window alone), and commit order is close order. Checked against an independent reference model. Follow-ups in `window-field-ownership-and-apply-hold.md` | +| [`window-field-ownership-and-apply-hold.md`](design/window-field-ownership-and-apply-hold.md) | **proposal, not built.** Two separate follow-ups to concurrent author windows. Part 1: a window owns the fields it changed, the way an SSA field manager does, so two authors on one object split by field. It reopens the decided `manifestedit-field-ownership-spike.md` and argues the use case it asks for; every field-owned commit writes only owned fields, and convergence is a separate, explicitly attributed repair that compares Git with a desired view (withheld applies undone), never the live object. The withheld-field record that view needs was built in #466. Part 2: an incoming Git commit that is provably disjoint from the open windows holds the apply instead of closing them, with an absolute deadline of its own. The next candidate if measurements justify it | | [`commit-timing-surface.md`](design/commit-timing-surface.md) | **built.** One configuration surface for commit windows and saves, an in-place `v1alpha3` break with one tested upgrade procedure: only writes open a window; a `window` block (`idleTimeout`, `maxDuration`, target default `1m`) on both kinds, which a `CommitRequest` replaces whole for the window it attaches to; `attach: CurrentOrNext \| Next` with `attachTimeout`; opt-in `whenNothingToCommit: CommitEmpty` that keeps the cause in the reason. Precise clock and zero rules. Supersedes the save-wait timer model. Open: the target's `maxDuration` value | | [`metrics-observability-plan.md`](design/metrics-observability-plan.md) | **built.** the canonical metrics document, rewritten 2026-09 and shipped in the same wave. Reads the product as two ingestion paths — object state through watch, authorship through audit — joining at the resolver, and gives each stage one counter with a bounded `outcome` so the flow and its loss paths are drawable. Records what three review rounds corrected, including two reversals worth not re-litigating: the fact-loss counters must NOT be merged (an eviction counts a fact, a trim gap an occurrence, a decode error a whole batch), and a cumulative counter cannot replace a current count, which is why `status.streams` and `status.retainedDocuments` stayed. The instrument inventory lives in [`interpreting-metrics.md`](interpreting-metrics.md), not here. Open: the Grafana JSON and alert rules | -| [`reconcile-triggering.md`](design/reconcile-triggering.md) | which controllers still fail to wake up | -| [`applier-echoes.md`](design/applier-echoes.md) | **built 2026-10-20, released as one change.** The applier's own write, back through the watch, is mirrored and re-planned onto a newer tip on replay, reverting a Git-side push. Two ideas close every way a stale picture can reach a commit: a write that changes nothing does not exist, and a pending write carries the actions decided on its tree, not a picture. The applier's write in the apply window is not mirrored (F1); a write is upserts and deletes decided once, so a replay never sweeps again and a save keeps its objects (F2); a resync never decides from a snapshot collected while Git was ahead or across an apply, by an apply epoch (F3); another applier's object is not mirrored into a coordinated folder (F4); flush, then apply (S1); `CatchUp` only to adopt API creates and to converge, for Flux and Argo CD alike, and no apply before every initial snapshot is published (S2). Carries the collisions left open, the promises, the code plan, thirteen e2e scenarios, and the twelve decisions taken while building (among them: siblings do not count for F3, a 5-minute hold, initial snapshots exempt from F3, the prefetch removed, no heal resync, and the flush covering the sibling workers) | +| [`applier-echoes.md`](design/applier-echoes.md) | **built 2026-10-20, released as one change.** The applier's own write, back through the watch, was mirrored and re-planned onto a newer tip on replay, reverting a Git-side push. Two ideas close every way a stale picture can reach a commit: a write that changes nothing does not exist, and a pending write carries the actions decided on its tree, not a picture. The applier's write in the apply window is not mirrored (F1); a write is upserts and deletes decided once, so a replay never sweeps again and a save keeps its objects (F2); a resync never decides from a snapshot collected while Git was ahead or across an apply, by an apply epoch (F3); another applier's object is not mirrored into a coordinated folder (F4); flush, then apply (S1); `CatchUp` only to adopt API creates and to converge, and no apply before every initial snapshot is published (S2). Carries the collisions left open, the promises, the code plan, the e2e scenarios, and the decisions taken while building; the ones about sibling workers were superseded by one worker per branch (#428) | | [`apply-trigger.md`](design/apply-trigger.md) | **built for Flux and Argo CD; a webhook applier is the aim.** Why GitOps Reverser decides when Git is applied, and why the applier never applies on its own: a Flux `Kustomization` is held suspended by its `FluxClaim` and resumed for each apply, an Argo CD `Application`'s automated sync is held off by its `ArgoCDClaim`. Carries what building it taught (timers are triggers, a long interval is not exclusive control, a decision must hold at the write, a retry is a decision so only the worker makes one, an apply is an operation whose end is observed), the kustomize-controller v1.9.6 facts the suspended lifecycle rests on, its operation (resume token, outcome, the suspend after the apply), who owns `spec.suspend` (an applier claim acts only while an eligible `GitTarget` references it, one applier claim per applier), restart recovery, shared sources, the measured ~50 ms cost, flush then apply, `CatchUp` as adopt and converge for Flux and Argo CD alike, the apply window with the applier's own writes kept out of Git, resyncs held off while Git is ahead, another applier's objects kept out of a coordinated folder, and the decisions of 2026-10-17, 2026-10-19, 2026-10-20 and 2026-10-23 (the rule, the `allowCrossNamespace.appliers` grant, `InControl`, the reversals of 2026-10-20, Argo CD's automated sync held off like a suspend) | -| [`push-cooldown.md`](design/push-cooldown.md) | **partly built.** Whether the fixed `5s` push cooldown still earns its place now that the commit window exists. Ordinary timer closures already space one author writing one target, but identity changes, saves with shorter timers and buffer-limit closures commit sooner, and serialization does not batch queued commits, so the cooldown is what batches them. Option C, the failure backoff, is built in #412 and shares the push timer; removing the success wait would now remove only `lastPushAt` and its branch, so that decision waits on the event-loop measurements in §9 | -| [`multi-source-audit-ingress-hardening.md`](design/multi-source-audit-ingress-hardening.md) | how independent sources authenticate to a named audit route, when annotation routing is trustworthy, and how multi-provider ingestion remains fair | +| [`push-cooldown.md`](design/push-cooldown.md) | **partly built.** Whether the fixed `5s` push cooldown still earns its place now that commit windows exist. One window closing on its own timer is already spaced past it; the cooldown batches commits that land close together anyway: windows closed early (another `GitTarget` on the branch, an overlapping or unproven footprint, the buffer limit, `idleTimeout: 0s`) and side-by-side windows closing within seconds of each other. The failure backoff is built (#412, #413); dropping the success wait waits on the event-loop measurements in §9 | +| [`multi-source-audit-ingress-hardening.md`](design/multi-source-audit-ingress-hardening.md) | **design, open.** How independent sources authenticate to a named audit route, when annotation routing is trustworthy, and how multi-provider ingestion remains fair | | [`release-image-reuse-plan.md`](design/release-image-reuse-plan.md) | PRs 2–5 unstarted | | [`e2e-coverage-gaps-and-improvements-plan.md`](design/e2e-coverage-gaps-and-improvements-plan.md) | tests A/B/C still proposals | | [`sensitive-resource-diagnostics-follow-up.md`](design/sensitive-resource-diagnostics-follow-up.md) | deferred diagnostics | -| [`e2e-git-server-choice.md`](design/e2e-git-server-choice.md) | stay on Gitea or move to Forgejo — the `_csrf` pin is fixable in place on both, so the migration is now a preference call, not a fix; also why we adopt no SDK either way | +| [`applier-claim-held-applier-watch.md`](design/applier-claim-held-applier-watch.md) | **decided, follow-up not built.** Whether an `ArgoCDClaim` should also switch `selfHeal` off (no, shipped with #437), and the wanted follow-up: watch the held applier so a switch-on or a `flux resume` is undone at once rather than within `heldClaimSteadyInterval` | +| [`live-edit-dropped-while-render-fidelity-pending.md`](design/live-edit-dropped-while-render-fidelity-pending.md) | **built.** A live edit dropped while a GitTarget's watches start: Fix 1 and the drop counter (#420), and Fix 2a, holding the event until the render-fidelity gate opens (`internal/git/render_fidelity_hold.go`). The sections below the status are the argument as it was | +| [`watch-plane-status-convergence-failures.md`](design/watch-plane-status-convergence-failures.md) | **investigation log; Failure A solved, Failure B open.** Two reproducible status failures from the watch-plane rework (#315), with facts separated from hypotheses so a fresh context can continue | +| [`git-write-preflight.md`](design/git-write-preflight.md) | **proposal, not built; status undecided.** An opt-in, fail-open admission check that warns about an edit no `GitTarget` could publish. [`TODO.md`](TODO.md) calls it superseded by the red-status plan, while [`support-boundary/README.md`](design/support-boundary/README.md) still treats it as the plan | +| [`e2e-suite-trim.md`](design/e2e-suite-trim.md) | **open plan.** What is left to trim from the e2e suite: run image-refresh only when its inputs change, and the detail gaps and redundancy tables | +| [`mutation-capture-lab-own-cluster.md`](design/mutation-capture-lab-own-cluster.md) | **built (#458).** The mutation-capture lab on a cluster of its own, its requirements, and where the build departs from the design | -### Built, and kept here anyway (7 pages) +### Built, and kept here anyway (6 pages) These have shipped. They stay in `design/` under the exception above, because Go source cites them by path as the rationale for what the code does, and `finished/` declares @@ -175,12 +177,11 @@ itself non-binding. Read them as history that the code still points at. | Doc | Open question | |---|---| | [`push-notification-and-reconcile-trigger.md`](design/push-notification-and-reconcile-trigger.md) | **built, and superseded** by [`apply-trigger.md`](design/apply-trigger.md): the record of how the applier claim was decided. Kept here because its Part 1, how the branch worker reads the remote (the pre-push fetch, the push advertisement, base trust), is cited from the Git code by section number | -| [`vocabulary-cleanup.md`](design/vocabulary-cleanup.md) | **built.** The breaking release that made the API obey [`definitions.md`](definitions.md): condition reasons that restated their type are `Succeeded`, `GitRepoConfigNotFound` is `GitProviderNotFound`, printer columns are named for what they read and the dependency projections are gone, a bare commit hash is `commit` in every status field, and the source-cluster provider's throttles are `spec.client`. Kept here because `definitions.md` cites it as the record of which audit findings the existing rulebooks already settled: the `insecure` flag split, bare-noun booleans, `--redis-addr`, and the lowercase `sops` enum | | [`created-root-namespace.md`](design/created-root-namespace.md) | **built.** One question with five answers: what namespace a `kustomization.yaml` the operator CREATES should carry. Decided **B, never write one** — `spec.serializeNamespace: false` means the artifact does not encode its deployment namespace, and adding a root must not quietly change that contract; the namespace comes from the documents when the field is unset or `true`, and from the installer (Flux `targetNamespace`, Argo `destination.namespace`) when it is `false`. Records the three facts an earlier draft got wrong (a namespace-less root is ordinary, both installers supply one, and what refused it was our own fidelity gate rather than kustomize), and carries the scoped fidelity rule that follows: the namespace is ignored in the render comparison ONLY when the governing root sets none, so a root that declares `namespace: shop` still rejects a live `billing` object. Shipped in #328, which is also why [`kustomization_bootstrap.go`](../internal/git/kustomization_bootstrap.go) cites the page by path. Also the sibling call: under `useKustomize` a placement no `resources:` list would name is refused rather than committed unrendered | | [`watch-manager-ownership.md`](design/watch-manager-ownership.md) | A rule edit used to be applied inline by the controller worker that observed it, and it re-planned EVERY GitTarget rather than the one the rule names: 1256 plan reconciles across 28 targets in one e2e run, peaking at 78 in a second, behind two network calls, on a shared worker pool. The watch manager had no owner, so eleven mutexes stood in for one. Now controllers post a trigger naming a GitTarget and return, one loop owns the plan and paces itself, and repeated triggers for one target collapse into a single pass. The debounce is framed around how the config is actually edited: a GitTarget and its rules are one piece of configuration applied together, so a per-target ROLLING SILENCE window of 2s (max wait ~10s) turns a five-object `kubectl apply` into one pass, the same mechanism `DefaultCommitWindow` already uses one layer down on the write path. That reverses an earlier revision's "never debounce the first declaration": declaring a GitTarget the instant it lands means declaring it with no rules yet, which manufactures a transient EMPTY plan on every cold start, and an empty plan is what vacuously cleared a write divergence in the fidelity gate. States the contract as "one settled configuration adjustment, not one function invocation": the window is a heuristic and never a correctness boundary (Kubernetes has no apply-complete event), a per-target DIRTY SEQUENCE means a change arriving mid-pass is never lost, and the pass reads a coherent rule-store snapshot rather than the rule that triggered it. Carries the deletion inventory, because the point is that the system got smaller: four trigger mechanisms collapsed to one (`signalCatalogRefresh` and `catalogRefreshCh` are gone), `refreshRunningTargetWatches` and its running-set filter are gone (that filter is why a target whose first declare never completed was never picked up again), and six mutexes went for stated reasons, with `RenderFidelityGate.mu` and the two event-channel locks kept and justified. The four steps shipped: the local-cluster discovery call is bounded (a real defect — the legacy non-context `ServerGroupsAndResources()` ran with no deadline at all), the owner loop carries the debounce, dirty sequence, per-target deadline and 2s/5s/10s/30s/1m backoff, the plan now carries NO lock while the projection is a published snapshot, and catalog invalidation is scoped by diffing each target's rendered plan across the re-projection. "What shipped" records where the implementation departed from the page, including the one bug only e2e caught: streams were parented to the PASS context, which a deadline cancels the moment the pass returns, so every stream died the instant its plan was applied — and it reads like health, because the plan logs `start:1`, every later pass reports `keep:1` and never restarts it, and nothing logs an error while readiness sits at `Replaying` and every WatchRule sits `Ready=False`. A stream's parent is the manager's lifetime; the pass deadline bounds the pass. A second e2e catch is a BEHAVIORAL consequence worth knowing: toggling a rule off and on inside the settle window is no longer a replay — it used to tear the stream down and re-establish it because each apply replanned synchronously, and it is now one pass over a plan that never changed. Correct (a net-zero change is no change; widening `prune.mode` remains the supported force) but a real difference in what an operator gesture does. Both specs that broke were also gating on the wrong thing: asking a GitTarget "are all your streams running" about a change to ONE rule, which a target that is already mirroring answers True to before that rule has been planned, and which a different controller publishes than the one that compiled the rule — so the rule's OWN StreamsRunning is the gate, and `waitForWatchRuleStreamsRunning` existed unused for exactly this. Departures: reports became a published snapshot rather than a second channel; ISOLATION came from taking the I/O off the loop rather than from the deadline (a pass never dials, the shared refresh runs on its own goroutine, and the deadline is the backstop it should have been — a first cut that kept two network calls on the loop had one unreachable cluster holding every healthy target, which is the same availability failure relocated); DELETION names an incarnation resolved when it is queued, because both production callers react to a NotFound and carry no UID, so a UID-less delete matched everything and could tear down the successor of a same-name recreate; and persistent failure surfaces as `WatchPlanFailing`, where pending means "no pass has ever landed", not "dirty right now". Step 3's type-to-target index did not earn its staleness. Still open: whether the settle window ever needs to be configurable | -| [`docs-linting.md`](design/docs-linting.md) | how to mechanize [`style-guide.md`](style-guide.md) with markdownlint-cli2 and Vale. Both are wired into `task lint`, gated on the files [`.docs-lint-scope`](../.docs-lint-scope) lists rather than the whole tree: 102 of 174 files fail markdownlint and 148 of 174 fail Vale, so the two backlogs need different gates. Open: how the scope list grows to cover the tree, the `MD013` limit, and whether `AGENTS.md` and the chart READMEs are in scope | +| [`docs-linting.md`](design/docs-linting.md) | **built.** How to mechanize [`style-guide.md`](style-guide.md) with markdownlint-cli2 and Vale. Both run in `task lint`: markdownlint over every tracked file, Vale only over the files [`.docs-lint-scope`](../.docs-lint-scope) lists. Open: how the Vale scope grows to cover the tree, the `MD013` limit, and whether `AGENTS.md` and the chart READMEs are in scope | | [`azure-devops-multi-ack.md`](design/azure-devops-multi-ack.md) | go-git v6 was the answer: why Azure DevOps rejects our fetches, and what to do instead of PR [#292](https://github.com/ConfigButler/gitops-reverser/pull/292)'s bundled `git` binary. The capability filter fails in two independent halves: advertising `multi_ack` is a four-line change, but v5 then cannot parse the multi-ACK **response**, which only a fetch with `have` lines provokes. That is why **Flux ships ADO support on v5 with no git binary — it never fetches**, only `CloneContext`, so it never enters the path v5 cannot serve; our persistent-clone-plus-incremental-fetch design is the opposite, which makes the trim alone insufficient for us. **go-git v6 already implements `multi_ack`** (PR #1204, in every v6 tag; upstream then deleted their ADO example saying it "works out of the box"), and its churn in the packages we import runs 96 → 39 → **1** → **9** removals per alpha, so it is one settled breaking wave rather than a moving target; the migration is four known API removals over two rewritten files, `transport.AuthMethod` being the invasive one. Prices PR #292 as measured rather than argued: the image goes **217 MB → 940 MB**, of which 723 MB is a `cp -rL` that dereferences 165 hardlinks to one binary (a one-character fix), arm64 is unaffected and native, but **Trivy reports zero findings on both images** while the new one carries git 2.54.0, OpenSSH 10.3p1 and OpenSSL 3.5.7 as loose files no package database describes — so the CRITICAL gate is blind to a third of the runtime. Also catches an unflagged non-ADO regression (`Depth: 1` dropped, so every provider full-fetches) and 10% patch coverage on an untestable path. The unlock is that **canonical `git upload-pack` advertises `multi_ack`** (verified), so the Gitea already in the e2e lab plus a 400-injecting proxy is a faithful ADO simulator — no tenant needed, and the only way any option becomes CI-testable. Four options priced, and Option A (v6) is the one shipped. Carries a measured **capability matrix** over our three network calls with two diagrams, which narrows the blast radius to **one call, `repo.Fetch`**: `receive-pack` never advertises `multi_ack` (measured), so **the atomic push is out of scope for every option** — its safety rests on the same-session advertisement plus the server-side `Old`/`New` compare-and-swap in `packp.Command`, neither of which touches `upload-pack`, and we already push from a shallow store today. v6 keeps that pattern 1:1 (`Handshake` → `GetRemoteRefs`/`Push`, same `[]*packp.Command`), which is an argument *for* migrating. Records what the migration actually cost, including the four v6 behaviour changes it surfaced — two of them settings v6 reads from the environment and fails closed on, invisible to unit tests | -| [`source-scope-simplification.md`](design/source-scope-simplification.md) | except the additive `SelfSubjectAccessReview` pass it explicitly leaves for later. Declines Flux-style impersonation, deletes `GitTarget.spec.allowedSourceNamespaces` and its selector machinery (**4,569 lines**, and the only cross-cluster read in the authorization path), renames two source-cluster provider fields, and redefines `sourceNamespace: "*"` as one cluster-wide list and watch. The argument is an API reading, not a security one: the chain from a Git folder back to the object that fills it never leaves one namespace, so ordinary RBAC on `watchrules` already answers it. **Keeps `allowedNamespaces`** (renamed `accessFrom`), reversing an earlier draft — source RBAC bounds what a credential may READ, never which tenant may WIELD it. Prices what is lost: source-side label selectors, which have no replacement. Archaeology in [`facts/kubernetes-impersonation-and-flux-identity.md`](facts/kubernetes-impersonation-and-flux-identity.md) | +| [`source-scope-simplification.md`](design/source-scope-simplification.md) | **built**, except the additive `SelfSubjectAccessReview` pass it leaves for later. Declines Flux-style impersonation, deletes `GitTarget.spec.allowedSourceNamespaces` and its selector machinery (**4,569 lines**, and the only cross-cluster read in the authorization path), renames two source-cluster provider fields, and redefines `sourceNamespace: "*"` as one cluster-wide list and watch. The argument is an API reading, not a security one: the chain from a Git folder back to the object that fills it never leaves one namespace, so ordinary RBAC on `watchrules` already answers it. Its kept `allowedNamespaces` (renamed `accessFrom`) was since replaced by the explicit grants of #441. Prices what is lost: source-side label selectors, which have no replacement. Archaeology in [`facts/kubernetes-impersonation-and-flux-identity.md`](facts/kubernetes-impersonation-and-flux-identity.md) | ## The layout topic — [`layout/`](layout/README.md) @@ -214,12 +215,8 @@ event model owns execution semantics and the Git operation-deadline fix; its pub shipped in #412. [`least-privilege-remaining-work.md`](future/least-privilege-remaining-work.md) has three open RBAC items. -[`config-surface-for-a-structured-repository.md`](future/config-surface-for-a-structured-repository.md) -reviews the configuration docs and argues the API never caught up with what the -folder analysis learned — a look-before-you-write mode, a `status.layout` -projection, an inference switch, and moving `commitWindow` onto the GitTarget. [`direction-and-configuration-surface.md`](future/direction-and-configuration-surface.md) -is the strategy review on top of it: the config-as-data direction as the headline with +is the strategy review: the config-as-data direction as the headline with brownfield mirroring as the on-ramp, a decided Helm standpoint (declaration editing plus the values projection; helm-light inversion parked behind entry criteria), and worked examples of where the configuration surface should go. @@ -228,7 +225,16 @@ deferred investigation into two independent edits to different fields of one object: what a common baseline would have to be, five evidence sources weighed for supplying or standing in for one, and the counterexample that limits each. It opens by naming what API-first already settles, and records why a partial merge would be worse than the rule we have. -Four more ideas sit beside them. +[`gitops-api-open-asks.md`](future/gitops-api-open-asks.md) is the gitops-api team's own letter of +asks, kept as their input rather than our queue. Eight more ideas sit beside them: +[applier claim ownership](future/idea-applier-claim-ownership.md), +[claiming an `ApplicationSet`](future/idea-argocd-applicationset-claim.md), +[Argo CD UI user attribution](future/idea-argocd-ui-user-attribution.md), +[commit-signing key validation](future/idea-commit-signing-key-validation.md), +[cross-kind dependency watches](future/idea-cross-kind-dependency-watches.md), +[a source namespace selector](future/idea-source-namespace-selector.md), +[age recipients as metadata](future/publish-age-recipients-as-metadata.md), and +[task-oriented docs and a website](future/task-oriented-docs-and-website.md). ## History — [`finished/`](finished/) diff --git a/docs/TODO.md b/docs/TODO.md index 2b87aef9..740bec86 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -135,6 +135,27 @@ Their argument for each is in [gitops-api-open-asks.md](future/gitops-api-open-a `GitTarget`, plus the durable queue, plus whether a replica warms its fact index before starting a watch it took over. Redis becomes required in this mode. [ha-gittarget-distribution-plan.md](future/ha-gittarget-distribution-plan.md). +- [ ] **Learn an empty repository's default branch, and simplify around it.** The pinned go-git + (`v6.0.0-beta.1`) now keeps what protocol v2 `ls-refs` reports about an unborn `HEAD` + (`transport.RemoteRefs.Unborn`); at alpha.5 it was discarded, which is why status reports + `unknown` and the code carries `ParentUnborn`, `ErrDefaultBranchUnresolved` and the + `branch_worker_empty_repo_test.go` pins. Check whether `Remote.List` exposes it or only the + transport session does, read it in `refresh.go` and `git_smart_fetch.go`, and remove what it makes + unnecessary. Which branch a first commit may create stays a separate policy decision + ([gittarget-parent-empty-repository.md](design/gittarget-parent-empty-repository.md); its §4 and + the go-git entry in [misconceptions.md](misconceptions.md) need updating with it). +- [ ] **The branch claim is first-proven-wins** (`internal/git/branch_owner.go`). Anyone who can + create a `GitProvider` and a `GitBranch` with a credential that passes the push-session proof can + claim a branch no older `GitBranch` holds; on Gitea and Forgejo a read-only credential passes it + (AGit). An older `GitBranch` takes the branch from a live holder once it proves itself, and the + claim is not persisted. Only a per-repository authority closes this (an object per repository URL, + or a cluster rule naming the one provider a repository's branches may be claimed through). +- [ ] **Alias detection.** `repourl.Normalize` cannot unify Gitea's SSH on `:2222` beside HTTPS, a + separate SSH hostname, a vanity domain or a renamed repository, so two spellings get two claims + and two workers on one branch, undetected. Design: warn when a commit pushed under one key shows up + under the other. Also the HA plan's HA-0 question. +- [ ] Accepted for now: no rate bound on push deliveries. Folding bounds the queue, not the rate, so + a `GitReceiver` token holder can keep a worker reading; a per-worker cooldown would bound it. - [ ] Re-assess unifying the pending-write kinds (grouped window, resync and request record, since #450 removed the atomic kind). Deferred. - [ ] Improve queue and worker observability, with more high-load tests. @@ -217,19 +238,21 @@ Their argument for each is in [gitops-api-open-asks.md](future/gitops-api-open-a [git-write-preflight.md](design/git-write-preflight.md) (superseded by the red-status plan), [gittarget-red-status-plan.md](design/gittarget-red-status-plan.md) (steps 5 and 6 built), [target-watch-plan.md](design/target-watch-plan.md) (queue depth is 1000, not 100), - and the go-git version in [azure-devops-multi-ack.md](design/azure-devops-multi-ack.md) and - [gittarget-parent-empty-repository.md](design/gittarget-parent-empty-repository.md). -- [ ] **Wrong claims in design bodies**: [support-contract.md](design/support-boundary/support-contract.md) - calls overlays, values files and ImageUpdateAutomation paths refused; + and the go-git version in [azure-devops-multi-ack.md](design/azure-devops-multi-ack.md). +- [ ] **Wrong claims in design bodies**: [acceptance-precision.md](design/support-boundary/acceptance-precision.md) says everything is open; [admission-consent.md](design/support-boundary/admission-consent.md) says the preflight "fails closed" (it was decided fail-open, and is not built); [multi-source-audit-ingress-hardening.md](design/multi-source-audit-ingress-hardening.md) says the - server checks the named provider exists; [event-pipeline-overview.md](design/event-pipeline-overview.md) - has no applier-claim stage. + server checks the named provider exists. - [ ] **`(GVR, scope)` is still in [architecture.md](architecture.md)** (nine times); [definitions.md](definitions.md) says "resource collection" and retires the old spelling. A mechanical reword, as its own docs PR. +- [ ] **Write down API Priority and Fairness for the operator's load**: the inflight ceiling, which + built-in priority level our watches and audit webhook land in, whether a dedicated + `FlowSchema`/`PriorityLevelConfiguration` helps, and what to observe. An earlier facts page was + removed because it recommended `distinguisherMethod: ByResource`, which does not exist + (`ByUser` and `ByNamespace` only). Measure before writing it. - [ ] Settle two words: "replay" in [applier-echoes.md](design/applier-echoes.md) departure 11 (definitions.md says "initial events"), and "sweep" against "prune" for the act of removing. diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index b3d49439..76c07e57 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -992,8 +992,7 @@ Plan descriptions in those logs drop the operation suffix: a collection renders Every change here is a name, and the controllers decide nothing differently; the exceptions are the moved throttle above and the two removed flags at the end. The rules the names now follow are in -[`definitions.md`](definitions.md), and the plan is -[`design/vocabulary-cleanup.md`](design/vocabulary-cleanup.md). +[`definitions.md`](definitions.md). ### Before you upgrade diff --git a/docs/architecture.md b/docs/architecture.md index bf63d234..659ccbd1 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1664,8 +1664,7 @@ Every `GitTarget` on the branch reports the outage from one worker report held save says the same in its `WaitingForPush` message, and the `git_retained_*`, `git_intake_paused` and `git_next_retry_timestamp_seconds` metrics describe the backlog. What this recovers, and what watch history and a restart cannot, is the -[recovery contract](design/gittarget-branch-worker-pending-writes.md#recovery-contract); the -[event pipeline overview](design/event-pipeline-overview.md) draws it. +[recovery contract](design/gittarget-branch-worker-pending-writes.md#recovery-contract). ### Durability of the write queue (planned) diff --git a/docs/definitions.md b/docs/definitions.md index aac5f0f2..40bb7d67 100644 --- a/docs/definitions.md +++ b/docs/definitions.md @@ -12,9 +12,7 @@ This page completes a set of four rulebooks, and each owns one surface: | [`spec/status-conditions-guide.md`](spec/status-conditions-guide.md) | conditions and reasons: the kstatus trio, polarity, the generic reason vocabulary | | this page | concepts, and the API names that carry them: kinds, fields, blocks, timestamps, enum values, printer columns | -It binds. A name in the code that contradicts a rule here is a defect, not a variation, and the -cleanup that produced this page is tracked in -[`design/vocabulary-cleanup.md`](design/vocabulary-cleanup.md). +It binds. A name in the code that contradicts a rule here is a defect, not a variation. ## Who we borrow from, and for what diff --git a/docs/design/azure-devops-multi-ack.md b/docs/design/azure-devops-multi-ack.md index eee4b225..17dc20a6 100644 --- a/docs/design/azure-devops-multi-ack.md +++ b/docs/design/azure-devops-multi-ack.md @@ -443,7 +443,7 @@ failure halves reproduce locally: `upload-pack` will actually emit multi-ACK streams on a fetch with `have` lines, which is the thing v5 cannot parse. -The e2e lab already runs Gitea (see [`e2e-git-server-choice.md`](e2e-git-server-choice.md)), whose +The e2e lab already runs Gitea, whose backend is canonical git, so no new server is needed — only the proxy and a `GitProvider` pointed at it. This is cheap, it is deterministic, it runs in CI, and it would let us: diff --git a/docs/design/e2e-git-server-choice.md b/docs/design/e2e-git-server-choice.md deleted file mode 100644 index de86d9ef..00000000 --- a/docs/design/e2e-git-server-choice.md +++ /dev/null @@ -1,223 +0,0 @@ -# The e2e Git server: stay on Gitea, or move to Forgejo? - -> **design**: open, not yet built. Index: [`../INDEX.md`](../INDEX.md) -> -> Scope is test infrastructure only: no shipped code path talks to the Git server. This document -> records what was measured in both upstreams, the decision **not** to adopt a Go SDK either way, and -> — the point it turns on — that the pin forcing this decision is fixable **without** migrating. - -## The forcing function is gone - -The premise for migrating was that we are stuck. [`test/e2e/setup/flux/releases/gitea.yaml`](../../test/e2e/setup/flux/releases/gitea.yaml) -holds chart `12.5.3` (Gitea 1.25.5) deliberately, because Gitea 1.26 removed the `_csrf` form input -and cookie from `/user/login`, which [`internal/giteaclient/webclient.go`](../../internal/giteaclient/webclient.go) -scrapes in order to log in and verify an SSH signing key. - -That premise does not survive contact with the source. **Gitea and Forgejo made the same change**: -both replaced form-token CSRF with Go 1.25's stdlib `http.CrossOriginProtection`. - -| | Gitea (`v1.28.0-dev`) | Forgejo | -|---|---|---| -| `_csrf` in templates | **0 matches** | **0 matches** | -| `services/context/csrf.go` | deleted | deleted | -| Replacement | `http.NewCrossOriginProtection()` `routers/web/web.go:170` | same, `routers/web/web.go:228` | -| Guard | `if !options.SignOutRequired && !options.DisableCrossOriginProtection` `web.go:212` | `if !options.SignOutRequired` `web.go:286` | - -`http.CrossOriginProtection.Check` returns `nil` when a request carries **neither** `Sec-Fetch-Site` -nor `Origin` (`net/http/csrf.go:154-159`, Go 1.26.5). A plain Go `http.Client` sends neither, so it -passes unconditionally on both. `/user/login` is additionally exempt on both, being `SignOutRequired`. - -**Therefore the fix is the same on either server, and it is a deletion:** remove `fetchCSRF`, -`extractCSRFToken`, `csrfInputRE`, and the two `form.Set("_csrf", …)` calls — roughly 45 lines. That -unpins Gitea *in place*. Migration and unpinning are **not** the same piece of work, and an earlier -draft of this document wrongly claimed they were. - -## What that leaves - -With the pin removable either way, the comparison is no longer "migrate or stay broken". It is a -straight cost comparison, and Gitea wins it on cost. - -### Staying on Gitea costs one deletion - -Everything else in our integration is already correct for Gitea and stays correct: - -- **Signing namespace unchanged.** Gitea still verifies with the literal `"gitea"` - (`models/asymkey/ssh_key_verify.go:28`), which is exactly what `verification.go:116` signs with. -- **Session cookie unchanged** — `i_like_gitea` (`modules/setting/session.go:37`). -- **Chart source unchanged** — the existing HTTP `HelmRepository` at `dl.gitea.com/charts`. -- **Service names, values, hostnames, scripts** — all unchanged. - -### Moving to Forgejo costs four changes, one of them risky - -- **Signing namespace becomes `setting.Domain`** (`ssh_key_verify.go:34`). And `setting.Domain` is - not `[server] DOMAIN`: `modules/setting/server.go:150` defaults it to `localhost`, then `:263-267` - overwrites it with the **`ROOT_URL` hostname** when that is non-empty and not a bare IP. So the - namespace becomes `gitea-http.gitea-e2e.svc.cluster.local` — host only, no scheme or port. - **This is the one genuinely risky item in either direction.** A wrong namespace produces a - *valid-looking* signature that the server rejects with a generic - `settings.ssh_invalid_token_signature` flash, so it reads as a credentials or key-material bug. -- **Session cookie becomes `session`** (`modules/setting/session.go:37`, commit `97a3837215`, - v15.0.0, flagged breaking upstream). There are **zero** `i_like_*` literals anywhere in the Forgejo - tree. Our `webclient.go:23` hardcodes the old name and `NewWebSession` hard-fails without it. -- **Chart moves to OCI** — `oci://code.forgejo.org/forgejo-helm/forgejo`, chart `17.1.3`, - appVersion `15.0.5` (verified with `helm show chart`). Our `HelmRepository` must become `type: oci`. - Whether Flux's OCI `HelmRepository` works in our e2e cluster is the one **untested** item. -- **Values churn**, though this direction is a simplification: the five `*.enabled: false` subchart - toggles are deleted (Forgejo's chart dropped every DB/cache subchart and defaults to SQLite), and - `fullnameOverride: gitea` keeps Services at `gitea-http` / `gitea-ssh` so no hostname changes. - The values root is still `gitea:` — the chart is a fork that kept the key — so the entire - `gitea.config.*` block ports verbatim. - -Plus, eventually, ~50 files of `gitea*` → `forgejo*` rename churn to stop the tree lying about what -it talks to. - -### Where Gitea is genuinely ahead - -Two capabilities exist on Gitea with no equal on Forgejo. - -**1. `TRUSTED_SSH_KEYS` is strictly better than Forgejo's equivalent.** Both have a config-level lever -that trusts a known public key without per-user web verification, and both skip identity checks -entirely (`sshsig.Verify(…, "git")` and nothing else): - -| | Gitea | Forgejo | -|---|---|---| -| Lever | `[repository.signing] TRUSTED_SSH_KEYS` (a list) | `FORMAT=ssh` + `SIGNING_KEY=` → `setting.SSHInstanceKey` | -| Where | `services/asymkey/commit.go:396-405` | `models/asymkey/ssh_key_object_verification.go:68-81` | -| Reported `signer.email` | the **commit's own committer email** | the **fixed** configured `SIGNING_EMAIL` | -| Preflight | `GET /api/v1/signing-key.pub` | `GET /api/v1/signing-key.ssh` | - -That last row decides it. `assertGiteaVerified` ([`signing_common_test.go:73-96`](../../test/e2e/signing_common_test.go)) -asserts `v.Signer.Email == expectedSignerEmail`. On Gitea's path that passes unchanged with an -arbitrary per-test committer; on Forgejo's, the spec would have to pin its committer email to -`SIGNING_EMAIL`, and the assertion stops proving user attribution. Gitea also accepts a *list*, where -Forgejo takes a single key. (The Gitea `app.example.ini` comment claiming this path exposes -`SIGNING_EMAIL` is wrong — the code passes `c.Committer.Email`.) - -**2. The SDK option stays open, and only makes sense against Gitea.** `code.gitea.io/sdk/gitea` -(v0.25.1) is actively maintained and tracks Gitea minors. Its version gating — `NewClient` GETs -`/version` and gates features on semver — is *meaningful* against real Gitea and **actively -misleading** against Forgejo, which reports a far higher version so every Gitea feature gate passes -trivially. We are not adopting it (below), but on Gitea it stays a live option; on Forgejo it would -be a liability. - -## Decision: do not adopt a Go SDK, on either server - -**There is no official Forgejo SDK.** Nothing under the `forgejo` org on `code.forgejo.org`. -`earl-warren/forgejosdk`, from a core Forgejo maintainer, is untouched since 2024-01-01. -`mvdkleijn/forgejo-sdk` is a third-party fork of the Gitea SDK with a single maintainer. Forgejo's own -`go.mod:20` depends on `code.gitea.io/sdk/gitea`; it neither uses nor built a Forgejo SDK. - -The Gitea SDK covers all 21 endpoints we call, including the unusual ones -(`POST /users/{login}/tokens`, `GET /user/gpg_key_token`, `POST /admin/users/{u}/keys`, -`POST /admin/users/{owner}/repos`). Coverage is not the problem. Four things are: - -- **No SDK can cover `webclient.go`.** SSH signing-key verification is a web form flow, not REST, on - both servers. Adopting an SDK does not remove hand-rolled HTTP — it *splits* the client into an SDK - half and a hand-rolled half, with two auth models and two error idioms. The web half is where - breakage actually happens, so this is the wrong half to modernise. -- **Closed response types are brittle, and Forgejo proves it.** - `external-sources/forgejo/services/migrations/gitea_sdk_hack.go` uses `go:linkname` to reach the - SDK's **unexported** `(*Client).getParsedResponse`, purely to add one JSON field the SDK does not - model. Forgejo could not extend it through supported means. -- **Supply chain, for test-only code.** Five new direct dependencies we do not share, including two - HTTP-signature libraries and a Windows PuTTY agent shim, for SSH-cert auth we never use — plus an - `x/crypto` version-floor conflict. Our current client is stdlib-only: zero. Hard to justify on a - repo running Scorecard, cosign, and govulncheck, for code absent from the shipped binary. -- **Context is client-scoped, not per-call.** Our client threads `ctx` per method. Under parallel - Ginkgo we would construct a client per context (paying the `/version` round-trip each time) or - mutate shared state and race. - -**Revisit if** we need many more endpoints. The SDK's value rises with breadth of API use, and ours -is narrow and static. - -## SSH key verification works on both - -The signing spec needs the host to report commits as `Verified`, and that is the one thing driven -through the web UI. Forgejo's `verify_ssh` handler (`routers/web/user/setting/keys.go:207-241`) is -logically identical to Gitea's: same `VerificationToken(doer, 1)` / `lastToken(doer, 0)` pair giving a -two-minute window across the minute boundary, same fallback retry, same flash keys. `AddKeyForm` is -byte-identical between the forks. `GET /user/gpg_key_token` serves the **SSH** token too, despite the -GPG-flavoured name. So this is not a differentiator — it works either way. - -**Registering a key is not enough**, on either. `k.Verified` gates the per-user path (Forgejo -`ssh_key_object_verification.go:57-64`, which additionally requires the committer email to be an -*activated* address; Gitea `commit.go:383-390`). The column is written in exactly one place per tree, -reachable only from the web handler. Forgejo's `AddPublicKey` has no `verified` parameter at all, -`POST /admin/users/{u}/keys` creates unverified keys, and no CLI touches it. There is no API -shortcut — it is the web flow, a direct DB write, or a config-level trust lever. - -Neither config lever helps the **generated-key** spec ([`signing_e2e_test.go:71`](../../test/e2e/signing_e2e_test.go)), -which mints a key at runtime: both are read at config load and would need a restart mid-suite. So the -web flow stays regardless of server — which is why the CSRF deletion is the work that actually matters. - -## Recommendation - -**Decouple the two decisions, and do the cheap one first.** - -1. **Unpin Gitea now.** Delete the CSRF scraper and move the chart forward off 12.5.x. This is - required work under *either* outcome, it is a net code deletion, and it removes the only real - pressure. Also drop the `allowedVersions: <12.6.0` guard in - [`.github/renovate.json`](../../.github/renovate.json) and the inline pin comment, which exist - solely to describe a problem that no longer needs a workaround. -2. **Then decide Forgejo on its merits, not under duress.** After step 1 the technical case is close - to neutral-to-negative: Gitea costs nothing further, keeps the literal signing namespace (avoiding - the one hazardous change), and has the better trusted-key lever. Forgejo's advantages are a simpler - chart and project/governance preference — a legitimate reason to move, but not a technical one, and - it should be stated as such rather than dressed up as a fix. - -The honest summary: **the migration was justified by a pin that turns out to be self-inflicted.** -Once the scraper is gone, moving to Forgejo buys a cleaner chart and costs a risky namespace change -plus an untested OCI dependency. If the project preference is strong the migration is entirely -feasible and the plan below holds. If it is not, staying is cheaper and slightly safer. - -## Migration plan, if Forgejo is chosen - -Estimated **1–2 days**, risk concentrated in step 3. - -| # | Step | Size | Risk | -|---|---|---|---| -| 0 | Smoke-test Flux **OCI** `HelmRepository` against `oci://code.forgejo.org/forgejo-helm`. The chart pulls fine with `helm`; Flux OCI support in our cluster is the untested piece. | 30 min | infra unknown — **do first** | -| 1 | `HelmRepository` → `type: oci`; `HelmRelease` → `chart: forgejo`, `version: 17.1.3`. | 30 min | low | -| 2 | Values: delete the five subchart toggles, add `fullnameOverride: gitea`, keep `gitea.config.*` verbatim. | 45 min | low | -| 3 | **SSH signing namespace** → the ROOT_URL hostname. Own commit, own bisect point; land on a scratch cluster first. | half day | **medium — hostile failure mode** | -| 4 | Session cookie constant → `session`. | 10 min | low | -| 5 | Full `task test-e2e`, plus the signing and bi-directional corners. | 1 hour | — | - -The CSRF deletion is deliberately **not** in this table — it belongs to step 1 of the recommendation -and ships either way. - -### Verify immediately after the first install: SSH reachability - -Both Gitea Services today are **headless** (`clusterIP: None`), so DNS returns pod IPs and the -declared Service port is bypassed. Probed from inside the live cluster against `gitea-ssh.gitea-e2e.svc`: - -```text -port 22 closed <- the Service's declared port; nothing listens -port 2222 OPEN <- the rootless image's real listener -port 13000 OPEN <- http, on the *ssh* service name, because headless -``` - -So `repo_setup.go:39`'s `…gitea-ssh…:2222` works by reaching the pod directly, and `SSH_PORT: 22` in -our values is cosmetic — it only sets the advertised clone URL. The apparent contradiction between the -two is not a bug today. But the URL depends on headless behaviour *plus* the rootless listener, not on -a port mapping: if Forgejo's chart Services are not headless, `:2222` breaks. Check this before -debugging anything else. - -### Deliberately out of scope - -**The `gitea*` → `forgejo*` rename.** `internal/giteaclient`, `GiteaTestInstance`, `waitForGiteaAPI`, -`GITEA_*` Taskfile vars, the `gitea-e2e` namespace, `cmd/gitea-signing-debug`, -`hack/checkout-gitea-repo.sh` — ~50 files of churn with real review and merge-conflict cost. Separate -PR once the functional migration is green. `fullnameOverride: gitea` means nothing functionally -depends on it. - -**Crossplane / OpenTofu-managed repositories.** Evaluated and rejected for e2e: each spec creates a -throwaway repo, user, and collaborator inline and uses it milliseconds later (15+ repos per suite -run). A `Workspace` CR reconciled through `tofu plan` is far slower, needs durable remote state that a -recreated k3d cluster destroys, and does not address SSH key *verification* — our actual pain. A -`ForgejoProject` XRD is an interesting **product** direction; it does not belong on the test path. - -## Rollback - -Every change is confined to `test/e2e/setup/flux/**` and `internal/giteaclient/**`, neither of which -ships. Reverting restores the previous server. No CRD, no API, no user-visible surface is touched. diff --git a/docs/design/event-pipeline-overview.md b/docs/design/event-pipeline-overview.md deleted file mode 100644 index ab354ceb..00000000 --- a/docs/design/event-pipeline-overview.md +++ /dev/null @@ -1,577 +0,0 @@ -# Event pipeline overview: from a watch event to a Git commit - -> **Snapshot, 2026-10-04, at the end of #413.** A picture-first tour of how a change -> in the cluster becomes a commit, with the replay paths drawn out and an honest list of what is -> still missing. What was removed since is taken out; the apply trigger, which came later, is not -> drawn here (its timer, and the flush that closes a window when an apply comes due): see -> [`apply-trigger.md`](apply-trigger.md). The plan that changes it is -> [`gittarget-branch-worker-pending-writes.md`](gittarget-branch-worker-pending-writes.md); the longer-term event model -> is [`branch-worker-event-model.md`](branch-worker-event-model.md). - -## 1. The whole pipeline on one page - -```mermaid -flowchart LR - subgraph SRC["Source cluster"] - API["kube-apiserver"] - AUD["audit webhook"] - end - - subgraph WATCH["Watch plane, one stream per (GitTarget, collection)"] - WS["target watch stream
replay, live, resume"] - DD{"desired-state
change filter"} - AT["attach author
from audit facts"] - PG["producer gate
(stream retired?)"] - CUR[("durable cursor
per GitTarget UID + collection")] - end - - subgraph ROUTE["Routing"] - ER["EventRouter"] - GES["GitTargetEventStream"] - end - - subgraph CTRL["Controllers"] - CRC["CommitRequest controller"] - GTC["GitTarget controller"] - end - - subgraph BW["Branch worker, one per (GitProvider, branch)"] - ADM{"intake
queue room and
budget gate"} - FIFO[["FIFO
default 1,000 items"]] - LOOP(("event loop
single goroutine")) - WIN["open commit window"] - PW[("pending writes, in order
in memory")] - MAT["materialize
(the only commit path)"] - CO[("local checkout")] - PUB["publish
(atomic push, CAS)"] - end - - REMOTE[("Git remote")] - - API -- "watch frames" --> WS - AUD -. "who did it" .-> AT - WS --> DD - DD -- "no-op UPDATE: skip" --> CUR - DD -- "changed" --> AT --> PG --> ER --> GES --> ADM - ADM -- "accepted" --> FIFO - ADM -. "refused: session ends,
stream waits for intake" .-> WS - GES -. "accepted: record content,
then the cursor" .-> CUR - WS -- "replay snapshot (resync)" --> ADM - CRC -- "attach / withdraw a save" --> ADM - GTC -- "refresh" --> ADM - FIFO --> LOOP - LOOP --> WIN -- "window closes" --> PW - LOOP -- "resync, save record,
refusal touch, atomic" --> PW - PW --> MAT --> CO --> PUB --> REMOTE - REMOTE -. "rejected: replay onto new tip" .-> MAT -``` - -Read it left to right: - -1. **One watch stream per collection.** A `GitTarget` that watches ConfigMaps in `apps` has one - stream for `(configmaps, apps)`. It starts with a replay of current state and then streams live - changes. -2. **A live change is checked, attributed, and routed.** The desired-state change filter passes - creates, deletes, and changes to Git-visible content, including retained metadata. It drops - unchanged UPDATEs, counted as `watch_events_total{outcome="unchanged"}`. The author comes from - audit facts. The producer gate prevents retired streams from enqueueing behind their replacements. -3. **The branch worker admits it or refuses it.** Admission is non-blocking: a full FIFO, a - stopping worker, or a paused intake gate refuses the item. A refused live event travels back up - as an error, the session ends, and the cursor stays put. When the branch has paused intake, the - stream waits for it to reopen before it reconnects. -4. **One goroutine owns everything after the FIFO.** It collects live events into a commit window, - decides pending writes, materializes them into commits, and publishes them. - -Several `GitTarget`s that write to the same provider and branch share one worker, and with it one -FIFO, one set of pending writes, one checkout, and one retry schedule. - -## 2. The watch stream: replay, live, and resume - -```mermaid -stateDiagram-v2 - [*] --> Replaying: stream started
(always replays first) - - Replaying --> Replaying: Added/Modified folded
into the desired set - Replaying --> SnapshotOffered: initial-events-end bookmark - Replaying --> ListFallback: sendInitialEvents unsupported - - ListFallback --> SnapshotOffered: LIST complete
(watch opened first, buffered) - - SnapshotOffered --> Live: worker accepted the resync
record cursor, mark stream replayed - SnapshotOffered --> Backoff: worker refused the resync
(no cursor, not replayed) - - Live --> Live: event accepted
record cursor - Live --> Backoff: event refused, watch error,
or server closed the watch - Live --> Backoff: 410 Gone (cursor expired) - - Backoff --> WaitingForIntake: the branch has paused intake - WaitingForIntake --> WaitingForIntake: woken, but paused again
(or 1 min fallback recheck) - WaitingForIntake --> Backoff: intake reopened - - Backoff --> Replaying: 2s later, stream has NOT replayed yet - Backoff --> Resuming: 2s later, stream has replayed - Resuming --> Live: watch opened from stored cursor - Resuming --> Replaying: 410 Gone on open
(fresh replay) - - Live --> [*]: stream retired or GitTarget gone -``` - -Four rules hold this together: - -- **A new stream always replays.** Starting a stream issues a fresh render-fidelity revision for - its collection; resuming an old cursor would never report that revision, so the scope would stay - pending forever. -- **A stream resumes only after its own replay was accepted** (step 5a). If the worker refuses the - snapshot, the stored cursor still belongs to a previous stream, so the reconnect replays again. -- **The cursor only moves past what the worker accepted.** A refused event ends the session before - the cursor is written; an event that a retiring stream never enqueued records no cursor at all. -- **A stream waits for a paused branch** (step 5b). Before every reconnect the stream asks - its branch worker whether intake is paused, and if so waits on a channel the worker closes when - it reopens. Reconnecting on the 2s backoff would gather a snapshot the branch refuses again, - for as long as the outage lasts. - -### The step 5a case, as a sequence - -```mermaid -sequenceDiagram - autonumber - participant API as kube-apiserver - participant S as watch stream - participant D as desired-state change filter - participant W as branch worker - participant C as cursor store - - API->>S: ADDED cm rv=10 data=A - S->>D: check: new object - S->>W: Enqueue - W-->>S: accepted - S->>D: record hash(A) - S->>C: cursor = 10 - - API->>S: MODIFIED cm rv=11 data=B - S->>D: check: B != A, changed - S->>W: Enqueue - W-->>S: refused (queue full or intake paused) - Note over S,D: nothing recorded:
the baseline stays A - S--xS: session ends with an error, cursor stays 10 - - Note over S,W: if intake is paused: wait until the worker reopens it - Note over S: 2s backoff, resume from rv=10 - API->>S: MODIFIED cm rv=11 data=B (redelivered) - S->>D: check: B != A, still a change - S->>W: Enqueue - W-->>S: accepted - S->>D: record hash(B) - S->>C: cursor = 11 -``` - -Before step 5a, the hash of B was recorded at step 8, so the redelivered frame matched it, was -skipped as unchanged, and the cursor moved to 11 without the worker ever seeing B. - -### The desired-state change filter - -The filter keeps status churn out of the FIFO. It also protects commit windows: `/status` updates -have no audit author, so passing one can close a named author's window even when Git has no diff. - -Each stream owns its baselines: a hash per UID of what the worker last accepted, from a live event -or from the stream's accepted replay snapshot, which replaces them all. A refused or unfinished -replay installs nothing. A `GitTarget` cannot hold two overlapping collections, so one stream is the -only producer for an object. See -[step 5a2](gittarget-branch-worker-pending-writes.md#step-5a2-desired-state-change-filter-built). - -## 3. Three different things called "replay" - -The word covers three mechanisms that guarantee different things. Most confusion about recovery -comes from mixing them up. - -```mermaid -flowchart TB - subgraph R1["1. Watch replay: re-observe the cluster"] - direction LR - A1["sendInitialEvents watch
or LIST + buffered watch"] --> A2["desired set for one collection"] - A2 --> A3["ResyncRequest
(scope = collection)"] - end - - subgraph R2["2. Resync: apply a snapshot as a write"] - direction LR - B1["resync decided as a pending write
(fetches first: judged against
the newest remote tree)"] --> B2["mark-and-sweep plan
under the target's prune mode"] - B2 --> B3["reply: created / updated / deleted
marks scope accepted + render-clean"] - end - - subgraph R3["3. Git replay: re-apply decided writes"] - direction LR - C1["remote moved, or checkout
lost the commits"] --> C2["reset to remote tip"] - C2 --> C3["re-plan each pending write in order
on the new tree"] - C3 --> C4["entry the new tree refuses:
settled alone (step 4)"] - end - - R1 --> R2 - R2 -. "the resync is now a pending write,
so it can be Git-replayed too" .-> R3 -``` - -| Replay | Input | What it recovers | What it cannot recover | -|---|---|---|---| -| Watch replay | Current cluster state | Current content of every object in scope | Intermediate versions, who made them, which save they belonged to | -| Resync | One collection's desired set | Git matches that set; with `prune.mode: always`, objects gone from the cluster are swept | With `onEvent` (the default), a delete that happened while nobody watched cannot be inferred from absence | -| Git replay | The pending writes, in memory | Every decided write, in order, with its message, author, and save | Anything lost with the process: pending writes are not persisted | - -### Resync coalescing and fences - -A resync rides the same FIFO as live events, so it lands in arrival order. Several snapshots for -the same scope collapse into one, but never across a live write for that scope: - -```mermaid -flowchart LR - N["new resync for scope S"] --> Q{"a resync for S
already queued?"} - Q -- "no" --> M["queue a marker at the tail"] - Q -- "yes, nothing for S
queued behind it" --> SW["swap in the newer payload
answer the old caller: superseded"] - Q -- "yes, but a write for S
is queued behind it (fenced)" --> REL["old marker keeps the payload it holds
new request takes a fresh marker"] -``` - -Swapping a payload behind a live write would let the older write overwrite the newer snapshot, so -the fence (`tailPassed`) forces a new position instead. The payload the old marker holds may be a -request swapped into it earlier; since step 5b that request runs, where before it was dropped and -its caller never answered. - -Every resync closes the open window first, to keep arrival order. No snapshot is parked. - -## 4. Inside the branch worker: inputs and the loop - -```mermaid -flowchart TB - subgraph FIFO_IN["FIFO items (ordered, one at a time)"] - I1["Request: live events
CommitModePerEvent"] - I2["Request: atomic batch
(tests only)"] - I3["Attach a save"] - I4["Withdraw a save"] - I5["Resync"] - I6["Refresh"] - end - - subgraph TIMERS["Timers (in the same select)"] - T1["commit window:
idle / max duration"] - T2["push cooldown"] - T3["attach deadline"] - T4["refusal action"] - T5["retry deadline"] - end - - SEL(("select")) - FIFO_IN --> SEL - TIMERS --> SEL - - SEL --> H["handler"] - H --> AFTER["after every wake:
service waiting saves,
publish held bytes to the intake gate"] - AFTER --> SEL -``` - -Everything the loop does starts from one of these eleven inputs. Two more act from outside the -select: worker shutdown (context cancel) and `SetParentBranch`, an atomic setter that the push's -admission check reads. - -### How a commit window closes - -```mermaid -flowchart LR - E["live event"] --> G{"render fidelity
established for
this target?"} - G -- "no" --> DROP["dropped
(the pending replay repairs it)"] - G -- "open" --> CAN{"an open window of
the same author
and target?"} - CAN -- "yes" --> ADD["add to window"] - CAN -- "no" --> PROVE{"footprints proven
and disjoint?"} - PROVE -- "yes" --> NEW - PROVE -- "no" --> FIN1["close the windows
it cannot sit beside
(identity change,
path conflict)"] --> NEW - NEW["open window,
snapshot target's timers"] --> ATT["attach a waiting save"] --> ADD - ADD --> CAP{"retained bytes
over budget?"} - CAP -- "yes" --> FIN2["close window
(buffer limit)"] - CAP -- "no" --> ARM["arm idle / max timer"] -``` - -Other things that close a window: the idle or maximum-duration timer, an atomic batch or a -resync (to keep arrival order), a save with `attach: Next`, and shutdown. A closed window -becomes one decided write. - -The other write gate, `spec.suspend`, applies later, when the write is committed: a suspended -target's events are dropped, its folder is still scanned so its status stays fresh, and resuming -replays current state. - -## 5. Pending writes: decide, materialize, publish - -Every write takes one path, whatever produced it: a closed window, a resync, a save's empty -record, or an atomic batch. - -```mermaid -stateDiagram-v2 - [*] --> Decided: decide()
gates and build passed
save goes WaitingForPush - - Decided --> Decided: remote unreachable
or retry not due yet
(kept, retry scheduled) - Decided --> Committed: materialize()
one local commit - - Committed --> Committed: push failed
(kept, retry scheduled) - Committed --> Committed: push rejected, remote moved
Git replay re-plans it - Committed --> Published: push succeeded
save resolved with its SHA - - Decided --> Failed: the write itself failed
(plan refused, cannot apply) - Committed --> RefusedOnReplay: the moved remote's tree
refuses this entry - RefusedOnReplay --> Failed: settled alone (step 4)
refusal reported, its save fails - - Published --> [*] - Failed --> [*] -``` - -`decide` only appends to the pending writes. One driver, `advance`, runs at the end of every wake of -the loop: it materializes what was decided, schedules the push, and closes the retry and parent -recovery once nothing is pending. That last step is in one place, so no path that settles the last -pending write can leave intake paused. - -Three outcomes, classified in exactly one place, `materialize` in -[`pending_writes_loop.go`](../../internal/git/pending_writes_loop.go): - -- **Committed** (`settleCommitted`): the write is now a commit in the checkout, waiting to be pushed. -- **Failed for good** (`settleFailed`): the write itself was refused. Only that entry stops being pending. -- **Unreachable**: the remote could not be reached. Everything stays pending for the retry, and a - waiting resync caller is answered with the error (`replyToDeferredResyncs`). - -The checkout is a *projection* of the pending writes: the remote tip the writes were planned on, plus one -commit per materialized entry. The worker tracks how many entries the checkout holds -(`checkoutApplied`); when that is unknown, `materialize` resets to the remote tip and replays. - -### Publishing, with a rejection - -```mermaid -sequenceDiagram - autonumber - participant L as event loop - participant M as materialize - participant G as Git remote - - L->>M: pushPending() - M->>M: commit any decided-but-uncommitted entries - M->>G: atomic push (compare-and-swap on the branch) - G-->>M: rejected: branch moved - M->>G: fetch new tip - M->>M: reset, replay entries 1..n on the new tree - Note over M: entry k refused by the new tree:
undone, stamped, replay continues - M->>G: push again (at most 3 attempts per cycle) - G-->>M: accepted - M-->>L: pushed - L->>L: resolve saves on pushed entries
(each with its own SHA) - L->>L: clear the pending writes, clear the retry - L->>L: settle entry k as a refusal
(may decide an empty "touch" commit) -``` - -The refused entry is taken out before the pushed saves resolve and settled after the pending -writes are cleared, because settling it can decide new work. - -Every Git call in this sequence is bounded: two minutes per advertisement, fetch, or push session, -and five minutes for the whole cycle. When a push fails without a rejection (a deadline, a dropped -connection), the cycle probes the remote. If the branch is at the commits it sent, the push landed -and only the reply was lost, so the cycle settles as published instead of replaying them. - -## 6. Retry and intake - -One retry deadline covers all of the worker's pending writes: 10 seconds, doubling to 5 minutes. -While a parent branch is missing, the deadline's attempt is a single advertisement probe; -otherwise it materializes and pushes. The deadline records the failure it is waiting out and when -the outage began. The worker publishes both, with the intake pause, as its publication report: -every `GitTarget` on the branch shows it on `Ready`, and a held save in its message. The report -changes only when one of those does, so a retry that fails the same way writes no status. - -```mermaid -stateDiagram-v2 - [*] --> Healthy - Healthy --> RetryPending: an attempt failed
scheduleRetry(cause) - RetryPending --> RetryPending: new decisions wait
(no connection spent);
a resync is answered with the cause - RetryPending --> Attempt: retry deadline fires - Attempt --> Healthy: published, nothing pending
clearRetry(): intake reopens - Attempt --> RetryPending: failed again
backoff doubles - - state RetryPending { - [*] --> IntakeOpen - IntakeOpen --> IntakePaused: a payload would take
held + queued past the budget
(default 8 MiB) - } -``` - -The intake gate ([`intake.go`](../../internal/git/intake.go)) applies only while a retry is -pending; a healthy branch never pauses, because its pending writes drain at the next push. - -- **What it counts.** Payload on the FIFO, charged at enqueue so producers cannot fill the FIFO - between two loop iterations, plus what the loop holds: the open window, the pending writes, - and saves waiting for a window. Every item is charged 1 KiB beyond its payload and message, so - empty saves and refusal commits count and the budget bounds the item count too. Sizes are - serialized YAML, estimated once by the producer. -- **Pausing is a latch.** The first payload that does not fit is refused and pauses the branch. - Only a cleared retry reopens it. Room under the budget again, a partial replay, or a remote that - can be read but refuses the push keep it paused. -- **Waking producers.** `BranchWorker.IntakePaused` returns a channel the gate closes when it - reopens; the watch streams wait on it. - -What each producer does when the gate refuses it: - -| Producer | On refusal | Comes back through | -|---|---|---| -| Live watch event | Session ends, cursor not advanced | Waits for intake, then the cursor resume redelivers the frame | -| Replay snapshot | Session ends, stream not marked replayed | Waits for intake, then the reconnect replays again | -| Snapshot larger than the whole budget | Refused during an outage, with both sizes in the message | Gathered again once the branch reopens; accepted on a healthy branch | -| Save attach | Dropped | The `CommitRequest` controller re-sends on its next poll | -| Save withdraw, refresh | Not refused by a pause: lifecycle work | Only a full FIFO or a stopping worker drops them; the sender asks again | - -## 7. A save (`CommitRequest`) from start to finish - -```mermaid -sequenceDiagram - autonumber - participant U as user - participant CR as CommitRequest controller - participant W as branch worker loop - participant G as Git remote - - U->>CR: create CommitRequest (author from admission) - CR->>W: EnqueueAttach - W->>W: register (first come, first served) - alt a same-author window is open or opens before the deadline - W->>W: attach to that window - W->>W: window closes, decided as a pending write
save: WaitingForPush - else no window by the attach deadline - W->>W: decide an empty "save record" commit
save: WaitingForPush - end - loop controller polls - CR->>W: LookupCommitRequestOutcome - end - W->>G: push - G-->>W: accepted - W->>W: resolve: Committed with this entry's SHA
(or AlreadyPresent if nothing changed) - CR->>CR: project status: Pushed=True -``` - -The controller never fails a save the worker still holds. Past its safety bound it sends a -withdraw, and the worker decides: an unattached request can be withdrawn, an attached one stays -`WaitingForPush` until it publishes. - -## 8. What is an event today, and what is not - -The direction in [`branch-worker-event-model.md`](branch-worker-event-model.md) is an -event-sourced state machine per branch. Today the worker is a single-owner event loop that is -partway there: - -```mermaid -flowchart LR - subgraph YES["Explicit, ordered inputs"] - Y1["6 FIFO item kinds"] - Y2["5 timers in one select"] - Y3["5 pending write kinds
(window, atomic, resync,
save record, refusal touch)"] - Y4["3 settle outcomes"] - end - subgraph PARTLY["Recorded, but only in memory"] - P1["the pending writes"] - P2["save registrations
and outcomes"] - P3["retry deadline
and intake pause"] - end - subgraph NO["Implicit inputs a decision still reads"] - N1["time.Now()"] - N2["live GitTarget / GitProvider config"] - N3["render-fidelity gate"] - N4["synchronous Git results"] - N5["SetParentBranch, shutdown"] - end - YES --> PARTLY --> NO -``` - -| Concern | Today | -|---|---| -| Single owner of branch state | Yes: one goroutine per worker | -| Ordered inputs | Yes, for what enters the FIFO; timers interleave by wall clock | -| Decisions as recorded facts | Partly: the pending writes record decided writes; window membership, save attachment, and timer firings are not recorded | -| Effects separated from decisions | Partly: `decide` and `materialize` are separate, but both run synchronously on the loop | -| Replayable without side effects | No: replaying would call Git and read current config | -| Durable | No: everything after the FIFO is in memory | - -## 9. What we do not have yet - -This is the honest list. Each item names who plans to close it, or says nobody does yet. - -### Durability - -- **Nothing after admission survives a restart.** The FIFO, the pending writes, open windows, save - registrations, and outcomes are in memory. The watch cursor *is* durable (Redis), so it can be - ahead of work the process lost. A restart replays every stream, which repairs current content - under each target's write gates and prune policy. It does not restore decided writes, - intermediate versions, authors, or save membership. Owner: the - [HA and durable delivery plan](../future/ha-gittarget-distribution-plan.md); deferred. -- **A save published shortly before a crash loses its receipt.** A re-sent request can create a - second empty commit. Owner: HA plan. - -### Bounded work - -- **Git call budgets are fixed.** Since step 6 every call to a Git server ends within two minutes - and a push cycle within five, over HTTP and SSH alike. The values are not configurable, so a - depth-1 fetch that needs longer fails every attempt. Local work inside a cycle has no bound of - its own. Owner: nobody yet; a flag waits for a measured need. -- **A lost push reply is settled by evidence only in the simple case.** When the remote is still at - the commits a failed push sent, step 6 publishes them as they are. When another writer pushed on - top in the meantime, the writes replay and a save's empty commit lands twice. Exactly-once is not - promised. Owner: HA plan. - -### Bounded memory and pause/resume - -Step 5b closed the gaps this section used to list: queued payload, empty records, -and waiting saves are counted; the pause holds until a push lands; producers wait instead of -polling; resyncs respect the retry deadline; and an oversized snapshot has an explicit path. What -remains: - -- **The budget counts serialized bytes.** A queued object is held as an unstructured map of - about six times its YAML size, so the 8 MiB default is well below the memory it stands for. Owner: nobody yet; - documented in [Interpreting metrics](../interpreting-metrics.md). -- **A healthy branch's FIFO is bounded by count only.** The budget applies while a retry is pending; - until then, up to 1,000 queued items cost memory on top of the window and the pending writes. By design: a - healthy branch drains. -- **Work accepted before the outage can overshoot.** It is kept, so the held total can pass the - budget by that much, and no further. - -### Status and observability - -- **The outage reason is generic.** Since step 5c every `GitTarget` on a branch that cannot publish - reports `Ready=False` under `Progressing`, with the start, the cause, and the pause in the message, - and a held save carries the same cause. A dedicated reason such as `PublicationFailing` waits for - a measured need. Owner: nobody yet. - -### Watch history and ordering - -- **Kubernetes watch history is not an archive.** An expired cursor forces a fresh snapshot. With - the default `prune.mode: onEvent`, a delete that happened while nothing was watching is not - inferred from absence. Inherent; documented, not planned. -- **Arrival order is not a save barrier.** A save can reach the worker before an earlier edit - from an independent watch. Only the documented attachment contract is claimed. Not planned. -- **A snapshot reply means "applied locally".** It is no publication receipt. Render fidelity - is proven against the fresh tree the resync fetched. By design. - -### Not yet event-sourced - -- **Decisions read implicit inputs**: the clock, live configuration, the fidelity gate, and Git - results. Replaying the same items tomorrow can produce different windows. Owner: the transition - boundary in [`branch-worker-event-model.md`](branch-worker-event-model.md), after the pending-writes plan. -- **Timers, attaches, withdrawals, and refreshes are not recorded as transitions.** Owner: the PR - after #413. -- **Known structural smells in the pending writes**: `checkoutApplied` living on the worker instead - of with the pending writes, and two replay functions (`replayOntoRemote` and its locked wrapper). Owner: the same - follow-up. - -### High availability - -- **One process owns a branch.** There is no failover, no handoff of a worker's pending writes, and - no shared journal. Owner: HA plan; deferred. - -## Where to read the code - -| Piece | File | -|---|---| -| Watch streams, replay, resume, desired-state change filter | [`internal/watch/target_watch.go`](../../internal/watch/target_watch.go) | -| Routing, resync replies, fidelity marks | [`internal/watch/event_router.go`](../../internal/watch/event_router.go) | -| Enqueue into a worker | [`internal/reconcile/git_target_event_stream.go`](../../internal/reconcile/git_target_event_stream.go) | -| Admission, FIFO, loop, windows, push | [`internal/git/branch_worker.go`](../../internal/git/branch_worker.go) | -| Intake budget and pause | [`internal/git/intake.go`](../../internal/git/intake.go) | -| Pending writes: decide, materialize, settle | [`internal/git/pending_writes_loop.go`](../../internal/git/pending_writes_loop.go) | -| Retry deadline | [`internal/git/retry.go`](../../internal/git/retry.go) | -| Publication report and backlog gauges | [`internal/git/publication.go`](../../internal/git/publication.go) | -| Resyncs | [`internal/git/resync_flush.go`](../../internal/git/resync_flush.go) | -| Saves | [`internal/git/commit_request_attach_loop.go`](../../internal/git/commit_request_attach_loop.go) | -| Missing parent probe | [`internal/git/parent_recovery.go`](../../internal/git/parent_recovery.go) | diff --git a/docs/design/gitbranch-and-kubeprovider-plan.md b/docs/design/gitbranch-and-kubeprovider-plan.md deleted file mode 100644 index abf877a0..00000000 --- a/docs/design/gitbranch-and-kubeprovider-plan.md +++ /dev/null @@ -1,367 +0,0 @@ -# GitBranch, KubeProvider, and a phased API break - -Status: **plan**, 2026-10-21. Phase 1 shipped in #429, phase 2 in #430, and phase 3 after them. -The current contract is in [architecture.md](../architecture.md) and -[configuration.md](../configuration.md). This page records what we decided and in what order we -build it. - -> **Superseded in part.** That a `GitBranch` has no access list of its own, and that the -> `GitProvider`'s `accessFrom` governs both hops, no longer hold: grants on each referenced object -> replace `accessFrom`, `allowedBranches` and `allowCrossNamespace` -> ([who may reference what](../finished/provider-access-rules.md)). - -The plan breaks the API on purpose, more than once. There are no production users to carry across, -so each phase replaces a shape outright instead of keeping a deprecated twin beside it. A removed -field stays in the schema only so that setting it is refused by name, the pattern the API already -uses for `qps`, `burst`, and `allowAnySourceNamespace`. - -## Decisions - -| Decision | Choice | -|---|---| -| The repository branch gets its own kind | `GitBranch`, namespaced, referenced as `gitBranchRef` | -| A `GitBranch` names its credential | `gitProviderRef` in its own namespace only | -| A `GitTarget` names its branch | `gitBranchRef` is required; `gitProviderRef`, `branch`, `parentBranch` leave the target | -| Who may write a branch | One `GitBranch` per repository branch, and it must prove push access to claim it | -| Changing a branch's identity | `gitProviderRef`, `branch`, and `parentBranch` are immutable; replace the object instead | -| Deleting a `GitBranch` | Never deletes the branch on the remote | -| The Kubernetes connection | `KubeProvider`, namespaced, replacing `ClusterProvider` | -| Its credential | A flat `secretRef` to a kubeconfig Secret in the provider's namespace | -| The operator's own identity | An explicit `inCluster: {}` arm, honored only where the installation authorizes it | -| References between kinds | Explicit everywhere; no `default` name, no fallback | - -## The API after phase 2 - -```text - GitProvider (credential, allowedBranches, accessFrom) - ^ - | gitProviderRef (same namespace) - | -GitTarget --gitBranchRef--> GitBranch (repository branch: one per repo+branch, one branch worker) - | - +--kubeProviderRef--> KubeProvider (source cluster: kubeconfig or inCluster, accessFrom) - | - +--applyTriggerRef--> FluxTrigger / ArgoCDTrigger --kubeProviderRef--> KubeProvider -``` - -A `GitTarget` is a folder: what it mirrors, where in the branch it writes, how it places, prunes, -encrypts, and commits, and which applier it holds. Everything that is one fact per repository -branch lives on the `GitBranch`. Everything that is one fact per credential lives on the provider. - -| Field or status | Moves to | Why | -|---|---|---| -| `GitTarget.spec.gitProviderRef`, `.branch`, `.parentBranch` | `GitBranch.spec` | Two targets can no longer disagree about one branch | -| `GitTarget.status.remote` | `GitBranch.status.remote` | It describes the branch, so N targets carried N copies | -| `GitProvider.status.branches` | Removed | Listing `GitBranch` objects answers the same question | -| `GitProvider.spec.commit` (committer, signing) | Stays | The signing key belongs to the account the credential is for | -| `GitProvider.spec.allowedBranches` | Stays | It is the ceiling every `GitBranch` must fit under | -| `GitTarget.spec.commit.window`, `.commit.message`, `.suspend` | Stays | They describe the folder | - -## Phase 0: merge the shared `GitProvider` - -PR #428 keys the branch worker by repository and branch, adds `GitProvider.spec.accessFrom`, and -lets a `GitTarget` name a `GitProvider` in another namespace. Phase 1 builds on all three. Its one -known gap is that the oldest `GitTarget` decides which credential owns a branch, so any namespace -can squat a repository branch without a credential. Phase 1 closes that, so do not cut a release -from main between the two unless that gap is acceptable for it. - -## Phase 1: `GitBranch` - -### Shape - -```yaml -apiVersion: configbutler.ai/v1alpha3 -kind: GitBranch -metadata: - name: customer-main - namespace: platform-connections -spec: - gitProviderRef: - name: customer-config # a GitProvider in this namespace - branch: main # must match the provider's allowedBranches - parentBranch: develop # optional; only used to create a missing branch -status: - observedGeneration: 3 - conditions: [] # Ready, Reconciling, Stalled - repository: git.example.com/acme/config # normalized; half of the claim key - remote: - commit: 4f1c... - lastVerifiedAt: "2026-10-21T09:12:00Z" - verifiedBy: Push - parent: {} # unchanged shape, moved from GitTarget - gitTargetCount: 2 ---- -apiVersion: configbutler.ai/v1alpha3 -kind: GitTarget -metadata: - name: settings - namespace: tenant-a -spec: - gitBranchRef: - name: customer-main - namespace: platform-connections # omitted means the target's own namespace - path: tenants/a -``` - -The schema requires `gitProviderRef.name` and `branch`. CEL keeps all three spec fields immutable -(`self == oldSelf`), the rule `GitTarget` applies to these fields today. On the target, -`gitBranchRef` becomes immutable the same way, and `path` stays immutable. - -`gitProviderRef` is a `LocalObjectReference`. Creating a `GitBranch` therefore requires the right to -create objects in the namespace that holds the credential, and that is what makes a branch claim -an act of the credential's owner. A `GitBranch` has no `accessFrom` of its own: a `GitTarget` in -another namespace may reference it when the branch's `GitProvider` admits the target's namespace. -One policy governs both hops, so it cannot be split across two objects that disagree. - -### Who owns a repository branch - -The claim key is the provider's repository URL, as `internal/repourl` normalizes it, plus the -branch. The `GitBranch` UID holds the claim, so a recreated object with the same name starts -without it. - -1. A `GitBranch` becomes a candidate once its `GitProvider` is ready and its credential opens a - receive-pack session on the remote. On GitHub and GitLab that requires push permission, so a - provider with a missing or wrong Secret, or none on a public repository, never becomes a - candidate. On a host that advertises receive-pack anonymously this proof degrades to - reachability; the upgrade notes say so. -2. Among the candidates for one key, the oldest by `creationTimestamp` (then UID) holds the claim - and gets the branch worker. Every other `GitBranch` for that key reports `Stalled=True`, reason - `BranchConflict`, with a message naming the holder. -3. The holder keeps the claim through a later credential or network failure. It reports - `Ready=False` with the failure and does not hand the branch to another `GitBranch`. -4. Admission refuses a new `GitBranch` whose key a holder already claims. It reads `GitBranch` - status only and makes no Git call. An unproven `GitBranch` claims nothing, so creating one first - cannot block anyone at admission. -5. The claim lives in the manager's memory. After a restart it is recomputed by rules 1 and 2. A - persisted claim, and with it a holder that survives a restart while its credential is broken, - comes with the HA work, not here. - -This replaces `internal/git/branch_owner.go`. Its three callers (the reconciler's conflict check, -the worker sweep, and `EnsureWorker`) ask the claim instead of electing the oldest `GitTarget`. -`git.BranchKey` stays as it is. The worker's credential, signing key, and metric labels come from -the claiming `GitBranch`'s provider, so `provider_namespace` and `provider_name` keep their -meaning for existing dashboards. - -### References are checked in status, not at admission - -Admission refuses what is wrong with the object itself: a missing required field, a removed field, -an immutable field changed, and a duplicate claim from rule 4. It does not refuse a `GitTarget` -whose `GitBranch` does not exist yet. Flux and Argo CD apply a folder of manifests in one pass, in -no guaranteed order, and an admission rule that demands dependency order breaks the most common -way this API is installed. A missing or unauthorized reference is a status condition instead: - -| Object | Reason | When | -|---|---|---| -| `GitTarget` | `GitBranchNotFound` | The referenced `GitBranch` does not exist | -| `GitTarget` | `GitBranchNotReady` | It exists but holds no claim or is failing | -| `GitTarget` | `GitTargetNamespaceNotAuthorized` | The branch's `GitProvider` does not admit the target's namespace | -| `GitBranch` | `GitProviderNotFound`, `GitProviderNotReady` | Moved from `GitTarget` | -| `GitBranch` | `BranchNotAllowed` | `branch` is outside the provider's `allowedBranches`; moved from `GitTarget` | -| `GitBranch` | `ParentBranchNotFound`, `RecoveringParentBranch` | Moved from `GitTarget`, with their rules unchanged | -| `GitBranch` | `BranchConflict` | Another `GitBranch` holds the key (new) | - -Strict admission, which refuses a reference to an object that does not exist yet, was considered -and rejected for that reason. `BranchConflict` is a new reason; the others exist today and change -only which kind reports them. - -### Lifecycle - -A `GitBranch` carries a finalizer. Deletion stops new intake for every target on the branch. The -worker publishes or refuses each pending write, and each attached `CommitRequest` gets its outcome. -Then the finalizer is removed and the claim released. Nothing deletes the branch on the remote. - -Deleting a `GitBranch` that targets still reference is allowed; they report `GitBranchNotFound` -afterwards. A new `GitBranch` with the same name is a new UID and cannot pick up the old one's -pending work. - -### Removed from `GitTarget` - -`spec.gitProviderRef`, `spec.branch`, `spec.parentBranch`, and `status.remote` leave `GitTarget`. -The three spec fields stay in the schema only to be refused by name, with a message that names -`gitBranchRef`. `GitProvider.status.branches` is removed outright: it is status, so there is no -stored value for a user to be refused on. - -### Done when - -- Two `GitTarget`s in two namespaces reference one `GitBranch` and publish through one worker with - one credential, without copying the Secret. -- A `GitBranch` with a bad Secret created first does not stop a valid one created later from - claiming the branch. -- A second `GitBranch` for a claimed repository branch is refused at admission, also when it names - another `GitProvider` for the same repository. -- Two `GitBranch` objects for one key that both pass admission (a create race) never run two - workers; the younger reports `BranchConflict`. -- Setting a removed `GitTarget` field is refused with a message naming `gitBranchRef`. -- Deleting a `GitBranch` with pending writes gives each one an outcome before the finalizer goes. -- The parent-branch e2e specs pass with `parentBranch` on the `GitBranch`. -- The chart's quick start renders a `GitBranch`. - -## Phase 2: `KubeProvider` - -### Shape - -```yaml -apiVersion: configbutler.ai/v1alpha3 -kind: KubeProvider -metadata: - name: workspace-a - namespace: platform-connections -spec: - secretRef: - name: workspace-a-kubeconfig # a kubeconfig Secret in this namespace - key: value # optional; default "value", then "value.yaml" - accessFrom: - selector: - matchLabels: - platform.example.com/tenant: "true" - allowCrossNamespace: - sources: false - appliers: false - client: - qps: 20 - burst: 30 - attribution: - auditRoute: workspace-a ---- -apiVersion: configbutler.ai/v1alpha3 -kind: KubeProvider -metadata: - name: local - namespace: gitops-reverser -spec: - inCluster: {} -``` - -The schema requires exactly one of `secretRef` and `inCluster`. `client` is refused beside -`inCluster`; today it is silently ignored there. The kubeconfig safety checks (exec auth, insecure -TLS, file references) and their reasons stay as they are, because they describe the Secret's -contents. - -`KubeProvider` is a new kind, not `ClusterProvider` changing scope. Kubernetes cannot change an -existing CRD's scope in place, so a rename is also the cheapest way to make the break: the -`ClusterProvider` CRD is removed and `KubeProvider` is added beside it. Rule 1 of -[definitions.md](../definitions.md) then renames every `ClusterProvider*` reason and identifier, -with no alias left behind. - -### Behavior that changes - -- **Secrets resolve in the provider's namespace.** Today they resolve in the operator's namespace. - A tenant's connection keeps its credential with the tenant. -- **`inCluster` needs the installation's consent.** An installation flag lists the exact - `namespace/name` of each `KubeProvider` allowed to use the operator's own identity. The chart sets - it for the provider it creates. A `KubeProvider` with `inCluster` that is not on the list reports - `Stalled=True`, reason `InClusterNotAuthorized`, and is never used. There is no path from an - omitted, malformed, or forbidden credential to the operator's identity. -- **The own namespace is admitted.** Like `GitProvider`, a `KubeProvider` admits consumers in its - own namespace, and `accessFrom` admits the others. Today an empty `ClusterProvider.accessFrom` - admits nobody, because a cluster-scoped object has no own namespace. -- **References are required.** `kubeProviderRef` is a required `NamespacedObjectReference` on - `GitTarget`, `FluxTrigger`, and `ArgoCDTrigger`; an omitted namespace means the referrer's own. - The provider named `default` that an omitted `clusterProviderRef` selects today goes away. -- **Audit routes cannot be borrowed across namespaces.** An omitted `auditRoute` defaults to - `.`, which cannot collide because a namespace name has no dots. An explicit - route may be shared by providers in one namespace (several connections to one API server). It is - held by one namespace: the one its first segment names, when a provider there carries it, and - otherwise the namespace of the oldest provider carrying it. A provider in another namespace that - carries it reports `Stalled=True` (`AuditRouteConflict`) and is not used. This is a status - condition rather than an admission refusal, for the reasons `where-validation-lives.md` gives for - every cross-object check: the webhook is optional and fail-open, and it cannot see a provider that - changes its route later. - -The `allowCrossNamespace` grants keep today's meaning. Narrower per-tenant grants are a later -design. - -### Done when - -- A tenant namespace mirrors a remote cluster through its own `KubeProvider` and Secret, and a - second tenant cannot reference it without `accessFrom`. -- A `KubeProvider` with `inCluster` outside the authorized list never reaches the API server with - the operator's identity. -- Two `KubeProvider`s with one name in two namespaces keep separate watches, caches, and - attribution facts, also after one is deleted and recreated. -- A remote-source `GitTarget` with a local Flux applier (gitops-api ask #34) still exercises the - apply window end to end. -- A provider carrying an explicit audit route another namespace holds reports - `AuditRouteConflict`, and nothing mirrors or applies through it. - -## Phase 3: one receiver for many repositories - -Gitops-api ask #30: a GitHub App has one webhook endpoint, and today every `GitProvider` carries its -own receiver with its own token. Replace `GitProvider.spec.receiver` with a namespaced -`GitReceiver` and a reference to it: - -```yaml -kind: GitReceiver -metadata: - name: github-app - namespace: platform-connections -spec: - type: github - secretRef: - name: github-app-webhook-token - accessFrom: - selector: {} ---- -kind: GitProvider -metadata: - name: customer-config - namespace: platform-connections -spec: - url: https://git.example.com/acme/config - receiverRef: - name: github-app -``` - -A delivery authenticates against the receiver's token, resolves the repository and branch from the -payload, and wakes each claiming `GitBranch` whose provider references that receiver, once. A -delivery only wakes; the worker reads the remote with the provider's credential to find out what -happened. Unknown repositories are counted and ignored. A self-service team gets its own endpoint by -creating a `GitReceiver` in its own namespace, so there is one mechanism, not two. - -The name avoids Flux's `Receiver` kind, which sits in the same clusters. - -Built with three corrections. **There is no reference and no `accessFrom`:** a delivery wakes every -`GitProvider`, in any namespace, whose `spec.url` names its repository. Once the receiver routes by -the payload's repository, a per-provider reference only narrowed a set the URL already narrows, and -cost a line of wiring per provider plus an admission in every namespace for a GitHub App. Waking -across namespaces grants nothing, because a woken worker reads the remote with its own credential; -it holds at most one pending look, and the `202` carries no count, so it says nothing about other -namespaces. The one cost left is availability: a token holder who keeps delivering can keep a -matching worker reading continuously. That is accepted for now; a per-worker cooldown with a -deferred follow-up would bound it, without bringing the reference back. Whether deliveries reach a -branch is `GitBranch.status.lastPushNoticeAt`, a fact rather than a condition about a reference. A -receiver whose token Secret is missing looks again every 30 seconds, because Secrets are not watched -and no delivery reaches it until its path is published. And a `generic-hmac` body must name its -repository, which it used to be allowed to omit, because a shared receiver cannot route one that -does not. - -## Not in these phases - -Each of these needs its own design when it is next on the list. None blocks phases 1 to 3. - -- **Folder and branch grants per namespace.** Until they exist, every namespace a `GitProvider` - admits may write any folder on any allowed branch. That suits trusted teams; it is not isolation - between tenants, and the docs must say so. -- **Two installations on one control cluster** (ask #28): disjoint namespace sets per installation, - with the cache, webhook, and status-writer consequences. -- **Durable pending writes and HA**: a persisted claim, fencing, and saves that survive a restart. - See the [HA plan](../future/ha-gittarget-distribution-plan.md). -- **Repository aliases**: URLs the normalizer cannot prove equal, such as vanity domains. -- **A branch-wide suspend**: target `suspend` covers today's need, and adding it later breaks - nothing. -- **The observability asks #31 to #33**: they are not about sharing and do not need this work. - -## Open questions per phase - -Phase 1: - -- Should `GitBranch` get printer columns for provider, branch, commit, and the holder's readiness? - [definitions.md](../definitions.md) rule 8 governs what a dependency projection may show. -- Is the receive-pack proof enough on the Git hosts the e2e suite covers, or does one of them need a - documented exception? - -Phase 2 (settled): the `inCluster` list is `--in-cluster-kube-providers` (chart value -`controllerManager.inClusterKubeProviders`), and the chart's provider is named `local`. - -Phase 3 (settled): routing is by URL alone, so a `GitReceiver` has no `accessFrom` and a -`GitProvider` no `gitReceiverRef` or `GitReceiverReady` condition (see the corrections above). diff --git a/docs/design/label-selection-follow-ups.md b/docs/design/label-selection-follow-ups.md index f32e0c98..d825875e 100644 --- a/docs/design/label-selection-follow-ups.md +++ b/docs/design/label-selection-follow-ups.md @@ -1,7 +1,7 @@ # Label selection: follow-ups > **design**: open. Nothing here blocks label selection from shipping. Item 1 is resolved. -> Index: [`../INDEX.md`](../INDEX.md). Parent: [`watches-labels-simplification.md`](watches-labels-simplification.md). +> Index: [`../INDEX.md`](../INDEX.md). Contract: [selecting objects by label](../configuration.md#selecting-objects-by-label-rulesobjectselector). > Checked: 2026-09-29, against `feat/watch-object-selector` (PR #399). The label-selection branch went through several review rounds before merge. Most findings were diff --git a/docs/design/metrics-observability-plan.md b/docs/design/metrics-observability-plan.md index 54fc721b..051b3b05 100644 --- a/docs/design/metrics-observability-plan.md +++ b/docs/design/metrics-observability-plan.md @@ -513,7 +513,7 @@ the same reason. The doc comment now says so, so a timestamp well in the past re as "measuring stopped". The field is also **renamed** `observedTime` → `lastChangedTime`, since renamed again to -`lastChangedAt` by [`vocabulary-cleanup.md`](vocabulary-cleanup.md). It behaved this way as soon as the +`lastChangedAt` by the vocabulary cleanup (#397). It behaved this way as soon as the restamping stopped, but the old name invited clients to read it as a freshness signal, and a behavioral change under an unchanged name is the kind a consumer discovers in production. The rename makes the break visible. `retainedDocuments`, `mode`, and the whole of `status.streams` are diff --git a/docs/design/one-writer-per-branch-plan.md b/docs/design/one-writer-per-branch-plan.md deleted file mode 100644 index dc9431ff..00000000 --- a/docs/design/one-writer-per-branch-plan.md +++ /dev/null @@ -1,173 +0,0 @@ -# One writer per branch: where it stands - -> **status**, 2026-10-23. This page was the plan of 2026-10-21. Most of it has shipped, unreleased, -> in #428, #429, #430 and #432. It now records what exists, how that differs from the plan, and what -> the plan asked for that is still missing. The contract itself is in -> [configuration.md](../configuration.md#sharing-connections-across-namespaces) and -> [architecture.md](../architecture.md); the phased API break that carried most of it is -> [gitbranch-and-kubeprovider-plan.md](gitbranch-and-kubeprovider-plan.md). - -## The direction it served - -GitOps Reverser is moving toward an event-sourced state machine per branch, with durable execution -([branch worker event model](branch-worker-event-model.md)). The plan judged every option against -five properties: - -| | Property | -|---|---| -| **E1** | One owner per branch: one loop orders every decision about a branch | -| **E2** | Every input is an event, in order, on that branch's stream | -| **E3** | Nothing waits by polling another owner | -| **E4** | Decisions are recorded as actions (pending writes) | -| **E5** | The journal key is the branch itself: canonical repository plus branch | - -In October 2026 the worker was keyed by `GitProvider` and branch, so two `GitProvider`s for one -repository gave a branch two workers ("siblings"). Siblings broke E1, E2, E3 and E5, and every -defect the #425 self-review found in the sibling flush came from that shape. - -The plan recommended option F (one shared `GitProvider` per repository, referenced across -namespaces, the Flux model) together with five rules. All five properties now hold. - -## What we have now - -### One worker per repository branch (rule 1): done, #428 - -- The worker key is [`BranchKey{Repository, Branch}`](../../internal/git/types.go), with the - repository as `repourl.Normalize` returns it. Same-host SSH and HTTPS spellings are one key. -- `apply_siblings.go` and `apply_sibling_flush.go` are deleted, and with them the shared flush - generation, the copied observations and the cross-worker source-group claims. Source groups are - local to the one worker. -- The worker keeps the owning provider for credential, committer, signing and metric labels. -- The HA plan's `BranchWriteShard` is this key - ([shard identity](../future/ha-gittarget-distribution-plan.md#shard-identity)). - -### One credential per repository branch (rule 2): done, in a stronger form, #429 - -The plan said: elect implicitly from the `GitTarget`s and refuse a branch written through two -`GitProvider`s. #428 built exactly that, with the oldest `GitTarget` winning. That left a squat gap: -any namespace could hold a branch without a working credential. #429 replaced the rule with an -explicit object: - -- **`GitBranch`** is one branch of the repository its `GitProvider` names. It holds the branch for - every `GitTarget` that references it: one worker, one clone, one credential, one signing key. -- **The branch claim**, in [`branch_owner.go`](../../internal/git/branch_owner.go): a `GitBranch` - becomes a candidate only once its credential opens a receive-pack session. The oldest candidate - holds the branch. The holder keeps it through later proof failures. The claim is in memory and is - recomputed after a restart. -- A younger `GitBranch` for a held branch reports `Stalled=True`, reason `BranchConflict`, naming - the holder. Status carries this; the plan's admission refusal was dropped - so that a folder of manifests can be applied in any order. -- `GitBranch.spec.gitProviderRef` was a `LocalObjectReference`, so creating a claim took the right - to create objects where the credential lives. Since #441 it may name a `GitProvider` in another - namespace, and the right to claim comes from that provider's `spec.allowedGitBranches` instead. - -So option B's per-write credential selection never had to exist, which was the purpose of rule 2. - -### The shared credential (rule 3, option F): done, with a different reference, #428 + #429 - -- #428 shipped `GitProvider.spec.accessFrom`; #441 replaced it with explicit grants, and nothing is - admitted automatically, the object's own namespace included. A `GitProvider`'s - `spec.allowedGitBranches` decide which namespaces may claim which branches through it, and a - `GitBranch`'s `spec.allowedGitTargets` decide which namespaces may write through it. -- The cross-namespace reference is **`GitTarget.spec.gitBranchRef`** (namespaced), as well as - `gitProviderRef` since #441. Each hop is checked against the grants of the object it reaches - ([`gitBranchAdmitted`](../../internal/authz/kubeprovider_admission.go), - [`GitBranchClaimAdmitted`](../../internal/authz/kubeprovider_admission.go)). This is checked on - every reconcile, so a grant that is narrowed later revokes access. -- One Secret in the platform namespace serves every team. Self-service stays: a team can still own - a `GitProvider`, a `GitBranch` and a Secret in its own namespace. -- Committer and signing come with the credential, so a branch is signed by one key. - -### The decoupled receiver (rule 5, gitops-api #30): done, #432 - -The plan's text was one endpoint, routed by repository and branch, with a `GitProvider`'s own -receiver kept for self-service. What shipped is simpler: - -- `GitReceiver` is a namespaced kind. `GitProvider.spec.receiver` is gone. -- A verified delivery wakes the worker of **every** `GitProvider`, in any namespace, whose - `spec.url` names the pushed repository, for that branch only. It has no reference and no - `accessFrom`. -- Waking across namespaces grants nothing: the worker reads the remote with its own credential and - holds at most one pending push notice. The `202` has no body. -- For self-service, a team creates a `GitReceiver` in its own namespace. -- `GitBranch.status.lastPushNoticeAt` shows whether deliveries arrive. - -### Decided differently from the plan - -| Plan | What we did | Why | -|---|---|---| -| Report `status.branches` to every routed `GitProvider` | Removed; `GitBranch` status replaces it (`status.remote`, `gitTargetCount`) | Listing `GitBranch` objects answers the question | -| Move metric labels to the repository-branch key (breaking) | Kept `{provider_namespace, provider_name, branch}`, taken from the holding `GitBranch`'s provider | gitops-api dashboards; the label set still names one branch worker | -| Refuse a second `GitProvider` for one repository in one namespace, before anything else | Not built; no longer needed | The claim is per `GitBranch`, so extra providers on one repository are harmless, and useful (see "Still missing", item 1) | -| Ship additively, then the key change a release later | One release, hard cuts, no migration | No users yet | -| C2 (`ClusterGitProvider` or `GitCredential`) only if one object per repository is too many | Not built | Nobody has hit the limit | - -## Still missing against the original - -Ordered by how much they matter now. - -1. **Branch-scoped access ("who may write what").** The plan's rule 3 and decision 3 wanted - `accessFrom` to say which namespaces may write *which branches*, optionally *which paths*. What - shipped is namespace-only and provider-wide: every namespace a `GitProvider` admits may use - every `GitBranch` of that provider, and write any folder that the overlap check does not already - hold. Branch isolation is possible today without code: create one `GitProvider` per team in the - platform namespace. Every one of them references the same Secret, with - `allowedBranches: [teams/team-a/*]` and `accessFrom: {names: [team-a]}`. Path isolation is not - possible at all. [gitbranch-and-kubeprovider-plan.md](gitbranch-and-kubeprovider-plan.md) - parks this as "Folder and branch grants per namespace", and - [who may use a provider](../finished/provider-access-rules.md) built it as grants - (`allowedGitBranches`, `GitBranch.allowedGitTargets`), and also covers item 2 below. - -2. **The claim is weaker than "one credential per branch, enforced".** Three gaps: - - **The push-session proof proves less on some hosts.** On Gitea and Forgejo, a credential that - can only *read* opens a receive-pack session (the AGit flow). On a host that offers push - sessions anonymously, it proves only that the repository is reachable. - - **Who claims first holds the branch.** Anyone who can create a `GitProvider` and a `GitBranch` - with a credential that passes the proof can claim a branch that no older `GitBranch` holds. - That includes a tenant in its own namespace with a copy of the token. The plan's rule 2 tied - the branch to the shared provider. The claim instead ties it to the oldest proven object on - that repository, wherever it lives. - - **An older `GitBranch` takes the branch from a live holder** once it proves itself, and after - a restart a holder whose credential broke loses its claim. A persisted claim belongs to the - HA work. - - Only a per-repository authority closes these: option C's object per repository URL, or a - cluster-level rule naming the one provider through which a repository's branches may be - claimed. Nothing of that exists. - -3. **Alias detection (rule 4, option E).** Not built. `repourl.Normalize` still cannot unify Gitea's - SSH on `:2222` beside HTTPS, a separate SSH hostname, a vanity domain, or a renamed repository. - Two such URLs get two keys, two claims and two workers on one branch: the sibling problem, - undetected. The HA plan's HA-0 question about aliases is still open. The design is unchanged: - a warning when a commit we pushed under one key shows up under the other, in both directions. - -4. **The #425 self-review items that are not about siblings.** R2 and R3 went with the siblings, - and R5 (the `UPGRADING.md` entries) is done. Reading the code, R1 and R4 still look open. - - **R1:** an applier that joins a flush already in progress starts `flushEdits` at the moment it - joins, not at the flush mark - ([`flushed`](../../internal/git/applier_claim.go)). - - **R4:** `forgetSettledSnapshots` still runs only on `GitTarget` teardown, so a replaced worker - can read the old worker's settled-snapshot record. - - Both affect a user with a single `GitProvider`. Both are tracked in - [TODO.md](../TODO.md#before-the-next-release). - -5. **A rate bound on deliveries.** Folding bounds the queue but not the rate, so a token holder can - keep a worker reading continuously. This is accepted for now; a per-worker cooldown would bound - it. - -6. **Per-tenant identity on the Git host.** This is by design. A shared provider - pushes as the platform's account, so the host's audit log names the platform instead of the - team, and one team's credential cannot be revoked separately. A team that needs either keeps its - own provider. The plan named this as Flux's argument for an identity per object and accepted the - trade. - -## Stale text elsewhere - -- [HA plan, shard identity](../future/ha-gittarget-distribution-plan.md#shard-identity) says "one - `GitProvider` owns a repository branch: a `GitTarget` writing it through another one is - refused". Since #429 it is the holding `GitBranch`, and a younger `GitBranch` reports - `BranchConflict`. -- [gitbranch-and-kubeprovider-plan.md](gitbranch-and-kubeprovider-plan.md) still draws - `FluxTrigger`/`ArgoCDTrigger` and `applyTriggerRef` (now `FluxClaim`/`ArgoCDClaim` and - `applierClaimRef`, #438). Its status line says phase 3 is still to come, but it shipped in #432. diff --git a/docs/design/open-asks-priority.md b/docs/design/open-asks-priority.md index abbaa346..7625780a 100644 --- a/docs/design/open-asks-priority.md +++ b/docs/design/open-asks-priority.md @@ -42,7 +42,7 @@ > 2026-07-28, which is the revision that filed #23), the API-surface block left unbuilt by the > status and configuration-model review — now sequenced in > `gittarget-api-wave.md` (removed) — and the config-surface proposal in -> [`config-surface-for-a-structured-repository.md`](../future/config-surface-for-a-structured-repository.md) +> `config-surface-for-a-structured-repository.md` (removed) > (B1–B6). This page merges them into one queue and says where we deliberately do **not** do > what was asked. > diff --git a/docs/design/push-cooldown.md b/docs/design/push-cooldown.md index 44d98721..bb35a4bb 100644 --- a/docs/design/push-cooldown.md +++ b/docs/design/push-cooldown.md @@ -1,170 +1,159 @@ # The push cooldown: what it still buys, and what removing it would cost > **design**: open. Option C (§7), the failure backoff, is built in -> [`retry.go`](../../internal/git/retry.go), now one schedule with parent recovery's probe; the success cooldown is -> unchanged. Option D needs measurement; option E remains rejected without that evidence. -> Index: [`../INDEX.md`](../INDEX.md). Reviewed at `373bf8d7` on 2026-10-03. +> [`retry.go`](../../internal/git/retry.go) (#412), one schedule with parent recovery's probe since +> #413. The success cooldown is unchanged. Concurrent commit windows (#467, #476) changed where it +> engages (§3). Option D needs the measurements in §9; option E stays rejected without them. +> Index: [`../INDEX.md`](../INDEX.md). Reviewed at `11b8b4e0` on 2026-11-10. > Related: [`../api-first-publication.md`](../api-first-publication.md), -> [`push-notification-and-reconcile-trigger.md`](push-notification-and-reconcile-trigger.md), +> [`concurrent-author-windows.md`](concurrent-author-windows.md), > [`../spec/commit-window-refactor.md`](../spec/commit-window-refactor.md) -**The question.** `PushCooldown` predates the commit window. The window now groups an editing burst -into one commit, which is most of what the cooldown was invented to do. Is the cooldown still -earning its place, and would removing it simplify the worker? +**The question.** `PushCooldown` predates the commit window. The window groups an editing burst into +one commit, which is most of what the cooldown was invented to do. Is the cooldown still earning its +place, and would removing it simplify the worker? -**The short answer.** For a single author writing a single target, ordinary timer closures at the -defaults already space commits enough to avoid the success cooldown. Identity changes (§3), saves -with shorter timers, and buffer-limit closures can produce commits sooner. The cooldown batches -those commits when they arrive within five seconds of a successful push. +**The short answer.** One window closing on its own idle timer is already spaced past the cooldown +(§2). The cooldown batches the commits that land close together anyway: windows closed early (a +write of another `GitTarget`, an overlapping or unproven footprint, the buffer limit, +`idleTimeout: 0s`), and windows open side by side whose idle timers run out within seconds of each +other (§3). Removing the success wait would remove `lastPushAt`, `pushTimer` and their scheduling +branch; the retry timer and the retained-write machinery stay. Whether that is worth the extra +publications is the measured decision in §9. -Removing the success wait would remove `lastPushAt`, `pushTimer`, and their scheduling branches. -The separate retry timer and retained-write recovery would remain. +## Outage handling is not the cooldown's job -Option C is built in #412. The remaining decision is whether changing the success cooldown earns -its extra publication cost; that needs the measurements in §9. - -## An outage pause is a separate decision - -Keep the success cooldown unchanged while completing the -[branch-worker recovery contract](gittarget-branch-worker-pending-writes.md#recovery-contract). Failure backoff -paces attempts at Git; capacity admission controls how much new work the process accepts. Neither -one supplies storage durability. - -The proposed saturation behavior pauses intake for the affected worker, retains accepted entries, -and uses the existing retry deadline to attempt recovery. A successful read alone cannot reopen -intake when pushes still fail. Producers resume after the accepted backlog settles, using their -cursor or a fresh scoped snapshot if history expired. This does not require another cooldown, -a manager restart, or Redis. All targets sharing that worker share the pause; other workers run. +Failure backoff paces attempts at Git (`retry.go`); capacity admission pauses intake for the +affected worker and resumes producers once the accepted backlog settles +([recovery contract](gittarget-branch-worker-pending-writes.md#recovery-contract)). Both are built. +Neither needed the success cooldown, and removing it would close none of their gaps. Neither one +supplies storage durability: that is the HA plan's. ## 1. What it is today -`PushCooldown` is a fixed `5s` per worker. `maybeSchedulePush` runs after every local commit: +`PushCooldown` is a fixed `5s` per worker. `maybeSchedulePush` +([`branch_worker.go`](../../internal/git/branch_worker.go)) runs after every local commit: ```go -if len(l.pendingWrites) == 0 { return } -if l.awaitingRetry() { return } // a failed attempt has its own deadline -if l.lastPushAt.IsZero() || l.publishesAtOnce() { // never pushed, or an apply waits: go now - l.pushPending(); return +if len(l.pendingWrites) == 0 || l.awaitingRetry() { return } // a failed attempt has its own deadline +if l.lastPushAt.IsZero() || (l.flushing && l.pendingUpTo(l.flushMark)) { + l.pushPending(); return // never pushed, or an apply waits: go now +} +if !l.cooldownHoldsSaves && l.pendingCarriesCommitRequest() { + l.pushPending(); return // a save waits: go now } -if l.pendingCarriesCommitRequest() { l.pushPending(); return } // a save waits: go now -if time.Since(l.lastPushAt) >= PushCooldown { l.pushPending(); return } -if l.pushTimer == nil { l.pushTimer = time.NewTimer(...) } // otherwise wait out the remainder +if elapsed := now - l.lastPushAt; elapsed >= PushCooldown { l.pushPending(); return } +if l.pushTimer == nil { l.pushTimer = time.NewTimer(PushCooldown - elapsed) } // wait out the rest ``` `lastPushAt` advances **only on a successful push**, so the cooldown paces successful publication -and does not pace retries. The publication backoff does that separately. Parent recovery owns -its attempts while active, and new commits wait for its deadline; see the -[worker event model](branch-worker-event-model.md). A flush for an apply skips the cooldown: the -writes up to the flush mark are pushed at once (`publishesAtOnce`), because the apply waits for -them ([the apply trigger](apply-trigger.md#flush-then-apply)). A push that a `CommitRequest` waits on -skips it too: a person waits for the save, and an author has one save in progress at a time, so -saves cannot make the burst the cooldown exists to batch. Plain writes keep it. +and never retries; the retry schedule does that. Two pushes skip it: -## 2. Ordinary timer closures already space a single identity +- **An apply flush.** The writes up to the flush mark go at once, because the apply waits for them + ([the apply trigger](apply-trigger.md#flush-then-apply)). +- **A save.** A person waits for a `CommitRequest`, and an author has one save in progress at a time, + so saves cannot make the burst the cooldown exists to batch. `cooldownHoldsSaves` is a test seam + that keeps a save behind the cooldown. -`DefaultCommitWindow` and `PushCooldown` are both `5s`. The window is a rolling **silence** timer -per `(author, GitTarget)`. With no save or other early closure, a new window opens only after the -previous synchronous push returns, so its silence deadline already falls outside the cooldown: +Plain writes keep it. + +## 2. One window closing on its own timer is already spaced + +A window is per `(author, GitTarget)` and closes after `spec.commit.window.idleTimeout` of silence +(default `5s`, `DefaultCommitWindow`) or at `maxDuration` (default `1m`). With no early closure, a +writer's next window opens only after the previous commit, so its idle deadline already falls +outside the cooldown: ```mermaid sequenceDiagram participant E as Events - participant W as Window (5s silence) + participant W as Window (5s idle) participant P as Push Note over E,P: One author, one target, both timers at 5s E->>W: edit at t=0 - Note over W: silence deadline t=5 - W->>P: commit finalized at t=5 + Note over W: idle deadline t=5 + W->>P: commit at t=5 P->>P: push at t=5, cooldown runs to t=10 E->>W: next edit at t=5+e - Note over W: a NEW window opens
its silence deadline is t=10+e - W->>P: commit finalized at t=10+e - Note over P: cooldown expired at t=10
nothing was ever delayed, nothing was ever batched + Note over W: a NEW window opens
its idle deadline is t=10+e + W->>P: commit at t=10+e + Note over P: cooldown expired at t=10
nothing was delayed, nothing was batched ``` -The next commit for the same identity cannot finalize until at least `window` after the previous -one, and the cooldown is measured from the push that followed that previous commit. When -`window >= cooldown`, the cooldown has always expired before there is anything to hold. +When `idleTimeout >= PushCooldown`, the cooldown has always expired for that writer before there is +anything to hold. A shorter `idleTimeout`, an early closure, or other writers on the branch can make +it engage. -Shorter window timers, early closures, and several identities on a branch can all make the -cooldown engage. Equal default durations alone do not establish that every single-author workload -avoids it. +## 3. Where it still batches -## 3. Where it does still work, and it is the normal case +Since #467 a worker holds a window per author side by side, as long as every footprint is proven and +none overlaps ([`window_admission.go`](../../internal/git/window_admission.go), +[concurrent-author-windows.md](concurrent-author-windows.md)). Two authors on one `GitTarget` +editing different files no longer close each other's windows. Before #467 they did, on every +alternation, and the cooldown was then the only batching left. That case is gone; these remain. -There is exactly **one** open window per worker. An event that does not belong to it does not open -a second window: it **finalizes the open one immediately** and a new one opens in its place -([`branch_worker.go`](../../internal/git/branch_worker.go), `canAppend` and -`windowFinalizeReasonIdentityChange`). +**Early closes.** A write still closes other windows first, so their commits land before its own, +when it cannot sit beside them: + +| Source | Close reason | +| --- | --- | +| A write of another `GitTarget` on the same branch (per-target encryption and bootstrap stay serial) | `author-or-target-change` | +| A write, or an open window, whose footprint cannot be proven | `author-or-target-change` | +| A worker that fell back to one window at a time | `author-or-target-change`, after `footprint-fallback` | +| Overlapping footprints | `path-conflict` | +| The retained-byte budget | `buffer-limit` | +| `commit.window.idleTimeout: 0s` | every event commits on arrival | +| Resync snapshots and request records | they commit outside the windows | -That has a consequence worth stating plainly: **under alternating authors the silence timer never -fires at all.** Every commit is produced by the identity boundary, at the moment the next event -arrives. The window provides no spacing whatsoever, and the cooldown becomes the only thing -batching anything. +Several `GitTarget`s sharing a branch is the multi-tenant shape, and there every switch between +targets is a close: ```mermaid sequenceDiagram participant E as Events - participant W as The one open window + participant W as Windows participant P as Push - Note over E,P: Alternating authors on one branch
window 5s, cooldown 5s, previous push finished at t=0 + Note over E,P: Targets A and B on one branch
idle 5s, cooldown 5s, previous push finished at t=0 - E->>W: t=1 Alice edits - Note over W: window opens for Alice - E->>W: t=2 Bob edits - Note over W: identity change: Alice's window
finalizes NOW, not at t=6 + E->>W: t=1 edit for A + Note over W: window opens for A + E->>W: t=2 edit for B + Note over W: another GitTarget: A's window
closes NOW, not at t=6 W->>P: commit 1 at t=2 Note over P: cooldown active, timer armed for t=5 - E->>W: t=3 Alice edits + E->>W: t=3 edit for A W->>P: commit 2 at t=3 - E->>W: t=4 Bob edits + E->>W: t=4 edit for B W->>P: commit 3 at t=4 P->>P: t=5: one push carries all three Note over E,P: 2 requests. Without the cooldown: 6 ``` -The same shape applies to several `GitTarget`s sharing a branch, and to an unattributed event -arriving against an attributed window (a `/status` change with no audit fact is the common cause, -which is why that log line exists). +**Side-by-side windows closing together.** Concurrent windows add a case of their own. Three +authors who stop editing at about the same time have three idle deadlines a second or two apart, so +three commits. Commit order is close order. The first push goes at once, and the cooldown carries +the other two in one push. -| Source | Why the window does not space it | -| --- | --- | -| An author or target change | The identity boundary finalizes the open window on arrival of the next event | -| Several `GitTarget`s on one branch | One window, so every switch between them is an identity change | -| `commit.window.idleTimeout: 0s` | Every event commits on arrival | -| Atomic writes | They bypass the window by contract | -| Resync snapshots | They finalize and commit outside the window | - -Ledger row 4 prices the batching: three commits published together cost **2** HTTP requests. Three -separate publications would cost **6**. - -**This matters more than the first draft of this page assumed.** An earlier version drew two -concurrent windows and had Alice's commit waiting for her silence timer, which is not how the -worker behaves. Once the mechanism is right, the multi-identity case is not an edge: a multi-user -cluster with attribution enabled, the configuration this product is built for, produces one commit -per identity change, which is close to one commit per event. +Ledger row 4 prices the batching: three commits published together cost **2** HTTP requests; three +separate publications cost **6**. ## 4. What removing the success cooldown would simplify -This is the part worth being precise about, because the hope ("it would clean up a lot of state") -is half right. +The hope ("it would clean up a lot of state") is half right. **Goes away:** | Item | Size | | --- | --- | -| `lastPushAt` field | 1 field | -| Successful-push wait in `maybeSchedulePush` | The elapsed-time check and cooldown scheduling | -| `pushTimer` and its select arm | Success-cooldown scheduling only since step 3b | -| `PushCooldown` constant and its documentation | 1 constant, several doc paragraphs | - -The failure backoff uses its own timer in `retrySchedule`. The worker currently has six timer -sources: window, success push, attach, refusal action, applier, and retry. Parent recovery uses the -retry deadline. Removing the success cooldown would leave five sources. +| `lastPushAt` | 1 field | +| The elapsed-time branch in `maybeSchedulePush` | a few lines | +| `pushTimer`, `pushTimerAt` and their select arm | success-cooldown scheduling only | +| `PushCooldown` and its documentation | 1 constant, several doc paragraphs | +| `cooldownHoldsSaves` | a test seam | **Stays exactly as it is:** @@ -173,8 +162,8 @@ retry deadline. Removing the success cooldown would leave five sources. | The retry schedule (`retry.go`) | A failed publication still needs a scheduled attempt | | `pendingWrites` retention | A push can fail or be rejected; the writes must survive to be replayed | | `baseTrusted` | The head-of-cycle fetch decision, unrelated to push cadence | -| `checkoutApplied` | A write that failed part-way, or a reset that discarded local commits, unrelated to push cadence | -| `materialize`, `refreshRemoteAndRebuildPendingWrites`, the whole replay path | Contention recovery | +| `checkoutApplied` | A write that failed part-way, or a reset that discarded local commits | +| `materialize`, `refreshRemoteAndRebuildPendingWrites`, the replay path | Contention recovery | ```mermaid flowchart TD @@ -200,57 +189,40 @@ cooldown does not touch it.** Every defect found in PR #382 lived there. ## 5. The one thing it does change -Today, "locally committed but not yet pushed" is the **normal** state: every burst passes through -it. Without the cooldown it becomes an **exceptional** state, reached only when a push fails or is -rejected. - -That cuts both ways, and neither direction is decisive: +Today "committed locally but not yet pushed" is a **normal** state: any burst of closes passes +through it. Without the cooldown it becomes an **exceptional** state, reached only when a push fails +or is rejected. That cuts both ways: - **For removal.** A narrower window in which the retained-write paths can bite in production. -- **Against removal.** Those paths do not disappear, they get exercised far less. Code that is - only reached on failure is code whose bugs are found later. The five defects PR #382 fixed were - all in paths the cooldown makes routine. +- **Against removal.** Those paths do not disappear; they run less, so their bugs are found later. + The five defects PR #382 fixed were all in paths the cooldown makes routine. -Tests use `loop.lastPushAt = time.Now()` to hold the push back so they can inspect retained state. -Removing the cooldown requires another way to reach that state without changing the failure path -each test exercises. +Tests set `loop.lastPushAt` to hold a push back and inspect retained state. Removing the cooldown +needs another way to reach that state without changing the failure path each test exercises. ## 6. Serialization limits concurrency, but does not batch queued commits -The branch worker is a single goroutine, and Git operations run synchronously on it. While a push -is in flight, arriving events sit in the queue. After it returns, each dequeued item runs its -handler, which can finalize a window and call `maybeSchedulePush` before the next item is read. - -Without the success wait, queued alternating identities or zero-window writes can therefore each -cause a push. Serialization limits concurrent Git work to one operation; it does not automatically -combine queued commits into one publication. The cooldown can batch both a slow trickle and a -backlog that drains after a slow push. +The branch worker is one goroutine, and Git operations run on it synchronously. While a push is in +flight, arriving events wait in the queue. After it returns, each dequeued item runs its handler, +which can close a window and call `maybeSchedulePush` before the next item is read. Without the +success wait, a backlog of early closes drained after a slow push would each cause a push. +Serialization keeps Git work to one operation at a time; it does not combine queued commits. ## 7. Options | Option | What it is | Complexity | Verdict | | --- | --- | --- | --- | | **A. Keep the success cooldown** | Preserve successful-push spacing | Unchanged | Current default alongside C | -| **B. Delete the success cooldown** | Push after every commit unless failure backoff applies | Removes `lastPushAt` and `pushTimer`; keeps the retry timer | §3 can become one push per identity change | -| **C. Add a failure backoff, keep the cooldown** | Schedule failed publications independently of new writes | One retry timer shared with parent recovery | Built in #412; schedule unified in step 3b of #413 | -| **D. Add the backoff AND shorten or drop the success cooldown** | C, plus reducing the `5s` wait once §9 has priced it | Depends on the outcome | The measured follow-up to C | -| **E. Make it conditional or configurable** | Engage only when more than one identity is active, or expose it on `GitProvider` | **+complexity, +API surface** | Rejected unless D measurably fails | - -### Why C is the one to take first, and why it is not what this page started out recommending - -An earlier draft of this page recommended removing the wait on success and replacing it with a -failure backoff, on the strength of the cooldown being inert. §3's correction undermines the first -half of that: the cooldown is inert only while one identity is writing, and it is load-bearing as -soon as two are. Dropping the success wait is therefore a **measured** decision, not an obvious one, -and it belongs in option D behind the numbers §9 asks for. - -Before #412, `pushPending` stopped its timer on failure and armed no replacement outside parent -recovery. A branch with no new arrivals retained its work indefinitely, while continued arrivals could provoke -a failed push per commit because `lastPushAt` advances only on success. #412 adds the independent -failure backoff. Step 3b then unified the publication and parent-recovery deadlines. While parent -recovery is open, the due attempt services it. The following diagram describes publication retries; -the linked recovery contract adds admission pause and producer resume. Resync fetches can still -bypass backoff today; step 5b closes that gap. +| **B. Delete the success cooldown** | Push after every commit unless the retry schedule applies | Removes `lastPushAt` and `pushTimer` | §3 becomes one push per early close | +| **C. Add a failure backoff, keep the cooldown** | Schedule failed publications independently of new writes | One retry timer shared with parent recovery | Built in #412; unified with parent recovery in #413 | +| **D. C, then shorten or drop the success cooldown** | Reduce the `5s` wait once §9 has priced it | Depends on the outcome | The measured follow-up to C | +| **E. Make it conditional or configurable** | Engage only with several windows or targets, or expose it on `GitProvider` | **+complexity, +API surface** | Rejected unless D measurably fails | + +C came first because the retry gap was a defect. Before #412, `pushPending` stopped its timer on +failure and armed no replacement outside parent recovery, so a quiet branch kept its work +indefinitely, while continued arrivals each provoked a failed push. Dropping the success wait was +never the fix for that, and it is not an obvious simplification either: §3 is where it is +load-bearing. ```mermaid stateDiagram-v2 @@ -269,60 +241,39 @@ stateDiagram-v2 end note ``` -## 8. Risks +## 8. Risks of removing it Ordered by how much they should worry you. -1. **Request amplification on shared branches.** The architecture explicitly supports several - `GitTarget`s per branch, and multi-tenant installs are the case where the cooldown is load-bearing. - Option B turns row 4's `2` into `6`. This contradicts the premise of - [`push-notification-and-reconcile-trigger.md`](push-notification-and-reconcile-trigger.md), - which spent a whole change removing two requests per publication. -2. **Hosted Git rate limits.** With serialization the bound is one push per push-duration, which on - a fast remote is several per second sustained. Measure throttling on the supported hosts before - increasing publication frequency. -3. **`commit.window.idleTimeout: 0s` becomes a push per event.** On a healthy remote the cooldown - spaces publications even with that setting. Anyone who set `0s` for prompt commits did not - necessarily ask for prompt *pushes*. -4. **Losing a well-exercised path.** §5. The retained-write machinery stays; its production - exposure shrinks; its bug-discovery rate shrinks with it. -5. **Test migration.** Tests use `lastPushAt` to reach retained state. They need a replacement - lever, and a careless one (for example, stubbing `pushAtomicFn` to fail) changes what is being - tested. -6. **A latency expectation somebody may already rely on.** Removing the cooldown makes publication - up to five seconds faster, including for `CommitRequest`. That is a user-visible improvement, but - it is a behavior change on a path people write automation against. +1. **Request amplification on shared branches.** Several `GitTarget`s per branch is supported, and + that is where the cooldown is load-bearing. Option B turns row 4's `2` into `6`. +2. **Hosted Git rate limits.** With serialization the bound is one push per push duration, several + per second on a fast remote. Measure throttling on the supported hosts first. +3. **`commit.window.idleTimeout: 0s` becomes a push per event.** Someone who set `0s` for prompt + commits did not necessarily ask for prompt *pushes*. +4. **Losing a well-exercised path.** §5. +5. **Test migration.** Tests use `lastPushAt` to reach retained state; a careless replacement (for + example, stubbing `pushAtomicFn` to fail) changes what is tested. +6. **A latency expectation.** Plain writes would publish up to five seconds sooner. Saves and apply + flushes already skip the cooldown, so the visible change is smaller than it was. ## 9. What to measure before deciding -The ledger built in PR #382 answers most of this, and turns the argument into numbers: +Extend the event-loop ledger from PR #382, which counts connections per scheduled workload: -| # | Operation to add to the ledger | What it settles | +| # | Workload | What it settles | | --- | --- | --- | -| 1 | One author, one target, a burst at default window, with no early closure | Confirms §2 for ordinary timer closures | -| 2 | Two authors alternating on one branch | Prices the shared-branch identity changes described in §3 | +| 1 | One author, one target, a burst at the default window, no early close | Confirms §2 | +| 2 | Three authors on one target, disjoint files, stopping together | Prices side-by-side windows closing together | | 3 | Three `GitTarget`s on one branch, edited together | The multi-tenant bill for option B | -| 4 | `commit.window.idleTimeout: 0s`, ten events | Risk 3, as a number | -| 5 | A failing remote with continued arrivals, including parent recovery | Confirms retry pacing, including across the parent-recovery handoff | - -Rows 1 to 4 need connection counts driven through the event loop because scheduling determines -how many publication cycles occur. Extend the existing event-loop fixtures, including #412's -held-request retry test, to measure those workloads. +| 4 | Two authors on overlapping files (`path-conflict`) | Prices the early closes that remain on one target | +| 5 | `commit.window.idleTimeout: 0s`, ten events | Risk 3, as a number | +| 6 | A failing remote with continued arrivals, across a parent-recovery handoff | Confirms retry pacing | ## 10. Recommendation -1. **Keep the success cooldown while evaluating its cost.** Changing publication cadence needs - a separate change and its own validation. -2. **Option C's retry schedule is built.** Step 3b unified it with parent recovery. The outage work - that followed it (admission correctness, bounded Git calls, pause/resume, and visible status) is - built too, in the branch-worker pending-writes plan. Removing the success cooldown would not have - closed any of those gaps. -3. **Build the event-loop ledger rows in §9 before changing the success wait.** Retry tests prove - progress, but the ledger still needs the cost of these scheduled workloads. Row 2 prices the - multi-identity case. -4. **Only then consider option D.** If the numbers show the multi-identity case is rare in practice - or the amplification is small, shortening or dropping the success cooldown becomes defensible. - If they show what §3 predicts, keep it and the question is settled with evidence. - -Removing the success wait keeps the retry timer and retained-write machinery. Its benefit must -therefore come from measured save latency, weighed against the additional publications. +1. **Keep the success cooldown** until the §9 rows exist. Changing publication cadence is its own + change with its own validation. +2. **Then consider option D.** If the numbers show early closes and clustered closes are rare or + cheap, shortening or dropping the wait is defensible. If they show what §3 predicts, keep it, and + the question is settled with evidence. diff --git a/docs/design/reconcile-triggering.md b/docs/design/reconcile-triggering.md deleted file mode 100644 index 17a89254..00000000 --- a/docs/design/reconcile-triggering.md +++ /dev/null @@ -1,479 +0,0 @@ -# Reconcile triggering — how our controllers wake up - -> **design**: open. §5 is superseded by [the apply trigger](apply-trigger.md); F1 is built for -> `GitTarget` (`reconcile.configbutler.ai/requestedAt`), and F4 is built. -> Index: [`../INDEX.md`](../INDEX.md) -> Date: 2026-06-29. - -A controller is only as good as the events that wake it. Periodic requeue is a -**safety net**, not a mechanism: if state changes and nothing enqueues the owner, -the change is invisible until the next periodic tick — up to 10 minutes here. This -document inventories how every controller is currently triggered, records the -**fix already shipped** for the GitPath case, lists the **cases still suffering**, -and lays out the **trigger mechanisms** (grounded in Flux's proven patterns) that -turn this into a responsive controller. - -Facts first, then the fix, then the gaps, then the toolbox. - ---- - -## 1. Facts: the current trigger inventory - -### 1.1 Requeue intervals (`internal/controller/constants.go`) - -| Constant | Value | -|---|---| -| `RequeueStreamSettleInterval` | 10 s | -| `RequeueSteadyInterval` | 5 min | - -> **Update (2026-07-07, secret-value-retention):** the former `RequeueShortInterval` -> (2 min), `RequeueMediumInterval` (5 min), and `RequeueLongInterval` (10 min) collapsed -> into a single 5-minute `RequeueSteadyInterval`. The control plane no longer watches -> Secrets, so out-of-band credential/age-key changes are picked up on this unified steady -> cadence instead of via a Secret informer. Where the narrative below says "up to 10 -> minutes," read 5 minutes. See -> [`security-model.md`](../security-model.md). - -### 1.2 Per-controller triggers - -| Controller | Watches (besides `For`) | Predicate | Happy-path requeue | Data-plane edge | -|---|---|---|---|---| -| **GitProvider** | *(none)* | `GenerationChanged` on self | 5 min | ❌ | -| **GitTarget** | GitProvider, ClusterProvider, Namespace, WatchRule, ClusterWatchRule | `GenerationChanged` on **self** and on deps | 5 min converged / 10 s settling | ✅ GitPath channel | -| **WatchRule** | GitTarget, GitProvider, ClusterProvider | `GenerationChanged` on **self** and on deps | 5 min converged / 10 s settling | ✅ source-namespace channel | -| **ClusterWatchRule** | GitTarget, GitProvider | `GenerationChanged` on **self** and on deps | 5 min converged / 10 s settling | ❌ | -| **CommitRequest** | *(none)* | — | polls every 2 s | polls worker, no push | - -The `For()` predicate on GitTarget, WatchRule and ClusterWatchRule is not an -optimisation, it closes a self-triggering edge: a status write bumps `resourceVersion` -and fires an Update watch event that `EnqueueRequestForObject` turns straight back into -a queued request, un-rate-limited, so every reconcile cost roughly two. The shared status -writer already suppresses no-op writes (see -[`../spec/status-conditions-guide.md`](../spec/status-conditions-guide.md)); the predicate -makes it structural, and matches what GitProvider and ClusterProvider already did. - -The GitTarget no longer watches Secrets: the encryption-Secret watch was removed so the -process stops retaining every Secret value in the cluster; generated-age-Secret recovery -and out-of-band age-key updates ride the 5-minute reconcile instead. - -Sources: `gitprovider_controller.go`, `gittarget_controller.go`, -`watchrule_controller.go`, `clusterwatchrule_controller.go`, -`commitrequest_controller.go`. - -### 1.3 Two structural consequences - -- **`GenerationChangedPredicate` ignores status-only updates.** `generation` bumps - on **spec** changes only, so a dependency's *status* flip (GitTarget goes - refused, GitProvider goes `Ready=False`) does **not** re-enqueue dependents. This - is deliberate — without it every status heartbeat would re-list and re-enqueue - all dependents (a storm) — but it means status propagation falls back to the - periodic requeue. -- **The data plane (`internal/git` branch workers, `internal/watch`) cannot reach - the controllers**, except via the single GitPath channel added below. The - `internal/git` package never writes any CRD `.Status`. - ---- - -## 2. The fix already shipped — the GitPath acceptance channel - -The branch worker discovers a refused Git path **asynchronously**, after the -GitTarget's reconcile has already run and scheduled its 10-minute requeue. Before -the fix, `GitPathAccepted` could read `True` for up to 10 minutes after the path -was actually refused (it caused a CI flake; full write-up in -[gitpathaccepted-projection-race-and-external-drift.md](../spec/manifest-system.md)). - -The fix gives the data plane a controller-runtime `source.Channel` edge: - -- `internal/watch/gitpath_events.go` — `Manager.GitPathEvents()` exposes a lazily - created `chan event.GenericEvent`; `enqueueGitPathChange(gitDest)` does a - non-blocking send of a `GenericEvent` naming the GitTarget. -- `internal/watch/git_path_acceptance.go` — `MarkTargetGitPathRefused` / - `MarkTargetGitPathAccepted` emit on a **real transition only** (newly refused, - reason changed, or refusal cleared), so the steady-state resync stream never - causes a reconcile storm. -- `internal/controller/gittarget_controller.go:1003` — `SetupWithManager` wires it: - - ```go - b = b.WatchesRawSource(source.Channel( - r.EventRouter.WatchManager.GitPathEvents(), - &handler.EnqueueRequestForObject{}, - )) - ``` - -```mermaid -flowchart LR - DP[Branch worker / event-router
data plane] -- MarkTargetGitPath* --> M[Acceptance map] - M -- transition only --> CH[(GitPathEvents channel)] - CH -- GenericEvent --> C[GitTarget reconciler] - C -- projects --> S[GitPathAccepted condition] -``` - -This is the **prototype recipe**: *a change to data-plane reality enqueues the -owning CRD*. Everything in §3 is a place that needs the same recipe. - -**Scope caveat (see §4 for the trade-off):** an in-memory channel is correct here -because acceptance is recomputed from scratch on every reconcile and the periodic -requeue is the backstop. It does **not** survive a restart and is single-process — -so it is not the right tool for durable or cross-process triggers. - ---- - -## 3. Cases suffering today — "where we make it a good controller" - -### Gap A — data-plane outcomes never reach status - -**A1. Git push failures are invisible.** A failed push/resync lands in -`recordBackgroundResyncFailure` (`internal/watch/event_router.go:157`), which -**only increments a Prometheus counter** (`ResyncBackgroundFailuresTotal`). Push -retry exhaustion just returns an error (`internal/git/branch_worker.go:1179`). The -`internal/git` package writes no CRD status. **Result:** while pushes fail (auth -scope, non-fast-forward, branch protection, remote down), `kubectl get -gittarget`/`gitprovider` still read `Ready`. GitProvider `Ready` reflects only a -read-connectivity probe at reconcile time (`gitprovider_controller.go:238`) on a -10-minute cadence — and a read probe will not even catch a write-only failure. - -**A2. Background resync timeouts are invisible.** Same path: a per-type resync that -times out (`event_router.go:254`) is a metric, not a status. - -**A3. External Git drift is detected but not acted on.** `syncWithRemote` computes -`IncomingChanges` when the remote SHA moved; the field is documented as "requiring -resource-level reconcile" (`internal/git/types.go:86`) but its only consumer -**logs** it (`branch_worker.go:1414`). Out-of-band edits/deletes to the repo are -not reconciled until an unrelated event happens to fire. (Full treatment, incl. the -subtree-hash detection and Flux/Argo coordination, in the sibling doc.) - -**A4. CommitRequest outcomes are polled, not pushed.** The worker records a terminal -outcome (`recordCommitRequestOutcome`, `internal/git/commit_request_attach_loop.go:47`) -and the controller reads it (`LookupCommitRequestOutcome`) — but by **requeuing -every 2 s** (`commitrequest_controller.go:195`). The capture half of the recipe -already exists; only the push edge is missing. A nudge would replace the poll. - -### Gap B — control-plane status does not propagate to dependents - -**B1. WatchRule / ClusterWatchRule lag their GitTarget/GitProvider.** They watch -those deps with `GenerationChangedPredicate`, so when a GitTarget flips to refused or a -GitProvider goes `Ready=False` (status-only changes), the rules do **not** -re-reconcile; their projected `GitTargetReady` dependency condition is stale for up -to `RequeueSteadyInterval` (5 min). - -### Gap C — missing dependency watches - -**C1. GitProvider does not watch its credentials Secret.** `SetupWithManager` is -`For(GitProvider)` only. A rotated or corrected Git credentials Secret is not picked up -until the 5-minute steady requeue, so `Ready=False (ConnectionFailed)` lingers that long. - -> **Superseded (2026-07-07, secret-value-retention):** not watching the auth Secret is now a -> deliberate least-privilege choice, not a gap to fix with a Secret watch. The control plane -> reads credential and age-key Secrets directly by name and relies on the 5-minute -> `RequeueSteadyInterval` to pick up rotations; the GitTarget's former encryption-Secret watch -> was also removed. Adding a Secret watch would reintroduce cluster-wide Secret retention. See -> [`security-model.md`](../security-model.md). The -> §5 recommendation to "watch the auth Secret" below is retained only as historical context. - ---- - -## 4. The trigger toolbox — best practices (how Flux does it) - -Flux controllers are reconcile-loop only (no long-lived data plane), so a failure -is naturally written to status at the end of the same loop — they sidestep Gap A by -construction. Where Flux *does* need out-of-band or cross-resource triggers, it -uses four reusable mechanisms. All are directly applicable here. - -### F1. Persisted reconcile-request annotation - -`meta.ReconcileRequestAnnotation = "reconcile.fluxcd.io/requestedAt"` -(`fluxcd/pkg/apis/meta`): anyone patches the annotation with a fresh token to force -a reconcile. `predicates.ReconcileRequestedPredicate` enqueues only when the token -changes, combined as `predicate.Or(GenerationChangedPredicate{}, -ReconcileRequestedPredicate{})`. The handled token is recorded in status -(`LastHandledReconcileAt`) for idempotency. - -**Why it beats an in-memory channel for durable cases:** the annotation is -persisted on the object, so it survives controller restart, is visible in -`kubectl -o yaml`, works across HA replicas/processes, and replays via the -apiserver watch. - -### F2. Webhook Receiver → annotation - -Flux's notification-controller `Receiver` is a webhook endpoint that validates a -git-host payload (signature/HMAC) and patches the `requestedAt` annotation on the -listed resources. This is exactly the **pull-on-push** trigger (§5) and the -**pre-reconcile webhook** idea from the sibling doc. - -**But the stock Receiver cannot target our CRDs.** `spec.resources[].kind` is a -hard-coded enum (`notification-controller/api@v1.8.4/v1/reference_types.go:27`): - -```text -Enum=Bucket;GitRepository;Kustomization;HelmRelease;HelmChart;HelmRepository; - ImageRepository;ImagePolicy;ImageUpdateAutomation;OCIRepository; - ArtifactGenerator;ExternalArtifact -``` - -Anything outside that list fails schema validation, and notification-controller's -RBAC is scoped to Flux kinds. So a stock `Receiver` cannot point at -`GitProvider`/`GitTarget` without forking its CRD + RBAC (fragile, lost on upgrade). -The *mechanism* (validate webhook → patch annotation) is reusable; the *component* -is not. See §5 for what this means in practice. - -### F3. Status-field change predicate (fixes Gap B) - -Flux's source-aware controllers watch their dependency with a -`SourceRevisionChangePredicate` — a predicate that passes **only when the -meaningful status field changes** (the artifact revision), ignoring every other -status heartbeat. The fix for B1 is the same shape: `predicate.Or( -GenerationChangedPredicate{}, gitTargetReadinessChangedPredicate{})` rather than -removing the predicate (which reintroduces the storm). - -### F4. Conditions **and** Events, every loop (fixes Gap A surfacing) — SHIPPED - -Flux always patches kstatus conditions (`fluxcd/pkg/runtime/conditions` + `patch`) -**and** emits a Kubernetes Warning Event (`fluxcd/pkg/runtime/events`, which also -forwards to notification-controller), then requeues at a shorter failure interval. -A failure is immediately visible and alertable, not buried in a metric. - -**Built.** Every reconciler takes an `EventRecorder` and the shared status writer -(`internal/controller/status.go`) emits one Event per **persisted** `Ready` -transition: `Normal` when `Ready=True`, `Warning` otherwise, carrying the condition's -reason and message. Emitting after the patch rather than beside each condition write -is deliberate — a reconcile may write `Ready` several times and only the last is -stored, so intermediate values never reach `kubectl describe`. That also means the -suppressed no-op status write emits nothing, which is the intended behavior: nothing -changed, so nothing happened. - -### F5. Generalize the data-plane nudge (the §2 recipe, reused) - -Extend the GitPath channel into a small shared "status nudge": the data plane -records a fact (push error, commit outcome, drift) and enqueues the owning CRD; -controllers wire it via `WatchesRawSource`. CommitRequest (A4) then drops its 2 s -poll. - -### Choosing a mechanism - -| Mechanism | Survives restart | Cross-process | External trigger | Best for | -|---|---|---|---|---| -| In-memory `source.Channel` (§2) | ❌ | ❌ | ❌ | in-process signal recomputed each loop (GitPath) | -| Reconcile-request annotation (F1) | ✅ | ✅ | ✅ | external triggers, drift, anything that must outlive a restart | -| Status-field predicate (F3) | ✅ (apiserver-driven) | ✅ | n/a | dependency status → dependents (Gap B) | -| Conditions + Events (F4) | ✅ | ✅ | n/a | surfacing failures (Gap A) | - ---- - -## 5. Use case: trigger on Git repository change (pull-on-push) - -**What we want.** When the Git repo changes out-of-band — a human edit, a delete, a -foreign file, or (in bi-directional mode) a commit by Flux/Argo — we want to learn -about it **the moment it happens** via an inbound webhook from the git host, and -pull immediately. Pulling on the push event is strictly better than discovering the -drift on a later fetch: we re-align (or refuse) while the change is fresh, and we -usually avoid the expensive replay-mark session entirely. It is the push-driven -complement to the poll-driven drift detection in A3 and the subtree-OID check in the -[sibling doc](../spec/manifest-system.md). - -**Flow.** - -```mermaid -flowchart LR - GH[Git host
GitHub/GitLab/Gitea/Bitbucket] -- push webhook --> R[Webhook receiver
validate signature] - R -- map repoURL → owners --> A[patch requestedAt annotation
on affected GitProvider/GitTarget] - A -- ReconcileRequestedPredicate --> C[reconcile: fetch now] - C --> H{subtree OID
changed?} - H -- no / self-push --> Z[done — no replay] - H -- yes --> R2[reconcile affected GitTargets] -``` - -> **Superseded: pull-on-push is built, and not as this section planned.** The Git host's webhook -> reaches `GitProvider.spec.receiver`, and a delivery is only a hint: the branch worker reads the -> remote itself and classifies the move as its own push or somebody else's, which is also how the -> self-push dedupe of §5.3 is done. The refresher catches what no webhook reports. Neither Path A -> nor Path B in §5.4 was taken. The design of record is [the apply trigger](apply-trigger.md). -> -> **Why the receiver does not patch the annotation:** -> [inbound push notification](push-notification-and-reconcile-trigger.md) §7 shows that -> `requestedAt` drives a full cluster-to-Git snapshot, so a receiver that patches it can publish pre-push cluster state -> over the very change it was told about, before Flux or Argo CD has applied it. The receiver needs -> a request shape of its own: invalidate the base, record the new tip, replay retained work, and -> never snapshot. §8.3 of that page is the wire contract. Read everything below as the options that -> were weighed before it. - -### 5.1 Can we reuse the Flux component? - -Two separable things, opposite answers: - -- **The Receiver CRD/controller — no (not as-is).** Its `spec.resources[].kind` - enum (F2) rejects our kinds, and its RBAC is Flux-scoped. We would have to fork - its CRD and grant it patch RBAC on our types — fragile and lost on upgrade. Not - recommended. -- **The trigger *mechanism* — yes, fully.** "Validate a git-host webhook, then - patch a reconcile-request annotation on the owning object" is a pattern, not a - component. We already run webhook-serving infrastructure (audit/admission), so we - add one inbound git-webhook endpoint that: - 1. validates the provider payload (HMAC/secret) — the same per-provider logic Flux - implements for `github`/`gitlab`/`gitea`/`generic-hmac`; - 2. maps the payload's repo URL + branch to the `GitProvider`/`GitTarget`s that use - it (we already index providers by URL in the worker layer); - 3. patches the reconcile-request annotation on those objects (F1), so the trigger - is durable and observable rather than an in-process call. - - Modelling our own small `Receiver`-like CRD on Flux's API shape - (`Type`/`Events`/`Resources`/`SecretRef`/`ResourceFilter`) keeps it familiar. - -### 5.2 Do we need the exact name `reconcile.fluxcd.io/requestedAt`? - -**No — and adopting it buys little today.** The annotation key is just an internal -contract between whoever sets it and the `ReconcileRequestedPredicate` that reads -it. We can pick any key; annotations need no registration, and using a foreign -prefix is allowed (though `reconcile.fluxcd.io/` is semantically Flux's namespace). - -The *only* reason to use the exact Flux name is to let **Flux ecosystem tools** -trigger us — but both candidates are blocked anyway: the Receiver by its kind enum -(§F2), and `flux reconcile` by the same hard-coded kind list in its CLI. So neither -can target our CRDs regardless of the annotation name. - -**Recommendation:** define a **project-owned** key (e.g. -`configbutler.ai/requestedAt`) with the *same semantics* (opaque token value + -`status.lastHandledReconcileAt` for idempotency + a `ReconcileRequestedPredicate`). -Optionally **also honor** `reconcile.fluxcd.io/requestedAt` so `kubectl annotate` -muscle memory and any future Flux interop work for free — cheap, since the predicate -can accept either key. We can import `fluxcd/pkg/apis/meta` + -`fluxcd/pkg/runtime/predicates` (Apache-2.0, ~tiny) or reimplement the ~20-line -predicate ourselves to avoid a new dependency. - -### 5.3 Footgun: our own pushes will fire the webhook - -We write to the same repo, so our own commits trigger the push webhook → a feedback -loop. The reconcile must **dedupe self-pushes**: compare the incoming head SHA to -the SHA we just pushed (the branch worker already tracks `lastCommitSHA`), or skip -when the subtree OID equals the one we materialized. This is the same no-op guard -that makes the bi-directional acknowledgment loop (sibling doc §6) safe. - -### 5.4 Reusing a user's *existing* Flux setup (no second git-host webhook) - -§5.1 is about us standing up our own receiver. But most Flux power users **already** -have a git-host webhook wired to notification-controller and a `GitRepository` -tracking the repo. The best end-user experience is to piggyback on that and ask for -near-zero new config. The stock `Receiver` can't name our kinds (§F2), but the -surrounding Flux machinery gives two reuse paths. - -**Path A — watch the existing `GitRepository.status.artifact` (recommended).** -source-controller already fetches the repo and records the result in -`GitRepository.status.artifact` — a `meta.Artifact` with `Revision` -(e.g. `main@sha1:abc…`), `Digest`, and a tarball `URL` -(`source-controller/api/v1/gitrepository_types.go:259`, `pkg/apis/meta/artifact.go`). -Whether that update was driven by the user's webhook (instant) or source-controller's -poll interval (eventual), the artifact revision is the canonical "the repo changed" -signal *already in their cluster*. We simply **watch that `GitRepository`** and react -when the revision/digest changes — the F3 status-field-predicate pattern applied to a -Flux object. - -- **End-user cost: ~zero.** No webhook on our side, no git-host webhook #2, no - notification CRs. They already run source-controller; we just need read - (get/list/watch) RBAC on `source.toolkit.fluxcd.io` GitRepositories and a way to - associate one with our GitProvider. -- **Association:** an explicit optional `spec.sourceRef` on our GitProvider pointing - at a `GitRepository` is far cleaner than URL matching (URL normalization — ssh vs - https, `.git` suffix, embedded creds — is fiddly and error-prone). -- **Bonus — skip our own clone:** the artifact `URL` is the materialized tree at that - revision, integrity-checked by `Digest`, served in-cluster by source-controller. We - could read the subtree straight from it for the drift check **without our own git - fetch or credentials on the read path**. Bigger change to our model, but a real - simplification worth a follow-up. -- **Degrades gracefully:** with no webhook, source-controller still polls, so we still - converge — just not instantly. -- **Constraint:** requires Flux source-controller + a `GitRepository` for the repo. - Argo-only users need a separate integration; audit-only users without Flux fall back - to §5.1 or polling. - -**Path B — receive Flux notifications via a generic Provider/Alert.** If we'd rather -be pushed to than watch, the user keeps their git-host webhook and adds two small -Flux CRs: an `Alert{eventSources: [their GitRepository], …}` (a GitRepository **is** -allowed as an event source — same enum, but GitRepository is in it) targeting a -`Provider{type: generic-hmac, address: , secretRef: }` -(`notification-controller/api/v1beta3/provider_types.go`, -`.../v1beta3/alert_types.go`). notification-controller then HMAC-signs and POSTs every -GitRepository event to us; we validate and react. More config than Path A, but it's a -documented, supported eventing integration and needs no GitRepository *watch* RBAC. - -**Recommendation:** ship **Path A** as the primary Flux integration (best UX, least -config, additive/read-only), keep §5.1 (our own receiver) for non-Flux users, and -treat Path B as an optional alternative. All three converge on the same internal -trigger (fetch now → subtree-OID check → reconcile affected GitTargets) and all need -the §5.3 self-push dedupe. - -### 5.5 We already ingest these edits — tap the stream, share the Secret - -Path A describes a controller-runtime `Watches(&GitRepository{})`. But note what we -*are*: the tool that watches everything in the cluster. The `requestedAt` annotation -patch and the `GitRepository.status.artifact` bump are **cluster mutations**, and -mutations are exactly what our watch data plane already ingests. So the Flux signal -can be derived from the **same followable-type watch machinery** we use for every -other resource — configure a watch on `source.toolkit.fluxcd.io/GitRepository` and -route its observations internally — rather than bolting on a second watch path. - -Two distinct signals fall out of this, and they mean different things: - -- **`metadata.annotations[reconcile.fluxcd.io/requestedAt]` changes** — a *pre*-signal: - "source-controller is *about to* fetch." Useful for the bi-directional - acknowledgment wait (sibling doc §6): hold our reconcile until the fetch lands. -- **`status.artifact.revision` / `digest` changes** — the *content-ready* signal: the - new revision is fetched and available. This is the accurate trigger for our actual - pull/drift-check. - -**Configurable source tracking.** Expose an optional reference on our GitProvider -(working name `spec.sourceRef`) naming the Flux **`GitRepository`** to track. (Flux's -object is the *source* `GitRepository`; there is no "GitDestination" — the -destination-shaped concept is ours, the `GitTarget`.) When set, observed -revision changes on that source drive the trigger. - -**Reuse the same credentials Secret.** The user's `GitRepository` already references a -git-credentials Secret. Our credential reader already accepts the Flux **and** Argo -Secret dialects (`internal/git/credentials.go:81` — `identity`/`ssh-privatekey`, -`known_hosts`/`ssh_known_hosts`, `bearerToken`, username/password), so a GitProvider -can point at that **same** Secret — git creds configured once, for both tools. This -pairs naturally with `sourceRef`: one reference gives us the change signal *and* the -credentials. - -**Caveats unchanged:** we must actually be watching the source type (demand-driven — -add it to the watched set), and the §5.3 self-push dedupe still applies (our own -commits bump the same revision). The artifact-`URL` read-without-clone bonus from -Path A applies here too. - ---- - -## 6. Recommended roadmap - -1. **Surface write health (A1/A2).** Branch-worker push/commit outcome → nudge the - owning CRD (F5) → project `Ready=False(reason=GitOperationFailed)` on - GitProvider (provider-wide faults) and/or GitTarget (per-target/path faults), - and emit a Warning Event (F4). Highest value: silent push failure is the worst - blind spot. -2. **Propagate dependency status (B1).** Add status-field-change predicates (F3) to - WatchRule/ClusterWatchRule for their GitTarget/GitProvider deps. -3. **Watch the auth Secret (C1).** Add a `Watches(&corev1.Secret{}, …)` mapping - credentials Secrets to their GitProviders. -4. **Adopt the reconcile-request annotation (F1)** as the durable trigger; reserve - the in-memory channel for in-process, recomputed-each-loop signals like GitPath. -5. **Pull-on-push (§5).** Primary path: **track an existing Flux `GitRepository`** - via `spec.sourceRef` and react to its `status.artifact.revision` changes — observed - through our own watch stream (§5.5), reusing the same credentials Secret. Near-zero - end-user config for Flux users. Fall back to our own inbound git-webhook (§5.1) for - non-Flux users. Both feed the subtree-OID drift check with §5.3 self-push dedupe. - **Superseded:** built as the push receiver instead; see the note in §5. -6. **Replace CommitRequest polling (A4)** with the F5 nudge. - -## 7. Open questions - -- One shared nudge channel + a typed event payload, or one channel per concern? -- Should write-health be a new GitProvider/GitTarget condition, or fold into `Ready` - with a write-specific reason? (kstatus prefers a dedicated reason over a new - top-level condition.) -- Own the inbound git-webhook in the existing webhook server, or as a separate - Deployment? Which git providers do we validate first (GitHub/GitLab/Gitea)? -- §5.2 leans to a project-owned annotation that *also* honors the Flux key — confirm - that's the call, given neither Flux Receiver nor `flux reconcile` can target our - CRDs today anyway. -- Should the webhook patch the annotation (durable, decoupled) or enqueue in-process - (faster, but lost on restart)? §5.1 leans annotation. -- For §5.5: react to `requestedAt` (pre-fetch) or `artifact.revision` (content-ready), - or both for different purposes (coordination wait vs actual pull)? -- Should `spec.sourceRef` imply credential reuse from the referenced `GitRepository`'s - Secret by default, or require the Secret to be named explicitly? diff --git a/docs/design/support-boundary/support-contract.md b/docs/design/support-boundary/support-contract.md index dfe5206c..3e623290 100644 --- a/docs/design/support-boundary/support-contract.md +++ b/docs/design/support-boundary/support-contract.md @@ -2,7 +2,8 @@ > **design** — the single statement of the boundary. Index: [`../../INDEX.md`](../../INDEX.md) > -> Status: the front door for the **target** boundary. Captured 2026-07-11. +> Status: the front door for the **target** boundary. Captured 2026-07-11, rows checked against +> the code on 2026-11-10. > This page combines shipped behaviour with the design that must precede the rest > of the boundary. Rows marked **planned** state the intended contract, not > implemented behaviour; other rows describe the current runtime. If a claim here @@ -78,7 +79,7 @@ templates, and multi-input ResourceSet templates alike. It is specified once, in | Construct | Verdict | Why | |---|---|---| | kustomize `resources`, `namespace`, `images`, `replicas` | **Editable** | invertible; `images`/`replicas` edit-through is shipped | -| kustomize base + un-fancy overlays | **Editable, narrow** | render-root scoping reads `../../base` as read-only context and writes only inside the overlay; the base is never written. It edits existing overlay-local documents and declared image/replica entries, and **authors a new `images:`/`replicas:` entry** when a base-supplied image/replica is changed in one environment. `scan-repo` still reports these as `overlay-fan-out-unsupported` — the discovery-side flip is a follow-up | +| kustomize base + un-fancy overlays | **Editable, narrow** | render-root scoping reads `../../base` as read-only context and writes only inside the overlay; the base is never written. It edits existing overlay-local documents and declared image/replica entries, and **authors a new `images:`/`replicas:` entry** when a base-supplied image/replica is changed in one environment. `scan-repo` accepts them too; `overlay-fan-out-unsupported` is retired | | New object in an external-base overlay | **Editable** | placed as an overlay-local file and registered in the overlay's own `resources:` entry (never the base's), verified by re-render before the commit; proven by `TestPlacement_ExternalBaseOverlay_NewObject`. A new object a folder `images:`/`replicas:` entry would override is refused loudly by the oracle, not committed | | Change a base-supplied image/replica in one overlay | **Editable** | the writer authors a new `images:`/`replicas:` entry in the overlay's own kustomization (the base stays read-only), verified by re-render; proven by `TestOverlayAuthors_ImageEntry_ForBaseSuppliedImage` / `..._ReplicaEntry_ForBaseSuppliedCount` | | Delete a base-inherited object in one overlay | **Editable** | the writer authors a `$patch: delete` (patch file plus `patches:` entry) in the overlay, verified by re-render that the object leaves the render; the base is untouched. Proven by `TestOverlayAuthors_DeletePatch_ForInheritedObject`. A patch that fails to match is refused by the oracle, not committed | @@ -91,15 +92,16 @@ templates, and multi-input ResourceSet templates alike. It is specified once, in | kustomize `helmCharts:` (inflation) | **Refused** | we never render a chart | | A Helm chart (`Chart.yaml` + `templates/` + `values.yaml` + `crds/`) | **Planned: skipped as a unit** | this needs chart-folder detection; today `scan-repo` can report an incidental `crds/` directory as accepted | | Helm knobs on a `HelmRelease` / `Application` (chart version, inline values, parameters) | **Editable** | the Helm surface people actually use — see below | -| Free-standing Helm values file (`values/production.yaml`) | **Refused today** | not KRM; a `ValuesFile` projection is proposed | +| Helm values file named by a `HelmRelease` (`spec.chart.spec.valuesFiles`) or an Argo CD `Application` (`helm.valueFiles`) | **Read-only context** | retained, never written, never a reason to refuse the folder; editing it is the planned `ValuesFile` projection ([values-file-projection.md](values-file-projection.md), Move 2) | +| Free-standing Helm values file no release names | **Refused** | `non-krm-yaml`: not KRM, and nothing in the folder names it | | CI-rendered `rendered/` output | **Refused** | the next render destroys the edit | ## Machine-written Git, and secrets | Construct | Verdict | Why | |---|---|---| -| A path written by Flux `ImageUpdateAutomation` | **Refused** | a bot commits here; `$imagepolicy` comments are load-bearing | -| `.argocd-source*.yaml` (Argo CD Image Updater) | **Refused / read-only** | written by a bot, and it outranks the config a reader would inspect | +| A path written by Flux `ImageUpdateAutomation` | **Planned: refused** | a bot commits here; `$imagepolicy` comments are load-bearing. Nothing detects it today | +| `.argocd-source*.yaml` (Argo CD Image Updater) | **Planned: read-only** | written by a bot, and it outranks the config a reader would inspect. Nothing detects it today | | SOPS `Secret` | **Write-only** | not schema-conformant; the `mac` binds the whole document | | `SealedSecret` | **Editable** | ordinary KRM; each value sealed independently | | `ExternalSecret` | **Editable** | ordinary KRM; the value simply lives elsewhere | @@ -118,11 +120,10 @@ documents landed. What stays refused, permanently and correctly, is **chart inflation**: we never render `templates/`, and we never learn what a value means. -The one genuine gap is the **free-standing values file**, which is not KRM and so -has no object for a user to edit. The proposed fix reuses the -[`EncryptedSecret`](write-only-encrypted-secrets.md) trick — project it into a kind -we can store — and is argued in -[expansion-boundary-and-corpus-organisation.md](expansion-boundary-and-corpus-organisation.md). +The one genuine gap is the **values file**, which is not KRM and so has no object for +a user to edit. A values file a release names is already read-only context, so it no +longer refuses the folder. Making it editable means projecting it into a kind we can +store; that is Move 2 of [values-file-projection.md](values-file-projection.md). ## What we will not do @@ -153,6 +154,6 @@ we can store — and is argued in | [resource-capability-model.md](resource-capability-model.md) | what may I do to *this document* (schema conformance, visibility, write unit) | | [write-only-encrypted-secrets.md](write-only-encrypted-secrets.md) | SOPS: describe the ciphertext, never read it | | [sealed-secrets-and-external-secrets.md](sealed-secrets-and-external-secrets.md) | why neither needs a new kind, and the derived-object gate | -| [Git write preflight](../git-write-preflight.md) | what happens to an edit with no legal destination | +| [Git write preflight](../git-write-preflight.md) | proposal, not built: what happens to an edit with no legal destination | | [repo-discovery-and-onboarding-scan.md](repo-discovery-and-onboarding-scan.md) | reporting all of this over a whole repository | | [`test/fixtures/gitops-layouts/`](../../../test/fixtures/gitops-layouts/) | the corpus of real-world shapes every verdict is checked against | diff --git a/docs/design/vocabulary-cleanup.md b/docs/design/vocabulary-cleanup.md deleted file mode 100644 index bf7ae1b3..00000000 --- a/docs/design/vocabulary-cleanup.md +++ /dev/null @@ -1,590 +0,0 @@ -# Vocabulary cleanup: one name per concept, as a breaking change - -> **built**: every item below shipped in one breaking release, recorded in -> [`../UPGRADING.md`](../UPGRADING.md). The rules are in [`../definitions.md`](../definitions.md); -> every call this page once left to the maintainer is settled under "Decided: break it in place, -> and warn". -> Index: [`../INDEX.md`](../INDEX.md) -> Governed by: [`../facts/crd-upgrade-strategies.md`](../facts/crd-upgrade-strategies.md) -> Completes: [`../config-flag-conventions.md`](../config-flag-conventions.md) - -## The one-line version - -An audit of all six CRDs found that the API is named well where the naming rules were written down -first, and named inconsistently everywhere the rules arrived late. This change writes the remaining -rules down ([`../definitions.md`](../definitions.md)) and then makes the surface obey them, in one -breaking release, because every remaining problem is a user-visible string. - -Almost every item is a name, and the controllers decide nothing differently. Two change what a -cluster does. The move to `spec.client`: a manifest still setting the old `spec.qps` is refused, and -a provider already storing it falls back to the built-in throttle. And the removal of -`--source-cluster-qps` and `--source-cluster-burst`, whose defaults are now built in. Removed -printer columns change what `kubectl get` shows, never what is decided. - -## Why one breaking release rather than a deprecation cycle - -Thirteen of the items are condition reasons, which no schema can deprecate: a reason is a string a -controller writes into `status`, so there is no field to retain, nothing to reject, and no admission -point to warn from. The only way to change one is to change it, and the only way to give a user -warning is a document. Spreading the change over two releases would therefore buy nothing for the -largest group while leaving two spellings of several concepts live in between, which is the state -[`../definitions.md`](../definitions.md) rule 1 exists to end. - -The schema changes are small enough to ride along. Per -[`../facts/crd-upgrade-strategies.md`](../facts/crd-upgrade-strategies.md), each is checked against -the question that matters: **when this value is pruned, does the object do more or less?** Only one -item in this whole change prunes fail-open, and it is retained so admission can refuse it. - -## What the audit found, and what it got wrong - -The audit is the reason this page exists. Three of its findings do not survive contact with the -rulebooks that already existed, and they are recorded here so they are not re-raised: - -**The `insecure` flag split is deliberate.** -[`../config-flag-conventions.md`](../config-flag-conventions.md) rule 6 already governs it: the -`-insecure` suffix is a per-component transport toggle (`--metrics-insecure`, `--audit-insecure`, -`--redis-insecure`) and the `insecure-` prefix is a loud dev-only escape hatch -(`--insecure-kubeconfig-exec`, `--insecure-allow-missing-known-hosts`). Only one flag sits on the -wrong side of that rule, and it is a word-order slip rather than a missing convention. - -**Bare-noun booleans are sanctioned.** `--author-attribution` is the shape of Flux's `--token-auth`, -and rule 2 names `--enable-http2` as the one tolerated Kubebuilder exception. There is no -inconsistency to fix. - -**`--redis-addr` with a `valkey:6379` default is rule 1 working as intended.** The flag is named for -what it controls, not for the implementation behind it, which is the rule's own worked example. It -stays. - -And one finding gets narrower rather than disappearing: **`spec.encryption.provider: sops` is -correct.** Flux spells its `spec.decryption.provider` value `sops`, lowercase -(`external-sources/flux/flux2/internal/flags/decryption_provider.go`), and matching the ecosystem -beats internal symmetry, the same argument that justifies aliasing Flux's condition reasons. What -was missing was not the PascalCase rule but its exception, which -[`../definitions.md`](../definitions.md) rule 7 now states. - -### The enum casing question, asked and closed - -That one value raised a bigger question, since a breaking release is the moment to ask it: should the -PascalCase enums be lowercase, the way much of Flux's surface is? **No, and nothing changes.** The -evidence is in rule 7 and was measured rather than recalled. - -Kubernetes `core/v1` is PascalCase for every mode it invents, uppercase for acronyms, and lowercase -only where a value names something outside the API (`linux`, `noexec`, `cpu`). Flux approximates that -convention without holding it: it lowercases modes it invented itself (`extract;copy`, -`none;client;server`, `enabled;warn;disabled`, `poller;legacy`), one of its enums mixes casing -internally (`head;HEAD;Tag;TagAndHEAD`), and `HelmRelease.spec.uninstall.deletionPropagation` is -`background;foreground;orphan` where the `metav1.DeletionPropagation` values it names are -`Background`, `Foreground` and `Orphan`. - -So Flux is the wrong model for this specific rule, and our enums already match the right one: `Never` -and `Always` and `Ignore` are literal `core/v1` values, and `ConfigMap;Secret` matches Flux's own -`Secret;ConfigMap`. Flipping them to lowercase would move away from both projects at once. What the -question did produce is the "who we borrow from, and for what" table at the top of -[`../definitions.md`](../definitions.md), which exists so this is settled once instead of per field. - -## The changes - -Grouped by surface, because the surface decides the migration cost. Every row cites the -[`../definitions.md`](../definitions.md) rule it satisfies. - -### Condition reasons - -Reasons are the largest group and the cheapest to change in code: they are constants in -`internal/controller`. They are also the only group with no schema-level migration path at all, so -every reason below lands in one commit. - -The rule most of these breach is not new and is not this change's to make. **"A reason that restates -the condition type answers nothing and is not used" is already in -[`../spec/status-conditions-guide.md`](../spec/status-conditions-guide.md), under Reason -vocabulary**, and `spec/` binds. So the eight `Succeeded` rows below are not a proposal: they are a -spec the code does not currently obey. The `guide` rows cite it; the numbered rows cite -[`../definitions.md`](../definitions.md). - -| Now | Becomes | Rule | Note | -|---|---|---|---| -| `GitRepoConfigNotFound` | `GitProviderNotFound` | 1 | on `WatchRule` and `ClusterWatchRule`. Names a kind that no longer exists; the Go identifier was renamed years ago and the string was not | -| `GitRepoConfigNotReady` | deleted | 1 | declared on both rule kinds, emitted by neither. Dead constants keeping a dead name alive | -| `GitPathAccepted` | `Succeeded` | guide | on `GitPathAccepted=True` | -| `RenderMatchesLive` | `Succeeded` | guide | on `RenderMatchesLive=True` | -| `GitProviderReady` | `Succeeded` | guide | on `GitProviderReady=True` | -| `ClusterProviderReady` | `Succeeded` | guide | on `ClusterProviderReady=True` | -| `Pushed` | `Succeeded` | guide | on `CommitRequest`'s `Pushed=True` | -| `Validated` | `Succeeded` | guide | on `Validated=True`. `InCluster` stays: it is a second answer with something to say, and it shows the shape rule 6 is asking for | -| `Suspended` | alias `fluxmeta.SuspendedReason` | guide | same string, now shared rather than re-spelled. No user-visible change | -| `Reconciling` | deleted | guide | unused in production code; `Progressing` (already a Flux alias) is what the controllers set | -| `Checking` | deleted | 1 | unused in production code, asserted only by tests | -| `Stalled` | `Failed` | guide | the one generic reason with no upstream equivalent. `fluxmeta` has no `Stalled`, so this aliases `FailedReason`. Used once, in `gittarget_dependency_status.go` | -| `Resolved` / `UnresolvedResources` | `Succeeded` / `CatalogNotReady` | 6 | on `ResourcesResolved`. The False reason inverted the noun order of its own type instead of naming a cause. The cause is the only one resolution can report: the discovery catalog is not ready. A selector matching nothing the cluster serves resolves `True`, watching zero types | - -The `Succeeded` collapse is the item most worth arguing about, so the argument is here rather than -in a commit message. Six conditions currently answer "why are you True?" with their own name. The -alternative to collapsing them is inventing six domain reasons for the uninteresting end of six -binary gates, and `Ready=True` already answers `Succeeded`, so the vocabulary exists and is in use. -Where a True state does have something to say, a domain reason stays: `InCluster` on `Validated`, -`Received` on `AuditFactsReceived`, `AttributedFromAdmission` on `AuthorAttributed`. - -Also here, and not a rename: the five test-only compatibility aliases -(`ConditionTypeStreamsReady`, `GitTargetConditionStreamsReady`, `GitTargetStreamsReadyReasonNotReady`, -`GitTargetReasonReady`, `GitTargetReasonConflict`) are deleted and their call sites updated. They -keep the pre-rename `StreamsReady` spelling readable in the tree, which is the single biggest reason -"is this named consistently?" cannot be answered by grep. - -### Printer columns - -Cheap, visible, and breaking only for someone parsing `kubectl get` output, which nothing should do. - -| Kind | Now | Becomes | Rule | -|---|---|---|---| -| `GitTarget` | `ProviderReady`, `ClusterProviderReady` | **both removed** | 8 | -| `WatchRule`, `ClusterWatchRule` | `Target` | `GitTarget` | 1 | -| `ClusterProvider` | `Facts` | `FactsReceived` | 8 | -| `GitProvider`, `ClusterProvider`, `GitTarget` | `Status` | `Message` | 8 | -| `WatchRule`, `ClusterWatchRule`, `CommitRequest` | (absent) | `Message` added | 8 | -| `WatchRule`, `ClusterWatchRule` | (absent) | `ResourcesResolved` added | 8 | - -The first row replaces what an earlier draft of this page proposed, and the reasoning is worth keeping -because it changed the rule rather than the row. The draft renamed `ProviderReady` to -`GitProviderReady` so it would stop reading as the general case of `ClusterProviderReady` beside it. -Shortening the other way (`GitReady` and `ClusterReady`, symmetric and narrower) does not work: -`GitReady` sits beside `GitPathAccepted`, where `Git` is the repository, and `ClusterReady` sits beside -`SourceReachable`, which is about the cluster itself. `Provider` is the token that distinguishes the -config object from the thing it names, so neither name can lose it. - -Which left the question worth asking: why are those two columns there at all? Both only project another -object's `Ready`, and `Ready`'s own reason on the `GitTarget` already carries `GitProviderNotReady` or -`ClusterProviderNotReady` when either is why the target is not ready. They are the first two -progressing contributors in `internal/controller/gittarget_controller.go`, so in the ordinary -single-cause case the default `REASON` column has already said it. The column repeats it and then -sends the reader to the other object, which is where the detail lives anyway. - -So rule 8 now excludes a dependency-readiness projection from getting a column, and the asymmetry -disappears without a rename. `GitTarget`'s output goes from twenty columns to eighteen. The two -conditions are still set, still in `kubectl describe`, and still aggregated into `Ready`. - -`SourceReachable` stays: dropping `Cluster` collides with nothing, which rule 8 permits. `Validated` -and `EncryptionConfigured` are **not** given columns after all; they were in the draft under the -"every condition gets a column" clause, and dropping them is the same judgment applied consistently to -`GitTarget`'s own-spec gates. - -**One consequence to accept or reject.** The same rule condemns the existing `GitTargetReady` column -on `WatchRule` and `ClusterWatchRule`: it too projects another object's `Ready`, and it literally -copies that object's reason (`gitTargetReadyReasonIsStalled` in `internal/controller/stream_status.go` -switches on `GitTarget` reasons). Dropping it would be consistent. It is listed here rather than in the -table because it was not part of the audit, and a rule discovering extra work is exactly the moment to -say so out loud instead of quietly widening the change. - -`ResourcesResolved` is not a projection and keeps its new column. It reports this rule's own work, -answering whether `spec.rules[]` matched anything the cluster serves, and no other object holds that -answer. - -### Status fields - -| Kind | Now | Becomes | Prune fails | Strategy | -|---|---|---|---|---| -| `CommitRequest` | `status.sha` | `status.commit` | n/a, status | delete and rename | -| `GitTarget` | `status.remote.revision` | `status.remote.commit` | n/a, status | delete and rename | -| `GitTarget` | `status.placement.resolvedAtRevision` | `resolvedAtCommit` | n/a, status | delete and rename | -| `CommitRequest` | column `SHA` | column `Commit` | n/a | follows the field | -| `GitTarget` | `status.retention.lastChangedTime` | `lastChangedAt` | n/a, status | delete and rename | -| `GitProvider` | `status.branches[].gitTargets` | `gitTargetCount` | n/a, status | delete and rename | -| `GitTarget` | `status.retention.mode` with no enum | gains the `PruneMode` enum | n/a | narrow, the cheapest row in the matrix | -| both rule kinds and `GitTarget` | `WatchRuleStreamsStatus` + `GitTargetStreamsStatus` | one `StreamsStatus` | n/a | Go-level only; `GitTarget` gains an optional `pendingSample` | - -#### One commit hash, three field names - -`CommitRequest.status.sha`, `GitTarget.status.remote.revision` and -`GitTarget.status.placement.resolvedAtRevision` all hold the same kind of value: a bare 40-character -commit hash. Verified rather than assumed, at the three places each is written: -`pw.CommitSHA.String()` in `internal/git/branch_worker.go`, `outcome.Head.String()` and -`report.HEAD.Sha` for the remote observation, and `head.Hash().String()` in `worktreeRevision`. One -concept, two words, which is rule 1. - -Neither of the two words is the right one, and Flux says why in its own doc comments. Its -`Artifact.Revision` is "a human readable identifier traceable in the origin source system. It can be -a Git commit SHA, Git tag, a Helm index timestamp, a Helm chart version, etc.", so `revision` is -deliberately polymorphic across source kinds, and its Git form in `v1` is composite -(`main@sha1:`) rather than a bare hash. Its `GitRepository.spec.ref.commit` is documented as -"Commit SHA to check out", which is the bare hash. And Flux has no `sha` field anywhere in any of its -APIs. - -So the word for a bare Git commit hash is **`commit`**. It is what Flux calls that value, it is one -word for all three fields, and it drops an algorithm name that Git's SHA-256 transition makes a poor -choice for a field. `revision` is left available for a composite identifier if this project ever needs -one, which is the distinction Flux is making and which naming both the same thing would destroy. - -`status.placement.resolvedAtRevision` becomes `resolvedAtCommit`, which also lands it on rule 4: the -`At` names a point, and what follows now says what kind of point. - -Status fields are the easy case and the matrix says why: the operator writes them, no user manifest -carries them, and nothing is pruned from a spec. A renamed status field is absent for one -reconcile and then present under its new name, except on a finished `CommitRequest`, which is -never reconciled again and keeps an empty `status.commit`; the commit is in Git. The enum narrowing -on `status.retention.mode` is safe -for the same reason: only the controller writes it, and it only ever writes the three values the -enum lists. - -The two flat-versus-nested findings are **resolved by writing the rule down, not by moving -anything**. Rule 2 says a block groups two or more facts about one subject and a single fact stays -flat, which retro-fits `GitTarget.status.remote` (three facts, a block) and -`GitProvider.status.lastVerifiedAt` (one fact, flat) with no change. The same is true of the -first-column ordering finding, which rule 8's "defining fact, else Ready" covers as written. - -### Spec fields and settings - -| Surface | Now | Becomes | Prune fails | Strategy | -|---|---|---|---|---| -| `ClusterProvider` | `spec.qps`, `spec.burst` | `spec.client.qps`, `spec.client.burst` | **open** | retain-and-refuse, see below | -| flag | `--allow-insecure-git-http` | `--insecure-allow-git-http` | n/a | rename, chart value follows | -| chart | `controllerManager.gitRefreshInterval` | `git.refreshInterval` | n/a | rename, documented in `UPGRADING.md` | -| flag | `--source-cluster-qps`, `--source-cluster-burst` | removed; 20 and 30 are built in | n/a | delete: the chart never set them, and `spec.client` is the per-cluster knob | - -`spec.qps`/`spec.burst` is the only fail-open row in the change: pruning a per-cluster override -falls back to the built-in default of 20 QPS, which on a deliberately throttled provider -means the operator starts talking to that source cluster faster than the user asked. That is a -widening, and the matrix's rule for a widening is retain-and-refuse or a loud pre-upgrade inventory -rather than a quiet delete. - -`accessFrom` and `allowAnySourceNamespace` are **not renamed.** The audit read them as two -namespace-policy settings with names that do not say which direction each governs, which is true, -but `accessFrom` is Flux's spelling and `allowAnySourceNamespace` already names its axis. What was -missing is a page that says "access" is the control-cluster direction and "source namespace" is the -source-cluster one, which [`../definitions.md`](../definitions.md) now does. This is the clearest -case on the page of a definition being the right fix for an ambiguity, and it is worth noticing that -the audit reached for a rename first. - -`allowAnySourceNamespace` was later replaced by `allowCrossNamespace.sources`, when a second -cross-namespace grant, for appliers, needed a place beside it -([`apply-trigger.md`](apply-trigger.md#decisions)). - -## Decided: break it in place, and warn - -Nothing is open any more. The posture, decided by the maintainer: **there is no known production -install, so nothing is kept for compatibility.** No retain-and-refuse release, no conversion, no -alias left behind for an old spelling. The release version carries the break and -[`../UPGRADING.md`](../UPGRADING.md) carries a loud warning. - -That is what `v1alpha3` is for. The Kubernetes API conventions allow an alpha version to change -incompatibly without a new version, which is the whole content of the alpha signal, so these renames -land in `v1alpha3` itself rather than opening a `v1alpha4`. Opening one would be the conservative move -and would cost either a conversion webhook or an unreadable set of stored objects, for a guarantee -nobody asked for. - -The four calls that were open, now settled: - -| Was open | Decided | -|---|---| -| how to land `spec.client.{qps,burst}` (the one fail-open row) | **retain-and-refuse.** First decided as a one-shot delete, reversed in review: the old spellings stay in the schema, never read, and a spec-level CEL rule refuses them by name. A provider already storing them passes by ratcheting until its spec is next edited, so `UPGRADING.md` still says to check before upgrading | -| `Message` on all six kinds, or none | **all six**, `priority=1`, second to last | -| whether the True-state reasons collapse to `Succeeded` | **yes.** An alerting rule keying on `reason=GitPathAccepted` has to key on the condition type instead, where it belonged | -| the `GitTargetReady` column on the two rule kinds | **dropped**, consistent with rule 8 excluding dependency-readiness projections | - -The one consequence worth writing down before it surprises someone: a renamed reason is also a -**metrics label change**. `resource_condition` carries `reason` -(`internal/controller/resource_condition_metrics.go`), but only for `Ready`, `Reconciling` and -`Stalled`. So the `True`-state collapse changes no series, because `Ready=True` was already -`Succeeded`; the renames that do reach the metric are the three a rule's `Ready` can carry: -`GitRepoConfigNotFound`, `UnresolvedResources` and `Stalled`. A dashboard selecting one of those goes -empty rather than erroring, which is why the `UPGRADING.md` entry names them. - -## Commit split - -Seven commits, each independently green, ordered so the rules exist before anything cites them: - -1. `docs: define the project vocabulary and the naming rules` (`definitions.md`, this page, the - `INDEX.md` entries) -2. `fix(api): stop naming a kind that no longer exists in two rule reasons` (A, the - `GitRepoConfig` rows, plus the dead constants) -3. `refactor(api): stop restating condition types in reasons` (A, the rest) -4. `refactor(api): delete the pre-rename condition aliases` (A, the five test-only aliases) -5. `feat(api): name every printer column for the condition it reads` (B) -6. `feat(api): rename the two status fields the naming rules rule out` (C, plus the enum and the - shared `StreamsStatus`) -7. `feat(api)!: group the per-cluster client throttles, and fix one flag's word order` (D, the - breaking half, gated on the throttle-override call above) - -Then `docs: record the vocabulary cleanup in the upgrade guide` (`UPGRADING.md`), written in the -present tense the way that file requires: "`GitRepoConfigNotFound` is `GitProviderNotFound`", never -"will be renamed". - -## Out of scope - -- `CommitRequest` not implementing the shared status accessors. Deliberate and documented; it does - not use the status session helper. -- The `controllerManager.*` chart block as a whole. `gitRefreshInterval` moves because it has an - obvious home; the remaining four leftovers do not, and inventing a block for them is a bigger - argument than this change should carry. -- Metric names and labels. They have their own consistency question and - [`../interpreting-metrics.md`](../interpreting-metrics.md) is the surface that documents them. -- Anything behavioral. If a rename is tempting because the behavior is confusing, the behavior is - the change and it does not belong here. - -## Validation - -The usual gate (`task lint`, `task test`, `task test-e2e`, run sequentially), plus two things this -change specifically needs: - -Every renamed reason and column string appears in e2e assertions and in docs. `task lint-docs` runs -`hack/doccheck` over every tracked file, so a document still citing an old spelling by path fails the -lint run, but a document citing one in prose does not. A grep for each retired string across -`docs/`, `test/` and `charts/` is part of the commit that retires it. - -After the API comment edits, confirm only descriptions moved, per the `AGENTS.md` procedure: -regenerate into a scratch directory with `controller-gen` and compare against `config/crd/bases` -with every `description` key stripped from both. This change edits comments on types that carry -`+kubebuilder:` marker blocks, which is exactly the case where a misplaced comment silently drops a -marker. - -## Worked before and after, per kind - -The tables above name strings without showing where they sit. This section is the same change seen -from `kubectl`. Column rows are the current headers, taken from `config/crd/bases`; status -snippets show only the fields and reasons this change touches. - -### GitProvider - -```text -# before -NAME URL READY REASON AGE - -o wide adds: STATUS VERIFIED -# after -NAME URL READY REASON AGE - -o wide adds: MESSAGE VERIFIED -``` - -```yaml -# before -status: - branches: - - name: main - gitTargets: 3 # a count named like a list - lastVerifiedAt: "2026-09-25T09:14:02Z" -# after -status: - branches: - - name: main - gitTargetCount: 3 # rule 3 - lastVerifiedAt: "2026-09-25T09:14:02Z" # unchanged: rule 4 already satisfied -``` - -### ClusterProvider - -The `Facts` header is the one column that renames its concept instead of shortening it, so an -operator who sees it has no way to guess which condition to describe. - -```text -# before -NAME READY REASON FACTS AGE - -o wide adds: VALIDATED STATUS -# after -NAME READY REASON FACTSRECEIVED AGE - -o wide adds: VALIDATED MESSAGE -``` - -```yaml -# before -status: - conditions: - - type: Validated - status: "True" - reason: Validated # restates its own type - - type: AuditFactsReceived - status: "Unknown" - reason: RouteUnused # already correct: names the cause -# after -status: - conditions: - - type: Validated - status: "True" - reason: Succeeded # spec/status-conditions-guide.md, Reason vocabulary - - type: AuditFactsReceived - status: "Unknown" - reason: RouteUnused -``` - -`InCluster` stays as the other `Validated=True` reason. It is the shape the rule is asking for: a -second answer that tells you something `Succeeded` cannot. - -### GitTarget - -The two provider gates sit side by side in wide output, and only one of them is abbreviated. - -```text -# before, -o wide (20 columns, the provider/source ones shown) -... STREAMSRUNNING SOURCEREACHABLE PROVIDERREADY CLUSTERPROVIDERREADY STATUS ... AGE -# after (18 columns) -... STREAMSRUNNING SOURCEREACHABLE ... MESSAGE AGE -``` - -`SOURCEREACHABLE` stays: dropping `Cluster` from `SourceClusterReachable` collides with nothing. -The two provider columns are gone rather than renamed, because each only projected another object's -`Ready` (rule 8), and `MESSAGE` moves to second to last. - -```yaml -# before -status: - conditions: - - type: GitPathAccepted - status: "True" - reason: GitPathAccepted # restates its own type - - type: RenderMatchesLive - status: "True" - reason: RenderMatchesLive # restates its own type - - type: GitProviderReady - status: "True" - reason: GitProviderReady # restates its own type - remote: - revision: 4f2c1ab9e3d5... # a bare commit hash, called something else on CommitRequest - verifiedBy: Push - placement: - resolvedAtRevision: 4f2c1ab9e3d5... - retention: - mode: OnEvent # no enum on the schema, unlike spec.prune.mode - retainedDocuments: 0 - lastChangedTime: "2026-09-25T09:14:02Z" - streams: - summary: 3/4 - total: 4 - ready: 3 -# after -status: - conditions: - - type: GitPathAccepted - status: "True" - reason: Succeeded - - type: RenderMatchesLive - status: "True" - reason: Succeeded - - type: GitProviderReady - status: "True" - reason: Succeeded - remote: - commit: 4f2c1ab9e3d5... # rule 1: one word for one concept - verifiedBy: Push - placement: - resolvedAtCommit: 4f2c1ab9e3d5... - retention: - mode: OnEvent # now carries Enum=Never;OnEvent;Always, rule 7 - retainedDocuments: 0 - lastChangedAt: "2026-09-25T09:14:02Z" # rule 4 - streams: - summary: 3/4 - total: 4 - ready: 3 - pendingSample: [apps/deployments] # gained with the shared StreamsStatus type -``` - -Nothing changes on the False side. A refused write still reads `GitPathAccepted=False` with -`reason: UnsupportedContent` or `WriteBoundaryRefused`, which is what rule 9 and the conditions -guide both already ask for. - -### WatchRule and ClusterWatchRule - -This is where the kind-renamed-but-string-did-not shows up, and it is the row an operator -hits: point a rule at a `GitProvider` that does not exist and the reason names a kind that has not -existed for a long time. - -```text -# before -NAME TARGET READY REASON STREAMS AGE -rule my-target False GitRepoConfigNotFound 0/0 4m -# after -NAME GITTARGET READY REASON STREAMS AGE -rule my-target False GitProviderNotFound 0/0 4m - -o wide: GITTARGETREADY is gone; RESOURCESRESOLVED and MESSAGE are added -``` - -```yaml -# before -status: - conditions: - - type: ResourcesResolved - status: "False" - reason: UnresolvedResources # inverts its own type instead of naming a cause - - type: ResourcesResolved # when True, on another object - status: "True" - reason: Resolved # restates its own type -# after -status: - conditions: - - type: ResourcesResolved - status: "False" - reason: CatalogNotReady # rule 6: name the cause - - type: ResourcesResolved - status: "True" - reason: Succeeded -``` - -`ResourcesResolved` also gains a wide column on both kinds. It is the condition that answers "did -your `rules[]` match anything the cluster serves", and today you have to describe the object to see -it at all. - -`ClusterWatchRule` gets the same two changes and not `SourceAuthorized`, which it has no business -carrying: it selects no namespaces. - -### CommitRequest - -```text -# before -NAME GITTARGET READY REASON SHA AGE - -o wide adds: AUTHORATTRIBUTED PUSHED BRANCH -# after -NAME GITTARGET READY REASON COMMIT AGE - -o wide adds: AUTHORATTRIBUTED PUSHED BRANCH MESSAGE -``` - -`GITTARGET` is already right here, and it is the reason the two rule kinds move to match it rather -than the other way round. - -```yaml -# before -status: - sha: 4f2c1ab9e3d5... # the only field in the API that says "sha" - conditions: - - type: Pushed - status: "True" - reason: Pushed # restates its own type - - type: AuthorAttributed - status: "True" - reason: AttributedFromAdmission # already correct -# after -status: - commit: 4f2c1ab9e3d5... # same value, the word Flux uses for it - conditions: - - type: Pushed - status: "True" - reason: Succeeded - - type: AuthorAttributed - status: "True" - reason: AttributedFromAdmission -``` - -### The spec change, and the one flag - -```yaml -# before -apiVersion: configbutler.ai/v1alpha3 -kind: ClusterProvider -spec: - kubeConfig: - secretRef: {name: prod-kubeconfig} - qps: 20 - burst: 40 -# after -spec: - kubeConfig: - secretRef: {name: prod-kubeconfig} - client: - qps: 20 - burst: 40 -``` - -This is the only fail-open row in the change. Pruned, the two values fall back to the operator-wide -`--source-cluster-qps` and `--source-cluster-burst`, so a deliberately throttled provider starts -talking to its source cluster faster than asked. - -```text -# before ---allow-insecure-git-http -# after ---insecure-allow-git-http # config-flag-conventions.md rule 6, and Flux's --insecure-allow-http -``` diff --git a/docs/design/watches-labels-simplification.md b/docs/design/watches-labels-simplification.md deleted file mode 100644 index 9bc661bb..00000000 --- a/docs/design/watches-labels-simplification.md +++ /dev/null @@ -1,359 +0,0 @@ -# Label selection for watch rules - -> **Implemented.** `rules[].objectSelector` shipped with all three prune modes, including `Always` -> with supported Kustomize layouts, filtered-removal attribution, and ordered producer replacement. -> **Step 2 of 2.** Step 1, the collection rename and event-filter removal, -> shipped first. This page is now the design record; the -> [configuration reference](../configuration.md#selecting-objects-by-label-rulesobjectselector) -> describes the behavior. -> [Issue #146](https://github.com/ConfigButler/gitops-reverser/issues/146). -> Index: [`../INDEX.md`](../INDEX.md). Checked: 2026-09-28, source at `69836e70`. -> Related: [Kubernetes watch facts](../facts/kubernetes-watch-options.md), -> [Definitions](../definitions.md), [Ordering](../spec/watch-event-ordering-and-attribution-grace.md). - -**Kubernetes is the source of truth. Git follows the cluster closely.** Within a GitTarget's -managed type and namespace scope, `Always` removes resource documents absent from a complete -server-selected snapshot, including documents that never matched. Git labels and historical -membership do not exempt a document. An object outside the selection is absent from this mirror -even if it still exists in Kubernetes. `OnEvent` and `Never` explicitly limit deletion. - -The kube-apiserver selects objects through native LIST/WATCH `labelSelector` parameters. The -operator joins their identities to Git and uses its existing source-editing machinery. It -performs no local label matching for mirror membership or pruning. This contract makes filtered -`Always` possible without admission emulation or a durable membership inventory. - -## The API - -Add optional `objectSelector`, typed as the upstream `metav1.LabelSelector`, to -[`ResourceRule`](../../api/v1alpha3/watchrule_types.go) and -[`ClusterResourceRule`](../../api/v1alpha3/clusterwatchrule_types.go): - -```yaml -rules: - - apiGroups: [""] - resources: ["secrets"] - objectSelector: - matchLabels: - internal.cozystack.io/tenantresource: "true" -``` - -The field follows the existing admission-style rule vocabulary, but uses LIST/WATCH matching -semantics; admission's old-or-new matching does not apply. The native type's schema checks only -shape, so validate at compilation with `metav1.LabelSelectorAsSelector`, per -[where validation lives](../spec/where-validation-lives.md); no admission webhook. Omitted and -`{}` both select everything: normalize nil explicitly because the helper otherwise means -“nothing.” Invalid selectors refuse the rule; -a valid zero-match selector initializes an empty collection. - -Selectors may use labels that [`sanitize`](../../internal/sanitize/types.go) strips before writing -Git. Selection happens on stored API objects, and labels need not survive in Git for identity -comparison. Preserve unsanitized watch provenance for attribution. - -## One collection is one list/watch request - -Extend [`CollectionKey`](../../internal/types/collection.go) within a GitTarget: - -```text -collection: group, resource, namespace ("" = all namespaces), canonical label selector -watch spec: served version (watchSpec) -``` - -Rules have no operation filter. Deduplicate identical collections and choose their served -version deterministically; every collection observes all object event types. Served version remains -data, so changing it does not move Git files. Keep each selector attached to its own collection. - -### Canonical selector identity - -Validate, normalize equality in `matchLabels` to singleton `In`, sort and deduplicate values, -deduplicate requirements, then sort by the complete `(key, operator, values)` tuple and serialize. -Omitted and empty selectors yield the empty string. This is syntactic normalization, with no -general Boolean simplification. Reuse the identity for requests, planning, cursors, resync -coalescing, render fidelity, and retention scopes. - -`labels.Selector.String()` alone is insufficient: -[`ByKey.Less`](https://github.com/kubernetes/apimachinery/blob/v0.37.0/pkg/labels/selector.go#L158) -compares only keys. Test both orders of `team in (a,b),team notin (b)`, plus reordered values, -duplicates, and equivalent `matchLabels`/singleton-`In` forms; none may restart a watch or cause -an overlap refusal. - -### One selector per overlapping collection - -Collections overlap when their group/resource matches and their namespaces are equal or one is -all-namespaces. **Overlapping collections must have the same canonical selector.** Different -selectors in disjoint namespaces are allowed. For a conflict, the oldest rule by creation -timestamp, then name, keeps the collection; the newer rule reports refusal. - -Without this restriction, one watch could delete an object another still selects, and independent -snapshots could prune each other's documents. Use a single set-based selector for several values. -The existing same-selector all-namespace/named-namespace overlap remains supported with its -[known ordering limitation](../spec/watch-event-ordering-and-attribution-grace.md#overlapping-watches-are-a-separate-ordering-problem). - -## Membership follows Kubernetes - -Use the server's [membership events](../facts/kubernetes-watch-options.md#event-types-describe-membership-in-the-selected-collection): - -| Source observation | Watch event | Git operation | -|---|---|---| -| Matching object created, or existing object enters selection | `ADDED` | `CREATE` | -| Selected object changes and remains selected | `MODIFIED` | `UPDATE` | -| Selected object acquires `deletionTimestamp` | `MODIFIED` | `DELETE` (existing deletion-intent policy) | -| Object disappears or leaves selection | `DELETED` | `DELETE` | - -Label exit and physical deletion have the same Git membership effect. No follow-up GET or local -label check distinguishes them. Prune policy decides whether the document is removed: - -| Mode | Observed exit/deletion | Absent from a complete snapshot | -|---|---|---| -| `Never` | Retain | Retain | -| `OnEvent` (default) | Remove | Retain | -| `Always` | Remove | Remove | - -Every observed removal reaches the normal processing path; there is no rule operation filter. -`Never` disables both Git deletion paths while continuing to mirror creates and updates. All -modes cover plain YAML and supported Kustomize layouts. - -## Snapshot and prune boundary - -For one target and source cluster: - -```text -C = managed Git identities of this group/resource in the requested namespace scope -D = identities in the complete server-selected snapshot, excluding terminating objects -Always removals = C - D -OnEvent/Never retained documents = C - D -``` - -Update identities present in both sets; create Git representations for identities only in `D`. -The target path and write boundaries constrain `C`. Other types and sibling namespaces stay -outside this sweep. No raw or rendered Git-label predicate narrows `C`. - -If Git holds Secrets A and B in `tenant-a`, but the request returns only A, `Always` removes B, -even if B never matched. The retaining modes keep it. Intentionally keeping unselected objects -requires a retaining mode or a separate target path; ownership is not inferred from old labels. - -Match group/resource, effective namespace, and name within the source-cluster context. Name and -namespace alone collide across kinds. API version is representation data; UID distinguishes -incarnations for attribution, while Git identity survives recreation. See -[`ResourceIdentifier`](../../internal/types/identifier.go) and -[Kubernetes object identity](https://kubernetes.io/docs/reference/using-api/api-concepts/#object-names). - -### Retention reporting - -Keep orphan classification for `OnEvent` and `Never`. Under this ownership contract, every -in-scope identity in `C - D` is a document that `Always` would remove, including never-selected -ones. Counting it as retained is intentional; suppressing the count would hide non-convergence. -The count means “absent from the selected mirror,” without claiming physical cluster deletion. - -[`planGitOnly`](../../internal/manifestanalyzer/plan.go) already counts these suppressed drops. -`applyResyncPlan` carries `RetainedOrphans` into its result, and `tallyPruneRetention` publishes -per-type metrics in [`resync_flush.go`](../../internal/git/resync_flush.go). Status uses the -[`retention roll-up`](../../internal/watch/retention_rollup.go), consistent with -[`RetainedDocuments`](../../api/v1alpha3/gittarget_types.go)'s converged-mirror definition. -Include the selector in scope identity and preserve revision checks so retired reports cannot -restore stale counts. A failed observation must not publish a new zero count. -The current roll-up sums per-scope counts, so all-namespace/named-namespace overlap can count a -document twice. That existing limitation is separate from counting never-selected documents. - -### Complete observation and installation timing - -Initial events, every LIST page, the fallback watch, and resumed watches must carry the same -selector. Only completed initialization or a complete consistent LIST supplies `D`; see the -[watch facts](../facts/kubernetes-watch-options.md). Failure, denial, expired pagination, or -unavailable discovery produces no sweep. A successful empty snapshot authorizes removing all -`C` under `Always`. Keep [`desiredFromObject`](../../internal/watch/scope_resolve.go)'s exclusion -of terminating objects. Each snapshot covers its own structural scope; RV is no cross-type clock. - -A complete API observation does not prove that a deployment controller finished installing a -Git revision. Current [`mappingRefusals`](../../internal/manifestanalyzer/acceptance.go) checks -followability/scope, and [`RenderTokenDivergences`](../../internal/manifestanalyzer/render_fidelity.go) -checks rendered tokens against live fields. Neither proves every Git object was installed; see -[render fidelity](support-boundary/render-fidelity.md). Requiring every object to remain present -would also prevent cleanup after offline deletions. - -Enable mirroring when the cluster is ready to be authoritative. Under `Always`, objects awaiting -installation and new Git-first additions can be pruned before deployment. Coordinating that -handoff is an operating responsibility; this design adds no orchestrator reconciliation barrier. - -### Kustomize support boundary - -Use the existing [Kustomize editing boundary](support-boundary/kustomize-support-boundary.md). -Rendering resolves effective identities and source locations and verifies edits. It performs no -label-based prune selection. Existing machinery includes -[`renderRoot`](../../internal/manifestanalyzer/kustomize_render.go), -[`DocumentModel.Rendered`](../../internal/manifestanalyzer/store.go), and -[`VerifyBatchRenders`](../../internal/manifestanalyzer/render_verify.go). - -For Kustomize, `C` describes objects represented by the target's render roots. Patch documents -and build directives are context; an inherited object already suppressed by a deletion patch -is absent from `C` despite its base file remaining. Resolve overlay-supplied namespaces before -identity comparison. Unavailable rendering is an error, never an empty object set. - -Live [`applyDelete`](../../internal/git/plan_flush.go) already handles inherited objects through -an overlay-local `$patch: delete`. Snapshot `applyResyncPlan` instead calls `dropDocument`, which -only removes YAML documents. Day-one `Always` requires a shared semantic removal path that: - -- Resolves effective identities back to source documents and preserves multi-document siblings. -- Cleans `resources:` references when the final document leaves a file. -- Removes inherited objects through overlay-local patches, preserving bases and sibling overlays. -- Records removal intent and verifies intended absence plus unchanged unrelated rendered objects. -- Handles re-entry by retiring a recognizable operator-owned deletion patch and its reference, - then upserting and verifying presence. Persist artifact ownership in Git; an edited patch, - uncertain ownership, or path collision refuses the batch. Retries must be idempotent. - -Retain separate prune gates, attribution, and live/sweep counters. Refuse ambiguous identity or -source mapping, escaped write boundaries, and failed render verification before commit. Nil -`Rendered` is not proof of plain-YAML context. Report the affected candidate and retry after -relevant inputs change; skipped removals must not count as convergence. Build and apply against -one Git revision, rebuilding mappings after fetch/conflict changes. Existing unsupported features -remain unsupported, and no shared base becomes writable through this feature. - -Admission/controller labels can differ from Git labels in either direction: trust the selected -identity set. Removing a representation can cause downstream GitOps pruning to delete the live -object. Rule narrowing or a valid but mistaken selector has the same consequence; the operator -cannot distinguish that mistake from an intentionally smaller mirror. - -### Attribution for filtered removals - -The current [`attachAuthor`](../../internal/watch/target_watch.go) sets `ExactCapable=false` for -all Git deletions. That **does not skip the exact join**: [`FactIndex.Lookup`](../../internal/queue/fact_index.go) -tries the sticky deletion pointer first, then UID/RV, then weaker fallbacks. If Bob's label-exit -fact is missing, Alice's earlier write can author the removal after grace expires. - -There is also an avoidable delay. When an exact PATCH/UPDATE wins lookup, `awaitsBetterEvidence` -still treats it as a write fallback and waits the remaining grace for delete evidence (default -`3s`). This is not true of every exit: eligible delete evidence can finish the wait earlier. - -> **Superseded (2026-10-22).** The selector-aware policy below was replaced by one that never reads -> the selector: a removal of an object that was not terminating is credited first to a write at its -> exact UID/RV (the exit), then to deletion evidence, as any removal is. Real deletions on a selected -> stream now resolve on their delete fact instead of waiting out the grace and shipping unresolved, -> which stalled the stream for N x grace on a prune. The cost is one accepted wrong author, listed in -> [what attribution gives up, on purpose](../spec/attribution.md#what-attribution-gives-up-on-purpose). -> -> **As built until then:** exactly this policy, and the mutationlab `selector-membership` capture -> explains its cost: a deletion never carries the `DELETED` frame's resourceVersion (an immediate delete answers -> with a `Status` holding only the uid; a finalizer-held one is stamped one step earlier), so a real -> deletion from a selected collection is unresolved. A uid-keyed deletion fact is not accepted -> instead: an object relabeled out and then deleted by someone else produces one that would name -> the deleter for the exit. See the -> [attribution contract](../spec/attribution.md#four-rules-that-are-easy-to-miss). - -Filtered `DELETED` needs an explicit lookup policy, retaining raw event type, selector presence, -UID/RV, and deletion timestamp. Require matching audit route, group/resource, UID, and RV; -eligible evidence returns immediately, including a late arrival during grace. A nonterminating -previous object permits an exact PATCH/UPDATE to name a label exit. A terminating previous object -requires exact deletion evidence; a finalizer PATCH must not name the deletion initiator. -Without eligible evidence, expire as unresolved. Do not use last-writer, sticky, collection, -RV-only, or name-only fallbacks for these filtered removals. - -This deliberately extends the [current attribution contract](../spec/attribution.md#four-rules-that-are-easy-to-miss): -physical-removal fallbacks cannot establish who changed a label. Keep existing unfiltered and -`MODIFIED` deletion-intent behavior. Merely changing `ExactCapable` is insufficient; weaker -fallbacks remain. Update the attribution contract with the implementation. Resolution stays -inline and ordered; uncertainty affects authorship only. Resyncs retain configured authorship. - -### Scope integration and queue ordering - -Keep [`ResyncScope.Matches`](../../internal/git/types.go) structural: target, group/resource, and -namespace. The collection identity additionally carries the selector. Update the `CollectionKey`/scope -comments to distinguish the selected collection from the Git objects it owns. -[`markResyncTailForWriteLocked`](../../internal/git/branch_worker.go) must keep matching identifiers: -a `DELETE` may have no object payload, and different selector generations can affect the same file. -A live write prevents a newer snapshot from coalescing ahead of it; selector identities must not -share coalescing keys. - -For replacement, quiesce the retired producer's enqueue path before the new producer enqueues -its initial snapshot. Accepted old work drains ahead of it; none may arrive afterward and restore -an excluded object. Current `targetWatchSet.stop` only cancels. Add producer completion or enqueue -serialization, without making the branch worker consult watch configuration. Preserve the -[ordering regression cases](../spec/watch-event-ordering-and-attribution-grace.md#regression-cases). - -## Rule edits, recovery, and status - -A selector edit starts a new collection and complete snapshot over the same type/namespace -ownership scope. Narrowing `team in (a,b)` to `team=a` removes excluded identities under `Always`, -regardless of stale Git labels or missed UPDATEs; retaining modes keep them on that snapshot. -Widening initializes newly selected objects without waiting for mutations. Equivalent canonical -selectors keep the watch. Removing a type/namespace entirely enqueues no cleanup of its retired -scope; existing [namespace retention](../configuration.md#deletion-policy-specprunemode) still applies. - -Add the canonical selector to [`watchCursorKey`](../../internal/queue/redis_store.go); never resume -under another selector. Render-fidelity and retention scopes follow that same identity and watch -revision. `status.streams` continues counting resource types, rather than watch connections. - -## Documentation and boundaries - -The configuration reference must lead with cluster authority, the prune table and `OnEvent` -default, and the removal of never-selected documents under `Always`. Explain rule narrowing, -installation timing, downstream GitOps deletion, overlap refusals, Kustomize write limits, -retention counts, and unresolved attribution. These are consequences of the chosen contract. - -Filtered requests reduce transferred objects but add watches; measure the cost. Keep requests -filtered because [authorization can depend on selectors](../facts/kubernetes-watch-options.md#selectors-and-authorization). -A denial cannot become an empty snapshot. Field selectors remain separate work. Different selectors over -overlapping scopes need union membership and stale-observation handling; independent sweeps are -insufficient. The existing same-selector overlap ordering gap also remains outside this change. - -When this step ships, remove this design's line from [`.docs-lint-scope`](../../.docs-lint-scope). -It is gated only while the design is active; afterwards it is a historical record. - -## Tests - -Preserve step 1's version, duplicate-collection, unchanged-watch, and replacement checks in -[`target_watch_test.go`](../../internal/watch/target_watch_test.go) and -[`target_watch_plan_test.go`](../../internal/watch/target_watch_plan_test.go), plus sibling-namespace -protection in [`resync_scope_test.go`](../../internal/git/resync_scope_test.go). Add: - -### Selection and pruning - -- Omitted, empty, invalid, zero-match, and sanitizer-stripped-label selectors. Assert identical - query propagation on every initialization, fallback, pagination, and resume path. -- The canonicalization cases above preserve cursor/watch identity; changed selections initialize - afresh. Disjoint namespaces permit distinct selectors; overlaps refuse the newer rule. -- Equal names across resource types/namespaces remain distinct. Version changes and new UIDs do - not orphan Git representations. Missing Git objects use placement; mapping failures refuse. -- Observe entry, label exit, and physical deletion under all modes, with no operation filter. - Simulate missed deletes and updates during disconnection: a fresh snapshot restores present - object state and prunes absent identities only under `Always`. Keep terminating-object behavior. -- With `C={A,B}` and `D={A}`, including never-selected B, `OnEvent`/`Never` retain B and report one - retained document; `Always` removes B and reports zero retained. Check status and per-type - metrics. Other namespaces/types stay untouched. An empty `D` removes all `C` only under `Always`. -- Narrow a selector with matching, stale, and missing Git labels; the same absent identities are - removed under `Always`. Widening adds unchanged newly selected objects. Retiring coverage does - not sweep it, and retired reports cannot overwrite current retention counts. -- Interrupted initialization, a failed/expired LIST page, denied authorization, and unavailable - discovery enqueue no sweep or new zero retention report. A complete empty snapshot does sweep. -- Pre-installation and new Git-first documents are removable under `Always`. Switching prune mode - or introducing a supported Kustomize layout keeps full support without silently retaining. - -### Kustomize - -- Overlay-injected/overridden labels with positive, negative, and `DoesNotExist` selectors; - admission/controller drift in both directions; stripped labels. Only server membership decides - `D`, for both plain YAML and Kustomize. Resolve effective namespaces before matching. -- Partial multi-document deletion preserves siblings; final-document deletion cleans references - and leaves the root buildable. An inherited removal authors an overlay patch, with base and - sibling overlay unchanged. Require parity between live and snapshot removals. -- Restart and re-enter a previously removed inherited object: retire its owned patch, verify - presence, and prove idempotency. Edited/unowned patches and occupied paths refuse without loss. -- Conflicting roots, absent attribution, unavailable renders, out-of-bound writes, and changes to - unrelated rendered objects refuse before commit. Nil `Rendered` cannot imply plain YAML; keep - a valid plain-YAML control. Git revision changes invalidate old mappings before writes. - -### Attribution and ordering - -- Alice's RV 10 fact plus Bob's exact RV 11 label-exit PATCH resolves Bob immediately, without - consuming the full grace, even with unrelated sticky/collection evidence available. Repeat - with Bob arriving during grace and both PATCH and UPDATE. -- Without Bob's fact, expire unresolved despite Alice's fact and available sticky, collection, - RV-only, and name-only fallbacks. Exact filtered deletion evidence resolves immediately; - insufficient evidence remains unresolved. A terminating finalizer PATCH cannot author removal. -- Preserve unfiltered/deletion-intent attribution. Cancel during grace and refuse a full queue - without advancing its cursor. Later events cannot overtake an earlier attribution wait. -- Queue R100 snapshot, payload-free R101 exit, then R103 snapshot where the object re-enters. - R103 cannot coalesce into R100's position; final content contains the object. -- Pause a retired producer before enqueue, replace its selector, and initialize an empty snapshot. - Old work enters ahead of the replacement or is canceled, never afterward. Repeat with old - snapshots and a full queue while unrelated collections continue. -- Run e2e against a real kube-apiserver for initial events, label entry/exit, and offline `Always` - recovery; unit fakes cannot establish these server semantics. diff --git a/docs/facts/kubernetes-apf-and-inflight-tuning.md b/docs/facts/kubernetes-apf-and-inflight-tuning.md deleted file mode 100644 index 979bae7f..00000000 --- a/docs/facts/kubernetes-apf-and-inflight-tuning.md +++ /dev/null @@ -1,266 +0,0 @@ -# Kubernetes API Priority and Fairness — and How to Tune It for High Concurrency - -> **reference** — durable background. Index: [`../INDEX.md`](../INDEX.md) - -When you throw a sudden burst of requests at a Kubernetes API server — say, 300 clients all -authenticating simultaneously — it doesn't just pass everything through. It applies a two-layer -traffic control system: a hard concurrency ceiling called **max-requests-inflight**, and a -sophisticated queuing layer called **API Priority and Fairness (APF)**. Understanding both is the -key to making high-concurrency workloads reliable without adding more hardware. - ---- - -## The inflight ceiling - -Before APF even runs, the API server enforces a raw cap on simultaneous in-flight requests: - -| Flag | Default | Controls | -|---|---|---| -| `--max-requests-inflight` | 400 | Non-mutating requests (GET, LIST, WATCH) | -| `--max-mutating-requests-inflight` | 200 | Mutating requests (POST, PUT, PATCH, DELETE) | - -Any request that arrives when the ceiling is already hit gets an immediate -`HTTP 429 Too Many Requests`. No queuing, no retry — just a rejection. The defaults are -conservative: they were set for mid-2010s hardware and have barely moved since. - -For a k3d/k3s cluster you can raise them at cluster creation time: - -```bash -k3d cluster create my-cluster \ - --k3s-arg '--kube-apiserver-arg=--max-requests-inflight=800@server:0' \ - --k3s-arg '--kube-apiserver-arg=--max-mutating-requests-inflight=400@server:0' -``` - -Or, for an existing cluster started via a shell script, add those args to the `k3d cluster create` line in [test/e2e/cluster/start-cluster.sh](../../test/e2e/cluster/start-cluster.sh). - -Raising the ceiling is a blunt instrument. It lets more requests compete for the same CPU and memory -simultaneously, which can actually make things worse under sustained load. It is most useful when -you have headroom on the node and bursty (not sustained) traffic. - ---- - -## API Priority and Fairness (APF) - -APF is the sophisticated layer that sits *inside* the inflight ceiling. It was promoted to stable in -Kubernetes 1.29 and is on by default in all modern clusters. The goal is to prevent one noisy caller -from starving every other caller — and to give operators control over how the shared budget is -allocated. - -APF is configured through two resource types: - -- **`PriorityLevelConfiguration`** — defines a traffic class (a bucket of capacity and a queue) -- **`FlowSchema`** — defines which requests match a priority level, and how to group them into flows - -### PriorityLevelConfiguration - -A priority level can be `Exempt` (bypasses all limits, for system-critical traffic like the kubelet) -or `Limited` (gets a share of the inflight budget and a queue). - -For `Limited` levels the key fields are: - -```yaml -spec: - type: Limited - limited: - nominalConcurrencyShares: 100 # relative share of the inflight budget - limitResponse: - type: Queue - queuing: - queues: 64 # number of independent queues (more = better fairness) - handSize: 6 # how many queues each request is assigned to (shuffle sharding) - queueLengthLimit: 50 # max requests waiting per queue before rejection -``` - -**`nominalConcurrencyShares` (NCS)** is a proportional share, not an absolute count. If the total -inflight ceiling is 400 and the sum of NCS across all active priority levels is 1000, a level with -100 NCS gets `400 × (100/1000) = 40` concurrent slots. This is recalculated dynamically as levels -become active and idle. - -**`queues` and `handSize`** together implement *shuffle sharding*: each incoming request is hashed -into `handSize` candidate queues and placed in the shortest one. This probabilistically isolates a -misbehaving caller — they are unlikely to land in the same queues as well-behaved callers. More -queues mean better isolation but higher memory overhead. - -**`queueLengthLimit`** is the per-queue depth. Once a queue is full, new requests are rejected with -`HTTP 429`. The total buffer capacity is `queues × queueLengthLimit`. - -### FlowSchema - -A FlowSchema matches requests to a priority level and assigns a *flow distinguisher* — the key used -to group requests within a priority level's queues. - -```yaml -spec: - matchingPrecedence: 800 # lower = evaluated first - priorityLevelConfiguration: - name: my-priority-level - distinguisherMethod: - type: ByUser # or ByNamespace, or None - rules: - - subjects: - - kind: ServiceAccount - serviceAccount: - name: my-sa - namespace: my-ns - resourceRules: - - verbs: ["get", "list"] - apiGroups: ["mygroup.example.com"] - resources: ["myresources"] - namespaces: ["*"] -``` - -**`matchingPrecedence`** controls evaluation order. Every request is matched against all FlowSchemas -sorted by ascending `matchingPrecedence`; the first match wins. The built-in `exempt` schema has -precedence 1; `catch-all` has 10000. - -**`distinguisherMethod`** determines how flows are separated within the priority level's queues: - -- `ByUser`: one flow per authenticated user/SA — fair between callers but concentrates load from a - single busy SA into one flow -- `ByNamespace`: one flow per source namespace -- `None`: all matched requests go into a single flow - -### The built-in priority levels - -Kubernetes ships with several built-in levels you cannot delete (but can reference): - -| Level | NCS | Purpose | -|---|---|---| -| `exempt` | — | Bypass all limits; for `system:masters` and health checks | -| `node-high` | 40 | High-priority node requests (e.g. kubelet status updates) | -| `system` | 30 | Other system components | -| `leader-election` | 10 | Controller leader election; small but never queued behind workload | -| `workload-high` | 40 | Workload controllers (Deployments, etc.) | -| `workload-low` | 100 | Generic service account requests | -| `global-default` | 20 | Catch-all for everything else | -| `catch-all` | 5 | Last resort; very small to force explicit classification | - -The `service-accounts` built-in FlowSchema (precedence 9000) sends all service account requests to -`workload-low`. This is a large bucket (100 NCS) but uses `ByUser` distinguishing — so all requests -from a *single* service account share one flow, and a burst from that SA competes only with itself -within a few queues. - ---- - -## What "ByUser" means in practice under load - -This is the subtle behaviour that surprises most people. - -Suppose your application makes 300 concurrent GET requests from the same service account. With -`ByUser` distinguishing, every single one of those 300 requests maps to the *same flow key*. Shuffle -sharding spreads them across `handSize` (default 6) candidate queues, but they still all land in the -same small region of the queue space. The queue depth limit kicks in quickly, and the rest get -rejected. - -Compare this to 300 requests each from a *different* user. Those spread across all 64 queues, and -each individual user's queue is much less likely to overflow. - -The practical implication: if one service account drives high concurrency, `ByUser` works against -you. `ByNamespace` or `None` (with a dedicated priority level sized for that traffic) is usually -better. - ---- - -## Creating a dedicated lane for a high-concurrency workload - -The right pattern for a workload that legitimately needs high concurrency is to pull it out of the -shared buckets entirely and give it its own priority level with a matching FlowSchema: - -```yaml -apiVersion: flowcontrol.apiserver.k8s.io/v1 -kind: PriorityLevelConfiguration -metadata: - name: quiz-realtime -spec: - type: Limited - limited: - nominalConcurrencyShares: 400 - limitResponse: - type: Queue - queuing: - queues: 16 - handSize: 4 - queueLengthLimit: 200 ---- -apiVersion: flowcontrol.apiserver.k8s.io/v1 -kind: FlowSchema -metadata: - name: quiz-auth-service -spec: - matchingPrecedence: 800 - priorityLevelConfiguration: - name: quiz-realtime - distinguisherMethod: - type: ByResource # spread load by resource type, not by SA identity - rules: - - subjects: - - kind: ServiceAccount - serviceAccount: - name: quiz-access - namespace: vote - resourceRules: - - verbs: ["get", "list", "watch", "create"] - apiGroups: ["examples.configbutler.ai"] - resources: ["quizsessions", "quizsubmissions"] - namespaces: ["vote"] -``` - -Key decisions here: - -- **`matchingPrecedence: 800`** ensures this FlowSchema is evaluated before the generic - `service-accounts` schema at 9000. Without this, the SA would still fall into `workload-low`. -- **`nominalConcurrencyShares: 400`** gives this level a large share. With a 400-request ceiling and - all NCS summing to around 600 (400 + the ~200 from other active levels), this level gets roughly - 260 concurrent slots — far more than the default 100 NCS of `workload-low`. -- **`ByResource` distinguishing** means the flow key is the resource type being accessed, not the - SA. Concurrent reads and writes spread across different flows instead of piling onto one. -- **`queues: 16, queueLengthLimit: 200`** gives a total buffer of 3200 requests, with 16 isolated regions for shuffle sharding. - ---- - -## Would more nodes help? - -Adding **worker nodes** in k3d does not help at all. Worker nodes run workloads but do not run the -API server. All of the APF logic lives in the API server process, which runs on the server node(s) -only. - -Adding **server nodes** (k3d HA mode with multiple server nodes) gives you more API server instances -behind the load balancer, which does increase total inflight capacity linearly. However: - -- k3d HA is significantly more complex to configure and maintain -- Writes still go through a single etcd leader -- The improvement for *read-heavy* burst traffic is real but rarely worth the operational overhead - -For a single-server k3d cluster, the right approach is always: raise the inflight ceiling if the -node has headroom, then create a dedicated APF lane for the high-concurrency workload. These are -both pure YAML/flag changes that take effect without restarting the cluster. - ---- - -## Observability - -APF exposes Prometheus metrics under the `apiserver_flowcontrol_` prefix: - -| Metric | What it tells you | -|---|---| -| `apiserver_flowcontrol_rejected_requests_total` | Requests dropped; labeled by `flow_schema`, `priority_level`, `reason` | -| `apiserver_flowcontrol_request_wait_duration_seconds` | Time spent in queue before execution | -| `apiserver_flowcontrol_request_execution_duration_seconds` | Time actually executing (in the API server) | -| `apiserver_flowcontrol_current_inqueue_requests` | Current queue depth per priority level | -| `apiserver_flowcontrol_current_executing_requests` | Current in-flight count per priority level | - -The fastest diagnostic: `kubectl get --raw /metrics | grep apiserver_flowcontrol_rejected`. Any -non-zero `reason=queue-full` or `reason=concurrency-limit` on a priority level points directly at -the bottleneck. - -You can also inspect the live configuration at any time: - -```bash -kubectl get prioritylevelconfiguration -kubectl get flowschema -kubectl get --raw /debug/api_priority_and_fairness/dump_priority_levels -kubectl get --raw /debug/api_priority_and_fairness/dump_queues -``` - -The `dump_queues` endpoint shows the current queue depths per priority level in real time — useful -for catching thundering-herd events as they happen. diff --git a/docs/finished/config-plane-split.md b/docs/finished/config-plane-split.md index 95c1b01c..597451ff 100644 --- a/docs/finished/config-plane-split.md +++ b/docs/finished/config-plane-split.md @@ -936,7 +936,7 @@ YAML applied to the cluster. The source-cluster corner is its own gated leg 5. **`GitProviderReady` projection trigger and `Ready` gating.** Wire a `Watches(&GitProvider{})` so a provider going un-ready promptly re-reconciles its GitTargets (best, but adds an edge to the set that - [`reconcile-triggering.md`](../design/reconcile-triggering.md) tracks), or lean on the + `reconcile-triggering.md` (removed) tracked), or lean on the 5-minute periodic reconcile (simpler, laggier)? And does `SourceClusterReachable=Unknown` (pre-first-discovery) hold `Ready` at `Unknown`, or is `Ready` allowed to settle on the other axes first? (Proposal: `Watches`, and diff --git a/docs/finished/provider-access-rules.md b/docs/finished/provider-access-rules.md index 96368457..9bcdcbce 100644 --- a/docs/finished/provider-access-rules.md +++ b/docs/finished/provider-access-rules.md @@ -8,7 +8,8 @@ > drafts: `docs/reviews/20261023-gitbranch-claims-from-other-namespaces.md`, > `docs/reviews/20261023-gitbranch-claims-review.md` and `docs/future/gitprovider-branch-rules.md`, > and follows up items 1 and 2 of "Still missing" in the -> [one-writer-per-branch status](../design/one-writer-per-branch-plan.md). +> one-writer-per-branch status (`one-writer-per-branch-plan.md`, removed; its open items are in +> [TODO.md](../TODO.md)). > [Tests to add](#tests-to-add) is the checklist for the PR. ## How it works: one tenant, end to end diff --git a/docs/future/config-surface-for-a-structured-repository.md b/docs/future/config-surface-for-a-structured-repository.md deleted file mode 100644 index 2c6a026a..00000000 --- a/docs/future/config-surface-for-a-structured-repository.md +++ /dev/null @@ -1,417 +0,0 @@ -# The configuration surface, now that a folder has structure - -> Status: proposal — review findings and an ordered plan. Nothing here binds until scheduled. -> Date: 2026-07-24 -> Absorbs and extends the API-surface block left unbuilt by the status and configuration-model -> review — `spec.suspend`, `spec.interval` and the reconcile-request annotation, the -> `CommitRequest` lifecycle, and the `ClusterWatchRule` scope question — which is now sequenced in -> `gittarget-api-wave.md` (removed). - -## The one-sentence finding - -The product's premise — *the kube-apiserver writes back into Git* — is sound and worth -building on. What has drifted is that **`spec.path` is still one string**, while the folder -it names has quietly acquired structure: a layout, render roots, read-only context, build -directives, an inferred placement convention, and a support verdict. The operator now knows -a great deal about that folder, and the API offers the user **no way to say what they expect, -no way to see what was understood, and no way to look before writing.** - -Everything below follows from that one gap. - ---- - -## Part A — Review of `docs/configuration.md` and `docs/architecture.md` - -### A1. Factual errors (fixed on this branch) - -| Where | Was | Now | -|---|---|---| -| `architecture.md:533` | cluster-scoped resources use the literal `` `cluster/` `` | `` `_cluster/` `` — verified against `internal/manifestanalyzer/placement.go:418`, `internal/types/identifier.go:66`, and `configuration.md:700`, which all say `_cluster` | -| `configuration.md:664-665` | two links, both labelled `design/manifest/…` (a path layout that no longer exists), both resolving to the *same* file, one of them claiming to be a different document ("the vision behind it") | one correctly-labelled link | -| `architecture.md:862` | link label `design/manifest/version2/type-followability.md` | correct label | -| `architecture.md:1281` | "GitTarget lifecycle and repo architecture" linked to `architecture.md` — i.e. to itself | links to the support contract, which is what that list was missing | - -### A2. `configuration.md` is stale against the CRD it documents - -The GitTarget status list (`configuration.md:515-524`) names `Ready`, `Reconciling`, `Stalled`, -`Validated`, `EncryptionConfigured`, `StreamsRunning`, `GitPathAccepted`. The CRD's own printer -columns (`gittarget_types.go:283-297`) also ship **`RenderMatchesLive`**, **`GitProviderReady`**, -and **`ClusterProviderReady`**. `RenderMatchesLive` is the condition that carries the entire -kustomize-verification story — the newest and least obvious part of the product — and the -configuration guide does not mention it once. A user who hits it has nowhere to read what it means. - -### A3. The structural problem: three documents are wearing one trench coat - -`configuration.md` is ~1150 lines and is simultaneously - -- a **tutorial** (the setup flow, the quickstart pointers), -- a **field reference** (every spec field of six kinds), and -- a **design-rationale record** (why the provider scopes differ, why omitted ≠ empty, why the - sweep fails closed, why `default` is never auto-created). - -The rationale is genuinely good — better than what Flux has written down, as the maintainer -review says — but it is interleaved with the reference at a 1:1 ratio, so the reference is -unusable at speed and the rationale is unfindable on purpose. Concretely, the reader who wants -to answer *"which folder do I point this at?"* has to get through ~460 lines of Git credential -dialects, known-hosts precedence, commit templates and signing before reaching `GitTarget` at all. - -Recommendation (not urgent, but do it before there are users): - -- `configuration.md` → **reference only**: one section per kind, fields, defaults, a table, an - example. Rationale moves out to `docs/spec/`, linked inline as "why:". -- A new `docs/onboarding.md` → **the tutorial**: pick a repo, scan it, pick a folder, apply, verify. - This is where Part B's missing product step lives. -- Credentials interop (`configuration.md:95-136`) is a *guide*, not configuration — it belongs - beside `github-setup-guide.md`. - -### A4. Three surfaces are described as if they were one - -`configuration.md`'s "Audit ingestion settings" section (line 1027 onward) freely mixes: - -- CRD fields (`ClusterProvider.spec.attribution.auditRoute`), -- Helm values (`attribution.enabled`, `queue.redis.addr`), -- controller flags (`--author-attribution`, `--author-attribution-grace`, `--redis-key-prefix`), - -with no visual marker for which is which. `attribution.enabled` and `--author-attribution` are -the *same knob* under two names in the same paragraph. There is a `docs/config-flag-conventions.md` -already; this section should adopt its vocabulary and label every knob with its surface. - -### A5. `architecture.md` is in good shape - -It is long but it is *one* document with one job, and the ground-rules → mental-model → detail -progression works. Two notes: - -- The "Design documents" list at the end (`:1263`) omits the entire - `docs/design/support-boundary/` tree, which is now where the product's most consequential - reasoning lives. `support-contract.md` should be linked from the top of the file, not the bottom. -- "Operational boundaries" (`:1210`) is the most honest section in the repo and should be - linked from the README. It is currently only reachable by reading 1200 lines. - ---- - -## Part B — What the config surface is missing, and why - -### B0. The direction is right; the API did not keep up - -The doubt in the original question — *"analysing git folders, deciding where to place files, -understanding Kustomize … it's all very cool, but it requires more understanding to pick which -folder to reverse"* — is correct, and it is not an argument against the work. It is an argument -that the work **finished in the engine and never reached the API**. - -The engine now answers, per folder: what layout is this, which directories are render roots, -which files are read-only build context, which are build directives, which documents are -ambiguous, is this folder supportable at all, and where would a new resource go. Every one of -those answers exists in `internal/manifestanalyzer` and in the `manifest-analyzer` CLI. The -GitTarget API exposes exactly **one bit** of it: `GitPathAccepted`. - -That asymmetry produces four concrete problems. - -### B1. There is no "look before you write" - -For a product whose job is *writing to your GitOps repository*, first contact is: apply a -GitTarget, and it starts committing. `spec.prune.mode` guards deletions and is well designed — -but nothing guards the *initial* write, which for a populated folder is the scary one. - -**Proposal: `GitTarget.spec.mode`.** - -```yaml -spec: - mode: Observe # Observe | Write (default Write, or Observe — see below) -``` - -`Observe` runs the entire pipeline — watches, acceptance gate, layout analysis, placement -resolution, plan construction — and reports what it *would* do, without touching the remote. -It is the field the current direction most obviously demands, because the folder analysis is -now the interesting part and it is invisible until after the first commit. - -This is **not** `spec.suspend` (F6 in the maintainer review). Suspend means *stop reconciling -this object*; Observe means *keep reconciling and keep telling me, just don't write*. Ship both; -they answer different questions and Flux users expect `suspend` by name. - -Whether `Observe` should be the **default** is the interesting call. Arguments for: a -reverse-GitOps tool defaulting to "writes to your repo on apply" is a startling default, and -there are no users yet to break. Arguments against: it adds a step to the quickstart, which is -currently the product's best asset. Recommendation: **default `Write`, but make the chart's -`quickstart` render `Observe`** — the fast path stays fast, and the deliberate path is safe. - -### B2. A refusal is a boolean, not a diagnosis - -`GitPathAccepted=False` + reason `UnsupportedContent` + a message string is the whole -report. What the operator actually computed — layout kind, render roots, external bases, -tolerated context files, ambiguous documents — reaches the user only through the message, -or through `-v1` logs, or by running a separate CLI against a separate checkout. - -**Proposal: `GitTarget.status.layout`**, bounded and count-based, exactly like the existing -`status.streams` and `status.retention` precedents: - -```yaml -status: - layout: - kind: KustomizeOverlay # PlainManifests | KustomizeRoot | KustomizeOverlay | Mixed | Empty - renderRoots: 1 - externalBases: 1 # rendered as read-only context, outside spec.path - contextFiles: 3 # rendered, never written - buildDirectives: 1 # kustomization.yaml / .sops.yaml — retained, never swept - ambiguousDocuments: 0 # reachable from >1 render root; degrade to plain in-place edit - observedTime: "2026-07-24T…" -``` - -This is the highest-value, lowest-risk item in this document. The analyzer computes all of it -already; this is a projection, not new logic. It turns "the operator did something clever with -my repo" into "the operator says my repo is an overlay with one external base and three context -files", which is the difference between trusting the product and not. - -`ambiguousDocuments` deserves special mention. Per `configuration.md:779-784`, a document -reachable from more than one render root with differing override chains **silently degrades** -to plain in-place editing, recorded only as a store diagnostic at debug verbosity. That is a -correctness-relevant fallback that is currently invisible in the API. It is exactly the class -of surprise the original question is worried about. - -### B3. Inference cannot be turned off - -Sibling inference (`configuration.md:626-661`) is good, careful work — the namespace-agnosticism -proof, the sensitive/plaintext firewall, the deterministic tie-break. But it is **unconditional**. -A user who wants determinism — "if I didn't declare it, use the canonical path, do not read my -repo's mind" — has no field to say so. - -**Proposal: `GitTarget.spec.placement.mode`.** - -| Value | Ladder that runs | -|---|---| -| `Infer` (default, today's behaviour) | `byType` → `default` → sibling inference → canonical | -| `Declared` | `byType` → `default` → canonical. Inference off. | -| `Strict` | `byType` → `default` only; a type with no declared route is **refused**, not guessed. | - -One enum, no new machinery — the ladder already exists, this just truncates it. `Strict` is what -a platform team standardising a repo layout across many clusters will want, and it converts a -silent surprise into a loud condition. - -### B4. `commitWindow` is on the wrong object - -Flagged in the maintainer review (§3) and worth acting on now. The split today: - -| `GitProvider` (namespaced) holds | Which is really about | -|---|---| -| `url`, `secretRef`, `knownHostsRef`, `allowedBranches` | **the connection** ✅ | -| `commit.committer`, `commit.signing` | **the repo's identity on the platform** ✅ | -| `push.commitWindow` | **a workload's write cadence** ❌ | -| `commit.message.*Template` | **a workload's commit style** ❌ | - -Consequence: two GitTargets sharing one repository — a fast-moving app folder and a -cluster-RBAC mirror — cannot have different batching or different subjects. That is not -hypothetical; it is the normal shape as soon as a second GitTarget exists. - -**Proposal:** move `push.commitWindow` and `commit.message` to `GitTarget`. Keep `committer` -and `signing` on `GitProvider` (they are properties of the key registered with the Git host). - -Implementation note, so this is not underestimated: the commit window is owned by the -`BranchWorker`, which is keyed per `(provider namespace, provider, branch)` and **shared across -GitTargets**. But the open window is already keyed by `(author, GitTarget)` -(`architecture.md:974-980`) — so the duration becomes a property of the open window rather than -of the worker. The worker keeps serializing the branch; only the timer moves. That is a real -change but a contained one, and it is materially easier now than after a release. - -Do it in the same breaking wave as F6/F12, not separately. - -### B5. Onboarding is a product step the docs pretend does not exist - -`docs/design/support-boundary/repo-discovery-and-onboarding-scan.md` is explicit and correct: -**the operator never discovers.** A GitTarget is told exactly one subtree. Argo and Flux scan; -we deliberately do not. - -That architectural call is right — and it makes onboarding a **step the product must provide**, -performed by `manifest-analyzer --mode scan-repo`, which answers precisely the question the -original doubt raises: *which folders can become GitTargets, what layout is each, which are -supported and why not, and what GitTarget/WatchRule would express each one.* - -That CLI is mentioned in `docs/style-guide.md` and `docs/UPGRADING.md`. It appears in -`configuration.md` **zero times** and in `architecture.md` **zero times**. The tool that closes -the gap exists and is undocumented where the gap is felt. - -**Proposal:** `docs/onboarding.md`, step 0 of every non-quickstart install, with the scan output -of a real repo and the GitTargets it suggests. Link it from the README, from `configuration.md`'s -`spec.path` section, and from `architecture.md`'s configuration model. - -### B6. Absorb the outstanding maintainer-review items into this wave - -Still open from the companion review, all API-surface, all cheaper now than after release: - -- **F6** — `spec.suspend` on GitTarget / WatchRule / ClusterWatchRule / GitProvider; - `spec.interval` on GitProvider at minimum; jitter the requeue; - `reconcile.configbutler.ai/requestedAt` + `status.lastHandledReconcileAt`. -- **F10** — CommitRequest lifecycle: `ttlSecondsAfterFinished` or an ownerRef, plus the - `delete` verb. Today they accumulate in etcd forever and the controller cannot reap them. -- **F9** — verify the stored `scope: Namespaced` status-write path on the minimum supported - Kubernetes version (one envtest). -- **F12 remainder** — unify the six near-identical reference shapes onto `fluxcd/pkg/apis/meta` - where they match. - -Plus one this review adds: **the `ProviderNotFound` message for the literal name `default`** -should name the fix (`clusterProvider.createDefault` in the chart, or commit the object). It is -the most likely first-run support ticket and it is a one-line message change. - ---- - -## Part C — Worked examples - -Three repositories, run through today's configuration and through the proposal. These are the -shapes the layout corpus (`test/fixtures/gitops-layouts/`) already covers. - -### C1. Per-environment folders — the `commitWindow` problem - -```text -repo/ - clusters/prod/… # ~40 objects, changes hourly - clusters/staging/… # ~40 objects, changes constantly during the day - platform/rbac/… # ClusterRoles, changes monthly -``` - -Three GitTargets, one GitProvider, one branch. Today they **must** share -`push.commitWindow: 5s` and one set of message templates. - -What you actually want: `staging` batching at `60s` (a `kubectl apply -k` storm should be one -commit, not twelve), `platform/rbac` at `0s` (every RBAC change is individually meaningful and -should be individually reviewable in `git log`), `prod` at the `5s` default. - -Today: impossible without three GitProviders pointing at the same URL — which means three -credential Secrets, three signing configurations, and three `ls-remote` probes per interval -against the same host, for a batching preference. **This is the clearest argument for B4.** - -With B4: - -```yaml -kind: GitTarget -metadata: {name: platform-rbac} -spec: - gitProviderRef: {name: platform-repo} - branch: main - path: platform/rbac - push: - commitWindow: "0s" # every RBAC change is its own commit - commit: - message: - liveTemplate: "rbac: {{.Author}} changed {{.Count}} object(s)" -``` - -### C2. `base` + `overlays` — the folder-choice problem - -```text -apps/podinfo/ - base/ kustomization.yaml + deployment.yaml - overlays/prod/ kustomization.yaml (resources: [../../base], images:, replicas:) - overlays/test/ kustomization.yaml (resources: [../../base], images:) -``` - -**Which folder is the GitTarget?** The API gives no help, and the two plausible answers behave -very differently: - -- `path: apps/podinfo/overlays/prod` — the supported, designed case. The base is read as - read-only context; writes stay in the overlay; a `kubectl set image` lands on the overlay's - own `images:` entry. This is what render-root scoping shipped for. -- `path: apps/podinfo` — **also accepted.** Two render roots inside one target. `base/deployment.yaml` - is now reachable from both, with differing override chains, so per `configuration.md:779-784` - it is **ambiguous** and the writer silently degrades to plain in-place editing of the base — - which means a `kubectl scale` in *prod* rewrites the **base**, changing test too. - -Both report `GitPathAccepted=True` and `Ready=True`. Nothing in `kubectl get gittarget` -distinguishes them. The degradation is a debug log line. - -That is the single most important example in this document, and it is the concrete form of the -original doubt. The proposal addresses it on three sides: - -1. **B5 / scan-repo** tells you `apps/podinfo/overlays/prod` and `…/test` are the GitTargets, - before you apply anything. -2. **B2 / `status.layout`** makes the wrong choice visible after you apply: - `kind: Mixed, renderRoots: 2, ambiguousDocuments: 1`. -3. **B1 / `mode: Observe`** means the wrong choice costs you a status read, not a commit that - changed the wrong environment. - -Optionally a fourth: a `spec.expect.layout: KustomizeOverlay` that refuses when the observed -layout differs. I would **not** ship that yet — `status.layout` first, and see whether anyone -wants to gate on it. Publish the observation before inventing the assertion. - -### C3. A bundle file and a new namespace — the inference problem - -```text -clusters/prod/ - all.yaml # 9 ConfigMaps in one multi-document file, - # spanning namespaces team-a and team-b - team-a/secrets/db.sops.yaml -``` - -The two namespaces are load-bearing, not decoration. Per `configuration.md:652-655`, a resource -in a namespace the target has never written joins an existing cohort **only** when that cohort -has proven itself namespace-agnostic by already holding more than one — one directory holding one -namespace is indistinguishable from a per-namespace layout whose second namespace has not arrived -yet. So with a single-namespace `all.yaml` the answer below inverts, and the new ConfigMap takes -the canonical path. - -Today (`configuration.md:638-646`): a new ConfigMap is **appended to `all.yaml`**, including one -in a brand-new `billing` namespace, because the bundle has cleared that bar. A new Secret goes to -`team-a/secrets/`. Both are the right calls, and both are *inferred from mutable repo state*. - -The failure mode is not the logic; it is that a **human editing the repo changes the operator's -behaviour without touching any Kubernetes object.** Delete every `team-b` ConfigMap from -`all.yaml` and the bundle stops being namespace-agnostic, so the next ConfigMap in a new namespace -takes the canonical path instead. Nothing in the GitTarget changed. Nothing in its status says the -placement basis moved — and this is a sharper version of the same point, because the file the -inference reads is still there and still full of ConfigMaps. - -With B3, a team that finds this unacceptable writes one field: - -```yaml -spec: - placement: - mode: Declared # never read the repo's mind - byType: - v1/configmaps: "{namespace}/configmaps.yaml" - v1/secrets: "{namespace}/secrets/{name}.yaml" -``` - -…and gets the same practical layout, permanently, from a declaration rather than an inference. -Inference stays the default and stays excellent for "point me at an existing repo and it just -works" — which remains the product's best demo. - ---- - -## Part D — Ordered plan - -### Wave 0 — docs, no code (this branch has started it) - -1. ✅ Fix the four factual/link errors (A1). -2. Add `RenderMatchesLive`, `GitProviderReady`, `ClusterProviderReady` to `configuration.md`'s - status list, with one line each (A2). -3. Write `docs/onboarding.md` around `manifest-analyzer --mode scan-repo`, with the C2 repo as - the worked example. Link it from README, `configuration.md`, `architecture.md` (B5). -4. Label every knob in the attribution section with its surface: CRD field / Helm value / - controller flag (A4). -5. Link `support-contract.md` from the top of `architecture.md` and add the support-boundary - tree to its design-document list (A5). - -### Wave 1 — the breaking API wave, before any release - -All of these change or add a spec field. Do them together, in one `feat(api)!` sequence, while -still `v1alpha3` and while there are no users. - -1. **B4** — `push.commitWindow` and `commit.message` move `GitProvider` → `GitTarget`. -2. **B1** — `GitTarget.spec.mode: Observe|Write`; chart quickstart renders `Observe`. -3. **F6** — `spec.suspend` on the four reconciled kinds; `spec.interval` on GitProvider; - requeue jitter; `reconcile.configbutler.ai/requestedAt` + `status.lastHandledReconcileAt`. -4. **B3** — `spec.placement.mode: Infer|Declared|Strict`. -5. **F10** — CommitRequest TTL/ownerRef + the `delete` verb. - -### Wave 2 — legibility, non-breaking, ships any time after - -1. **B2** — `GitTarget.status.layout`, including `ambiguousDocuments`. Highest value per line - of code in this document. -2. The `default` ClusterProvider not-found message names the fix (B6). -3. **F9** — the one envtest. -4. `configuration.md` splits into reference + rationale + guides (A3). - -### Explicitly not now - -- `spec.expect.layout` — publish the observation (B2) before inventing the assertion. -- Making `Observe` the global default — revisit once there is one real user's onboarding to - watch. -- Ordered placement rules (Option A of the placement design) — `byType` + `mode` covers the - demand we can actually see. diff --git a/docs/future/direction-and-configuration-surface.md b/docs/future/direction-and-configuration-surface.md index ec6fe798..81ac14b0 100644 --- a/docs/future/direction-and-configuration-surface.md +++ b/docs/future/direction-and-configuration-surface.md @@ -3,7 +3,7 @@ > Status: proposal — a strategy review with a decided Helm standpoint and worked > configuration examples. Nothing here binds until scheduled. > Date: 2026-08-27. -> Companions: [`config-surface-for-a-structured-repository.md`](config-surface-for-a-structured-repository.md) +> Companions: `config-surface-for-a-structured-repository.md` (removed) > (the field-level review this extends), > [`../layout/model.md`](../layout/model.md) and > `../design/gittarget-api-wave.md` (removed) (the API work this @@ -158,7 +158,7 @@ against what actually exists. ## Where the configuration surface should go — by example The one-sentence defect stands as stated in the -[config-surface review](config-surface-for-a-structured-repository.md): `spec.path` is one +config-surface review (removed): `spec.path` is one string while the folder has structure, and the API offers no way to declare expectations, see what was understood, or look before writing. The layout model and the API wave are the fix. The examples below show the surface the two directions actually need — and that it is diff --git a/docs/misconceptions.md b/docs/misconceptions.md index 518d3e27..ffbb77e1 100644 --- a/docs/misconceptions.md +++ b/docs/misconceptions.md @@ -73,7 +73,7 @@ Two habits caused more of these than any single gap in knowledge. They are liste the applier's writes arrived after the window, were mirrored as edits, and reverted the newer API edit. *Fixed:* the window closes on watch progress, with the mark taken as the newest `resourceVersion` among the collection's objects, never the list's - ([review](review/20261026-apply-window-late-events.md), `internal/git/apply_barrier.go`). + (#456, `internal/git/apply_barrier.go`). - **The newest `managedFields` entries being the applier's alone proves the applier wrote.** An update that removes every field a manager owns deletes that manager's entry, so an older kustomize-controller entry ends up newest. *Cost:* a `kubectl label x tmp-` undo was withheld, and @@ -174,9 +174,10 @@ Two habits caused more of these than any single gap in knowledge. They are liste - **`windowPathKey` is the Git path after placement.** It is the `GitTarget` path joined with the generated file name; the write plan resolves the destination only at commit ([concurrent-author-windows.md](design/concurrent-author-windows.md)). -- **The push cooldown is inert now that the commit window batches.** Under alternating authors an - identity change closes the window, the silence timer never fires, and the cooldown is the only - batching left. The earlier "remove it" was retracted ([push-cooldown.md](design/push-cooldown.md)). +- **The push cooldown is inert now that the commit window batches.** A write of another `GitTarget`, + or one whose footprint overlaps or cannot be proven, still closes the open windows early, and + windows open side by side close within seconds of each other; the cooldown batches those commits. + The earlier "remove it" was retracted ([push-cooldown.md](design/push-cooldown.md)). - **An idle apply follow ends when the worker's record ages out.** That happened only when another save resolved, so long applies were reported `Lost` and quiet workers followed forever. *Fixed:* an explicit `SaveApplyTimeout` (#460). @@ -263,7 +264,7 @@ Two habits caused more of these than any single gap in knowledge. They are liste ([e2e-signing-followups.md](finished/e2e-signing-followups.md)). - **Migrating to Forgejo is required to unpin Gitea.** Both moved to Go's `CrossOriginProtection`, which a plain Go client passes; deleting the `_csrf` scrape unpinned Gitea in place - ([e2e-git-server-choice.md](design/e2e-git-server-choice.md)). + (#253). ## Kustomize, rendering and placement diff --git a/docs/review/20261026-apply-window-late-events.md b/docs/review/20261026-apply-window-late-events.md deleted file mode 100644 index aaed75da..00000000 --- a/docs/review/20261026-apply-window-late-events.md +++ /dev/null @@ -1,269 +0,0 @@ -# The apply window misses late events from a cluster with no audit stream - -On a source cluster that sends no audit facts, every event the watch delivers waits the full -attribution grace (10s in e2e) before it reaches the branch worker. The stream resolves events one at -a time, so the waits add up. The apply window closes on a wall clock, 10s after the applier reports -the apply finished, so the applier's own writes can reach the worker after the window has closed. -A write that arrives then is mirrored as an edit. If that write was an apply reverting a newer API -edit, the revert lands in Git and the edit is lost. - -This was found by the source-cluster e2e for gitops-api's production topology (ask #34): Flux in the -operator's cluster applies into a kcp workspace, and the GitTarget mirrors that workspace through a -`secretRef` KubeProvider. The e2e kcp has no audit stream. The run is at commit `cf3b97be`. - -Status: fix 2 below is implemented (`internal/git/apply_barrier.go`, `internal/watch/apply_barrier.go`), -with one change from the first draft: the mark is the newest resourceVersion among the collection's -objects, not the list's own, so a quiet stream does not wait for a bookmark. - -## The apply window, in short - -When the branch worker asks an applier to apply a commit, it opens an **apply window** for that -applier. Every live event for a GitTarget the applier covers is judged against it: - -- A write by the applier's field manager alone, to an object nobody edited since the window opened, - is **the apply landing**. Its content is the commit's, which Git already holds, so it is not - mirrored (`apply_echoes_total{outcome="not_mirrored"}`). -- A write by the applier's field manager alone, to an object the API side edited after the window - opened, is **the apply arriving late over that edit**. It is withheld, and a Repair puts the edit - back on the cluster (`apply_window_collisions_total{outcome="withheld"}`). -- Any other write is an edit, and is mirrored as usual. - -The window opens when the attempt is sent (`internal/git/applier_claim.go`, `openApplyWindow`) and -closes `applyWindowGrace` (10s) after the operation completed (`settleApplyWindow`). With no window -open, a write by the applier's manager alone is mirrored as an edit, and counted as -`apply_echoes_total{outcome="late"}`. - -```mermaid -flowchart LR - Send["Worker sends the attempt"] -->|window opens| Open(["Apply window open"]) - Open --> Done["Applier reports the operation completed"] - Done -->|"+ 10s grace"| Closed(["Apply window closed"]) - - Event["A live event arrives at the worker"] --> When{"Is a window open
when it arrives?"} - When -->|yes| Judge["Judged: the apply landing,
withheld, or an edit"] - When -->|no| Mirror["Mirrored as an edit
(late, if Flux wrote it)"] -``` - -The window is judged by **when the event arrives at the worker**, not by when the write happened on -the cluster. That gap is the whole problem. - -## Why events from the workspace arrive late - -Between the watch and the worker, each event waits for its author: an audit fact that names who made -the write. On a cluster with audit, the fact usually arrives within a second or two. On a cluster -with no audit stream, no fact ever arrives, so every event waits the full grace -(`--author-attribution-grace`, 10s in e2e) and then goes on unattributed. In this run, the median of -`attribution_resolution_wait_seconds` was 10s. - -A watch stream handles its events in order, one at a time (`internal/watch/target_watch.go`, -`attachAuthor` is called inline). So an event queued behind others waits for their waits as well. - -```mermaid -sequenceDiagram - participant K as kcp workspace - participant S as Watch stream - participant A as Attribution - participant W as Branch worker - - K->>S: write 1 (Flux) - K->>S: write 2 (an API edit), 1s later - S->>A: who wrote 1? - Note over A: no audit fact ever comes
waits the full 10s - A-->>S: unattributed - S->>W: event 1, 10s after write 1 - S->>A: who wrote 2? - Note over A: waits another 10s - A-->>S: unattributed - S->>W: event 2, 19s after write 2 -``` - -## What this run observed - -The collision spec pushed a Git-side change (`o1` to `during-other`), waited until the FluxClaim -showed the apply had been asked for, and then edited `during` through the workspace's API. These -are the worker's log lines, with the times the writes happened inferred from the spec's log. - -```mermaid -sequenceDiagram - participant T as Test - participant W as Branch worker - participant F as Flux - participant K as kcp workspace - - T->>W: push o1 (commit a9cb18d) - W->>F: 16:36:16 apply a9cb18d (ForeignCommit) - Note over W: window opens - F->>K: about 16:36:16, writes during-other - T->>K: about 16:36:17, edits during - F-->>W: about 16:36:20, operation completed - Note over W: 16:36:30 window closes
(completion + 10s) - K-->>W: 16:36:32, Flux's write to during-other arrives - Note over W: no window open:
mirrored as an edit, late echo counted - K-->>W: 16:36:42, the edit of during arrives
(25s after it was made) - Note over W: mirrored and committed -``` - -Two things happened here: - -- **Flux was faster than the edit.** It wrote before the spec's edit landed, so it never reverted the - edit. There was nothing to withhold, the edit reached Git and stayed on the workspace, and - `apply_window_collisions_total` stayed at 0. That is why the spec failed: it asserts that one - collision is counted. Waiting for the operation token and then editing does not guarantee that - the edit lands before the applier writes. The local E5 spec uses the same technique and passes - whichever side wins, which is why it never asserts the counter. -- **One of Flux's writes arrived after the window closed.** `apply_echoes_total{outcome="late"}` is 1 - on this branch. Here it was harmless, because the write held what Git already held. - -## What goes wrong when Flux does revert an edit - -Had the edit landed before Flux's write, Flux would have applied the commit it was asked for, which -does not hold the edit yet, and so reverted it on the workspace. The window exists to catch that -write. With the delays above, the write reaches the worker after the window has closed: - -```mermaid -sequenceDiagram - participant U as Someone editing - participant W as Branch worker - participant F as Flux - participant K as kcp workspace - participant G as Git - - W->>F: t+0 apply commit C (does not hold the edit) - Note over W: window opens - U->>K: t+1 edit: value = "edit" - F->>K: t+2 apply C: value = "old" (reverts the edit) - F-->>W: t+4 operation completed - K-->>W: t+11 the edit arrives - Note over W: window open: recorded as an edit,
mirrored - W->>G: commit value = "edit" - Note over W: t+14 window closes - K-->>W: t+21 Flux's revert arrives - Note over W: no window open: mirrored as an edit - W->>G: commit value = "old" - Note over G,K: Git and the workspace both hold "old".
No Repair is asked for. The edit survives
only in Git history. -``` - -This is the loss the apply window was built to prevent (#29), and from such a cluster nothing in the -window can catch it. - -A longer grace does not fix it. Events wait in a queue, so the delay grows with the number of events -ahead: the edit in this run waited 25s, more than twice the grace, because it queued behind Flux's -write. No fixed grace covers every burst. - -## Fix 1: do not wait for audit facts that never come - -The KubeProvider's `AuditFactsReceived` condition says whether an audit fact has ever arrived on its -route. When it is not `True`, waiting for one is pointless, so attribution could skip the wait for -events from that provider. Events would then reach the worker within moments of the write, inside -the window. - -```mermaid -flowchart LR - E["Event from the watch"] --> Q{"Has this KubeProvider ever
received an audit fact?"} - Q -->|yes| Wait["Wait up to the grace
for this write's fact"] - Q -->|no| Skip["Go on unattributed
at once"] - Wait --> W["Branch worker"] - Skip --> W -``` - -It is small, and it also removes the 10s or more of delay on every edit mirrored from such a cluster. -It has two limits: - -- On a newly added audited provider, writes made before its first fact arrives are recorded with no - author. With the CommitRequest takeover (`cf3b97be`), a request could then take over their window. -- It does nothing for a cluster that does send audit facts but sends them slowly. The window still - closes on the clock. - -## Fix 2: close the window by how far the watch has got - -The idea in one sentence: instead of closing the window "10 seconds after Flux said it was done", close -it "once the watch has delivered everything that happened on the cluster up to the moment Flux said it -was done". - -Every write to a Kubernetes object gets a **resourceVersion**, a position in the cluster's log of -changes, and a watch delivers events in that order. So the worker can mark a position and wait until -the watch has passed it: - -1. **Take a mark when the apply completes.** The worker lists the watched collection and takes the - newest resourceVersion among its objects, say `1284`: the position of the latest write to anything - in it. Every write Flux made for this apply happened before the apply completed, so each has a - resourceVersion of at most `1284`. -2. **Keep the window open until the watch passes the mark.** The stream records how far it has got - each time it records its cursor: after every event it handled, routed or not, and every - **bookmark**, a progress-only event in which the API server says "you have everything up to - resourceVersion X". Once it is at or past `1284`, it puts an answer on the worker's queue. -3. **Then close.** The answer rides the worker's queue behind every event the stream routed before - it, so once every stream has answered, every write Flux made has reached the worker, however long - attribution held each one. The window then closes at the end of its grace, or at once if that has - passed. - -The list's own resourceVersion would be the cluster's current position, which a quiet stream passes -only on a bookmark, and a bookmark can be a minute away. An object's resourceVersion is a write the -stream has seen or will see. - -```mermaid -sequenceDiagram - participant F as Flux - participant K as kcp workspace - participant S as Watch stream - participant W as Branch worker - - W->>F: apply commit C - Note over W: window opens - F->>K: writes (resourceVersions 1281, 1284) - F-->>W: operation completed - W->>K: list the collection - K-->>W: newest object resourceVersion 1284, the mark - Note over W: window stays open until the
stream passes 1284 - S->>W: event at 1281 (after its attribution wait) - Note over W: judged inside the window - S->>W: event at 1284 (after its attribution wait) - Note over W: judged inside the window - S->>W: the stream's answer: at 1284 - Note over W: past the mark: everything Flux
wrote has arrived. Window closes. -``` - -The decision the worker makes, for each window: - -```mermaid -flowchart TB - Done["Operation completed:
mark = the newest object resourceVersion now"] --> Check{"Has every stream of the
applier's GitTargets handed over
an event or bookmark at or past the mark?"} - Check -->|yes| Close(["Close the window"]) - Check -->|"not yet"| Bound{"Held longer than
the upper bound?"} - Bound -->|no| Check - Bound -->|yes| Give(["Close anyway, and log and count it"]) -``` - -What it costs: - -- **resourceVersions are opaque in the API contract.** Clients are not promised that they compare as - numbers, though the API server's own watch cache compares them that way on etcd. A source whose - resourceVersions do not parse as a number fails the barrier, and its window closes after its grace - alone, as before. -- **A write Flux deleted is not in the list**, so its event can still arrive after the window. A - DELETE is never withheld anyway, and the object it removes is one Git no longer holds. -- **A list per stream per apply.** It is paged, and made once when the operation completes. -- **A GitTarget can have several streams**, one per watched collection, and the applier can cover - several GitTargets. The window waits for all of them. -- **An upper bound** keeps a stream that is disconnected or stuck from holding the window open - forever. It is two minutes. Past it the window closes, and the close is logged and counted - (`apply_window_barriers_total{outcome="timed_out"}`), so the miss is visible. -- **A window held open longer** judges more writes. A write by Flux alone to an object nobody edited - is treated as the apply landing and not mirrored. That is still correct for an apply nobody asked - for, because its content is what Git holds. - -Fix 2 is the general fix: it covers a cluster with no audit stream, a cluster with slow audit, and a -burst of events of any size, because it waits for the watch itself rather than guessing a delay. -Fix 1 can come on top of it, to cut the delay on every mirrored edit from a cluster with no audit -stream. - -## The spec, separately - -Neither fix makes E5's way of provoking a collision deterministic: it edits once the apply was asked -for, and Flux often writes within a second of the resume, before the edit lands. The source-cluster -spec uses the delay this document is about instead. It edits first and pushes a Git-side change -right after. The edit's event waits out the attribution grace, so the push is heard, its apply asked -for and applied before the edit reaches the worker. The applied commit does not hold the edit, so -Flux reverts it, and with fix 2 the window still judges that write when it arrives, and withholds -it. That holds while the attribution grace (10s in e2e) outlasts the time from the push to the -apply request. diff --git a/docs/spec/watch-event-ordering-and-attribution-grace.md b/docs/spec/watch-event-ordering-and-attribution-grace.md index a64f30e9..cdb1273a 100644 --- a/docs/spec/watch-event-ordering-and-attribution-grace.md +++ b/docs/spec/watch-event-ordering-and-attribution-grace.md @@ -7,7 +7,7 @@ > Related: [Kubernetes watch options](../facts/kubernetes-watch-options.md), > [Architecture](../architecture.md), > [Attribution contract](attribution.md), -> [Label-selection design](../design/watches-labels-simplification.md) +> [Selecting objects by label](../configuration.md#selecting-objects-by-label-rulesobjectselector) ## The question @@ -162,17 +162,15 @@ test today. cursor unadvanced; queue admission is the boundary asserted by this check. - For a selector replacement, pause the retired producer immediately before enqueue and initialize the replacement. The retired work must enter ahead of the replacement snapshot or - be canceled. It cannot arrive behind that snapshot and restore an excluded object. The - [label-selection design](../design/watches-labels-simplification.md#scope-integration-and-queue-ordering) - requires a producer handoff for this case, and `producerGate` in + be canceled. It cannot arrive behind that snapshot and restore an excluded object. This needs + a producer handoff, and `producerGate` in [`target_watch.go`](../../internal/watch/target_watch.go) provides it: retiring a stream waits out an enqueue in flight and admits none after. The check lives in [`producer_gate_test.go`](../../internal/watch/producer_gate_test.go). - Exercise overlapping watches with delayed attribution on one. Assert each watch's own order; - do not infer a global mutation order from their combined arrivals. The label-selection design - refuses conflicting selectors but retains the existing same-selector namespace overlap. + do not infer a global mutation order from their combined arrivals. Conflicting selectors are + refused, but the existing same-selector namespace overlap is retained. -The label-selection design owns the additional -[selector, attribution, and prune cases](../design/watches-labels-simplification.md#tests), including -missing label-removal audit facts. Those cases concern which content and author reach Git in -addition to the ordering checks here. +Selector, attribution and prune cases, including missing label-removal audit facts, concern which +content and author reach Git rather than ordering; the +[label-selection contract](../configuration.md#selecting-objects-by-label-rulesobjectselector) covers them. diff --git a/internal/git/apply_barrier.go b/internal/git/apply_barrier.go index 44dbf66c..c900839c 100644 --- a/internal/git/apply_barrier.go +++ b/internal/git/apply_barrier.go @@ -19,7 +19,7 @@ import ( // not cover one that is behind: every event waits for its audit fact in turn, and on a source cluster // that sends none each waits the whole attribution grace, so the applier's writes can arrive long // after the window closed. Mirrored then, a write that reverted an edit made during the apply put the -// revert in Git and lost the edit (docs/review/20261026-apply-window-late-events.md). +// revert in Git and lost the edit (fixed in #456). // // So a window whose operation completed also waits for the watch. For each GitTarget the applier // covers, the watch side lists every stream's collection and takes the newest resourceVersion among diff --git a/internal/git/branch_owner.go b/internal/git/branch_owner.go index 65af9dd9..6c68c2cd 100644 --- a/internal/git/branch_owner.go +++ b/internal/git/branch_owner.go @@ -13,8 +13,7 @@ import ( // One GitBranch per repository branch. // // A branch worker is keyed by its repository and branch (BranchKey) and pushes with one credential, -// so exactly one GitBranch may write a repository branch: the one holding its claim. The rules are -// the plan's (docs/design/gitbranch-and-kubeprovider-plan.md): +// so exactly one GitBranch may write a repository branch: the one holding its claim. The rules: // // 1. A GitBranch becomes a candidate once its credential opens a receive-pack session on the // remote (ProvePushSession). An unproven GitBranch claims nothing, so creating one first diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index eb264e11..1b8d25b2 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -2061,7 +2061,8 @@ func (w *BranchWorker) ensureBaseForCycle(provider *configv1alpha3.GitProvider, // Resolve credentials only on the branch that touches the remote. A cycle planning on a // trusted base never reads the credentials Secret at all, which is a saved API GET per cycle - // now that the Secret cache is disabled. See docs/rbac.md §5. + // now that the Secret cache is disabled. See docs/rbac.md, "The operator does not read + // Secrets wholesale". auth, err := getAuthFromSecret(w.ctx, w.Client, provider, w.sshHostKeys, w.credentialPolicy) if err != nil { return fmt.Errorf("resolve auth: %w", err) diff --git a/internal/git/branch_worker_credread_test.go b/internal/git/branch_worker_credread_test.go index 0880fb3c..f1e7b5ea 100644 --- a/internal/git/branch_worker_credread_test.go +++ b/internal/git/branch_worker_credread_test.go @@ -27,7 +27,8 @@ import ( // re-read the Git credentials Secret for every commit in a push cycle. Only the first commit // (the checkout holds no retained write yet) touches the remote through PrepareBranch and needs auth; later commits // build on the local repo and must not issue another Secret GET — with the Secret cache disabled -// that would be a wasted API round-trip per commit. See docs/rbac.md §5. +// that would be a wasted API round-trip per commit. See docs/rbac.md, "The operator does not read Secrets +// wholesale". func TestCommitPendingWrites_ResolvesCredentialsOncePerPushCycle(t *testing.T) { tempDir := t.TempDir() remotePath := filepath.Join(tempDir, "remote") diff --git a/internal/manifestanalyzer/kustomization_parse.go b/internal/manifestanalyzer/kustomization_parse.go index c399ac26..0b9ce8fd 100644 --- a/internal/manifestanalyzer/kustomization_parse.go +++ b/internal/manifestanalyzer/kustomization_parse.go @@ -17,7 +17,6 @@ import ( // reflecting over that type's fields, which makes the check exhaustive by construction: a field // the operator has never heard of lands outside supportedKustomizationFields and refuses the // folder rather than being silently tolerated. -// See docs/design/support-boundary/kustomize-support-boundary.md §7. // supportedKustomizationFields names the struct fields of kustypes.Kustomization // that the operator models. Every other field, present and non-zero, refuses the diff --git a/internal/manifestanalyzer/kustomize_render.go b/internal/manifestanalyzer/kustomize_render.go index 1912a0f2..c6e36d7c 100644 --- a/internal/manifestanalyzer/kustomize_render.go +++ b/internal/manifestanalyzer/kustomize_render.go @@ -22,8 +22,6 @@ import ( // re-implementing its transformers. It is the ground truth the projection is // checked against: what we believe a folder renders to becomes what the library // Flux renders with says it renders to. -// -// See docs/design/support-boundary/kustomize-support-boundary.md §7. // The provenance kustomize emits when buildMetadata asks for it. These are the // reason we need not walk the resources graph ourselves: diff --git a/internal/manifestanalyzer/override_chain.go b/internal/manifestanalyzer/override_chain.go index becafe21..83d358e7 100644 --- a/internal/manifestanalyzer/override_chain.go +++ b/internal/manifestanalyzer/override_chain.go @@ -21,8 +21,6 @@ import ( // What the annotation does NOT say is which images: ENTRY supplied a value — kustomize // keeps no field-level provenance at all — so the projection still has to derive the // supplier itself. See overrides_projection.go. -// -// See docs/design/support-boundary/kustomize-support-boundary.md §7. // reasonRenderFailed marks a render root kustomize could not build. The folder is // refused: if the build fails, Flux cannot deploy it either. diff --git a/internal/manifestanalyzer/store.go b/internal/manifestanalyzer/store.go index 1a4947b7..c57e2aa7 100644 --- a/internal/manifestanalyzer/store.go +++ b/internal/manifestanalyzer/store.go @@ -969,7 +969,6 @@ func collapseAssignments(nsByFile map[string]map[string]string) map[string]names // kustomize resolves a remote base by shelling out to `git fetch`, under LoadRestrictionsRootOnly // and an in-memory filesystem alike (both measured), and no build option turns it off. Refusing // before any build is invoked is what keeps "the operator never fetches a remote base" true. -// See docs/design/support-boundary/kustomize-support-boundary.md §7. func hasRemoteResource(entries []string) bool { for _, e := range entries { if isRemoteResource(e) { diff --git a/test/mutationlab/e2e/selector_membership_test.go b/test/mutationlab/e2e/selector_membership_test.go index bb4ef0a1..377c9227 100644 --- a/test/mutationlab/e2e/selector_membership_test.go +++ b/test/mutationlab/e2e/selector_membership_test.go @@ -39,7 +39,7 @@ const ( // TestSelectorMembership captures what a label-selected collection sees, beside the unfiltered // stream and the audit and admission records of the same writes. It is the evidence behind the -// objectSelector design (docs/design/watches-labels-simplification.md): the API server decides +// objectSelector contract (docs/configuration.md, "Selecting objects by label"): the API server decides // membership, a label change that stops matching is a collection removal even though the object // still exists, and the only evidence that names who caused such a removal is the write at the // removal's exact resourceVersion.