From 2df357262e9aa447b2f5583e143d107e7c5ef5a4 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sat, 24 Oct 2026 08:05:02 +0000 Subject: [PATCH 1/4] docs: fold docs/reviews into the design docs and one open-work list docs/reviews/ held review rounds for merged PRs. Resolved and superseded reviews are deleted; what is still open moves to docs/design/ and docs/future/, and docs/TODO.md becomes the open-work list, led by what must land before the next release. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/TODO.md | 467 +++++++++-------- .../applier-claim-held-applier-watch.md | 122 +++++ .../attribution-removal-wait-options.md | 7 + docs/design/e2e-suite-trim.md | 73 +++ ...t-dropped-while-render-fidelity-pending.md | 342 +++++++++++++ docs/design/one-writer-per-branch-plan.md | 4 +- docs/future/gitops-api-open-asks.md | 124 +++++ docs/future/idea-applier-claim-ownership.md | 46 ++ .../idea-argocd-applicationset-claim.md | 139 +++++ docs/future/idea-source-namespace-selector.md | 66 +++ docs/reviews/20261022-apply-binding-rename.md | 479 ------------------ 11 files changed, 1177 insertions(+), 692 deletions(-) create mode 100644 docs/design/applier-claim-held-applier-watch.md create mode 100644 docs/design/e2e-suite-trim.md create mode 100644 docs/design/live-edit-dropped-while-render-fidelity-pending.md create mode 100644 docs/future/gitops-api-open-asks.md create mode 100644 docs/future/idea-applier-claim-ownership.md create mode 100644 docs/future/idea-argocd-applicationset-claim.md create mode 100644 docs/future/idea-source-namespace-selector.md delete mode 100644 docs/reviews/20261022-apply-binding-rename.md diff --git a/docs/TODO.md b/docs/TODO.md index 8e5ad683..705c85a7 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -1,219 +1,264 @@ # TODO -This file is meant to track the smaller current backlog, not historical notes. - -## Follow-ups from #404 (commit window surface) - -PR #404 is merged; these need fix PRs against main, in this order. (Its CommitRequest timeout -finding is fixed in #407: the controller no longer fails a request the worker holds.) - -The two `CommitEmpty` findings (a suspended target reporting `Ready=True` with no commit, and a -record made while the render-fidelity gate was closed) are fixed in #411: -a request on a target that may not be written fails, whatever path ends it -(`internal/git/write_gate.go`). - -Retry progress on idle branches is fixed: a failed publication schedules its own bounded retry -(`internal/git/retry.go`, [`push-cooldown.md`](design/push-cooldown.md) §7 option C). A window whose -rebuild fails is no longer dropped: it stays pending for that retry -([`gittarget-branch-worker-pending-writes.md`](design/gittarget-branch-worker-pending-writes.md)). - -**Later, non-blocking:** - -- [ ] `expireWaitingCommitRequests` ranges over a map, so empty commits for saves that time out in - the same pass land in random order; sort by `seq`. -- [ ] `buildRequestRecordWrite` keeps only the username; carry the full `UserInfo` so the record has - the same author identity as a window commit. -- [ ] A record commit skips `requestTemplate`; fix the code or the doc comment that says it is phrased - like every other commit. -- [ ] Nits: a stale doc comment stacked on `commitWindowFor`; kstatus test names that still say - "close-delay". - -## Current backlog - -- [ ] After #407 and the #404 fixes above, take the first - [parent-observation step](design/gittarget-parent-observation.md#implementation-and-validation): - retain parent availability and its timestamp after the write branch exists. Use existing - advertisements, with no extra connections or changes to readiness or publication. Keep ancestry - classification and empty-repository bootstrap policy in separate changes. The - [merge review](design/gittarget-parent-hardening.md#merge-review-at-93120caf) records the two - recovery fixes made before #407 merged. - -- [ ] Finish making `GitTarget` turn red quickly and explain why no commit was made. - The plan is in [gittarget-red-status-plan.md](design/gittarget-red-status-plan.md). This is the - replacement direction for Git write preflight: no admission webhook Git work, no extra state - source, and no promise before etcd. The first slice has shipped: stale refusal messages now - republish, scoped refusal recovery is not over-broad, bounded refusal messages include stable - issue detail, and the unsupported-folder e2e covers Git-side break and recovery. Remaining work is - the broader reason/status contract table and any later durable restart behavior. - -- [ ] **High availability (near-term priority).** Support running more than one controller replica; - `replicaCount > 1` is currently hard-rejected by the chart. Redis/Valkey is already the shared store - for watch resume cursors, command-author facts, and — since the fact-stream switchover — the - per-`(route, group/resource)` attribution fact streams, whose independent cursors are the primitive - HA needs for an audit POST and a watch shard that land on different replicas. The remaining - work is leader/ownership coordination so two replicas never write the same `GitTarget`, plus the - durable worker queue below, plus one small ordering call: whether a replica warms its fact index - before starting a watch it has taken over. Redis becomes required (not just advised) in this mode. - -- [ ] Optional: capture CommitRequest authors without Redis in single-pod mode. - The admission webhook is on by default but no-ops author capture without Redis (it writes the author - to Redis and the controller reads it back). Since both run in one process today (`replicaCount=1`), - an in-memory `CommandAuthorStore` fallback would make capture work with no Redis. Now the *only* - thing that still forces Redis on a single replica, since `--author-attribution-transport=memory` - runs attribution in-process. Only useful until HA lands (HA needs the shared Redis store); low - priority. - -- [ ] Prevent same-repository write collisions across multiple `GitProvider` objects. - Decide whether the fix should be validation, a shared queue/lock per repo, or both. - Until then, keep recommending one `GitProvider` per repository. - -- [ ] Re-assess unifying `PendingWriteAtomic` and `PendingWriteCommit` into one shape. - Trade-off and complexity sketch in - [docs/future/idea-unify-pending-write-kinds.md](future/idea-unify-pending-write-kinds.md). - Currently deferred; worth revisiting if a second snapshot-style producer appears or - the reconciler's fabricated `UserInfo` causes a bug. - -- [ ] Improve queue and worker observability. - Better metrics, queue visibility, and more high-load test coverage would help. - -- [ ] Make the `BranchWorker` queue durable and move watch cursor advancement behind a durable - worker acknowledgment. - The current Redis watch cursor only remembers the last resourceVersion handed to the in-memory - worker queue. That detects queue-full drops, but a controller crash after cursor advancement and - before the queued write lands in Git can still skip work on restart. The intended direction is a - durable worker queue/journal so replay, live events, and resyncs are acknowledged only after the - write is recoverable. This is also the realistic boundary for Kubernetes watch history: the API - server does not guarantee every old revision remains available long enough for us to rebuild from - resourceVersion alone. - -- [ ] Fix recurring full e2e flakiness around WatchRule/snapshot convergence. - This has shown up more than once as timeout-based failures in manager SOPS bootstrap and - signing snapshot-message specs, then passed on rerun. Capture and mitigation notes live in the - [e2e serial registry](spec/e2e-serial-registry.md). - This should be addressed before the next feature that expands commit-message, snapshot, or - write-window behavior, otherwise new failures will be hard to separate from the existing timing flakes. - -- [ ] Use [bestpractices.dev](https://www.bestpractices.dev/en) as a project maturity checklist. - Review the current gaps, decide which items matter for this project, and turn the useful ones - into concrete follow-up work instead of treating the badge as the goal. - -- [ ] Filter more cluster-generated noise. - Examples include Kubernetes-generated ConfigMaps such as `kube-root-ca.crt` and similar - cluster-specific resources that do not belong in a portable Git view by default. - -- [ ] Decide how SOPS rules should cover sensitive custom resources that are not Secret-shaped. - The current bootstrapped `.sops.yaml` encrypts `data` and `stringData`, which fits Kubernetes - Secrets and CozyStack `tenantsecrets`; resources with sensitive fields under shapes such as - `spec.credentials` need an explicit field policy or full-file encryption decision. - -- [ ] Decide the four layout questions [`layout/model.md`](layout/model.md#open-questions) left open. - The model itself is **built** — the path template stayed and gained `spec.placement.useKustomize` - and `spec.serializeNamespace`, shipped in - [#326](https://github.com/ConfigButler/gitops-reverser/pull/326) and - [#328](https://github.com/ConfigButler/gitops-reverser/pull/328) with the corpus that executes it. - None of the four blocks anything: nested roots per directory (re-open trigger written down), - refusing rather than reporting when a governing root disappears, a CRD default for - `placement.default` (legibility only, since #319 removed the correctness objection), and a - namespace-local `GitProvider` so three targets in three namespaces need not copy one credential. - That last one is answered: a `GitBranch` may name a `GitProvider` in another namespace, whose - `spec.allowedGitBranches` admit it. - -- [ ] Reduce duplication between `WatchRule` and `ClusterWatchRule` code paths where it makes sense. - -- [ ] Revisit `StreamsStatus.PendingSample`, whose five-entry cap stops being - representative now that `sourceNamespace: "*"` produces one cluster-wide collection instead of N named - ones. This is all that survives of a "collapse the wildcard fan-out" item: the redefinition that - shipped in [#330](https://github.com/ConfigButler/gitops-reverser/pull/330) deleted the fan-out - rather than optimizing it, so the optimization it described is no longer a thing to build. - [`source-scope-simplification.md`](design/source-scope-simplification.md) is the definition of - record. - -- [ ] Subscribe the watch plane to `typeset.Registry` lifecycle events. - `Registry.Subscribe` has **no production observer**: the events are computed on every `Update` - and dispatched to nobody, because the Materializer that consumed them is gone. It is the producer - of the only signal that can separate a type genuinely withdrawn (a settled `TypeRemoved`, past - `RemovalGrace`) from a discovery wobble, and mistaking the second for the first deletes a user's - manifests. Its intended consumer is the `stop` classification in - [target-watch-plan.md](design/target-watch-plan.md), "What a collection leaving means": a - settled removal drops its collection from the plan and the Git-side sweep converges the mirror under the - target's existing `spec.prune.mode`. Nothing has to be decided first — only removal on *intent* - waits on an open question. Tracked here so an unconsumed producer with a good comment on it does - not quietly rot into dead code. - -- [ ] Decide whether the watch-plane settle window should be configurable. - `settleWindow` (2s) and `maxSettleWait` (10s) in [owner.go](../internal/watch/owner.go) are fixed, - on the same reasoning `DefaultCommitWindow` gives one layer down: what a user cares about is how - quickly their config takes effect, not how the controller batches its internal work. Revisit if a - real deployment reports either that config changes feel slow or that a busy config plane replans - too often — and note the observable consequence already shipped, that toggling a rule off and on - *inside* the window is no longer a replay. - -- [ ] Settle whether a retention report can be dropped on a revision mismatch. - `MarkTargetRetention` discards any report whose revision does not match the scope's currently - installed one, and `retainTargetRetentionScopes` deliberately KEEPS the previous count when a - stream is restarted rather than zeroing a scope nobody re-measured — both in - [retention_rollup.go](../internal/watch/retention_rollup.go). Together those produce a stale - count with no notification involved: sweep succeeds, `status.retention.retainedDocuments` stays - at its old value, and on a converged GitTarget the next correction is ~5 minutes away. - - That is exactly the signature of an intermittent `E2E (full-manager)` failure in the - `prune_mode` spec "converges an existing orphan when `prune.mode` is widened" - (`retainedDocuments` frozen at 1 for 30s while the files it counts had already been swept). - A shared-event-channel fix landed for that failure and is the more likely cause, but this path - is **independent of it** and would produce the same symptom, so a green run does not retire it. - - Full evidence, the controller-log timeline, and the diagnostics now armed for it are in +The open work, in one list. Each item is one or two lines with a code pointer or the design page +that argues it. Ranked within each group, most urgent first. **Before the next release** marks what +should land before the next tag is cut (the last release is v0.53.0). Checked against `main` at +`1c4fade4` on 2026-10-24. + +## Before the next release + +- [ ] **R1: an applier that comes due during a flush records nothing between the mark and its + joining.** Its `flushEdits` starts when it joins, not at the mark, so an edit decided in between + is neither published before the apply nor recorded; if the apply overwrites it, Git keeps the edit + and the cluster runs the old value with no `Repair` pending. Start `flushEdits` for every applier + when the flush begins, or take a new mark when the due set grows. `flushed()` in + `internal/git/applier_claim.go`. +- [ ] **R4: a replaced branch worker reads the old worker's settled-snapshot record.** + `forgetSettledSnapshots` runs only on `GitTarget` teardown (`internal/watch/materialization.go`), + so after "Branch worker replaced" (`gittarget_controller.go`, the `RequestRecheckForGitTarget` + path) the new worker's startup converge passes the barrier at once and can apply before the folder + is rebuilt. Argo CD can then be asked for a path that does not exist. +- [ ] **Override #428's changelog entry.** Its `BREAKING CHANGE` footer describes + `GitProvider.spec.accessFrom` and "every GitTarget writing one repository branch must name the same + GitProvider", both replaced before release by `GitBranch` and grants (#429, #441). Add a + `BEGIN_COMMIT_OVERRIDE` block to #428's PR body, as #419 did, before the release PR is cut. +- [ ] **Remove the "loud rejection" fields.** `RemovedReceiver` (`gitprovider_types.go`), + `RemovedClusterProviderRef` (`gittarget_types.go`, `fluxclaim_types.go`, `argocdclaim_types.go`), + `RemovedGitProviderRef`, `RemovedBranch` and `RemovedParentBranch` (`gittarget_types.go`) exist + only to refuse an old spelling at admission. With no users yet, an API break is a hard cut; drop + them and their CEL rules, and the matching sentences in [UPGRADING.md](UPGRADING.md). +- [ ] **F-i: CRD text beyond the contract.** `RepairPending` says "an API edit made after its + flush", and `kubectl explain` never defines "flush" (`fluxclaim_types.go`, `argocdclaim_types.go`). + Check `applierClaimRef`'s description for rationale too (AGENTS.md, API changes). + +## Applier claims and bi-directional editing + +- [ ] **Decide Fix 2 for live edits dropped while render fidelity is pending**: hold the events + (2a, the user stays the author) or re-derive them with a resync (2b, the operator becomes the + author), or keep the drop and alert on `git_writes_dropped_total`. It is also the cause behind the + flaky audit-route override e2e spec. + [live-edit-dropped-while-render-fidelity-pending.md](design/live-edit-dropped-while-render-fidelity-pending.md). +- [ ] **Reopen the render-fidelity gate after a Git repair.** A target stuck on + `RenderMatchesLive=False` clears only on a re-declaration or a restart. `Invalidate` and + `RequestRecheckForGitTarget` exist; their only caller is branch-worker replacement + (`gittarget_controller.go`). Specified in + [render-fidelity.md](design/support-boundary/render-fidelity.md) §6b; no decision needed. +- [ ] **Watch the held applier**, so a switch-on or a `flux resume` is undone at once instead of + within the one-minute `heldClaimSteadyInterval`. Also makes ask #31 below exact. + [applier-claim-held-applier-watch.md](design/applier-claim-held-applier-watch.md). +- [ ] **F-a: a held barrier is invisible.** A `GitTarget` with no planned collection holds its + applier, and with it every reason of every `GitTarget` on that claim, `ForeignCommit` included, + with no log, condition or metric. The hold is approved; the silence is the defect. + (`internal/git/applier_claim.go`, `internal/watch/apply_state.go`) +- [ ] **F-e: the apply window compares two clocks.** `sentAt` is the operator's clock, + `LastFieldManagedAt` the source API server's, at second granularity; a skew of 1s flips the echo + decision. Add a skewed-clock test, and probably a one-second grace. (`internal/git/apply_window.go`) +- [ ] **F-b, F-c, F-d: edge cases of the snapshot hold.** Live events dropped at the gate while a + discarded snapshot is held (F-b); an adopt lost while render fidelity is closed, because + `mirrors()` returns before `noteCreated` (F-c); a `GitTarget` that names a claim after its + snapshots settled empty never gets its folder (F-d, `resync_flush.go`). +- [ ] **F-h: small leaks and races.** The 5-minute hold can restart; the `Initial` flag races + settlement; `snapshotHeldSince`, `applierState.named` and dropped collections in + `settledSnapshots` leak. +- [ ] **F2: accept a Flux fetch whose token was stamped over** when the artifact holds the commit. + Today `Flux.fetch` waits only for its own token, so a commit another stamp fetched fails after 30s + (`internal/applier/flux.go`). Also check F3: the hold is checked after the fetch, so a + `Kustomization` resumed by hand applies the fetch at once. +- [ ] **A2: judge a gone Argo CD token by `status.history`** (our commit, + `initiatedBy: gitops-reverser`, `deployStartedAt` at or after our request means `Succeeded`). + Today a manual sync that wipes our token after a success reads as failed and syncs again, so hooks + run twice (`internal/applier/argocd.go`). +- [ ] **Check Argo CD sync windows in `Configured`.** The guide says a sync window must not block + GitOps Reverser's syncs; nothing checks it. +- [ ] **Refuse or flag overrides that are not in Git** (Argo `spec.source.kustomize`; Flux + `spec.images`, `patches`, `targetNamespace`): they change what is applied where the fidelity check + cannot see. `applicationMismatch` checks only repo and revision. Needs a decision; + [render-fidelity.md](design/support-boundary/render-fidelity.md) §8. +- [ ] **One holder per applier across `KubeProvider`s** (cluster identity or an object-local stamp). + [idea-applier-claim-ownership.md](future/idea-applier-claim-ownership.md). +- [ ] **A targeted review of `internal/applier/`.** CodeRabbit skipped #420 (over its 100-file + limit), so the largest new package has had no bot review. +- [ ] Later: claim an Argo CD `ApplicationSet` + ([idea-argocd-applicationset-claim.md](future/idea-argocd-applicationset-claim.md)); a webhook + applier, a third claim kind ([apply-trigger.md](design/apply-trigger.md), "Where this goes next"). + +## The gitops-api team's asks + +Their argument for each is in [gitops-api-open-asks.md](future/gitops-api-open-asks.md). + +- [ ] **#32**: a troubleshooting row in [bi-directional-flux.md](bi-directional-flux.md) for a + controller that replaces the `Kustomization`'s whole spec with an Update. Docs only. +- [ ] **#31**: a metric for an apply the claim did not start; today it is only the + `ClaimedResourceReconciled` Event, which the API server aggregates and expires. +- [ ] **#34**: an e2e with an in-cluster applier and a remote source cluster, provoking one apply + window collision. +- [ ] **#23**: verify with their reproduction that #431 fixed deletion-as-intent naming the + finalizing controller instead of the deleter. +- [ ] **#11**: one YAML encoder for `[CREATE]` and `[UPDATE]` bodies (indent churn). +- [ ] **#28**: `--watch-namespaces`, then an owner selector, so two instances can share one + config-plane cluster. It is the same mechanism HA needs. +- [ ] **#10**: consider the namespace when placement follows siblings. Cosmetic. + +## Attribution + +- [ ] **Option F: stop waiting for a fact a (route, type) has never produced.** A removal on a type + the audit policy drops spends the full grace for nothing. + [attribution-removal-wait-options.md](design/attribution-removal-wait-options.md). +- [ ] **Release attribution per object, not per watch (head-of-line).** One slow lookup delays + every later event on its stream. Before relying on per-object order, check three places: the + watch cursor may advance only to the oldest unreleased event; the `producerGate` handoff on + retire and replace, and the snapshot-before-live boundary; and the apply window, which classifies + an edit by when it reaches the worker. + [The ordering spec](spec/watch-event-ordering-and-attribution-grace.md#preserve-ordered-release-if-attribution-becomes-concurrent). +- [ ] Two doc sentences: no condition detects an API server's 30s default + `--audit-webhook-batch-max-wait` (say so in the `KubeProvider` docs), and a relist after a watch + gap ships its changes as the configured author (say so in the operator docs). +- [ ] Optional: capture `CommitRequest` authors without Redis in single-pod mode, with an in-memory + `CommandAuthorStore`. The only thing that still forces Redis on one replica. Low priority, and + only useful until HA. + +## Branch worker and CommitRequests + +- [ ] `expireWaitingCommitRequests` ranges over a map, so empty commits for saves that expire in + the same pass land in random order; sort by `seq` (`internal/git/commit_request_attach_loop.go`). +- [ ] `buildRequestRecordWrite` keeps only the username (`UserInfo{Username: pcr.author}`); carry the + full `UserInfo` so a record commit has the same author identity as a window commit. +- [ ] A record commit skips `requestTemplate`, though its doc comment says it is phrased like every + other commit; fix the code or the comment. +- [ ] Nits: a doubled doc comment on `commitWindowFor`; "close-delay" in the test names in + `internal/controller/commitrequest_kstatus_test.go`. +- [ ] **Parent observation, step 1**: keep parent availability and its timestamp after the write + branch exists, from existing advertisements, with no extra connections. + [gittarget-parent-observation.md](design/gittarget-parent-observation.md#implementation-and-validation). +- [ ] **Finish making a `GitTarget` turn red quickly and say why no commit was made**: the broader + reason and status contract table remains. + [gittarget-red-status-plan.md](design/gittarget-red-status-plan.md). +- [ ] **Transition boundary** in the branch worker, the prerequisite for the durable queue. + [branch-worker-event-model.md](design/branch-worker-event-model.md). +- [ ] **Make the worker queue durable**, and advance the watch cursor only after a durable worker + acknowledgment. Today a crash after cursor advancement and before the write lands in Git can skip + work on restart. +- [ ] **High availability**: leader or ownership coordination so two replicas never write the same + `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). +- [ ] Re-assess unifying the pending-write kinds + ([idea-unify-pending-write-kinds.md](future/idea-unify-pending-write-kinds.md), now four kinds, + not two). Deferred. +- [ ] Improve queue and worker observability, with more high-load tests. + +## Watch plane + +- [ ] **Subscribe the watch plane to `typeset.Registry` lifecycle events.** `Registry.Subscribe` + has no production observer, so nothing sweeps the manifests of an uninstalled CRD, and since #419 + the operator asks Flux or Argo CD to apply that folder. Its consumer is the `stop` classification + in [target-watch-plan.md](design/target-watch-plan.md), "What a collection leaving means". +- [ ] **Drop the "TEMPORARY at Info" logs to V(1)** (`gittarget_controller.go`, + `watch/render_fidelity_gate.go`, `watch/event_router.go`), now that Failure A of [watch-plane-status-convergence-failures.md](design/watch-plane-status-convergence-failures.md) - §3. - - **The question to answer:** widening `prune.mode` sets `force`, which classifies every collection as - `restart` and issues fresh revisions, while the 30s periodic sweep also re-runs - `retainTargetRetentionScopes` for every declared target. Can a pass landing *while a replay is - in flight* install a revision that differs from the one the in-flight replay captured at start? - If yes, that replay's report is dropped on arrival and the count stays stale until something - else re-measures it. CI is slower than a dev machine, which would widen that window and explain - why it has never reproduced locally. - -- [ ] Re-enable the `goconst` linter with a path-scoped exclusion instead of the current repo-wide - disable in [.golangci.yml](../.golangci.yml). Exempting `test/` and `internal/git/commit.go` - would silence the existing noise (~45 findings, mostly test fixtures) while still catching - genuine new string repetition. - -- [ ] Preserve more user-facing file structure where feasible. - Comments, ordering, and other low-noise formatting details are still easy to lose when rewriting - manifests. - -- [ ] Handle resources whose GVK cannot be resolved against the live cluster. - A manifest may reference a `apiVersion`/`kind` whose CRD is not installed, so the RESTMapper - cannot map it to a GVR. This is already a problem today and also blocks the manifest-inventory - work in [`spec/manifest-system.md`](spec/manifest-system.md): indexing must record the manifest - identity and defer rather than fail the whole scan. - -## Future directions worth revisiting - + is fixed. +- [ ] Pin with a test that an unforced retention pass keeps a scope's revision, so only + `Invalidate` re-issues one (`internal/git/render_fidelity_gate.go`), and close the retention + revision-mismatch question in + [watch-plane-status-convergence-failures.md](design/watch-plane-status-convergence-failures.md) §3. +- [ ] Filter more cluster-generated noise, such as `kube-root-ca.crt`; decide which objects and + whether a user can opt back in (`resource_policy.go` is a type-level denylist only). +- [ ] A manifest whose CRD is missing refuses the whole folder (`IssueUnresolvedKRM`, + `manifestanalyzer/acceptance.go`): keep that, or defer only that document. The scan already + records `unresolved-mapping` instead of failing. +- [ ] Decide whether the settle window should be configurable (`settleWindow` 2s, `maxSettleWait` + 10s in `internal/watch/owner.go`). Revisit only on a report that config changes feel slow. +- [ ] Reduce duplication between the `WatchRule` and `ClusterWatchRule` code paths where it makes + sense. +- [ ] Later: a namespace selector in a `KubeProvider` grant's scope + ([idea-source-namespace-selector.md](future/idea-source-namespace-selector.md)). + +## Support boundary and Git content + +- [ ] **Scalar strategic-merge patch authoring**: an env var or a limit edited in one overlay is + still refused, and since #419 the user sees the edit reverted. Decide "we author patches", then + about two weeks. [patch-authoring.md](design/support-boundary/patch-authoring.md). +- [ ] Decide how SOPS rules cover sensitive custom resources that are not Secret-shaped (fields such + as `spec.credentials`): an explicit field policy, or full-file encryption. +- [ ] Should comments survive a SOPS re-encrypt? (e2e Test C in + [e2e-coverage-gaps-and-improvements-plan.md](design/e2e-coverage-gaps-and-improvements-plan.md)). +- [ ] Decide the three open layout questions in [layout/model.md](layout/model.md#open-questions): + nested roots per directory, refusing rather than reporting when a governing root disappears, and + a CRD default for `placement.default`. (The fourth, a shared `GitProvider`, is answered by + `GitBranch` and grants.) +- [ ] Preserve more user-facing file structure: comments, ordering and other low-noise formatting. +- [ ] **Values Move 2**, the editable values projection + ([values-file-projection.md](design/support-boundary/values-file-projection.md)). +- [ ] **Least privilege**: never mirror Secrets, permission-aware followability, an RBAC generator + ([least-privilege-remaining-work.md](future/least-privilege-remaining-work.md)). +- [ ] Support-boundary gaps: the `Generated{path}` claim, Argo `directory.exclude`, paths written by + ImageUpdateAutomation, the provenance and ownerReference gate, a Helm chart skipped as a unit, the + Overlay-Diverged trailer. + +## E2E and CI + +- [ ] **Fix the recurring full-e2e flakes** around WatchRule and snapshot convergence (SOPS + bootstrap, signing snapshot messages). Notes in the [e2e serial registry](spec/e2e-serial-registry.md). + Address before the next feature that widens commit-message, snapshot or write-window behavior. +- [ ] **Run image-refresh only when its inputs change**, a path filter in `ci.yml`; it removes a + 14–18 minute leg from most PRs. [e2e-suite-trim.md](design/e2e-suite-trim.md). +- [ ] **F-f: E6 can leak its editing goroutine**: `close(stop)` runs only after the `Eventually` + succeeds. Use `DeferCleanup` (`test/e2e/bi_directional_echoes_e2e_test.go`). +- [ ] The e2e detail gaps and redundancy tables in [e2e-suite-trim.md](design/e2e-suite-trim.md): + four specs that look tested and are not, then the moves and merges. +- [ ] `SetDefaultEventuallyTimeout` is global and set in 17 e2e files, so the last `Describe` body + built wins; use per-spec timeouts (`Eventually(...).WithTimeout`). +- [ ] e2e Test A: a delete missed while the controller is scaled to 0; then Tests B and C. + [e2e-coverage-gaps-and-improvements-plan.md](design/e2e-coverage-gaps-and-improvements-plan.md). +- [ ] Re-enable `goconst` with a path-scoped exclusion (`test/`, `internal/git/commit.go`) instead + of the repo-wide disable in [.golangci.yml](../.golangci.yml). +- [ ] Release-image reuse PRs 2, 3 and 5 ([release-image-reuse-plan.md](design/release-image-reuse-plan.md)). +- [ ] Metrics Phase 5: dashboard JSON, recording and alert rules + ([metrics-observability-plan.md](design/metrics-observability-plan.md) §7). +- [ ] Use [bestpractices.dev](https://www.bestpractices.dev/en) as a maturity checklist, and turn the + useful items into concrete work. + +## Docs and vocabulary + +- [ ] **Stale design-doc headers**, one docs PR: [open-asks-priority.md](design/open-asks-priority.md) + (#339 closed, #23 fixed), [gittarget-api-wave.md](design/gittarget-api-wave.md) (riders shipped), + [git-write-preflight.md](design/git-write-preflight.md) (superseded by the red-status plan), + [commitrequest-save-wait-options.md](design/commitrequest-save-wait-options.md) (written against + `closeDelay`), [gittarget-red-status-plan.md](design/gittarget-red-status-plan.md) (steps 5 and 6 + built), [watch-and-catalog-architecture.md](design/watch-and-catalog-architecture.md) + (superseded), [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; + [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. +- [ ] **`(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. +- [ ] 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. + +## Future directions + +- [ ] A `Writable` condition on `GitProvider`. A push session that sends nothing proves the server + opens one for the credential, not that a write is accepted (branch protection and pre-receive + hooks apply at ref update), so a green probe is necessary and not sufficient. `False` only on an + explicit refusal; a failed probe never clears a proven denial. Gating `Ready` on it is a separate + release. (`GitBranch` now opens a push session before it holds a branch, which covers part of + this.) - [ ] Simpler setup flows, including more Git provider bootstrap automation. - - [ ] Constrained reverse actions for simple, known Kustomize-style mutations. - -- [ ] A `Writable` condition on `GitProvider`. A read probe rules out a bad URL, a dead credential - and a missing repository; it does not prove we can push, so a read-only token looks healthy right - up to the first write. A push session that sends nothing proves the server opens one for this - credential, and behaves as expected against GitHub; it does not prove a write would be accepted, - since branch protection and pre-receive hooks are applied when refs are updated, so a green probe - is necessary and not sufficient while a red one is definitive. Two rules it needs: `False` only - on an explicit refusal (a probe that could not run leaves `Unknown`), and a failed probe never - clears a proven denial, or a revoked token goes - green on the next network flap. Gating `Ready` on it is a second, separate release with an - upgrade note, because a provider that reads but cannot write goes `True` to `False` the day it - lands. - [ ] Better branching and promotion strategies. -- [ ] A webhook applier, so an applier claim can hold a tool other than Flux or Argo CD. Flux and - Argo CD are built; see the [applier claim design](design/apply-trigger.md). - -Research work: +Research: -- Replace metrics mechanism with - (so that it's also helm and - so that we can have proper deps) -- Read more on how resource versions work (and can work in the HA rebruild): +- Replace the metrics mechanism with the + [VictoriaMetrics operator](https://docs.victoriametrics.com/helm/victoria-metrics-operator/), so it + is also Helm and has proper dependencies. +- Read up on how resource versions work and can work in the HA rebuild: + . diff --git a/docs/design/applier-claim-held-applier-watch.md b/docs/design/applier-claim-held-applier-watch.md new file mode 100644 index 00000000..525233fc --- /dev/null +++ b/docs/design/applier-claim-held-applier-watch.md @@ -0,0 +1,122 @@ +# Watching the held applier, and Argo CD `selfHeal` under a held switch + +Status: the `selfHeal` answer is decided (leave it alone) and shipped with #437, where an +`ArgoCDClaim` holds `spec.syncPolicy.automated.enabled: false`. The follow-up at the end, watching +the held applier, is **wanted and not built**. Argo CD facts are read from Argo CD's source at 3.5.0 +(`external-sources/argo-cd`, `controller/appcontroller.go`). + +## The question + +Should the claim also switch `selfHeal` off, so that nothing can revert an incoming edit, for +example during a sync? + +## Answer: no + +### `selfHeal` is part of automated sync + +Self-heal is not a watcher of its own. It is a step inside `autoSync`, and `autoSync` returns before +that step when automated sync is off: + +```go +// controller/appcontroller.go:2587 +if app.Spec.SyncPolicy == nil || !app.Spec.SyncPolicy.IsAutomatedSyncEnabled() { + return nil, 0 +} +// ... the selfHeal check comes later, at :2668 +``` + +So while the claim holds `enabled: false`, `selfHeal: true` has no effect, whether or not an +operation is running. + +### During a sync it changes nothing + +During a sync GitOps Reverser started, an incoming edit can be undone by two things: + +- the sync itself applying the pinned commit over it; +- Argo CD's retries of that same pinned commit. + +Neither is self-heal. The apply window and the `Repair` reason already cover them. + +### It would only matter in the switch-on window, at a price + +If somebody switches automated sync on, the claim notices at its next periodic read (at most +`heldClaimSteadyInterval`, one minute) and switches it off again. During that window: + +| | `selfHeal` left as is | `selfHeal` also held off | +| --- | --- | --- | +| A live edit is reverted within seconds | yes, if `selfHeal: true` | no | +| A new commit, GitOps Reverser's own pushes included, is synced over pending edits | yes | yes | + +So holding `selfHeal` off would reduce the harm in the window, but not remove it. + +The price is the hand-back. Measured in envtest (`argocdclaim_apiserver_test.go`): + +- a forced apply of a **different** value takes the field from every other owner; +- giving the field up then removes it. + +A user's `selfHeal: true` would therefore come back unset, which is `false`. Restoring it would mean +storing the original value somewhere: the extra state the `enabled` switch avoids. The switch has +no such problem, because a user who had automated sync on never owned `enabled`; Argo CD reads a +missing `enabled` as on. + +## Follow-up (wanted): watch the held applier, and respond at once + +The window, not `selfHeal`, is the exposure. Responding at once from our side closes it for +automated sync and `selfHeal` alike, and for a `FluxClaim`'s `flux resume` too. It takes "the +claim takes over the applier" from a one-minute poll to a real control loop. + +**Use cases behind it.** + +- **Testing.** Today a test that switches automated sync on (or runs `flux resume`) has to request + a reconcile, or wait up to a minute, before anything happens. With the watch the response is part + of the behavior under test: an e2e spec can switch it on and assert it is off again within + seconds, and the unit tests can drive the same path through the fake's watch. +- **Understanding user edits that come in during a sync.** What happens to an API edit that arrives + while an apply runs is the hardest part of bi-directional editing: the apply window, the applier's + echoes, `Repair`. We now learn when an applier's state changed only when we next read it. A watch + gives exact moments: when a sync started, ended, or was started by somebody else, and when the + switch flipped. Lined up against the times of incoming edits, that shows which edits landed during + a sync and what became of them, in logs and in the test assertions. +- **Alerting.** The gitops-api team asked for a metric of applies the claim did not start (their + ask #31, in [the gitops-api asks](../future/gitops-api-open-asks.md)); they saw the detection time + vary from 2s to about 75s, which is the poll. A watch makes that metric exact. + +**Now.** The applier claim reconciler reads the applier on a timer (`heldClaimSteadyInterval`, one +minute) and watches nothing in the applier's cluster. That is deliberate +(`applierclaim_controller.go`): Flux or Argo CD may not be installed there, and an informer on a +kind that is not served would be a failure to report, not a check that answers "not found". + +**Proposal.** Watch an applier only while a claim holds it, so the reason for not watching does +not apply: + +- **When:** start the watch when the claim puts its finalizer on (it holds the applier, so the + kind is served and readable), and stop it when the finalizer comes off (hand-back done). +- **What:** one watch per held object, with a field selector on `metadata.name` in its namespace, + through the `KubeProvider`'s dynamic client. A watch per held object, not a cluster-wide informer, + keeps the RBAC at `get`/`list`/`watch` on what the claim already reads, and the cost + proportional to what is held. +- **On an event:** enqueue the claim (a `source.Channel`, or `GenericEvent`s into the applier claim + controller). The reconcile does what it does now: it reads, and switches off or suspends again + with the same Event. The watch only makes the read immediate. Nothing is decided from the event + itself. +- **Filter:** only enqueue when the held field changed (`spec.suspend`, or + `spec.syncPolicy.automated.enabled`). An Argo CD `Application` writes its status on every + refresh, and has no status subresource, so an unfiltered watch would reconcile the claim every + few seconds. +- **Record, don't reconcile, the sync transitions:** a change of the applier's operation state (an + `Application`'s `status.operationState.phase` or a new `operation`; a `Kustomization`'s handled + `requestedAt` and `Ready`) is logged with its time, and is available for the dispatcher to follow, + so the moments a sync started and ended sit next to the incoming edits (the second use case). + These do not enqueue the claim. +- **Keep the timer:** the one-minute read stays as the fallback for a watch that dropped, expired + or could not start (a `KubeProvider` whose identity lacks `watch`). A watch that cannot start is + logged and leaves the claim on the timer; it never makes the claim not Ready. +- **Both kinds:** a Flux `Kustomization`'s `spec.suspend` gets the same treatment, so `flux resume` + is suspended again at once, before kustomize-controller has finished its apply in most cases. + +**Cost.** A watch per held applier, its lifecycle tied to the finalizer, `watch` added to the +applier grants (chart and `config/rbac`), and a test that a switch-on is undone well within the +timer. The e2e spec E (`argocd_bi_directional_e2e_test.go`) can then drop its explicit +`requestClaimReconcile`, and prove the response time instead. + +Recommendation: build the watch as its own PR, for both kinds. diff --git a/docs/design/attribution-removal-wait-options.md b/docs/design/attribution-removal-wait-options.md index 855d4e1b..c9ad2b8c 100644 --- a/docs/design/attribution-removal-wait-options.md +++ b/docs/design/attribution-removal-wait-options.md @@ -14,6 +14,13 @@ > deleter or in `absent`, never in the last writer. The cost this page is about is unchanged: > situation 5 still spends the whole grace. See > [what attribution gives up, on purpose](../spec/attribution.md#what-attribution-gives-up-on-purpose). +> +> **Direction, 2026-10-22.** The attribution pipeline review behind #431 recommends +> [option F](#f-a-per-route-type-circuit-breaker-stop-waiting-for-a-fact-that-has-never-once-arrived) +> in its own PR, with no new flag, now that #431 has made removal attribution independent of the +> watch's label selector. C stays the follow-up for the intermittent case. Measure how often a +> watched type is one the audit policy excludes before building C +> ([what is not yet measured](#what-is-not-yet-measured)). ## The question diff --git a/docs/design/e2e-suite-trim.md b/docs/design/e2e-suite-trim.md new file mode 100644 index 00000000..a4bcc360 --- /dev/null +++ b/docs/design/e2e-suite-trim.md @@ -0,0 +1,73 @@ +# E2E suite: what is left to trim + +Status: **open plan**, condensed on 2026-10-24 from the 2026-10-20 timing review (three green CI +runs: a4c2e633 on main, e880cdc6 for #422, 2a6c1ab2 for #420). The timing half is done: +`SpecPriority` on the long containers, `--wait=false` namespace teardown, the remote-refresh spec +reduced to one foreign-push spec after #424 fixed the `status.remote` requeue, and the `Serial` CRD lifecycle +spec moved off the manager leg. What follows is the part not built. + +**Do not shorten `RequeueSteadyInterval` for e2e.** A test-only 60s tick would rescue a broken event +path through the periodic reconcile, and the spec would pass, just slower. Today the 5-minute tick +is longer than nearly every bound, so an event-wiring regression fails loudly. + +## Run image-refresh only when its inputs change + +The leg tests the developer Taskfile's stamp chain (rebuild, k3d import, rollout), not the product, +and 266s of its 369s suite is the BeforeSuite rebuild. Gate it on a path filter in +[`ci.yml`](../../.github/workflows/ci.yml): `Taskfile*.yml`, `test/e2e/Taskfile-e2e.yml`, +`Dockerfile*`, `hack/**`, `.dockerignore`, `go.mod`. Run it on every push to main as a backstop. S3 +("second Go change also restarts") and S6 ("pod uses the imported image", already asserted in S2) +can go; each costs a full rebuild. This removes a 14–18 minute leg from most PRs. + +## Detail gaps: things that look tested and are not + +These matter more than the duplicates below. + +1. **[`commit_request_e2e_test.go`](../../test/e2e/commit_request_e2e_test.go)**: a comment says the + generateName spec "is skipped in configured-author mode", but it has no `Skip`. Add it, or fix + the comment. +2. **[`signing_e2e_test.go`](../../test/e2e/signing_e2e_test.go)**: its own comment says the + `GitTarget` recreate "writes nothing", and the asserted commit is the one from before the + recreate. The step costs a reconcile and proves nothing; delete it, or make it the thing under + test. +3. **[`watchrule_configmap_secret_e2e_test.go`](../../test/e2e/watchrule_configmap_secret_e2e_test.go)**: + the backfill spec says it guards the "GVR already covered by other rules" early return. The + earlier specs delete their rules first, so that condition is probably not met (reader-reported, + unverified). As written it is a plain backfill. +4. **[`tilt_playground_e2e_test.go`](../../test/e2e/tilt_playground_e2e_test.go)**: `Serial`, and + the default label filter does not exclude `playground`, so it holds a CI serial phase for a + fixture check. Gate it like `demo`. + +## Redundancy: what to move, merge or drop + +Hygiene, not wall clock: the duplicates are mostly 5–30s specs, and per-container setup is cheap +(`SetupRepo` median 0.7s). + +### Move out of e2e: no cluster, Gitea or controller needed + +| Spec | Why it does not need e2e | Already covered at | +| --- | --- | --- | +| `watchrule_generation_barrier`: the 8-entry `DescribeTable` "accepts only a current generation that is streaming" | a pure Go predicate over an `unstructured` value | nothing yet; move it to a plain `_test.go` | +| `prune_mode`: "defaults, omits, and validates spec.prune.mode at the API server" | CRD schema defaulting and an enum | no envtest yet; `internal/controller` envtest loads the CRDs (`applierclaim_schema_test.go` is the pattern) | +| `gittarget_overlap`: sibling paths Validated, nested path `TargetConflict` | controller logic | `internal/controller/gittarget_controller_test.go` | +| `source_cluster`: the "fails KubeProvider Validated (no dial)" specs and the `KubeProviderNotFound` spec | no dial, no kcp; they run serially on a procs=1 leg | `internal/kubeconfig/kubeconfig_test.go` and the `KubeProvider` controller tests | +| `gitprovider_validation`: "BranchNotAllowed" | controller logic, now a `GitBranch` grant check | `internal/controller/gitbranch_controller_test.go` (verify before deleting) | + +### Merge, keeping the unique detail + +| Cluster | Keep | Fold in (and the detail it brings) | +| --- | --- | --- | +| Basic create/update/delete | `aggregated_apiserver` "separate commits for Flunder create, update, delete" (three commits, three hashes) | `crd_lifecycle`'s three CR specs into ONE CR lifecycle spec, keeping the field-filtering checks; the `watchrule_configmap_secret` delete spec into its update spec | +| Author attribution | `audit_route_attribution`, `deletecollection_intent` | `commit_author_attribution`'s spec is another with other data; drop it. `deployment_scale_subresource` folds into `deployment_scale_author_attribution` (author check conditional on configured-author mode) | +| Collection delete | `deletecollection_intent`'s selector-scope and finalizer specs | the two specs that are subsets of those. Keep `aggregated_deletecollection` (no response body is a real difference) | +| Signing | generated key, late joiner | BYOK shares every step but the key source, so make it a table entry; the committer/message spec needs no signing provider | +| Smoke checks | none | `watchrule_configmap_secret`'s "provider Ready" and "rule Ready", `aggregated_apiserver`'s, `gitprovider_validation`'s "HTTP provider Ready": every other spec proves these | +| New write branch | parent omitted | the `parentBranch: release` run repeats the same 3-minute path under another name; keep one, or make the second a short variant | +| Source cluster | three workspaces | one workspace is strictly subsumed | + +Keep, despite looking similar: the Flux and Argo CD bi-directional specs (different contracts), and +`foreign_content` against `unsupported_folder` (first materialization against after a healthy +start), and the two values-file Describes (different producers). + +Past this, the remaining lever is the fixed ~6 minutes each leg pays before its first spec (runner +disk cleanup, cluster bring-up, prepare): a CI-infrastructure project, not a test-content one. diff --git a/docs/design/live-edit-dropped-while-render-fidelity-pending.md b/docs/design/live-edit-dropped-while-render-fidelity-pending.md new file mode 100644 index 00000000..5c8fc507 --- /dev/null +++ b/docs/design/live-edit-dropped-while-render-fidelity-pending.md @@ -0,0 +1,342 @@ +# A live edit dropped while a GitTarget's watches start + +> **Status (2026-10-24): Fix 1 is built, Fix 2 needs a decision.** Fix 1 and the drop counter +> shipped with #420 (d7ce1bd4): the worker records a scope clean itself (`recordReplayClean` in +> `internal/git/render_fidelity_drops.go`), and `gitopsreverser_git_writes_dropped_total{kind, +> reason}` counts every drop, the first per gate status logged at Info. What is left is +> [Fix 2](#fix-2-stop-dropping-live-events-while-the-gate-is-pending): an edit that arrives while +> another collection of the same `GitTarget` is still replaying is still dropped. + +The e2e spec `Audit route attribution` › *attributes a commit reached through a +`rules[].sourceNamespace` override* has failed 9 times in CI since 2026-09-21, and passes on a +rerun. The test is fine. It catches a product race: GitOps Reverser can **drop an API edit for +good** when the edit arrives while one of the GitTarget's watches is starting. This document +explains the race, shows it in a CI log, and lays out the two proposed fixes. + +## The short version + +1. Each collection a GitTarget watches has a **render-fidelity scope**. A new scope starts out + pending, and the GitTarget stays closed to live writes until **every** scope has reported + clean. +2. A scope reports clean when the branch worker has applied that collection's first snapshot (the + replay resync). The worker answers the resync on its own goroutine, and a *separate* goroutine + receives the answer and records the scope clean. +3. In the gap between those two steps the gate is still closed. The worker takes the next item off + its queue, a live event, finds the gate closed, and **drops** the event. +4. Nothing brings it back. The watch has already moved past the event, no replay follows, and the + drop is logged only at debug level (`V(1)`). + +**Fix 1** removes the gap: the worker records the scope clean itself, before it answers. That is +small, changes no documented behavior, and closes the race this spec hits. + +**Fix 2** goes further: while the gate is pending, live events are held rather than dropped. That +also covers edits that arrive while some *other* collection of the same GitTarget is still +replaying, which Fix 1 cannot reach. It changes a drop that +[api-first-publication.md](../api-first-publication.md#what-can-stop-a-write-before-it-reaches-git) +documents as intended, so it needs your decision. + +## The pieces involved + +| Piece | Where | What it does here | +| --- | --- | --- | +| Target watch stream | [`internal/watch/target_watch.go`](../../internal/watch/target_watch.go) | One per collection (for example "ConfigMaps in namespace X"). Lists the current objects (the replay), hands that snapshot to the worker as a **replay resync**, then streams live events. Marks itself `Streaming` as soon as the resync is *enqueued*, without waiting for it to apply | +| Branch worker | [`internal/git/branch_worker.go`](../../internal/git/branch_worker.go) | One goroutine per repository and branch. Takes items off one FIFO queue in order: resyncs, live events, atomic writes | +| Render-fidelity gate | [`internal/git/render_fidelity_gate.go`](../../internal/git/render_fidelity_gate.go) | Holds one result per scope. `AllowsWrites` is true only when **all** of the GitTarget's scopes are clean | +| Resync drain | [`internal/watch/event_router.go`](../../internal/watch/event_router.go) `drainScopedResync` | A goroutine per replay resync. Waits for the worker's answer, then records the scope clean in the gate and publishes status | + +Two properties of these pieces explain everything below: + +- **The gate is per GitTarget, the scopes are per collection.** One pending scope closes live + writes for every collection of that GitTarget (`AllowsWrites` at + [`render_fidelity_gate.go:261`](../../internal/git/render_fidelity_gate.go#L261)). +- **The answer and the clean mark happen on different goroutines.** The worker answers in + `settleResyncApplied` ([`pending_writes_loop.go:348`](../../internal/git/pending_writes_loop.go#L348)); + the drain marks clean later ([`event_router.go:357`](../../internal/watch/event_router.go#L357)). + +## How a new collection is supposed to start + +```mermaid +sequenceDiagram + autonumber + participant API as Kubernetes API + participant S as Target watch stream + participant Q as Worker queue (FIFO) + participant W as Branch worker + participant D as Resync drain + participant G as Render-fidelity gate + + Note over G: scope "ConfigMaps in src" pending, so the GitTarget is closed + S->>API: watch with sendInitialEvents + API-->>S: current objects, then initial-events-end + S->>Q: replay resync (snapshot) + S->>D: start drain goroutine + Note over S: marks itself Streaming + W->>Q: take replay resync + W->>W: commit the snapshot + W-->>D: answer: applied + D->>G: record scope clean + Note over G: every scope clean, so the GitTarget is open + API-->>S: live event: ConfigMap created + S->>Q: live event + W->>Q: take live event + W->>G: AllowsWrites? + G-->>W: yes + W->>W: open commit window, commit with the user as author +``` + +In this ordering the live event reaches the worker after the drain has recorded the scope. Nothing +in the code guarantees that ordering. + +## The race + +Steps 9 and 10 above run on two goroutines. If the live event is already in the queue, the +worker can take it between the answer and the clean mark: + +```mermaid +sequenceDiagram + autonumber + participant Q as Worker queue (FIFO) + participant W as Branch worker + participant D as Resync drain + participant G as Render-fidelity gate + participant S as Target watch stream + + Note over Q: queue holds [replay resync, live event] + W->>Q: take replay resync + W->>W: commit the snapshot + W-->>D: answer: applied + W->>Q: take live event + W->>G: AllowsWrites? + G-->>W: no (scope still pending) + W->>W: drop the event, log at V(1) + D->>G: record scope clean (too late) + Note over S: cursor already past the event, so it is never replayed +``` + +The gap is small, often well under a millisecond, but the worker needs no time to reach the next +item: it is the same loop. The test makes the timing likely by creating its ConfigMap the moment +the new streams report `Streaming`, which they do as soon as their resyncs are queued. + +### What a failing run shows + +From CI run 36055043371 (job 107821494360, 2026-09-24), filtered to the GitTarget under test. +The new WatchRule starts **two** collections in the source namespace: ConfigMaps and Secrets. + +| Time | Event | +| --- | --- | +| 20:43:08 | Watch plan reconciled: 1 collection kept, 2 started (ConfigMaps and Secrets in `…-src`). The gate now has 3 scopes, 2 pending | +| 20:43:08 | ConfigMaps replay complete → replay resync queued | +| 20:43:08 | Secrets replay complete → replay resync queued | +| 20:43:09 | Worker applies the ConfigMaps resync; drain records it clean. Gate: **Unknown, "Waiting for 1 of 3"** | +| 20:43:09.087 | The test creates the ConfigMap as `oidc-override-user` | +| 20:43:09 | Worker handles and applies the Secrets resync | +| 20:43:09 | Drain records Secrets clean. Gate: **True** | +| after | No `Opening commit window` for the ConfigMap, ever. The spec times out 30 seconds later | + +This run shows the gate's per-GitTarget reach. The ConfigMap's own scope was already clean; the +event was dropped because the **Secrets** scope, whose resync sat ahead of it in the queue, had +been answered and not yet recorded. The drop log itself is at `V(1)`, which CI does not enable, so +the drop is inferred from the missing commit window. The agent that surveyed CI found the same +shape in all 9 failures: the gate opens in the same second the ConfigMap is created, and the +worker never logs the ConfigMap. + +## Why the edit is gone for good + +A dropped event is not retried anywhere: + +- The stream records its watch cursor right after it routes a live event + ([`target_watch.go:955`](../../internal/watch/target_watch.go#L955)), so a reconnect resumes + *after* the event. +- A replay happens only when a stream (re)starts: operator restart, an expired cursor, or a plan + change for that collection. The 30-second periodic plan pass keeps running streams as they are. +- The drop is logged at `V(1)` ([`branch_worker.go:1371`](../../internal/git/branch_worker.go#L1371)) + and counted nowhere. + +So the edit stays missing from Git until the object changes again or the collection replays. +Raising the spec's timeout would not help, because the event no longer exists anywhere to wait +for. + +## Two windows, not one + +Fix 1 and Fix 2 differ in which events they save. There are two ways a live event can meet a +pending gate: + +```mermaid +flowchart TD + E["Live event for collection C
reaches the worker while the gate is pending"] --> Q1{"Was every pending scope's
replay resync queued
ahead of this event?"} + Q1 -- yes --> A["Window A
The resyncs were applied and answered,
the drain has not recorded them yet.
Lasts microseconds to milliseconds."] + Q1 -- no --> B["Window B
Some scope is still replaying:
a new or restarted collection with many objects,
or operator start, when every scope is pending.
Lasts as long as that replay, often seconds."] + A --> F1["Fix 1 closes it"] + B --> F2["Only Fix 2 saves it"] +``` + +- **Window A** is the e2e failure. Both resyncs were in the queue before the ConfigMap existed, + because the test waited for `Streaming`. +- **Window B** reaches users the test does not cover. While a GitTarget adds a collection with a + thousand objects, an edit to any of its *other* collections is dropped until that replay + finishes. On operator start every scope is pending, so edits to a collection whose replay + finished early are dropped while the slower ones are still listing. Window B is the documented + behavior today: "Live events and atomic writes are dropped while the gate is closed." + +## Fix 1: record the scope clean on the worker's goroutine + +The worker already holds the gate (`w.renderFidelityGate`) and is the one that knows the resync +applied. If it records the scope clean itself, before it answers and before it takes the next +item, the gap disappears. + +```mermaid +sequenceDiagram + autonumber + participant Q as Worker queue (FIFO) + participant W as Branch worker + participant G as Render-fidelity gate + participant D as Resync drain + + Note over Q: queue holds [replay resync, live event] + W->>Q: take replay resync + W->>W: commit the snapshot + W->>G: record scope clean (stream's revision) + W-->>D: answer: applied + W->>Q: take live event + W->>G: AllowsWrites? + G-->>W: yes + W->>W: open commit window, commit with the user as author + D->>G: record scope clean again (no-op) + D->>D: publish status, acceptance, retention +``` + +What changes: + +1. `ResyncRequest` gains the stream's render-fidelity revision (`stream.revision`, already passed to + the drain), set by `enqueueReplayResync`. +2. `settleResyncApplied` calls `RecordScopeClean(target, revision, collection)` before + `answerResync`, when the request carries a revision. +3. The drain keeps its call. A second clean report under the same revision is accepted unchanged + (`recordScope` at [`render_fidelity_gate.go:188`](../../internal/git/render_fidelity_gate.go#L188)), + so it costs nothing. The drain still publishes the condition, `GitPathAccepted` and retention, + which stay where they are. +4. Divergence stays on the drain. A divergent scope keeps the gate closed, and so does a pending + one, so recording it later loses nothing. + +What it guarantees: **a live event queued behind every pending scope's replay resync sees the gate +those resyncs produced.** That covers Window A completely. + +What it does not cover: Window B. An event that enters the queue before a slower scope has even +finished listing still meets a pending gate. + +Size: a field, two call sites, and a unit test in `internal/git` that queues a replay resync and +a live event back to back and asserts the event is committed with its author. It runs the same +code path the e2e spec does, with no timing in it. No user-facing doc changes. + +## Fix 2: stop dropping live events while the gate is pending + +The gate exists so that GitOps Reverser does not write an edit to Git while it cannot yet tell +whether the folder's rendered values match the live objects. Holding an event back serves that +purpose as well as dropping it does. The difference is what happens once the gate opens. + +There are two ways to build it, and they trade completeness against attribution. + +### 2a: hold the events, release them when the gate opens + +```mermaid +stateDiagram-v2 + [*] --> Held: live event, gate Unknown + Held --> Applied: gate turns True
(released in arrival order) + Held --> Superseded: a newer replay resync
for the same collection arrives + Held --> Dropped: gate turns False (divergence),
or the hold exceeds its budget + Applied --> [*]: committed with the user as author + Superseded --> [*]: the snapshot carries the object's state,
committed by the operator + Dropped --> [*]: counted and logged at Info +``` + +- The worker keeps a per-GitTarget list of held live events instead of discarding them. Held + events count against the intake budget, like pending writes do. +- The gate tells the worker when a GitTarget's state changes (a channel or callback), so the + worker releases held events on the next wake instead of polling. +- **Ordering rule.** A replay resync for collection C that arrives while events of C are held + replaces those events: its snapshot is newer and already includes their effect. Releasing them + after it would write older state over newer. Events of other collections stay held. +- When the gate turns **False** (a divergence), held events are dropped, as today. That is the + gate doing its job. The difference is that the drop is counted. +- A budget (bytes and time) bounds the hold, because a scope can stay pending for a long time. The + watch-plane failures in + [watch-plane-status-convergence-failures.md](watch-plane-status-convergence-failures.md) + show one waiting indefinitely. Past the budget the worker falls back to 2b for that collection. + +Keeps: the user as commit author, which is what this e2e spec asserts and what the audit-route +work exists for. Costs: the most code of the three options. A new hold structure, a gate +notification and the supersede rule all need tests. + +### 2b: drop, then re-derive with a resync + +The worker keeps dropping, but remembers which collections lost an event while the gate was +pending. When the gate opens it asks the watch layer for a resync of those collections, and the +snapshot puts the current state in Git. + +```mermaid +sequenceDiagram + autonumber + participant W as Branch worker + participant G as Render-fidelity gate + participant S as Target watch stream + + W->>G: AllowsWrites? + G-->>W: no + W->>W: drop event, note "ConfigMaps in src lost an event" + G-->>W: GitTarget open + W->>S: request a resync of ConfigMaps in src + S->>W: snapshot resync + W->>W: commit current state (author: the operator) +``` + +Keeps: the "dropped while closed" rule, with the loss repaired afterwards. This is how a failed +window finalize is already handled ("gone until the next resync re-derives them"). Costs: the +commit carries the **operator** as author, not the user, because a snapshot has no audit fact +behind it. This e2e spec would still fail, since it asserts the author. + +### Either way: a drop is visible + +Built with Fix 1: `gitopsreverser_git_writes_dropped_total{reason="render_fidelity_pending"}` +counts each drop, and the first drop per gate status is logged at Info. Read it before choosing +between 2a and 2b: it says how often window B happens. + +## Side by side + +| | Fix 1 | Fix 2a: hold | Fix 2b: re-derive | +| --- | --- | --- | --- | +| Closes the e2e flake | Yes | Yes | No: right state, wrong author | +| Window A (answered, not recorded) | Closed | Closed | Repaired later | +| Window B (another scope replaying, operator start) | Still dropped | Held, then committed | Dropped, then re-derived | +| Commit author for a saved edit | The user | The user | The operator | +| Changes documented behavior | No | Yes: "dropped" becomes "held" | Partly: the drop is repaired | +| Size | Small | Large | Medium | +| Needs a decision | No | Yes | Yes | + +## Recommendation + +1. **Fix 1 and the drop counter are built.** +2. **Decide Fix 2.** If an API edit must never vanish while a GitTarget changes its + watches, which is what "API-first" promises, choose **2a**, with 2b as its fallback past the + budget. If dropping during a watch change stays acceptable, keep today's rule and let the new + counter show how often it happens. Measuring first is a reasonable step before building 2a. + +## A separate, smaller test bug + +The spec's `Describe` body calls `SetDefaultEventuallyTimeout(3 * time.Minute)` +([`audit_route_attribution_e2e_test.go:121`](../../test/e2e/audit_route_attribution_e2e_test.go#L121)). +That setting is global, and Ginkgo runs every `Describe` body while it builds the tree, so the last +file to set it wins. Seventeen e2e files set it, so this spec runs with whatever value the last of +them sets (30 seconds in the failing runs). This did not cause the flake, because a dropped event +never arrives however long the test waits. A per-spec timeout (`Eventually(...).WithTimeout`) or a +`BeforeEach` would make the value mean what it says. + +## How sure this is + +- **Read in the code:** the separate goroutines, the per-GitTarget gate, the drop at `V(1)`, the + cursor recorded past a routed event, the drain's idempotent clean record, and `Streaming` being + marked before the resync applies. +- **Seen in one CI log** (above): the timeline matches Window A on a sibling scope, and no commit + window ever opened for the ConfigMap. +- **Not seen directly:** the drop log line itself, which CI's log level hides. An e2e run with + `-v=1` on the manager, or the Info log from "make a drop visible", would show it. +- **Agent survey, not re-checked line by line:** the claim that all 9 failures have the same shape. diff --git a/docs/design/one-writer-per-branch-plan.md b/docs/design/one-writer-per-branch-plan.md index dd2a3fcb..dc9431ff 100644 --- a/docs/design/one-writer-per-branch-plan.md +++ b/docs/design/one-writer-per-branch-plan.md @@ -149,8 +149,8 @@ Ordered by how much they matter now. - **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`. The local review is - `docs/reviews/20261021-review-425.md`. + 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 diff --git a/docs/future/gitops-api-open-asks.md b/docs/future/gitops-api-open-asks.md new file mode 100644 index 00000000..335d816a --- /dev/null +++ b/docs/future/gitops-api-open-asks.md @@ -0,0 +1,124 @@ +# The gitops-api team's open asks + +Status: **open asks from a production user**, condensed on 2026-10-24 from their standing list +(revision 15, 2026-10-20, written against `v0.53.0`). The asks keep their numbers. Closed asks are +left out: #30 shipped as `GitReceiver` (#432), #33 went with the prefetch in #425, #22 is fixed +(`pkg/manifestanalyzer/repo.go`), #26 and #27 shipped (`git_branch_targets`, +`commit_requests_total`), #25 shipped in 0.44.0 (#343), and #6 is answered by immutability that +fails at admission with "delete and recreate". Kind and field names are updated to `main` +(`FluxClaim` for `FluxTrigger`, `KubeProvider` for `ClusterProvider`, `ClaimedResourceReconciled` +for `ResumedElsewhere`). + +Their suggested order, as of revision 15: #32, then #31, then #34 when the e2e budget allows; +then #23, #11 and #10; #28 when two owners share a cluster. + +## 32. A troubleshooting row: something replaces the Kustomization's whole spec + +**Ask.** One row in [the Flux guide](../bi-directional-flux.md)'s troubleshooting table: repeated +`ClaimedResourceReconciled` Events mean a controller writes the `Kustomization` with a full-spec +Update that omits `spec.suspend`; make it carry the field across. + +**Why.** The guide covers a manifest that *sets* `spec.suspend` (`SuspendOwnedElsewhere`). The more +common case in practice is a controller that never sets the field and replaces the whole spec +(`controllerutil.CreateOrUpdate` with a spec it builds from scratch). An Update that removes a field +another manager owns takes no ownership of it, so `SuspendOwnedElsewhere` can never fire: the +symptom is a resume Event plus an `OperationUnresolved` on every reconcile of that controller, and +Flux applying unchecked until the claim suspends it again. They reproduced it on purpose. The fix on +their side was three lines; finding it from the guide alone would not have been obvious. + +## 31. Count "resumed elsewhere" in a metric, not only in an Event + +**Ask.** A counter for an apply the claim did not start (the resume, and the `OperationUnresolved` +that follows it), labeled like `apply_requests_total`. Possibly also a timestamp in the claim's +status. + +**Why.** A resume behind the operator's back is the one way left for Flux to apply unchecked, so it +is what they most want to alarm on. Today it is only an Event, and an Event is a poor alarm source: +the API server aggregates identical ones (three resumes in three minutes showed as one Event with +`count: 3`) and drops them after an hour. They also saw the detection time vary from 2s to about +75s, which is the claim's steady poll; a watch on the held applier would close that window. That +watch is designed in [watching the held applier](../design/applier-claim-held-applier-watch.md). + +## 34. An e2e for an in-cluster applier with a remote source cluster + +**Ask.** One e2e where the `GitTarget`'s `KubeProvider` names a remote cluster and the `FluxClaim`'s +names the operator's own, with an API edit landing during an apply. + +**Why.** It is their production topology: Flux runs in the management cluster and applies into a +remote workspace through `spec.kubeConfig`; GitOps Reverser reads that workspace remotely and holds +the `Kustomization` locally. They proved the hold, the apply cycle, the receiver and `ForeignCommit` +in it, but never saw the apply window withhold anything (`apply_window_collisions_total` stayed at +zero), so the part that reads `managedFields` on the remote object is untested in this topology. +An e2e that provokes one collision would settle it. + +Two smaller things they noted: `OperationUnresolved` is recorded three times for one operation (on +the claim, the `GitTarget` and the `WatchRule`), and `apply_requests_total{outcome="sent"}` was +said to count both "the applier was asked" and "nothing needed asking". (On `main` an apply that is +not needed is never sent, so it is not counted; check whether that answers them.) + +## 28. Two operator instances cannot share one config-plane cluster + +**Ask.** A way for two instances sharing one config-plane cluster to partition the CR set, so that +exactly one instance reconciles a given `GitTarget`. Smallest useful first, and they stack: + +1. **`--watch-namespaces`**, a `Cache.DefaultNamespaces` pass-through. It partitions the namespaced + kinds, which since #430 includes `KubeProvider`; only `ClusterWatchRule` stays cluster-scoped. +2. **An explicit owner selector**, a label selector or a `spec.instance`-style field paired with an + `--instance-name` flag, which covers the cluster-scoped kinds too. + +**Why.** It is the same mechanism the chart's replica guard +(`charts/gitops-reverser/templates/validate-replica-count.yaml`) says is missing for HA: "Standby +replicas need ownership coordination first, so that two instances never write the same GitTarget." +They hit it moving onto a cluster that already runs an instance for its owner's own workloads: two +populations, different source clusters, providers, credentials and namespaces, and still both +instances would reconcile both sets. RBAC is no workaround: controller-runtime issues a cluster-wide +List, so a narrowed ClusterRole fails the cache sync and the manager exits. + +**What sharing costs them**, for calibration: one Valkey holding both populations' attribution +facts; one upgrade cadence; one RBAC posture that is the union of both; and, customer-visible, a +restart for one owner's reasons costs the other's users their commit message, because +`RenderMatchesLive` starts `Unknown` after a restart and an open window is dropped. They are not +blocked: one shared instance works. + +## 23. Deletion-as-intent picks the wrong actor when a controller clears the finalizer + +**Probably fixed by #431; verify with their reproduction.** #431 credits a terminating object's +removal to its deletion evidence first. + +**Ask.** Attribute a deletion-as-intent `DELETE` to the actor who **set** `deletionTimestamp`, not +to whoever makes the next modification while it is set. + +**Why.** Measured on `v0.40.1`: a human deletes a CR that their operator finalizes, and the commit +names the operator's service account. The operator's finalizer-removal `UPDATE` is itself a +modification of a terminating object, so it renders as the `DELETE` and wins the commit; the human's +own delete arrives a moment later and folds to a no-op because the file is already gone. The more +finalizers an object has, the more likely a controller's cleanup write is observed first. + +**Reproduction** (about 90 seconds): create a namespaced CR with a finalizer, mirror it with a +`ClusterWatchRule`, delete it as a human, let the controller clear the finalizer, read the commit. + +## 11. Serialize `[CREATE]` and `[UPDATE]` bodies identically + +**Ask.** One encoder for both paths. + +**Why.** A create rendered list items at 2-space indent, an update at 4, so every install churned +all the lines of a list to carry one field. Unverified since `v0.35.0`, but the two paths still +reach different encoders (`pkg/manifestanalyzer` renders via JSON to YAML; `internal/git/manifestedit` +uses `yaml.v3` with `SetIndent(2)`). Cosmetic, but it makes `git diff` on the mirror useless for +seeing what changed. + +## 10. Sibling inference can place an object in another namespace's directory + +**Ask.** Consider the namespace when inferring where a *new* document goes. Cosmetic. + +**Why.** The cold-start layout is namespace-first, but once a layout exists, placement follows +siblings. Two tenants syncing a repository named `infra` produce two objects named `infra` in +different namespaces, and one lands in the other's directory. Nothing is corrupted, but +`git log -- tenants/tenant-a/` shows another tenant's history. + +## A possible #24: say when a stored field was pruned + +From their 0.43 upgrade: a deleted field is pruned by the API server, so the object stays +`Ready=True` and quietly uses a default or a wider scope. They had to write a test asserting the +*absence* of the old spellings. A condition or Event on an object applied carrying a field the CRD +no longer serves would make a migration self-diagnosing. Not filed as an ask. diff --git a/docs/future/idea-applier-claim-ownership.md b/docs/future/idea-applier-claim-ownership.md new file mode 100644 index 00000000..a3101c34 --- /dev/null +++ b/docs/future/idea-applier-claim-ownership.md @@ -0,0 +1,46 @@ +# One holder per applier, across `KubeProvider`s + +Status: **idea**, nothing built. Two ways to close a known gap, and a third model for consent, +recorded from the applier claim rename plan (#438) and review 7 of #420. + +## The gap + +[`holder.go`](../../internal/applier/holder.go) (`sameApplier`) decides that two applier claims +name the same applier by `ClaimKind()`, `KubeProviderKey()`, and the applier's namespace and name. +Two `KubeProvider`s that reach **one** cluster, for example the chart's `local` and a team's own +provider with a kubeconfig for the same API server, therefore give two holders for one +`Kustomization`. Both suspend it under the same field manager. Each reads the other's resume as a +`flux resume` and suspends it mid-apply. Deleting one hands back a hold the other still needs. For +an `Application`, both start syncs. + +This is a misconfiguration, not an attack, so the decision not to design against spoofed Flux or +Argo CD identity does not cover it. It is documented today (O1): an applier claim is exclusive only +among claims made through the same `KubeProvider` +([definitions.md](../definitions.md#applying-back), +[configuration.md](../configuration.md#who-applies-this-folder-specapplierclaimref)). + +## O2: compare cluster identity (partial) + +- The `KubeProvider` records the cluster's identity in status, the `kube-system` Namespace UID read + once at validation, and `sameApplier` compares that instead of the provider key. +- Where the credential cannot read that Namespace, it falls back to the provider key, and the O1 + limitation still applies. +- Cost: `get` on `namespaces/kube-system`, plus a note in + [architecture.md](../architecture.md) (which says the operator "reads no `Namespace` objects in a + source cluster") that this read is about identity, not authorization. + +## O3: an object-local stamp (complete) + +- Write the holder's identity into the `Kustomization` in the same server-side apply as the + suspend, and refuse to adopt over a live stamp that is not ours. This also catches two operator + installs on one cluster. +- Cost: two first adopters can race, so arbitration has to work across operators, and an + `Application` needs a write of its own. + +## Consent on the applier (`ReferenceGrant` style) + +Who may claim an applier is decided today by the `KubeProvider`'s `allowedApplierClaims` grants, +which name the applier namespaces a consumer namespace may reach. Review 7 of #420 weighed a +stronger model: consent on the `Kustomization` or `Application` itself, in the shape of Gateway +API's `ReferenceGrant`, so an applier's owner says which namespaces may hold it. It is the best +long-term model, and it is worth building only if delegation per provider proves too coarse. diff --git a/docs/future/idea-argocd-applicationset-claim.md b/docs/future/idea-argocd-applicationset-claim.md new file mode 100644 index 00000000..9ca946ba --- /dev/null +++ b/docs/future/idea-argocd-applicationset-claim.md @@ -0,0 +1,139 @@ +# Claiming an Argo CD ApplicationSet: parked design + +Status: **parked**, 2026-10-23. Nothing is built. The direct-claim refusal it builds on shipped +in #437 (`OwnedByApplicationSet`). Argo CD facts below were read from Argo CD's source at 3.5.0 +(`external-sources/argo-cd`, commit `3cbae653d`). + +## Where this comes from + +Since #437 an `ArgoCDClaim` behaves like a `FluxClaim`. It owns +`spec.syncPolicy.automated.enabled: false` on its `Application` through server-side apply, switches +it off again when someone switches it on (with an Event), and releases it on hand-back. + +That can't work on an `Application` an `ApplicationSet` generated, because the `ApplicationSet` +rewrites the generated spec on every reconcile. So a direct claim on a generated `Application` is +refused, with a reason that names the `ApplicationSet` (`OwnedByApplicationSet`). + +This document is the other half: let the claim name **the `ApplicationSet` itself**. The user +then chooses where control sits. A direct claim to an `Application` someone else manages stays an +error, with a clear message that points here. + +## The use case: rolling out multi-branch editing + +1. A team creates a `GitTarget` on a new branch (a `GitBranch` with `parentBranch: main`), and + GitOps Reverser creates the branch. +2. An `ApplicationSet` discovers the branch and generates an `Application` for it, with + `targetRevision` set to the branch. +3. One claim on the `ApplicationSet` keeps every generated `Application` manual and coordinates + each of them with the `GitTarget` that writes its branch. + +Steps 1 and 2 already work today. Step 3 is what is missing: today each generated `Application` +would need its own claim, and those are refused. + +## What Argo CD does + +| Fact (3.5.0) | Consequence for us | +| --- | --- | +| The controller replaces the whole spec of each generated `Application` with the rendered template (`found.Spec = generatedApp.Spec`, `applicationset_controller.go`), minus `spec.ignoreApplicationDifferences` | Any field we own on a generated `Application` is wiped on the next reconcile. **Control has to sit on the `ApplicationSet`'s template**, not on its `Application`s | +| It sets a controller `ownerReference` from each generated `Application` to its `ApplicationSet`, and removes it when the `ApplicationSet` is deleted with orphaning | A generated `Application` can be recognized from its `ownerReferences`. The direct-claim refusal can name the owner | +| `spec.syncPolicy.applicationsSync`: `create-only`, `create-update`, `create-delete`, `sync` (the default). Only `create-update` and `sync` update existing `Application`s | Under `create-only` / `create-delete`, a template change never reaches `Application`s that already exist. Taking the template switch is not enough there; the claim must also check (or own) the switch on each existing `Application` | +| `spec.templatePatch` is a Go-templated patch applied per `Application` after the template | One generated `Application` can switch automated sync back on even when the template has it off. **Check the generated `Application`s, not only the template** | +| **`RollingSync` (progressive syncs)** forces `automated.enabled: false` on the generated `Application`s, and the `ApplicationSet` controller starts their syncs itself by setting `operation` (`progressivesync/progressive_sync.go`) | Two parties would start syncs. An `ApplicationSet` with `strategy.type: RollingSync` must be refused | +| `status.resources` lists the generated `Application`s (capped; `status.resourcesCount` has the full count) | Where we read which `Application`s exist. The cap means we should list by owner, not trust `status.resources` alone | +| The SCM Provider generator lists branches (`allBranches`, `filters[].branchMatch`) and re-scans every 30 minutes by default (`requeueAfterSeconds`). The `ApplicationSet` webhook refreshes Git, Pull Request and Plugin generators, **not** the SCM Provider generator | Branch discovery through SCM Provider is slow unless `requeueAfterSeconds` is lowered. See [the generator question](#which-generator-finds-the-branches) | + +## The shape + +### Option A: a reference on `ArgoCDClaim` (lean) + +```yaml +kind: ArgoCDClaim +metadata: {name: checkout, namespace: team-a} +spec: + kubeProviderRef: {name: management} + applicationSetRef: {namespace: argocd, name: checkout} # exactly one of applicationRef / applicationSetRef +``` + +- CEL: exactly one of `applicationRef` and `applicationSetRef`. Both immutable, like today. +- It reuses the claim's existing machinery: `KubeProvider`, its `allowedApplierClaims` grants + (`applierNamespaces`), `Configured` / `Ready`, finalizer. +- `GitTarget.spec.applierClaimRef` is unchanged: a `GitTarget` names the claim, and the claim + finds the generated `Application` that applies that `GitTarget`'s branch. + +### Option B: a separate kind, `ArgoCDApplicationSetClaim` + +Clearer status (a per-`Application` list instead of one `operation`), but a fourth kind and a +third value in the `applierClaimRef` kind enum. Choose this only if the status shapes turn out too +different to share. + +### What the claim does to the `ApplicationSet` + +- **Takes the switch on the template:** owns + `spec.template.spec.syncPolicy.automated.enabled: false` through server-side apply. It switches + it off again on drift (with a `Normal` Event) and releases it on hand-back, as the + `Application` claim does. +- **Refuses** (`Configured=False`, each with a message that names the field to change): + - `strategy.type: RollingSync`: the `ApplicationSet` starts syncs itself. + - `applicationsSync` that doesn't update (`create-only`, `create-delete`) while an existing + `Application` has automated sync on: the template switch can't reach it. + - a `templatePatch` that leaves automated sync on for an `Application` a `GitTarget` needs. + - a template with more than one source, or a `targetRevision` that isn't a branch (the existing + `UnsupportedSource` / `SourceMismatch` rules, applied to the template). +- **Matches `GitTarget`s to `Application`s:** a generated `Application` belongs to the `GitTarget` + that writes the repository and branch it syncs (`repoURL` + `targetRevision`). That is today's + eligibility rule, applied per `Application`. A `GitTarget` with no matching `Application` yet + waits: the generator may not have found its branch. +- **Syncs per `Application`:** each sync is still started on the generated `Application` + (`operation`, pinned to the commit), exactly as today. Setting `operation` is safe: the + `ApplicationSet` replaces `spec`, and only copies `operation` from the template when the template + sets one. + +### Status + +One claim now covers many `Application`s, so `status.operation` and `status.repairPending` become +per `Application`: + +```yaml +status: + applications: # listType=map, listMapKey=name; only Applications a GitTarget is bound to + - name: checkout-feature-x + branch: feature-x + operation: {phase: Succeeded, commit: 1a2b3c4, reason: ForeignCommit} + repairPending: false +``` + +Printer columns would show counts (bound / applying), not one operation. + +## Which generator finds the branches + +| Generator | How it finds GitOps Reverser's branches | Fit | +| --- | --- | --- | +| SCM Provider (`allBranches` + `branchMatch`) | Lists branches through the Git host's API (GitHub, GitLab, Gitea, Bitbucket, Azure DevOps, CodeCommit) | Works today. Needs a host API token, and has a 30-minute default re-scan with no webhook refresh | +| Pull Request | One `Application` per open PR | Fits the editing product (branch → PR → preview). An edit branch only appears once its PR is opened | +| **Plugin**, served by GitOps Reverser | The `ApplicationSet` calls `POST /api/v1/getparams.execute` on an HTTP service; GitOps Reverser would answer with the branches its `GitTarget`s write (`branch`, `path`, `namespace`, …) | The most direct answer to "watch for GitOps Reverser's branches". It's refreshed on every `ApplicationSet` webhook. It's also a new network surface with its own auth; out of scope until the claim exists | + +## Open questions + +- **Who manages the `ApplicationSet`?** It is often applied by another Argo CD `Application` + (app-of-apps) or by Flux. If that manifest sets `template.spec.syncPolicy.automated`, the fight + moves one level up. Same answer as Flux's "the manifest must omit `suspend`": detect a foreign + manager of that field in `managedFields` and refuse with the name of that manager. +- **Several `GitTarget`s on one generated `Application`** (different paths, same branch): allowed + today for one `Application`, so allowed here. The per-`Application` status entry holds one + operation. +- **Hand-back across many `Application`s:** releasing the template switch turns automated sync on + for all of them at once. Does hand-back need to wait for every in-flight sync, as the Flux + hand-back waits for its last apply? +- **Applications in any namespace:** generated `Application`s may land outside `argocd`. The + `KubeProvider`'s `allowedApplierClaims` grant must name every namespace they land in in + `applierNamespaces`, or the claim refuses the ones it doesn't reach. +- **RBAC:** `get`, `list`, `watch`, `patch` on `applicationsets`, on top of what the `Application` + claim needs. + +## Not doing + +- **Claiming generated `Application`s one by one.** The `ApplicationSet` owns their spec; that is + exactly the fight the direct-claim refusal prevents. +- **Using `ignoreApplicationDifferences` to let us own the switch per `Application`.** It works, + but asks the user to edit the `ApplicationSet` to make room for us, and the switch then lives in + two places. Owning the template switch is simpler. diff --git a/docs/future/idea-source-namespace-selector.md b/docs/future/idea-source-namespace-selector.md new file mode 100644 index 00000000..79247aec --- /dev/null +++ b/docs/future/idea-source-namespace-selector.md @@ -0,0 +1,66 @@ +# A namespace selector in a `KubeProvider` grant's scope + +Status: **idea**, nothing built. Recorded from the review of the grant shapes that #441 shipped. +A grant's consumer side (`consumerNamespaces`) already takes a selector; this page is about the +scope side, the source-cluster namespaces a grant lets a `GitTarget` mirror (`sourceNamespaces`). + +Should a platform-owned `KubeProvider` be able to grant "namespaces labeled +`platform.example.com/tenant: team-a`" in the source cluster? Then a tenant's newly provisioned +namespaces would be covered as soon as they carry the label, without anyone editing the grant. That +is a reasonable capability. It is not built; this page records where it would fit. + +**The September removal priced the work; it did not judge the design.** +[source-scope-simplification.md](../design/source-scope-simplification.md) deleted +`GitTarget.spec.allowedSourceNamespaces` and its source-side selector on 2026-09-01, about 4,500 +lines. Part of its argument no longer holds: it called the field "cheap only because it enforces +nothing", because the tenant wrote it on their own `GitTarget`. Grants now sit on the +platform-owned `KubeProvider`, so a selector there would enforce something. + +**Two capabilities, to be kept apart:** + +- **Authorization.** A selector in the grant admits namespaces that a WatchRule still names + explicitly. That is useful on its own: the platform manages labels instead of grant lists. It is + also the smaller piece, because the scope check reads the labels of a namespace that is already + named. +- **Automatic following** ("watch everything my scope selects"). Today a `"*"` rule is refused + under a narrower scope, never narrowed to it, and a `WatchRule` item otherwise names one namespace. So this needs + its own rule-side design, such as a `"*"` that is narrowed instead of refused. That would change + the meaning of a value users already write, so it needs its own decision. It is later work, and + not a precondition for authorization. + +**The operational contract to settle before building either:** + +- revocation when a label is removed, mirroring + `TestGitBranch_ANamespaceLabelLossRevokesASelectorGrant` + (`internal/controller/gitbranch_access_test.go`) on the control side; +- what happens when source-cluster labels cannot be read: a failed lookup must never become an + empty selection or a silent allow, and it is not a denial either. The deleted three-valued + verdict existed for this; +- who may write authorization labels in the source cluster, where a tenant's own Flux or Argo CD + may be able to label Namespaces; +- `Namespace` get/list/watch for the provider's identity. + +**Shape, when it comes:** + +- **Add a sibling field, `sourceNamespaceSelector`, next to the `sourceNamespaces` list.** It is + additive: every manifest written today stays valid, which is what justifies keeping + `sourceNamespaces` a list now. The alternative, turning it into an object + (`sourceNamespaces: {names: [...], selector: ...}`), would break every existing manifest. Once + there are users, that needs a migration or a new API version. That cost decides it, not + aesthetics. Nesting it now only to avoid that later would cost every reader today, for a field + that doesn't exist. +- **Make the two arms alternatives, not additions.** In `consumerNamespaces`, `names` and + `selector` are alternatives picked by `from`. A grant with both `sourceNamespaces` and + `sourceNamespaceSelector` should be refused, so the same two ideas keep one combination rule in + one grant. Users get the union through separate grants, as they already do for consumers ("Team + B or every tenant is two grants"). +- **Don't reuse the `from` enum.** `Same` means the holder's control-cluster namespace and has no + meaning in the provider's cluster. Exact names, `"*"` and `{consumer.namespace}` already cover + the useful cases, and `"*"` stays in the list, where it is today. +- **Consider `{consumer.namespace}` in label values** + (`matchLabels: {platform.example.com/tenant: "{consumer.namespace}"}`). Static selectors already + pay for themselves: a tenant's newly labeled namespaces are authorized without editing its + grant. The variable adds one thing on top: one grant can serve every tenant, instead of one + grant per tenant. Useful, but not what makes selectors worthwhile. +- **`applierNamespaces` gets no selector** until someone asks for one. Appliers live in a few + fixed namespaces such as `flux-system`. diff --git a/docs/reviews/20261022-apply-binding-rename.md b/docs/reviews/20261022-apply-binding-rename.md deleted file mode 100644 index d5b466e2..00000000 --- a/docs/reviews/20261022-apply-binding-rename.md +++ /dev/null @@ -1,479 +0,0 @@ -# Applier claim rename and doc corrections: work plan - -Status: proposal, 2026-10-22. The kind name was settled on 2026-10-23: **`FluxClaim` / -`ArgoCDClaim`**, referenced as `spec.applierClaimRef`. The original review was checked against -`feat/gitreceiver` at `a4928dcf`. The coordinated-folder terminology is already in definitions.md -and bi-directional.md. - -This plan folds in three rounds: - -- an external review that proposed renaming `FluxTrigger` / `ArgoCDTrigger`, listed four doc - corrections, and found an ownership caveat; -- that reviewer's follow-up on the first draft, which moved the name from "Applier" to "Binding" and - corrected C1, the PR scope and O2. See [What the follow-up changed](#what-the-follow-up-changed); -- a naming discussion that weighed active names against Binding and chose Claim, and the - reviewer's agreement with three qualifications. See - [What the naming discussion changed](#what-the-naming-discussion-changed). - -There are no users yet, so the rename is a **hard cut**: no migration path, no compatibility -period, and no superseded field kept around to reject. - -## What to do - -**Decided 2026-10-23: one PR, stacked on #437, in separate commits.** #432 has merged, so nothing -forces the doc corrections to land first, and writing them in trigger terms would only have them -rewritten a commit later. The sections below still say "PR A" and "PR B" for the two halves: - -**PR A: doc corrections.** C1–C3 and the ownership limitation (O1), written straight in claim -terms. It changes a Go comment and a godoc that flows into the generated CRDs. - -**PR B: the rename.** `FluxTrigger` → `FluxClaim`, `ArgoCDTrigger` → `ArgoCDClaim`, -`spec.applyTriggerRef` → `spec.applierClaimRef`, removed outright. C4 is limited to text swaps -(`ClusterProvider` → `KubeProvider`) and deleting the unused SVG; the structural redraw of -config-basics is done by hand in Excalidraw, here or in a follow-up. - -**Not in this PR:** a code fix for the ownership limitation (O2/O3). Decide on that separately. - -**Since #437** (Argo CD automated sync held off, as a suspend): an `ArgoCDClaim` holds its -`Application` the way a `FluxClaim` holds its `Kustomization`, so the definition below says so, and -**Held** covers both kinds. The Argo CD finalizer `configbutler.ai/argocd-trigger` becomes -`configbutler.ai/argocd-claim`; `argocdtrigger_hold*.go`, `argocdtrigger_apiserver_test.go` and -`applytrigger_holder_test.go` are renamed with the rest. #437's own names stay: -`status.previousSuspendState`, `SuspendState`, `ArgoCDVersionNotSupported` and the -`ClaimedResourceReconciled` Event, which already reads in claim terms. - -## The vocabulary - -| Word | Means | Status | -| --- | --- | --- | -| **Applier** | The external Flux `Kustomization` or Argo CD `Application` that applies a folder | unchanged | -| **Applier claim** | Our exclusive, persistent claim on one applier: a `FluxClaim` or an `ArgoCDClaim` | replaces "apply trigger" | -| **Apply request** | One ask to an applier: this commit, for this reason | unchanged | -| **Apply operation** | The execution of a request, followed until the applier's status shows it ended | unchanged | -| **Bound** | An applier claim with at least one eligible `GitTarget` | already in [definitions.md:219](../definitions.md#applying-back); says nothing about whether it holds the applier (that is **Held**) | -| **Coordinated** | A folder, or its `GitTarget`, configured for GitOps Reverser to control apply timing through an applier claim | describes the folder's setup; `Bound` still describes the applier claim | - -The sentence that ties them together: - -> An applier claim declares that GitOps Reverser should take and retain control of when a -> referenced applier applies. A `GitTarget` names one with `spec.applierClaimRef`. - -The sentence says "declares", not "gives". An applier claim can exist without control: no eligible -`GitTarget` references it yet, another applier claim holds the applier, or the applier isn't ready. - -Proposed entry for [definitions.md "Applying back"](../definitions.md#applying-back), replacing -**Apply trigger**: - -> **Applier claim.** This operator's resource that declares GitOps Reverser should take and retain -> control of when one applier applies: a `FluxClaim` (`spec.kustomizationRef`) or an `ArgoCDClaim` -> (`spec.applicationRef`), each with the `KubeProvider` that reaches it. That control covers apply -> timing and nothing else: the applier still fetches, renders, applies, prunes and reports health. -> The claim does not say when to apply either; the branch worker decides that. Among applier claims -> that reach an applier through the same `KubeProvider`, one holds it and the others report -> `HeldByAnotherClaim`; claims made through two `KubeProvider`s for one cluster are not arbitrated -> (see **In line**). Whether a claim holds its applier is **Held**, not **Bound**. While it holds a -> `Kustomization`, a `FluxClaim` keeps it suspended and resumes it only for an apply GitOps Reverser -> asks for, and deleting the claim releases the `Kustomization` back to Flux. While it holds an -> `Application`, an `ArgoCDClaim` keeps automated sync off and starts each sync itself, and deleting -> the claim switches automated sync back as `status.previousSuspendState` recorded it. A `GitTarget` -> names one with `spec.applierClaimRef` and -> reports it in `ApplierClaimReady`. The applier claim is never called the applier: the applier is -> the object it claims. - -**Prose rule:** write "applier claim" or the kind name, never a bare "claim". definitions.md already -reserves a bare "claim" for a rule's demand on the read side (`claimed` in code), and defines -**Branch claim** for the Git side. The audit code also talks about OIDC token claims. - -Proposed **Coordinated** entry for definitions.md after the rename: - -> **Coordinated.** A folder configured for GitOps Reverser to decide when its applier applies. -> Naming an applier claim makes a `GitTarget`'s folder coordinated. The word also describes the -> `GitTarget` that writes it. It describes the setup; `ApplierClaimReady` reports whether the -> applier claim is ready. A shared folder applied without that coordination is **uncoordinated**: -> the applier keeps its own schedule and can revert live edits before they reach Git. - -Use these words in the bi-directional guide's decision tree: **coordinated folder** where GitOps -Reverser controls apply timing, and **uncoordinated shared folder** where the applier keeps its own -schedule. Keep the one-way case separate. `Bound` continues to describe the applier claim. - -## Why rename, and why Claim - -### Why drop "Trigger" - -- **These resources don't declare the triggering conditions.** A trigger can be persistent, so - that isn't the objection. The objection is that the spec only names the external object and the - connection to reach it. *When* to apply is decided by the branch worker (after a flush, on a - quiet branch, for a `ForeignCommit` / `Refusal` / `Repair` / `CatchUp`). A name that suggests - the object holds those conditions sends readers to the wrong place. -- **"Trigger" already means three other things here:** watch-plan triggers - (`gitopsreverser_watch_plan_triggers_total`, `TriggerRuleChange`), API-surface triggers - (`internal/watch/api_surface_triggers*.go`), and Argo Events triggers. - -### Why Claim - -The naming question is **authority over apply timing**. GitOps Reverser takes that authority, -keeps it, and eventually hands it back, while Flux still does the applying. The resource that -establishes this is **exclusive and taken**: among applier claims reaching an applier through the -same `KubeProvider`, one holds it, the others report `HeldByAnotherClaim`, and deleting the holder -releases the applier. That is what a claim is. - -- **It fits the lifecycle.** An applier claim is declared, competes for the applier, holds it, and - releases it. Every step has a plain sentence. -- **It separates intent from success.** A `FluxClaim` is still an applier claim while another one - holds the `Kustomization`, so `HeldByAnotherClaim` reads naturally. Binding and Control both - suggest the relationship already exists. -- **It does not limit the authority to timing by itself.** "This is ours for now" could suggest - broader ownership. The definition sets the boundary, as it would for any name; Claim's advantage - is the lifecycle and the resource model, not a narrower meaning. -- **The reference structure is a Kubernetes precedent.** Pod → `PersistentVolumeClaim` → - `PersistentVolume` is `GitTarget` → `FluxClaim` → `Kustomization`. Several pods can share one - PVC, just as several `GitTarget`s name one applier claim. The workload points at the claim, which - also settles Binding's main weakness: in a `RoleBinding` the binding names both sides, while here - the `GitTarget`s point at it. Naming an applier claim from a `GitTarget` is the consent to hand - over deployment timing. The analogy stops at the structure: a `Bound` PVC has its volume, while a - **Bound** applier claim only has an eligible `GitTarget` referencing it. Holding the applier is - **Held**. -- **It is the same idea as the Git side's branch claim.** A `GitBranch` holds a repository branch - and another reports `BranchConflict`; an applier claim holds an applier and another reports - `HeldByAnotherClaim`. One word for one idea on both sides. -- **It keeps the four concepts distinct.** Applier, applier claim, apply request and apply operation - each get their own word. "Applier" keeps its meaning in about 330 places, and `Applier()`, - `ApplierKind*`, `git.ApplierRef`, `ApplierNotFound` and `ApplierNamespaceNotAllowed` still name - the external object. `ApplierClaim.Applier()` reads correctly. holder.go's "holder" is simply the - applier claim that holds the applier. -- **Against:** - - Neither Claim nor any other short name says "timing"; the only one that does, Scheduler, - misleads. The definition and the `HELD` column carry it. - - A bare "claim" already has other meanings here, hence the prose rule. - - Crossplane users know a claim as a request that gets provisioned, not a takeover of an existing - object. - -### The example - -```yaml -kind: GitTarget -metadata: {name: checkout, namespace: team-a} -spec: - path: apps/checkout - applierClaimRef: {kind: FluxClaim, name: checkout} ---- -kind: FluxClaim -metadata: {name: checkout, namespace: team-a} -spec: - kubeProviderRef: {name: management} - kustomizationRef: {namespace: flux-system, name: checkout} -``` - -```text -$ kubectl get fluxclaims -n team-a -NAME READY REASON HELD KUSTOMIZATION OPERATION AGE -checkout True Succeeded Suspended checkout Succeeded 3d -``` - -The doc sentences become: "Deleting a `FluxClaim` releases the `Kustomization` back to Flux." "Two -applier claims on one `Kustomization`: one holds it, the other reports `HeldByAnotherClaim`." - -### Alternatives considered - -| | Claim | Binding | Applier | Gate | ApplyControl | -| --- | --- | --- | --- | --- | --- | -| On a `GitTarget` | `applierClaimRef` | `applyBindingRef` | `applierRef` (reads best) | `applyGateRef` | `applyControlRef` | -| Shows the authority | yes: declared, taken, released; the definition limits it to timing | no (the definition does) | yes | yes | yes, but too broadly | -| Keeps "applier" = `Kustomization` / `Application` | yes | yes | **no**: about 330 uses re-read, plus a new generic word | yes | yes | -| Collides with an existing word | bare "claim" (read-side demand); same sense as branch claim | no | `ApplierNotFound`, `ApplierNamespaceNotAllowed` | **Gate** ([definitions.md:321](../definitions.md), about 1,400 lines) | "controller", "reconcile" | -| Fits Argo CD as well as Flux | yes | yes | yes | weaker: nothing is held closed | yes | - -- **Binding** (`FluxBinding`, `spec.applyBindingRef`) was the working name until 2026-10-23. It - describes the relationship but hides that the relationship is exclusive and takes authority, and - the `RoleBinding` precedent points the reference the wrong way. -- **Applier** (`FluxApplier`, `spec.applierRef`) reads best on a `GitTarget`, but a clearer public - name produces less clear internal names. It also suggests GitOps Reverser applies manifests - itself, the way `kubectl apply` or cli-utils' `apply.Applier` does. -- **Gate** (`FluxGate`, `spec.applyGateRef`) is the most accurate description of what Flux sees, and - Pod `spec.schedulingGates` is a good precedent. But "Gate" is already a defined term (a condition - `Ready` aggregates), and the acceptance, render-fidelity and write gates are everywhere. -- **Hold** (`FluxHold`) names the mechanism and matches `Held` and holder.go. But "hold" is used - loosely all over the code and docs ("Git holds a commit", "hold off resyncs", "a route is held"), - and a Hold that also starts applies reads as a contradiction. -- **ApplyControl / ApplyController / ApplyReconciler** name the authority, but in an operator - codebase they read as the reconcile loop, not a resource. "An apply control" is awkward as a - countable noun, and Control suggests authority over what is applied as well as when. -- **ApplyGovernor** suggests rate limits, quotas or policy. **ApplyScheduler** suggests intervals, - calendars or queued jobs, and understates keeping and handing back control. -- **ApplyCoordinator** fits publishing, requesting and tracking, but understates unilateral control. - Coordination still supplies **coordinated** for the folder's setup. -- **Lease** has the right single-holder semantics but implies expiry and renewal, and collides with - `coordination.k8s.io/Lease`, which leader election already uses. -- **Custody** is accurate (temporary authority, handed back) but too unusual for a kind. -- **Link** is a shorter Binding with the same weakness. **Driver** suggests a plugin or binary. - **Cooperation** suggests shared control. - -Whichever words change, **applier**, **apply request**, and **apply operation** keep their meanings. - -## PR B: rename inventory - -### Public surface - -| Current | New | Where | -| --- | --- | --- | -| kind `FluxTrigger`, plural `fluxtriggers` | `FluxClaim`, `fluxclaims` | [fluxtrigger_types.go](../../api/v1alpha3/fluxclaim_types.go), CRD `configbutler.ai_fluxtriggers.yaml` | -| kind `ArgoCDTrigger`, plural `argocdtriggers` | `ArgoCDClaim`, `argocdclaims` | [argocdtrigger_types.go](../../api/v1alpha3/argocdclaim_types.go), CRD file | -| `GitTarget.spec.applyTriggerRef` (`ApplyTriggerReference`) | `spec.applierClaimRef` (`ApplierClaimReference`); old field removed, not rejected | [gittarget_types.go:82-93](../../api/v1alpha3/gittarget_types.go); enum at [applytrigger_types.go:43](../../api/v1alpha3/applierclaim_types.go) | -| `onRefusal: Reapply` CEL rule and message ("needs spec.applyTriggerRef, the trigger asked to reapply") | names `spec.applierClaimRef`, "the applier claim" | [gittarget_types.go:36](../../api/v1alpha3/gittarget_types.go) | -| condition `ApplyTriggerReady` and its printer column | `ApplierClaimReady` | [gittarget_apply_trigger.go:19](../../internal/controller/gittarget_applier_claim.go), [gittarget_types.go:545](../../api/v1alpha3/gittarget_types.go) | -| reason `ApplyTriggerNotFound` | `ApplierClaimNotFound` | [gittarget_apply_trigger.go:23](../../internal/controller/gittarget_applier_claim.go) | -| reason `HeldByAnotherTrigger` | `HeldByAnotherClaim` | [holder.go:32](../../internal/applier/holder.go) | -| finalizer `configbutler.ai/flux-trigger` | `configbutler.ai/flux-claim` | [fluxtrigger_types.go:17](../../api/v1alpha3/fluxclaim_types.go) | -| RBAC markers and `config/rbac/role.yaml` (`fluxtriggers`, `/status`, `/finalizers`, `argocdtriggers`) | `fluxclaims`, `argocdclaims` | [applytrigger_controller.go:139-142](../../internal/controller/applierclaim_controller.go), [gittarget_controller.go:151](../../internal/controller/gittarget_controller.go) | -| chart README (CRD list, uninstall commands), `values.yaml:123-130,373`, `values.schema.json:419`, `templates/applier-rbac.yaml` comments | the new kinds; the keys `applierAccess` and `allowCrossNamespace.appliers` **stay** (they are about appliers) | `charts/gitops-reverser/` | -| samples `fluxtrigger.yaml`, `argocdtrigger.yaml`, samples README | `fluxclaim.yaml`, `argocdclaim.yaml` | `config/samples/` | -| e2e CRD wait list | the new CRD names | [Taskfile-e2e.yml:1332](../../test/e2e/Taskfile-e2e.yml) | - -These keep their names: the `Held` condition and its reasons, `status.operation`, -`status.repairPending`, `ApplyReason`, `ApplyOperationPhase`, every `apply_*` metric, and the -`applier_kind` label with its values `kustomization` / `application`. - -### Internal Go - -Rename the files: - -- `api/v1alpha3/{applytrigger,fluxtrigger,argocdtrigger}_types.go` → `{applierclaim,fluxclaim,argocdclaim}_types.go` -- `internal/controller/applytrigger_*.go` → `applierclaim_*.go` -- `fluxtrigger_hold*.go`, `fluxtrigger_suspend_apiserver_test.go` → `fluxclaim_*` -- `gittarget_apply_trigger*.go` → `gittarget_applier_claim*.go` -- `internal/git/apply_trigger*.go` → `applier_claim*.go` -- `test/e2e/apply_trigger.go` → `applier_claim.go` - -Rename the identifiers: - -| Current | New | -| --- | --- | -| `ApplyTrigger` (interface) | `ApplierClaim` | -| `ApplyTriggerReconciler` | `ApplierClaimReconciler` | -| `TriggerKind()` | `ClaimKind()` | -| `ApplyTriggerKindFlux` / `ApplyTriggerKindArgoCD` | `ApplierClaimKindFlux` / `ApplierClaimKindArgoCD` | -| `ApplyTriggerConditionHeld` / `…Configured` | `ApplierClaimConditionHeld` / `…Configured` | -| `ApplyTriggerStatus`, `ApplyTriggerStatusWriter`, `PublishApplyTriggerStatus`, `SetApplyTriggerStatusWriter` | `ApplierClaim…` | -| `ResolveApplyTrigger`, `nameApplyTrigger`, `observeApplyTrigger`, `NewApplyTrigger` | `…ApplierClaim` | -| `FluxTrigger*` / `ArgoCDTrigger*` (`Spec`, `Status`, `List`, `Source`, `Finalizer`) | `FluxClaim*` / `ArgoCDClaim*` | -| `TriggersOfApplier`, `holderTrigger`, `errTriggerReleased`, `applyTriggerLoop`, `applyTriggerVerdict`, `applyTriggerSuspendingInterval`, `GitTargetConditionApplyTriggerReady`, `ReasonApplyTriggerNotFound` | `ClaimsOfApplier`, `holderClaim`, `errClaimReleased`, `applierClaimLoop`, `applierClaimVerdict`, `applierClaimSuspendingInterval`, `GitTargetConditionApplierClaimReady`, `ReasonApplierClaimNotFound` | -| test helpers (`testFluxTrigger`, `createFluxTrigger`, `reconcileTrigger`, `fluxTriggerResource`, …) | `…Claim` | - -**Keep:** every `Applier*` identifier, since the applier is unchanged. - -**Do not rename** these; they are different concepts: `TriggerRuleChange`, -`TriggerAllRuleChange`, `watchPlaneTriggers`, `WatchPlanTriggersTotal`, `crdTriggerGVR`, -`apiServiceTriggerGVR`, `ensureAPISurfaceTriggerInformers`, `selectAPISurfaceTriggers`, -`internal/watch/api_surface_triggers*.go`. - -**Do not collide with** the existing claim identifiers: the branch claim (`BranchClaims`, -`BranchClaimant`, `NewBranchClaims`, `Claims()`), a `CommitRequest` claiming a window, the -read-side `claimed` demand, and OIDC token claims in the audit code. Every new identifier says -`ApplierClaim`, `FluxClaim` or `ArgoCDClaim`, never a bare `Claim`. The one exception is -`ClaimKind()`, a method on `ApplierClaim`, where the receiver already says which claim it is. - -### Docs - -- **[definitions.md "Applying back"](../definitions.md#applying-back):** - - Replace **Apply trigger** with the entry above. - - Keep **Applier**, and change "An apply trigger names it" to "An applier claim names it". - - Update **Eligible**, **Bound**, **In line** and **Held** ("trigger" → "applier claim"). - - Update **Coordinated** to the entry above, using the renamed field and condition vocabulary. - - In **Branch claim**, extend "never a bare 'claim'" to mention that the applier claim is the - cluster side of the same idea. -- **definitions.md "Words we do not use":** - - Add "trigger, apply trigger (for an applier claim) → applier claim". - - Update the "fence" row, which says "what a trigger does to its applier". -- **The configuration heading:** rename [Who applies this folder (`spec.applyTriggerRef`)](../configuration.md#who-applies-this-folder-specapplierclaimref) - to "Who applies this folder (`spec.applierClaimRef`)". That changes the anchor, so fix the 12 - links: 8 in configuration.md, and one each in UPGRADING, api-first-publication, - interpreting-metrics and design/apply-trigger. -- **Guide pages:** about 1,550 lines mention the trigger names. Most are mechanical. Read these by - hand, because they mix "trigger" and "applier" in the same sentences, and a bare "claim" must not - slip in: - - `bi-directional*.md` (about 180 trigger mentions) - - `architecture.md` - - `configuration.md` - - `rbac.md` - - `security-model.md` - - `interpreting-metrics.md` (the `apply_requests_total` row says "names the trigger") -- **`docs/design/*` and `docs/reviews/*`:** leave as the historical record. Add one line at the top - of [apply-trigger.md](../design/apply-trigger.md) saying the kinds are now applier claims. -- **[UPGRADING.md](../UPGRADING.md):** add a short entry, nothing more: - - > **`FluxTrigger` and `ArgoCDTrigger` are now `FluxClaim` and `ArgoCDClaim`.** Breaking, with no - > migration path. Before upgrading, delete every `FluxTrigger` and `ArgoCDTrigger` (a - > `FluxTrigger` hands its `Kustomization` back to Flux), then delete their CRDs. After upgrading, - > recreate them under the new kinds with the same `spec`, and on each `GitTarget` replace - > `spec.applyTriggerRef` with `spec.applierClaimRef`. - - Local e2e clusters: run `task clean-cluster` once after pulling PR B. - -### C4. Redraw the diagrams - -- [config-basics.excalidraw.svg](../images/config-basics.excalidraw.svg) is shown in - [configuration.md:29](../configuration.md) and [quickstart.md:11](../quickstart.md). - - It shows `ClusterProvider` and has no `GitBranch` or `KubeProvider`. - - The SVG embeds its Excalidraw scene, so edit it in place. - - Draw: `WatchRule` → `GitTarget` → `GitBranch` → `GitProvider` → `Secret`; `GitTarget` → - `KubeProvider` (`local`, greyed out as "usually exists"); a dashed optional row `GitTarget` → - `FluxClaim` → `Kustomization`; and `GitReceiver` ⇢ `GitProvider` ("wakes by spec.url"). - - Update the alt text in both docs. -- [overview.excalidraw.svg](../images/overview.excalidraw.svg) (README.md:42) still says - `ClusterProvider`. Change it to `KubeProvider`. -- `config-cluster.excalidraw.svg` isn't referenced anywhere. Delete it. - -## Ownership: one holder per KubeProvider, not per cluster - -[holder.go:36-39](../../internal/applier/holder.go) decides that two triggers name the same applier -by `TriggerKind()`, `KubeProviderKey()`, and the object's namespace and name. Two `KubeProvider`s -that reach **one** cluster (for example the chart's `local`, plus a team's own provider with a -kubeconfig for the same API server) therefore give two holders for one `Kustomization`. Both -suspend it under the same field manager. Each reads the other's resume as a `flux resume` and -suspends it mid-apply. Deleting one hands back a hold the other still needs. These are exactly the -failures the comment at the top of holder.go lists. For an `Application`, both start syncs. - -This is a misconfiguration, not an attack, so it falls outside the "don't design against spoofed -identity" decision. Renaming doesn't change it, though "claim" makes the gap easier to state: an -applier claim is exclusive only among claims made through the same `KubeProvider`. - -- **O1 (PR A): document it.** Add a paragraph to - [Who applies this folder](../configuration.md#who-applies-this-folder-specapplierclaimref) and to - the **In line** entry in definitions.md: *"One holder per `Kustomization` or `Application` is - enforced among triggers that reach it through the same `KubeProvider`. GitOps Reverser can't tell - that two `KubeProvider`s reach the same cluster, so name one `KubeProvider` for every trigger that - reaches a given cluster."* -- **O2 (later): compare cluster identity. This is partial protection.** - - The `KubeProvider` records the cluster's identity in status (the `kube-system` Namespace UID, - read once at validation), and `sameApplier` compares that instead of the provider key. - - Where the credential can't read that Namespace, it falls back to the provider key, and the O1 - limitation still applies. - - Cost: `get` on `namespaces/kube-system`, plus a note at - [architecture.md:389](../architecture.md) ("reads no `Namespace` objects in a source cluster") - that this read is about identity, not authorization. -- **O3 (later, complete): an object-local stamp.** - - Write the holder's identity into the `Kustomization` in the same server-side apply as the - suspend, and refuse to adopt over a live stamp that isn't ours. This also catches two operator - installs on one cluster. - - Cost: two first adopters can race, so arbitration has to work across operators, and an - `Application` needs a write of its own. - -## PR A: doc corrections - -PR A ships before the rename, so its text uses the current names. PR B renames them with -everything else. - -### C1. Qualify "the Kubernetes API is the source of truth" - -[architecture.md:53](../architecture.md). The rule is still right for GitOps Reverser's own -pipeline. What's missing is how Git edits reach the cluster, and that the applier's echoes are kept -out of Git (see **Echo**, [definitions.md:249](../definitions.md#applying-back)). Replace the first -two sentences with: - -> **The Kubernetes API is the source of truth for what GitOps Reverser writes.** Git is a -> materialized mirror of desired state from the API, ingested by **watch**; GitOps Reverser never -> reads Git as object state. An edit made in Git reaches the cluster only through Flux or Argo CD. -> When a `GitTarget` names an apply trigger, those applies are coordinated: GitOps Reverser first -> publishes the API edits it has decided, then asks the applier to apply. The watch observes the -> result, and the applier's own writes during that apply (its echoes) are not mirrored, because -> Git already holds them. - -Keep the existing sentence about push conflicts after it. - -### C2. Describe KubeProvider as a connection and an identity - -Today the docs call it the source cluster, but a trigger uses it to **write** (suspend, resume, -sync) in a cluster that may be a management cluster. - -- [architecture.md:569](../architecture.md): replace "the read-side peer of `GitProvider`" with "the - Kubernetes-side peer of `GitProvider`: a connection to one cluster and the identity used there. - What it's used for depends on who names it: a `GitTarget` reads its source cluster through it, - and an apply trigger reaches its `Kustomization` or `Application` through it, often in a - management cluster." -- [kubeprovider_types.go:196](../../api/v1alpha3/kubeprovider_types.go): change "KubeProvider names - a source cluster: the cluster a GitTarget mirrors from, or an apply trigger reaches its applier - in" to "KubeProvider is a connection to a Kubernetes cluster and the identity used there. A - GitTarget mirrors from it; an apply trigger reaches its applier through it." Then run - `task manifests`, since this godoc flows into the CRD description. -- [definitions.md:46-49](../definitions.md#the-two-clusters-and-the-two-directions), **Source - cluster**: change "A `KubeProvider` names one" to "A `GitTarget` names it through its - `KubeProvider`". Add a **KubeProvider** entry: "A connection and identity, not a role. The same - provider can be a `GitTarget`'s source cluster and an apply trigger's management cluster." - Change "`spec.inCluster` means the control cluster, acting as a source" to "… the control - cluster, reached with the operator's own identity". - -### C3. Remove the obsolete multi-worker text - -Since #428 (`c49803f3`) a branch worker is keyed by repository and branch, and -[branch_owner.go](../../internal/git/branch_owner.go) lets exactly one `GitBranch` write a -repository branch. `apply_sibling_flush.go` is gone. - -- [configuration.md:1470-1471](../configuration.md): delete "The branch workers of other - `GitProvider`s writing the same repository and branch flush too, and the request waits until - they have." -- [bi-directional.md:434](../bi-directional.md): delete the bullet "An edit through another - `GitProvider` on the same repository and branch …". That exposure no longer exists. -- [internal/git/apply_trigger.go:382](../../internal/git/applier_claim.go): delete ", and so does a - sibling's flush" from the comment. - -## Validation and acceptance - -Both PRs run the full sequence: `task fmt` → `task generate` → `task manifests` → `task vet` → -`task lint` → `task test` → `task test-e2e`. Push once lint and unit pass. A PR is done when -e2e passes, in CI or locally (sequentially). - -**PR A is done when:** - -- the full sequence passes; -- `task lint-docs` passes (anchors and links); -- the regenerated KubeProvider CRD description shows the C2 wording. - -**PR B is done when:** - -- the full sequence passes, **plus** `task test-e2e-bi-directional`, because the Flux and Argo CD - specs only run in that corner; -- `git grep -niE 'fluxtrigger|argocdtrigger|apply.?trigger'` returns nothing outside `docs/design`, - `docs/reviews`, `docs/UPGRADING.md`, `CHANGELOG.md` and `external-sources/`; -- `git grep -niw 'trigger'` over `docs/*.md` returns only the unrelated senses (watch plan, API - surface, Argo Events) and the "Words we do not use" row; -- definitions.md contains no entry that calls the applier claim a trigger or an applier, and no - bare "claim" for it. - -## What the follow-up changed - -- **The name moved from Applier to Binding**, to keep applier / apply binding / apply request / - apply operation distinct. The first draft's D1 ("held object" as the generic word) is gone with - it; it also mixed identity with lifecycle state. -- **The argument against "Trigger" is now** that these resources don't declare triggering - conditions, not that a trigger sounds fire-and-forget. -- **C1 no longer says the applied result is mirrored "like any other change".** Echoes are not - mirrored. -- **C4 moved to PR B only** (the first draft had it in both). PR A now lists e2e in its validation. -- **O2 is marked as partial protection.** -- **The migration section is gone.** There are no users yet. The follow-up's points about - sequencing a handoff and about allowing a temporary compatibility mechanism would apply if that - changes. - -## What the naming discussion changed - -2026-10-23. - -- **The name moved from Binding to Claim:** `FluxClaim` / `ArgoCDClaim`, generically "applier - claim". Binding kept the four concepts distinct but hid the authority. Claim keeps them distinct - and shows that the resource exclusively takes an applier and releases it on delete. -- **The field is `spec.applierClaimRef`, not `applyClaimRef`.** "Apply claim" can read as a claim - that something was applied; "applier claim" is a claim *on* the applier, and keeps "applier" for - the external object. -- **The active-name families were rejected.** ApplyControl, ApplyController, ApplyReconciler, - ApplyGovernor, ApplyCoordinator and ApplyScheduler each either collide with operator vocabulary - or describe the wrong mechanism; see [Alternatives considered](#alternatives-considered). -- **The prose rule now guards a bare "claim"** instead of a bare "binding", because definitions.md - already gives a bare "claim" to the read side's demand and defines the branch claim. -- **The reviewer agreed with Claim and qualified three arguments**, all taken: - - Claim does not limit authority to timing by itself; the definition does. Its advantage is the - lifecycle (declare, compete, hold, release) and that it separates intent from success. - - The definition states the scope of exclusivity (applier claims through the same - `KubeProvider`) instead of "one applier claim holds an applier at a time", which overstated O1. - - **Bound** stays distinct from **Held**. The PVC analogy explains the reference structure only. -- **The defining sentence says "declares … should take and retain control"**, so an unreferenced, - conflicting or not-yet-ready applier claim is still described correctly. From e23181213fe2821679ef7e0f04e22af281e3d20a Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sat, 24 Oct 2026 08:05:02 +0000 Subject: [PATCH 2/4] docs(upgrading): describe the net change from 0.53.0, and name shipped versions The entries added since 0.53.0 each narrated one PR's step, including states that never shipped. They are now one ordered upgrade procedure and one entry per concept, written against 0.53.0 with the final names. The released Reapply entry gets its release text back, and the old "Unreleased" headings name the version they shipped in. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/UPGRADING.md | 621 ++++++++++++++++++++++++---------------------- 1 file changed, 324 insertions(+), 297 deletions(-) diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 53d7a0ed..34ce8c64 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,190 +7,60 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. -## Grants say who may use a provider or a branch; `allowedBranches` is `allowedGitBranches` - -**Breaking.** Who may use a `GitProvider`, a `GitBranch` or a `KubeProvider` is a list of grants on -that object, and **nothing is admitted automatically, the object's own namespace included**. An -omitted or empty list admits nobody. - -| Grant list | Admits | Replaces | -|---|---|---| -| `GitProvider.spec.allowedGitBranches` | `GitBranch`es claiming its `branchPatterns` | `spec.allowedBranches`, and `spec.accessFrom` | -| `GitBranch.spec.allowedGitTargets` | `GitTarget`s writing through it | the `GitProvider`'s `spec.accessFrom` | -| `KubeProvider.spec.allowedGitTargets` | `GitTarget`s mirroring its `sourceNamespaces` and `clusterResources` | `spec.accessFrom`, `spec.allowCrossNamespace.sources` | -| `KubeProvider.spec.allowedApplierClaims` | applier claims holding an applier in its `applierNamespaces` | `spec.accessFrom`, `spec.allowCrossNamespace.appliers` | - -Every grant has a required `consumerNamespaces` block with a required `from`: `Same` (the namespace -of the object holding the grant), `Names` with `names`, `Selector` with a non-empty `selector`, or -`All`. Beside the block sits the scope the grant gives. `spec.accessFrom`, -`spec.allowedBranches`, `spec.allowCrossNamespace` and their chart values are gone, and the API -server prunes them. The equivalents of the old defaults: - -```yaml -kind: GitProvider -spec: - allowedGitBranches: # was allowedBranches: [main], for this namespace's GitBranches - - consumerNamespaces: {from: Same} - branchPatterns: [main] ---- -kind: GitBranch -spec: - allowedGitTargets: # this namespace's GitTargets, as before; add from: Names to share - - consumerNamespaces: {from: Same} ---- -kind: KubeProvider -spec: - allowedGitTargets: # was its own namespace, plus accessFrom, without allowCrossNamespace - - consumerNamespaces: {from: Same} - sourceNamespaces: ["{consumer.namespace}"] - allowedApplierClaims: - - consumerNamespaces: {from: Same} - applierNamespaces: ["{consumer.namespace}"] -``` - -- **Every `GitBranch`, `GitTarget` and applier claim needs a grant, also in the provider's own - namespace.** Without one it reports `NamespaceNotAuthorized` and recovers once a grant admits it. -- **`allowedBranches` became `allowedGitBranches`, which also says who may claim.** A - `GitBranch.spec.gitProviderRef` may name a `GitProvider` in another namespace. Branch patterns match - the whole branch name, `*` crosses `/`, and `teams/{consumer.namespace}/*` gives each namespace its - own prefix. A branch outside the patterns is `BranchNotAllowed`. A grant's scope names the object - asking for access, the consumer, with `{consumer.namespace}`; a branch pattern may add - `{consumer.name}` beside it. See [the consumer variables](configuration.md#the-consumer-variables). -- **Who writes a branch is the `GitBranch`'s decision.** A `GitTarget` in another namespace writes - through it only while its `spec.allowedGitTargets` admit that namespace. -- **The chart's `local` provider grants nothing** unless `kubeProvider.allowedGitTargets` and - `kubeProvider.allowedApplierClaims` do, or `quickstart.enabled=true`, which grants exactly the - quickstart namespace. To keep the old open default, every namespace mirroring its own, install with - the chart's `values-playground.yaml`. -- **A `ClusterWatchRule` needs `clusterResources`.** It mirrors only the types the grant admitting - its `GitTarget` names (`clusterroles.rbac.authorization.k8s.io`, a bare `nodes`, or `"*"`), and - one type outside them refuses the whole rule (`ClusterResourceNotAllowed`). No default grants any, - the chart's included, so a `ClusterWatchRule` through `local` stops mirroring until a grant does. -- **A `WatchRule`'s own namespace needs a grant too:** `sourceNamespaces: ["{consumer.namespace}"]`. An - explicit list is the whole scope, the union across the admitting grants, and - `sourceNamespace: "*"` needs `"*"` in at least one of them: it is refused, never narrowed. -- **An applier claim refused by `allowedApplierClaims` hands its applier back**, as one no eligible - `GitTarget` references does. A `GitBranch` no grant admits any more gives up its branch after its - branch worker pushed or refused what it held, and keeps its finalizer. -- `SourceNamespaceAuthorized` is `True` with reason `SourceNamespaceAllowed` for every admitted - rule; `LegacySourceNamespace` is gone. - -Before upgrading, list what each provider admits today, and write the grants that keep it: - -```bash -kubectl get gitproviders,kubeproviders -A -o yaml | grep -E '^ name:|namespace:|accessFrom|allowedBranches|allowCrossNamespace' -``` +## Upgrading from 0.53.0: recreate the connection objects in one pass -See [configuration.md](configuration.md#sharing-connections-across-namespaces) for the full model, -including a platform and its tenants. +**Breaking, with no in-place conversion.** The objects that connect a folder to a cluster, a +repository and an applier are reshaped, and the next five entries say how. `ClusterProvider` is a +namespaced `KubeProvider`, a repository branch is a `GitBranch`, the push receiver is a +`GitReceiver`, `FluxTrigger` and `ArgoCDTrigger` are `FluxClaim` and `ArgoCDClaim`, and grants say +who may use each provider and branch. Every `GitTarget` and applier claim is recreated, because +the references they gain are required and immutable. A `GitTarget`'s folder in Git is untouched: +the new object takes it over on its first snapshot. -## `FluxTrigger` and `ArgoCDTrigger` are now `FluxClaim` and `ArgoCDClaim` - -**Breaking, with no migration path.** Before upgrading, delete every `FluxTrigger` and -`ArgoCDTrigger` (a `FluxTrigger` hands its `Kustomization` back to Flux, an `ArgoCDTrigger` its -`Application`'s automated sync), then delete their CRDs. After upgrading, recreate them under the -new kinds with the same `spec`, and on each `GitTarget` replace `spec.applyTriggerRef` with -`spec.applierClaimRef`. The `GitTarget` condition is `ApplierClaimReady` (reason -`ApplierClaimNotFound`), the reason `HeldByAnotherTrigger` is `HeldByAnotherClaim`, the -finalizers are `configbutler.ai/flux-claim` and `configbutler.ai/argocd-claim`, and a `FluxClaim`'s -`status.source.otherConsumers` is `status.source.alsoAppliedBy`. The entries below -use the old names. See [applier claim](definitions.md#applying-back). - -## An `ArgoCDTrigger` holds its `Application`'s automated sync off - -An `ArgoCDTrigger` no longer only checks that automated sync is off: it switches it off, with a -server-side apply of `spec.syncPolicy.automated.enabled: false` under the field manager -`gitops-reverser`, switches it off again when somebody switches it on, and hands the switch back -when it is deleted or no longer named, as it was before (`status.previousSuspendState`). A trigger -that holds the switch carries the finalizer `configbutler.ai/argocd-trigger`. Argo CD before 3.1, -which has no `enabled`, is not supported: the trigger reports `ArgoCDVersionNotSupported`, is -Stalled and never Ready, and writes nothing. A `FluxTrigger` now records -`status.previousSuspendState` too: a `Kustomization` that was suspended before the trigger held it -stays suspended on hand-back, where it used to be resumed. -Both kinds record a `ClaimedResourceReconciled` Event when they put back what somebody changed of -the applier they hold: automated sync switched on, or a `flux resume`. It replaces the Flux Event -`ResumedElsewhere`. -`AutomatedSyncEnabled` is gone. Automated sync on is refused only where the switch is somebody -else's: `OwnedByApplicationSet` for an `Application` an `ApplicationSet` generated, and -`AutomatedSyncOwnedElsewhere` for a switch another field manager applies. The operator now needs -`patch` on `applications` as well as `get` and `update`: Helm `applierAccess.argocd: true` grants it; -grant it yourself where you applied `config/rbac/argocd-applier-role.yaml` or hold a remote -cluster's role. See [bi-directional GitOps with Argo CD](bi-directional-argocd.md). - -## `GitReceiver` replaces `GitProvider.spec.receiver` - -**Breaking.** The push receiver is a namespaced kind of its own, `GitReceiver`, and nothing names it: -a delivery wakes every `GitProvider`, in any namespace, whose `spec.url` names the repository the -delivery reports. One `GitReceiver` therefore serves every repository its Git host delivers for, so a -GitHub App or an organization webhook, which has one endpoint for many repositories, needs one route. -`GitProvider.spec.receiver` stays in the schema only so that setting it is refused at admission, -with a message naming the replacement. `GitProvider.status.webhookPath` is gone; the path is the -`GitReceiver`'s `status.webhookPath`. - -```yaml -kind: GitReceiver -metadata: {name: platform, namespace: team-a} # the provider's name keeps the webhook's path -spec: - type: github # the old spec.receiver.type - secretRef: {name: platform-receiver} # the old spec.receiver.secretRef, in this namespace -``` - -Migrate in this order: - -1. Record what you have, before upgrading the CRDs: +1. Record what you have, against the running 0.53.0 operator. The new CRDs no longer serve the old + fields. ```bash - kubectl get gitproviders -A -o jsonpath='{range .items[?(@.spec.receiver)]}{.metadata.namespace}/{.metadata.name}{"\t"}{.spec.receiver.type}{"\t"}{.spec.receiver.secretRef.name}{"\t"}{.status.webhookPath}{"\n"}{end}' + kubectl get clusterproviders -o yaml > clusterproviders.yaml + kubectl get gitproviders,gittargets,fluxtriggers,argocdtriggers -A -o yaml > before-upgrade.yaml ``` -2. Upgrade. A `GitProvider` that still carries `spec.receiver` keeps working as a provider, and its - status keeps updating, but any write that changes its `spec` is refused until that same write - removes the field. Nothing is served on its old path. -3. For each, create a `GitReceiver` in the provider's namespace with the same `type` and - `secretRef`, and **give it the provider's name**: the path is the same HMAC of the namespace and - name, keyed by the same token, so the Git host's webhook needs no change. Then remove - `spec.receiver` from the provider. -4. Check `kubectl get gitreceiver`: `Ready` is `True` once its token is readable and the listener is - on. A receiver under another name publishes a new `status.webhookPath`; register that one. After - the next push, the `GitBranch`'s `status.lastPushNoticeAt` shows the delivery reached it. - -**A delivery is routed by the repository it names.** A route used to name one provider, so the -receiver read only the branch. Now the repository URLs in the payload (`repository.clone_url`, -`ssh_url` and `html_url` from GitHub and Gitea, `project.git_http_url` and `git_ssh_url` from GitLab) -are compared with every provider's `spec.url` as a branch claim compares repositories. A provider that -reaches the host under another name, such as an SSH alias (`ssh.github.com:443`), is not matched, and -its foreign pushes are found by the next rejected push or the periodic refresh instead. - -**Any namespace's `GitReceiver` can wake any matching provider.** Waking only makes a branch worker -read the remote with its own provider's credential, so a delivery cannot choose what is read, and a -worker holds at most one pending look however many deliveries arrive. That bounds the queue, not the -rate: a token holder who keeps delivering can keep a matching worker reading its remote -continuously. That availability cost is accepted; there is no per-worker cooldown. - -**A `generic-hmac` body must name its repository.** One without `repository` is refused with `400`, -because a receiver cannot tell which provider it is for. - -**A verified delivery is answered `202` with no body.** The `{"matched": N}` body is gone: any -namespace can hold a `GitReceiver`, and a count would tell its token holder whether another namespace -tracks a repository. A relay that read it reads the metric below or `lastPushNoticeAt` instead. - -**`ListenerDisabled`.** A `GitReceiver` reports `Ready=False` with this reason while the operator runs -without `--receiver-bind-address` (chart value `servers.receiver.enabled=false`). - -**The delivery metric is per receiver.** `gitopsreverser_receiver_deliveries_total` gains the -`receiver_namespace` and `receiver_name` labels, and its `unmatched` outcome is split into -`unknown_repository` (no `GitProvider` has the repository the delivery names) and `unknown_branch` -(one has, but no branch worker writes that branch). A query that read `outcome="unmatched"` reads -both. +2. Delete every `FluxTrigger` and `ArgoCDTrigger` while 0.53.0 still runs, so it removes the + finalizer and hands each `Kustomization` back to Flux. Then delete their CRDs: + `kubectl delete crd fluxtriggers.configbutler.ai argocdtriggers.configbutler.ai`. Until a claim + takes over in step 6, Flux and Argo CD apply on their own. +3. Upgrade the CRDs, the operator and its permissions. The chart's `clusterProvider.*` values are + `kubeProvider.*`, and the provider it renders is a `KubeProvider` named `local` in the release + namespace, which admits only the release namespace's own objects until you set its grants (see + the grants entry). +4. Create the new connection objects: + - a `KubeProvider` for each other `ClusterProvider`, in the namespace that should own the + credential, with its kubeconfig Secret copied into that namespace; + - for each `GitProvider`, in one write: remove `spec.receiver` and replace `spec.allowedBranches` + with `spec.allowedGitBranches`. For a provider that had a receiver, create a `GitReceiver` in + its namespace **with the provider's name**, so the Git host's webhook path stays the same; + - a `GitBranch` for each repository branch your `GitTarget`s wrote, from their `gitProviderRef`, + `branch` and `parentBranch`. +5. Delete each `GitTarget` and create it again with `spec.gitBranchRef` and `spec.kubeProviderRef`, + and `spec.applierClaimRef` where it had `spec.applyTriggerRef`. +6. Create a `FluxClaim` or `ArgoCDClaim` for each trigger you deleted: the same `spec`, with + `spec.kubeProviderRef` for `spec.clusterProviderRef` and without `spec.quietPeriod`. It takes + hold of its applier again once a `GitTarget` names it. +7. Point each API server's audit webhook at its provider's new route, or set + `spec.attribution.auditRoute` to the route it posts under today. +8. Delete the old CRD, which Helm leaves installed: `kubectl delete crd clusterproviders.configbutler.ai`. + +Check the result with `kubectl get kubeproviders,gitbranches,gitreceivers,gittargets,fluxclaims,argocdclaims -A`. +`NamespaceNotAuthorized` anywhere means a grant is missing. ## `KubeProvider` replaces `ClusterProvider`; every reference names one with `spec.kubeProviderRef` -**Breaking.** The source cluster is a namespaced `KubeProvider`. The cluster-scoped -`ClusterProvider` kind is gone, and nothing converts one: a CRD cannot change its scope. `GitTarget`, -`FluxTrigger` and `ArgoCDTrigger` name their provider with the required, immutable -`spec.kubeProviderRef`, whose omitted namespace means the referrer's own. There is no provider named -`default` that an omitted reference falls back to. `spec.clusterProviderRef` stays in the schema only -so that setting it is refused at admission, with a message naming `spec.kubeProviderRef`. +The source cluster is a namespaced `KubeProvider`. The cluster-scoped `ClusterProvider` kind is +gone, and nothing converts one: a CRD cannot change its scope. `GitTarget`, `FluxClaim` and +`ArgoCDClaim` name their provider with the required, immutable `spec.kubeProviderRef`, whose +omitted namespace means the referrer's own. There is no provider named `default` that an omitted +reference falls back to. `spec.clusterProviderRef` stays in the schema only so that setting it is +refused at admission, with a message naming `spec.kubeProviderRef`. ```yaml kind: KubeProvider @@ -212,28 +82,10 @@ spec: path: apps ``` -Migrate in this order: - -1. Record what you have, before upgrading the CRDs: - - ```bash - kubectl get clusterproviders -o yaml > clusterproviders.yaml - kubectl get gittargets,fluxtriggers,argocdtriggers -A -o jsonpath='{range .items[*]}{.kind}{"\t"}{.metadata.namespace}/{.metadata.name}{"\t"}{.spec.clusterProviderRef.name}{"\n"}{end}' - ``` - -2. Upgrade. The chart renders a `KubeProvider` named `local` in the release namespace, using the - operator's own identity (see below). Its values are `kubeProvider.create`, `kubeProvider.name`, - `kubeProvider.secretRef`, `kubeProvider.allowedGitTargets` and `kubeProvider.allowedApplierClaims`; - the `clusterProvider.*` values are gone. -3. Create a `KubeProvider` for each other `ClusterProvider`, in the namespace that should own the - credential. **The kubeconfig Secret is read from the provider's own namespace**, not from the - operator's, so move or copy it there. `spec.kubeConfig.secretRef` is `spec.secretRef`. -4. Delete and recreate each `GitTarget`, `FluxTrigger` and `ArgoCDTrigger` with - `spec.kubeProviderRef`. The reference is immutable and the old field is refused, so an edit cannot - do it. A `GitTarget`'s folder in Git is untouched: the new object takes it over on its first - snapshot. A `FluxTrigger` hands its `Kustomization` back to Flux when it is deleted, and the new - one takes hold of it again. -5. Delete the old CRD, which Helm leaves installed: `kubectl delete crd clusterproviders.configbutler.ai`. +**The kubeconfig Secret is read from the provider's own namespace**, where 0.53.0 read the +operator's. `spec.kubeConfig.secretRef` is `spec.secretRef`. The chart's values are `kubeProvider.create`, +`kubeProvider.name`, `kubeProvider.secretRef`, `kubeProvider.allowedGitTargets` and +`kubeProvider.allowedApplierClaims`. **The operator's own identity is an explicit arm, and the installation must allow it.** A provider that omitted `kubeConfig` used the operator's identity. A `KubeProvider` sets exactly one of @@ -243,9 +95,9 @@ and `controllerManager.inClusterKubeProviders` adds others. Any other provider t `spec.inCluster` reports `Validated=False` and `Stalled=True` (`InClusterNotAuthorized`) and is never used. `spec.client` is refused beside `spec.inCluster`, where it was ignored. -**`accessFrom` and `allowCrossNamespace` are replaced by grants.** A `KubeProvider` admits nobody, -its own namespace included, until a grant in `spec.allowedGitTargets` or `spec.allowedApplierClaims` -does; see "Grants say who may use a provider or a branch". +**`accessFrom` and `allowCrossNamespace` are replaced by grants.** A `KubeProvider` admits nobody +until a grant in `spec.allowedGitTargets` or `spec.allowedApplierClaims` does; see "Grants say who +may use a provider or a branch". **Audit routes default to `.`, so the audit webhook URL changes.** The chart's provider posts under `/audit-webhook/.local`, `/audit-webhook/gitops-reverser.local` @@ -265,15 +117,13 @@ name, so two providers with one name in two namespaces are two series. ## `GitBranch` holds a repository branch; a `GitTarget` names one with `spec.gitBranchRef` -**Breaking.** A repository branch is its own object, `GitBranch`, claiming the branch through the -`GitProvider` whose credential writes it. A `GitTarget` names one with the required, immutable +A repository branch is its own object, `GitBranch`, claiming the branch through the `GitProvider` +whose credential writes it. A `GitTarget` names one with the required, immutable `spec.gitBranchRef`, and `spec.gitProviderRef`, `spec.branch` and `spec.parentBranch` are gone from `GitTarget`: setting one is refused at admission, with a message naming `spec.gitBranchRef`. `GitTarget.status.remote` is now `GitBranch.status.remote`, and `GitProvider.status.branches` is gone; `kubectl get gitbranch -A` answers what it did. -Replace each GitTarget's three fields with a `GitBranch` and a reference to it: - ```yaml kind: GitBranch metadata: {name: platform-main, namespace: platform} @@ -288,26 +138,16 @@ kind: GitTarget metadata: {name: apps, namespace: platform} spec: gitBranchRef: {name: platform-main} + kubeProviderRef: {name: local, namespace: gitops-reverser} path: apps ``` -`spec.gitBranchRef` is immutable and the old fields are refused, so delete each `GitTarget` and -create it again with the new shape. Its folder in Git is untouched: the new object takes it over -on its first snapshot. List the GitTargets to recreate, with what each `GitBranch` needs: - -```bash -kubectl get gittargets -A -o jsonpath='{range .items[*]}{.metadata.namespace}/{.metadata.name}{"\t"}{.spec.gitProviderRef.name}{"\t"}{.spec.branch}{"\t"}{.spec.parentBranch}{"\n"}{end}' -``` - -Run it before upgrading the CRDs: the new schema no longer serves the old fields. - **One `GitBranch` holds a repository branch.** It holds it once a grant on its `GitProvider` admits it and the provider's credential opens a push session on the remote; the oldest such `GitBranch` holds it, keeps it through a later credential failure, and every other `GitBranch` for the same repository branch (compared as URLs, so an `https://` and an SSH spelling of one repository are one) -reports `Stalled=True` -(`BranchConflict`) and writes nothing. Create one `GitBranch` per repository branch, and point every -`GitTarget` on that branch at it. +reports `Stalled=True` (`BranchConflict`) and writes nothing. Create one `GitBranch` per repository +branch, and point every `GitTarget` on that branch at it. **The push session is a new check, and it needs a credential that may push.** A `GitProvider` with no `secretRef` on a public repository used to be enough to write; its `GitBranch` now reports @@ -317,21 +157,21 @@ repository, because their AGit flow accepts pushes from readers and checks a bra arrives. On a host that offers push sessions anonymously it proves only that the repository is reachable. -**Sharing a repository branch across namespaces.** A `GitTarget` names a `GitBranch` in another -namespace with `spec.gitBranchRef.namespace`; that `GitBranch`'s `spec.allowedGitTargets` must admit -the `GitTarget`'s namespace, or the target reports `Validated=False` (`NamespaceNotAuthorized`). The -tenant namespace holds no copy of the credential. +**A repository branch can be shared across namespaces.** A `GitBranch` may name a `GitProvider` in +another namespace with `spec.gitProviderRef.namespace`, and a `GitTarget` may name a `GitBranch` in +another namespace with `spec.gitBranchRef.namespace`, each as far as the grants on the named object +admit it. The tenant namespace holds no copy of the credential. New reasons follow from it. A `GitTarget` whose `GitBranch` does not exist reports -`Validated=False` (`GitBranchNotFound`); one whose `GitBranch` does not hold its branch, or is not -`Ready`, reports `GitBranchReady=False` (`GitBranchNotReady`) and waits. That condition replaces -`GitProviderReady`. `NamespaceNotAuthorized`, `BranchNotAllowed`, `ParentBranchNotFound` and -`RecoveringParentBranch` are now reported by the `GitBranch`, and `GitTarget`s on it show them in the `GitBranchReady` message. -`TargetConflict` now means only overlapping paths on one `GitBranch`. A `WatchRule` or -`ClusterWatchRule` no longer reads the GitProvider and no longer reports `GitProviderNotFound`; it -follows its `GitTarget`'s readiness. The chart's quick start renders a `GitBranch` -(`quickstart.gitBranch.name`, `quickstart.gitBranch.branch`), and `quickstart.gitTarget.branch` is -gone. +`Validated=False` (`GitBranchNotFound`, which replaces `ProviderNotFound`); one whose `GitBranch` does +not hold its branch, or is not `Ready`, reports `GitBranchReady=False` (`GitBranchNotReady`) and +waits. That condition replaces `GitProviderReady`. `BranchNotAllowed`, `ParentBranchNotFound` and +`RecoveringParentBranch` are now reported by the `GitBranch`, and `GitTarget`s on it show them in +the `GitBranchReady` message. `TargetConflict` now means only overlapping paths on one `GitBranch`. +A `WatchRule` or `ClusterWatchRule` no longer reads the GitProvider and no longer reports +`GitProviderNotFound`; it follows its `GitTarget`'s readiness. The chart's quick start renders a +`GitBranch` (`quickstart.gitBranch.name`, `quickstart.gitBranch.branch`), and +`quickstart.gitTarget.branch` is gone. Deleting a `GitBranch` stops its branch worker after it has pushed or refused every pending write and settled every `CommitRequest` it held. It never deletes the branch on the remote. @@ -339,14 +179,220 @@ settled every `CommitRequest` it held. It never deletes the branch on the remote The metrics keep their `provider_namespace` and `provider_name` labels, which name the `GitProvider` of the `GitBranch` holding the branch. Log lines name a branch as `repository@branch`. -## `FluxTrigger` `spec.quietPeriod` and `status.source.prefetch` are removed +## Grants say who may use a provider or a branch; `allowedBranches` is `allowedGitBranches` + +Who may use a `GitProvider`, a `GitBranch` or a `KubeProvider` is a list of grants on that object. +**On a `GitBranch` and a `KubeProvider`, an omitted list is one grant for the object's own +namespace**, filled in by the API server; **a written list is the whole policy**, with nothing +added to it, and `[]` admits nobody. A `GitProvider`'s list has no default, because which branches +may be claimed is always written: omitted or empty, it admits no `GitBranch`. -**Breaking.** An apply no longer waits for a quiet branch: the branch worker publishes what is -pending, then applies, so `spec.quietPeriod` has nothing left to set. The source prefetch is gone -with `status.source.prefetch`. Server-side apply and `kubectl`'s strict field validation reject a -`FluxTrigger` manifest that still sets `spec.quietPeriod`; a client with field validation off drops -the field without an error. Remove `spec.quietPeriod` from your `FluxTrigger` manifests before -upgrading. Nothing replaces either field. +| Grant list | Admits | Replaces | +|---|---|---| +| `GitProvider.spec.allowedGitBranches` | `GitBranch`es claiming its `branchPatterns` | `spec.allowedBranches` | +| `GitBranch.spec.allowedGitTargets` | `GitTarget`s writing through it | nothing: a `GitTarget` named a `GitProvider` in its own namespace | +| `KubeProvider.spec.allowedGitTargets` | `GitTarget`s mirroring its `sourceNamespaces` and `clusterResources` | `ClusterProvider.spec.accessFrom` and `spec.allowCrossNamespace.sources` | +| `KubeProvider.spec.allowedApplierClaims` | applier claims holding an applier in its `applierNamespaces` | `ClusterProvider.spec.accessFrom` and `spec.allowCrossNamespace.appliers` | + +Every grant has a required `consumerNamespaces` block with a required `from`: `Same` (the namespace +of the object holding the grant), `Names` with `names`, `Selector` with a non-empty `selector`, or +`All`. Beside the block sits the scope the grant gives. A grant's scope names the object asking for +access, the consumer, with `{consumer.namespace}`; a branch pattern may add `{consumer.name}` +beside it. See [the consumer variables](configuration.md#the-consumer-variables). +`spec.allowedBranches`, `spec.accessFrom`, `spec.allowCrossNamespace` and their chart values are +gone, and the API server prunes them. + +What 0.53.0 allowed, written as grants: a `GitTarget` wrote any `allowedBranches` branch of a +`GitProvider` in its own namespace, and the chart's `default` provider admitted every namespace +(`accessFrom.selector: {}`), each reaching only its own namespace. + +```yaml +kind: GitProvider +spec: + allowedGitBranches: # was allowedBranches: [main]; required, no default + - consumerNamespaces: {from: Same} + branchPatterns: [main] +--- +kind: GitBranch +spec: {} # allowedGitTargets omitted: this namespace's GitTargets, as before +--- +kind: KubeProvider +spec: + allowedGitTargets: # was accessFrom: {selector: {}} without allowCrossNamespace; + - consumerNamespaces: {from: All} # omitted, only this namespace's GitTargets + sourceNamespaces: ["{consumer.namespace}"] + allowedApplierClaims: + - consumerNamespaces: {from: All} + applierNamespaces: ["{consumer.namespace}"] +``` + +The omitted defaults are, written out: + +```yaml +kind: GitBranch +spec: + allowedGitTargets: + - consumerNamespaces: {from: Same} +--- +kind: KubeProvider +spec: + allowedGitTargets: # no clusterResources + - consumerNamespaces: {from: Same} + sourceNamespaces: ["{consumer.namespace}"] + allowedApplierClaims: + - consumerNamespaces: {from: Same} + applierNamespaces: ["{consumer.namespace}"] +``` + +- **Branch patterns match the whole branch name**, `*` crosses `/`, and + `teams/{consumer.namespace}/*` gives each namespace its own prefix. A branch outside the patterns + is `BranchNotAllowed`. +- **Who writes a branch is the `GitBranch`'s decision.** A `GitTarget` in another namespace writes + through it only while its `spec.allowedGitTargets` admit that namespace; otherwise the target + reports `Validated=False` (`NamespaceNotAuthorized`). +- **The chart's `local` provider admits only the release namespace** unless + `kubeProvider.allowedGitTargets` and `kubeProvider.allowedApplierClaims` say otherwise; `[]` there + admits nobody. `quickstart.enabled=true` replaces the default with a grant for exactly the + quickstart namespace. To keep 0.53.0's open default, every namespace mirroring its own, install + with the chart's `values-playground.yaml`, which sets the `KubeProvider` grants above. +- **The default trusts the object's own namespace.** Whoever may create a `GitTarget` or an applier + claim beside a `KubeProvider` may use its credential within the default scope. Keep a provider in + a namespace of its own, or write `[]`, where administering it and using it are separate roles. +- **A `ClusterWatchRule` needs `clusterResources`.** It mirrors only the types the grant admitting + its `GitTarget` names (`clusterroles.rbac.authorization.k8s.io`, a bare `nodes`, or `"*"`), and + one type outside them refuses the whole rule (`ClusterResourceNotAllowed`). No default grants any, + the chart's included, so a `ClusterWatchRule` through `local` stops mirroring until a grant does. +- **A `WatchRule`'s own namespace is in scope through a grant**, the default one included: + `sourceNamespaces: ["{consumer.namespace}"]`. An explicit list is the whole scope, the union + across the admitting grants, and + `sourceNamespace: "*"` needs `"*"` in at least one of them: it is refused, never narrowed. +- **An applier claim refused by `allowedApplierClaims` hands its applier back** (`InControl=False`, + `Revoking`), as one no eligible `GitTarget` references does. A `GitBranch` no grant admits any more + gives up its branch after its branch worker pushed or refused what it held, and keeps its + finalizer. +- `SourceNamespaceAuthorized` is `True` with reason `SourceNamespaceAllowed` for every admitted + rule; `LegacySourceNamespace` is gone. + +See [configuration.md](configuration.md#sharing-connections-across-namespaces) for the full model, +including a platform and its tenants. + +## `FluxTrigger` and `ArgoCDTrigger` are `FluxClaim` and `ArgoCDClaim` + +The kinds are renamed, with every name that carried them. Nothing converts the old objects; step 2 +and step 6 of the upgrade above delete and recreate them. + +| 0.53.0 | Now | +|---|---| +| `FluxTrigger`, `ArgoCDTrigger` | `FluxClaim`, `ArgoCDClaim` | +| `GitTarget.spec.applyTriggerRef` | `GitTarget.spec.applierClaimRef` | +| `spec.clusterProviderRef` | `spec.kubeProviderRef` (see the `KubeProvider` entry) | +| `GitTarget` condition `ApplyTriggerReady`, reason `ApplyTriggerNotFound` | `ApplierClaimReady`, `ApplierClaimNotFound` | +| condition `Held`, printer column `Held` | condition `InControl`, printer column `Control` | +| reason `HeldByAnotherTrigger` | `HeldByAnotherClaim` | +| finalizer `configbutler.ai/flux-trigger` | `configbutler.ai/flux-claim`; an `ArgoCDClaim` holding its switch carries `configbutler.ai/argocd-claim` | +| `FluxTrigger.status.source.otherConsumers` | `FluxClaim.status.source.alsoAppliedBy` | +| `FluxTrigger.spec.quietPeriod`, `status.source.prefetch` | gone, with nothing in their place | + +Server-side apply and `kubectl`'s strict field validation reject a `FluxClaim` manifest that sets +`spec.quietPeriod`. See [applier claim](definitions.md#applying-back). + +## An applier claim applies once the branch is published, and an `ArgoCDClaim` holds automated sync off + +**An apply waits for publishing, not for a quiet branch.** The branch worker publishes what is +pending on the branch, then applies; no apply goes out before every initial snapshot of the +`GitTarget`s naming the claim is published. A claim asks for an apply when somebody else pushes to +the branch, after a refusal or a reverted edit, to adopt an object created through the API in the +folder (Flux prunes only what it applied, so such an object would otherwise survive the deletion of +its file in Git), and to converge, once when the operator starts and once when a `GitTarget` first +names the claim. An API edit asks for no apply. A `GitTarget` that names a claim has its folder +created before it is applied: a resync into a folder the branch does not hold yet commits the +folder's bootstrap files (`README.md`, `.gittargetignore`, and `.sops.yaml` with encryption) even +when there is nothing to mirror. An `Application` is asked to sync for the same reasons as a +`Kustomization`, so its hooks also run after an API create in the folder. + +**Naming a claim narrows what the folder mirrors.** An object another applier owns, one whose Flux +owner labels (`kustomize.toolkit.fluxcd.io/name` and `kustomize.toolkit.fluxcd.io/namespace`) or Argo +CD tracking annotation (`argocd.argoproj.io/tracking-id`) name an applier other than the claim's, is +no longer mirrored into that folder, and a resync leaves its document in Git alone. Mirrored, it +would be applied by two appliers, which fight each other. After naming a claim, read +`gitopsreverser_apply_ownership_conflicts_total` and the log line `Not mirroring an object another +applier owns`: each object they report was mirrored before, and belongs to one applier only. See +[objects another applier owns](bi-directional.md#objects-another-applier-owns). + +**An `ArgoCDClaim` switches automated sync off itself**, where an `ArgoCDTrigger` only checked that +you had: a server-side apply of `spec.syncPolicy.automated.enabled: false` under the field manager +`gitops-reverser`, applied again when somebody switches it on, and handed back as it was before +(`status.previousSuspendState`) when the claim is deleted or no longer named. `InControl` is `True` +with reason `AutomatedSyncOff`. Argo CD before 3.1, which has no `enabled`, is not supported: the +claim reports `ArgoCDVersionNotSupported`, is Stalled and never Ready, and writes nothing. +`AutomatedSyncEnabled` is gone. Automated sync on is refused only where the switch is somebody +else's: `OwnedByApplicationSet` for an `Application` an `ApplicationSet` generated, and +`AutomatedSyncOwnedElsewhere` for a switch another field manager applies. The operator now needs +`patch` on `applications` as well as `get` and `update`: Helm `applierAccess.argocd: true` grants it; +grant it yourself where you applied `config/rbac/argocd-applier-role.yaml` or hold a remote +cluster's role. See [bi-directional GitOps with Argo CD](bi-directional-argocd.md). + +**A `FluxClaim` records `status.previousSuspendState` too:** a `Kustomization` that was suspended +before the claim held it stays suspended on hand-back, where a `FluxTrigger` resumed it. Both kinds +record a `ClaimedResourceReconciled` Event when they put back what somebody changed of the applier +they hold: automated sync switched on, or a `flux resume`. It replaces the Flux Event +`ResumedElsewhere`. + +## `GitReceiver` replaces `GitProvider.spec.receiver` + +The push receiver is a namespaced kind of its own, `GitReceiver`, and nothing names it: a delivery +wakes every `GitProvider`, in any namespace, whose `spec.url` names the repository the delivery +reports. One `GitReceiver` therefore serves every repository its Git host delivers for, so a GitHub +App or an organization webhook, which has one endpoint for many repositories, needs one route. +`GitProvider.status.webhookPath` is gone; the path is the `GitReceiver`'s `status.webhookPath`. + +```yaml +kind: GitReceiver +metadata: {name: platform, namespace: team-a} # the provider's name keeps the webhook's path +spec: + type: github # the old spec.receiver.type + secretRef: {name: platform-receiver} # the old spec.receiver.secretRef, in this namespace +``` + +**Give the `GitReceiver` the provider's name** and create it in the provider's namespace: the path +is the same HMAC of the namespace and name, keyed by the same token, so the Git host's webhook needs +no change. A receiver under another name publishes a new `status.webhookPath`; register that one. +`kubectl get gitreceiver` shows `Ready=True` once its token is readable and the listener is on, and +after the next push the `GitBranch`'s `status.lastPushNoticeAt` shows the delivery reached it. + +`GitProvider.spec.receiver` stays in the schema only so that setting it is refused at admission, +with a message naming the replacement. A `GitProvider` that still carries it keeps working as a +provider, and its status keeps updating, but any write that changes its `spec` is refused until that +same write removes the field. Nothing is served on its old path. + +**A delivery is routed by the repository it names.** The repository URLs in the payload +(`repository.clone_url`, `ssh_url` and `html_url` from GitHub and Gitea, `project.git_http_url` and +`git_ssh_url` from GitLab) are compared with every provider's `spec.url` as a branch claim compares +repositories. A provider that reaches the host under another name, such as an SSH alias +(`ssh.github.com:443`), is not matched, and its foreign pushes are found by the next rejected push or +the periodic refresh instead. + +**Any namespace's `GitReceiver` can wake any matching provider.** Waking only makes a branch worker +read the remote with its own provider's credential, so a delivery cannot choose what is read, and a +worker holds at most one pending look however many deliveries arrive. That bounds the queue, not the +rate: a token holder who keeps delivering can keep a matching worker reading its remote +continuously. That availability cost is accepted; there is no per-worker cooldown. + +**A `generic-hmac` body must name its repository.** One without `repository` is refused with `400`, +because a receiver cannot tell which provider it is for. + +**A verified delivery is answered `202` with no body.** The `{"matched": N}` body is gone: any +namespace can hold a `GitReceiver`, and a count would tell its token holder whether another namespace +tracks a repository. A relay that read it reads the metric below or `lastPushNoticeAt` instead. + +**`ListenerDisabled`.** A `GitReceiver` reports `Ready=False` with this reason while the operator runs +without `--receiver-bind-address` (chart value `servers.receiver.enabled=false`). + +**The delivery metric is per receiver.** `gitopsreverser_receiver_deliveries_total` gains the +`receiver_namespace` and `receiver_name` labels, and its `unmatched` outcome is split into +`unknown_repository` (no `GitProvider` has the repository the delivery names) and `unknown_branch` +(one has, but no branch worker writes that branch). A query that read `outcome="unmatched"` reads +both. ## `allowAnySourceNamespace` is gone; `allowCrossNamespace.sources` replaces it @@ -374,10 +420,10 @@ default stored on every provider, is accepted. **Breaking.** `GitTarget.spec.onRefusal` accepts `Ignore` and `Reapply`. `PushEmptyCommit` is gone: GitOps Reverser no longer moves the branch with an empty commit when an edit is refused. With -`Reapply` it asks the target's applier to apply the current tip, after closing the open commit -window and publishing the writes pending on the branch. The applier is the Flux `Kustomization` a -`FluxTrigger` holds, or the Argo CD `Application` an `ArgoCDTrigger` holds, and the `GitTarget` -names the trigger in the new `spec.applyTriggerRef`. Only that applier applies, rather than every applier watching the +`Reapply` it asks the target's applier to apply the current tip once the branch holds no open +commit window and no pending writes. The applier is the Flux `Kustomization` a `FluxTrigger` holds, +or the Argo CD `Application` an `ArgoCDTrigger` holds, and the `GitTarget` names the trigger in the +new `spec.applyTriggerRef`. Only that applier applies, rather than every applier watching the branch; a `GitRepository` that has to fetch the tip wakes only the consumers no trigger holds. An allowed edit still in a commit window is published before the apply instead of being reverted with the refused one. The API rejects `Reapply` without `spec.applyTriggerRef`. @@ -402,42 +448,25 @@ See [who applies this folder](configuration.md#who-applies-this-folder-specappli A trigger also changes what else applies the folder. A `FluxTrigger` holds its `Kustomization` suspended, so Flux applies it only when GitOps Reverser resumes it: when somebody else pushes to the -branch, after a refusal or a reverted edit, to adopt an object created through the API in the folder -(Flux prunes only what it applied, so such an object would otherwise survive the deletion of its -file in Git), and to converge, once when the operator starts and once when a `GitTarget` first names -the trigger. An API edit asks for no apply. Each apply goes out once the writes pending on the -branch are published, and none before every initial snapshot of the `GitTarget`s naming the trigger -is published. A `GitTarget` that names a trigger has its folder created before it is applied: a -resync into a folder the branch does not hold yet commits the folder's bootstrap files (`README.md`, -`.gittargetignore`, and `.sops.yaml` with encryption) even when there is nothing to mirror. -`flux get ks` shows it `Suspended`, and while the operator is down it waits suspended: a graceful -stop suspends one it had resumed, and after a crash the next process does. Remove -`spec.suspend` from whatever applies the `Kustomization`'s manifest (the trigger reports -`SuspendOwnedElsewhere` otherwise) and any Git-host webhook into a Flux `Receiver` for the -repository, as [the Flux guide](bi-directional-flux.md) describes. A trigger acts only while an -eligible `GitTarget` names it, so create the two in any order: until one names it, it is `Ready` -(`NotReferenced`) and holds nothing. Deleting the trigger, or the last `GitTarget` naming it, hands -the `Kustomization` back to Flux. The operator needs `get`, `list` and `patch` on `kustomizations`, -`list` on `helmreleases` and `helmcharts`, `get` and `patch` on `gitrepositories`, and `create` on -`selfsubjectaccessreviews`, in the trigger's cluster: Helm `applierAccess.flux: true` grants them -for the operator's own cluster. - -For an Argo CD `Application`, GitOps Reverser starts every sync, so its trigger holds its automated -sync off ([since](#an-argocdtrigger-holds-its-applications-automated-sync-off); before that, you -switched it off yourself, and automated sync on was refused). An `Application` is asked to sync -for the same reasons as a `Kustomization`, adopt and -converge included, so its hooks also run after an API create in the folder and when a `GitTarget` -first names the trigger. The operator needs `get`, `update` and `patch` on `applications`: Helm -`applierAccess.argocd: true`. See [bi-directional GitOps with Argo CD](bi-directional-argocd.md). - -Naming a trigger also narrows what the folder mirrors. An object another applier owns, one whose -Flux owner labels (`kustomize.toolkit.fluxcd.io/name` and `kustomize.toolkit.fluxcd.io/namespace`) -or Argo CD tracking annotation (`argocd.argoproj.io/tracking-id`) name an applier other than the -trigger's, is no longer mirrored into that folder, and a resync leaves its document in Git alone. -Mirrored, it would be applied by two appliers, which fight each other. After naming a trigger, read -`gitopsreverser_apply_ownership_conflicts_total` and the log line `Not mirroring an object another -applier owns`: each object they report was mirrored before, and belongs to one applier only. See -[objects another applier owns](bi-directional.md#objects-another-applier-owns). +branch, after a refusal or a reverted edit, and to catch up with its own pushes, or with the branch +once a `GitTarget` first names the trigger, after the branch has been quiet for the trigger's +`quietPeriod`. `flux get ks` shows it `Suspended`, and while the +operator is down it waits suspended: a graceful stop suspends one it had resumed, and after a crash +the next process does. Remove `spec.suspend` from whatever applies the +`Kustomization`'s manifest (the trigger reports `SuspendOwnedElsewhere` otherwise) and any Git-host +webhook into a Flux `Receiver` for the repository, as [the Flux guide](bi-directional-flux.md) +describes. A trigger acts only while an eligible `GitTarget` names it, so create the two in any +order: until one names it, it is `Ready` (`NotReferenced`) and holds nothing. Deleting the trigger, +or the last `GitTarget` naming it, hands the `Kustomization` back to Flux. The operator needs `get`, +`list` and `patch` on `kustomizations`, `list` on `helmreleases` and `helmcharts`, `get` and `patch` +on `gitrepositories`, and `create` on `selfsubjectaccessreviews`, in the trigger's cluster: Helm +`applierAccess.flux: true` grants them for the operator's own cluster. + +For an Argo CD `Application`, GitOps Reverser starts every sync, so switch its automated sync off +(`spec.syncPolicy.automated.enabled: false`, keeping `prune`) before its trigger holds it; one with +automated sync on reports `Configured=False` (`AutomatedSyncEnabled`) on the trigger and is never +synced. The operator needs `get` and `update` on `applications`: Helm `applierAccess.argocd: true`. +See [bi-directional GitOps with Argo CD](bi-directional-argocd.md). ## `git_queue_drops_total` is now `git_queue_refusals_total` @@ -2421,7 +2450,7 @@ is the acceptance gate's own issue" compile-checked rather than merely stated. - Record the `status.generator.version` you consumed a report with, if you pin our release elsewhere. That is the whole point of the field. -## Unreleased: status vocabulary and two removed status fields (next minor; status-only change) +## 0.39.2 — status vocabulary and two removed status fields (status-only change) Nothing in `spec` changed and no manifest you wrote needs editing. Three things a **reader of status** may notice: @@ -2466,7 +2495,7 @@ columns (`Ready`, `Reason`, `Streams`, `Age`); `Provider`, `Branch` and `Path` m visible in `kubectl describe` and routable by anything that consumes Events. The operator's ServiceAccount gained `create`/`patch` on `events` for it, which `helm upgrade` applies. -## Unreleased — a resync no longer deletes Git documents by default (next minor; behavior change) +## 0.39.0 — a resync no longer deletes Git documents by default (behavior change) **`GitTarget` gained `spec.prune.mode`, and its effective default changes what a resync does.** Previously every resync mark-and-swept: a managed document whose resource was absent from the @@ -2522,7 +2551,7 @@ This ships in the **same release** as the rule-kind scope change below, and is w migration non-destructive: a converted `WatchRule` that resolves to a narrower set of namespaces than you intended leaves the affected documents in Git instead of deleting them. -## Unreleased — scope is now carried by the rule kind (next minor; breaking) +## 0.39.0 — scope is now carried by the rule kind (breaking) **Scope moved from a per-rule field onto the rule KIND.** `WatchRule` is the namespaced surface and gained `spec.rules[].sourceNamespace`; `ClusterWatchRule` is cluster-scoped only and lost its scope @@ -2622,7 +2651,7 @@ The same skew exists inside a rolling upgrade: CRDs are cluster-wide, so an old new `WatchRule`, ignore `rules[].sourceNamespace`, and mirror the wrong namespace's content into Git. **Complete the controller rollout before applying migrated manifests.** -## Unreleased — unresolved attribution is visible in Git (next minor; author identity changes) +## 0.39.0 — unresolved attribution is visible in Git (author identity changes) When `attribution.enabled=true` and the operator cannot match a live watch event to an audit fact within `attribution.grace`, the Git author is now: @@ -2645,7 +2674,7 @@ be attributable, this author normally means the audit policy, webhook route, sou connectivity, or grace window is not configured correctly. It can also represent a change with no usable audit actor, so use the audit metrics before assigning a cause. -## Unreleased — a `patches:` block no longer refuses the folder (next minor; more folders accepted) +## 0.37.0 — a `patches:` block no longer refuses the folder (more folders accepted) A kustomization declaring `patches:` used to refuse the whole `GitTarget`. Not the edit — the **target**. A folder whose patch touched a replica count also lost `images:`/`replicas:` @@ -2673,7 +2702,7 @@ Exactly one shape is tolerated. The rest are refused **by name**, so the message | a `path:` naming no file in the tree, or escaping it | refused: `patches-outside-tree` | | `patchesStrategicMerge:`, `patchesJson6902:` (deprecated spellings) | refused under their own names | -## Unreleased — the build's output stops leaking into the build's input (next minor; bug fix + behavior change) +## 0.37.0 — the build's output stops leaking into the build's input (bug fix + behavior change) **If your kustomization declares `labels:`, `commonLabels:`, `commonAnnotations:` or `namespace:`, the operator has been writing those injected values into your source manifests.** Measured, on a @@ -2719,7 +2748,7 @@ to say which of the source's bytes you meant to keep, and pairing the lists by p measurably wrong (kustomize *prepends* a container a patch adds), so the operator refuses rather than guesses. -## Unreleased — kustomize decides what it renders, and what it touched (next minor; bug fixes + behavior change) +## 0.36.0 — kustomize decides what it renders, and what it touched (bug fixes + behavior change) The write path no longer contains a re-implementation of kustomize's image and replica transformers. It asks kustomize what a folder renders to, and — by rendering a second time @@ -2763,7 +2792,7 @@ worked. If you see this refusal, the live state cannot be expressed in the repos stands — most often because something we do not model (a `patches:` block, a `replacements:` entry) owns the field. The refusal names it. -## Unreleased — a folder kustomize cannot build is now refused (next minor; behavior change) +## 0.36.0 — a folder kustomize cannot build is now refused (behavior change) The analyzer now **builds** every render root with kustomize, instead of only parsing the kustomization structurally. A root that fails to build refuses the `GitTarget`, quoting @@ -2800,7 +2829,7 @@ protecting it. now fails the target with `GitPathAccepted=False` / `UnsupportedContent` instead of writing into a folder whose render it cannot know. Fix the build. -## Unreleased — a `digest:` override no longer strips the tag out of your source file (next patch; bug fix) +## 0.36.0 — a `digest:` override no longer strips the tag out of your source file (bug fix) **If any of your kustomizations use `images:` with `digest:`, or `newTag:` on an image that carries a digest, the operator has been rewriting your source manifests. This stops.** @@ -2826,7 +2855,7 @@ concluded the user had *removed* the tag, and wrote the tag out of the source do - Nothing to configure. The behaviour is simply correct now, and pinned against a real `kustomize build` so it cannot regress. -## Unreleased — `kustomization.yaml` is now read by kustomize itself (next minor; behavior change) +## 0.36.0 — `kustomization.yaml` is now read by kustomize itself (behavior change) The analyzer used to decode `kustomization.yaml` with a hand-written walk over a generic YAML map, checked against a hand-maintained list of 17 unsupported keys. It now decodes with @@ -2857,7 +2886,7 @@ every case the new behaviour is the one that agrees with the renderer: in Git; edit the source field instead. - Nothing to do for the last two rows: they only ever accept more. -## Unreleased — `pkg/manifestanalyzer`: the overlay fan-out refusal code was renamed (next minor; breaking for consumers) +## 0.35.0 — `pkg/manifestanalyzer`: the overlay fan-out refusal code was renamed (breaking for consumers) One refusal reason changed its name, in both the Go constant and the machine-readable value it carries: @@ -2880,7 +2909,7 @@ our planning docs; the new one says what it means. a `switch` or `if` on `"overlay-fan-out-needs-f2"` simply stops matching, and the refusal falls through to whatever your default branch does. Grep for the old string. -## Unreleased — the wildcard cluster read moved to its own, droppable ClusterRole (next minor; behavior change) +## 0.34.0 — the wildcard cluster read moved to its own, droppable ClusterRole (behavior change) The shipped RBAC now says what the binary actually does. Nothing is taken away from a default install; what changes is that the parts can be separated. @@ -2930,9 +2959,7 @@ reflector retried the denial forever. Discovery reports what the server serves, may read, so a narrowed role reached this path — which is exactly the path this release makes easy to enter. -## Unreleased — `manifest-analyzer` scan modes renamed, and `--format json` now emits a - -versioned contract (next minor; breaking) +## 0.34.0 — `manifest-analyzer` scan modes renamed, and `--format json` now emits a versioned contract (breaking) The analyzer's machine-readable output moved to the new public [`pkg/manifestanalyzer`](../pkg/manifestanalyzer), which is a supported Go API and the single @@ -2979,7 +3006,7 @@ release you are upgrading *across*, not as current advice.) adopt (or refuse) the folder. - If you parsed `plan` from folder-scan mode, you were reading an empty object; drop the field. -## Unreleased — chart defaults now run Redis-free (next minor; behavior change) +## 0.32.0 — chart defaults now run Redis-free (behavior change) The Helm chart now defaults to the simple, Redis-free `configured-author` path, so a bare `helm install` comes up healthy without external infrastructure: @@ -3005,7 +3032,7 @@ The Helm chart now defaults to the simple, Redis-free `configured-author` path, - `helm upgrade --reuse-values` preserves your existing settings, so reused-value upgrades are unaffected; only fresh installs (or upgrades that re-specify values) pick up the new defaults. -## Unreleased — API group version bumped `v1alpha2` → `v1alpha3` (next minor; breaking) +## 0.29.0 — API group version bumped `v1alpha2` → `v1alpha3` (breaking) The served API version moved from `configbutler.ai/v1alpha2` to `configbutler.ai/v1alpha3` to reflect the accumulated schema and status changes on this branch. `v1alpha2` is **removed**, not @@ -3024,7 +3051,7 @@ CRDs are applied. - `kubectl` commands that pin the version (`kubectl get gittargets.v1alpha2.configbutler.ai`) must switch to `v1alpha3`. Unqualified short names (`kubectl get gittargets`) need no change. -## Unreleased — first-run and status surface cleanup (next minor; breaking) +## 0.29.0 — first-run and status surface cleanup (breaking) This branch changes the default install to be easier to try, and it tightens the v1alpha3 status surface around conditions. Existing installs should check the items below before upgrading. @@ -3130,7 +3157,7 @@ kubectl wait --for=condition=StreamsRunning=true gittarget/ -n `WatchRule` and `ClusterWatchRule` use the same condition vocabulary for source readiness (`StreamsRunning`) and referenced target readiness (`GitTargetReady`). -## Unreleased — Config flag naming pass (next minor; breaking) +## 0.29.0 — Config flag naming pass (breaking) Controller command-line flags were renamed to follow [`config-flag-conventions.md`](config-flag-conventions.md). The Helm chart and the @@ -3154,7 +3181,7 @@ Redis/Valkey over TLS unless told otherwise. The Helm chart in-cluster Valkey. **If you run the controller directly against a plaintext Redis, add `--redis-insecure`** — otherwise startup fails on a TLS handshake. -## Unreleased — Git credentials interop (next minor; breaking) +## 0.28.0 — Git credentials interop (breaking) Two user-visible breaking changes land together. Both come from [`design/git-credentials-interop.md`](finished/git-credentials-interop.md). From f746c012483d5e85785b19c38e7bd1dd3790c152 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sat, 24 Oct 2026 08:05:03 +0000 Subject: [PATCH 3/4] feat(api): default GitBranch and KubeProvider grants to the own namespace An omitted GitBranch.spec.allowedGitTargets, KubeProvider.spec.allowedGitTargets or KubeProvider.spec.allowedApplierClaims is now one from: Same grant (scope {consumer.namespace}, no clusterResources), set by the CRD default. A written list is the whole policy and [] admits nobody. GitProvider.spec.allowedGitBranches keeps no default: which branches may be claimed is always written. The three fields drop omitempty so [] survives an Update instead of being re-defaulted, Go mirrors the default for a nil list, and the chart renders [] as [] while an unset value leaves the field to the CRD default. Co-Authored-By: Claude Opus 5.5 (1M context) --- api/v1alpha3/access_grants.go | 65 ++++++++++- api/v1alpha3/access_grants_defaults_test.go | 79 +++++++++++++ api/v1alpha3/access_grants_test.go | 47 +++++++- api/v1alpha3/gitbranch_types.go | 12 +- api/v1alpha3/gitprovider_types.go | 7 +- api/v1alpha3/kubeprovider_types.go | 19 +++- charts/gitops-reverser/README.md | 4 +- .../templates/kubeprovider.yaml | 31 ++++-- charts/gitops-reverser/values.schema.json | 8 +- charts/gitops-reverser/values.yaml | 12 +- cmd/main_chart_kubeprovider_test.go | 25 ++++- .../bases/configbutler.ai_gitbranches.yaml | 12 +- .../bases/configbutler.ai_gitproviders.yaml | 7 +- .../bases/configbutler.ai_kubeproviders.yaml | 23 +++- config/kubeprovider-local.yaml | 6 +- config/samples/kubeprovider.yaml | 3 +- config/samples/quickstart-gitbranch.yaml | 3 +- docs/architecture.md | 6 +- docs/attribution-setup-guide.md | 4 +- docs/configuration.md | 45 +++++--- docs/definitions.md | 4 +- docs/finished/provider-access-rules.md | 5 + docs/quickstart.md | 2 +- hack/crdfields/fields.yaml | 14 +-- internal/authz/kubeprovider_admission_test.go | 54 +++++++-- .../controller/gittarget_gitbranch_test.go | 5 +- .../gittarget_source_cluster_test.go | 11 +- .../grant_defaults_apiserver_test.go | 105 ++++++++++++++++++ 28 files changed, 513 insertions(+), 105 deletions(-) create mode 100644 api/v1alpha3/access_grants_defaults_test.go create mode 100644 internal/controller/grant_defaults_apiserver_test.go diff --git a/api/v1alpha3/access_grants.go b/api/v1alpha3/access_grants.go index b6ba70c8..a6c39a4c 100644 --- a/api/v1alpha3/access_grants.go +++ b/api/v1alpha3/access_grants.go @@ -221,6 +221,59 @@ type KubeProviderApplierClaimGrant struct { ApplierNamespaces []ScopeNamespace `json:"applierNamespaces"` } +// The API server defaults an omitted grant list on GitBranch and KubeProvider (the +// +kubebuilder:default markers on their fields), so a list read from it is never nil. These mirror +// those markers for an object that did not come from the API server, such as one built in memory or +// read through a fake client: nil is the default, and only a non-nil empty list admits nobody. +// TestGrantDefaultsMatchCRDs keeps the two in step. + +// DefaultGitBranchGitTargetGrants is GitBranch.spec.allowedGitTargets when it is omitted: GitTargets +// in the GitBranch's own namespace. +func DefaultGitBranchGitTargetGrants() []GitBranchGitTargetGrant { + return []GitBranchGitTargetGrant{{ConsumerNamespaces: ConsumerNamespaces{From: GrantFromSame}}} +} + +// DefaultKubeProviderGitTargetGrants is KubeProvider.spec.allowedGitTargets when it is omitted: +// GitTargets in the provider's own namespace, mirroring the namespace of the same name. +func DefaultKubeProviderGitTargetGrants() []KubeProviderGitTargetGrant { + return []KubeProviderGitTargetGrant{{ + ConsumerNamespaces: ConsumerNamespaces{From: GrantFromSame}, + SourceNamespaces: []ScopeNamespace{ConsumerNamespaceVariable}, + }} +} + +// DefaultKubeProviderApplierClaimGrants is KubeProvider.spec.allowedApplierClaims when it is omitted: +// applier claims in the provider's own namespace, holding appliers in the namespace of the same name. +func DefaultKubeProviderApplierClaimGrants() []KubeProviderApplierClaimGrant { + return []KubeProviderApplierClaimGrant{{ + ConsumerNamespaces: ConsumerNamespaces{From: GrantFromSame}, + ApplierNamespaces: []ScopeNamespace{ConsumerNamespaceVariable}, + }} +} + +// orDefault is grants, or def when grants is nil (omitted rather than empty). +func orDefault[G any](grants []G, def func() []G) []G { + if grants == nil { + return def() + } + return grants +} + +// gitTargetGrants is spec.allowedGitTargets as the API server serves it. +func (b *GitBranch) gitTargetGrants() []GitBranchGitTargetGrant { + return orDefault(b.Spec.AllowedGitTargets, DefaultGitBranchGitTargetGrants) +} + +// gitTargetGrants is spec.allowedGitTargets as the API server serves it. +func (p *KubeProvider) gitTargetGrants() []KubeProviderGitTargetGrant { + return orDefault(p.Spec.AllowedGitTargets, DefaultKubeProviderGitTargetGrants) +} + +// applierClaimGrants is spec.allowedApplierClaims as the API server serves it. +func (p *KubeProvider) applierClaimGrants() []KubeProviderApplierClaimGrant { + return orDefault(p.Spec.AllowedApplierClaims, DefaultKubeProviderApplierClaimGrants) +} + // consumers is the admitting half of a GitProviderGitBranchGrant. func (g *GitProviderGitBranchGrant) consumers() ConsumerNamespaces { return g.ConsumerNamespaces } @@ -438,13 +491,13 @@ func (p *GitProvider) DescribeAllowedGitBranches() string { // AdmitsGitTarget reports whether spec.allowedGitTargets admits a GitTarget in namespace, by the // labels that namespace carries in the control cluster. func (b *GitBranch) AdmitsGitTarget(namespace string, nsLabels map[string]string) (bool, error) { - admitting, err := admitAll(consumersOf(b.Spec.AllowedGitTargets), b.Namespace, namespace, nsLabels) + admitting, err := admitAll(consumersOf(b.gitTargetGrants()), b.Namespace, namespace, nsLabels) return len(admitting) > 0, err } // DescribeAllowedGitTargets names the namespaces spec.allowedGitTargets admits. func (b *GitBranch) DescribeAllowedGitTargets() string { - return describeAdmitted(consumersOf(b.Spec.AllowedGitTargets), b.Namespace) + return describeAdmitted(consumersOf(b.gitTargetGrants()), b.Namespace) } // KubeProviderGitTargetScope is what a KubeProvider's allowedGitTargets give a GitTarget: whether it may mirror at @@ -492,7 +545,7 @@ func scopeAllows(scope []string, value string) bool { func (p *KubeProvider) AdmitsGitTarget( consumer types.NamespacedName, nsLabels map[string]string, ) (KubeProviderGitTargetScope, error) { - grants := p.Spec.AllowedGitTargets + grants := p.gitTargetGrants() admitting, err := admitAll(consumersOf(grants), p.Namespace, consumer.Namespace, nsLabels) if err != nil || len(admitting) == 0 { return KubeProviderGitTargetScope{}, err @@ -512,7 +565,7 @@ func (p *KubeProvider) AdmitsGitTarget( // DescribeAllowedGitTargets names the namespaces spec.allowedGitTargets admits. func (p *KubeProvider) DescribeAllowedGitTargets() string { - return describeAdmitted(consumersOf(p.Spec.AllowedGitTargets), p.Namespace) + return describeAdmitted(consumersOf(p.gitTargetGrants()), p.Namespace) } // KubeProviderApplierClaimScope is what a KubeProvider's allowedApplierClaims give an applier claim. @@ -535,7 +588,7 @@ func (s KubeProviderApplierClaimScope) AllowsApplierNamespace(namespace string) func (p *KubeProvider) AdmitsApplierClaim( consumer types.NamespacedName, nsLabels map[string]string, ) (KubeProviderApplierClaimScope, error) { - grants := p.Spec.AllowedApplierClaims + grants := p.applierClaimGrants() admitting, err := admitAll(consumersOf(grants), p.Namespace, consumer.Namespace, nsLabels) if err != nil || len(admitting) == 0 { return KubeProviderApplierClaimScope{}, err @@ -549,5 +602,5 @@ func (p *KubeProvider) AdmitsApplierClaim( // DescribeAllowedApplierClaims names the namespaces spec.allowedApplierClaims admits. func (p *KubeProvider) DescribeAllowedApplierClaims() string { - return describeAdmitted(consumersOf(p.Spec.AllowedApplierClaims), p.Namespace) + return describeAdmitted(consumersOf(p.applierClaimGrants()), p.Namespace) } diff --git a/api/v1alpha3/access_grants_defaults_test.go b/api/v1alpha3/access_grants_defaults_test.go new file mode 100644 index 00000000..affd4dc9 --- /dev/null +++ b/api/v1alpha3/access_grants_defaults_test.go @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: Apache-2.0 + +package v1alpha3 + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "sigs.k8s.io/yaml" +) + +// crdSpecFieldDefault is the default the generated CRD gives spec. of its first version. +func crdSpecFieldDefault(t *testing.T, file, field string) []byte { + t.Helper() + raw, err := os.ReadFile(filepath.Join("..", "..", "config", "crd", "bases", file)) + require.NoError(t, err) + var crd apiextensionsv1.CustomResourceDefinition + require.NoError(t, yaml.Unmarshal(raw, &crd)) + require.NotEmpty(t, crd.Spec.Versions) + prop, ok := crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"].Properties[field] + require.True(t, ok, "%s has no spec.%s", file, field) + require.NotNil(t, prop.Default, "%s spec.%s has no default", file, field) + return prop.Default.Raw +} + +// TestGrantDefaultsMatchCRDs keeps the Go defaults the grant methods use for a nil list equal to the +// defaults the API server applies from the CRDs. +func TestGrantDefaultsMatchCRDs(t *testing.T) { + t.Run("GitBranch allowedGitTargets", func(t *testing.T) { + assertCRDDefault(t, "configbutler.ai_gitbranches.yaml", "allowedGitTargets", DefaultGitBranchGitTargetGrants()) + }) + t.Run("KubeProvider allowedGitTargets", func(t *testing.T) { + assertCRDDefault(t, "configbutler.ai_kubeproviders.yaml", "allowedGitTargets", + DefaultKubeProviderGitTargetGrants()) + }) + t.Run("KubeProvider allowedApplierClaims", func(t *testing.T) { + assertCRDDefault(t, "configbutler.ai_kubeproviders.yaml", "allowedApplierClaims", + DefaultKubeProviderApplierClaimGrants()) + }) +} + +// assertCRDDefault asserts that the CRD's default for spec. decodes to want. +func assertCRDDefault[G any](t *testing.T, file, field string, want []G) { + t.Helper() + var got []G + require.NoError(t, json.Unmarshal(crdSpecFieldDefault(t, file, field), &got)) + assert.Equal(t, want, got) +} + +// TestGrantLists_EmptySurvivesSerialization pins that [] stays [] on the wire, so an explicit deny-all +// is never sent as an omitted field that the API server would default back to a grant. +func TestGrantLists_EmptySurvivesSerialization(t *testing.T) { + branch := GitBranchSpec{AllowedGitTargets: []GitBranchGitTargetGrant{}} + provider := KubeProviderSpec{ + AllowedGitTargets: []KubeProviderGitTargetGrant{}, + AllowedApplierClaims: []KubeProviderApplierClaimGrant{}, + } + for name, spec := range map[string]any{"GitBranch": branch, "KubeProvider": provider} { + raw, err := json.Marshal(spec) + require.NoError(t, err) + var fields map[string]json.RawMessage + require.NoError(t, json.Unmarshal(raw, &fields)) + for _, field := range []string{"allowedGitTargets", "allowedApplierClaims"} { + if name == "GitBranch" && field == "allowedApplierClaims" { + continue + } + assert.JSONEq(t, "[]", string(fields[field]), "%s spec.%s", name, field) + } + } + + var decoded GitBranchSpec + require.NoError(t, json.Unmarshal([]byte(`{"allowedGitTargets":[]}`), &decoded)) + assert.NotNil(t, decoded.AllowedGitTargets, "a decoded [] stays distinguishable from omitted") +} diff --git a/api/v1alpha3/access_grants_test.go b/api/v1alpha3/access_grants_test.go index 58663bd1..cf3fc49c 100644 --- a/api/v1alpha3/access_grants_test.go +++ b/api/v1alpha3/access_grants_test.go @@ -136,18 +136,55 @@ func TestConsumerNamespaces_InvalidSelectorIsAnError(t *testing.T) { assert.False(t, admits) } -func TestGitBranch_AdmitsGitTarget_NothingAutomatic(t *testing.T) { +func TestGitBranch_AdmitsGitTarget_DefaultsToSameNamespace(t *testing.T) { branch := &GitBranch{ObjectMeta: metav1.ObjectMeta{Namespace: "team-a"}} admits, err := branch.AdmitsGitTarget("team-a", nil) require.NoError(t, err) - assert.False(t, admits, "an omitted list admits nobody, the GitBranch's own namespace included") + assert.True(t, admits, "an omitted list is the default grant for the GitBranch's own namespace") + admits, err = branch.AdmitsGitTarget("team-b", nil) + require.NoError(t, err) + assert.False(t, admits, "the default admits only the GitBranch's own namespace") + assert.Equal(t, "admits team-a", branch.DescribeAllowedGitTargets()) + + branch.Spec.AllowedGitTargets = []GitBranchGitTargetGrant{} + admits, err = branch.AdmitsGitTarget("team-a", nil) + require.NoError(t, err) + assert.False(t, admits, "[] admits nobody, the GitBranch's own namespace included") assert.Equal(t, "admits no namespace", branch.DescribeAllowedGitTargets()) branch.Spec.AllowedGitTargets = []GitBranchGitTargetGrant{ - {ConsumerNamespaces: ConsumerNamespaces{From: GrantFromSame}}, {ConsumerNamespaces: ConsumerNamespaces{From: GrantFromNames, Names: []GrantNamespace{"team-b"}}}, } - assert.Equal(t, "admits team-a, team-b", branch.DescribeAllowedGitTargets()) + admits, err = branch.AdmitsGitTarget("team-a", nil) + require.NoError(t, err) + assert.False(t, admits, "a written list is the whole policy: the default is not added to it") + assert.Equal(t, "admits team-b", branch.DescribeAllowedGitTargets()) +} + +func TestKubeProvider_DefaultsToSameNamespace(t *testing.T) { + provider := &KubeProvider{ObjectMeta: metav1.ObjectMeta{Namespace: "team-a"}} + scope, err := provider.AdmitsGitTarget(consumer("team-a", "apps"), nil) + require.NoError(t, err) + assert.True(t, scope.Admitted) + assert.Equal(t, []string{"team-a"}, scope.SourceNamespaces, + "an omitted list mirrors the namespace of the same name") + assert.Empty(t, scope.ClusterResources, "and no cluster-scoped resource") + scope, err = provider.AdmitsGitTarget(consumer("team-b", "apps"), nil) + require.NoError(t, err) + assert.False(t, scope.Admitted) + + claim, err := provider.AdmitsApplierClaim(consumer("team-a", "checkout"), nil) + require.NoError(t, err) + assert.Equal(t, KubeProviderApplierClaimScope{Admitted: true, ApplierNamespaces: []string{"team-a"}}, claim) + + provider.Spec.AllowedGitTargets = []KubeProviderGitTargetGrant{} + provider.Spec.AllowedApplierClaims = []KubeProviderApplierClaimGrant{} + scope, err = provider.AdmitsGitTarget(consumer("team-a", "apps"), nil) + require.NoError(t, err) + assert.False(t, scope.Admitted, "[] admits nobody") + claim, err = provider.AdmitsApplierClaim(consumer("team-a", "checkout"), nil) + require.NoError(t, err) + assert.False(t, claim.Admitted, "[] admits nobody") } func TestGitProvider_AdmitsGitBranch(t *testing.T) { @@ -194,7 +231,7 @@ func TestGitProvider_AdmitsGitBranch(t *testing.T) { provider.Spec.AllowedGitBranches = nil got, err = provider.AdmitsGitBranch(consumer("platform", "live"), nil, "main") require.NoError(t, err) - assert.False(t, got.Admitted, "nothing is automatic, the provider's own namespace included") + assert.False(t, got.Admitted, "allowedGitBranches has no default, the provider's own namespace included") } func TestKubeProvider_AdmitsGitTarget(t *testing.T) { diff --git a/api/v1alpha3/gitbranch_types.go b/api/v1alpha3/gitbranch_types.go index 018346d8..dbce71bc 100644 --- a/api/v1alpha3/gitbranch_types.go +++ b/api/v1alpha3/gitbranch_types.go @@ -56,14 +56,16 @@ type GitBranchSpec struct { // +kubebuilder:validation:MinLength=1 ParentBranch string `json:"parentBranch,omitempty"` - // AllowedGitTargets says which GitTargets may write through this GitBranch. Nothing is admitted - // automatically, this GitBranch's own namespace included: a GitTarget there needs a grant too, - // such as consumerNamespaces: {from: Same}. Omitted or empty, no GitTarget writes through it. - // Grants combine by union. Mutable: claiming a branch includes the right to share it. + // AllowedGitTargets says which GitTargets may write through this GitBranch. Omitted, it defaults + // to one grant for this GitBranch's own namespace (consumerNamespaces: {from: Same}). A list you + // write is the whole policy: nothing is added to it, so a list without a from: Same grant shuts + // this namespace out, and [] admits no GitTarget. Grants combine by union. Mutable: claiming a + // branch includes the right to share it. // +optional // +listType=atomic // +kubebuilder:validation:MaxItems=64 - AllowedGitTargets []GitBranchGitTargetGrant `json:"allowedGitTargets,omitempty"` + // +kubebuilder:default={{consumerNamespaces: {from: Same}}} + AllowedGitTargets []GitBranchGitTargetGrant `json:"allowedGitTargets"` } // GitBranchStatus defines the observed state of GitBranch. diff --git a/api/v1alpha3/gitprovider_types.go b/api/v1alpha3/gitprovider_types.go index 0525e2a7..4c12436a 100644 --- a/api/v1alpha3/gitprovider_types.go +++ b/api/v1alpha3/gitprovider_types.go @@ -53,9 +53,10 @@ type GitProviderSpec struct { // AllowedGitBranches says which GitBranches may claim a branch of this repository through this // GitProvider, and which branches: a GitBranch claims only while a grant admits its namespace and - // one of that grant's branchPatterns matches its spec.branch. Nothing is admitted automatically, this - // GitProvider's own namespace included; omitted or empty, no GitBranch claims through it. Grants - // combine by union. Who may write through a claimed branch is that GitBranch's + // one of that grant's branchPatterns matches its spec.branch. It has no default, unlike the grant + // lists of GitBranch and KubeProvider, because which branches a namespace may claim is always + // written: omitted or empty, no GitBranch claims through it, this GitProvider's own namespace + // included. Grants combine by union. Who may write through a claimed branch is that GitBranch's // spec.allowedGitTargets. // +optional // +listType=atomic diff --git a/api/v1alpha3/kubeprovider_types.go b/api/v1alpha3/kubeprovider_types.go index 716b6dbc..7dcff0de 100644 --- a/api/v1alpha3/kubeprovider_types.go +++ b/api/v1alpha3/kubeprovider_types.go @@ -46,21 +46,28 @@ type KubeProviderSpec struct { // AllowedGitTargets says which GitTargets may mirror through this provider, and what: each grant // admits some control-cluster namespaces and bounds what the WatchRules feeding a GitTarget there // may watch (sourceNamespaces), and what a ClusterWatchRule may mirror through it - // (clusterResources). Nothing is admitted automatically, this KubeProvider's own namespace - // included; omitted or empty, no GitTarget mirrors through it. Grants combine by union. + // (clusterResources). Omitted, it defaults to one grant for this KubeProvider's own namespace, + // mirroring the namespace of the same name (consumerNamespaces: {from: Same}, sourceNamespaces: + // ["{consumer.namespace}"]); it grants no cluster-scoped resource. A list you write is the whole + // policy: nothing is added to it, and [] admits no GitTarget. Grants combine by union. // +optional // +listType=atomic // +kubebuilder:validation:MaxItems=64 - AllowedGitTargets []KubeProviderGitTargetGrant `json:"allowedGitTargets,omitempty"` + // +kubebuilder:default={{consumerNamespaces: {from: Same}, sourceNamespaces: {"{consumer.namespace}"}}} + AllowedGitTargets []KubeProviderGitTargetGrant `json:"allowedGitTargets"` // AllowedApplierClaims says which applier claims (FluxClaims and ArgoCDClaims) may hold an applier // through this provider, and in which namespaces that applier may live (applierNamespaces). - // Admission to mirror does not admit to apply. Nothing is admitted automatically; omitted or - // empty, no applier claim holds an applier through it. Grants combine by union. + // Admission to mirror does not admit to apply. Omitted, it defaults to one grant for this + // KubeProvider's own namespace, holding appliers in the namespace of the same name + // (consumerNamespaces: {from: Same}, applierNamespaces: ["{consumer.namespace}"]). A list you + // write is the whole policy: nothing is added to it, and [] admits no applier claim. Grants + // combine by union. // +optional // +listType=atomic // +kubebuilder:validation:MaxItems=64 - AllowedApplierClaims []KubeProviderApplierClaimGrant `json:"allowedApplierClaims,omitempty"` + // +kubebuilder:default={{consumerNamespaces: {from: Same}, applierNamespaces: {"{consumer.namespace}"}}} + AllowedApplierClaims []KubeProviderApplierClaimGrant `json:"allowedApplierClaims"` // Client overrides the operator's outgoing kube-client throttles for this cluster. Omitted, // the built-in defaults apply (20 QPS, burst 30). Refused beside inCluster. diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index 047b8866..89cb01f4 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -218,8 +218,8 @@ nodeSelector: | `kubeProvider.name` | Name of the rendered provider. Its audit route is `.`, the `/audit-webhook/` path the API server posts to | `local` | | `kubeProvider.secretRef.name` | Secret in the release namespace holding a kubeconfig for a **remote** cluster. Empty renders `spec.inCluster`, the operator's **own** identity, and lists the provider in `--in-cluster-kube-providers`. Immutable once created: delete and recreate to repoint | `""` | | `kubeProvider.secretRef.key` | Key within that Secret. Empty reads `value` then `value.yaml` | `""` | -| `kubeProvider.allowedGitTargets` | Rendered as the provider's `spec.allowedGitTargets`: which namespaces' `GitTarget`s may mirror through it, and what. Each grant has a `consumerNamespaces` block (`from`: `Same`, `Names`, `Selector`, `All`), and `sourceNamespaces`, `clusterResources` or both beside it. **Empty grants nothing, the release namespace included**: a `GitTarget` through `local` is refused with a message naming `spec.allowedGitTargets`. `quickstart.enabled` adds a grant for the quickstart namespace, and [`values-playground.yaml`](values-playground.yaml) grants every namespace its own. With an `inCluster` provider a grant bypasses live namespace RBAC through the operator's identity | `[]` | -| `kubeProvider.allowedApplierClaims` | Rendered as the provider's `spec.allowedApplierClaims`: which namespaces' `FluxClaim`s and `ArgoCDClaim`s may hold an applier through it (`consumerNamespaces`), and in which `applierNamespaces` it may live, such as `flux-system`. Admission to mirror is no admission to apply. Empty grants nothing | `[]` | +| `kubeProvider.allowedGitTargets` | Rendered as the provider's `spec.allowedGitTargets`: which namespaces' `GitTarget`s may mirror through it, and what. Each grant has a `consumerNamespaces` block (`from`: `Same`, `Names`, `Selector`, `All`), and `sourceNamespaces`, `clusterResources` or both beside it. **Unset, the field is omitted and the CRD defaults it to the release namespace mirroring its own**; a list is the whole policy, and `[]` admits nobody, the release namespace included: a refused `GitTarget` names `spec.allowedGitTargets`. `quickstart.enabled` adds a grant for the quickstart namespace, and [`values-playground.yaml`](values-playground.yaml) grants every namespace its own. With an `inCluster` provider a grant bypasses live namespace RBAC through the operator's identity | `null` | +| `kubeProvider.allowedApplierClaims` | Rendered as the provider's `spec.allowedApplierClaims`: which namespaces' `FluxClaim`s and `ArgoCDClaim`s may hold an applier through it (`consumerNamespaces`), and in which `applierNamespaces` it may live, such as `flux-system`. Admission to mirror is no admission to apply. Unset, the CRD defaults it to the release namespace holding appliers in its own; `[]` admits nobody | `null` | | `servers.admission.enabled` | Install the validate-operator-types admission webhook that captures CommitRequest authors (a form of author attribution). Enabled by default; a no-op until `queue.redis.addr` is set | `true` | | `rbac.create` | Create the manager ClusterRole and its binding | `true` | | `rbac.watchTypes.mode` | Which types a `WatchRule` may read. `any` grants cluster-wide read on everything, including every Secret in the cluster. `selected` grants read on `rbac.watchTypes.selected` only, so the reverser cannot list or watch Secrets (it keeps `get` on named Secrets it is pointed at). See [`docs/rbac.md`](../../docs/rbac.md) | `any` | diff --git a/charts/gitops-reverser/templates/kubeprovider.yaml b/charts/gitops-reverser/templates/kubeprovider.yaml index b720a3a1..24c5db24 100644 --- a/charts/gitops-reverser/templates/kubeprovider.yaml +++ b/charts/gitops-reverser/templates/kubeprovider.yaml @@ -4,10 +4,12 @@ # makes Helm delete it again. Every GitTarget names its KubeProvider explicitly # (spec.kubeProviderRef); nothing falls back to this one. With kubeProvider.secretRef.name empty it # uses the operator's own identity (spec.inCluster), and the chart lists it in -# --in-cluster-kube-providers, which is the only way an inCluster provider is honored. Nothing may -# use it without a grant: by default it admits no namespace. kubeProvider.allowedGitTargets and -# kubeProvider.allowedApplierClaims grant it; quickstart.enabled adds a grant for exactly the -# quickstart namespace, and values-playground.yaml grants every namespace its own. +# --in-cluster-kube-providers, which is the only way an inCluster provider is honored. Left unset, +# kubeProvider.allowedGitTargets and kubeProvider.allowedApplierClaims are omitted, and the CRD +# defaults each to one grant for the release namespace's own objects. A list set there is rendered +# as written and is the whole policy, so [] renders as [] and admits nobody. quickstart.enabled adds +# a grant for exactly the quickstart namespace, and values-playground.yaml grants every namespace +# its own. apiVersion: configbutler.ai/v1alpha3 kind: KubeProvider metadata: @@ -31,15 +33,24 @@ spec: {{- $applierClaims := .Values.kubeProvider.allowedApplierClaims }} {{- if .Values.quickstart.enabled }} {{- $names := list .Values.quickstart.namespace }} - {{- $gitTargets = append $gitTargets (dict "consumerNamespaces" (dict "from" "Names" "names" $names) "sourceNamespaces" (list "{consumer.namespace}")) }} - {{- $applierClaims = append $applierClaims (dict "consumerNamespaces" (dict "from" "Names" "names" $names) "applierNamespaces" (list "{consumer.namespace}")) }} + {{- $gitTargets = append (default (list) $gitTargets) (dict "consumerNamespaces" (dict "from" "Names" "names" $names) "sourceNamespaces" (list "{consumer.namespace}")) }} + {{- $applierClaims = append (default (list) $applierClaims) (dict "consumerNamespaces" (dict "from" "Names" "names" $names) "applierNamespaces" (list "{consumer.namespace}")) }} {{- end }} - {{- with $gitTargets }} + {{- /* An unset list stays unset, so the CRD default applies; [] must reach the API as []. */}} + {{- if kindIs "slice" $gitTargets }} + {{- if $gitTargets }} allowedGitTargets: - {{- toYaml . | nindent 4 }} + {{- toYaml $gitTargets | nindent 4 }} + {{- else }} + allowedGitTargets: [] + {{- end }} {{- end }} - {{- with $applierClaims }} + {{- if kindIs "slice" $applierClaims }} + {{- if $applierClaims }} allowedApplierClaims: - {{- toYaml . | nindent 4 }} + {{- toYaml $applierClaims | nindent 4 }} + {{- else }} + allowedApplierClaims: [] + {{- end }} {{- end }} {{- end }} diff --git a/charts/gitops-reverser/values.schema.json b/charts/gitops-reverser/values.schema.json index 3220c4fc..4eb69f9d 100644 --- a/charts/gitops-reverser/values.schema.json +++ b/charts/gitops-reverser/values.schema.json @@ -401,13 +401,13 @@ } }, "allowedGitTargets": { - "type": "array", - "description": "Rendered as the provider's spec.allowedGitTargets: which namespaces' GitTargets may mirror through it (consumerNamespaces), and what (sourceNamespaces, clusterResources). Empty grants nothing. The CRD validates each grant.", + "type": ["array", "null"], + "description": "Rendered as the provider's spec.allowedGitTargets: which namespaces' GitTargets may mirror through it (consumerNamespaces), and what (sourceNamespaces, clusterResources). Null leaves the field out, and the CRD defaults it to the release namespace mirroring its own; [] admits nobody. A list is the whole policy. The CRD validates each grant.", "items": { "type": "object" } }, "allowedApplierClaims": { - "type": "array", - "description": "Rendered as the provider's spec.allowedApplierClaims: which namespaces' applier claims may hold an applier through it (consumerNamespaces), and where it may live (applierNamespaces). Empty grants nothing. The CRD validates each grant.", + "type": ["array", "null"], + "description": "Rendered as the provider's spec.allowedApplierClaims: which namespaces' applier claims may hold an applier through it (consumerNamespaces), and where it may live (applierNamespaces). Null leaves the field out, and the CRD defaults it to the release namespace holding appliers in its own; [] admits nobody. A list is the whole policy. The CRD validates each grant.", "items": { "type": "object" } } } diff --git a/charts/gitops-reverser/values.yaml b/charts/gitops-reverser/values.yaml index bbc26ed7..5def3889 100644 --- a/charts/gitops-reverser/values.yaml +++ b/charts/gitops-reverser/values.yaml @@ -364,9 +364,11 @@ kubeProvider: name: "" key: "" # Who may use the operator's identity, and for what: rendered as the provider's - # spec.allowedGitTargets and spec.allowedApplierClaims. Empty by default, so NOTHING mirrors or - # holds an applier through this provider until you grant it, the release namespace included; a - # refusal names spec.allowedGitTargets. The installer decides, never the chart. + # spec.allowedGitTargets and spec.allowedApplierClaims. Unset (null) by default, so the fields are + # omitted and the CRD defaults each to one grant for the release namespace: its GitTargets mirror + # the namespace of the same name, its applier claims hold appliers there, and nothing else uses + # the operator's identity. A list you set is the whole policy, the default is not added to it, and + # [] admits nobody, the release namespace included; a refusal names spec.allowedGitTargets. # quickstart.enabled=true adds a grant for exactly the quickstart namespace, and # values-playground.yaml grants every namespace its own namespace. # @@ -394,8 +396,8 @@ kubeProvider: # from: Names # names: [team-a] # applierNamespaces: [flux-system] - allowedGitTargets: [] - allowedApplierClaims: [] + allowedGitTargets: null + allowedApplierClaims: null # RBAC configuration rbac: diff --git a/cmd/main_chart_kubeprovider_test.go b/cmd/main_chart_kubeprovider_test.go index e9c78292..13156fd8 100644 --- a/cmd/main_chart_kubeprovider_test.go +++ b/cmd/main_chart_kubeprovider_test.go @@ -64,11 +64,26 @@ func mirrors(t *testing.T, provider *configbutleraiv1alpha3.KubeProvider, namesp return decision } -// TestChartKubeProvider_ADefaultInstallGrantsNothing: the installer chooses who may use the -// operator's identity; the chart never chooses for them, the release namespace included. -func TestChartKubeProvider_ADefaultInstallGrantsNothing(t *testing.T) { +// TestChartKubeProvider_ADefaultInstallGrantsTheReleaseNamespace: unset values leave both grant +// lists out, so the CRD defaults them to the release namespace's own objects and nobody else. +func TestChartKubeProvider_ADefaultInstallGrantsTheReleaseNamespace(t *testing.T) { provider := renderLocalKubeProvider(t) + assert.Nil(t, provider.Spec.AllowedGitTargets, "omitted, so the API server applies the CRD default") + assert.Nil(t, provider.Spec.AllowedApplierClaims, "omitted, so the API server applies the CRD default") + assert.True(t, mirrors(t, provider, "gitops-reverser").Allowed) + decision := mirrors(t, provider, "team-a") + assert.False(t, decision.Allowed) + assert.Contains(t, decision.Message, "KubeProvider gitops-reverser/local admits gitops-reverser") +} + +// TestChartKubeProvider_AnEmptyListRendersAsEmpty: [] must reach the API server as [], or the CRD +// default would turn an explicit deny-all back into a grant. +func TestChartKubeProvider_AnEmptyListRendersAsEmpty(t *testing.T) { + provider := renderLocalKubeProvider(t, "--set-json", "kubeProvider.allowedGitTargets=[]", + "--set-json", "kubeProvider.allowedApplierClaims=[]") + assert.NotNil(t, provider.Spec.AllowedGitTargets) assert.Empty(t, provider.Spec.AllowedGitTargets) + assert.NotNil(t, provider.Spec.AllowedApplierClaims) assert.Empty(t, provider.Spec.AllowedApplierClaims) for _, namespace := range []string{"gitops-reverser", "team-a"} { decision := mirrors(t, provider, namespace) @@ -104,8 +119,8 @@ func TestChartKubeProvider_TheQuickstartGrantsItsNamespaceOnly(t *testing.T) { assert.False(t, mirrors(t, provider, "team-a").Allowed, "and nothing else") } -// TestChartKubeProvider_ThePlaygroundGrantsEveryNamespaceItsOwn: values-playground.yaml is today's -// open default, chosen visibly: every namespace mirrors its own namespace, no other, and no +// TestChartKubeProvider_ThePlaygroundGrantsEveryNamespaceItsOwn: values-playground.yaml is the open +// default, chosen visibly: every namespace mirrors its own namespace, no other, and no // cluster-scoped resource. func TestChartKubeProvider_ThePlaygroundGrantsEveryNamespaceItsOwn(t *testing.T) { provider := renderLocalKubeProvider(t, "-f", "../charts/gitops-reverser/values-playground.yaml") diff --git a/config/crd/bases/configbutler.ai_gitbranches.yaml b/config/crd/bases/configbutler.ai_gitbranches.yaml index e8ad2607..de025fd0 100644 --- a/config/crd/bases/configbutler.ai_gitbranches.yaml +++ b/config/crd/bases/configbutler.ai_gitbranches.yaml @@ -82,11 +82,15 @@ spec: description: spec defines the desired state of GitBranch properties: allowedGitTargets: + default: + - consumerNamespaces: + from: Same description: |- - AllowedGitTargets says which GitTargets may write through this GitBranch. Nothing is admitted - automatically, this GitBranch's own namespace included: a GitTarget there needs a grant too, - such as consumerNamespaces: {from: Same}. Omitted or empty, no GitTarget writes through it. - Grants combine by union. Mutable: claiming a branch includes the right to share it. + AllowedGitTargets says which GitTargets may write through this GitBranch. Omitted, it defaults + to one grant for this GitBranch's own namespace (consumerNamespaces: {from: Same}). A list you + write is the whole policy: nothing is added to it, so a list without a from: Same grant shuts + this namespace out, and [] admits no GitTarget. Grants combine by union. Mutable: claiming a + branch includes the right to share it. items: description: |- GitBranchGitTargetGrant admits GitTargets in some control-cluster namespaces to write through a diff --git a/config/crd/bases/configbutler.ai_gitproviders.yaml b/config/crd/bases/configbutler.ai_gitproviders.yaml index 7bf535b7..e435bc1e 100644 --- a/config/crd/bases/configbutler.ai_gitproviders.yaml +++ b/config/crd/bases/configbutler.ai_gitproviders.yaml @@ -64,9 +64,10 @@ spec: description: |- AllowedGitBranches says which GitBranches may claim a branch of this repository through this GitProvider, and which branches: a GitBranch claims only while a grant admits its namespace and - one of that grant's branchPatterns matches its spec.branch. Nothing is admitted automatically, this - GitProvider's own namespace included; omitted or empty, no GitBranch claims through it. Grants - combine by union. Who may write through a claimed branch is that GitBranch's + one of that grant's branchPatterns matches its spec.branch. It has no default, unlike the grant + lists of GitBranch and KubeProvider, because which branches a namespace may claim is always + written: omitted or empty, no GitBranch claims through it, this GitProvider's own namespace + included. Grants combine by union. Who may write through a claimed branch is that GitBranch's spec.allowedGitTargets. items: description: |- diff --git a/config/crd/bases/configbutler.ai_kubeproviders.yaml b/config/crd/bases/configbutler.ai_kubeproviders.yaml index ed83d30a..d9d157c5 100644 --- a/config/crd/bases/configbutler.ai_kubeproviders.yaml +++ b/config/crd/bases/configbutler.ai_kubeproviders.yaml @@ -70,11 +70,19 @@ spec: description: spec defines the desired state of KubeProvider. properties: allowedApplierClaims: + default: + - applierNamespaces: + - '{consumer.namespace}' + consumerNamespaces: + from: Same description: |- AllowedApplierClaims says which applier claims (FluxClaims and ArgoCDClaims) may hold an applier through this provider, and in which namespaces that applier may live (applierNamespaces). - Admission to mirror does not admit to apply. Nothing is admitted automatically; omitted or - empty, no applier claim holds an applier through it. Grants combine by union. + Admission to mirror does not admit to apply. Omitted, it defaults to one grant for this + KubeProvider's own namespace, holding appliers in the namespace of the same name + (consumerNamespaces: {from: Same}, applierNamespaces: ["{consumer.namespace}"]). A list you + write is the whole policy: nothing is added to it, and [] admits no applier claim. Grants + combine by union. items: description: |- KubeProviderApplierClaimGrant admits applier claims in some control-cluster namespaces to hold an @@ -228,12 +236,19 @@ spec: type: array x-kubernetes-list-type: atomic allowedGitTargets: + default: + - consumerNamespaces: + from: Same + sourceNamespaces: + - '{consumer.namespace}' description: |- AllowedGitTargets says which GitTargets may mirror through this provider, and what: each grant admits some control-cluster namespaces and bounds what the WatchRules feeding a GitTarget there may watch (sourceNamespaces), and what a ClusterWatchRule may mirror through it - (clusterResources). Nothing is admitted automatically, this KubeProvider's own namespace - included; omitted or empty, no GitTarget mirrors through it. Grants combine by union. + (clusterResources). Omitted, it defaults to one grant for this KubeProvider's own namespace, + mirroring the namespace of the same name (consumerNamespaces: {from: Same}, sourceNamespaces: + ["{consumer.namespace}"]); it grants no cluster-scoped resource. A list you write is the whole + policy: nothing is added to it, and [] admits no GitTarget. Grants combine by union. items: description: |- KubeProviderGitTargetGrant admits GitTargets in some control-cluster namespaces to mirror through a diff --git a/config/kubeprovider-local.yaml b/config/kubeprovider-local.yaml index 6171bd9f..883e2d5e 100644 --- a/config/kubeprovider-local.yaml +++ b/config/kubeprovider-local.yaml @@ -5,9 +5,9 @@ # spec.inCluster is honored only because deployment.yaml lists gitops-reverser/local in # --in-cluster-kube-providers. # -# Unlike the chart, whose provider grants nothing by default, this one grants what the e2e suite -# needs: every namespace mirrors its own namespace and any cluster-scoped resource, and holds an -# applier in its own namespace. Do not copy it into a shared cluster. +# Unlike the chart, whose provider grants only the release namespace by default, this one grants +# what the e2e suite needs: every namespace mirrors its own namespace and any cluster-scoped +# resource, and holds an applier in its own namespace. Do not copy it into a shared cluster. apiVersion: configbutler.ai/v1alpha3 kind: KubeProvider metadata: diff --git a/config/samples/kubeprovider.yaml b/config/samples/kubeprovider.yaml index 0c1e444d..b9fe86d2 100644 --- a/config/samples/kubeprovider.yaml +++ b/config/samples/kubeprovider.yaml @@ -17,7 +17,8 @@ metadata: spec: secretRef: name: prod-eu-1-kubeconfig - # Nothing uses this provider without a grant, its own namespace included. Each grant says where it + # Omitted, each list is one grant for this provider's own namespace; a list you write is the whole + # policy, and [] admits nobody. Each grant says where it # admits from (Same, Names, Selector or All) and what: the source namespaces a GitTarget's # WatchRules may watch ("{consumer.namespace}" is the GitTarget's own; "*" is every namespace, # which `sourceNamespace: "*"` needs), and the cluster-scoped resources a ClusterWatchRule may mirror. diff --git a/config/samples/quickstart-gitbranch.yaml b/config/samples/quickstart-gitbranch.yaml index fe02f312..35b1f0cb 100644 --- a/config/samples/quickstart-gitbranch.yaml +++ b/config/samples/quickstart-gitbranch.yaml @@ -14,6 +14,7 @@ spec: branch: main # Optional: the branch `main` is created from while the remote does not have it. # parentBranch: develop - # Which GitTargets may write through this branch. Its own namespace needs a grant too. + # Which GitTargets may write through this branch. Omitted, it is this one grant: GitTargets in + # this namespace. A list you write is the whole policy, and [] admits nobody. allowedGitTargets: - consumerNamespaces: {from: Same} diff --git a/docs/architecture.md b/docs/architecture.md index e0710f46..10c3b7e5 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -40,7 +40,8 @@ writes the branch, with one credential: the oldest admitted `GitBranch` whose cr push session on the remote holds it, and any other reports `BranchConflict`. A repository written from several namespaces needs no copy of the credential in each: their `GitTarget`s name the shared `GitBranch` with `spec.gitBranchRef.namespace`, admitted by that `GitBranch`'s -`spec.allowedGitTargets`. Nothing is admitted automatically, the own namespace included +`spec.allowedGitTargets`. Omitted, that list admits the `GitBranch`'s own namespace, and a written +list is the whole policy ([configuration.md](configuration.md#sharing-connections-across-namespaces)). Neither kind can change scope later: a CRD's `spec.scope` is immutable once the definition is `Established`, so the change would mean deleting the definition (and every object of the kind with it) and recreating both. @@ -501,7 +502,8 @@ Represents a Git repository and the credentials/configuration used to write it. - `spec.url`: immutable repository URL. - `spec.secretRef`: optional Secret in the same namespace for HTTP/SSH authentication. - `spec.allowedGitBranches`: which namespaces' `GitBranch`es may claim which branches with this - credential, as grants with a `from` and branch patterns. Its own namespace needs a grant too. A + credential, as grants with a `from` and branch patterns. It has no default: its own namespace + needs a grant too. A `GitBranch` it no longer admits gives its branch up, keeping its finalizer. - `spec.knownHostsRef`: optional SSH known hosts source. - `spec.commit.committer`: committer identity (defaults to `GitOps Reverser` / `noreply@configbutler.ai`). diff --git a/docs/attribution-setup-guide.md b/docs/attribution-setup-guide.md index e76a13ab..5e3ff512 100644 --- a/docs/attribution-setup-guide.md +++ b/docs/attribution-setup-guide.md @@ -29,8 +29,8 @@ and watches cold-replay. For a disposable single-pod setup, `attribution.transpo no Redis but loses facts whenever the reverser restarts. It also leaves Redis-backed features such as `CommitRequest` author capture inactive unless you configure Redis separately. -The chart's `KubeProvider`, `local` in the release namespace, admits no namespace by default, its -own included. Grant the namespaces whose `GitTarget`s may mirror through it in +The chart's `KubeProvider`, `local` in the release namespace, admits only the release namespace by +default. Grant the namespaces whose `GitTarget`s may mirror through it in `kubeProvider.allowedGitTargets` (see [the grants](configuration.md#the-grants)), or install with the chart's `values-playground.yaml` to let every namespace mirror its own. See [RBAC](rbac.md) for the controller's read permissions. diff --git a/docs/configuration.md b/docs/configuration.md index 4e789986..d46e78a6 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -82,7 +82,7 @@ Every field but `allowedGitTargets` is immutable: to change one, delete the `Git |---|---|---| | `gitProviderRef` | **required** | The `GitProvider` whose repository and credential write this branch: in this namespace, or `namespace` names another. Its `allowedGitBranches` must admit this namespace and `branch` | | `branch` | **required** | Branch to write. Only one `GitBranch` holds a repository branch; see [who writes a repository branch](#who-writes-a-repository-branch) | -| `allowedGitTargets` | **no `GitTarget`**, its own namespace included | Which namespaces' `GitTarget`s may write through this branch, each grant with a `consumerNamespaces` block. The only mutable field. See [the grants](#the-grants) | +| `allowedGitTargets` | `[{"consumerNamespaces":{"from":"Same"}}]` | Which namespaces' `GitTarget`s may write through this branch, each grant with a `consumerNamespaces` block. Omitted, this branch's own namespace; `[]` admits nobody. The only mutable field. See [the grants](#the-grants) | | `parentBranch` | the remote's default branch | Branch a `branch` the remote does not have yet is created from. Only read, so it need not be allowed. See [starting the write branch from another branch](#starting-the-write-branch-from-another-branch-specparentbranch) | ### `GitReceiver` (namespaced): the endpoint a Git host notifies on a push @@ -100,8 +100,8 @@ Set exactly one of `secretRef` and `inCluster`. Both are immutable: delete and r |---|---|---| | `secretRef` | none; set `inCluster` instead | Secret in this provider's namespace holding a kubeconfig for the source cluster. `key` defaults to `value`, then `value.yaml`. See [`KubeProvider`](#kubeprovider) | | `inCluster` | none; set `secretRef` instead | `{}` uses the operator's own identity. Honored only for a provider listed in `--in-cluster-kube-providers`. See [the operator's own identity](#the-operators-own-identity-specincluster) | -| `allowedGitTargets` | **no `GitTarget`**, its own namespace included | Which namespaces' `GitTarget`s may mirror through this provider, and what: each grant has a `consumerNamespaces` block, and `sourceNamespaces`, `clusterResources` or both. See [the grants](#the-grants) | -| `allowedApplierClaims` | **no applier claim**, its own namespace included | Which namespaces' applier claims may hold an applier through this provider, and the `applierNamespaces` that applier may live in. See [the grants](#the-grants) | +| `allowedGitTargets` | `[{"consumerNamespaces":{"from":"Same"},"sourceNamespaces":["{consumer.namespace}"]}]` | Which namespaces' `GitTarget`s may mirror through this provider, and what: each grant has a `consumerNamespaces` block, and `sourceNamespaces`, `clusterResources` or both. Omitted, this provider's own namespace mirrors the namespace of the same name; `[]` admits nobody. See [the grants](#the-grants) | +| `allowedApplierClaims` | `[{"applierNamespaces":["{consumer.namespace}"],"consumerNamespaces":{"from":"Same"}}]` | Which namespaces' applier claims may hold an applier through this provider, and the `applierNamespaces` that applier may live in. Omitted, this provider's own namespace, for appliers in the namespace of the same name; `[]` admits nobody. See [the grants](#the-grants) | | `client.qps` | `20` | Client rate limit for this source cluster. Refused beside `inCluster` | | `client.burst` | `30` | Client burst for this source cluster. Refused beside `inCluster` | | `attribution.auditRoute` | `.` | Audit route this provider joins. A route is held by one namespace. See [audit route](#audit-route-and-auditfactsreceived) | @@ -291,9 +291,25 @@ object that lends something, and is named `allowed` plus the kind that refers to | `KubeProvider.spec.allowedGitTargets` | `GitTarget`s mirroring with its credential | `sourceNamespaces`, `clusterResources`, or both | | `KubeProvider.spec.allowedApplierClaims` | `FluxClaim`s and `ArgoCDClaim`s holding an applier with its credential | `applierNamespaces` (required) | -- **Nothing is automatic, the object's own namespace included.** An omitted or empty list admits - nobody. GitOps Reverser is stricter than Flux here on purpose: administering a provider and - creating the objects that use it can then carry different RBAC permissions. +- **An omitted list is the same-namespace default; a written list is the whole policy.** On a + `GitBranch` and a `KubeProvider` the API server fills an omitted list with one grant for the + object's own namespace, which you can read back with `kubectl get -o yaml`: + + | You write | The list is | + |---|---| + | nothing | `GitBranch`: `[{consumerNamespaces: {from: Same}}]`. `KubeProvider`: `allowedGitTargets: [{consumerNamespaces: {from: Same}, sourceNamespaces: ["{consumer.namespace}"]}]` and `allowedApplierClaims: [{consumerNamespaces: {from: Same}, applierNamespaces: ["{consumer.namespace}"]}]` | + | a list | exactly that list; the default is not added, so a list without a `from: Same` grant shuts the own namespace out | + | `[]` | nobody, the own namespace included | + + The default grants no cluster-scoped resource, and it reaches only the namespace of the same + name in the provider's cluster. +- **`allowedGitBranches` has no default.** Which branches a namespace may claim is always written + as `branchPatterns`, so an omitted or empty list on a `GitProvider` admits no `GitBranch`, its own + namespace included. +- **The default trusts the object's own namespace.** Whoever may create a `GitTarget` or an applier + claim beside a `KubeProvider` may use its credential within the default scope. To keep + administering a provider apart from using it, give the provider a namespace of its own, or write + `[]` and grant the namespaces you mean. - **Grants combine by union**, and order never matters. - **An explicit scope is the whole scope**, never "these plus your own": `applierNamespaces: [flux-system]` means `flux-system` only. An object's scope is the union of @@ -319,8 +335,8 @@ allowedGitTargets: # on a KubeProvider sourceNamespaces: ["{consumer.namespace}"] # what: namespaces in this provider's cluster ``` -Unlike Gateway API, nothing defaults: `consumerNamespaces` is required on every grant, and so is -its `from`. A grant without the block, with `consumerNamespaces: null`, or with +Unlike Gateway API, nothing inside a grant defaults: `consumerNamespaces` is required on every +grant you write, and so is its `from`. A grant without the block, with `consumerNamespaces: null`, or with `consumerNamespaces: {}` is refused. | `from` | Admits | Requires | @@ -569,8 +585,8 @@ that wants an endpoint of its own creates a `GitReceiver` in its own namespace. Nothing has a default. The operator cannot infer a safe repository, branch, or write credential, nor which cluster a folder means, so every `GitTarget` names its `GitBranch` and its `KubeProvider`. -Nothing falls back to the provider the chart renders, and that provider grants nothing until the -installer says so. +Nothing falls back to the provider the chart renders, and that provider admits only the release +namespace's own objects until the installer grants more. ## `GitProvider` @@ -584,7 +600,7 @@ The important fields are: - `spec.secretRef.name`: Secret with Git credentials such as SSH or HTTPS auth - `spec.knownHostsRef`: optional ConfigMap/Secret with SSH `known_hosts` shared across providers - `spec.allowedGitBranches`: which namespaces' `GitBranch`es may claim which branches with this - credential ([the grants](#the-grants)); its own namespace needs a grant too + credential ([the grants](#the-grants)). It has no default: its own namespace needs a grant too - `spec.commit`: committer identity and signing How writes are batched and phrased is **not** here: `spec.commit.window` and `spec.commit.message` @@ -945,7 +961,8 @@ before it uses it, without dialing the cluster, and reports the verdict as `Vali | `KubeConfigInsecureTLSNotAllowed` | sets `insecure-skip-tls-verify`. Allowed with `--insecure-kubeconfig-tls` | | `KubeConfigFileReferenceNotAllowed` | names a token, certificate or key by file path, which the operator would read from its own Pod. Always refused: embed the `*-data` form instead | -A `KubeProvider` is used only through its [grants](#the-grants), its own namespace included: +A `KubeProvider` is used only through its [grants](#the-grants). Omitted, each list is one grant +for the provider's own namespace, reaching the namespace of the same name; `[]` admits nobody: - **`spec.allowedGitTargets`** admits `GitTarget`s to mirror through it. The union of the admitting grants' `sourceNamespaces` bounds which namespaces the `WatchRule`s feeding a target may name, and @@ -1004,7 +1021,9 @@ remote one: there the config plane *is* the watched cluster, so a scope delibera namespace RBAC and lets the owner of an admitted object act on the namespaces in its scope through the operator's own credential. A grant is a delegation, not owner consent: it does not prove that a namespace owns the objects it reaches, or the repository its `GitTarget` writes. That is legitimate -to grant on purpose, which is why the chart's `local` provider grants nothing by default. +to grant on purpose, which is why the chart's `local` provider grants only its default: objects in +the release namespace, reaching the release namespace. Set `kubeProvider.allowedGitTargets: []` and +`kubeProvider.allowedApplierClaims: []` to admit nobody. `spec.secretRef` and `spec.inCluster` are immutable, switching from one to the other included, and so is `GitTarget.spec.kubeProviderRef`: changing either would silently make an existing diff --git a/docs/definitions.md b/docs/definitions.md index f1300067..40444872 100644 --- a/docs/definitions.md +++ b/docs/definitions.md @@ -64,7 +64,9 @@ The two directions, which share the word "source" and must not share anything el - **Access** is the control-cluster direction: which control-cluster namespaces may reference an object that lends something (a `GitProvider`, a `GitBranch`, a `KubeProvider`). It is granted on - the referenced object, in a **grant** list, and nothing is automatic, the own namespace included. + the referenced object, in a **grant** list. An omitted list on a `GitBranch` or a `KubeProvider` + is one grant for the object's own namespace; a written list is the whole policy, and `[]` admits + nobody. A `GitProvider`'s list has no default. - **Source namespace** is the source-cluster direction: which source-cluster namespace a rule reads objects from. `WatchRule.spec.rules[].sourceNamespace`, bounded by the union of the `sourceNamespaces` of the `KubeProvider` grants that admit the `GitTarget`. diff --git a/docs/finished/provider-access-rules.md b/docs/finished/provider-access-rules.md index 9f4a5e4c..4ada02a6 100644 --- a/docs/finished/provider-access-rules.md +++ b/docs/finished/provider-access-rules.md @@ -107,6 +107,11 @@ consumers can then carry different RBAC permissions. | Who owns a grant list? | One policy owner per object: every grant list is `+listType=atomic`, and `names`, `branches`, `sourceNamespaces`, `clusterResources` and `applierNamespaces` are `+listType=set` | | Are there booleans? | No. A grant's source is an enum (`from`), and every scope is a list, so either can grow a value later without a new field ([API conventions](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md): think twice about bool fields) | +> **Since changed:** the first two answers no longer hold for `GitBranch` and `KubeProvider`. An +> omitted list there is one `from: Same` grant for the object's own namespace, a written list is +> the whole policy, and `[]` admits nobody; `GitProvider.spec.allowedGitBranches` keeps no default. +> See [the grants](../configuration.md#the-grants). + The field is `allowed` plus the referring kind, or a family of kinds the API already defines, followed by a `from` in each grant. That is Gateway API's `allowedRoutes.namespaces.from`, the shape `from` is borrowed from. "Applier claim" is such a family: there is no `ApplierClaim` kind, diff --git a/docs/quickstart.md b/docs/quickstart.md index ef078593..2486e4c8 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -150,7 +150,7 @@ kubectl describe gitprovider,gittarget,watchrule -n gitops-reverser-quickstart-d Two `GitTarget` conditions stop the data plane and are worth recognizing: `KubeProviderNotFound` (the `local` `KubeProvider` in `gitops-reverser` is missing) and `NamespaceNotAuthorized` (no grant in its `spec.allowedGitTargets` admits the demo namespace). `quickstart.enabled=true` adds the grant -for `quickstart.namespace`; the chart grants nothing else by default. +for `quickstart.namespace` in place of the release-namespace default. ## Clean up diff --git a/hack/crdfields/fields.yaml b/hack/crdfields/fields.yaml index 5774808d..de2573cf 100644 --- a/hack/crdfields/fields.yaml +++ b/hack/crdfields/fields.yaml @@ -70,10 +70,10 @@ kinds: Branch to write. Only one `GitBranch` holds a repository branch; see [who writes a repository branch](#who-writes-a-repository-branch) - path: allowedGitTargets - default: "**no `GitTarget`**, its own namespace included" doc: >- Which namespaces' `GitTarget`s may write through this branch, each grant with a - `consumerNamespaces` block. The only mutable field. See [the grants](#the-grants) + `consumerNamespaces` block. Omitted, this branch's own namespace; `[]` admits nobody. The + only mutable field. See [the grants](#the-grants) - path: parentBranch default: the remote's default branch doc: >- @@ -109,16 +109,16 @@ kinds: `--in-cluster-kube-providers`. See [the operator's own identity](#the-operators-own-identity-specincluster) - path: allowedGitTargets - default: "**no `GitTarget`**, its own namespace included" doc: >- Which namespaces' `GitTarget`s may mirror through this provider, and what: each grant has a - `consumerNamespaces` block, and `sourceNamespaces`, `clusterResources` or both. See - [the grants](#the-grants) + `consumerNamespaces` block, and `sourceNamespaces`, `clusterResources` or both. Omitted, + this provider's own namespace mirrors the namespace of the same name; `[]` admits nobody. + See [the grants](#the-grants) - path: allowedApplierClaims - default: "**no applier claim**, its own namespace included" doc: >- Which namespaces' applier claims may hold an applier through this provider, and the - `applierNamespaces` that applier may live in. See [the grants](#the-grants) + `applierNamespaces` that applier may live in. Omitted, this provider's own namespace, for + appliers in the namespace of the same name; `[]` admits nobody. See [the grants](#the-grants) - path: client.qps default: "`20`" doc: Client rate limit for this source cluster. Refused beside `inCluster` diff --git a/internal/authz/kubeprovider_admission_test.go b/internal/authz/kubeprovider_admission_test.go index 7e6dca47..fc256672 100644 --- a/internal/authz/kubeprovider_admission_test.go +++ b/internal/authz/kubeprovider_admission_test.go @@ -47,12 +47,21 @@ func targetIn(providerName string) *configv1alpha3.GitTarget { return t } +// grantList is grants written out, never omitted: no grants is [], which admits nobody, where an +// omitted list would be the default grant for the provider's own namespace. +func grantList[G any](grants []G) []G { + if grants == nil { + return []G{} + } + return grants +} + // providerNamed builds a KubeProvider in platformNS with grants as its spec.allowedGitTargets, so // the GitTarget in testNS is admitted only through a grant. func providerNamed(name string, grants ...configv1alpha3.KubeProviderGitTargetGrant) *configv1alpha3.KubeProvider { return &configv1alpha3.KubeProvider{ ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: platformNS}, - Spec: configv1alpha3.KubeProviderSpec{AllowedGitTargets: grants}, + Spec: configv1alpha3.KubeProviderSpec{AllowedGitTargets: grantList(grants)}, } } @@ -84,7 +93,7 @@ func TestGitTargetAdmitted_Policy(t *testing.T) { ownNamespace := func(grants ...configv1alpha3.KubeProviderGitTargetGrant) *configv1alpha3.KubeProvider { return &configv1alpha3.KubeProvider{ ObjectMeta: metav1.ObjectMeta{Name: "local", Namespace: testNS}, - Spec: configv1alpha3.KubeProviderSpec{AllowedGitTargets: grants}, + Spec: configv1alpha3.KubeProviderSpec{AllowedGitTargets: grantList(grants)}, } } ownTarget := &configv1alpha3.GitTarget{ @@ -119,7 +128,7 @@ func TestGitTargetAdmitted_Policy(t *testing.T) { }, { // Nothing is automatic, the provider's own namespace included. - name: "the provider's own namespace is refused when no grant names it", + name: "the provider's own namespace is refused by an empty list", objects: []client.Object{ownNamespace(), namespaceLabeled(nil)}, target: ownTarget, wantReason: ReasonNamespaceNotAuthorized, @@ -146,7 +155,24 @@ func TestGitTargetAdmitted_Policy(t *testing.T) { wantMessage: `KubeProvider platform/prod-eu-1 admits platform in spec.allowedGitTargets; namespace "team-a"`, }, { - name: "an omitted list denies every namespace", + name: "an omitted list admits the provider's own namespace, mirroring its own", + objects: []client.Object{&configv1alpha3.KubeProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "local", Namespace: testNS}, + }, namespaceLabeled(nil)}, + target: ownTarget, + wantAllowed: true, + }, + { + name: "an omitted list admits no other namespace", + objects: []client.Object{&configv1alpha3.KubeProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "prod-eu-1", Namespace: platformNS}, + }, namespaceLabeled(nil)}, + target: targetIn("prod-eu-1"), + wantReason: ReasonNamespaceNotAuthorized, + wantMessage: "admits platform in spec.allowedGitTargets", + }, + { + name: "an empty list denies every namespace", objects: []client.Object{providerNamed("prod-eu-1"), namespaceLabeled(nil)}, target: targetIn("prod-eu-1"), wantReason: ReasonNamespaceNotAuthorized, @@ -345,7 +371,7 @@ func TestApplierClaimAdmitted(t *testing.T) { own := []configv1alpha3.ScopeNamespace{"{consumer.namespace}"} provider := func(grants ...configv1alpha3.KubeProviderApplierClaimGrant) *configv1alpha3.KubeProvider { p := providerNamed("hub", mirrorFrom(testNS)) - p.Spec.AllowedApplierClaims = grants + p.Spec.AllowedApplierClaims = grantList(grants) return p } tests := []struct { @@ -395,9 +421,9 @@ func TestApplierClaimAdmitted(t *testing.T) { } } -// TestApplierClaimAdmitted_OwnNamespaceNeedsAGrant: a provider admits no applier claim in its own -// namespace by itself, also one it admits to mirror. -func TestApplierClaimAdmitted_OwnNamespaceNeedsAGrant(t *testing.T) { +// TestApplierClaimAdmitted_OwnNamespace: an omitted allowedApplierClaims admits an applier claim in +// the provider's own namespace; [] refuses it, also one the provider admits to mirror. +func TestApplierClaimAdmitted_OwnNamespace(t *testing.T) { provider := &configv1alpha3.KubeProvider{ ObjectMeta: metav1.ObjectMeta{Name: "local", Namespace: testNS}, Spec: configv1alpha3.KubeProviderSpec{AllowedGitTargets: []configv1alpha3.KubeProviderGitTargetGrant{ @@ -407,6 +433,7 @@ func TestApplierClaimAdmitted_OwnNamespaceNeedsAGrant(t *testing.T) { }, }}, } + provider.Spec.AllowedApplierClaims = []configv1alpha3.KubeProviderApplierClaimGrant{} claim := &configv1alpha3.FluxClaim{ ObjectMeta: metav1.ObjectMeta{Namespace: testNS, Name: "apps"}, Spec: configv1alpha3.FluxClaimSpec{ @@ -423,6 +450,15 @@ func TestApplierClaimAdmitted_OwnNamespaceNeedsAGrant(t *testing.T) { assert.False(t, decision.Allowed) assert.Equal(t, ReasonNamespaceNotAuthorized, decision.Reason) assert.Contains(t, decision.Message, "KubeProvider team-a/local admits no namespace in spec.allowedApplierClaims") + + provider.Spec.AllowedApplierClaims = nil + reader = fake.NewClientBuilder(). + WithScheme(admissionScheme(t)). + WithObjects(provider, namespaceLabeled(nil)). + Build() + decision, err = ApplierClaimAdmitted(context.Background(), reader, claim) + require.NoError(t, err) + assert.True(t, decision.Allowed, "omitted, the default grant admits the provider's own namespace") } // TestApplierAdmitted: asking an applier to apply needs both admissions, always. Mirroring and @@ -430,7 +466,7 @@ func TestApplierClaimAdmitted_OwnNamespaceNeedsAGrant(t *testing.T) { func TestApplierAdmitted(t *testing.T) { own := []configv1alpha3.ScopeNamespace{"{consumer.namespace}"} withAppliers := func(p *configv1alpha3.KubeProvider, grants ...configv1alpha3.KubeProviderApplierClaimGrant) *configv1alpha3.KubeProvider { - p.Spec.AllowedApplierClaims = grants + p.Spec.AllowedApplierClaims = grantList(grants) return p } tests := []struct { diff --git a/internal/controller/gittarget_gitbranch_test.go b/internal/controller/gittarget_gitbranch_test.go index f3dc8cb2..ce01343f 100644 --- a/internal/controller/gittarget_gitbranch_test.go +++ b/internal/controller/gittarget_gitbranch_test.go @@ -129,13 +129,14 @@ func TestReconcile_ATargetOnAHeldGitBranchInAnotherNamespaceIsWired(t *testing.T } // TestReconcile_AGitBranchThatDoesNotAdmitTheNamespace: who may write through a GitBranch is decided -// by the GitBranch's spec.allowedGitTargets, against the TARGET's namespace. +// by the GitBranch's spec.allowedGitTargets, against the TARGET's namespace. [] survives the update +// as [], so it admits nobody instead of reading back as the omitted, same-namespace default. func TestReconcile_AGitBranchThatDoesNotAdmitTheNamespace(t *testing.T) { f := newDestinationFixture(t) branch := &configbutleraiv1alpha3.GitBranch{} require.NoError(t, f.client.Get(context.Background(), k8stypes.NamespacedName{Namespace: "platform", Name: "main"}, branch)) - branch.Spec.AllowedGitTargets = nil + branch.Spec.AllowedGitTargets = []configbutleraiv1alpha3.GitBranchGitTargetGrant{} require.NoError(t, f.client.Update(context.Background(), branch)) _, got := f.reconcile(t) diff --git a/internal/controller/gittarget_source_cluster_test.go b/internal/controller/gittarget_source_cluster_test.go index 37fa95b4..42ccefbf 100644 --- a/internal/controller/gittarget_source_cluster_test.go +++ b/internal/controller/gittarget_source_cluster_test.go @@ -287,8 +287,17 @@ func TestCheckSourceAuthorization_OwnNamespaceAndAuditRoute(t *testing.T) { assert.True(t, ok) }) - t.Run("own namespace is refused with no grant", func(t *testing.T) { + t.Run("own namespace is admitted by an omitted list", func(t *testing.T) { cl := fake.NewClientBuilder().WithScheme(scScheme(t)).WithObjects(inClusterKubeProvider("local")).Build() + ok, _, _, err := (&GitTargetReconciler{Client: cl}).checkSourceAuthorization(context.Background(), target) + require.NoError(t, err) + assert.True(t, ok) + }) + + t.Run("own namespace is refused by an empty list", func(t *testing.T) { + denyAll := inClusterKubeProvider("local") + denyAll.Spec.AllowedGitTargets = []configbutleraiv1alpha3.KubeProviderGitTargetGrant{} + cl := fake.NewClientBuilder().WithScheme(scScheme(t)).WithObjects(denyAll).Build() ok, reason, msg, err := (&GitTargetReconciler{Client: cl}).checkSourceAuthorization( context.Background(), target) require.NoError(t, err) diff --git a/internal/controller/grant_defaults_apiserver_test.go b/internal/controller/grant_defaults_apiserver_test.go new file mode 100644 index 00000000..d767e175 --- /dev/null +++ b/internal/controller/grant_defaults_apiserver_test.go @@ -0,0 +1,105 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + + configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +// This is a claim about the APISERVER and the wire format together, so it is pinned by execution: +// an omitted grant list on GitBranch and KubeProvider reads back as the CRD default, and [] reads +// back as [] through a create and a full update. The Go fields carry no omitempty, so a nil list +// goes out as null, which the API server prunes and then defaults, and an empty list goes out as +// []. With omitempty, any Update of the object (a finalizer, a label) would send [] as omitted, and +// an explicit deny-all would come back as the default grant. +func TestGrantLists_APIServerDefaultsOmittedAndKeepsEmpty(t *testing.T) { + env := &envtest.Environment{ + CRDDirectoryPaths: []string{"../../config/crd/bases"}, + ErrorIfCRDPathMissing: true, + } + cfg, err := env.Start() + if err != nil { + t.Fatalf("start envtest control plane: %v", err) + } + t.Cleanup(func() { _ = env.Stop() }) + + sch := runtime.NewScheme() + require.NoError(t, scheme.AddToScheme(sch)) + require.NoError(t, configbutleraiv1alpha3.AddToScheme(sch)) + c, err := client.New(cfg, client.Options{Scheme: sch}) + require.NoError(t, err) + ctx := context.Background() + + branch := func(name string, grants []configbutleraiv1alpha3.GitBranchGitTargetGrant) *configbutleraiv1alpha3.GitBranch { + b := &configbutleraiv1alpha3.GitBranch{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: name}, + Spec: configbutleraiv1alpha3.GitBranchSpec{Branch: "main", AllowedGitTargets: grants}, + } + b.Spec.GitProviderRef.Name = "repo" + return b + } + provider := func(name string) *configbutleraiv1alpha3.KubeProvider { + return &configbutleraiv1alpha3.KubeProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: name}, + Spec: configbutleraiv1alpha3.KubeProviderSpec{ + InCluster: &configbutleraiv1alpha3.KubeProviderInCluster{}, + }, + } + } + relabel := func(t *testing.T, obj client.Object) { + t.Helper() + obj.SetLabels(map[string]string{"touched": "true"}) + require.NoError(t, c.Update(ctx, obj)) + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(obj), obj)) + } + + t.Run("an omitted GitBranch list reads back as the default", func(t *testing.T) { + obj := branch("omitted", nil) + require.NoError(t, c.Create(ctx, obj)) + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(obj), obj)) + assert.Equal(t, configbutleraiv1alpha3.DefaultGitBranchGitTargetGrants(), obj.Spec.AllowedGitTargets) + }) + + t.Run("an empty GitBranch list stays empty through an update", func(t *testing.T) { + obj := branch("deny-all", []configbutleraiv1alpha3.GitBranchGitTargetGrant{}) + require.NoError(t, c.Create(ctx, obj)) + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(obj), obj)) + require.NotNil(t, obj.Spec.AllowedGitTargets) + assert.Empty(t, obj.Spec.AllowedGitTargets) + + relabel(t, obj) + require.NotNil(t, obj.Spec.AllowedGitTargets, "an update must not turn [] into the default") + assert.Empty(t, obj.Spec.AllowedGitTargets) + }) + + t.Run("omitted KubeProvider lists read back as the defaults", func(t *testing.T) { + obj := provider("omitted") + require.NoError(t, c.Create(ctx, obj)) + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(obj), obj)) + assert.Equal(t, configbutleraiv1alpha3.DefaultKubeProviderGitTargetGrants(), obj.Spec.AllowedGitTargets) + assert.Equal(t, configbutleraiv1alpha3.DefaultKubeProviderApplierClaimGrants(), obj.Spec.AllowedApplierClaims) + }) + + t.Run("empty KubeProvider lists stay empty through an update", func(t *testing.T) { + obj := provider("deny-all") + obj.Spec.AllowedGitTargets = []configbutleraiv1alpha3.KubeProviderGitTargetGrant{} + obj.Spec.AllowedApplierClaims = []configbutleraiv1alpha3.KubeProviderApplierClaimGrant{} + require.NoError(t, c.Create(ctx, obj)) + relabel(t, obj) + require.NotNil(t, obj.Spec.AllowedGitTargets) + assert.Empty(t, obj.Spec.AllowedGitTargets) + require.NotNil(t, obj.Spec.AllowedApplierClaims) + assert.Empty(t, obj.Spec.AllowedApplierClaims) + }) +} From d788473505716c4c50c79fdfbf2812c1bff88542 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Sat, 24 Oct 2026 08:26:50 +0000 Subject: [PATCH 4/4] docs: address review on the upgrade guide and the moved design docs The KubeProvider and GitBranch entries now match the same-namespace default, the trigger deletion step separates Flux from Argo CD, the released Reapply entry says which names it uses, versioned headings drop the em dash, and the moved design docs state what Fix 1 made visible. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/UPGRADING.md | 71 ++++++++++--------- docs/design/e2e-suite-trim.md | 3 +- ...t-dropped-while-render-fidelity-pending.md | 9 +-- docs/finished/provider-access-rules.md | 2 +- docs/future/gitops-api-open-asks.md | 8 +-- 5 files changed, 51 insertions(+), 42 deletions(-) diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 34ce8c64..c895d68b 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -25,10 +25,12 @@ the new object takes it over on its first snapshot. kubectl get gitproviders,gittargets,fluxtriggers,argocdtriggers -A -o yaml > before-upgrade.yaml ``` -2. Delete every `FluxTrigger` and `ArgoCDTrigger` while 0.53.0 still runs, so it removes the - finalizer and hands each `Kustomization` back to Flux. Then delete their CRDs: - `kubectl delete crd fluxtriggers.configbutler.ai argocdtriggers.configbutler.ai`. Until a claim - takes over in step 6, Flux and Argo CD apply on their own. +2. Delete every `FluxTrigger` and `ArgoCDTrigger` while 0.53.0 still runs. A `FluxTrigger` removes + its finalizer and hands its `Kustomization` back to Flux, resumed, so Flux applies it on its own + until step 6. An `ArgoCDTrigger` holds no finalizer and changes nothing on delete: its + `Application` keeps automated sync off, as you set it for 0.53.0, so Argo CD syncs it only when + someone starts a sync until step 6. Then delete their CRDs: + `kubectl delete crd fluxtriggers.configbutler.ai argocdtriggers.configbutler.ai`. 3. Upgrade the CRDs, the operator and its permissions. The chart's `clusterProvider.*` values are `kubeProvider.*`, and the provider it renders is a `KubeProvider` named `local` in the release namespace, which admits only the release namespace's own objects until you set its grants (see @@ -95,9 +97,9 @@ and `controllerManager.inClusterKubeProviders` adds others. Any other provider t `spec.inCluster` reports `Validated=False` and `Stalled=True` (`InClusterNotAuthorized`) and is never used. `spec.client` is refused beside `spec.inCluster`, where it was ignored. -**`accessFrom` and `allowCrossNamespace` are replaced by grants.** A `KubeProvider` admits nobody -until a grant in `spec.allowedGitTargets` or `spec.allowedApplierClaims` does; see "Grants say who -may use a provider or a branch". +**`accessFrom` and `allowCrossNamespace` are replaced by grants.** Omitted, a `KubeProvider`'s +`spec.allowedGitTargets` and `spec.allowedApplierClaims` admit only its own namespace; a written list +is the whole policy, and `[]` admits nobody. See "Grants say who may use a provider or a branch". **Audit routes default to `.`, so the audit webhook URL changes.** The chart's provider posts under `/audit-webhook/.local`, `/audit-webhook/gitops-reverser.local` @@ -138,8 +140,8 @@ kind: GitTarget metadata: {name: apps, namespace: platform} spec: gitBranchRef: {name: platform-main} - kubeProviderRef: {name: local, namespace: gitops-reverser} - path: apps + kubeProviderRef: {name: platform-cluster} # a KubeProvider in this namespace, which its omitted + path: apps # grants admit ``` **One `GitBranch` holds a repository branch.** It holds it once a grant on its `GitProvider` admits @@ -218,8 +220,8 @@ spec: {} # allowedGitTargets omitted: this namespace's G --- kind: KubeProvider spec: - allowedGitTargets: # was accessFrom: {selector: {}} without allowCrossNamespace; - - consumerNamespaces: {from: All} # omitted, only this namespace's GitTargets + allowedGitTargets: # was accessFrom: {selector: {}} without allowCrossNamespace: + - consumerNamespaces: {from: All} # every namespace's GitTargets, each mirroring its own sourceNamespaces: ["{consumer.namespace}"] allowedApplierClaims: - consumerNamespaces: {from: All} @@ -418,6 +420,11 @@ default stored on every provider, is accepted. ## `onRefusal: PushEmptyCommit` is now `onRefusal: Reapply`, and needs an apply trigger +This entry is as 0.53.0 shipped it, with 0.53.0's names. Upgrading past 0.53.0, read +`FluxTrigger`, `ArgoCDTrigger` and `spec.applyTriggerRef` as the claims of +[the rename above](#fluxtrigger-and-argocdtrigger-are-fluxclaim-and-argocdclaim), and step 3's +`allowCrossNamespace.appliers` as a grant in `allowedApplierClaims`. + **Breaking.** `GitTarget.spec.onRefusal` accepts `Ignore` and `Reapply`. `PushEmptyCommit` is gone: GitOps Reverser no longer moves the branch with an empty commit when an edit is refused. With `Reapply` it asks the target's applier to apply the current tip once the branch holds no open @@ -2206,7 +2213,7 @@ different one the namespace is now written explicitly — omitting it would have kustomize and rendered a different object than the one being mirrored. This also now applies to a path you declared, which previously wrote a `namespace:` line the rest of that folder omits. -## 0.41.1 — the Go floor rises for module consumers (patch release; breaking for module consumers) +## 0.41.1: the Go floor rises for module consumers (patch release; breaking for module consumers) `v0.41.1` raises the `go` directive in `go.mod` from `1.26.5` to `1.27.0`, in a release whose changelog entry reads only *"let's release some security updates"*. The floor rises again after it. @@ -2245,7 +2252,7 @@ A `go` directive move is treated as consumer-breaking for module users. It gets line in the release notes even when it ships in a patch release, so it is never again something you discover from a failing build. -## 0.41.0 — attribution facts travel on a selectable transport, and Redis is no longer implied +## 0.41.0: attribution facts travel on a selectable transport, and Redis is no longer implied Attribution stopped meaning Redis. The audit receiver appends its facts to a per-type **stream**, the watch side follows the streams for the types it watches into a bounded in-process index, and @@ -2277,7 +2284,7 @@ New tuning knobs, all with behaviour-preserving defaults: `attribution.maxFactsP (10000). They bound the in-process index and the collection join; see [configuration.md](configuration.md). -## 0.41.0 — the attribution metrics are relabeled and partly renamed (breaking for queries) +## 0.41.0: the attribution metrics are relabeled and partly renamed (breaking for queries) The `result` label is **gone** from `gitopsreverser_attribution_resolutions_total` and `gitopsreverser_attribution_resolution_wait_seconds`. It crammed two orthogonal questions into one @@ -2450,7 +2457,7 @@ is the acceptance gate's own issue" compile-checked rather than merely stated. - Record the `status.generator.version` you consumed a report with, if you pin our release elsewhere. That is the whole point of the field. -## 0.39.2 — status vocabulary and two removed status fields (status-only change) +## 0.39.2: status vocabulary and two removed status fields (status-only change) Nothing in `spec` changed and no manifest you wrote needs editing. Three things a **reader of status** may notice: @@ -2495,7 +2502,7 @@ columns (`Ready`, `Reason`, `Streams`, `Age`); `Provider`, `Branch` and `Path` m visible in `kubectl describe` and routable by anything that consumes Events. The operator's ServiceAccount gained `create`/`patch` on `events` for it, which `helm upgrade` applies. -## 0.39.0 — a resync no longer deletes Git documents by default (behavior change) +## 0.39.0: a resync no longer deletes Git documents by default (behavior change) **`GitTarget` gained `spec.prune.mode`, and its effective default changes what a resync does.** Previously every resync mark-and-swept: a managed document whose resource was absent from the @@ -2551,7 +2558,7 @@ This ships in the **same release** as the rule-kind scope change below, and is w migration non-destructive: a converted `WatchRule` that resolves to a narrower set of namespaces than you intended leaves the affected documents in Git instead of deleting them. -## 0.39.0 — scope is now carried by the rule kind (breaking) +## 0.39.0: scope is now carried by the rule kind (breaking) **Scope moved from a per-rule field onto the rule KIND.** `WatchRule` is the namespaced surface and gained `spec.rules[].sourceNamespace`; `ClusterWatchRule` is cluster-scoped only and lost its scope @@ -2651,7 +2658,7 @@ The same skew exists inside a rolling upgrade: CRDs are cluster-wide, so an old new `WatchRule`, ignore `rules[].sourceNamespace`, and mirror the wrong namespace's content into Git. **Complete the controller rollout before applying migrated manifests.** -## 0.39.0 — unresolved attribution is visible in Git (author identity changes) +## 0.39.0: unresolved attribution is visible in Git (author identity changes) When `attribution.enabled=true` and the operator cannot match a live watch event to an audit fact within `attribution.grace`, the Git author is now: @@ -2674,7 +2681,7 @@ be attributable, this author normally means the audit policy, webhook route, sou connectivity, or grace window is not configured correctly. It can also represent a change with no usable audit actor, so use the audit metrics before assigning a cause. -## 0.37.0 — a `patches:` block no longer refuses the folder (more folders accepted) +## 0.37.0: a `patches:` block no longer refuses the folder (more folders accepted) A kustomization declaring `patches:` used to refuse the whole `GitTarget`. Not the edit — the **target**. A folder whose patch touched a replica count also lost `images:`/`replicas:` @@ -2702,7 +2709,7 @@ Exactly one shape is tolerated. The rest are refused **by name**, so the message | a `path:` naming no file in the tree, or escaping it | refused: `patches-outside-tree` | | `patchesStrategicMerge:`, `patchesJson6902:` (deprecated spellings) | refused under their own names | -## 0.37.0 — the build's output stops leaking into the build's input (bug fix + behavior change) +## 0.37.0: the build's output stops leaking into the build's input (bug fix + behavior change) **If your kustomization declares `labels:`, `commonLabels:`, `commonAnnotations:` or `namespace:`, the operator has been writing those injected values into your source manifests.** Measured, on a @@ -2748,7 +2755,7 @@ to say which of the source's bytes you meant to keep, and pairing the lists by p measurably wrong (kustomize *prepends* a container a patch adds), so the operator refuses rather than guesses. -## 0.36.0 — kustomize decides what it renders, and what it touched (bug fixes + behavior change) +## 0.36.0: kustomize decides what it renders, and what it touched (bug fixes + behavior change) The write path no longer contains a re-implementation of kustomize's image and replica transformers. It asks kustomize what a folder renders to, and — by rendering a second time @@ -2792,7 +2799,7 @@ worked. If you see this refusal, the live state cannot be expressed in the repos stands — most often because something we do not model (a `patches:` block, a `replacements:` entry) owns the field. The refusal names it. -## 0.36.0 — a folder kustomize cannot build is now refused (behavior change) +## 0.36.0: a folder kustomize cannot build is now refused (behavior change) The analyzer now **builds** every render root with kustomize, instead of only parsing the kustomization structurally. A root that fails to build refuses the `GitTarget`, quoting @@ -2829,7 +2836,7 @@ protecting it. now fails the target with `GitPathAccepted=False` / `UnsupportedContent` instead of writing into a folder whose render it cannot know. Fix the build. -## 0.36.0 — a `digest:` override no longer strips the tag out of your source file (bug fix) +## 0.36.0: a `digest:` override no longer strips the tag out of your source file (bug fix) **If any of your kustomizations use `images:` with `digest:`, or `newTag:` on an image that carries a digest, the operator has been rewriting your source manifests. This stops.** @@ -2855,7 +2862,7 @@ concluded the user had *removed* the tag, and wrote the tag out of the source do - Nothing to configure. The behaviour is simply correct now, and pinned against a real `kustomize build` so it cannot regress. -## 0.36.0 — `kustomization.yaml` is now read by kustomize itself (behavior change) +## 0.36.0: `kustomization.yaml` is now read by kustomize itself (behavior change) The analyzer used to decode `kustomization.yaml` with a hand-written walk over a generic YAML map, checked against a hand-maintained list of 17 unsupported keys. It now decodes with @@ -2886,7 +2893,7 @@ every case the new behaviour is the one that agrees with the renderer: in Git; edit the source field instead. - Nothing to do for the last two rows: they only ever accept more. -## 0.35.0 — `pkg/manifestanalyzer`: the overlay fan-out refusal code was renamed (breaking for consumers) +## 0.35.0: `pkg/manifestanalyzer`: the overlay fan-out refusal code was renamed (breaking for consumers) One refusal reason changed its name, in both the Go constant and the machine-readable value it carries: @@ -2909,7 +2916,7 @@ our planning docs; the new one says what it means. a `switch` or `if` on `"overlay-fan-out-needs-f2"` simply stops matching, and the refusal falls through to whatever your default branch does. Grep for the old string. -## 0.34.0 — the wildcard cluster read moved to its own, droppable ClusterRole (behavior change) +## 0.34.0: the wildcard cluster read moved to its own, droppable ClusterRole (behavior change) The shipped RBAC now says what the binary actually does. Nothing is taken away from a default install; what changes is that the parts can be separated. @@ -2959,7 +2966,7 @@ reflector retried the denial forever. Discovery reports what the server serves, may read, so a narrowed role reached this path — which is exactly the path this release makes easy to enter. -## 0.34.0 — `manifest-analyzer` scan modes renamed, and `--format json` now emits a versioned contract (breaking) +## 0.34.0: `manifest-analyzer` scan modes renamed, and `--format json` now emits a versioned contract (breaking) The analyzer's machine-readable output moved to the new public [`pkg/manifestanalyzer`](../pkg/manifestanalyzer), which is a supported Go API and the single @@ -3006,7 +3013,7 @@ release you are upgrading *across*, not as current advice.) adopt (or refuse) the folder. - If you parsed `plan` from folder-scan mode, you were reading an empty object; drop the field. -## 0.32.0 — chart defaults now run Redis-free (behavior change) +## 0.32.0: chart defaults now run Redis-free (behavior change) The Helm chart now defaults to the simple, Redis-free `configured-author` path, so a bare `helm install` comes up healthy without external infrastructure: @@ -3032,7 +3039,7 @@ The Helm chart now defaults to the simple, Redis-free `configured-author` path, - `helm upgrade --reuse-values` preserves your existing settings, so reused-value upgrades are unaffected; only fresh installs (or upgrades that re-specify values) pick up the new defaults. -## 0.29.0 — API group version bumped `v1alpha2` → `v1alpha3` (breaking) +## 0.29.0: API group version bumped `v1alpha2` → `v1alpha3` (breaking) The served API version moved from `configbutler.ai/v1alpha2` to `configbutler.ai/v1alpha3` to reflect the accumulated schema and status changes on this branch. `v1alpha2` is **removed**, not @@ -3051,7 +3058,7 @@ CRDs are applied. - `kubectl` commands that pin the version (`kubectl get gittargets.v1alpha2.configbutler.ai`) must switch to `v1alpha3`. Unqualified short names (`kubectl get gittargets`) need no change. -## 0.29.0 — first-run and status surface cleanup (breaking) +## 0.29.0: first-run and status surface cleanup (breaking) This branch changes the default install to be easier to try, and it tightens the v1alpha3 status surface around conditions. Existing installs should check the items below before upgrading. @@ -3157,7 +3164,7 @@ kubectl wait --for=condition=StreamsRunning=true gittarget/ -n `WatchRule` and `ClusterWatchRule` use the same condition vocabulary for source readiness (`StreamsRunning`) and referenced target readiness (`GitTargetReady`). -## 0.29.0 — Config flag naming pass (breaking) +## 0.29.0: Config flag naming pass (breaking) Controller command-line flags were renamed to follow [`config-flag-conventions.md`](config-flag-conventions.md). The Helm chart and the @@ -3181,7 +3188,7 @@ Redis/Valkey over TLS unless told otherwise. The Helm chart in-cluster Valkey. **If you run the controller directly against a plaintext Redis, add `--redis-insecure`** — otherwise startup fails on a TLS handshake. -## 0.28.0 — Git credentials interop (breaking) +## 0.28.0: Git credentials interop (breaking) Two user-visible breaking changes land together. Both come from [`design/git-credentials-interop.md`](finished/git-credentials-interop.md). diff --git a/docs/design/e2e-suite-trim.md b/docs/design/e2e-suite-trim.md index a4bcc360..6fb6024e 100644 --- a/docs/design/e2e-suite-trim.md +++ b/docs/design/e2e-suite-trim.md @@ -15,7 +15,8 @@ is longer than nearly every bound, so an event-wiring regression fails loudly. The leg tests the developer Taskfile's stamp chain (rebuild, k3d import, rollout), not the product, and 266s of its 369s suite is the BeforeSuite rebuild. Gate it on a path filter in [`ci.yml`](../../.github/workflows/ci.yml): `Taskfile*.yml`, `test/e2e/Taskfile-e2e.yml`, -`Dockerfile*`, `hack/**`, `.dockerignore`, `go.mod`. Run it on every push to main as a backstop. S3 +`Dockerfile*`, `hack/**`, `.dockerignore`, `go.mod`, and `charts/**`, since the leg installs the +packaged chart (`HELM_CHART_SOURCE=./gitops-reverser.tgz`). Run it on every push to main as a backstop. S3 ("second Go change also restarts") and S6 ("pod uses the imported image", already asserted in S2) can go; each costs a full rebuild. This removes a 14–18 minute leg from most PRs. diff --git a/docs/design/live-edit-dropped-while-render-fidelity-pending.md b/docs/design/live-edit-dropped-while-render-fidelity-pending.md index 5c8fc507..f6e4d9de 100644 --- a/docs/design/live-edit-dropped-while-render-fidelity-pending.md +++ b/docs/design/live-edit-dropped-while-render-fidelity-pending.md @@ -23,8 +23,9 @@ explains the race, shows it in a CI log, and lays out the two proposed fixes. receives the answer and records the scope clean. 3. In the gap between those two steps the gate is still closed. The worker takes the next item off its queue, a live event, finds the gate closed, and **drops** the event. -4. Nothing brings it back. The watch has already moved past the event, no replay follows, and the - drop is logged only at debug level (`V(1)`). +4. Nothing brings it back. The watch has already moved past the event, and no replay follows. Fix 1 + made the drop visible: it is counted in `git_writes_dropped_total`, and the first drop under + each gate status is logged at Info ([`render_fidelity_drops.go`](../../internal/git/render_fidelity_drops.go)). **Fix 1** removes the gap: the worker records the scope clean itself, before it answers. That is small, changes no documented behavior, and closes the race this spec hits. @@ -149,8 +150,8 @@ A dropped event is not retried anywhere: *after* the event. - A replay happens only when a stream (re)starts: operator restart, an expired cursor, or a plan change for that collection. The 30-second periodic plan pass keeps running streams as they are. -- The drop is logged at `V(1)` ([`branch_worker.go:1371`](../../internal/git/branch_worker.go#L1371)) - and counted nowhere. +- Before Fix 1 the drop was logged only at `V(1)` and counted nowhere. It is now counted in + `git_writes_dropped_total`, with the first drop under each gate status logged at Info. So the edit stays missing from Git until the object changes again or the collection replays. Raising the spec's timeout would not help, because the event no longer exists anywhere to wait diff --git a/docs/finished/provider-access-rules.md b/docs/finished/provider-access-rules.md index 4ada02a6..96368457 100644 --- a/docs/finished/provider-access-rules.md +++ b/docs/finished/provider-access-rules.md @@ -107,7 +107,7 @@ consumers can then carry different RBAC permissions. | Who owns a grant list? | One policy owner per object: every grant list is `+listType=atomic`, and `names`, `branches`, `sourceNamespaces`, `clusterResources` and `applierNamespaces` are `+listType=set` | | Are there booleans? | No. A grant's source is an enum (`from`), and every scope is a list, so either can grow a value later without a new field ([API conventions](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md): think twice about bool fields) | -> **Since changed:** the first two answers no longer hold for `GitBranch` and `KubeProvider`. An +> **Since changed:** the second and third answers no longer hold for `GitBranch` and `KubeProvider`. An > omitted list there is one `from: Same` grant for the object's own namespace, a written list is > the whole policy, and `[]` admits nobody; `GitProvider.spec.allowedGitBranches` keeps no default. > See [the grants](../configuration.md#the-grants). diff --git a/docs/future/gitops-api-open-asks.md b/docs/future/gitops-api-open-asks.md index 335d816a..bbc76734 100644 --- a/docs/future/gitops-api-open-asks.md +++ b/docs/future/gitops-api-open-asks.md @@ -12,7 +12,7 @@ for `ResumedElsewhere`). Their suggested order, as of revision 15: #32, then #31, then #34 when the e2e budget allows; then #23, #11 and #10; #28 when two owners share a cluster. -## 32. A troubleshooting row: something replaces the Kustomization's whole spec +## 32. A troubleshooting row: something replaces the `Kustomization`'s whole spec **Ask.** One row in [the Flux guide](../bi-directional-flux.md)'s troubleshooting table: repeated `ClaimedResourceReconciled` Events mean a controller writes the `Kustomization` with a full-spec @@ -26,7 +26,7 @@ symptom is a resume Event plus an `OperationUnresolved` on every reconcile of th Flux applying unchecked until the claim suspends it again. They reproduced it on purpose. The fix on their side was three lines; finding it from the guide alone would not have been obvious. -## 31. Count "resumed elsewhere" in a metric, not only in an Event +## 31. Count "resumed elsewhere" in a metric, not only in an `Event` **Ask.** A counter for an apply the claim did not start (the resume, and the `OperationUnresolved` that follows it), labeled like `apply_requests_total`. Possibly also a timestamp in the claim's @@ -120,5 +120,5 @@ different namespaces, and one lands in the other's directory. Nothing is corrupt From their 0.43 upgrade: a deleted field is pruned by the API server, so the object stays `Ready=True` and quietly uses a default or a wider scope. They had to write a test asserting the -*absence* of the old spellings. A condition or Event on an object applied carrying a field the CRD -no longer serves would make a migration self-diagnosing. Not filed as an ask. +*absence* of the old spellings. When an applied object still carries a field the CRD no longer +serves, a condition or `Event` on that object would make a migration self-diagnosing. Not filed as an ask.