diff --git a/api/v1alpha3/clusterprovider_types.go b/api/v1alpha3/clusterprovider_types.go index f3e85750..1621f714 100644 --- a/api/v1alpha3/clusterprovider_types.go +++ b/api/v1alpha3/clusterprovider_types.go @@ -181,9 +181,13 @@ func (p *ClusterProvider) IsInCluster() bool { // AllowsNamespace reports whether a namespace (by name and labels) may reference this provider // from a GitTarget, per spec.allowedNamespaces. It is DENY-BY-DEFAULT: a provider with no // allowedNamespaces policy (neither names nor selector) admits no namespace. Names and selector -// are ORed. This is the single authorization predicate shared by the admission webhook and the -// reconcile-time refusal, so the two can never diverge. A malformed selector is a configuration -// error surfaced to the caller (not a silent allow). +// are ORed. Enforced on every reconcile and NOWHERE else: checkSourceAuthorization in +// internal/controller/gittarget_source_cluster.go is the only non-test caller, and it returns +// before DeclareForGitTarget, so an unauthorized target starts no watch and writes no Git. +// Reconcile-time is deliberate rather than incidental — it re-evaluates continuously, so it also +// covers a policy tightened after the GitTarget was created, which an admission webhook could not +// see. There is no admission webhook for this (docs/spec/where-validation-lives.md). A malformed +// selector is a configuration error surfaced to the caller (not a silent allow). func (p *ClusterProvider) AllowsNamespace(nsName string, nsLabels map[string]string) (bool, error) { policy := p.Spec.AllowedNamespaces if policy == nil { diff --git a/docs/INDEX.md b/docs/INDEX.md index 38e1878f..877eb992 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -54,6 +54,7 @@ misled. Full list in [`spec/README.md`](spec/README.md); the ones that carry a | [`unsupported-folder-refusal-plan.md`](spec/unsupported-folder-refusal-plan.md) | `GitPathAccepted`, and refusing what we cannot own | | [`commitrequest-design.md`](spec/commitrequest-design.md) | the CommitRequest window and its conditions | | [`commitrequest-admission-authorship.md`](spec/commitrequest-admission-authorship.md) | how a real Kubernetes user becomes a commit author | +| [`where-validation-lives.md`](spec/where-validation-lives.md) | schema → CEL → **the reconciler**; a webhook only for what exists solely at admission | | [`e2e-serial-registry.md`](spec/e2e-serial-registry.md) | which e2e specs must run Serial, and why | ## What is being decided now — [`design/`](design/) @@ -66,7 +67,7 @@ says what we support and refuse** — and then its kustomize field taxonomy, the write boundary, the orchestrator/expansion line, and how secrets are handled. -Nine other open items: +Eleven other open items: | Doc | Open question | |---|---| @@ -79,6 +80,8 @@ Nine other open items: | [`e2e-finish-plan.md`](design/e2e-finish-plan.md) | remaining e2e harness work | | [`residual-e2e-flakes-2026-06-19.md`](design/residual-e2e-flakes-2026-06-19.md) | Flake B still open | | [`sensitive-resource-diagnostics-follow-up.md`](design/sensitive-resource-diagnostics-follow-up.md) | deferred diagnostics | +| [`e2e-git-server-choice.md`](design/e2e-git-server-choice.md) | stay on Gitea or move to Forgejo — the `_csrf` pin is fixable in place on both, so the migration is now a preference call, not a fix; also why we adopt no SDK either way | +| [`watchrule-source-namespace/`](design/watchrule-source-namespace/README.md) | letting a WatchRule address a differently-named namespace on its source cluster — a deny-by-default `allowedSourceNamespaces` on the **GitTarget** (so scope is per-tenant, not a provider-wide union), unlocked by a false-by-default delegation flag on the ClusterProvider. Split into five implementable PRs: three prerequisite scope fixes (the namespace-blind resync sweep that would delete other namespaces' manifests, the cluster-wide/named stream collapse, and ClusterWatchRule's unchecked GitTarget attachment), the field and its gate, and the ceiling that makes the allow-list bind ClusterWatchRule too — required because a multi-tenant deployment runs a ClusterWatchRule per tenant from day one to capture CRDs | ## Deferred, but still wanted — [`future/`](future/) diff --git a/docs/design/watchrule-source-namespace/README.md b/docs/design/watchrule-source-namespace/README.md new file mode 100644 index 00000000..80616cd0 --- /dev/null +++ b/docs/design/watchrule-source-namespace/README.md @@ -0,0 +1,239 @@ +# Source-namespace addressing and per-target source scope + +> **Design in five PRs. PRs 1 and 2 have landed; PRs 3–5 are not built.** Companion to upstream +> wishlist #14 and [config-plane-split.md](../../finished/config-plane-split.md). +> Written 2026-07-19, split into phases 2026-07-20. Index: [INDEX.md](../../INDEX.md). +> +> Every code reference in this folder was verified against the tree on 2026-07-20. Each PR page +> carries its own evidence section so a regression in any claim is detectable without re-deriving it. + +A WatchRule can only watch the namespace it lives in. On a shared config plane that forces a +tenant's configuration namespace and their source namespace to share a name, which collides as soon +as two tenants want the same one. This folder adds a source-namespace field, and — the part that +makes it safe — a per-GitTarget ceiling on which source namespaces may be mirrored into that target +at all, binding **both** rule kinds. Three pre-existing scope defects are fixed first, because each +of them would otherwise turn the new fan-out into silent Git data loss or an unenforced boundary. + +## The model + +Authorization follows the object references that already exist: + +~~~text +WatchRule ──uses──> GitTarget ──uses──> ClusterProvider + │ │ │ + │ │ └ permits this target namespace + │ └ permits source namespaces mirrored into it (any rule kind) + └ requests one source namespace +~~~ + +- **`WatchRule.spec.sourceNamespace`** — the requested source namespace. Omitted means the + WatchRule's own namespace. +- **`GitTarget.spec.allowedSourceNamespaces`** — the target owner's allow-list. It is a property of + the **destination**, not of the requesting rule: when declared it is exhaustive for every rule that + writes to this GitTarget, ClusterWatchRule included. +- **`ClusterProvider.spec.allowedNamespaces`** — unchanged platform-admin policy: which + control-cluster namespaces may create GitTargets using the provider. +- **`ClusterProvider.spec.allowWatchRuleSourceNamespaceOverride`** — new, false-by-default + delegation. While false a WatchRule may use only its own namespace. + +The one invariant everything else serves — **a declared policy is exhaustive**: + +> When a GitTarget declares `allowedSourceNamespaces`, the source namespaces mirrored into it are +> **exactly** those the policy admits, for every rule of every kind. When it declares none, each rule +> kind keeps its legacy scope. + +| `GitTarget.allowedSourceNamespaces` | WatchRule | ClusterWatchRule (`scope: Namespaced` rules) | +|---|---|---| +| Undeclared | Own namespace only (legacy) | All source namespaces (legacy) | +| Declared | Exactly what the policy admits | Exactly what the policy admits | + +Nothing changes on upgrade: the field is new, so it is undeclared everywhere until a target owner +opts in. + +### No self-namespace exception + +An earlier revision let a declared policy still implicitly permit a WatchRule's own namespace, on the +theory that it would be rude to break a legacy rule when a policy is added for an unrelated override. +That is rejected. It would mean the field does not actually bound what reaches the target — a reader +auditing `allowedSourceNamespaces: [repo-config]` would be wrong about the target's contents, which +defeats the reason the field exists — and "ceiling" would be a false description of it. + +So a policy lists everything, including the target's own namespace when a legacy WatchRule needs it: + +~~~yaml +allowedSourceNamespaces: + names: [tenant-acme, repo-config] # own namespace listed explicitly +~~~ + +The trade-off is a genuine authoring footgun: adding a policy for one override silently denies the +co-resident legacy rules unless their namespace is listed. It is mitigated three ways, and none of +them is "hope". The field is new, so no existing configuration is affected. The denial is loud, not +silent — `SourceNamespaceAuthorized=False` with reason `SourceNamespaceNotAllowed`, `Stalled=True`, +and the stream stopped. And the reason message must name the specific fix: *"namespace tenant-acme is +not in the GitTarget's allowedSourceNamespaces; add it to keep watching this rule's own namespace."* +A footgun you are told about, in the terms of the fix, is an acceptable price for a field that means +what it says. + +## The driving use case: multi-tenancy with CRDs on day one + +1. **Nothing changes for today's rules.** A WatchRule with no `sourceNamespace` still watches its + own namespace. +2. **Config namespace and source namespace differ.** A WatchRule in `tenant-acme` selects + `repo-config` in that tenant's workspace, removing the shared-config-plane collision. +3. **One source cluster, several tenants.** Each GitTarget declares what may be mirrored into it. + acme's target reaches its workspace namespace without widening zen's target. +4. **A tenant needs CRDs, so it needs a ClusterWatchRule from day one.** A ClusterWatchRule is the + only way to select cluster-scoped types, so a real multi-tenant deployment runs one per tenant + GitTarget immediately — this is not a later refinement. That rule kind can also carry + `scope: Namespaced` entries, and it points at a GitTarget by an explicit cross-namespace + reference. Without the ceiling, the operator's answer to "will this stream objects from + namespaces outside my allow-list?" is *hand-audit every ClusterWatchRule and hope*. With it, the + answer is a field you can read off the GitTarget. This is why + [PR 5](pr5-clusterwatchrule-source-ceiling.md) is part of the launch set, not a follow-up. +5. **A deliberately cross-namespace in-cluster source.** An explicit platform-admin delegation + through the operator's cluster RBAC — the same mechanism as remote, but a much sharper sign-off. + +### What the ceiling does not do + +**Cluster-scoped objects have no namespace, so a namespace allow-list cannot partition them.** A +tenant whose ClusterWatchRule selects CRDs receives *every* CRD the source credential can read, and +`allowedSourceNamespaces` neither narrows nor is consulted for those streams. That is defensible — +CRDs are genuinely cluster-global, and mirroring the cluster's type surface into a tenant repo is +usually the intent — but "my allow-list bounds what this tenant sees" is **false for the +cluster-scoped half**, and a multi-tenant operator must know that before relying on it. + +If a tenant must not see another tenant's cluster-scoped objects, this model is deliberately +insufficient. Use one ClusterProvider and source credential per tenant, so the credential's own RBAC +is the boundary. A per-target allow-list for cluster-scoped *types* is a plausible later field; it is +not in this workstream, because it is a different question (which types) from the one being answered +here (which namespaces). + +## Implementation phases + +Five PRs, ordered so each is independently reviewable and independently revertible. The first three +are pre-existing defect fixes that carry no API change; the last two are the feature. + +| # | PR | Scope | Depends on | Status | +|---|---|---|---|---| +| 1 | [Namespace-scoped resync](pr1-namespace-scoped-resync.md) | A per-namespace replay must not sweep other namespaces' manifests of the same type. Bug fix, no API. | — | **landed** | +| 2 | [Stream-scope collapse](pr2-stream-scope-collapse.md) | A cluster-wide selection stops silently widening a co-resident named-namespace stream for the same GVR. Bug fix, no API. | 1 | **landed** | +| 3 | [ClusterWatchRule target admission](pr3-clusterwatchrule-target-admission.md) | A ClusterWatchRule may no longer attach to a GitTarget whose namespace its ClusterProvider does not admit. Bug fix, no API. | — | not started | +| 4 | [The sourceNamespace field and gate](pr4-source-namespace-field.md) | `WatchRule.spec.sourceNamespace`, `GitTarget.spec.allowedSourceNamespaces`, the delegation flag, the gate, `SourceNamespaceAuthorized`, and the source-scope service. | 1, 2 | not started | +| 5 | [The ClusterWatchRule ceiling](pr5-clusterwatchrule-source-ceiling.md) | A declared `allowedSourceNamespaces` narrows a ClusterWatchRule's namespaced streams. | 1, 2, 4 | not started | + +**PR 1 gated everything, and has landed.** It was not a cleanup to slot in opportunistically: the +resync sweep was scoped by type but not by namespace, so the first change that let one GitTarget +watch a GVR in more than one namespace would have started deleting other namespaces' manifests from +Git. PRs 2, 4, and 5 each introduce exactly that fan-out, independently, so landing any of them +first would have been silent data loss in a tenant's repository. With PR 1 in, that floor is in +place and the remaining four are unblocked. + +**Release gate: do not cut a release between PR 4 and PR 5.** PR 4 ships the field; until PR 5 lands +the field is enforced on the rule kind that cannot bypass it and unenforced on the one that can, and +[use case 4](#the-driving-use-case-multi-tenancy-with-crds-on-day-one) is exactly the configuration +that hits the gap. They may merge separately; they must ship together. + +PR 3 is independent of the rest and can go at any point. + +### The shape all five are serving + +The end state is that a GitTarget's watch set is **exactly the streams that were declared for it** — +no accidental widening from a co-resident rule (PR 2), no attachment the provider never admitted +(PR 3), no scope +that outruns its declaration (PR 4, PR 5), and no sweep that acts outside the scope it was gathered +over (PR 1). Every defect in this folder is the same mistake in a different place: a scope that is +computed in one part of the system and then silently widened or dropped in another. That is why the +fixes precede the feature rather than accompanying it. + +## Why the scope lives on GitTarget + +GitTarget already binds one source cluster to one Git destination, branch, and path, so what may +arrive at that destination is a property of the target. The reference chain a reader follows is the +same one the controller follows: `WatchRule.sourceNamespace` → `targetRef` → +`GitTarget.allowedSourceNamespaces` → `clusterProviderRef` → `ClusterProvider.allowedNamespaces` and +the delegation flag. + +The decisive argument against putting both allow-lists on ClusterProvider is that two fields named +`allowedNamespaces` and `allowedSourceNamespaces` on the *same* object would mean namespaces in two +*different* clusters — an ambiguity no name can fix. Splitting them across the two objects that own +those clusters removes it. A provider-wide source list also cannot express ownership: a provider +admitting `tenant-acme` and `tenant-zen` and listing `acme-config` and `zen-config` has no way to say +which tenant owns which, and becomes a Cartesian product. + +The model works because `WatchRule.targetRef` is a `LocalTargetReference` with no namespace field +([watchrule_types.go:24-42](../../../api/v1alpha3/watchrule_types.go#L24-L42)), so every WatchRule +using a target is in that target's namespace. `ClusterWatchRule.targetRef` **is** cross-namespace, +which is precisely why PRs 2 and 4 exist. + +## Naming decisions + +- **`sourceNamespace`**, not `sourceClusterNamespace` or `remoteNamespace`. "source" is the + established axis in this API (~110 occurrences, plus `SourceClusterReachable`, + `SourceClusterResolver`, `SourceClusterAccessDenied`); `clusterProviderRef` already "names the + SOURCE cluster this GitTarget mirrors FROM" + ([gittarget_types.go:90](../../../api/v1alpha3/gittarget_types.go#L90)), while "remote" appears + only in informal prose. `sourceNamespaceOverride` is rejected for the field: it names the mechanism + relative to a default rather than the thing itself. The delegation *flag* keeps "Override" + deliberately, because a boolean gate does name a mechanism. +- **`allowedSourceNamespaces`**, not `sourceNamespaces`. The `allowed` prefix is load-bearing: a + reader meeting an absent field must not have to guess between "none" and "unrestricted", and the + fail-open reading is the catastrophic one. `allowed*` is already this repo's idiom for the shape + (`allowedNamespaces`, `allowedBranches`). +- **Deferred rename.** The fully symmetric pair would be `allowedGitTargetNamespaces` + + `allowedSourceNamespaces`. Do **not** rename in v1alpha3 — `allowedNamespaces` is shipped and is a + security control. The rename would fail *closed* (old field ignored → no policy → deny-by-default), + so it is churn rather than danger. Adopt it at the next API version, with conversion. + +Both policies use one generic `NamespaceMatcher` shape, with `names` and `selector` ORed, an +omitted-or-empty matcher admitting nothing, and a selector matching labels on the Namespace object in +that field's own cluster: + +| Field | Labels come from | Meaning | +|---|---|---| +| `ClusterProvider.allowedNamespaces` | control cluster | Which namespaces may create a GitTarget using the provider. | +| `GitTarget.allowedSourceNamespaces` | source cluster | Which namespaces may be mirrored into this target, by any rule kind. | + +## Compatibility + +This is a preliminary v1alpha3 API. We intentionally accept the following observable changes; no +migration, deprecation period, or conversion webhook is planned. The generated CRD remains served as +v1alpha3 with the existing status subresource and map-style conditions list, so no stored-object +migration is required — but release notes must call these out. + +| Change | Impact | PR | +|---|---|---| +| `sourceNamespace`, `allowedSourceNamespaces`, delegation flag | Additive. An omitted `sourceNamespace` still selects the rule's own namespace, but a manifest using an override requires the new controller. An older controller silently continues the legacy own-namespace watch and must not be used with such manifests. | 4 | +| `SourceNamespaceAuthorized` condition, `SourceAuthorized` printer column | Additive status surface. Scripts consuming a fixed condition set or column layout must tolerate it. PR 4 adds both to WatchRule; PR 5 adds them to ClusterWatchRule. | 4, 5 | +| Denied override is `Failed` | Intentional: an authorization refusal is `Ready=False`/`Reconciling=False`/`Stalled=True`, not a quietly inactive rule. An *unevaluatable* policy is not a refusal — see [establishing versus maintaining](pr4-source-namespace-field.md#establishing-versus-maintaining-a-scope). | 4 | +| Selector-based policies | The source credential now needs Namespace `get`, `list`, `watch`. Without them selector policies cannot be evaluated; exact-name policies keep working. | 4, 5 | +| Stream-scope fix | Narrows a stream that previously became cluster-wide through the named/cluster-wide collapse. Any configuration relying on that accidental widening changes behavior. | 2 | +| ClusterWatchRule target admission | Rejects a ClusterWatchRule whose GitTarget is not admitted by its ClusterProvider. | 3 | +| ClusterWatchRule ceiling | A ClusterWatchRule's namespaced streams narrow to a declared `allowedSourceNamespaces`. No existing config changes, since the field is undeclared everywhere on upgrade. Cluster-scoped rules unaffected. | 5 | + +## Alternatives considered + +- **No delegation flag** — rejected. A GitTarget policy would otherwise silently turn provider access + into cross-namespace source reachability. +- **Provider-wide `allowedSourceNamespaces`** — useful only for a one-tenant provider; cannot express + ownership. This was an earlier revision's recommendation, superseded by the ownership argument. +- **Provider-side pair policies** (`{gitTargetNamespace, sourceNamespaces}`) — can enforce a + platform-owned per-tenant maximum, which the GitTarget model deliberately cannot. More complex and + less followable. Reserve for a later stronger-boundary requirement; do not treat GitTarget's policy + as a substitute for it. +- **Gate the override on `kubeConfig` presence (remote-only)** — rejected. `kubeConfig` is + connectivity, not permission. It also welds the in-cluster case shut forever, conflating "unsafe by + default" with "impossible". See the `IsLocalSource()` trap in + [PR 4](pr4-source-namespace-field.md#locality-is-not-the-switch). +- **Use ClusterWatchRule instead of a WatchRule field** — mechanically viable, since a + ClusterWatchRule already resolves through the same source cluster, but it costs per-tenant + namespace ownership and tenant self-service authoring, and needs PRs 1 and 2 regardless. +- **Unique namespace naming** (`acme-config`) — works today with no code change and gives full + isolation, at the cost of an account-encoded namespace name the tenant sees in their own workspace. + This is the fallback if a tenant needs the capability before this ships. +- **`namespaceSelector` fan-out on WatchRule** — deferred. More powerful, but changes routing + semantics (which folder does each namespace map to?). The single-namespace field is the surgical + version; a selector can follow. +- **Define the watch CRs in the source cluster** — namespace-correct by construction, but re-diffuses + config across every workspace (the thing the config-plane split centralized) and drags the Git + token back onto the watched cluster, since `GitProvider.secretRef` is namespace-local. A reasonable + opt-in mode later, not the fix. diff --git a/docs/design/watchrule-source-namespace/pr1-namespace-scoped-resync.md b/docs/design/watchrule-source-namespace/pr1-namespace-scoped-resync.md new file mode 100644 index 00000000..75adaa67 --- /dev/null +++ b/docs/design/watchrule-source-namespace/pr1-namespace-scoped-resync.md @@ -0,0 +1,159 @@ +# PR 1 — the resync sweep must be scoped by namespace, not only by GVR + +> Phase 1 of [source-namespace addressing](README.md). **Depends on:** nothing. +> **Blocked every other PR in this folder.** Bug fix — no API change, no CRD regeneration. +> +> **Status: landed.** This was the prerequisite that makes namespace fan-out safe: until it landed, +> any change that let one GitTarget watch a GVR in more than one namespace would **delete Git +> content**. The rest of this page is the record of what was wrong and what shipped; sections below +> are past-tense by design, so a regression is recognisable against them. + +## The defect, as it stood before this PR + +A per-namespace replay produced a `desired` set covering **one** namespace, but the resulting +mark-and-sweep was scoped by **(group, resource) only**. Every managed document of that type in every +*other* namespace was therefore absent from `desired`, and a sweep deletes what is absent. + +The scope was dropped in one place. `targetWatchSpecs` already built per-namespace watch keys: + +~~~go +key := targetWatchKey{GVR: wt.GVR, Namespace: ns} // namespace is known here +~~~ + +but `enqueueReplayResync` passed only the GVR onward: + +~~~go +resultCh, enqueued, err := m.EventRouter.enqueueScopedResync( + ctx, gitDest, key.GVR, desired, revision, false) // key.Namespace dropped +~~~ + +`enqueueScopedResync` then set `scope := gvr`, and `resyncPlan` built a predicate that never looked +at the namespace: + +~~~go +inScope := func(ri types.ResourceIdentifier) bool { + return ri.Group == gvr.Group && ri.Resource == gvr.Resource +} +~~~ + +`ri` is a `types.ResourceIdentifier`, which **already carried `Namespace`**. The information was +present at both ends and discarded in the middle. + +## What the tree looks like now + +The scope travels end to end as one value: + +- `git.ResyncScope` carries GVR **and** Namespace, and owns the match predicate + ([types.go](../../../internal/git/types.go)) — `ResyncRequest` and `PendingWrite` both hold it, so + the two halves of a scope cannot be separated in transit. +- `resyncScopeForWatchKey` is the single watch-key → scope conversion + ([event_router.go:207-213](../../../internal/watch/event_router.go#L207-L213)), and + `enqueueReplayResync` passes `resyncScopeForWatchKey(key)` + ([target_watch.go:585-586](../../../internal/watch/target_watch.go#L585-L586)), so `key.Namespace` + is preserved rather than dropped. +- `resyncPlan` matches through `scope.Matches` + ([resync_flush.go:475](../../../internal/git/resync_flush.go#L475)); an empty `Namespace` keeps + the whole-GVR meaning a genuinely cluster-wide stream needs. +- `resyncHealKey` separates namespaces, so a parked heal for one no longer replaces another's. + +## Why it was latent, and live the moment anything else lands + +It could not fire before this PR because a GitTarget could only ever watch one namespace per GVR: +`WatchRule.targetRef` is a `LocalTargetReference` with no namespace field, so every WatchRule using a +target lives in that target's namespace, and a ClusterWatchRule's `""` key collapses the whole type +to all-namespaces (which is a correct whole-GVR sweep). One named namespace, or none. + +Each of the remaining PRs breaks that invariant, independently — which is why this one went first: + +| PR | New source of multi-namespace fan-out on one GVR | +|---|---| +| [PR 2](pr2-stream-scope-collapse.md) | Named and cluster-wide selections become distinct concurrent streams for the same GVR. | +| [PR 4](pr4-source-namespace-field.md) | Two WatchRules in the target's namespace can carry different `sourceNamespace` values. | +| [PR 5](pr5-clusterwatchrule-source-ceiling.md) | A declared ceiling expands one cluster-wide selection into N per-namespace selections. | + +So this was not a defect to fix opportunistically alongside the feature. It is the load-bearing floor +under all three, and the failure mode is silent data loss in a tenant's repository — a replay for +`team-a` removing `team-b`'s manifests of the same type. + +## The fix that shipped + +**Thread the namespace through the scope, and match on it.** Concretely: + +1. The scoped-resync request carries a namespace alongside its GVR — `enqueueScopedResync` takes a + `git.ResyncScope` instead of a bare `gvr`, and carries it into `git.ResyncRequest` and + `PendingWrite`. +2. `resyncPlan`'s predicate became `scope.Matches`: when the scope names a namespace it requires + `ri.Namespace == scope.Namespace` in addition to group and resource. An empty scope namespace + keeps the whole-GVR meaning, which is what a genuinely cluster-wide stream needs. +3. Every other `enqueueScopedResync` caller was audited for the same drop. The `heal: true` path and + any other scoped-resync producer passes a scope consistent with the `desired` set it built, and + `resyncHealKey` includes the namespace so two namespaces' parked heals no longer collide. + +The invariant now held, and stated in the code comment: **the sweep scope must be exactly the scope +the `desired` set was gathered over.** A `desired` narrower than its sweep scope deletes; a `desired` +wider than its sweep scope silently leaves content unmanaged. This is the rule that was violated. + +Having one conversion function (`resyncScopeForWatchKey`) rather than a namespace parameter threaded +by hand is the part that makes it stay fixed: there is no second place for a caller to forget. + +### Alternative considered: coalesce into one authoritative snapshot + +Rather than making the scope finer, make `desired` wider: gather every watched namespace for a GVR +into a single snapshot and keep sweeping by GVR. This is attractive because it yields one +authoritative picture per type and removes a class of partial-scope reasoning entirely. + +It is rejected for this PR because it couples the replay lifecycles of independent streams: one +namespace's watch failing or resuming late would hold up or falsify the whole type's snapshot, and +streams start and stop independently by design. Scope-narrowing is also the smaller, more directly +testable change. Revisit coalescing if per-namespace replay volume becomes the problem. + +## Revocation leaves prior content — a decision, not an oversight + +Stopping a stream does not remove what it already wrote. When a namespace leaves a watch set — a +[PR 5](pr5-clusterwatchrule-source-ceiling.md) ceiling tightening, a WatchRule deletion, a revoked +label — its manifests remain in Git. + +**Recommended: retain, and make it visible.** Deleting a tenant's manifests as a side effect of a +policy edit is destructive, hard to undo in the moment, and easy to trigger by accident (a typo in a +selector). Retention is also the safe direction under the failure mode in +[PR 5](pr5-clusterwatchrule-source-ceiling.md#2b-unknown-is-not-empty): if an unavailable selector were +ever read as an empty allow-list, a sweep-on-revocation would erase the target's entire namespaced +content. + +The cost is real and must be documented rather than glossed: after a revocation, Git holds manifests +from a namespace the policy no longer admits, and no automatic process removes them. Removing them is +a deliberate operator action. Whichever way this is settled, it must be settled explicitly and +covered by a test — the failure to avoid is discovering the behavior in production. + +## Tests that shipped + +- **`TestResync_NamespaceScopedSweepLeavesSiblingNamespacesAlone`** — a GitTarget managing one GVR in + `team-a` and `team-b`, replaying only `team-a`; `team-b`'s manifests survive untouched. This is the + test that failed before the fix and is the whole point of the PR. +- **`TestResync_NamespaceScopedSweepStillDropsOrphansInItsOwnNamespace`** — an object removed from + `team-a` while `team-a` replays is still swept. The fix narrows the sweep; it must not turn it off. +- **`TestResync_ClusterWideScopeStillSweepsEveryNamespace`** — a genuinely cluster-wide stream (empty + scope namespace) still sweeps every namespace for its type, so PR 2's cluster-wide half is + unaffected. +- **`TestResyncScopeForWatchKey_CarriesBothHalvesOfTheScope`** — the scope/`desired` agreement + invariant asserted directly, so a future caller that drops the namespace again fails here rather + than in a tenant's repo. +- **`TestResyncScope_MatchesRespectsTypeAndNamespace`**, + **`TestResyncHealKey_SeparatesNamespacesOfTheSameType`**, + **`TestResyncScope_StringIsNilSafeAndNamesTheNamespace`** — the predicate, the heal-key split, and + nil-safe formatting. + +Verified by reverting the namespace half of `ResyncScope.Matches`: +`TestResync_NamespaceScopedSweepLeavesSiblingNamespacesAlone` and the sibling-namespace row of +`TestResyncScope_MatchesRespectsTypeAndNamespace` both fail without the fix. + +## Done — with one item carried forward + +- ✅ A scoped resync carries a namespace end to end, and the plan predicate honours it. +- ✅ Multi-namespace replay is proven non-destructive by test. +- ✅ `task lint`, `task test`, `task test-e2e` pass. +- ⏭ **Retention-on-revocation is documented above but not yet enforced by a test.** Nothing in this + PR can revoke a namespace — no code path removes one from a watch set yet — so the test has no + subject until [PR 5](pr5-clusterwatchrule-source-ceiling.md) introduces ceiling tightening. It is + carried as `TestCeiling_UnknownScopeRetainsPreviousAndDoesNotSweep` and the revocation envtest in + PR 5's plan. Recording it here rather than silently dropping it. diff --git a/docs/design/watchrule-source-namespace/pr2-stream-scope-collapse.md b/docs/design/watchrule-source-namespace/pr2-stream-scope-collapse.md new file mode 100644 index 00000000..80fbff47 --- /dev/null +++ b/docs/design/watchrule-source-namespace/pr2-stream-scope-collapse.md @@ -0,0 +1,95 @@ +# PR 2 — a cluster-wide selection must not collapse named-namespace scoping + +> Phase 2 of [source-namespace addressing](README.md). **Depends on:** +> [PR 1](pr1-namespace-scoped-resync.md) — this PR is the first thing that makes a GitTarget watch one +> GVR in two namespaces at once, which is unsafe until the resync sweep is namespace-scoped. +> **Blocks:** PR 4 (the gate is only as good as the stream scoping underneath it) and PR 5. +> Bug fix — no API change, no CRD regeneration. +> +> **Status: landed.** `SnapshotNamespaces` is now `WatchedType.WatchScopes`, which returns every +> namespace scope including the cluster-wide `""` instead of collapsing to it, and both read sites +> project one stream per scope: `targetWatchSpecs` keys a watch per scope with that scope's own +> operation set, and `snapshotGVRsFromTable` emits one `snapshotGVR` per scope (its `namespaces` +> slice became a single `namespace`, matching `targetWatchKey`). The old assertion was deleted and +> replaced; the three replacements fail against the pre-fix collapse. + +## The defect + +`SnapshotNamespaces()` returns `nil` when a `WatchedType` has the `""` namespace key, and `nil` +means *all namespaces* at every read site +([watched_type_table.go:78-92](../../../internal/watch/watched_type_table.go#L78-L92)). So a +WatchRule scoped to one namespace and a ClusterWatchRule scoped cluster-wide, on the **same GVR and +the same GitTarget**, fold into one `WatchedType` — and the cluster-wide entry wins. The named +namespace survives only in the plan hash. + +The operation sets collapse the same way, not just the namespaces: `targetWatchSpecs` uses +`operationSpec(wt.NamespaceOps[""])` and discards the per-namespace op sets +([target_watch.go:214-224](../../../internal/watch/target_watch.go#L214-L224)). A `CREATE`-only +named rule co-resident with an `UPDATE` cluster-wide rule loses its filter too. + +### Why it matters to this workstream + +Under [PR 4](pr4-source-namespace-field.md) this is a gate bypass. A ClusterWatchRule may +legitimately select every source namespace once its GitTarget passes provider admission — but a +co-resident WatchRule must not silently inherit that cluster-wide stream. Otherwise a WatchRule +authorized only for `repo-config` receives events from every namespace the credential can read, and +its `allowedSourceNamespaces` check passed only *before* the data plane widened it. + +[PR 5](pr5-clusterwatchrule-source-ceiling.md) removes the `""` key **for the namespaced selections** +of any target with a declared ceiling — `scope: Cluster` rules keep emitting `""`, because a +namespace allow-list cannot constrain cluster-scoped types. So the collapse cannot trigger for a +namespaced GVR under a ceiling, but a target that mirrors a GVR both cluster-scoped and namespaced is +not covered by that, and the far more common undeclared case is not covered at all. This PR is what +governs both. + +### This behavior is currently asserted as intended + +`TestBuildWatchedTypeTable_ClusterWideOverridesNamedNamespaces` +([watched_type_table_test.go:64-82](../../../internal/watch/watched_type_table_test.go#L64-L82)) +documents it as "matching the historic `gvrSnapshotEntry` collapse". So this is a design-intent +versus security-intent conflict, not an oversight. The fix must consciously **replace** that test, +not work around it — leaving it green would mean the fix did not land. + +## Verified mechanism + +`buildWatchedTypeTable` +([watched_type_table.go:128-155](../../../internal/watch/watched_type_table.go#L128-L155)) is a pure +union: both the `""` key (ClusterWatchRule) and `"team-a"` (WatchRule) land in the same +`namespaceOps` map for the same GVR. The collapse happens later, at *read* time — `ClusterWide()` +merely tests for presence of the `""` key +([watched_type_table.go:73-76](../../../internal/watch/watched_type_table.go#L73-L76)), so +`SnapshotNamespaces()` short-circuits to `nil`. Two read sites consume that `nil` as all-namespaces: + +- `targetWatchSpecs` ([target_watch.go:214-224](../../../internal/watch/target_watch.go#L214-L224)) +- `snapshotGVRsFromTable` ([scope_resolve.go:180](../../../internal/watch/scope_resolve.go#L180)) + +The `team-a` entry survives in `NamespaceOps` for the plan hash only. + +## The fix + +Keep cluster-wide and named selections as distinct streams for the same GVR, or subtract the named +scope from the cluster-wide one — and preserve the per-namespace operation sets either way. Both +read sites above must agree; a fix applied to one of them is worse than no fix, because the plan hash +and the running streams then disagree. + +> **Do not land this before [PR 1](pr1-namespace-scoped-resync.md).** Distinct concurrent streams for +> one GVR is precisely the fan-out the resync sweep mishandles: the named stream's replay carries a +> `desired` set for one namespace, while the sweep it triggers is scoped to the whole type. Fixing +> the collapse first therefore converts a silent over-watch into silent deletion of the cluster-wide +> stream's manifests. + +## Tests + +- **Replacement for the old assertion:** a WatchRule scoped to `team-a` and a cluster-wide + ClusterWatchRule on the same GVR and GitTarget must **not** collapse to one all-namespaces stream. + This replaces `TestBuildWatchedTypeTable_ClusterWideOverridesNamedNamespaces`. +- **Operation sets:** a `CREATE`-only named rule co-resident with an `UPDATE` cluster-wide rule + preserves both op sets. +- **Both read sites:** assert on `targetWatchSpecs` *and* `snapshotGVRsFromTable`, so a fix that + lands in one path only is caught here rather than as a resync anomaly later. + +## Done when + +- The old test is deleted, not skipped, and its replacement asserts non-collapse. +- Named and cluster-wide streams for one GVR are independently observable in both read paths. +- `task lint`, `task test`, `task test-e2e` pass. diff --git a/docs/design/watchrule-source-namespace/pr3-clusterwatchrule-target-admission.md b/docs/design/watchrule-source-namespace/pr3-clusterwatchrule-target-admission.md new file mode 100644 index 00000000..6926831b --- /dev/null +++ b/docs/design/watchrule-source-namespace/pr3-clusterwatchrule-target-admission.md @@ -0,0 +1,111 @@ +# PR 3 — a ClusterWatchRule may attach to a GitTarget its provider never admitted + +> Phase 3 of [source-namespace addressing](README.md). **Depends on:** nothing — independent of the +> other PRs and orderable at any point. Bug fix — no API change, no CRD regeneration. + +## What this PR is, precisely + +It is a **RuleStore and data-plane consistency guard**: it makes the rule-compilation path apply the +same ClusterProvider admission check the GitTarget controller already applies, so a ClusterWatchRule +cannot compile against a GitTarget that admission rejected. + +It is **not** a new consent mechanism, and the plan should not be read as adding one. There is no +target-side authorization policy here — nothing lets a GitTarget owner say which ClusterWatchRules +may reference their target. The check re-applies the *same* provider→GitTarget-namespace admission +from a second call site. A genuine target-side consent policy (say, +`GitTarget.spec.allowedClusterWatchRules`) would be a separate API decision and is not proposed. + +What this PR does buy is that the admission decision is enforced wherever rules are compiled, rather +than only where GitTargets are reconciled — which matters because those two paths can disagree, as +below. + +## The defect + +The ClusterWatchRule reconciler resolves its GitTarget with a plain `r.Get` and **no authorization +check of any kind** +([clusterwatchrule_controller.go:160-162](../../../internal/controller/clusterwatchrule_controller.go#L160-L162)); +the 537-line file contains no authorization call at all — confirmed by grep, and `AllowsNamespace` +has exactly one non-test call site anywhere in the tree +([gittarget_source_cluster.go:68](../../../internal/controller/gittarget_source_cluster.go#L68)). +`bootstrap.go` seeds rules the same way +([bootstrap.go:71-91](../../../internal/watch/bootstrap.go#L71-L91)). + +Since `ClusterWatchRule.targetRef` is a `NamespacedTargetReference` with a **required** namespace +([clusterwatchrule_types.go:22-44](../../../api/v1alpha3/clusterwatchrule_types.go#L22-L44)), any +ClusterWatchRule may attach itself to any GitTarget in any namespace and widen that target's mirror +scope to cluster-wide, without the compilation path ever consulting the ClusterProvider's admission +policy for that target's namespace. + +`allowedNamespaces` is the ClusterProvider's explicit admission of the **GitTarget namespace** to use +that provider. The compilation path never consults it. + +## Not an escalation today — and why to fix it anyway + +ClusterWatchRule is cluster-scoped, so only a config-plane cluster-admin can create one, and that +subject can already read the kubeconfig Secrets directly. Nobody gains access they lacked. + +The gate is also effective *transitively*, which is worth knowing before assuming a live hole: +`checkSourceAuthorization` runs inside the Validated gate and returns before `DeclareForGitTarget` +([gittarget_controller.go:218](../../../internal/controller/gittarget_controller.go#L218)), which is +what populates `gitTargetClusters` and creates the `targetWatches` entry. The rule-change path cannot +bootstrap a watch on its own — `refreshRunningTargetWatches` +([target_watch.go:175-193](../../../internal/watch/target_watch.go#L175-L193)) snapshots the +*existing* `targetWatches` keys and skips any table whose destination is not already running. So a +ClusterWatchRule pointing at an unauthorized GitTarget builds a resident table but starts no stream. +`bootstrap.go` is benign for the same reason. + +Fix it regardless, for two reasons. First, the transitive protection is incidental: it depends on +ordering inside a controller that nobody is currently required to preserve, so a future refactor of +`DeclareForGitTarget` silently converts it into a real hole with no test to catch that. Second, it +makes `allowedNamespaces` mean what it says. A platform admin reading a ClusterProvider's admission +list should be able to conclude that no rule anywhere is mirroring through that credential on behalf +of an unadmitted target. + +Note one thing that is *already* confined: `providerNS := target.Namespace` +([clusterwatchrule_controller.go:173](../../../internal/controller/clusterwatchrule_controller.go#L173)), +so the GitProvider is resolved in the GitTarget's own namespace. The unchecked edge is +rule → GitTarget only. + +## The fix + +Factor the GitTarget provider-admission check into a shared helper and run it for the referenced +GitTarget's namespace before a ClusterWatchRule is stored — in **both** the reconciler +([clusterwatchrule_controller.go:160](../../../internal/controller/clusterwatchrule_controller.go#L160)) +and [bootstrap.go:71-91](../../../internal/watch/bootstrap.go#L71-L91). A helper used by one of the +two paths is the failure mode to avoid: bootstrap runs before the reconciler on every restart. + +On denial: remove any existing compiled ClusterWatchRule, replan the watch manager to stop its +stream, then set `GitTargetReady=False` with reason `GitTargetNamespaceNotAuthorized` and publish +the terminal kstatus trio (`Ready=False`, `Reconciling=False`, `Stalled=True`). Stop the data plane +*before* publishing status — a gate that only writes a condition is not a gate. + +Changes to a ClusterProvider's `allowedNamespaces` must requeue affected ClusterWatchRules, so a +later revocation has the same effect as an initial denial. The ClusterProvider → GitTargets mapper +already exists +([gittarget_controller.go:1138-1142](../../../internal/controller/gittarget_controller.go#L1138-L1142)); +ClusterProvider → ClusterWatchRules does not. + +This check is separate from, and does not replace, +[`GitTarget.allowedSourceNamespaces`](pr5-clusterwatchrule-source-ceiling.md). They answer different +questions: provider admission asks *may this target use this credential at all*, the ceiling asks +*which source namespaces may reach this target's destination*. + +## Tests + +- **Direct refusal:** a ClusterWatchRule referencing a GitTarget whose namespace the ClusterProvider + does not admit is refused in the reconciler **and** in the bootstrap path. The reconciler case + leaves no compiled rule and no running stream, and sets `GitTargetReady=False`, `Ready=False`, + `Reconciling=False`, `Stalled=True` with reason `GitTargetNamespaceNotAuthorized`. +- **Revocation:** start from an admitted, running ClusterWatchRule, then remove its GitTarget + namespace from `ClusterProvider.allowedNamespaces`. The new mapper must requeue it, remove the + compiled rule, stop the stream, and publish the same terminal status. +- **Admission still works:** an admitted target's ClusterWatchRule runs unchanged — the regression + guard for a helper that is accidentally too strict. +- **Ordering:** assert the compiled rule is gone *before* the terminal condition is observable, or at + minimum that no stream survives a refusal, so a status-only implementation fails. + +## Done when + +- Both the reconciler and `bootstrap.go` call one shared admission helper. +- A provider policy change requeues ClusterWatchRules, not only GitTargets. +- `task lint`, `task test`, `task test-e2e` pass. diff --git a/docs/design/watchrule-source-namespace/pr4-source-namespace-field.md b/docs/design/watchrule-source-namespace/pr4-source-namespace-field.md new file mode 100644 index 00000000..4cf71c6b --- /dev/null +++ b/docs/design/watchrule-source-namespace/pr4-source-namespace-field.md @@ -0,0 +1,561 @@ +# PR 4 — the sourceNamespace field and its authorization gate + +> Phase 4 of [source-namespace addressing](README.md). **Depends on:** +> [PR 1](pr1-namespace-scoped-resync.md) (two WatchRules on one target can now carry different +> `sourceNamespace` values, which is the fan-out PR 1 makes safe) and +> [PR 2](pr2-stream-scope-collapse.md). **Blocked from release without:** +> [PR 5](pr5-clusterwatchrule-source-ceiling.md) — see the +> [release gate](README.md#implementation-phases). API change: three new fields, one new condition, +> one printer column. + +Adds `WatchRule.spec.sourceNamespace`, `GitTarget.spec.allowedSourceNamespaces`, +`ClusterProvider.spec.allowWatchRuleSourceNamespaceOverride`, and the reconciler gate that binds +them. The model and naming rationale are in the [overview](README.md#the-model). + +## Example + +~~~yaml +apiVersion: configbutler.ai/v1alpha3 +kind: ClusterProvider +metadata: + name: workspaces +spec: + kubeConfig: + secretRef: + name: workspaces-kubeconfig + + # Existing gate: who may create a GitTarget using this provider. + allowedNamespaces: + selector: + matchLabels: + gitops.configbutler.ai/workspace-tenant: "true" + + # New gate: an admitted GitTarget may authorize source-namespace overrides. + allowWatchRuleSourceNamespaceOverride: true +--- +apiVersion: configbutler.ai/v1alpha3 +kind: GitTarget +metadata: + name: acme + namespace: tenant-acme +spec: + providerRef: + name: acme-git + branch: main + path: tenants/acme + clusterProviderRef: + name: workspaces + + # Source-cluster namespaces that may be mirrored into this target, by any rule kind. + allowedSourceNamespaces: + names: [repo-config] + selector: + matchLabels: + gitops.configbutler.ai/mirrorable: "true" +--- +apiVersion: configbutler.ai/v1alpha3 +kind: WatchRule +metadata: + name: repo-config + namespace: tenant-acme +spec: + targetRef: + name: acme + sourceNamespace: repo-config + rules: + - resources: [configmaps] +~~~ + +Accepted because tenant-acme may use the provider, the provider delegates overrides, and the +GitTarget admits `repo-config`. A GitTarget in tenant-zen carries its own policy and inherits nothing +from acme's. + +## What the delegation flag means + +The flag does not grant access by itself — the GitTarget must still admit the namespace. It says the +platform admin delegates source-namespace selection to admitted GitTargets. That is a real authority +grant: a target owner can configure a broad selector, including one matching every source namespace, +so the source credential's RBAC remains the hard maximum. Set it only when the owner of an admitted +GitTarget is trusted to choose a subset of what that credential may read. + +The flag gates **granting only**. `allowedSourceNamespaces` plays two roles — widening a WatchRule +beyond its own namespace, and (in [PR 5](pr5-clusterwatchrule-source-ceiling.md)) narrowing a +ClusterWatchRule below cluster-wide — and only +the widening one is an authority grant. Gating a *restriction* behind a delegation flag would mean an +admin has to grant extra authority in order to reduce scope. + +### Remote and in-cluster: same mechanism, very different sign-off + +For a **remote** source this is normally a clear delegation, because the coupling being removed was +never a boundary. The config-plane namespace and the source namespace are on different clusters, so +"who may create a WatchRule in config-plane namespace `repo-config`" already told you nothing about +who may read `repo-config` on the source. What governs a remote mirror is a pair that already ships: +`allowedNamespaces` on the config-plane side, and the kubeconfig's own RBAC on the source. Neither +depends on the two namespaces sharing a name, so naming one widens nothing. + +For an **in-cluster** provider the same-name coupling *was* the boundary. The config plane is the +watched cluster, so "who can cause namespace A to be mirrored" and "who has RBAC in A" are the same +question, and ordinary namespace RBAC answers it. Setting the flag on an in-cluster provider +deliberately bypasses live namespace RBAC: the owner of an admitted GitTarget in tenant-acme can +select tenant-zen's namespace, and every object in it is read through the operator's cluster-wide +credential and written into a Git destination acme controls. That is a real cross-namespace read +escalation — sharper than the intra-namespace RBAC-split edge the config-plane split already flags +and consciously leaves open — bounded only by the operator's own cluster RBAC, which is broad by +design. + +It remains legitimate for a cluster-admin to grant on purpose. It must never happen by default or as +a side effect of another field. That is the whole reason the flag exists and defaults to false. + +If a platform needs a platform-admin-owned per-tenant *maximum*, this model is intentionally +insufficient — use one ClusterProvider and credential per tenant, or add a later provider-side pair +policy. Do not silently treat GitTarget's policy as a substitute for that stronger boundary. + +### Locality is not the switch + +A ClusterProvider is in-cluster when `kubeConfig` is omitted and remote when it is set +([clusterprovider_types.go:174-178](../../../api/v1alpha3/clusterprovider_types.go#L174-L178)). The +name `default` is only the value of an omitted GitTarget `clusterProviderRef` +([gittarget_types.go:98](../../../api/v1alpha3/gittarget_types.go#L98)). Neither decides +authorization; only the explicit delegation flag and the policies do. Deriving an addressing +capability from a connectivity setting would mean an admin who sets `kubeConfig` to reach a cluster +silently hands WatchRule authors a power they were never asked about. + +> **Implementation trap.** `GitTarget.IsLocalSource()` +> ([gittarget_types.go:238](../../../api/v1alpha3/gittarget_types.go#L238)) — despite the name — **is +> a name test**. It exists solely to seed the pre-discovery `SourceClusterReachable` default before +> the watch manager is wired +> ([gittarget_controller.go:250](../../../internal/controller/gittarget_controller.go#L250)), and the +> manager overwrites it immediately. It is not a locality predicate and must never be used as one. +> Anything needing "is this the operator's own cluster" tests `kubeConfig`. The same warning is +> recorded in [multi-cluster-author-attribution.md](../../finished/multi-cluster-author-attribution.md). + +## The gate + +The effective source namespace is controller logic; an API-server default cannot refer to +`metadata.namespace`: + +~~~go +effectiveSourceNamespace := rule.Spec.SourceNamespace +if effectiveSourceNamespace == "" { + effectiveSourceNamespace = rule.Namespace +} +~~~ + +**The legacy case needs no new authorization — but only while the target declares no policy.** If +the effective source namespace equals the WatchRule's namespace *and* the GitTarget declares no +`allowedSourceNamespaces`, the rule works with no delegation flag and no policy. Once a policy is +declared it is exhaustive, including for own-namespace rules: see +[no self-namespace exception](README.md#no-self-namespace-exception). The denial in that case uses +reason `SourceNamespaceNotAllowed` with a message naming the fix — *"namespace tenant-acme is not in +the GitTarget's allowedSourceNamespaces; add it to keep watching this rule's own namespace."* + +An explicit *different* namespace requires all three: + +1. the GitTarget's namespace is admitted by the ClusterProvider; +2. the ClusterProvider delegation flag is true; and +3. the GitTarget policy admits the effective source namespace. + +This is cross-object authorization (WatchRule → GitTarget → ClusterProvider), and source selectors +also require remote state, so it is not expressible in CEL. Per +[where-validation-lives.md](../../spec/where-validation-lives.md) that makes it a **reconciler check, +not a webhook** — the same shape and ordering as `checkSourceAuthorization` / +`GitTargetReasonNamespaceNotAuthorized` +([gittarget_source_cluster.go:41](../../../internal/controller/gittarget_source_cluster.go#L41)). +Settled repo-wide; no further argument needed. The check runs before +`RuleStore.AddOrUpdateWatchRule` +([watchrule_controller.go:225](../../../internal/controller/watchrule_controller.go#L225)). + +`sourceNamespace` is optional with `MinLength=1` when present. `allowedSourceNamespaces` is an +optional deny-by-default `NamespaceMatcher`. + +### The gate must not be bypassable on restart + +Gating `reconcileWatchRuleViaTarget` alone is **not sufficient**. `bootstrapRuleStore` lists every +WatchRule and calls `AddOrUpdateWatchRule` directly after resolving only the GitTarget and GitProvider +([bootstrap.go:49-68](../../../internal/watch/bootstrap.go#L49-L68)) — no authorization of any kind — +and it runs *before* the first reconcile, then calls `RuleStore.MarkReady()`. So on every restart a +denied override is compiled and can be watched until the reconciler catches up and removes it. The +window is unbounded on a busy queue, and it reopens on every operator restart, which is exactly when +nobody is watching. + +**Route admission and compilation through one shared path.** A WatchRule must become a compiled rule +only via a single function that runs the gate first, called by both the reconciler and bootstrap. Two +call sites that each remember to check is the arrangement this codebase has already got wrong once — +it is the same defect [PR 3](pr3-clusterwatchrule-target-admission.md) fixes for ClusterWatchRule, and +the two should share the shape of the fix. + +Bootstrap runs before controllers are started, so it cannot publish status; a rule denied at bootstrap +is simply not compiled, and the first reconcile writes the terminal condition. That is the correct +ordering — fail closed first, explain second. + +## Reactivity and source-cluster RBAC + +Policy changes must grant and revoke promptly, not merely when a WatchRule happens to be edited. +Half of this is already wired: + +| Input changes | Required reaction | Status today | +|---|---|---| +| GitTarget `allowedSourceNamespaces` | Reconcile its WatchRules. | **Already wired** — the WatchRule controller watches GitTarget with `GenerationChangedPredicate` ([watchrule_controller.go:450-453](../../../internal/controller/watchrule_controller.go#L450-L453)); a spec edit bumps the generation. | +| ClusterProvider `allowedNamespaces` or delegation flag | Reconcile affected GitTargets **and their WatchRules**. | **Half wired** — ClusterProvider → GitTargets exists ([gittarget_controller.go:1138-1142](../../../internal/controller/gittarget_controller.go#L1138-L1142)). ClusterProvider → WatchRules does **not**: that reconcile changes only GitTarget *status*, which `GenerationChangedPredicate` deliberately ignores. Needs a new mapper. | +| Control-cluster Namespace labels | Reconcile affected GitTargets. | **Already wired** — `namespaceToGitTargets` with `LabelChangedPredicate` ([gittarget_controller.go:1147-1150](../../../internal/controller/gittarget_controller.go#L1147-L1150)). | +| Source-cluster Namespace labels | Reconcile WatchRules whose GitTarget uses a source selector. | **Not wired** — there is no Namespace informer in `internal/watch` at all today. Entirely new. | + +The watch manager already owns source-cluster watch lifecycles. This adds one label-filtered +Namespace informer **per active source cluster**, not one per WatchRule, emitting only meaningful +label changes and mapping them to WatchRules whose GitTarget resolves through that cluster. +[PR 5](pr5-clusterwatchrule-source-ceiling.md) extends the same informer to ClusterWatchRules. + +### The source-scope service — define this interface before writing the gate + +There is a structural gap to close first. The informer lives in `internal/watch`, but the **gate runs +in `internal/controller`**, and `WatchManagerInterface` +([constants.go:15-24](../../../internal/controller/constants.go#L15-L24)) exposes nothing that would +let a reconciler evaluate a source-cluster selector: its six methods cover rule resolution and stream +summaries only. There is no way to ask for a source Namespace's labels, and no way to learn whether +the answer is trustworthy yet. Writing the gate without settling this ends in the reconciler dialling +the source cluster itself on every pass, duplicating the connection and cache the watch manager +already owns. + +Define one **source-scope service** owned by the watch manager and exposed on the interface. It needs +three things, and the third is the one most likely to be skipped: + +1. **Resolution** — given a GitTarget and a candidate namespace, does the target's policy admit it? + Backed by the per-source-cluster Namespace cache, never by an inline API call from the reconciler. +2. **Readiness and error state, as a first-class result.** The answer is three-valued, not boolean: + *admitted*, *denied*, or *cannot say yet* (cache still syncing, or the source cluster is + unreachable). A two-valued interface forces "cannot say" to be encoded as "denied", which is how a + transient outage becomes a terminal `Stalled=True` and a stopped stream. The three-valued result is + what makes the `Unknown` row of the status table implementable at all. +3. **Enqueue** — a label change, a cache sync, or a source-cluster reconnection must requeue the + affected rules. Without this the cache goes stale silently and revocations never land. + +Exact-name policies must be answerable **without** the cache, so a source cluster whose Namespace +access is denied still supports name-based policies. That is the degradation path below, and it falls +out naturally if resolution checks names before consulting the cache. + +The same service is what [PR 5](pr5-clusterwatchrule-source-ceiling.md) resolves its ceiling through, +so its shape is worth settling here rather than retrofitting. + +The in-cluster manager role already grants `namespaces` `get`/`list`/`watch` +([config/rbac/role.yaml](../../../config/rbac/role.yaml)). A remote provider used with a source +selector needs the same for the identity in its kubeconfig: GET supplies current labels during +reconciliation, LIST and WATCH keep grants and revocations current. +**Exact-name entries remain usable without source Namespace access** — a deliberate degradation path, +not an oversight, and the half most likely to regress unnoticed. + +### Establishing versus maintaining a scope + +*Cannot say* is the third value of the source-scope service, and what the controller does with it +depends on **which direction the answer would move the scope**. This is one contract with two +instances, not two contracts; write it down here because the two read as contradictory otherwise. + +The rule: **an unevaluatable policy never produces a resolved namespace set.** It is never +substituted with the empty set, and never with the full set. + +| | **Establishing** a grant — no previously resolved scope for this rule | **Maintaining** a scope — a previously resolved scope exists | +|---|---|---| +| Where it applies | This PR's gate: a WatchRule asking for a namespace it has not been granted. | [PR 5](pr5-clusterwatchrule-source-ceiling.md)'s ceiling, and any rule already running under a resolved policy. | +| Effect of *cannot say* | The grant is not established, so the rule is **not compiled**. Nothing runs; nothing is swept. | The **last known-good scope is retained** and keeps running. No narrowing, no widening, **no sweep**. | +| Retryable error (cache syncing, source unreachable) | `Unknown` / `CheckingSourceNamespacePolicy`, `Stalled=False`. Retried. | Same. | +| Terminal error (source Namespace `list` is `Forbidden` for a selector policy) | `False` / `SourceNamespacePolicyUnavailable`, **`Stalled=True`**. | `Unknown` / `SourceNamespacePolicyUnavailable`, **`Stalled=False`**. | + +The asymmetry in the last row is the whole point, and it is deliberate: + +- When **establishing**, "fail closed" means *do not start the stream*. A permanent `Forbidden` means + the rule will never run without an operator change — granting the RBAC or switching to exact names + — so `Stalled=True` is an accurate, actionable claim about a rule that is doing nothing. +- When **maintaining**, "fail closed" would mean *narrow to nothing*, and a narrowed set is the input + to a sweep — so failing closed there **deletes a tenant's Git content** on a transient outage. The + rule is also still running its already-granted streams (and, for a ClusterWatchRule, its + cluster-scoped ones), so `Stalled=True` would be a false claim that nothing is progressing. + See [PR 5 § unknown is not empty](pr5-clusterwatchrule-source-ceiling.md#2b-unknown-is-not-empty). + +An actual **denial** — the policy evaluated and does not admit the namespace — is terminal in both +directions: `False` / `SourceNamespaceNotAllowed` / `Stalled=True`. That is a refusal, not an +unevaluatable policy, and the two must not share a code path. + +On a denial or a revocation, remove the compiled rule from RuleStore and replan the watch manager to +stop its stream **before** publishing terminal status. On *cannot say*, do neither. + +## Status contract (kstatus-compatible) + +WatchRule already uses the project's conditions-first contract, including `observedGeneration` and +the Ready / Reconciling / Stalled trio. This extends that contract; it adds no phase, state string, +or second readiness model, so `sigs.k8s.io/cli-utils/pkg/kstatus/status` clients see the same +Current / InProgress / Failed results as for the other CRDs. + +Add the positive, state-style **`SourceNamespaceAuthorized`** condition: + +- **`True`** — the effective source namespace is authorized for this observed generation. Reason + `LegacySourceNamespace` when it is the rule's own namespace, `SourceNamespaceAllowed` when the + override passed the three-part gate. +- **`False`** — the override cannot run. Reason `SourceNamespaceNotAllowed` for a disabled flag or a + non-matching/missing GitTarget policy; `SourceNamespacePolicyUnavailable` when a selector policy is + permanently unevaluatable *and* no scope was ever established for this rule. +- **`Unknown`** — authorization is still being established, or a retryable source-cluster read/watch + error is being retried (reason `CheckingSourceNamespacePolicy`); or a rule that already has a + resolved scope has lost the ability to re-evaluate its policy and is retaining that scope (reason + `SourceNamespacePolicyUnavailable`). Do not turn a temporary connection problem into a terminal + failure, and do not turn a retained scope into one either — see + [establishing versus maintaining](#establishing-versus-maintaining-a-scope). + +Even legacy rules set it to `True`, so the effective authorization is always visible and automation +has one condition to inspect. `GitTargetReady` remains the health of the referenced GitTarget and +must not be reused for source authorization; `ResourcesResolved` and `StreamsRunning` keep their +meanings and explain the Ready aggregate *after* this gate passes. + +`SourceNamespaceAuthorized` becomes an additional prerequisite of the existing `applyRuleKstatus` +aggregation, so `Ready=True` means the source namespace is authorized *and* the GitTarget is ready +*and* resources resolved *and* streams running — never merely that the gate passed. + +| Situation | SourceNamespaceAuthorized | Ready | Reconciling | Stalled | kstatus | +|---|---|---|---|---|---| +| Selector cache starting, or retryable source error pending | Unknown | False | True | False | InProgress | +| Authorized, but target validation / resolution / replay in progress | True | False | True | False | InProgress | +| Authorized and all existing prerequisites healthy | True | True | False | False | Current | +| Selector permanently unevaluatable, **scope already resolved** — retained and still running | Unknown | False | True | False | InProgress | +| Delegation disabled, or the policy evaluated and denies | False | False | False | True | Failed | +| Selector permanently unevaluatable, **no scope ever resolved** — nothing runs | False | False | False | True | Failed | + +A terminal refusal or revocation must **first** stop the compiled rule, **then** set +`SourceNamespaceAuthorized=False` plus the Failed trio. A retryable failure while *establishing* +instead leaves the compiled rule out of the store while status stays InProgress and the reconciler +retries. A rule that is *maintaining* an already-resolved scope keeps its compiled rule and its +streams; only the condition moves to `Unknown`. This three-way distinction avoids all of continuing +an unauthorized watch, declaring a transient remote outage permanently broken, and sweeping a +tenant's manifests because a policy could not be read. + +Every condition written here, including the generic trio, carries the current `observedGeneration`. +`lastTransitionTime` changes only on a status change, per the existing upsert helper. Existing Ready +and Reason printer columns stay primary; add a priority-1 `SourceAuthorized` column so +`kubectl get watchrules -o wide` exposes the gate. + +## Routing remains source-native + +**No write-path change is required.** Events already carry the namespace of the source object, which +feeds its resource identity and Git placement. `sourceNamespace` changes the *watched* namespace; it +must not substitute the WatchRule's control-cluster namespace into the Git path. This is the single +biggest thing making the change small rather than scary, and it is invisible from the API types +alone — so it is traced in [Appendix A](#appendix-a-the-source-objects-namespace-already-names-the-git-folder). + +## Implementation steps + +1. **Generalize the matcher.** Rename/extend `AllowedNamespaces` + ([clusterprovider_types.go:48](../../../api/v1alpha3/clusterprovider_types.go#L48)) into a + reusable `NamespaceMatcher`, keeping `AllowsNamespace` + ([clusterprovider_types.go:191](../../../api/v1alpha3/clusterprovider_types.go#L191)) and the new + source-side predicate as two thin wrappers over one helper, so the two policies cannot drift. + The Go type name may change; the **JSON field name must not**. +2. **API fields.** `WatchRule.spec.sourceNamespace` + ([watchrule_types.go:46](../../../api/v1alpha3/watchrule_types.go#L46)), + `GitTarget.spec.allowedSourceNamespaces` + ([gittarget_types.go](../../../api/v1alpha3/gittarget_types.go)), + `ClusterProvider.spec.allowWatchRuleSourceNamespaceOverride` + ([clusterprovider_types.go:77](../../../api/v1alpha3/clusterprovider_types.go#L77)), and the + priority-1 `SourceAuthorized` printer column. Run `task generate` and `task manifests`. +3. **Carry the effective namespace into the compiled rule.** `rulestore.CompiledRule` + ([store.go:20-39](../../../internal/rulestore/store.go#L20-L39)) holds only + `Source types.NamespacedName`; add an explicit source-namespace field rather than overloading + `Source`, which also names the rule object. +4. **Use it for selection.** `collectWatchRuleSelections` + ([watched_type_resolver.go:297](../../../internal/watch/watched_type_resolver.go#L297)) sets + `namespace: rule.Source.Namespace`; switch it to the effective source namespace. +5. **Resync path.** Confirm the snapshot/reconcile scope follows the same field — `desiredFromObject` + reads `u.GetNamespace()` + ([scope_resolve.go:200](../../../internal/watch/scope_resolve.go#L200)), but the *scope* it + iterates comes from the watched-type table, so step 4 should be sufficient. Verify, do not assume. +6. **Fingerprints.** `watchRuleFingerprint` + ([watched_type_resolver.go:478-482](../../../internal/watch/watched_type_resolver.go#L478-L482)) + hashes `rule.Source.Namespace` as its `src=` component; it **must** hash the effective source + namespace instead, or a change to the field will not re-project the table. This produces a stale + watch rather than a visible failure — one of the two steps that never announces itself. +7. **One compiled-rule path.** Route WatchRule compilation through a single gated function used by + both the reconciler and `bootstrapRuleStore` + ([bootstrap.go:49-68](../../../internal/watch/bootstrap.go#L49-L68)), so the gate cannot be + bypassed on restart. Do this *before* step 8, so there is only one place to add the check. +8. **The source-scope service.** Implement resolution, the three-valued readiness/error result, and + the enqueue edge described above, and extend `WatchManagerInterface` with it. +9. **The gate and status.** Add the three-part check in the shared compile path, after the + GitTarget fetch + ([watchrule_controller.go:186](../../../internal/controller/watchrule_controller.go#L186)) and + before `AddOrUpdateWatchRule` + ([watchrule_controller.go:225](../../../internal/controller/watchrule_controller.go#L225)), + modelled on `checkSourceAuthorization`. Set the condition per the contract above; on a terminal + refusal remove any compiled rule and replan **before** the Failed trio. Extend `applyRuleKstatus` + so the domain condition is a prerequisite and Unknown yields InProgress rather than a separate + status path. This produces a security hole rather than a visible failure — the other silent step. +10. **Reactivity.** Add the ClusterProvider → WatchRules mapper (the GitTarget → WatchRules edge + exists but is generation-filtered, so it will not carry a provider-driven change) and the + per-source-cluster label-filtered Namespace informer. +11. **Docs.** Every statement listed under + [Docs that become false](#docs-that-become-false-when-this-ships), the + WatchRule section of [status-conditions-guide.md](../../spec/status-conditions-guide.md), and the + INDEX entry. + +## Docs that become false when this ships + +Must change in the same PR: + +- the generated CRD description stating that WatchRule watches only its own namespace; +- [configuration.md](../../configuration.md) — "It only watches resources in its own namespace"; +- [configuration.md](../../configuration.md) — "It has no effect on which namespaces are read from + the source cluster; that remains entirely the source connection's Kubernetes RBAC." This is a + security-relevant claim about `allowedNamespaces` that this design directly changes. + +## Test plan + +Grouped by what they prove, because several exist to catch a *silent* failure. + +### The two tests that must exist + +**A WatchRule that omits `sourceNamespace` passes with no GitTarget policy and no delegation flag.** +If this fails, deny-by-default has broken every existing rule on upgrade. The gate must engage only +when the target declares no policy and the effective source namespace does not differ from the rule's +own. Place it first and make its name say so. + +**`TestBootstrap_DeniedSourceNamespaceIsNotCompiledOnRestart`.** Seed a WatchRule whose override the +policy denies, then run `bootstrapRuleStore` and assert no compiled rule exists when +`RuleStore.MarkReady()` returns — *before* any reconcile. Without this, the gate is a reconciler-only +check and every operator restart reopens the window it was written to close. It is the second +must-have because it is the one failure that a passing reconciler test suite actively hides. + +### Gate correctness — `internal/controller`, table-driven + +Modelled on `TestCheckSourceAuthorization` +([gittarget_source_cluster_test.go:117](../../../internal/controller/gittarget_source_cluster_test.go#L117)): + +| Case | Expected | +|---|---| +| `sourceNamespace` omitted, no policy, flag false | allowed (legacy) | +| equals the rule's own namespace, no policy, flag false | allowed | +| omitted, **policy declared** but does not list the rule's own namespace | denied, `SourceNamespaceNotAllowed` — the [no-self-namespace-exception](README.md#no-self-namespace-exception) rule, and the case most likely to be implemented as an accidental carve-out | +| omitted, policy declared and lists the rule's own namespace | allowed | +| differs, flag false | denied, `SourceNamespaceNotAllowed` | +| differs, flag true, target policy absent | denied (deny-by-default) | +| differs, flag true, target policy empty `{}` | denied (empty ≠ unrestricted) | +| differs, flag true, target names it | allowed | +| differs, flag true, target selector matches its labels | allowed | +| differs, flag true, target names a *different* namespace | denied | +| tenant-zen's target policy, acme's requested namespace | denied (target isolation) | +| invalid selector on the target policy | denied, `SourceNamespacePolicyUnavailable` | +| ClusterProvider read error (non-NotFound) | requeued as error, not silently denied | + +Plus the degradation path: with source Namespace access forbidden, an **exact-name** entry still +admits while a **selector** entry fails closed with `SourceNamespacePolicyUnavailable`. Both halves +need a case — the first is the one that will regress unnoticed. + +### The gate actually stops the data plane + +- `TestReconcile_DeniedSourceNamespaceStartsNoWatch` — mirrors + `TestReconcile_UnauthorizedNamespaceStartsNoWatch` + ([gittarget_source_cluster_test.go:320](../../../internal/controller/gittarget_source_cluster_test.go#L320)): + a denied rule leaves no compiled rule in RuleStore and starts no stream. +- **Revocation:** a rule accepted, then denied by a tightened GitTarget policy, must have its + compiled rule *removed* and the watch manager replanned — not merely reported unready. A gate that + only writes a condition is not a gate. +- Conditions: `SourceNamespaceAuthorized=False`, Ready=False, Reconciling=False, Stalled=True, with + the correct reason per terminal refusal class. + +### Status and kstatus + +- WatchRule table tests using the real `status.Compute` helper, following the existing GitTarget and + GitProvider kstatus tests. Assert all four rows of the table above. +- The domain condition independently: legacy is True with `LegacySourceNamespace`; an allowed + override is True with `SourceNamespaceAllowed`; a denied override is False; a retryable selector + lookup is Unknown. +- `observedGeneration` on the domain condition and every generic condition equals the WatchRule + generation after a `sourceNamespace`, GitTarget policy, or ClusterProvider flag change — preventing + a stale success from rendering as healthy. + +### Silent-failure guards — `internal/watch` + +- **Fingerprint:** two rules differing only in `sourceNamespace` produce different + `watchRuleFingerprint` values. Without this, the fingerprint step's omission is invisible until a stale watch is + noticed in production. +- **Selection:** `collectWatchRuleSelections` emits the effective source namespace — assert directly + on the resulting `watchSelection.namespace`. +- **Re-projection:** changing `sourceNamespace` on a stored rule rebuilds the watched-type table + (`rulesFingerprint` gates the rebuild at + [watched_type_resolver.go:88-96](../../../internal/watch/watched_type_resolver.go#L88-L96)). + +### Reactivity — envtest + +- Flipping `allowWatchRuleSourceNamespaceOverride` re-reconciles affected WatchRules. This + specifically catches the generation-predicate gap: without the new mapper the flag change reaches + the GitTarget's *status* only, and the WatchRules never re-run. +- Editing `GitTarget.allowedSourceNamespaces` re-reconciles its WatchRules — should pass on existing + wiring; assert it so a later predicate change cannot silently break it. +- Adding/removing the matching label on a **source-cluster** Namespace grants/revokes within a + bounded time. + +### End-to-end + +- A WatchRule in tenant-acme with `sourceNamespace: repo-config` against a remote source cluster + produces Git paths under `repo-config/…`, **not** `tenant-acme/…`. This is the end-to-end proof of + the appendix below, and the one assertion that would catch a regression there. +- A refused override surfaces `SourceNamespaceAuthorized=False`, Ready=False, Reconciling=False, + Stalled=True, kstatus Failed, and writes nothing to Git. + +## Done when + +- The legacy test above passes and no existing WatchRule changes behavior. +- A denied override leaves no stream running. +- `task lint`, `task test`, `task test-e2e` pass. +- PR 4 is queued — the field must not reach a release without its ClusterWatchRule half. + +--- + +## Appendix A: the source object's namespace already names the Git folder + +Verified against the tree on 2026-07-20. Evidence, not argument: it exists so the next reader need +not re-derive it, and so a regression is detectable. + +The value never comes from a config-plane object at all: + +~~~text +watch event (*unstructured, from the source cluster) + └─ u.GetNamespace() watch/target_watch.go:818 + └─ types.ResourceIdentifier{Namespace: …} types/identifier.go:18 + └─ git.Event.Identifier + └─ PlacementRequest.Identifier git/plan_flush.go:341 + ├─ placementVars → {namespace} manifestanalyzer/placement.go:436 + └─ canonicalPath → ToGitPath() types/identifier.go:58 +~~~ + +The resync/reconcile path ingests it the same way at a second site: `desiredFromObject` also reads +`u.GetNamespace()` ([scope_resolve.go:200](../../../internal/watch/scope_resolve.go#L200)). + +The config-plane namespace travels in a **separate field**, `git.Event.GitTargetNamespace` +([git/types.go:343](../../../internal/git/types.go#L343)). Every non-test use falls into exactly +three buckets — GitTarget/credential resolution, worker and commit-window identity keying, and the +write-permission guard plus logging. No `PlacementRequest` literal references it, and +`event.Identifier` is never rewritten from it. + +The one place a config-plane namespace becomes a "namespace" in the data plane is +`namespace: rule.Source.Namespace` +([watched_type_resolver.go:297](../../../internal/watch/watched_type_resolver.go#L297)) — a **watch +selector only**, discarded once the stream is open, because the identifier is rebuilt from +`u.GetNamespace()`. That line is precisely what step 4 changes. + +**So the write side needs zero change**: a rule in tenant-acme watching `repo-config` remotely already +renders `repo-config/…`. Three invariants confirm this is correct semantics rather than an accident: + +1. A placement template for a core Secret must be identity-complete — it must contain `{name}` and + either `{namespace}` or `{namespaceOrCluster}` + ([placement.go:550-552](../../../internal/manifestanalyzer/placement.go#L550-L552), enforced + statically at + [gittarget_placement_validation.go:67](../../../internal/controller/gittarget_placement_validation.go#L67)). + A config-plane `{namespace}` would collapse two source namespaces onto one path and silently break + uniqueness. The enforcement is admission-time and covers core Secrets; operator-configured + sensitive types rely on write-time guards instead. +2. The store keys `ByResourceIdentity` off the namespace **as written in the Git file** + ([store.go:1033](../../../internal/manifestanalyzer/store.go#L1033)), which must equal the live + object's. Strictly this is the *effective* identity: a namespace-less document can inherit its + namespace from a kustomization's `namespace:` transformer, with provenance in `NamespaceSource`. + Either way it is never a config-plane namespace. +3. Sibling inference pins on the source object's namespace (`cohortMembers`, + [placement.go:661](../../../internal/manifestanalyzer/placement.go#L661)), and the + `spansMultipleNamespaces` guard + ([placement.go:843](../../../internal/manifestanalyzer/placement.go#L843)) reads the *existing + store documents'* namespaces to prove a candidate file is namespace-agnostic. Two different + sources, neither of them config-plane. diff --git a/docs/design/watchrule-source-namespace/pr5-clusterwatchrule-source-ceiling.md b/docs/design/watchrule-source-namespace/pr5-clusterwatchrule-source-ceiling.md new file mode 100644 index 00000000..fae1c60f --- /dev/null +++ b/docs/design/watchrule-source-namespace/pr5-clusterwatchrule-source-ceiling.md @@ -0,0 +1,250 @@ +# PR 5 — a declared allowedSourceNamespaces bounds ClusterWatchRule too + +> Phase 5 of [source-namespace addressing](README.md). **Depends on:** +> [PR 4](pr4-source-namespace-field.md) (the field and the source-scope service), +> [PR 1](pr1-namespace-scoped-resync.md) (per-namespace expansion is unsafe until the sweep is +> namespace-scoped), and [PR 2](pr2-stream-scope-collapse.md). **Must ship with PR 4** — see the +> [release gate](README.md#implementation-phases). No new API fields; this changes what an existing +> field governs. + +## Why this is launch-set, not follow-up + +A multi-tenant deployment needs a ClusterWatchRule per tenant GitTarget **from day one**, because a +ClusterWatchRule is the only way to select cluster-scoped types and every tenant needs their CRDs +mirrored. So the rule kind that can bypass a WatchRule-only allow-list is not a rare edge — it is in +every tenant's baseline configuration. + +Without this PR, `allowedSourceNamespaces` is enforced on the rule kind that cannot bypass it and +unenforced on the one that can. The operator's answer to *"will this stream objects from namespaces +outside my allow-list?"* becomes "hand-audit every ClusterWatchRule and hope", and the audit has to +be repeated on every change by anyone with cluster-admin. With this PR the answer is a field you can +read off the GitTarget. + +The workaround — carefully writing ClusterWatchRules that contain only `scope: Cluster` rules — does +work today. It just cannot be *verified* from the GitTarget, which is where a tenant boundary should +be legible. + +## The invariant + +`ClusterWatchRule.targetRef` is a `NamespacedTargetReference` with a **required** namespace +([clusterwatchrule_types.go:22-44](../../../api/v1alpha3/clusterwatchrule_types.go#L22-L44)), so it +reaches across namespaces by design, and `collectClusterWatchRuleSelections` hardcodes +`namespace: ""` for every rule +([watched_type_resolver.go:317-318](../../../internal/watch/watched_type_resolver.go#L317-L318)). +Left alone, a ClusterWatchRule delivers every source namespace into its target's Git destination +regardless of what that target declared. + +So the allow-list is stated over the **destination**, and holds for both rule kinds: + +> When a GitTarget declares `allowedSourceNamespaces`, the source namespaces mirrored into it are +> **exactly** those the policy admits, for every rule of every kind. When it declares none, each rule +> kind keeps its legacy scope. + +There is **no implicit own-namespace exception** — a declared policy is exhaustive, including for a +WatchRule watching the namespace it lives in. The reasoning, and the authoring footgun it accepts, +are in [no self-namespace exception](README.md#no-self-namespace-exception); do not reintroduce the +carve-out here. Because a ClusterWatchRule has no own namespace, the kinds therefore differ only in +the legacy behavior when the field is **undeclared**: + +| `GitTarget.allowedSourceNamespaces` | WatchRule | ClusterWatchRule (`scope: Namespaced` rules) | +|---|---|---| +| Undeclared | Own namespace only (legacy) | All source namespaces (legacy) | +| Declared | Exactly what the policy admits | Exactly what the policy admits | + +One meaning for the field across both kinds — declared means ceiling, absent means no new grant — so +nobody has to remember which rule kind inverts it, and no existing ClusterWatchRule changes behavior +until a target owner declares a policy. + +### This is a restriction, so no delegation flag + +`allowWatchRuleSourceNamespaceOverride` gates *granting* a WatchRule a foreign namespace. The ceiling +only ever narrows, so it applies whether the flag is true or false. Gating a restriction behind a +delegation flag would mean an admin must grant extra authority in order to reduce scope. + +### Three precisions the implementation must not get wrong + +- **Cluster-scoped rules are exempt.** A namespace allow-list cannot constrain Nodes, CRDs, or + ClusterRoles. The ceiling applies per `ClusterResourceRule`, only where `scope: Namespaced`. A + ClusterWatchRule mixing both scopes keeps its cluster-scoped streams intact — and since the CRD use + case above *is* the cluster-scoped half, an over-broad implementation that refuses or narrows the + whole rule breaks the exact configuration this PR exists to serve. +- **Narrowing is not a refusal.** A declared ceiling admitting nothing leaves a namespaced-scope + ClusterWatchRule selecting no namespaces. That is a correct outcome, not a terminal failure: it + must not set `Stalled=True`. Report it through the existing `ResourcesResolved` surface and the + reason below, so a dead rule is still visible. +- **The ceiling does not partition cluster-scoped objects at all.** Stated plainly in the + [overview](README.md#what-the-ceiling-does-not-do): every tenant selecting CRDs gets every CRD the + credential can read. If tenants must not see each other's cluster-scoped objects, the answer is one + ClusterProvider and credential per tenant, not this field. + +### Status + +The narrowing is a scope change, not an authorization refusal, so it reuses the domain condition +[PR 4](pr4-source-namespace-field.md#status-contract-kstatus-compatible) defines rather than adding a +second one. On ClusterWatchRule, `SourceNamespaceAuthorized=True` with +reason `AllSourceNamespaces` when no ceiling applies, and `SourceNamespacesNarrowed` when one does. +The `SourceAuthorized` printer column is added to ClusterWatchRule as well, so the two rule kinds +read the same way in `kubectl get -o wide`. + +## Implementation + +### 1. Apply the ceiling in selection + +`collectClusterWatchRuleSelections` +([watched_type_resolver.go:306-323](../../../internal/watch/watched_type_resolver.go#L306-L323)) +hardcodes `namespace: ""`. When the referenced GitTarget declares `allowedSourceNamespaces`, expand +each `scope: Namespaced` rule into **one selection per admitted namespace** instead of a single `""` +selection. Leave `scope: Cluster` rules emitting `""`. + +**Prefer expansion over filtering events at the read site.** An expanded selection carries the scope +through the plan hash, the informers, **and** the resync path (`snapshotGVRsFromTable`, +[scope_resolve.go:180](../../../internal/watch/scope_resolve.go#L180)) for free. A read-site filter +must be repeated at each of those and is silently wrong if one is missed — and it would also mean an +unfiltered LIST/WATCH over all namespaces, so the data crosses into the process before being dropped. + +Two consequences worth knowing: + +- A name-based policy expands statically; a **selector**-based one makes the namespace set depend on + the source-cluster Namespace informer [PR 4](pr4-source-namespace-field.md) introduces, and can + produce many streams on a large cluster. That is the cost of the safe direction. +- A declared policy emits no `""` key **for its namespaced selections**, so the + [PR 2](pr2-stream-scope-collapse.md) collapse cannot trigger between two namespaced streams on that + target. It does **not** follow that the target emits no `""` key at all: a co-resident + `scope: Cluster` rule still emits one, so a GVR selected both cluster-scoped and namespaced can + still collapse. PR 2 governs that case, and the undeclared case, which remains the common one. + +### 2. Put the resolved scope into table invalidation + +`rulesFingerprint` is computed **only from compiled rules** — it iterates +`SnapshotWatchRules()` and `SnapshotClusterWatchRules()` and hashes their spec fields +([watched_type_resolver.go:463-500](../../../internal/watch/watched_type_resolver.go#L463-L500)) — +and it is what gates the table rebuild +([watched_type_resolver.go:88-96](../../../internal/watch/watched_type_resolver.go#L88-L96)). +`clusterWatchRuleFingerprint` has no `src=` component at all, on the assumption that a +ClusterWatchRule is always all-source-namespaces. Step 1 falsifies that assumption. + +The consequence is bigger than a missing hash component, and it is the failure mode to design +against: the new ceiling's inputs — GitTarget policy and source-cluster Namespace labels — are **not +rule state**, so nothing about them reaches the fingerprint. A mapper that requeues the +ClusterWatchRule is therefore not sufficient on its own: reconciliation runs, the fingerprint is +unchanged, the rebuild is skipped, and the resident table keeps the old namespace set. The streams +carry on at their previous width and every diff looks correct, because the rule object genuinely did +not change. + +**So carry a resolved-scope version into invalidation.** Either hash the resolved namespace set into +each rule's fingerprint, or add a separate source-scope generation to the rebuild trigger alongside +the rules fingerprint and the catalog generation. Hashing the resolved set is the smaller change and +composes with the existing gate; whichever is chosen, the test in the plan below asserts that an +unchanged rule object with a changed policy re-projects the table. + +This is why [PR 4](pr4-source-namespace-field.md)'s source-scope service must expose resolution to +the resolver, not only to the reconciler: the fingerprint is computed in `internal/watch` and needs +the same answer the gate got. + +### 2b. Unknown is not empty + +An unresolvable selector must **never** be treated as a valid empty allow-list. The distinction is +load-bearing because narrowing has a data-plane consequence: an empty resolved set means "watch +nothing in any namespace", and combined with a resync it means Git content for those namespaces is +no longer in `desired`. A transient source-cluster outage read as "the policy admits nothing" is +therefore not merely a stopped stream — it is the input to a sweep. This is the sharpest reason +[PR 1](pr1-namespace-scoped-resync.md) lands first and why its recommended retain-on-revocation +semantics matter. + +Required behavior when the resolved set is unknown — cache not synced, source cluster unreachable, +Namespace access denied for a selector policy: + +- **Retain the current resolved scope.** Do not narrow, do not widen, and do not sweep. The last + known-good scope keeps running. +- **Never synthesize an empty set.** "I could not evaluate" and "it admits nothing" must be different + values all the way through the resolver, which is what the three-valued result in PR 4's + source-scope service exists to provide. +- **Report it as non-terminal:** `SourceNamespaceAuthorized=Unknown` with reason + `CheckingSourceNamespacePolicy` while a retryable error is being retried, and + `SourceNamespacePolicyUnavailable` (still `Unknown`, still retained, **not** `Stalled`) when source + Namespace access is denied outright for a selector policy. Exact-name entries in the same policy + remain resolvable and keep working. + +A permanently unavailable selector is a legitimate long-lived `Unknown` **here**. Turning it into +`Stalled` would be a false claim that the operator knows the rule is wrong — the rule is still +running its cluster-scoped streams and its last-resolved namespaced ones — and turning it into an +empty set would be destructive. + +> **This is not in conflict with [PR 4](pr4-source-namespace-field.md), which makes the same +> permanent `Forbidden` terminal.** The ceiling is *maintaining* a scope, where failing closed would +> mean narrowing to nothing and sweeping; PR 4's gate is *establishing* one, where failing closed +> means never starting a stream and nothing is at risk. The single contract, with the table that +> settles which value applies where, is +> [establishing versus maintaining a scope](pr4-source-namespace-field.md#establishing-versus-maintaining-a-scope). +> A ClusterWatchRule that has *never* resolved its ceiling is in the establishing column too. + +### 3. Reactivity + +| Input changes | Required reaction | Status | +|---|---|---| +| GitTarget `allowedSourceNamespaces` | Re-resolve the ceiling and replan the ClusterWatchRule's streams. | Not wired — the ClusterWatchRule controller performs no GitTarget-driven re-resolution. Needs a GitTarget → ClusterWatchRules mapper. | +| Source-cluster Namespace labels | Re-resolve selector-based ceilings. | Extends the per-source-cluster informer [PR 4](pr4-source-namespace-field.md#reactivity-and-source-cluster-rbac) adds, to map to ClusterWatchRules as well. | +| ClusterProvider `allowedNamespaces` | Already handled by [PR 3](pr3-clusterwatchrule-target-admission.md)'s mapper. | — | + +## Test plan + +These prove the invariant. Without them the allow-list is enforced only where it cannot be bypassed. + +- **`TestCollectClusterWatchRuleSelections_DeclaredCeilingNarrowsClusterWideStream`** — a + ClusterWatchRule with a `scope: Namespaced` rule against a GitTarget declaring + `allowedSourceNamespaces: {names: [repo-config]}` emits selections for `repo-config` **only**, and + emits **no** `""` selection. Assert the absence of the `""` key directly: `""` present alongside + `repo-config` reads as "we did narrowing" in a diff while behaving as all-namespaces at runtime, + because `SnapshotNamespaces()` short-circuits on it + ([watched_type_table.go:78-92](../../../internal/watch/watched_type_table.go#L78-L92)). +- **`TestCollectClusterWatchRuleSelections_UndeclaredCeilingStaysClusterWide`** — the upgrade-safety + twin. No policy on the GitTarget means today's single `""` selection, unchanged. +- **`TestCollectClusterWatchRuleSelections_CeilingSparesClusterScopedRules`** — a ClusterWatchRule + mixing `scope: Cluster` (CRDs) and `scope: Namespaced` rules under a declared ceiling keeps the + cluster-scoped stream at `""` while the namespaced one narrows. This is the day-one multi-tenant + shape; it guards the over-correction of narrowing or refusing the whole rule. +- **`TestCollectClusterWatchRuleSelections_CeilingAdmittingNothingIsNotStalled`** — a declared policy + matching no namespace yields no namespaced selections, `SourceNamespaceAuthorized=True` with reason + `SourceNamespacesNarrowed`, and **not** the Failed trio. +- **`TestClusterWatchRuleFingerprint_ChangesWithResolvedSourceScope`** — two otherwise-identical + ClusterWatchRules whose GitTargets declare different `allowedSourceNamespaces` fingerprint + differently, and tightening a policy changes the fingerprint of an unchanged rule object. The rule + spec is byte-identical across both cases, so nothing else in the suite can catch a missing `src=` + component. +- **`TestWatchedTypeTable_RebuildsWhenOnlyThePolicyChanged`** — the invalidation twin of the above, + one level up: with the rule object untouched, editing `GitTarget.allowedSourceNamespaces` must + actually re-project the resident table, not merely re-run reconciliation. This is the test that + catches "the mapper fired but the fingerprint was unchanged, so the rebuild was skipped". +- **`TestCeiling_UnknownScopeRetainsPreviousAndDoesNotSweep`** — with the source-scope service + reporting *cannot say* (cache unsynced or source unreachable), the resolved namespace set is + retained, no narrowing occurs, and **no resync/sweep is enqueued**. Assert the absence of the + sweep, not only the condition — this is the path where a wrong answer deletes Git content. +- **`TestCeiling_ForbiddenSelectorIsUnknownNotStalled`** — a ClusterWatchRule that **has already + resolved** a ceiling, whose source Namespace access is then denied, reports + `SourceNamespaceAuthorized=Unknown` with `SourceNamespacePolicyUnavailable`, keeps running at its + last known scope, and does **not** set `Stalled=True`. Exact-name entries in the same policy keep + resolving. +- **`TestCeiling_ForbiddenSelectorBeforeFirstResolutionIsStalled`** — the establishing twin, and the + test that keeps the two halves of the contract from drifting: a ClusterWatchRule whose selector + ceiling has **never** resolved, under the same denial, is `False` / + `SourceNamespacePolicyUnavailable` / `Stalled=True` with no namespaced streams. Without this pair + an implementation can satisfy either page alone while contradicting the other. +- **Revocation, envtest** — a running ClusterWatchRule under `allowedSourceNamespaces: + [repo-config, team-a]`, narrowed to `[repo-config]`, stops the `team-a` stream within a bounded + time. Not merely re-renders status. +- **Selector reactivity, envtest** — removing the matching label from a source-cluster Namespace + revokes that namespace's stream for a ClusterWatchRule under a selector ceiling. +- **End-to-end, the one that proves the boundary** — the day-one multi-tenant shape: a + ClusterWatchRule selecting CRDs (`scope: Cluster`) **and** ConfigMaps (`scope: Namespaced`), + targeting acme's GitTarget which declares `allowedSourceNamespaces: [repo-config]`. Create a + ConfigMap in `repo-config` and one in `tenant-zen`, and a CRD. Assert all three: `repo-config/…` + appears, the CRD appears, and `tenant-zen/…` is **never** written. Assert the negative against a + real commit — every unit-level assertion above is on the plan, not on what reached Git. + +## Done when + +- A declared ceiling narrows namespaced ClusterWatchRule streams and leaves cluster-scoped ones + intact. +- Tightening a ceiling stops the removed namespaces' streams without touching the rule object. +- The e2e above passes, including its negative assertion. +- `task lint`, `task test`, `task test-e2e` pass. diff --git a/docs/finished/multi-cluster-author-attribution.md b/docs/finished/multi-cluster-author-attribution.md index c46b9eeb..aa887065 100644 --- a/docs/finished/multi-cluster-author-attribution.md +++ b/docs/finished/multi-cluster-author-attribution.md @@ -1,474 +1,192 @@ -# `ClusterProvider` multi-cluster author-attribution design record +# `ClusterProvider` multi-cluster author attribution: decision record > **finished** — shipped or closed. Kept for context only; **nothing here binds**. For current > behaviour see [`../architecture.md`](../architecture.md), [`../configuration.md`](../configuration.md), > and [`../../SECURITY.md`](../../SECURITY.md). Index: [`../INDEX.md`](../INDEX.md) > -> This design produced the cluster-scoped `ClusterProvider`, immutable -> `GitTarget.spec.clusterProviderRef`, provider-name-partitioned attribution facts, reconcile-time -> namespace authorization, and named audit routes. The remaining multi-source ingress work is tracked -> separately in [`../design/multi-source-audit-ingress-hardening.md`](../design/multi-source-audit-ingress-hardening.md). +> Produced the cluster-scoped `ClusterProvider`, immutable `GitTarget.spec.clusterProviderRef`, +> provider-name-partitioned attribution facts, reconcile-time namespace authorization, and named +> audit routes. Remaining ingress work: +> [`../design/multi-source-audit-ingress-hardening.md`](../design/multi-source-audit-ingress-hardening.md). +> Purge-on-delete was later answered separately: +> [`clusterprovider-fact-purge.md`](clusterprovider-fact-purge.md). -## Final implemented shape - -`ClusterProvider` is the cluster-scoped read-side peer of `GitProvider`. A `GitTarget` references a -provider by immutable name; an omitted reference defaults to `default`, which is only a convention — -any provider name may use the in-cluster client or a remote kubeconfig. That provider name partitions -the source client, discovery and watch state, and author-attribution facts. Its -`spec.allowedNamespaces` policy is deny-by-default and is checked on every reconcile before watches -start. - -Audit ingestion uses `/audit-webhook/` and stores facts in that provider's partition. The -current server verifies that the sender is signed by the audit CA and that the named provider exists; -it does **not** bind an individual certificate to a provider. This is an accepted shared-control-plane -trust boundary, not tenant isolation. The outstanding provider-bound identity, annotation-routing -trust, and ingress-fairness decisions are deliberately kept out of this archived design. - -## Design history - -The material below is the working record that led to the shipped shape. It retains alternatives that -were rejected or deliberately not implemented — especially the earlier reserved-`default`, admission -webhook, finalizer, and per-provider-certificate proposals — and must not be read as the current API -contract. - -## Problem +## The problem Author attribution is a **join keyed by `(group/resource, object-uid, resourceVersion)`** with no -cluster dimension: - -- **Write side** — the local apiserver POSTs audit events to `/audit-webhook`; the handler - ([`audit_handler.go`](../../internal/webhook/audit_handler.go)) records a minimal fact per - accepted mutation via `RecordFact` - ([`attribution_index.go`](../../internal/queue/attribution_index.go)), keyed - `…:author:v1:audit::object::` (plus a `:last` pointer and an rv-only - `:rv:` hatch). -- **Read side** — a live watch event calls `AuthorResolver.ResolveAuthor(ctx, gvr, uid, rv, - exactCapable)` ([`author_resolver.go`](../../internal/watch/author_resolver.go), - [`target_watch.go`](../../internal/watch/target_watch.go) `attachAuthor`), reading the fact back - by `(group/resource, uid, rv)`. - -Config-plane-split broke the symmetry: a remote `GitTarget` now **watches a remote cluster** (remote -UIDs/RVs), but the audit webhook is **local-only** (`validateAuditWebhookPath` rejects any cluster-id -segment). A remote watch event looks up `(gr, uid, rv)`, finds no fact, and ships as the committer. - -Two things are missing, and both need a **name for the cluster**: an *ingress* a remote apiserver can -reach tagged with which cluster it is, and a *cluster dimension in the keyspace* so a fact from -cluster A never joins a watch event from cluster B. - -## Identity model — the name is the key; the UID only authenticates - -`GitTarget.SourceClusterID()` (`//`) and every use of that string as a -data-plane key are **removed** (see *What we remove*). It fused two identities and leaked a Secret -reference. The replacement is **one identity, the provider name**, with the UID confined to the auth -layer: - -| Concern | Value | Why | -|---|---|---| -| **External** — the audit route | `ClusterProvider.metadata.name` | admin-chosen, DNS-safe, known before install (bakeable into apiserver params at cluster-creation time), immutable | -| **Internal** — the fact-index cluster key, GVR scoping, the `clusters` map key | **the same name** | unique because cluster-scoped; on the audit path already; known to the watch side with no lookup; **local keys by `default`** — no `""` special case | -| **Authentication** — which incarnation is talking | `ClusterProvider.metadata.uid` | binds the per-provider client cert to one incarnation; revoked on delete (see *Authenticated ingress*) | - -**Why the name is the key, not the UID (a reversal from v1, and a pushback on the review's -"UID-keyed facts").** Two reasons: - -1. **UID does not deliver the incarnation safety it seems to.** The audit *route* is name-based, so a - delayed/retried batch from a *deleted* cluster still hits `/audit-webhook/prod-eu-1`, resolves to - the **current** provider, and lands under whatever key we choose. UID-in-the-key does not stop - that; only **rejecting the stale sender** does (auth revocation) — plus a **fact purge on - delete**. Those close it regardless of key shape. -2. **The name needs no translation.** The audit path already carries it; the watch side already knows - its provider. UID-keying would add a `name → uid` lookup on *both* the write and read paths for no - gain, and make Redis keys opaque. - -(The concern that first surfaced this — an implicit-local cluster has no object and therefore no UID — -is now moot, since local is a shipped object with a name; but the two reasons above stand on their own, -and are why keying by name survives even now that every source has a UID.) - -So the UID is used where it is actually load-bearing — **binding the authenticated sender to a -physical incarnation** — and nowhere else. `git.Event.SourceClusterID` / -`ResolvedTargetMetadata.SourceClusterID` become **`SourceCluster`** carrying the provider *name* -(the shipped local provider's name for local); the target-scoped GVK→GVR resolution -config-plane-split added is unchanged in shape. - -## Decision - -1. **Add a cluster-scoped `ClusterProvider`** — the read-side peer of `GitProvider`; a `GitTarget` - references one by name (`spec.clusterProviderRef`), as it already references a `GitProvider`. -2. **Identity = the provider name**, everywhere; UID only authenticates (above). Retire - `SourceClusterID`. -3. **The provider carries a namespace-access policy** (`spec.allowedNamespaces`, **deny-by-default**); - a `GitTarget` may reference it only from an allowed namespace — enforced at admission *and* before - any watch starts. -4. **Authenticated ingress is a prerequisite**, not a late add: a per-provider client cert bound to - the provider, `cert-provider == path-provider`, revoked on delete. Remote paths are refused until - this exists. -5. **The fact index gains a cluster dimension** keyed by provider **name** (the local provider - included); a finalizer purges a provider's facts on delete. -6. **Local is the reserved `default` `ClusterProvider`** (kubeConfig omitted = in-cluster); - `clusterProviderRef` **defaults to `{name: "default"}`** (concrete, jumpable — never `nil`). CEL - enforces *named `default` iff no kubeConfig*, so name-uniqueness makes it a singleton (no webhook). - The chart ships it (`watchLocal: true`); `watchLocal: false` → not-found → `NotReady`. No `isDefault`. -7. **v1 = audit attribution on self-managed clusters only.** Remote `attribution.mode` defaults to - **`None`**; **`Admission` is deferred entirely** (it does not unlock managed clusters); provider - `kubeConfig` is **immutable**; workload identity and mutable repointing are deferred. - -``` -GitTarget ─ spec.clusterProviderRef ─▶ ClusterProvider (source: READ + attribution, authorized per namespace) - ─ spec.providerRef ────────▶ GitProvider (destination: WRITE) -``` - -## Why this is config-plane-split's planned step +cluster dimension. The write side records a fact per accepted mutation from the local apiserver's +audit stream ([`audit_handler.go`](../../internal/webhook/audit_handler.go) → `RecordFact`); the +read side resolves it back on each live watch event +([`author_resolver.go`](../../internal/watch/author_resolver.go)). -Config-plane-split argued against a dedicated CRD, but its reasoning was conditional: *"A dedicated -CRD's remaining benefits — reuse across many `GitTarget`s, platform-admin RBAC ownership, a home for -connectivity status — are real but not needed for the first version … its load-bearing rationale is -gone: `main` removed the `/audit-webhook/` path … Multi-cluster is now purely a kube-API -story."* Re-adding attribution restores the audit story and makes all three deferred benefits needed. -It pre-drew the shape: a **sibling** `sourceClusterRef` naming a `ClusterConnection`/`SourceCluster` -CRD "that only platform admins may create … referenced by name from the `GitTarget`." This is that -object. +Config-plane split broke the symmetry: a remote `GitTarget` watches a **remote** cluster (remote +UIDs and RVs) while the audit webhook was **local-only**. A remote watch event found no fact and +shipped as the committer. -## Does an object keep the Flux vision? Mostly — one item deferred +Two things were missing, and both needed a **name for the cluster**: an ingress a remote apiserver +can reach, tagged with which cluster it is, and a cluster dimension in the keyspace so a fact from +cluster A never joins an event from cluster B. -Moving the kubeconfig from inline into a `ClusterProvider` embeds the **same** `meta.KubeConfigReference` -type, so the Flux-shaped roadmap is preserved: +## What shipped -| config-plane-split capability | On a `ClusterProvider` | -|---|---| -| Embed `meta.KubeConfigReference` verbatim; a Flux kubeconfig Secret works unchanged | **Preserved** — same embedded type | -| `value`→`value.yaml` key order; reject-not-strip `exec`/insecure-TLS | **Preserved** — same resolver, run by the provider reconciler; verdict on the provider | -| Workload identity (`configMapRef`, `provider: generic`→cloud) | **Preserved, deferred** — in the type; a v1 CEL guard rejects it (config-plane-split already deferred it) | -| ServiceAccount impersonation (`serviceAccountName`) | **Preserved, more Flux-faithful** — a sibling of `kubeConfig` on the provider, as in a Flux Kustomization | -| Target-scoped GVK→GVR (no union) | **Preserved** — keyed by provider name | -| Split `Validated`/`SourceClusterReachable` conditions | **Preserved, re-homed** onto the provider | - -The object **unlocks** what config-plane-split parked for lack of a home — per-provider `qps`/`burst` -(off the global `--source-cluster-qps/-burst` flags), per-provider attribution mode, and the status -surface — which is exactly the *"real second property"* it said a wrapper lacked. **One thing is now -deferred, not unlocked:** mutable rotation. v1 promised it; the review is right that a mutable -*endpoint* silently retargets and misattributes (below), so **`kubeConfig` is immutable in v1**; only -Secret *contents* rotation stays transparent (the resolver re-reads — as config-plane-split already -did). Immutability sits on both `GitTarget.spec.clusterProviderRef` (which cluster a folder sources -from = folder identity) and `ClusterProvider.spec.kubeConfig` (which physical cluster a name means). - -## Local — the reserved `default` provider, and a defaulted ref - -Local is a **first-class `ClusterProvider`** named **`default`**: `kubeConfig` is optional, and -**omitted means in-cluster** (the operator's own cluster via in-cluster config — Flux's "the cluster I -run in"). The chart **ships it** by default (`attribution.watchLocal: true`, named `default`). - -**The ref is *defaulted*, never `nil`.** `GitTarget.spec.clusterProviderRef` carries a schema default of -`{name: "default"}`, so a target that omits it persists with a concrete ref to the `default` provider — -there is no implicit-`nil` sentinel. This is deliberately chosen over "`nil` means local" for one -forward-looking reason: **every reference is always populated and jumpable**, so a "follow reference" -(F12) traversal over the object graph never hits an implicit hop with nowhere to land. It also makes -`kubectl get gittarget -o yaml` self-describing — the source cluster is always shown, even for local. - -**`default` is reserved for the in-cluster cluster, enforced by per-object CEL** — no validating webhook -needed for this (a simplification over the v3 draft, which used one). The rule is *named `default` **iff** -`kubeConfig` is absent*: - -```go -// +kubebuilder:validation:XValidation:rule="(self.metadata.name == 'default') == !has(self.spec.kubeConfig)",message="the ClusterProvider named 'default' is the in-cluster provider and must omit kubeConfig; every other provider must set kubeConfig" -``` - -Kubernetes name-uniqueness makes `default` a singleton for free, so "exactly one in-cluster provider" -falls out — the cross-object count the v3 webhook did is gone. The bare `/audit-webhook` path, the ref -default, and "the operator's own cluster" all coincide on `default`. - -**Turning local watching off** stays explicit: `watchLocal: false` ships no `default` provider, and a -`GitTarget` (defaulted or explicit) referencing `default` is then a clear `NotReady` — the *same* -"referenced ClusterProvider not found" path as any missing remote, not a special case. - -**We still reject an `isDefault` flag.** `default` is a **fixed reserved name on an immutable object**, -not a movable pointer: because `clusterProviderRef` is immutable (a folder's source *is* its identity) -and `kubeConfig` is immutable, nothing about `default` can be re-pointed to silently retarget the -folders bound to it. A mutable/movable default (`isDefault`) would reintroduce exactly that -silent-retarget hazard, so it stays rejected. - -Because local is a named object it keys facts by its **name** (`default`) like every other provider (no `""` -special case) and gets the same status surface; the bare `/audit-webhook` path resolves to it (the -single in-cluster provider), so the local apiserver's config stays simple. - -## Authorization — the namespace is a *policy on the provider*, not the Secret's location - -**The confused-deputy this must close is data export, not credential read.** A cluster-scoped provider -holds a credential that may read a lot of a remote cluster. Any `GitTarget` that references it causes -the operator — using *its* credential — to mirror that cluster's state into the `GitTarget`'s -destination. So a tenant who can create a `GitTarget` **and their own `GitProvider`** could reference a -platform provider and **export whatever the operator can read on the remote into a repo they control**. -Pinning the kubeconfig Secret to the operator namespace does **not** help: the tenant never touches the -Secret. (The v1 draft's claim that Secret-pinning "closes the confused-deputy by construction" was -wrong and is retracted.) - -**Fix: a namespace-access policy on the provider, deny-by-default.** - -```yaml -spec: - allowedNamespaces: # deny-by-default: empty = no namespace may reference this provider - names: [team-a, team-b] # explicit list, and/or - selector: {matchLabels: {tier: trusted}} # a label selector on namespaces -``` - -Enforced in **two** places: a validating admission webhook rejects a `GitTarget` whose namespace is -not allowed by its referenced provider, and the watch manager **refuses to start watches** for a -target that fails the check at reconcile (defense in depth against a policy that tightened after the -`GitTarget` was created). Denial is tested explicitly. - -This is config-plane-split's "RBAC on the object" model made real: platform admins create providers -and decide who may reference them; tenants reference by name and are bounded by the policy. - -## Authenticated ingress — per-provider client identity, before any remote path - -Path is **routing, not authentication.** A name is guessable; on its own, anyone who can reach -`:9444/audit-webhook/` could inject false author facts. Today the server already does -`RequireAndVerifyClientCert` against a CA ([`main.go`](../../cmd/main.go): `buildAuditServerTLSConfig`), -but the chart issues **one** client cert with `commonName: kube-apiserver` -([`audit-certificates.yaml`](../../charts/gitops-reverser/templates/audit-certificates.yaml)) and the -handler **never reads the peer certificate** — so every source is indistinguishable and the path is the -only (unauthenticated) discriminator. That is fine for one local apiserver; it is not enough the moment -a second cluster can POST. - -**One audit server, not one per cluster** (a listener per cluster is unscalable and pointless). The -per-cluster distinction is the **client identity**: - -- Each `ClusterProvider` has its **own client credential** — a cert whose subject/SAN (or a pinned - SPKI fingerprint) maps to that provider. The apiserver on the source cluster presents it. -- The handler **reads `r.TLS.PeerCertificates`**, maps it to a provider, and **requires - `cert-provider == path-provider`** — a forged path without the matching cert is rejected. -- The credential is **bound to the provider incarnation (UID)** and **revoked on delete**, so a - delayed batch from a deleted/recreated cluster fails auth instead of misattributing (the P0.3 fix; - the fact-purge finalizer is the belt to this suspenders). - -This is a **prerequisite**, sequenced *before* remote path routing — the operator must never accept a -remote fact it cannot attribute to an authenticated provider. The cert issuance/rotation/revocation -flow is genuinely new design (the chart's cert-manager path mints the *local* client cert but cannot -reach a remote apiserver's filesystem), and is an explicit build step, not an afterthought. - -**Answer to "separate TLS per webhook or just path?"** — one server, path for routing, a per-provider -**client cert** for authentication, required to agree. Never path-only. - -## API shape - -```yaml -apiVersion: configbutler.ai/v1alpha3 -kind: ClusterProvider # cluster-scoped -metadata: - name: prod-eu-1 # identity: /audit-webhook/prod-eu-1 AND the fact-index key -spec: - kubeConfig: # OPTIONAL: omit for the in-cluster (local) provider. IMMUTABLE in v1. - secretRef: # Flux's meta.KubeConfigReference; secretRef pinned to the operator ns - name: prod-eu-1-kubeconfig - # configMapRef: {...} # RESERVED — Flux workload identity; v1 CEL guard rejects it - # serviceAccountName: ... # RESERVED — Flux remote impersonation; future sibling - allowedNamespaces: # authorization, DENY-BY-DEFAULT (empty = none) - names: [team-a] - attribution: - mode: None # None (default) | Audit. Admission is deferred (not in the enum) - qps: 20 # outgoing kube client throttle (was --source-cluster-qps) - burst: 40 - ingressLimits: # INCOMING audit protection — distinct from qps/burst - maxEventsPerSecond: 500 # per-provider ceiling; excess is shed, counted, not queued unbounded -status: - observedGeneration: 1 - conditions: # Validated, Reachable, DiscoveryHealthy (per cluster, not per GitTarget) - - type: Ready - lastAuditEventTime: "..." # NOT "AuditIngestionActive": a quiet cluster is not unready -``` +`ClusterProvider` is the cluster-scoped read-side peer of `GitProvider`. A `GitTarget` references +one by immutable name; that name partitions the source client, discovery and watch state, and the +attribution facts. `spec.allowedNamespaces` is deny-by-default and is checked on every reconcile, +before any watch starts. -```go -// GitTarget names its source by a ref. Inline spec.kubeConfig is removed. -type GitTargetSpec struct { - // … providerRef (GitProvider, write side), branch, path (unchanged, immutable) … - // ClusterProviderRef names the SOURCE cluster. DEFAULTS to {name: "default"} (the in-cluster - // provider) — a concrete, jumpable ref, never nil. IMMUTABLE. - // +kubebuilder:default={name: "default"} - // +optional - ClusterProviderRef *ClusterProviderReference `json:"clusterProviderRef,omitempty"` -} -// +kubebuilder:validation:XValidation:rule="self.clusterProviderRef == oldSelf.clusterProviderRef",message="spec.clusterProviderRef is immutable" +```text +GitTarget ─ spec.clusterProviderRef ─▶ ClusterProvider (source: READ + attribution, authorized per namespace) + ─ spec.providerRef ────────▶ GitProvider (destination: WRITE) ``` -The shipped local provider is the same kind, reserved-named `default`, with `kubeConfig` omitted: - -```yaml -kind: ClusterProvider -metadata: {name: default} # chart-shipped when attribution.watchLocal=true; RESERVED name -spec: - # no kubeConfig => in-cluster. CEL: (name == "default") == !has(kubeConfig) - allowedNamespaces: {...} # who may bind local (chart sets a sensible default) - attribution: {mode: None} # or Audit, fed by the local apiserver on the bare /audit-webhook path -``` +Audit ingestion uses `/audit-webhook/` and stores facts in that provider's partition. The +server verifies the sender is signed by the audit CA and that the named provider exists; it does +**not** bind an individual certificate to a provider. That is an accepted shared-control-plane +trust boundary, not tenant isolation — see the hardening doc. + +--- + +## The decisions, and what each one replaced + +### The name is the identity; the UID only authenticates + +`GitTarget.SourceClusterID()` (`//`) was **deleted, not adapted**. It fused +two identities and leaked a Secret reference. One identity replaced it — the provider **name** — +used for the audit route, the fact-index key, the GVK→GVR registry key, and the `clusters` map key. +`git.Event.SourceClusterID` became `SourceCluster`, carrying the name. + +An earlier revision keyed facts by **UID**. Reversed, for two reasons: + +1. **UID does not deliver the incarnation safety it appears to.** The audit *route* is name-based, + so a delayed batch from a deleted cluster still hits `/audit-webhook/prod-eu-1` and resolves to + the *current* provider. UID-in-the-key does not stop that; only rejecting the stale sender does. +2. **The name needs no translation.** It is already on the audit path and already known to the + watch side. UID-keying would add a `name → uid` lookup on *both* paths for no gain, and make + Redis keys opaque. + +### `default` is a defaulting convention, **not** a reserved local name + +This one was reversed late and matters most, because the rejected version is the intuitive one. + +An earlier draft made `default` **reserved for the in-cluster cluster**, enforced by CEL +`(self.metadata.name == 'default') == !has(self.spec.kubeConfig)`, with the chart shipping the +object. **None of that shipped.** + +What shipped: local-vs-remote follows from **`spec.kubeConfig` alone** — omitted means the +operator's own cluster, for *any* name +([`clusterprovider_types.go:174-178`](../../api/v1alpha3/clusterprovider_types.go#L174-L178)). A +provider named `default` may carry a `kubeConfig` and mirror a remote cluster; a provider named +anything else may omit it and be in-cluster. `default` is only the value that +`GitTarget.spec.clusterProviderRef` defaults to +([`gittarget_types.go:98`](../../api/v1alpha3/gittarget_types.go#L98)), and the operator never +creates that object — a user does. + +Two things drove the reversal. Pinning a *name* to a physical cluster is the same +silent-retarget hazard that `kubeConfig` immutability exists to prevent — it just moves it into the +schema. And the reserved name bought nothing the existence check did not already give: a +`GitTarget` naming a provider that does not exist is held unready through the ordinary +"provider not found" path, so turning local watching off needs no special case. + +The **ref is defaulted, never `nil`**, which did survive: every reference is populated and jumpable, +so a "follow reference" traversal never hits an implicit hop with nowhere to land, and +`kubectl get gittarget -o yaml` is self-describing. An `isDefault` **flag was rejected** — a movable +default would reintroduce the retarget hazard directly. + +> Downstream consequence: locality is derived from `kubeConfig`, never from the name, and +> `GitTarget.IsLocalSource()` — despite the name — **is a name test** that only seeds the +> pre-discovery `SourceClusterReachable` default. It is not a locality predicate. A later design +> took the further step of not deriving *permissions* from locality at all: see +> [`../design/watchrule-source-namespace/`](../design/watchrule-source-namespace/README.md), whose +> PR 3 page restates this trap verbatim. + +### Namespace authorization enforced once, on reconcile + +The confused deputy to close is **data export, not credential read**. A cluster-scoped provider +holds a credential that can read a lot of a remote cluster; any `GitTarget` referencing it makes +the operator mirror that state into the target's destination. So a tenant who can create a +`GitTarget` *and their own `GitProvider`* could export whatever the operator can read into a repo +they control. Pinning the kubeconfig Secret to the operator namespace does **not** help — the +tenant never touches the Secret. (An earlier claim that Secret-pinning "closes the confused deputy +by construction" was wrong and is retracted.) + +The fix is `spec.allowedNamespaces` on the provider, **deny-by-default** (empty admits nobody), +names and/or a label selector, ORed. + +This draft specified enforcement **"in two places"**, one of them a validating admission webhook. +**One shipped**, on every reconcile, returning before `DeclareForGitTarget` +([`gittarget_controller.go:311`](../../internal/controller/gittarget_controller.go#L311)) — which +also covers a policy tightened *after* the `GitTarget` was created, something admission cannot see. +That reasoning was generalized into a repo-wide rule: +[`../spec/where-validation-lives.md`](../spec/where-validation-lives.md). + +### Keyspace and the recorder API + +A `cluster:` infix on `factKeyExact/Last/RV`, the in-cluster provider included, so there is no +`""` special case. The rv-only hatch became `(cluster, group/resource, rv)` — **a correctness fix**, +since RV is not globally unique and the no-UID hatch was cross-cluster-ambiguous without it. +`RecordFact` gained an explicit `providerName` argument; `ResolveAuthor` / `LookupAuthorResolution` +the same. + +### Admission attribution: cut entirely + +An earlier draft sold a remote `ValidatingWebhookConfiguration` as the *"easy, identical-identity"* +fallback for managed clusters. Wrong on both counts, and removed from the enum: + +- **Not identical.** A `ValidatingWebhookConfiguration` configures the webhook URL and CA, not a + client identity for the *apiserver*. For the apiserver to authenticate *to* the webhook needs an + `AdmissionConfiguration` via `--admission-control-config-file` — an apiserver flag. +- **Not a managed-cluster unlock.** That flag is the same class of thing managed control planes + hide. [`attribution-setup-guide.md`](../attribution-setup-guide.md) already scopes attribution to + self-managed control planes. + +So it bought an *unauthenticated* callback on exactly the clusters where audit also fails, plus a +fail-open webhook that risks blocking tenant writes, plus a weaker (`:last`-only, no post-write RV) +join. The honest managed-cluster answer is a **source-side agent** — a separate, larger feature. +Command authorship (`/validate-operator-types`) is untouched by this and is unrelated. -The cluster-scoped `secretRef` needs a namespace Flux's type lacks (pinned to the operator ns) — the -*only* departure from verbatim-Flux reuse; the embedded type is otherwise unchanged. - -## Ingress, keyspace, and the recorder API - -- **Routing.** `ServeHTTP` switches on the path: bare `/audit-webhook` → the **`default`** (in-cluster) - provider; `/audit-webhook/` → the named provider **iff** the peer cert authenticates it; - unknown/unauthorized → 404/403. `validateAuditWebhookPath` relaxes to "accept a segment naming an - *authenticated* provider." -- **Keyspace.** A `cluster:` infix on `factKeyExact/Last/RV`, using the resolved provider name — - the local provider included, so there is no `""` special case. The rv-only hatch becomes - `(cluster, group/resource, rv)` — **the correctness fix** (RV is not globally unique, so the no-UID - hatch was cross-cluster-ambiguous without this). Facts are ephemeral (15-min TTL, - [`DefaultAttributionFactTTL`](../../internal/queue/attribution_index.go)); a **delete finalizer purges - `cluster::*`** so a recreated name starts clean. -- **Recorder API (explicit).** `RecordFact(ctx, providerName string, event auditv1.Event)` — the - current interface has **no** cluster argument ([`audit_handler.go`](../../internal/webhook/audit_handler.go) - `AuditFactRecorder`); the handler resolves the authenticated provider and threads its **name**. - `ResolveAuthor`/`LookupAuthorResolution` gain the same `providerName` (the local provider's name for - a `nil`-ref target); `attachAuthor` passes it. - -## Timing — batch-max-wait vs the grace window is a stated prerequisite - -Exact attribution needs the audit fact to arrive within the resolver's grace window -([`DefaultAttributionGraceWindow`](../../internal/watch/author_resolver.go) = 3s). The apiserver's -`--audit-webhook-batch-max-wait` **defaults to 30s**, so at low mutation volume a fact can arrive after -the commit and the event ships as committer. This is **not new** — the local path relies on the same -relationship, and the repo already handles it: e2e sets `--audit-webhook-batch-max-wait=1s` -([`start-cluster.sh`](../../test/e2e/cluster/start-cluster.sh)) and the chart NOTES / setup guide make -low max-wait the freshness knob. The multi-cluster design makes it **explicit and per-provider**: a low -`batch-max-wait` on the source apiserver is a documented prerequisite, and the grace is -**per-`ClusterProvider` configurable** for links that are laggier than local. (A missed fact degrades -to a committer commit — never wrong, just less rich — so this is a freshness SLO, not a correctness -bug.) - -## Status — a quiet cluster is not unready - -Owned by the provider: `Validated` (inputs), `Reachable` (discovery reached the API), -`DiscoveryHealthy` (types + followability, per cluster), aggregated `Ready`, with `observedGeneration`. -Attribution health is a **`lastAuditEventTime` timestamp**, *not* an `AuditIngestionActive` condition — -a normally-quiet cluster with no recent mutations must not read as unready. If a readiness signal is -wanted, it is `Unknown` until the first event, never `False` for silence. `GitTarget` **projects** a -one-line `ClusterProviderReady` (the `GitProviderReady` pattern, with a `Watches(&ClusterProvider{})` -trigger), so per-cluster detail lives once on the shared object, not copied onto every target. - -## Admission attribution — deferred entirely from v1 - -The v1 draft sold a remote `ValidatingWebhookConfiguration` as the *"easy, identical-identity"* fallback -for managed clusters. That is **wrong on both counts**, and it is removed from the v1 enum: - -- **Not identical.** A `ValidatingWebhookConfiguration` configures the webhook **server URL + CA**, not - a client identity for the *apiserver*. For the apiserver to authenticate *to* the webhook (so we can - trust and attribute the caller) needs an `AdmissionConfiguration` via `--admission-control-config-file` - — an **apiserver flag**. -- **Not a managed-cluster unlock.** That flag is the *same class* of thing managed control planes hide. - The product's own [attribution-setup-guide.md](../../docs/attribution-setup-guide.md) already scopes - attribution to self-managed control planes and lists EKS/GKE/AKS as **not supported**. Admission does - not change that. - -So admission attribution buys us an *unauthenticated* callback on exactly the clusters where audit also -fails, plus a fail-open webhook that risks blocking tenant writes, plus a weaker (`:last`-only, no -post-write RV) join. The honest managed-cluster answer is a **source-side agent** (runs in the cluster, -authenticates outward) — a separate, larger future feature. Admission stays out of v1; if it ever -returns it needs its own authenticated-source design, and command authorship (`/validate-operator-types`, -config-plane-only) is untouched regardless. - -## CRD invariants (to specify before coding) - -- `clusterProviderRef` / `providerRef`: typed local references; `clusterProviderRef` **defaults to - `{name: "default"}`** and is immutable. -- `attribution.mode`: enum `{None, Audit}`, default `None`; `configMapRef` rejected by CEL. -- `kubeConfig`: **optional (omitted = in-cluster), immutable**; `secretRef` namespace pinned (operator - ns); **CEL `(name == "default") == !has(kubeConfig)`** — the reserved `default` is the sole in-cluster - provider (name-uniqueness gives the singleton; **no validating webhook** for it); no `isDefault` field. -- `qps`/`burst`: positive, bounded defaults; `ingressLimits.maxEventsPerSecond` required with a bounded - default. -- `allowedNamespaces`: deny-by-default; names and/or selector; semantics enforced at admission + reconcile. -- Status: `observedGeneration`, kstatus-style `Ready` aggregation, `lastAuditEventTime`, no - quiet-cluster-unready condition. - -## What we remove (and it must not be left in place) - -The `SourceClusterID` string is **not** something to keep — it is deleted, not adapted: - -- **`GitTarget.SourceClusterID()`** ([`gittarget_types.go`](../../api/v1alpha3/gittarget_types.go)) — - deleted. Resolution is `spec.clusterProviderRef.name` → the provider (→ its uid only for auth). -- **The `//` string as any data-plane key** — gone. `clusters - map[string]*clusterContext` and `clusterIDForGitTarget` key by **provider name** (`default` for the - in-cluster one; no `""` sentinel and no implicit-`nil` case — the ref is always populated). -- **`git.Event.SourceClusterID` / `ResolvedTargetMetadata.SourceClusterID`** → renamed **`SourceCluster`**, - carrying the provider *name*. -- **Inline `GitTarget.spec.kubeConfig`** — removed (unreleased; no migration). -- **The v1-draft `cluster:` fact infix and any `name→uid` fact-key lookup** — never built; the - infix is the name. -- **The "mutable kubeConfig rotation" idea** — not built in v1 (immutable); Secret-contents rotation - stays. - -## Minimal safe v1, and what is deferred - -**v1 (build):** `ClusterProvider` (kubeConfig optional = in-cluster, immutable) + a **shipped local -provider** (`clusterProviderRef` defaults to `{name: "default"}`) + immutable source identity + **namespace authorization** + -**name-keyed facts** + **authenticated audit ingress** + per-provider ingress limits + name-based status -with `lastAuditEventTime`. - -**Deferred (explicitly not v1):** admission attribution; workload identity (`configMapRef`); -ServiceAccount impersonation; mutable kubeConfig / endpoint repointing; an `isDefault`/movable default -(the ref defaults to the reserved `default` provider instead); managed-control-plane support (needs a -source-agent). - -## Build order - -1. **`ClusterProvider` CRD + reconciler** — cluster-scoped; kubeConfig **optional (omitted = - in-cluster)** + immutable; the **`(name == "default") == !has(kubeConfig)`** CEL; `attribution.mode` - (`None` default); `allowedNamespaces`; validate kubeconfig (exec/TLS reject) → `Validated`; - `observedGeneration`. -2. **Retire `SourceClusterID`; re-home the engine onto the provider name** — delete - `SourceClusterID()`; key `clusters` by name; rename the `git` carriers to `SourceCluster` (name); - delete inline `kubeConfig`; **default `clusterProviderRef` to `{name: "default"}`** so a ref-less - target resolves to the reserved in-cluster provider. Behavior-preserving. -3. **Namespace authorization** — a validating webhook rejecting a `GitTarget` in a non-allowed - namespace; reconcile-time refusal to start watches; denial tests. (Before any remote data flows.) - *(The in-cluster singleton is CEL from step 1 — no webhook needed for it.)* -4. **Authenticated ingress** — per-provider client credential contract (subject/SAN or SPKI), handler - reads the peer cert, `cert-provider == path-provider`, incarnation-binding + revoke-on-delete. - (Before routing accepts remote paths.) -5. **Ingress routing + name-keyed facts** — relax `validateAuditWebhookPath` to authenticated - providers; `RecordFact(ctx, providerName, event)`; `cluster:` infix; thread `providerName` - through the read path; delete-finalizer fact purge; per-provider `ingressLimits`. Unit-prove a - single-provider install's keyspace matches a bare install. -6. **Status + projection** — `Reachable`/`DiscoveryHealthy`/`lastAuditEventTime` on the provider; - `ClusterProviderReady` projected onto `GitTarget` with a `Watches(&ClusterProvider{})` trigger; - per-provider grace override. -7. **Chart + docs** — **ship the local `ClusterProvider` by default** (`watchLocal: true`, sensible - `allowedNamespaces`); issue/rotate per-provider client certs; document the remote apiserver - audit-config recipe (name + low `batch-max-wait`); extend the attribution setup guide. - -## Test plan - -Reuse config-plane-split's kcp harness (`test/e2e/kcp_workspace_test.go`) where a real remote is needed. - -- **Unit** — a single-provider install's keyspace matches a bare install; cross-cluster isolation incl. - the rv-only hatch; recorder threads the provider name. -- **Local binding & defaulting** — a `GitTarget` omitting `clusterProviderRef` persists with - `{name: "default"}` and binds to the `default` provider; with `watchLocal:false` (no `default`) it is - `NotReady` via the ordinary "provider not found" path; CEL rejects a non-`default` provider without a - kubeConfig **and** a `default` provider *with* one; the ref is always populated (jumpable), never nil. -- **Authorization** — a `GitTarget` in a non-allowed namespace is rejected at admission and never - starts watches; allowed namespace works; tightening the policy stops an existing target. -- **Authentication** — cert/path mismatch is rejected; a provider's cert authenticates only its own - path; cert rotation continues ingestion; revocation on delete stops it. -- **Recreation/repoint** — stale audit delivery to a recreated name fails auth (not misattributed); - the fact purge ran; `kubeConfig` mutation is rejected (immutable). -- **Timing** — with `batch-max-wait` above the grace, a low-volume mutation degrades to committer (not - wrong); with the e2e's 1s max-wait it attributes exactly. -- **Isolation** — a noisy provider hitting `ingressLimits` sheds its own excess without starving other - providers' ingestion, Redis, or the commit queue. -- **e2e (kcp)** — remote author round-trip (`/audit-webhook/`, authored by the real user); the - three-workspace same-`(ns, ConfigMap, name)` non-leak centerpiece; `ClusterProviderReady` projection - and recovery. - -## Open questions - -1. **Namespace-authorization surface.** `allowedNamespaces` (names + selector) on the provider vs a - `ReferenceGrant`-like object each namespace opts in with. The former is one object for the admin; - the latter is standard cross-namespace-consent shape. Recommendation: `allowedNamespaces` - deny-by-default now; revisit ReferenceGrant if per-namespace self-service consent is wanted. -2. **Per-provider client-credential mechanism.** mTLS client cert (subject/SAN map, or SPKI pin) vs a - per-provider bearer token in the audit webhook kubeconfig. Cert fits the existing mTLS setup; token - is simpler for some operators. Pick one contract, incl. rotation/revocation. -3. **`:last`-as-weak** — only relevant if admission ever returns; leave the read policy exact-only for v1. -4. **Ingress backpressure policy** — shed vs buffer-with-bound when a provider exceeds `ingressLimits`, - and how that surfaces (a condition? a metric only?). +### Timing is a prerequisite, not a bug + +Exact attribution needs the fact inside the resolver's grace window +([`DefaultAttributionGraceWindow`](../../internal/watch/author_resolver.go) = 3s), while the +apiserver's `--audit-webhook-batch-max-wait` **defaults to 30s**. Not new — the local path always +relied on the same relationship, and e2e sets `1s`. Made explicit and per-provider here. A missed +fact degrades to a committer commit — never wrong, just less rich — so this is a freshness SLO. + +### Status: a quiet cluster is not unready + +`Validated` / `Reachable` / `DiscoveryHealthy` on the provider, aggregated into `Ready`, projected +onto `GitTarget` as a one-line `ClusterProviderReady` (the `GitProviderReady` pattern) so +per-cluster detail lives once on the shared object. Attribution health is a **`lastAuditEventTime` +timestamp**, deliberately *not* an `AuditIngestionActive` condition — a normally-quiet cluster with +no recent mutations must not read as unready. + +--- + +## Deferred, and why + +| Deferred | Why | +|---|---| +| Per-provider client-certificate binding | Real gap; the current CA-only check is a shared-control-plane boundary. Moved whole to [`../design/multi-source-audit-ingress-hardening.md`](../design/multi-source-audit-ingress-hardening.md). | +| Admission attribution | Cut entirely (above) — needs a source-side agent, not a webhook. | +| Mutable `kubeConfig` / endpoint repointing | v1 promised it; a mutable *endpoint* silently retargets and misattributes. `kubeConfig` is immutable; Secret **contents** rotation stays transparent. | +| Workload identity (`configMapRef`), ServiceAccount impersonation | Present in the embedded Flux type; rejected by CEL for now. | +| Fact purge on delete | Answered separately and **rejected**: [`clusterprovider-fact-purge.md`](clusterprovider-fact-purge.md). | +| Managed control planes (EKS/GKE/AKS) | Needs the source-side agent. | +| `ReferenceGrant`-style per-namespace consent | `allowedNamespaces` is one object for the admin; revisit if self-service consent is wanted. | + +## Why this was config-plane-split's planned step + +Config-plane split argued against a dedicated CRD, but conditionally: its remaining benefits — +reuse across many `GitTarget`s, platform-admin RBAC ownership, a home for connectivity status — +were *"real but not needed for the first version"*, and its load-bearing rationale was gone because +`main` had removed the `/audit-webhook/` path. Re-adding attribution restored the audit +story and made all three benefits needed. It even pre-drew the shape: a sibling ref naming a CRD +*"that only platform admins may create … referenced by name from the `GitTarget`."* + +Moving the kubeconfig from inline into the provider embeds the **same** +`meta.KubeConfigReference`, so the Flux-shaped roadmap survives intact (`value`→`value.yaml` key +order, reject-not-strip `exec`/insecure-TLS, target-scoped GVK→GVR, split +`Validated`/`SourceClusterReachable`). The object also **unlocked** what config-plane split parked +for lack of a home — per-provider `qps`/`burst`, per-provider attribution mode, and a real status +surface. The one departure from verbatim-Flux reuse: the cluster-scoped `secretRef` needs a +namespace Flux's type lacks, pinned to the operator namespace. diff --git a/docs/spec/README.md b/docs/spec/README.md index ebd8ed37..58ec3b38 100644 --- a/docs/spec/README.md +++ b/docs/spec/README.md @@ -33,6 +33,7 @@ If you change one of these behaviours, change the document in the same commit. | [`type-followability.md`](type-followability.md) | is a type followable, and if not, the single reason | | [`type-lifecycle-events-and-wobble-settling.md`](type-lifecycle-events-and-wobble-settling.md) | removal grace and flap coalescing | | [`gvk-gvr-mapping-layer.md`](gvk-gvr-mapping-layer.md) | the GVK↔GVR bijection contract | +| [`where-validation-lives.md`](where-validation-lives.md) | schema → CEL → **the reconciler**; a webhook only for what exists solely at admission | | [`sops-single-file-no-multidoc.md`](sops-single-file-no-multidoc.md) | one encrypted file is one document | | [`scale-subresource-audit-rehydration.md`](scale-subresource-audit-rehydration.md) | `/scale` → bounded field patch; every other subresource ignored | | [`commit-window-refactor.md`](commit-window-refactor.md) | one grouped commit = one (author, GitTarget) | diff --git a/docs/spec/where-validation-lives.md b/docs/spec/where-validation-lives.md new file mode 100644 index 00000000..3ab64dd1 --- /dev/null +++ b/docs/spec/where-validation-lives.md @@ -0,0 +1,64 @@ +# Where validation lives: schema, then CEL, then the reconciler + +> **spec** — current behaviour. The code depends on this document; change one, change the other. Index: [`../INDEX.md`](../INDEX.md) + +One rule, repo-wide, so it stops being re-argued per feature. + +## The ladder + +Validate at the **first** rung that can express the rule: + +1. **OpenAPI schema** — types, enums, required, min/max, immutability where the shape allows. +2. **CEL `XValidation`** — anything expressible from the object *itself*, including + transitions against `oldSelf`. Example: `spec.kubeConfig` immutability on `ClusterProvider` + ([`clusterprovider_types.go:68`](../../api/v1alpha3/clusterprovider_types.go#L68)). +3. **The reconciler** — everything **cross-object**, because CEL cannot read another object. + Refusal surfaces as `Validated=False` with a reason, and **returns before** the object + reaches the data plane. + +**A validating admission webhook is not a rung on this ladder.** It is only correct when the +information being validated *exists only at admission time* and is nowhere on the persisted +object. That is a narrow case, and we ship exactly one instance of it (below). + +## Why cross-object checks go to the reconciler, not a webhook + +Three things, in order of how often they get argued backwards: + +- **Reconcile-time is the *stronger* gate.** Admission is one-shot at write time and cannot see + a policy tightened *after* the object was created. The reconcile gate re-evaluates + continuously, so tightening a policy stops work that is already running. An admission-only + check is strictly less safe. +- **The data-plane ordering is what provides the security property**, not the rejection. Nothing + is read from a source cluster and nothing is written to Git until the check has passed. A + rejected-at-admission object and a `Validated=False` object have **identical blast radius: + zero**. +- **What admission actually buys is feedback latency** — `kubectl apply` fails in the user's + terminal instead of succeeding and leaving a condition they have to go look at. That is a UX + property, and it is worth less than the install cost of a webhook: cert wiring in the chart, + a failure mode that can block tenant writes, and an extra moving part in every install path. + +The worked example is `ClusterProvider.spec.allowedNamespaces`. An earlier design proposed +enforcing it "in two places", one of them a webhook. What shipped enforces it in **one**, on +every reconcile, before `DeclareForGitTarget` +([`gittarget_controller.go:311`](../../internal/controller/gittarget_controller.go#L311), +[`gittarget_source_cluster.go:68`](../../internal/controller/gittarget_source_cluster.go#L68)) — +and that is why the quickstart is a single command again. + +## The one webhook we ship, and why it is not an exception to the rule + +[`/validate-operator-types`](../../internal/webhook/validate_operator_types_handler.go#L26) +is **not a validation gate**. It captures the *submitter's identity* on a command kind +(`CommitRequest`) so a commit can be authored by a real Kubernetes user, and it **always +allows**. Identity is the textbook case for admission: it exists in the `AdmissionRequest` and +on no persisted field, so no reconciler could recover it later. See +[`commitrequest-admission-authorship.md`](commitrequest-admission-authorship.md). + +(`/validate-all` — [`admission_allow_handler.go`](../../internal/webhook/admission_allow_handler.go#L12) +— is an always-allow observation surface wired only by the e2e SUT. The chart does not ship it.) + +## Applying this + +When a new rule is cross-object, do not open the webhook question. Write the check in the +reconciler, return before the data-plane declaration, and surface `Validated=False` with a +specific reason. Where a same-object CEL rule can express *part* of it, add CEL too — but never +as the only gate, for the tightened-after-creation reason above. diff --git a/internal/git/commit.go b/internal/git/commit.go index d87ad92d..9b587ec9 100644 --- a/internal/git/commit.go +++ b/internal/git/commit.go @@ -70,7 +70,7 @@ func renderReconcileCommitMessageFromEvents( func renderReconcileCommitMessage( count int, gitTarget string, - scopeGVR *schema.GroupVersionResource, + scope *ResyncScope, revision string, config CommitConfig, ) (string, error) { @@ -79,11 +79,12 @@ func renderReconcileCommitMessage( GitTarget: gitTarget, Revision: revision, } - if scopeGVR != nil { - data.Group = scopeGVR.Group - data.Version = scopeGVR.Version - data.Resource = scopeGVR.Resource - data.APIVersion = buildAPIVersion(scopeGVR.Group, scopeGVR.Version) + if scope != nil { + data.Group = scope.GVR.Group + data.Version = scope.GVR.Version + data.Resource = scope.GVR.Resource + data.APIVersion = buildAPIVersion(scope.GVR.Group, scope.GVR.Version) + data.Namespace = scope.Namespace } return renderCommitTemplate("reconcile", config.Message.ReconcileTemplate, data) } @@ -148,7 +149,12 @@ func ValidateCommitConfig(config CommitConfig) error { // Validate the per-type splice reconcile path with the type and revision fields populated, // so a custom reconcile template that names its synced type ({{.Resource}} / {{.APIVersion}}) // or pins the {{.Revision}} is exercised at admission exactly as a per-type reconcile renders it. - sampleScope := schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + // The sample scope names a namespace so a template referencing {{.Namespace}} — populated + // only by a namespace-scoped reconcile — is validated here too. + sampleScope := ResyncScope{ + GVR: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, + Namespace: "example-namespace", + } if _, err := renderReconcileCommitMessage(1, "example-target", &sampleScope, "12345", config); err != nil { return err } diff --git a/internal/git/commit_request_attach_test.go b/internal/git/commit_request_attach_test.go index 17d01b7c..b542995a 100644 --- a/internal/git/commit_request_attach_test.go +++ b/internal/git/commit_request_attach_test.go @@ -411,12 +411,12 @@ func TestAttach_ResyncCutOffCarriesMessageAndResolvesOnPush(t *testing.T) { // Before the deadline, a resync for a different type cuts the window // (resync-before-apply). The cut commit must carry the attached message. - scope := schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + scope := ResyncScope{GVR: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}} resultCh := make(chan ResyncResult, 1) loop.handleResyncRequest(&ResyncRequest{ GitTargetName: "team-a", GitTargetNamespace: "default", - ScopeGVR: &scope, + Scope: &scope, Result: resultCh, }) require.NoError(t, (<-resultCh).Err) diff --git a/internal/git/commit_test.go b/internal/git/commit_test.go index 0e0c5f2c..4fb7d731 100644 --- a/internal/git/commit_test.go +++ b/internal/git/commit_test.go @@ -141,8 +141,9 @@ func TestRenderReconcileCommitMessage_DefaultTemplateNamesScopedTypeAndRevision( } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - gvr := tc.gvr - message, err := renderReconcileCommitMessage(tc.count, "demo", &gvr, tc.revision, ResolveCommitConfig(nil)) + scope := ResyncScope{GVR: tc.gvr} + message, err := renderReconcileCommitMessage( + tc.count, "demo", &scope, tc.revision, ResolveCommitConfig(nil)) require.NoError(t, err) assert.Equal(t, tc.expected, message) }) @@ -150,7 +151,7 @@ func TestRenderReconcileCommitMessage_DefaultTemplateNamesScopedTypeAndRevision( } func TestRenderReconcileCommitMessage_NilScopeRendersCleanly(t *testing.T) { - // A whole-target reconcile carries no ScopeGVR, so the default template must fall back to + // A whole-target reconcile carries no Scope, so the default template must fall back to // the type-less subject rather than emit an empty "/" token. message, err := renderReconcileCommitMessage(6, "demo", nil, "", ResolveCommitConfig(nil)) require.NoError(t, err) @@ -158,11 +159,11 @@ func TestRenderReconcileCommitMessage_NilScopeRendersCleanly(t *testing.T) { } func TestRenderReconcileCommitMessage_CustomTemplateUsesTypeAndRevisionFields(t *testing.T) { - gvr := schema.GroupVersionResource{Version: "v1", Resource: "secrets"} + scope := ResyncScope{GVR: schema.GroupVersionResource{Version: "v1", Resource: "secrets"}} message, err := renderReconcileCommitMessage( 6, "signing-snapshot-dest", - &gvr, + &scope, "1331", ResolveCommitConfig(&v1alpha3.CommitSpec{ Message: &v1alpha3.CommitMessageSpec{ diff --git a/internal/git/resync_flush.go b/internal/git/resync_flush.go index 59a42244..f546edf3 100644 --- a/internal/git/resync_flush.go +++ b/internal/git/resync_flush.go @@ -11,7 +11,6 @@ import ( gogit "github.com/go-git/go-git/v5" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" - "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/log" "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" @@ -49,7 +48,7 @@ func (l *branchWorkerEventLoop) stashDeferredHeal(req *ResyncRequest) { } l.deferredHeals = append(l.deferredHeals, req) l.w.Log.V(1).Info("heal resync deferred until the commit window is idle", - "scopeGVR", scopeGVRString(req.ScopeGVR), + "scope", req.Scope.String(), "gitTarget", req.GitTargetNamespace+"/"+req.GitTargetName, "deferred", len(l.deferredHeals)) } @@ -75,7 +74,7 @@ func resyncHealKey(req *ResyncRequest) healKey { return healKey{ name: req.GitTargetName, namespace: req.GitTargetNamespace, - scope: scopeGVRString(req.ScopeGVR), + scope: req.Scope.String(), } } @@ -90,7 +89,7 @@ func (l *branchWorkerEventLoop) applyResync(req *ResyncRequest) { l.w.Log.Info("Handling resync request", "resources", len(req.Desired), "revision", req.Revision, - "scopeGVR", scopeGVRString(req.ScopeGVR), + "scope", req.Scope.String(), "heal", req.Heal, "gitTarget", req.GitTargetNamespace+"/"+req.GitTargetName, "openWindow", l.openWindow != nil, @@ -148,13 +147,6 @@ func (l *branchWorkerEventLoop) applyResync(req *ResyncRequest) { req.reply(ResyncResult{Stats: *stats}) } -func scopeGVRString(gvr *schema.GroupVersionResource) string { - if gvr == nil { - return "" - } - return gvr.String() -} - // buildResyncPendingWrite resolves the GitTarget's write metadata (path, encryption, // signer) and packages the desired snapshot into a retained resync pending write. The // stats pointer is threaded onto the pending write so the apply can populate the @@ -189,7 +181,7 @@ func (w *BranchWorker) buildResyncPendingWrite( Kind: PendingWriteResync, Desired: req.Desired, Revision: req.Revision, - ScopeGVR: req.ScopeGVR, + Scope: req.Scope, ResyncStats: stats, CommitConfig: ResolveCommitConfig(provider.Spec.Commit), Signer: signer, @@ -248,7 +240,7 @@ func (w *BranchWorker) executeResyncPendingWrite( } stats, anyChanges, err := w.applyResyncToWorktree( - ctx, worktree, base, target.SourceCluster, pendingWrite.Desired, pendingWrite.ScopeGVR, target.Placement, + ctx, worktree, base, target.SourceCluster, pendingWrite.Desired, pendingWrite.Scope, target.Placement, ) if err != nil { return 0, err @@ -267,7 +259,7 @@ func (w *BranchWorker) executeResyncPendingWrite( // commitMetadata through the verbatim path. changed := stats.Created + stats.Updated + stats.Deleted rendered, err := renderReconcileCommitMessage( - changed, target.Name, pendingWrite.ScopeGVR, pendingWrite.Revision, pendingWrite.CommitConfig) + changed, target.Name, pendingWrite.Scope, pendingWrite.Revision, pendingWrite.CommitConfig) if err != nil { return 0, err } @@ -329,7 +321,7 @@ func (w *BranchWorker) applyResyncToWorktree( worktree *gogit.Worktree, base, clusterID string, desired []manifestanalyzer.DesiredResource, - scopeGVR *schema.GroupVersionResource, + scope *ResyncScope, policy *manifestanalyzer.PlacementPolicy, ) (ResyncStats, bool, error) { root := worktree.Filesystem.Root() @@ -351,7 +343,7 @@ func (w *BranchWorker) applyResyncToWorktree( // see identical bytes. The planner is the authoritative mark-and-sweep over the resolved // resource-identity index; the upserts reuse the steady-state writer. A scoped resync // (M12 per-type) restricts the sweep to one type so no sibling document is dropped. - plan := resyncPlan(batch.store, scoped.scan.YAMLFiles, desired, scopeGVR) + plan := resyncPlan(batch.store, scoped.scan.YAMLFiles, desired, scope) stats, err := batch.applyResyncPlan(ctx, desired, plan) if err != nil { @@ -461,25 +453,26 @@ func eventForDesired(dr manifestanalyzer.DesiredResource) Event { } } -// resyncPlan builds the mark-and-sweep plan for a resync. A nil scopeGVR is the -// whole-GitTarget resync (BuildPlan sweeps every managed document absent from desired); a -// non-nil scopeGVR is the M12 per-type reconcile/sweep, where BuildScopedPlan restricts the -// sweep to that type's (group, resource) so a removed type's documents drop while every -// sibling type is left exactly as Git holds it. The upsert side is scoped by desired itself. +// resyncPlan builds the mark-and-sweep plan for a resync. A nil scope is the whole-GitTarget +// resync (BuildPlan sweeps every managed document absent from desired); a non-nil scope is the +// M12 per-type reconcile/sweep, where BuildScopedPlan restricts the sweep to that type's +// (group, resource) — and, when the scope names a namespace, to that namespace — so a removed +// type's documents drop while every sibling type, and every sibling namespace, is left exactly +// as Git holds it. The upsert side is scoped by desired itself. +// +// The namespace half is load-bearing once one GitTarget watches a type in more than one +// namespace: the replay that produced desired covered a single namespace, so sweeping the whole +// type would delete every other namespace's documents of that type. See ResyncScope. func resyncPlan( store *manifestanalyzer.ManifestStore, files []manifestedit.FileContent, desired []manifestanalyzer.DesiredResource, - scopeGVR *schema.GroupVersionResource, + scope *ResyncScope, ) manifestanalyzer.Plan { - if scopeGVR == nil { + if scope == nil { return manifestanalyzer.BuildPlan(store, files, desired, resyncPlanPolicy()) } - gvr := *scopeGVR - inScope := func(ri types.ResourceIdentifier) bool { - return ri.Group == gvr.Group && ri.Resource == gvr.Resource - } - return manifestanalyzer.BuildScopedPlan(store, files, desired, resyncPlanPolicy(), inScope) + return manifestanalyzer.BuildScopedPlan(store, files, desired, resyncPlanPolicy(), scope.Matches) } // resyncPlanPolicy is the planning policy for a resync: the same sanitized projection diff --git a/internal/git/resync_flush_test.go b/internal/git/resync_flush_test.go index 2d231a4e..59e23048 100644 --- a/internal/git/resync_flush_test.go +++ b/internal/git/resync_flush_test.go @@ -211,7 +211,7 @@ func secretManifest(name string) string { "data:\n k: dg==\n" } -// The M12 per-type sweep: a ScopeGVR'd resync with an empty desired set drops only the +// The M12 per-type sweep: a scoped resync with an empty desired set drops only the // removed type's documents and leaves every sibling type exactly as Git holds it — even // though under a whole-folder resync the sibling would also be an orphan. func TestResync_ScopedSweepDropsOnlyTargetType(t *testing.T) { @@ -221,7 +221,7 @@ func TestResync_ScopedSweepDropsOnlyTargetType(t *testing.T) { secretFull := seedPlacedManifest(t, worktree, "apps/secret.yaml", secretManifest("sec")) w := &BranchWorker{contentWriter: writer, mapper: twoTypeMapper()} - scope := &schema.GroupVersionResource{Group: "", Version: "v1", Resource: "configmaps"} + scope := &ResyncScope{GVR: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "configmaps"}} stats, changed, err := w.applyResyncToWorktree(context.Background(), worktree, "", "", nil, scope, nil) require.NoError(t, err) require.True(t, changed, "the removed type's document is swept") diff --git a/internal/git/resync_heal_test.go b/internal/git/resync_heal_test.go index 3dc7b955..176a264d 100644 --- a/internal/git/resync_heal_test.go +++ b/internal/git/resync_heal_test.go @@ -36,12 +36,12 @@ func TestHandleResyncRequest_HealDefersWhileWindowOpenThenApplies(t *testing.T) require.NotNil(t, loop.openWindow, "the edit must open a window") // A HEAL resync for the same GitTarget arrives while the window is open. - scope := schema.GroupVersionResource{Version: "v1", Resource: "configmaps"} + scope := ResyncScope{GVR: schema.GroupVersionResource{Version: "v1", Resource: "configmaps"}} healCh := make(chan ResyncResult, 1) loop.handleResyncRequest(&ResyncRequest{ GitTargetName: "team-a", GitTargetNamespace: "default", - ScopeGVR: &scope, + Scope: &scope, Heal: true, Desired: nil, Result: healCh, @@ -86,11 +86,11 @@ func TestHandleResyncRequest_AtomicDrainsDeferredHealFirst(t *testing.T) { CommitMode: CommitModePerEvent, }}) require.NotNil(t, loop.openWindow) - scope := schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + scope := ResyncScope{GVR: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}} healCh := make(chan ResyncResult, 1) loop.handleResyncRequest(&ResyncRequest{ GitTargetName: "team-a", GitTargetNamespace: "default", - ScopeGVR: &scope, Heal: true, Result: healCh, + Scope: &scope, Heal: true, Result: healCh, }) require.Len(t, loop.deferredHeals, 1, "the heal parks behind the open window") @@ -133,12 +133,12 @@ func TestHandleResyncRequest_HealDoesNotStealSiblingCommitRequestWindow(t *testi require.NotNil(t, loop.openWindow.pendingCR, "the window must carry the attached CommitRequest") // A heal scoped to a DIFFERENT GitTarget arrives on the shared worker. - otherScope := schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + otherScope := ResyncScope{GVR: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}} healCh := make(chan ResyncResult, 1) loop.handleResyncRequest(&ResyncRequest{ GitTargetName: "team-other", GitTargetNamespace: "default", - ScopeGVR: &otherScope, + Scope: &otherScope, Heal: true, Result: healCh, }) diff --git a/internal/git/resync_push_test.go b/internal/git/resync_push_test.go index caaab457..74df2c5c 100644 --- a/internal/git/resync_push_test.go +++ b/internal/git/resync_push_test.go @@ -64,12 +64,12 @@ func TestHandleResyncRequest_ClosedWindowIsPushedEvenWhenNoOpResync(t *testing.T // A type-scoped resync for a DIFFERENT type with an empty desired set: it closes // the open window (resync-before-apply) but its own mark-and-sweep — scoped to a // type with no documents — changes nothing, so the resync itself does not commit. - scope := schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + scope := ResyncScope{GVR: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}} resultCh := make(chan ResyncResult, 1) loop.handleResyncRequest(&ResyncRequest{ GitTargetName: "team-a", GitTargetNamespace: "default", - ScopeGVR: &scope, + Scope: &scope, Desired: nil, Result: resultCh, }) diff --git a/internal/git/resync_scope_test.go b/internal/git/resync_scope_test.go new file mode 100644 index 00000000..4f632256 --- /dev/null +++ b/internal/git/resync_scope_test.go @@ -0,0 +1,196 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +var configmapsGVRForScope = schema.GroupVersionResource{Group: "", Version: "v1", Resource: "configmaps"} + +// cmManifestIn renders a ConfigMap in an explicit namespace, so a test can seed the same +// type in two namespaces and prove a scoped sweep touches only one of them. +func cmManifestIn(name, namespace, color string) string { + return "apiVersion: v1\nkind: ConfigMap\n" + + "metadata:\n name: " + name + "\n namespace: " + namespace + "\n" + + "data:\n color: " + color + "\n" +} + +// desiredCMIn builds a desired ConfigMap snapshot entry in an explicit namespace. +func desiredCMIn(name, namespace, color string) manifestanalyzer.DesiredResource { + return manifestanalyzer.DesiredResource{ + Resource: types.ResourceIdentifier{ + Group: "", Version: "v1", Resource: "configmaps", Namespace: namespace, Name: name, + }, + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{"name": name, "namespace": namespace}, + "data": map[string]interface{}{"color": color}, + }}, + } +} + +func TestResyncScope_MatchesRespectsTypeAndNamespace(t *testing.T) { + cmInTeamA := types.ResourceIdentifier{ + Group: "", Version: "v1", Resource: "configmaps", Namespace: "team-a", Name: "cfg", + } + cmInTeamB := types.ResourceIdentifier{ + Group: "", Version: "v1", Resource: "configmaps", Namespace: "team-b", Name: "cfg", + } + secretInTeamA := types.ResourceIdentifier{ + Group: "", Version: "v1", Resource: "secrets", Namespace: "team-a", Name: "sec", + } + + cases := []struct { + name string + scope *ResyncScope + id types.ResourceIdentifier + matches bool + }{ + { + name: "a nil scope is the whole-GitTarget resync and matches everything", + // Guards the fallback: BuildPlan, not BuildScopedPlan, must stay reachable. + scope: nil, id: cmInTeamA, matches: true, + }, + { + name: "an empty namespace is an all-namespaces scope for the type", + scope: &ResyncScope{GVR: configmapsGVRForScope}, id: cmInTeamB, matches: true, + }, + { + name: "a named namespace matches its own namespace", + scope: &ResyncScope{GVR: configmapsGVRForScope, Namespace: "team-a"}, id: cmInTeamA, matches: true, + }, + { + name: "a named namespace does NOT match a sibling namespace", + // This single row is the defect: before the namespace half existed, this + // returned true and the sibling namespace's document was swept. + scope: &ResyncScope{GVR: configmapsGVRForScope, Namespace: "team-a"}, id: cmInTeamB, matches: false, + }, + { + name: "a sibling type never matches, even in the scoped namespace", + scope: &ResyncScope{GVR: configmapsGVRForScope, Namespace: "team-a"}, id: secretInTeamA, matches: false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.matches, tc.scope.Matches(tc.id)) + }) + } +} + +func TestResyncScope_StringIsNilSafeAndNamesTheNamespace(t *testing.T) { + var nilScope *ResyncScope + assert.Empty(t, nilScope.String(), "a whole-GitTarget resync has no scope string") + + allNamespaces := &ResyncScope{GVR: configmapsGVRForScope} + assert.Equal(t, configmapsGVRForScope.String(), allNamespaces.String()) + + scoped := &ResyncScope{GVR: configmapsGVRForScope, Namespace: "team-a"} + assert.Contains(t, scoped.String(), "team-a", + "the namespace must appear in the scope string: it keys deferred heals, so two "+ + "namespaces of one type sharing a key would silently drop one heal") + assert.NotEqual(t, allNamespaces.String(), scoped.String()) +} + +// Deferred heals are keyed by (GitTarget, scope). Once one GitTarget can watch a type in +// several namespaces, the key must separate them — otherwise stashing team-b's heal +// replaces team-a's parked one and team-a's drift is never corrected. +func TestResyncHealKey_SeparatesNamespacesOfTheSameType(t *testing.T) { + req := func(ns string) *ResyncRequest { + return &ResyncRequest{ + GitTargetName: "team-a-config", + GitTargetNamespace: "default", + Scope: &ResyncScope{GVR: configmapsGVRForScope, Namespace: ns}, + } + } + assert.NotEqual(t, resyncHealKey(req("team-a")), resyncHealKey(req("team-b")), + "two namespaces of one type must key separately") + assert.Equal(t, healKey{ + name: "team-a-config", namespace: "default", + scope: (&ResyncScope{GVR: configmapsGVRForScope, Namespace: "team-a"}).String(), + }, resyncHealKey(req("team-a")), + "the key is stable for the same scope, so a re-stashed heal replaces rather than duplicates") +} + +// THE test for this change. A replay covers one namespace, so its desired set names only +// that namespace's objects. The sweep must therefore be confined to that namespace: a +// GVR-only sweep would find team-b's document absent from desired and delete it, silently +// removing a namespace's manifests from the tenant's repository. +func TestResync_NamespaceScopedSweepLeavesSiblingNamespacesAlone(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + teamAFull := seedPlacedManifest(t, worktree, "team-a/cm.yaml", cmManifestIn("cfg", "team-a", "blue")) + teamBFull := seedPlacedManifest(t, worktree, "team-b/cm.yaml", cmManifestIn("cfg", "team-b", "green")) + + // team-a replays and finds nothing: its namespace is empty at the pinned revision. + w := &BranchWorker{contentWriter: writer, mapper: configMapMapper()} + scope := &ResyncScope{GVR: configmapsGVRForScope, Namespace: "team-a"} + stats, changed, err := w.applyResyncToWorktree(context.Background(), worktree, "", "", nil, scope, nil) + require.NoError(t, err) + require.True(t, changed, "team-a's orphaned document is swept") + assert.Equal(t, 1, stats.Deleted, "exactly team-a's document is swept, not team-b's") + + _, teamAErr := os.Stat(teamAFull) + assert.True(t, os.IsNotExist(teamAErr), "the scoped namespace's orphan is deleted") + _, teamBErr := os.Stat(teamBFull) + require.NoError(t, teamBErr, + "a sibling namespace's document must survive a namespace-scoped replay of the same type") +} + +// The narrowing must not disable the sweep inside its own namespace: an orphan in the +// scoped namespace is still dropped. Without this, "fixing" the defect by never sweeping +// would pass the test above. +func TestResync_NamespaceScopedSweepStillDropsOrphansInItsOwnNamespace(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + keptFull := seedPlacedManifest(t, worktree, "team-a/kept.yaml", cmManifestIn("kept", "team-a", "blue")) + goneFull := seedPlacedManifest(t, worktree, "team-a/gone.yaml", cmManifestIn("gone", "team-a", "green")) + + w := &BranchWorker{contentWriter: writer, mapper: configMapMapper()} + scope := &ResyncScope{GVR: configmapsGVRForScope, Namespace: "team-a"} + stats, changed, err := w.applyResyncToWorktree( + context.Background(), worktree, "", "", []manifestanalyzer.DesiredResource{ + desiredCMIn("kept", "team-a", "blue"), + }, scope, nil) + require.NoError(t, err) + require.True(t, changed) + assert.Equal(t, 1, stats.Deleted, "the orphan inside the scoped namespace is still swept") + + _, keptErr := os.Stat(keptFull) + require.NoError(t, keptErr, "a desired document in the scoped namespace is retained") + _, goneErr := os.Stat(goneFull) + assert.True(t, os.IsNotExist(goneErr), "an orphan in the scoped namespace is dropped") +} + +// A genuinely cluster-wide stream (a ClusterWatchRule following a type across all +// namespaces) gathers every namespace, so its scope names none and its sweep must still +// cover the whole type. This is the behaviour the namespace half must NOT change. +func TestResync_ClusterWideScopeStillSweepsEveryNamespace(t *testing.T) { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + worktree := newWorktreeForTest(t) + teamAFull := seedPlacedManifest(t, worktree, "team-a/cm.yaml", cmManifestIn("cfg", "team-a", "blue")) + teamBFull := seedPlacedManifest(t, worktree, "team-b/cm.yaml", cmManifestIn("cfg", "team-b", "green")) + + w := &BranchWorker{contentWriter: writer, mapper: configMapMapper()} + scope := &ResyncScope{GVR: configmapsGVRForScope} // no namespace: all-namespaces + stats, changed, err := w.applyResyncToWorktree(context.Background(), worktree, "", "", nil, scope, nil) + require.NoError(t, err) + require.True(t, changed) + assert.Equal(t, 2, stats.Deleted, "an all-namespaces scope sweeps the type in every namespace") + + _, teamAErr := os.Stat(teamAFull) + assert.True(t, os.IsNotExist(teamAErr)) + _, teamBErr := os.Stat(teamBFull) + assert.True(t, os.IsNotExist(teamBErr)) +} diff --git a/internal/git/types.go b/internal/git/types.go index 4a23676f..277e665c 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -182,11 +182,12 @@ type PendingWrite struct { // PendingWriteResync. The worker folds it over the worktree's content-derived // store to produce the resync plan (upserts + mark-and-sweep drops). Desired []manifestanalyzer.DesiredResource - // ScopeGVR, when set, restricts the resync's mark-and-sweep to one type's - // (group, resource): the M12 per-type reconcile/sweep. Desired then carries only - // that type's objects (empty for a pure sweep), and no sibling type's document is - // ever dropped. Nil is the whole-GitTarget resync. - ScopeGVR *schema.GroupVersionResource + // Scope, when set, restricts the resync's mark-and-sweep to one type's + // (group, resource) and optionally to one namespace: the M12 per-type + // reconcile/sweep. Desired then carries only that scope's objects (empty for a pure + // sweep), and no sibling type's — nor, for a namespace-scoped resync, any sibling + // namespace's — document is ever dropped. Nil is the whole-GitTarget resync. + Scope *ResyncScope // Revision is the cluster snapshot resourceVersion the desired set is pinned to // (the joined streaming-watch bookmark). Carried for diagnostics and logging. Revision string @@ -233,6 +234,50 @@ type WorkItem struct { Resync *ResyncRequest } +// ResyncScope restricts a resync's mark-and-sweep to the slice of the mirror the desired +// snapshot was actually gathered over. GVR names the type; Namespace, when non-empty, +// further restricts the sweep to that one namespace. +// +// The invariant this type exists to hold: THE SWEEP SCOPE MUST BE EXACTLY THE SCOPE THE +// DESIRED SET WAS GATHERED OVER. A desired set narrower than its sweep scope deletes +// managed documents that were never in scope; a desired set wider than its sweep scope +// silently leaves documents unmanaged. Namespace lives here, next to GVR, precisely so a +// per-namespace replay cannot reach the sweep carrying only its type — the defect fixed in +// docs/design/watchrule-source-namespace/pr1-namespace-scoped-resync.md, where a replay of +// one namespace swept every other namespace's documents of the same type. +// +// An empty Namespace is a genuinely cluster-wide (all-namespaces) scope for the type, which +// is what a ClusterWatchRule's cluster-wide stream gathers. +type ResyncScope struct { + GVR schema.GroupVersionResource + Namespace string +} + +// String renders the scope for logs and for the deferred-heal key. It is nil-safe: a nil +// scope is the whole-GitTarget resync and renders empty. +func (s *ResyncScope) String() string { + if s == nil { + return "" + } + if s.Namespace == "" { + return s.GVR.String() + } + return s.GVR.String() + " in " + s.Namespace +} + +// Matches reports whether a resolved resource identity falls inside this scope. A nil scope +// matches everything (whole-GitTarget resync). An empty Namespace matches every namespace +// for the type. +func (s *ResyncScope) Matches(ri types.ResourceIdentifier) bool { + if s == nil { + return true + } + if ri.Group != s.GVR.Group || ri.Resource != s.GVR.Resource { + return false + } + return s.Namespace == "" || ri.Namespace == s.Namespace +} + // ResyncRequest is a synchronous resync of one GitTarget against a complete, // revision-pinned desired snapshot (M8). It rides the worker queue so the single // git-mutating goroutine applies it in order with live events, and replies on @@ -244,10 +289,12 @@ type ResyncRequest struct { Revision string GitTargetName string GitTargetNamespace string - // ScopeGVR, when set, makes this a per-type (M12) reconcile/sweep: the mark-and-sweep - // is restricted to the named type's (group, resource) and Desired carries only that - // type's objects (empty = pure sweep of a removed type). Nil is a whole-GitTarget resync. - ScopeGVR *schema.GroupVersionResource + // Scope, when set, makes this a per-type (M12) reconcile/sweep: the mark-and-sweep is + // restricted to the named type — and, when the scope names a namespace, to that + // namespace — while Desired carries only that scope's objects (empty = pure sweep of a + // removed type). Nil is a whole-GitTarget resync. See ResyncScope for the invariant + // binding this to Desired. + Scope *ResyncScope // Heal marks a non-urgent drift-correcting resync (a periodic checkpoint re-anchor or a // removed-type sweep) that the worker DEFERS while a commit window is open, instead of // force-finalizing it. Because one worker serves N GitTargets and the commit window is a @@ -431,6 +478,9 @@ type ReconcileCommitMessageData struct { Resource string APIVersion string Revision string + // Namespace is the single source namespace a namespace-scoped reconcile covered, and + // is empty for a whole-target or all-namespaces reconcile. + Namespace string } // ResourceRef is the lightweight resource identifier emitted to grouped commit diff --git a/internal/watch/event_router.go b/internal/watch/event_router.go index 8a89d6e2..f48829bb 100644 --- a/internal/watch/event_router.go +++ b/internal/watch/event_router.go @@ -12,7 +12,6 @@ import ( "github.com/go-logr/logr" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" - "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" @@ -173,16 +172,17 @@ func (r *EventRouter) resolveWorkerForGitDest( return worker, nil } -// enqueueScopedResync resolves the GitTarget's worker and enqueues a type-scoped resync, -// returning the buffered reply channel and whether the resync actually entered the FIFO. The -// ScopeGVR restricts the worker's mark-and-sweep to the one type, so desired must carry only that -// type's objects (empty for a sweep). heal marks a drift-correcting resync the worker defers while -// a commit window is open (see EmitType*ForGitDest). enqueued is false when the worker's queue was -// full and dropped the request (its failure is still delivered on resultCh for the drain to record). +// enqueueScopedResync resolves the GitTarget's worker and enqueues a scoped resync, returning +// the buffered reply channel and whether the resync actually entered the FIFO. The scope +// restricts the worker's mark-and-sweep to one type, and to one namespace when it names one, so +// desired MUST carry exactly that scope's objects (empty for a sweep) — passing a scope wider +// than the gather deletes managed documents outside it. heal marks a drift-correcting resync the +// worker defers while a commit window is open. enqueued is false when the worker's queue was full +// and dropped the request (its failure is still delivered on resultCh for the drain to record). func (r *EventRouter) enqueueScopedResync( ctx context.Context, gitDest types.ResourceReference, - gvr schema.GroupVersionResource, + scope git.ResyncScope, desired []manifestanalyzer.DesiredResource, revision string, heal bool, @@ -191,20 +191,27 @@ func (r *EventRouter) enqueueScopedResync( if err != nil { return nil, false, err } - scope := gvr resultCh := make(chan git.ResyncResult, 1) enqueued := worker.EnqueueResync(&git.ResyncRequest{ Desired: desired, Revision: revision, GitTargetName: gitDest.Name, GitTargetNamespace: gitDest.Namespace, - ScopeGVR: &scope, + Scope: &scope, Heal: heal, Result: resultCh, }) return resultCh, enqueued, nil } +// resyncScopeForWatchKey is the single conversion from a watch key to the resync scope its +// replay must be swept under. It exists so the two halves of the invariant — the namespace a +// stream gathered, and the namespace its sweep may touch — are derived from ONE value and +// cannot drift apart at a call site. +func resyncScopeForWatchKey(key targetWatchKey) git.ResyncScope { + return git.ResyncScope{GVR: key.GVR, Namespace: key.Namespace} +} + // drainScopedResync logs a per-type reconcile/sweep's outcome and, on failure or timeout, // counts it as a background resync failure so a silently-recovered fault stays observable. The // steady-state live-event path and the next type transition recover a failed apply, so this diff --git a/internal/watch/event_router_test.go b/internal/watch/event_router_test.go index 65ab260e..0aaaec78 100644 --- a/internal/watch/event_router_test.go +++ b/internal/watch/event_router_test.go @@ -98,7 +98,7 @@ func TestEnqueueScopedResync_ReportsMissingWorker(t *testing.T) { resultCh, enqueued, err := router.enqueueScopedResync( context.Background(), types.NewResourceReference("team-a-config", "team-a"), - configmapsGVR, + git.ResyncScope{GVR: configmapsGVR}, nil, "12", false, diff --git a/internal/watch/resync_scope_test.go b/internal/watch/resync_scope_test.go new file mode 100644 index 00000000..d71e9e33 --- /dev/null +++ b/internal/watch/resync_scope_test.go @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/ConfigButler/gitops-reverser/internal/git" +) + +// The scope/desired agreement invariant, asserted at the one place the two are bound: a +// replay gathers exactly the namespace named by its watch key, so the resync scope it is +// swept under must carry that same namespace. Dropping it here is what let a replay of one +// namespace sweep every other namespace's documents of the same type — the information was +// present at both ends and discarded in between. +func TestResyncScopeForWatchKey_CarriesBothHalvesOfTheScope(t *testing.T) { + gvr := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "configmaps"} + + t.Run("a named-namespace stream is swept only in its own namespace", func(t *testing.T) { + scope := resyncScopeForWatchKey(targetWatchKey{GVR: gvr, Namespace: "team-a"}) + assert.Equal(t, git.ResyncScope{GVR: gvr, Namespace: "team-a"}, scope) + }) + + t.Run("a cluster-wide stream keeps the all-namespaces scope", func(t *testing.T) { + // A ClusterWatchRule's stream gathers every namespace, so its sweep must too. + scope := resyncScopeForWatchKey(targetWatchKey{GVR: gvr}) + assert.Equal(t, git.ResyncScope{GVR: gvr}, scope) + assert.Empty(t, scope.Namespace) + }) + + t.Run("two namespaces of one type produce distinct scopes", func(t *testing.T) { + a := resyncScopeForWatchKey(targetWatchKey{GVR: gvr, Namespace: "team-a"}) + b := resyncScopeForWatchKey(targetWatchKey{GVR: gvr, Namespace: "team-b"}) + assert.NotEqual(t, a, b, + "the fan-out this change exists to make safe: one GitTarget watching a type in "+ + "two namespaces must not collapse to one sweep scope") + }) +} diff --git a/internal/watch/target_watch.go b/internal/watch/target_watch.go index 01183d49..968c8fd4 100644 --- a/internal/watch/target_watch.go +++ b/internal/watch/target_watch.go @@ -585,7 +585,7 @@ func (m *Manager) enqueueReplayResync( } epoch := m.RenderFidelityEpochForGitTarget(gitDest) resultCh, enqueued, err := m.EventRouter.enqueueScopedResync( - ctx, gitDest, key.GVR, desired, revision, false) + ctx, gitDest, resyncScopeForWatchKey(key), desired, revision, false) if err != nil { return err } diff --git a/test/e2e/setup/kcp/base/kcp-operator.yaml b/test/e2e/setup/kcp/base/kcp-operator.yaml index 556e63cc..966e35e4 100644 --- a/test/e2e/setup/kcp/base/kcp-operator.yaml +++ b/test/e2e/setup/kcp/base/kcp-operator.yaml @@ -34,9 +34,10 @@ spec: spec: # Pinned for reproducibility (the gitops-api reference leaves it floating). kcp-operator # 0.7.7 deploys kcp v0.8.x, which serves tenancy.kcp.io/v1alpha1 Workspaces — all this - # harness needs. + # harness needs. This is the CHART version; v0.8.3 is the appVersion it ships, and the + # two are not interchangeable — the repo publishes no kcp-operator chart above 0.7.x. chart: kcp-operator - version: "0.8.3" + version: "0.7.7" sourceRef: kind: HelmRepository name: kcp