diff --git a/docs/INDEX.md b/docs/INDEX.md index b675b0e6..214211aa 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -117,6 +117,8 @@ Eighteen other open items: | [`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 | | [`azure-devops-multi-ack.md`](design/azure-devops-multi-ack.md) | **decided and built: go-git v6** — why Azure DevOps rejects our fetches, and what to do instead of PR [#292](https://github.com/ConfigButler/gitops-reverser/pull/292)'s bundled `git` binary. The capability filter fails in two independent halves: advertising `multi_ack` is a four-line change, but v5 then cannot parse the multi-ACK **response**, which only a fetch with `have` lines provokes. That is why **Flux ships ADO support on v5 with no git binary — it never fetches**, only `CloneContext`, so it never enters the path v5 cannot serve; our persistent-clone-plus-incremental-fetch design is the opposite, which makes the trim alone insufficient for us. **go-git v6 already implements `multi_ack`** (PR #1204, in every v6 tag; upstream then deleted their ADO example saying it "works out of the box"), and its churn in the packages we import runs 96 → 39 → **1** → **9** removals per alpha, so it is one settled breaking wave rather than a moving target; the migration is four known API removals over two rewritten files, `transport.AuthMethod` being the invasive one. Prices PR #292 as measured rather than argued: the image goes **217 MB → 940 MB**, of which 723 MB is a `cp -rL` that dereferences 165 hardlinks to one binary (a one-character fix), arm64 is unaffected and native, but **Trivy reports zero findings on both images** while the new one carries git 2.54.0, OpenSSH 10.3p1 and OpenSSL 3.5.7 as loose files no package database describes — so the CRITICAL gate is blind to a third of the runtime. Also catches an unflagged non-ADO regression (`Depth: 1` dropped, so every provider full-fetches) and 10% patch coverage on an untestable path. The unlock is that **canonical `git upload-pack` advertises `multi_ack`** (verified), so the Gitea already in the e2e lab plus a 400-injecting proxy is a faithful ADO simulator — no tenant needed, and the only way any option becomes CI-testable. Four options priced, and Option A (v6) is the one shipped. Carries a measured **capability matrix** over our three network calls with two diagrams, which narrows the blast radius to **one call, `repo.Fetch`**: `receive-pack` never advertises `multi_ack` (measured), so **the atomic push is out of scope for every option** — its safety rests on the same-session advertisement plus the server-side `Old`/`New` compare-and-swap in `packp.Command`, neither of which touches `upload-pack`, and we already push from a shallow store today. v6 keeps that pattern 1:1 (`Handshake` → `GetRemoteRefs`/`Push`, same `[]*packp.Command`), which is an argument *for* migrating. Records what the migration actually cost, including the four v6 behaviour changes it surfaced — two of them settings v6 reads from the environment and fails closed on, invisible to unit tests | +| [`crd-handling.md`](design/crd-handling.md) | **design, unbuilt.** Where a mirrored folder's custom-resource **definitions** come from, since a `Widget` cannot exist in a cluster that never heard of its CRD. Recommends default **`None`** (the folder holds instances, the installer owns definitions), opt-in **`Referenced`** (a manifest of names, versions, sources and digests, never schemas), `Vendored` as an escape hatch. Selection is nearly free: `typeset` already classifies each type's origin with the CRD as evidence. Two verified facts carry it — Helm's `meta.helm.sh/*` ownership metadata survives `sanitize` (so a vendored CRD arrives claiming another cluster's release), and Flux's labels do not (so provenance must be captured live). Worked both ways in [`layout/examples/crd-closure/`](layout/examples/crd-closure/README.md) | +| [`source-scope-simplification.md`](design/source-scope-simplification.md) | **proposal, unbuilt.** Declines Flux-style impersonation, deletes `GitTarget.spec.allowedSourceNamespaces` and its selector machinery (**4,569 lines**, and the only cross-cluster read in the authorization path), renames two `ClusterProvider` fields, and redefines `sourceNamespace: "*"` as one cluster-wide list and watch. The argument is an API reading, not a security one: the chain from a Git folder back to the object that fills it never leaves one namespace, so ordinary RBAC on `watchrules` already answers it. **Keeps `allowedNamespaces`** (renamed `accessFrom`), reversing an earlier draft — source RBAC bounds what a credential may READ, never which tenant may WIELD it. Prices what is lost: source-side label selectors. Archaeology in [`facts/kubernetes-impersonation-and-flux-identity.md`](facts/kubernetes-impersonation-and-flux-identity.md) | ## The layout topic — [`layout/`](layout/README.md) @@ -131,7 +133,7 @@ by path from Go source. | [`new-file-placement-rules.md`](layout/new-file-placement-rules.md) | **spec** | where a new resource's file goes: declared, the folder's one kustomize root, canonical. Sibling inference is removed, and kept as history | | [`placement-visibility-and-declared-defaults.md`](layout/placement-visibility-and-declared-defaults.md) | **design** | the three questions the inference deletion left, **decided and then not built**: PR #291 shipped the deletion and none of the eight items queued behind it. The residue was filed as [#295](https://github.com/ConfigButler/gitops-reverser/issues/295) — **which shipped in 0.42.1 via [#319](https://github.com/ConfigButler/gitops-reverser/pull/319) and is what reversed the layout model** — and [#296](https://github.com/ConfigButler/gitops-reverser/issues/296). Its Question 2 is superseded outright by [`model.md`](layout/model.md). What still stands: keep `canonical` as the name for the built-in path and split `declared` into `byType`/`default`; **no CRD default for `placement.default`**, on the structural argument that a defaulted default is never empty and so shadows the kustomize-root rung; `status.layout` instead, over the `MarkTargetRetention` seam that already enqueues on change; and `{kindLower}`, not a `toLower` function | | [`model.md`](layout/model.md) | **design** | **reversed 2026-08-28, and much smaller than it was.** The earlier thesis wanted `spec.placement` replaced by a `spec.layout` discriminated union; three of its five arguments were retired by [#319](https://github.com/ConfigButler/gitops-reverser/pull/319), which made registration an invariant. So the template **stays** and gains two additive members of `spec.placement`: **`serializeNamespace`** (`Auto`, `Always`, `Never`) and **`kustomizeRoot`** (`Adopt`, `Create`, `Require`). Carries four kustomize facts **measured** against v5.8.1, three of which contradict the earlier model. The headline is what it deletes — `spec.layout`, `kind`, `scope`, `kustomize.create`, the `LayoutProfile` question, and the migration — so the largest breaking change in the queue stops being breaking at all | -| [`api-wave.md`](layout/api-wave.md) | **design** | filed as [#294](https://github.com/ConfigButler/gitops-reverser/issues/294). What is left of one breaking wave on `GitTarget` after the model reversed: B4's `commitWindow`/`commit.message` move off the connection, plus the riders. Organizing principle: **the folder is described on the GitTarget, the connection describes only the connection** — and this is where that becomes a struct boundary rather than a sentence, since grouping a field is free only in a release that is already breaking. `spec.mode` and `GitTarget.spec.interval` are both **dropped**, with re-open triggers. Records that F9's envtest stays OUTSIDE the wave and gates it, and that staying `v1alpha3` on loud rejections is a **one-consumer countdown**, not a constant | +| [`api-wave.md`](layout/api-wave.md) | **design** | filed as [#294](https://github.com/ConfigButler/gitops-reverser/issues/294). What is left of one breaking wave on `GitTarget` after the model reversed: B4's `commitWindow`/`commit.message` move off the connection, the source-scope deletion (the only member that makes the API smaller), and the riders. Organizing principle: **the folder is described on the GitTarget, the connection describes only the connection** — and this is where that becomes a struct boundary rather than a sentence, since grouping a field is free only in a release that is already breaking. `spec.mode` and `GitTarget.spec.interval` are both **dropped**, with re-open triggers. Records that F9's envtest stays OUTSIDE the wave and gates it, and that staying `v1alpha3` on loud rejections is a **one-consumer countdown**, not a constant | [`examples/`](layout/examples/README.md) makes the proposed `GitTarget` layout model tangible. Each scenario contains a repository folder and the matching proposed diff --git a/docs/design/crd-handling.md b/docs/design/crd-handling.md new file mode 100644 index 00000000..2c4d0aff --- /dev/null +++ b/docs/design/crd-handling.md @@ -0,0 +1,221 @@ +# CRD handling: what a folder needs to be applied, and where the definitions come from + +> Status: design, unbuilt. Nothing here binds until it is scheduled. +> Date: 2026-08-29. Index: [`../INDEX.md`](../INDEX.md). +> +> Worked example: [`../layout/examples/crd-closure/`](../layout/examples/crd-closure/README.md), +> which shows the same live objects under both candidate shapes so the choice can be read rather +> than argued. + +A mirrored folder of custom resources is not applicable on its own. A `Widget` cannot exist in a +cluster that has never heard of `widgets.apps.example.com`, so somewhere between the folder and the +cluster the definition has to arrive. This page decides where from, and deliberately does **not** +decide it by mirroring every CRD in the source cluster, which is the only thing a `WatchRule` can +express today. + +## Two consumers, and they want different things + +**1. Applicability.** Can this folder be applied to a fresh cluster? Answering yes by inspection +means the definitions are in the repository. + +**2. The per-branch editing cluster.** The direction this project is heading is that a branch under +edit gets its own small, workload-less Kubernetes cluster, and the branch's documents are hydrated +into it as real API objects so they can be edited through the API rather than as text. That cluster +has to be able to **hold** the objects, which means every custom type in the folder must be +installed before hydration. Here the definition does not have to be in the repository at all. It has +to be *installable at spin-up*. + +The second consumer is the demanding one, and it is better served by a reference than by a copy: a +reference can be resolved to the version the type's owner publishes, while a copy freezes whatever +happened to be installed in the cluster we were mirroring. + +## What we already know, at no cost + +`typeset` classifies every served type's origin as `builtin`, `crd` or `aggregated`, with the CRD +itself as evidence and a confidence that says whether an object was observed +([`model.go`](../../internal/typeset/model.go), [`type-followability.md`](../spec/type-followability.md)). + +So "which CRDs back the types this GitTarget writes" is a lookup over a join the operator already +performs. Whatever this page decides, the **selection** is close to free. The cost is in what we do +with the answer. + +This also settles one thing early: the selection cannot be expressed by a per-rule `objectSelector` +([#146](https://github.com/ConfigButler/gitops-reverser/issues/146)). The relevant set is *derived* +from the rules and changes when they do; it is not a label match anyone can write down. + +## Four options + +| | What lands in Git | Who owns the CRD | Where the branch cluster gets it | +|---|---|---|---| +| **A. Vendor** | the CRD objects | now two: the installer and this folder | the folder | +| **B. Reference** | a small manifest of names, versions, sources and digests | its installer, unchanged | resolving the manifest | +| **C. Nothing** | nothing | its installer | copied from the source cluster at spin-up | +| **D. Synthesize** | a generated minimal schema | nobody real | the generated schema | + +**A costs more than it looks.** A CRD installed by Helm carries `meta.helm.sh/release-name`, +`meta.helm.sh/release-namespace` and `app.kubernetes.io/managed-by: Helm`, and **none of those are +stripped on the way to Git**: [`sanitize`](../../internal/sanitize/types.go) removes Flux, kro and +applyset bookkeeping plus two exact Argo keys, and Helm's ownership metadata is not in either list. +So a vendored CRD arrives in the repository still claiming to belong to a release in another +cluster, and applying it somewhere else walks into Helm's own ownership check. Add the size (our +smallest CRD is 25 KB; Argo CD's `Application` CRD is about a megabyte) and the churn (every +operator upgrade rewrites the file), and a configuration repository becomes mostly vendored schema +diffs. + +**B is the one that matches who owns what.** For application configuration in particular, the app +team owns what its CRD looks like, and the CRD belongs in the app's own repository next to the +controller that serves it. A configuration folder that vendors it forks it. + +**C is right for the case it covers and silent about the rest.** Copying from the source cluster at +spin-up needs nothing committed and always matches production exactly. It fails precisely when the +editing cluster is most useful: a branch from a contributor without source-cluster access, an +environment that no longer exists, an edit made while the cluster is unreachable. + +**D is rejected.** A synthesized schema accepts objects the real API server would reject, and the +entire value of hydrating a branch into a real cluster is that it validates like the real one. + +## Recommendation + +- **Default `None`.** The folder holds instances. The installer owns the definitions. This is the + posture that does not create a second owner, and it is what a mirror of an existing cluster wants. +- **Opt-in `Referenced`**: commit a small manifest of references and digests, never schemas. +- **The branch cluster resolves in order**: the reference manifest first, the source cluster second + (which is option C kept as a fallback rather than a design), and otherwise **fail loudly, naming + the types it could not install**. Hydrating a branch into a cluster that silently lacks a type + produces an editing session that looks fine and drops objects. +- **`Vendored` stays available** for a type whose definition has no installable source. It is an + escape hatch with the ownership cost stated at the point of use, not a default. + +## Where a reference comes from, and the trap in reading it + +Provenance has to be read from the **live object**, in the operator, and can never be recovered from +the repository. `sanitize` strips `kustomize.toolkit.fluxcd.io/*` from labels and annotations, which +is exactly the evidence that would say a CRD came from a Flux `Kustomization`. Capture it at +observation time or lose it. + +The ladder, in the order it should be tried: + +1. **Helm**: `meta.helm.sh/release-name` and `release-namespace`, with + `app.kubernetes.io/managed-by: Helm`. Naming the chart and version means reading the release + object, which is a Secret in the source cluster. That is a new read, and an authorization + question, not a free one. +2. **Flux**: `kustomize.toolkit.fluxcd.io/name` and `namespace` on the live CRD, resolved through + the `Kustomization` to its source. Live-only, per the trap above. +3. **Argo CD**: no tracking annotation is stamped on CRDs, since its repo-server stamps every + *non-CRD* object it applies. Provenance for an Argo-installed CRD comes from the `Application` + that lists it, or not at all. +4. **Unattributed**: record the CRD name, its served versions and a digest, and say plainly that the + source is unknown. A reference that admits it cannot be resolved is more useful than a guess. + +## The finding that matters most for the branch cluster + +**A workload-less cluster runs no webhook of its own.** A CRD with +`spec.conversion.strategy: Webhook` needs something to convert between stored versions, and a type +whose real cluster mutates objects through a mutating admission webhook will hold *unmutated* +objects in the branch cluster. Installing the CRD is necessary and not always sufficient, and the +gap is invisible: the object is accepted, it is simply not what production would have stored. + +Three things narrow that, in increasing order of ambition. Take them in this order. + +**1. Most defaulting is not a webhook at all.** Structural-schema defaults are applied by the API +server itself +([`pkg/apiserver/schema/defaulting`](https://github.com/kubernetes/apiextensions-apiserver)), so a +CRD's `default:` values work in a cluster with nothing running. Only *mutating admission* is +missing, which is a much smaller set than "defaulting". + +**2. Conversion only fires when versions differ.** If the branch cluster installs the CRD with the +versions the folder actually uses, and serves what it stores, no conversion runs. Narrowing the +installed version list is a subset of the real CRD rather than a synthesized schema, so it does not +fall into option D, and it is the cheapest honest answer for the common case of a folder written at +one version. + +**3. The webhook can live outside the cluster, and this is the interesting one.** +`WebhookClientConfig` takes **either** `service` **or** `url` +([apiextensions v1 types](https://github.com/kubernetes/apiextensions-apiserver), verified at +v0.36.4), and the upstream field documentation is explicit that `url` is for a webhook that is *not* +running in the cluster: the API server cannot resolve in-cluster DNS, so `service` is the in-cluster +form and `url` is the external one. The same shape exists on admission webhook configurations. + +So a branch cluster can install the CRD pointed at a conversion endpoint that runs somewhere else, +and the type converts correctly with nothing deployed. The constraints, all from the field's own +contract: + +- **`https` only.** No query string, no fragment, no `user:password@`. A path *is* allowed and may + carry an arbitrary string, which upstream itself suggests using as a cluster identifier. +- **`caBundle` is optional**: omitted, the API server uses its system trust roots, so a publicly + trusted certificate needs no bundle at all. A private CA needs one, and needs it rotated. +- **The API server sends no bearer token.** Client credentials for webhooks come from API + server-level configuration, not from anything a per-CRD `url` can carry. An external endpoint + therefore authenticates its caller by mTLS or by the path identifier above, and must assume the + path is the only secret it has. +- **Reachability runs the other way from usual.** The branch cluster's control plane makes the + outbound call, so the endpoint has to be reachable from wherever we run those clusters, and every + object of that type is sent to it. That is an egress policy question and a data exposure question + before it is a design question. +- **It is in the edit path.** Latency and availability of that endpoint become latency and + availability of editing a branch. + +### Hosted transformations + +The natural extension: rather than every team exposing an endpoint, ConfigButler hosts one, with +common transformations composed by the user (clicked together, or scripted, or generated) and +addressed per branch through the URL path. + +It is a real product direction and the mechanism is already proven, since it is the same webhook +contract with us as the operator. Two things have to be true before it is safe, and they are worth +writing down now: + +- **A transformation is code, and it must be pinned.** A branch cluster whose behavior depends on + the current state of a hosted rule set is not reproducible: the same branch hydrates differently + next week. The reference manifest is the place to pin the transformation version, next to the + CRD digest it belongs to. +- **Divergence from the app's real webhook must be loud.** If our transformation and the + application's own conversion disagree, the branch cluster stores objects production would not, and + we have recreated the invisible gap this section opened with, except now we caused it. A type + served by a hosted transformation rather than by the app's own endpoint should be marked + non-authoritative in the session, and the marking should survive into whatever the branch produces. + +The honest ladder is therefore: **narrow the versions** so nothing converts; else **point at the +application's own endpoint**, which is where that code already lives and where the app team already +owns it; and only then **host a transformation**, for types whose owner has no endpoint to point at. + +## Placement consequences + +- **CRDs are cluster-scoped.** Canonically that is + `_cluster/apiextensions.k8s.io/customresourcedefinitions/{name}.yaml`; in a kustomize folder it is + a flat file beside the root, registered in `resources:` like any other placement. +- **Apply order.** A folder holding both a CRD and its custom resources has to apply the definition + first. Flux and Argo CD both handle this; `kubectl apply -k` over a plain folder is where it bites. + If we vendor, the folder has to say who guarantees the ordering. +- **Scope.** A namespaced `GitTarget` writing cluster-scoped documents widens what that tenant puts + in Git. That is an authorization question and it belongs with + [`source-scope-simplification.md`](source-scope-simplification.md), not with placement. + +## The API shape + +`GitTarget.spec.includeTypeDefinitions: None | Referenced | Vendored`, defaulting to `None`. +Additive, so it needs no coordinated bump and does not belong to the breaking wave. The derived set +is recomputed when rules change, over the isolation seam that already exists for exactly that +([`gittarget-isolation-on-rule-change.md`](../spec/gittarget-isolation-on-rule-change.md)). + +## Open questions + +- Does the reference manifest belong **in the folder** as a committed artifact, or only in + `status`? In the folder it is reviewable and travels with the branch; in status it never conflicts + and never goes stale in a PR. +- **A digest of what?** The CRD's `spec`, so a resource-version bump is not a change, or the whole + object, so any drift shows. +- One manifest **per target folder** or one **per repository**? Two targets in one repository will + reference overlapping types. +- Does the branch cluster install **only CRDs**, or the controllers too? With `url` webhooks the + question narrows: what has to run is an *endpoint*, not a controller, and it does not have to run + in the branch cluster at all. +- Does the reference manifest record a type's **conversion strategy** and, where it is `Webhook`, + the endpoint the branch cluster should point at? The manifest is the only place that knows both + the type and its origin, so it is the natural home, and it makes "this folder needs a reachable + endpoint to be editable" a reviewable fact rather than a runtime surprise. +- Who **owns the caBundle** for an app-hosted endpoint, and what happens to a branch when it + rotates? +- Should `Vendored` **strip Helm and Flux ownership metadata** on the way out? It would make the + vendored copy applicable elsewhere, at the cost of `sanitize` acquiring a rule that exists for one + option of one field. diff --git a/docs/design/source-scope-simplification.md b/docs/design/source-scope-simplification.md new file mode 100644 index 00000000..51d40156 --- /dev/null +++ b/docs/design/source-scope-simplification.md @@ -0,0 +1,247 @@ +# Source scope: what to delete, and what to keep + +> Status: design. Nothing here is built, and nothing binds until it is scheduled. +> Date: 2026-08-28. Index: [`../INDEX.md`](../INDEX.md). +> +> Answers a design review's proposal to adopt Flux-style `serviceAccountName` impersonation for +> source reads, by declining it. Evidence for the impersonation half is in +> [`../facts/kubernetes-impersonation-and-flux-identity.md`](../facts/kubernetes-impersonation-and-flux-identity.md), +> including a correction to a claim in that review. + +## Decision + +1. **Do not build impersonation.** It buys several identities behind one credential against one + source cluster. Nobody has asked for that, and the cost is four pieces of machinery and an + `impersonate` grant. +2. **Delete `GitTarget.spec.allowedSourceNamespaces`** and its selector machinery. +3. **Rename `ClusterProvider.spec.allowSourceNamespaceOverride`** to + `allowAnySourceNamespace`, still a boolean, still defaulting to false. +4. **Decide `sourceNamespace: "*"` explicitly** (see below). It is defined in terms of the field + being deleted, so it cannot be left alone. +5. **Keep `ClusterProvider.spec.allowedNamespaces`**, renamed `accessFrom`, until a replacement is + shipped and tested. It is the one field here doing work that nothing else does. + +The name keeps `Source` deliberately. `ClusterProvider` carries two namespace planes, and +`allowAnyNamespace` sitting directly beneath `accessFrom` would read as a modifier on it. +`allowCrossNamespace` was the other candidate, borrowing Flux's `--no-cross-namespace-refs` +vocabulary, and it was not taken: in Flux the phrase means object references across namespaces in +one cluster, while here the far side is a namespace in a different cluster. The field's own design +rationale already records why that matters, and it is the sharpest statement of it in the repo: for +a remote provider "the config-plane namespace and the source namespace are on different clusters, +so their sharing a name never was a boundary". Crossing is literally true only for the in-cluster +provider; `any` is literally true for both. It stays a +boolean because there are two states and no third one is in view: impersonation and source-side +selectors are both out, so an enum would only be leaving room for something nobody can name. The +review's objection to a boolean here was that it sat among "two policy objects answering +neighbouring halves"; deleting `allowedSourceNamespaces` removes the other half, and the objection +with it. + +## Why `allowedNamespaces` stays + +An earlier draft proposed deleting it, on the evidence that the chart renders +`allowedNamespaces: {selector: {}}` and so admits every namespace. That evidence is real but +narrower than it looked: it is the value for the **chart-owned `default` provider only** +([`values.yaml`](../../charts/gitops-reverser/values.yaml)). Every user-authored `ClusterProvider`, +which is every remote cluster, is deny-by-default and carries an explicit consumer list its author +wrote on purpose. + +The boundary it draws is not available anywhere else. Source-cluster RBAC bounds **what a +credential may read**. It cannot express **which control-plane tenant may wield that credential**, +because the tenant is not a subject in the source cluster at all. Deleting the field would make a +shared source credential usable from any namespace that can create a `GitTarget`. + +A `ValidatingAdmissionPolicy` is not a drop-in replacement either, and this repository has already +written down why: [`../spec/where-validation-lives.md`](../spec/where-validation-lives.md) states +that reconcile-time is the **stronger** gate, because admission is one-shot and cannot see a policy +tightened after the object was created, so "an admission-only check is strictly less safe". A VAP +can stop a new or updated `GitTarget` from selecting a provider. It cannot stop a stored one whose +namespace lost its label. Recommending a VAP here contradicted our own doctrine. + +What can be simplified without touching the boundary: the field reads **control-cluster** namespace +labels, locally, with no cross-cluster call and no degradation path, so its selector is cheap in a +way the source-side one is not. Keeping both halves is fine. Renaming to `accessFrom` is still +worth doing, and matters more once the two `allowed*Namespaces` fields no longer sit side by side +to disambiguate each other. + +## Why the rest goes + +`GitTarget.spec.allowedSourceNamespaces` presents itself as a destination policy: "it belongs to +the DESTINATION, not to any requesting rule". It cannot be one. `WatchRule.spec.targetRef` is a +`LocalTargetReference` ("Must be in the same namespace"), `GitTarget.spec.providerRef` is local +too, and `spec.path` is immutable, so the chain from a Git folder back to the object that fills it +never leaves one namespace: + +```text +Git folder <- GitProvider <- GitTarget <- WatchRule + (same namespace, all the way) +``` + +Whoever can create a `WatchRule` there can already write into that folder. What the field really +bounds is which source namespaces the folder's own tenant may **read**, and for a credential-scoped +provider that restates what the credential already carries, in the one place that cannot revoke. +`ClusterWatchRule` bypasses it entirely today and nobody has minded. + +**The complexity is in the matcher, not the fence.** `NamespaceMatcher`'s selector half is +evaluated against `Namespace` labels **in another cluster**. That one choice produces the +three-valued verdict (so a source-cluster outage is not a denial), the `SourceScopeUnavailable` +degradation path, the five condition reasons, and the operator's need for source-cluster +`Namespace` get/list/watch. Delete the source-side selector and all of it goes. + +```mermaid +flowchart LR + subgraph before["Today"] + direction TB + B1["ClusterProvider.allowedNamespaces
control-cluster labels, local read"] + B2["ClusterProvider.allowSourceNamespaceOverride"] + B3["GitTarget.allowedSourceNamespaces
SOURCE-cluster labels, cross-cluster read"] + B4["3-valued verdict, 5 reasons,
SourceScopeUnavailable path"] + B3 --> B4 + end + subgraph after["Proposed"] + direction TB + A1["ClusterProvider.accessFrom
unchanged boundary, renamed"] + A2["ClusterProvider.allowAnySourceNamespace
boolean, default false"] + A3["a string comparison"] + end + B1 ==>|"kept"| A1 + B2 ==>|"renamed"| A2 + B3 -.->|"deleted"| A3 + B4 -.->|"deleted with it"| A3 +``` + +## `sourceNamespace: "*"` needs its own decision + +Today `*` means "every source namespace this `GitTarget` admits", resolved live through +`allowedSourceNamespaces` into a concrete set, which is then planned as one stream per namespace. +[`watchrule_types.go`](../../api/v1alpha3/watchrule_types.go) says so in the constant's own doc: +"never `every namespace that exists`". So `*` is **defined in terms of the field being deleted** +and cannot survive unchanged. + +RBAC cannot supply the missing definition. It answers "may I watch X in namespace Y", never "which +namespaces may I watch". Any set-valued reading of `*` requires listing `Namespace` objects in the +source cluster, which is the cost this whole exercise is trying to remove. Three options: + +| Option | Meaning | Cost | +|---|---|---| +| **Delete `*`** | items name namespaces | safest, and a breaking change for anyone using it | +| **Redefine as cluster-wide** | one watch and one list at `metav1.NamespaceAll`, all or nothing | implementable with no `Namespace` access, and a real widening: it reaches everything the credential can see | +| **Keep enumeration** | as today, against some other policy | keeps the cross-cluster `Namespace` read, which was the point of the deletion | + +**Decided: redefine as cluster-wide**, rejected outright while `allowAnySourceNamespace` is false. +It applies to **both** halves of a cell's traffic (the initial list that warms the cell, and the +watch that follows it), because they are the same collection read two ways, and splitting them +would mean enumerating namespaces for the replay after all, which is the read this exercise +deletes. + +The plumbing already exists, which makes this the cheapest of the three as well as the clearest. +`CellKey.Namespace` is already documented as "empty is a genuinely cluster-wide (all-namespaces) +cell" ([`cell.go`](../../internal/types/cell.go)), and both +[`openTargetWatch`](../../internal/watch/target_watch.go) and `openTargetList` already branch on +it: a non-empty namespace calls `resource.Namespace(ns)`, an empty one calls `resource.Watch` +directly, which for a namespaced GVR is the all-namespaces collection. Readiness, retention +rollup, and event routing all key on `CellKey` already, and records carry the object's own +`u.GetNamespace()` rather than the cell's, so placement is unaffected. What changes is only the +planner: `*` compiles to one cell instead of calling `EnumerateSourceNamespaces` for N. That is a +deletion in `watchrule_compile.go`, not new machinery. + +One trap, and the code already records it. A cluster-wide cell is a **peer** of a named-namespace +cell on the same type, never a replacement, because each rule carries its own `operations` filter. +`CellKey`'s doc comment names the bug from a previous attempt: collapsing the two "widened the named +rule's stream to every namespace its credential could read and discarded its operation filter". So +a target carrying both `*` and a named rule for one type runs two streams over overlapping objects, +and that is correct rather than something to optimize away. +It is what a Kubernetes reader expects `*` to mean, and its failure is a clean 403 rather than a +silent empty set. + +**It is also the largest single efficiency win in this proposal, and that is a reason in its own +right.** A `*` rule over a type in a hundred-namespace cluster is a hundred watch connections and a +hundred list calls at warm-up today, one per namespace, each with its own cursor, its own retry +schedule and its own share of apiserver watch cache. Cluster-wide makes it one of each, and the +saving grows with the cluster, which is exactly the direction the enumeration got worse in. The +`TooManyStreams` cap was queued for the fan-out this deletes; see +[`../layout/api-wave.md`](../layout/api-wave.md), where that rider is now smaller than it was. + +## Consequences, including two breaking semantic changes + +**A declared policy can currently deny a rule's own namespace.** `allowedSourceNamespaces` is +exhaustive once declared, "with no exception for a rule's own namespace" +([`gittarget_types.go`](../../api/v1alpha3/gittarget_types.go)). So `allowAnySourceNamespace: false` +is **not** the current posture exactly. It matches the no-policy path, which is what a default install runs, and it is a +deliberate simplification for anyone who declared a policy that excluded their own namespace. Say +so in the migration note rather than claiming continuity. + +**`*` changes meaning**, per the decision above: it widens from "every namespace this GitTarget +admits" to "every namespace this credential can read", and it stops being available at all unless +`allowAnySourceNamespace` is true. For a user who had no `allowedSourceNamespaces` policy the old +`*` already resolved to whatever the credential could see, so the widening is narrower in practice +than it reads; for a user who had one, it is real. + +**Label selectors over source namespaces are lost.** Admitting every namespace carrying a label, +following namespaces as they appear, has no RBAC equivalent short of a binding per namespace. This +is the real capability cost. An N-way restriction costs N objects wherever it is expressed, and +today's version is cheap only because it enforces nothing. + +**What is deleted**, 4,569 lines in files that exist for nothing else, with coupling into shared +files running 1 to 26 mentions, so this is a deletion rather than a refactor: + +| File | Lines | +|---|---| +| `internal/authz/source_namespace.go` and its test | 1351 | +| `internal/watch/source_namespace_scope.go` and its tests | 1312 | +| `internal/controller/watchrule_source_namespace.go` and its test | 770 | +| `api/v1alpha3/namespace_matcher.go` selector half | part of 371 | +| `test/e2e/source_namespace_e2e_test.go` | 349 | + +`internal/authz/clusterprovider_admission.go` stays, since `accessFrom` stays. +`ClusterProviderNotFound` stays with it, though it is reference resolution rather than +authorization and would read better elsewhere. + +## The one thing to build + +A `SelfSubjectAccessReview` pass under the provider's own credential. It needs no new grant, since +every identity may issue one, and it reports two things a user cannot otherwise get: which of the +requested cells are reachable, and whether write verbs are permitted on them. + +Phrase the condition as **"no write permission observed for the requested resources at review +time"**. It cannot prove the mirror is unable to write: a review covers the verbs and resources +asked about, at that instant, and says nothing about other resources, subresources, or a later +change. It is a diagnostic, and a good one, not a proof. + +This is also what replaces the deleted policy in the place that matters: the user stops declaring +which namespaces are permitted, and starts being told which are reachable. + +## Migration + +One breaking change, in the wave already queued, riding the loud-rejection pattern the project uses +for superseded fields: + +- `GitTarget.spec.allowedSourceNamespaces` removed. +- `allowSourceNamespaceOverride` becomes `allowAnySourceNamespace`: same type, same default, same + semantics, so the shim is a pure rename. +- `allowedNamespaces` becomes `accessFrom`, same shape, same semantics. +- `sourceNamespace: "*"` becomes one cluster-wide list and watch, per the decision above, and is + rejected while `allowAnySourceNamespace` is false. This is a semantic change to a value that + keeps its spelling, so it needs its own `docs/UPGRADING.md` paragraph rather than a shim. + +Then the deletion, then the `SelfSubjectAccessReview` work, which is additive and can ship later. + +## Re-open triggers + +- **Impersonation**, if one source cluster must serve control-plane tenants with different + authority and they cannot each hold a credential. The cost is priced in the facts note. +- **Deleting `accessFrom`**, once an external policy recipe is shipped, tested, and its lack of + live revocation is an accepted product choice rather than an oversight. +- **Source-side selectors**, if a user is found who needs "every namespace labeled X" and cannot + generate bindings. + +## Open questions + +- ~~Which `*` option is taken.~~ **Decided: cluster-wide**, list and watch both, above. +- `CellKey.String()` renders an empty namespace as a bare type name ("configmaps"), which read as + cluster-scoped when only `ClusterWatchRule` produced those cells. Once `*` produces them for + namespaced types it wants a distinct rendering, "configmaps in all namespaces" or similar, in + logs and status messages. +- Does the chart keep an `allowAnySourceNamespace` value, and does the quickstart set it true? A + homelab wants it; a platform should refuse it. +- Does anything rely on `allowedSourceNamespaces.selector` today beyond the e2e suite, which is us + testing our own feature? diff --git a/docs/facts/kubernetes-impersonation-and-flux-identity.md b/docs/facts/kubernetes-impersonation-and-flux-identity.md new file mode 100644 index 00000000..7901a3eb --- /dev/null +++ b/docs/facts/kubernetes-impersonation-and-flux-identity.md @@ -0,0 +1,106 @@ +# Impersonation and service-account identity, as Kubernetes and Flux implement them + +> **facts** — durable reference. Index: [`../INDEX.md`](../INDEX.md) +> +> Read for [`../design/source-scope-simplification.md`](../design/source-scope-simplification.md), +> which decided against adopting impersonation. This page holds the evidence so the case does not +> have to be re-derived if the decision is re-opened. +> +> Verified against `k8s.io/apiserver@v0.34.1` in the module cache and the gitignored +> `external-sources/flux/` checkout. Both are named by path rather than linked, because neither is +> tracked by this repository. + +## What the API server does + +All five are from `pkg/endpoints/filters/impersonation.go`. + +**The subject does not have to exist.** An `Impersonate-User` header of +`system:serviceaccount:homelab-config:mirror` is split by `serviceaccount.SplitUsername` into a +`ServiceAccount` object reference, and nothing ever reads that object. The identity is a string, +and the namespace half need not name a namespace that exists. + +**The authorization check is namespaced and name-scoped.** The filter builds an attributes record +with verb `impersonate`, resource `serviceaccounts`, and both the namespace and the name taken from +the header. So an RBAC rule can bound impersonation with `resourceNames`. A `ClusterRole` doing so +bounds it by name only, across every namespace; bounding by namespace needs a `Role`. + +**The service-account groups are added without a check.** When no groups are requested the filter +sets `groups = serviceaccount.MakeGroupNames(namespace)`, which is `system:serviceaccounts` and +`system:serviceaccounts:`. Those are appended after the per-request authorization loop, +so no `impersonate` check runs against them. Any privilege the cluster binds to every service +account in that namespace comes along with the identity. + +**The impersonator's own permissions are not an upper bound.** The filter constructs a fresh +`user.DefaultInfo` and replaces the request user with it; the original survives only in the audit +log. There is no intersection between what the impersonator may do and what the impersonated +subject may do. + +> This last one is worth stating loudly, because the natural assumption is the opposite, and an +> earlier design review recorded that assumption as fact — that the operator's own `ClusterRole` +> remains the outer bound, so the effective permission is the intersection of what the operator may +> read and what the impersonated subject may read. It is not. The effective permission is +> exactly what the impersonated subject may do, which is why `resourceNames` scoping would be +> load-bearing rather than tidy: without it, `impersonate` on `serviceaccounts` is +> admin-equivalent in the target cluster. + +**Impersonation is per request, so it applies to watches.** The header travels on a watch request +like any other, and when the binding behind it is removed the API server ends the stream. Whoever +reads under an identity the target cluster issued gets revocation without implementing it. + +## What Flux does + +From `pkg/runtime/client/impersonator.go` and +`flux-operator/internal/controller/resourceset_controller.go`: + +- `setImpersonationConfig` builds `system:serviceaccount:%s:%s` from `i.serviceAccountNamespace`, + and every caller passes the reconciled object's own namespace + (`WithServiceAccount(r.DefaultServiceAccount, obj.Spec.ServiceAccountName, obj.GetNamespace())`). + No field anywhere lets a user write the namespace half. That is the whole security argument: a + tenant can claim only an identity bounded by the RBAC their own namespace already grants. +- `clientForKubeConfig` calls `setImpersonationConfig` too, so impersonation **composes** with a + remote kubeconfig rather than replacing it. +- `CanImpersonate` does a `Get` for the `ServiceAccount` through the local client. On the + kubeconfig path that checks the wrong cluster, and it is an existence check where an + authorization check is wanted. RFC 0010 flags it as a known bug. + +RFC 0001 states the opposite of the second point ("All accesses that would use impersonation use +the remote client instead"). The RFC predates the code. Cite the code. + +## Impersonating into another cluster + +RFC 0010 states the rule directly: with `spec.serviceAccountName`, the authenticated identity "must +have the necessary permissions to impersonate this `ServiceAccount` in the remote cluster". + +Combined with the first fact above, this means the impersonated `ServiceAccount` is resolved in the +remote cluster and nowhere else, and the namespace in the identity string comes from the local +object. The pair is a **name convention two clusters agree on**, not an object reference. The +remote cluster's admin honors it with an ordinary `RoleBinding` naming a subject whose namespace +may not exist locally to them. + +The consequence for any design that adopts this: whoever can create a namespace in the local +cluster can mint an identity claim against every remote cluster that ever granted that name. + +## Where Flux puts multi-tenancy + +Not in its API. `--no-cross-namespace-refs` and `--default-service-account` are controller flags, +and the workload-identity profile splits the latter into three by concern +(`--default-decryption-service-account`, `--default-kubeconfig-service-account`). The multi-tenant +lockdown is a documented Kyverno profile plus `flux-operator`'s +`internal/builder/profiles.go`. The one API-level exception, `acl.AccessFrom` with its +`namespaceSelectors`, is on sources and answers "which namespaces may reference this object". + +## What adopting impersonation here would require + +Recorded so the cost is not re-estimated: + +- The tenant must pick the name half and never the namespace half, or they can name + `kube-system:default`. +- `ClusterWatchRule` is cluster-scoped and has no namespace to derive from, so it needs an explicit + `serviceAccountRef`. Acceptable only because creating a cluster-scoped object is already an admin + act. +- The precheck is a `SubjectAccessReview` against the target cluster, not a `Get`, and it must + carry `system:serviceaccounts` and `system:serviceaccounts:` as groups or it answers + narrower than reality. That needs `create subjectaccessreviews` on a credential users have + already issued. +- Identity joins cluster, GVR, and namespace in the informer key, so it multiplies the resource the + `TooManyStreams` cap exists to bound. diff --git a/docs/layout/api-wave.md b/docs/layout/api-wave.md index 91d1bc41..0817c7fa 100644 --- a/docs/layout/api-wave.md +++ b/docs/layout/api-wave.md @@ -14,7 +14,8 @@ | Member | Object | Why breaking | |---|---|---| | **B4**: `commitWindow` and `commit.message` move off the connection | `GitProvider` → `GitTarget` | fields change object | -| The riders: **#5** asserted `CommitRequest.spec.author`, the `CommitRequest` lifecycle hole, `meta.LocalObjectReference` for our six reference shapes, the `TooManyStreams` cap (a `Stalled` reason and a bound on stream fan-out), the `default` `ClusterProvider` message | various | shape changes | +| The source-scope deletion ([`source-scope-simplification.md`](../design/source-scope-simplification.md)) | `GitTarget`, `ClusterProvider`, `WatchRule` | one removal, two renames, one redefinition | +| The riders: **#5** asserted `CommitRequest.spec.author`, the `CommitRequest` lifecycle hole, `meta.LocalObjectReference` for our six reference shapes, the `TooManyStreams` cap, the `default` `ClusterProvider` message | various | shape changes | Additive, and therefore **not** wave members even though they are discussed here: `spec.suspend`, `status.placement` and the post-scan pass, the reconcile-request annotation, and the two placement @@ -65,9 +66,9 @@ Two groupings, and one deliberate exception: reaches for it, and a `spec.write` wrapper invented to hold it plus `prune` plus `commit` would be a category we made up rather than one the API already has. `prune` is its own struct already. -After the wave the spec reads as named axes rather than a list: the immutable destination -(`providerRef`/`branch`/`path`), `encryption`, the source-scope fields, `placement`, `commit`, -`prune`, and the one switch. That is the test for the next field too — a new member either joins an axis or +After the wave the spec reads as six named axes rather than a list: the immutable destination +(`providerRef`/`branch`/`path`), `encryption`, `clusterProviderRef`, `placement`, `commit`, `prune`, +and the one switch. That is the test for the next field too — a new member either joins an axis or names a new one, and if it can do neither it is probably not a `GitTarget` field. ## The interactions that change the design @@ -129,7 +130,27 @@ data-plane facts into status with an enqueue on change. So the Event is: emit wh changes in a way a human should know about — `LayoutResolved` becoming `Ambiguous`, or a type falling back for the first time. One Event per persisted change, the pattern already established for `Ready`. -### 5. Two facts kept from arguments that dissolved +### 5. The source-scope deletion is the only member that shrinks the API + +It belongs here for the ordinary reason first: it removes a field from `GitTarget`, and B4 already +breaks `GitTarget` in the same release, so shipping them apart costs the consumer two bumps for one +object. The better reason is the review surface. Every other member adds a field; this one deletes +4,569 lines, a three-valued verdict, and the only cross-cluster read in the authorization path. A +wave that is otherwise all addition is easier to justify when the object comes out simpler than it +went in. + +Two interactions worth naming, because they change what gets built rather than merely when: + +- **The `SelfSubjectAccessReview` pass is the same shape as the dry run.** Both answer "tell me what + you would do before you do it". They are separate conditions on separate objects and neither + depends on the other, so the SAR work stays additive and can ship after the wave — but whoever + writes the second should read the first. +- **`TooManyStreams` is sized by the `*` decision.** `sourceNamespace: "*"` compiling to one + cluster-wide list and watch removes the fan-out the cap was queued for. What is left to bound is + explicit enumeration, so the cap is still worth a `Stalled` reason and a bound — sized against + enumerated rules, and not planned before the `*` change lands. + +### 6. Two facts kept from arguments that dissolved `spec.layout` immutability and the namespace-agreement rule were both settled by the reversal: neither field exists. Two facts from underneath them survive and will be reached for again. @@ -158,6 +179,8 @@ spec: name: platform branch: main path: clusters/prod + # allowedSourceNamespaces is gone: the provider's credential bounds what can be read, + # and ClusterProvider.accessFrom bounds who may wield it. # --- what the documents look like: ADDITIVE, not part of the wave --- placement: @@ -231,8 +254,8 @@ wave cost two, on two schedules. Two things follow, and both belong on this repo than only on the consumer's: - **Each wave leaves residue.** A refused field stays in the schema to say "no, not that anymore" — - one per removal, and it is paid per wave, so the number of remaining waves on `v1alpha3` is - finite. That graveyard is a real cost to a newcomer reading the CRD. Sweeping + `allowedSourceNamespaces` now, more later. That graveyard is a real cost to a newcomer reading the + CRD, and it is paid per wave, so the number of remaining waves on `v1alpha3` is finite. Sweeping the refusals is what `v1alpha4` should be for, and it should be one version bump carrying the removals rather than a version bump per change. - **The coupling is a dependency, not a courtesy.** Staying on `v1alpha3` requires the consumer to @@ -253,15 +276,22 @@ Dependencies first, then the things that only need the object to be breaking. 4. **`requestedAt` + `lastHandledReconcileAt`.** On-demand refresh of step 3. 5. **Events on a changed resolution**, over the existing recorder. 6. **B4**, as `spec.commit`. Last of the principle items, and the one that makes the object coherent. -7. **The riders.** Nothing else depends on them. - -Steps 2 to 5 are additive and need no bump. Steps 6 and 7 are one release; step 1 gates the planning; -step 7 can be trimmed if the wave gets too big to review, since nothing else depends on it. The +7. **The source-scope deletion.** Independent of every step above, so it can be written in parallel; + placed here because a deletion reviews better once the additions it is not entangled with are + settled. +8. **The riders.** `TooManyStreams` must come after step 7, which removes the fan-out it was written + to bound. + +Steps 2 to 5 are additive and need no bump. Steps 6 to 8 are one release; step 1 gates the planning; +step 8 can be trimmed if the wave gets too big to review, since nothing else depends on it. The placement fields are absent from this list on purpose: their order is [`implementation-plan.md`](implementation-plan.md)'s. ## What this costs, stated plainly -- **One coordinated consumer bump**, for `spec.commit` and the riders. -- **One `docs/UPGRADING.md` entry**, covering the field move and the riders. The placement work needs - no migration entry at all. +- **One coordinated consumer bump**, for `spec.commit`, the source-scope changes and the riders. +- **One `docs/UPGRADING.md` entry.** The placement work needs no migration entry at all. The `*` + paragraph is the one to write carefully: it is the only change here that keeps its spelling and + changes its meaning. +- **A capability genuinely lost**: source-side label selectors, priced in the source-scope document. + It is the only thing in this wave a user can do today and cannot do afterwards. diff --git a/docs/layout/examples/README.md b/docs/layout/examples/README.md index 711ced50..8d6fc3bd 100644 --- a/docs/layout/examples/README.md +++ b/docs/layout/examples/README.md @@ -37,6 +37,7 @@ is named only where that choice explains the scenario. | [Tree, multi-namespace](tree-multi-namespace/README.md) | How does a small cluster mirror several namespaces and cluster resources safely? | | [Homelab Argo CD](homelab-argocd/README.md) | How does an app-of-apps folder remain a narrow, editable target? | | [Homelab Flux](homelab-flux/README.md) | How do Flux declarations stay editable without treating chart output as source? | +| [CRD closure](crd-closure/README.md) | What else does a folder of custom resources need to be applied elsewhere? **A question, not a decision** | | [Overlay-scoped target](overlay-scoped-target/README.md) | How can an environment overlay change a supported field without claiming ownership of its base? **Write path, not placement** | ## Two fixture conventions diff --git a/docs/layout/examples/crd-closure/README.md b/docs/layout/examples/crd-closure/README.md new file mode 100644 index 00000000..3d57053b --- /dev/null +++ b/docs/layout/examples/crd-closure/README.md @@ -0,0 +1,103 @@ +# CRD closure: what else the folder needs + +> **This scenario has no decided answer, and that is deliberate.** It exists to make the choice in +> [`crd-handling.md`](../../../design/crd-handling.md) legible by showing the same event under both +> candidate shapes. Every other scenario in this folder illustrates a decision; this one illustrates +> a question, and the design page is where it gets settled. + +An application team's configuration folder holds `Widget` objects. The folder is complete as a +mirror and incomplete as an artifact: applied to a cluster that has never heard of +`widgets.apps.example.com`, every `Widget` in it is rejected. Something has to carry the definition, +and the interesting part is that it does not have to be this folder. + +## Starting repository + +[`repository/`](repository/) is `apps/shop` in the real repository: one kustomize root, one +ConfigMap, one existing `Widget`. + +```text +apps/shop/ + kustomization.yaml + configmap-storefront.yaml + widget-checkout.yaml +``` + +## Proposed configuration + +[`config/watchrule.yaml`](config/watchrule.yaml) subscribes to `widgets` and `configmaps`, and +**names no CRD at all**. That is the point: today the only way to get definitions into Git is a rule +for `customresourcedefinitions`, which mirrors every CRD in the cluster, Flux's and cert-manager's +included. The proposal derives the set instead, from the types the rules match, over a join +the operator already performs. + +[`config/gittarget.yaml`](config/gittarget.yaml) sets the proposed +`includeTypeDefinitions: Referenced`. The default is `None`. + +## The live input, and what it carries + +[`input/widget-search.yaml`](input/widget-search.yaml) is an ordinary new object. Its placement is +not the question: the folder's one root takes it as `widget-search.yaml` with a `resources:` entry, +in every option below. + +[`input/crd-widgets.yaml`](input/crd-widgets.yaml) is the definition behind it, and it is worth +reading for its metadata rather than its schema: + +- `meta.helm.sh/release-name` and `app.kubernetes.io/managed-by: Helm` say a Helm release owns this + object, and **`sanitize` does not strip them**, so they would arrive in Git intact. +- `kustomize.toolkit.fluxcd.io/name` says a Flux `Kustomization` applied it, and **`sanitize` does + strip that**, so it can only be read from the live object. + +Those two facts are most of the argument. + +## Option B: `Referenced` + +[`expected-referenced.patch`](expected-referenced.patch). The `Widget` lands as usual, and one extra +generated file appears: a `type-dependencies.yaml` naming the type, its served versions, the CRD, a +digest of its spec, and the Helm release the CRD came from. No schema, about fifteen lines, and it +changes only when the type set or the schema does. + +Whoever hydrates the folder reads it: the per-branch editing cluster installs the referenced +definitions before loading objects, and a reviewer can see at a glance which operators a branch +depends on. + +## Option A: `Vendored` + +[`expected-vendored.patch`](expected-vendored.patch). The definition itself is committed at the +canonical cluster-scoped path and registered in the root. It is self-contained, and it costs three +things the referenced shape does not: + +- **A second owner.** The committed copy still says `meta.helm.sh/release-name: widget-operator`, + so it claims to belong to a release in another cluster, and applying it elsewhere collides with + Helm's ownership check. +- **Size and churn.** Real schemas are hundreds of lines; Argo CD's `Application` CRD is about a + megabyte, and every operator upgrade rewrites it. +- **Ordering.** A folder holding a CRD and its custom resources has to apply the definition first. + Flux and Argo CD arrange that; `kubectl apply -k` over a plain folder does not. + +For application configuration it also puts the wrong team in charge: the app developer owns what the +`Widget` CRD looks like, and it belongs beside the controller that serves it. + +## Scenario contract + +- Starting repository: [`repository/`](repository/), which **is** `apps/shop/`. +- Live inputs: [`input/widget-search.yaml`](input/widget-search.yaml) and, as context rather than as + a watched object, [`input/crd-widgets.yaml`](input/crd-widgets.yaml). +- Expected Git change: [`expected-referenced.patch`](expected-referenced.patch) under the + recommendation, [`expected-vendored.patch`](expected-vendored.patch) under the alternative. +- Expected status: `Ready=True`; the type closure is an observation, never a condition. +- Boundary: the operator never installs, upgrades or deletes a CRD. It records or copies one. + +## What this scenario cannot show + +The definition is necessary and not always sufficient. `Widget` converts with nothing running, which +is why its manifest entry says `conversion: {strategy: None}` and why this scenario stays simple. A +type whose CRD declares `spec.conversion.strategy: Webhook`, or whose real cluster mutates objects +through a mutating admission webhook, needs something to answer, and a workload-less branch cluster +has nothing to answer with. The objects are accepted and are quietly not what production would have +stored. + +It does not need a *controller*, though, only an endpoint: `WebhookClientConfig` takes a `url` as +well as a `service`, and `url` is precisely the form for a webhook that does not run in the cluster. +The ladder, the constraints on that URL, and what it would take for ConfigButler to host +transformations rather than every team exposing an endpoint, are in +[`crd-handling.md`](../../../design/crd-handling.md). diff --git a/docs/layout/examples/crd-closure/config/gittarget.yaml b/docs/layout/examples/crd-closure/config/gittarget.yaml new file mode 100644 index 00000000..fac23d88 --- /dev/null +++ b/docs/layout/examples/crd-closure/config/gittarget.yaml @@ -0,0 +1,18 @@ +apiVersion: configbutler.ai/v1alpha3 +kind: GitTarget +metadata: + name: shop-configuration + namespace: shop +spec: + providerRef: + name: app-repository + branch: main + path: apps/shop + suspend: true # adoption dry run: scans and publishes, writes nothing + # The proposed field. None is the default and the posture this folder would have + # without the line: instances here, definitions owned by whoever installs the + # operator. Referenced commits a manifest of references, never schemas. + includeTypeDefinitions: Referenced + placement: + serializeNamespace: Never + kustomizeRoot: Require diff --git a/docs/layout/examples/crd-closure/config/watchrule.yaml b/docs/layout/examples/crd-closure/config/watchrule.yaml new file mode 100644 index 00000000..3038194d --- /dev/null +++ b/docs/layout/examples/crd-closure/config/watchrule.yaml @@ -0,0 +1,16 @@ +apiVersion: configbutler.ai/v1alpha3 +kind: WatchRule +metadata: + name: shop-configuration + namespace: shop +spec: + targetRef: + name: shop-configuration + rules: + # No rule names customresourcedefinitions. The closure is DERIVED from the + # types these rules actually match, which is the whole point: a rule for + # CRDs would mirror every CRD in the cluster. + - apiGroups: ["apps.example.com"] + resources: ["widgets"] + - apiGroups: [""] + resources: ["configmaps"] diff --git a/docs/layout/examples/crd-closure/expected-referenced.patch b/docs/layout/examples/crd-closure/expected-referenced.patch new file mode 100644 index 00000000..cc48c7b1 --- /dev/null +++ b/docs/layout/examples/crd-closure/expected-referenced.patch @@ -0,0 +1,46 @@ +diff --git a/apps/shop/kustomization.yaml b/apps/shop/kustomization.yaml +--- a/apps/shop/kustomization.yaml ++++ b/apps/shop/kustomization.yaml +@@ -3,4 +3,5 @@ kind: Kustomization + namespace: shop + resources: + - configmap-storefront.yaml + - widget-checkout.yaml ++ - widget-search.yaml +diff --git a/apps/shop/widget-search.yaml b/apps/shop/widget-search.yaml +new file mode 100644 +--- /dev/null ++++ b/apps/shop/widget-search.yaml +@@ -0,0 +1,8 @@ ++apiVersion: apps.example.com/v1alpha1 ++kind: Widget ++metadata: ++ name: search ++spec: ++ replicas: 3 ++ variant: faceted +diff --git a/apps/shop/type-dependencies.yaml b/apps/shop/type-dependencies.yaml +new file mode 100644 +--- /dev/null ++++ b/apps/shop/type-dependencies.yaml +@@ -0,0 +1,19 @@ ++# Generated. The types this folder contains that the API server does not serve ++# on its own, and where their definitions come from. Not applied: this file is ++# read by whatever hydrates the folder into a cluster. ++apiVersion: configbutler.ai/v1alpha3 ++kind: TypeDependencies ++observedAt: "2026-08-29T09:12:51Z" ++types: ++ - group: apps.example.com ++ resource: widgets ++ kind: Widget ++ servedVersions: [v1alpha1] ++ definition: widgets.apps.example.com ++ specDigest: sha256:8f2a1c9d ++ # Whether a cluster can serve this type with nothing running. Webhook here ++ # would name the endpoint a branch cluster has to point its CRD at. ++ conversion: {strategy: None} ++ source: ++ kind: HelmRelease ++ name: widget-operator ++ namespace: widget-system diff --git a/docs/layout/examples/crd-closure/expected-vendored.patch b/docs/layout/examples/crd-closure/expected-vendored.patch new file mode 100644 index 00000000..0aa3cb45 --- /dev/null +++ b/docs/layout/examples/crd-closure/expected-vendored.patch @@ -0,0 +1,48 @@ +# The alternative shape, for comparison. Same event, includeTypeDefinitions: Vendored. +# The Widget document and the kustomization entry are identical to the referenced +# patch and are elided here; what differs is everything below. +diff --git a/apps/shop/kustomization.yaml b/apps/shop/kustomization.yaml +--- a/apps/shop/kustomization.yaml ++++ b/apps/shop/kustomization.yaml +@@ -3,4 +3,6 @@ kind: Kustomization + namespace: shop + resources: + - configmap-storefront.yaml + - widget-checkout.yaml ++ - widget-search.yaml ++ - _cluster/apiextensions.k8s.io/customresourcedefinitions/widgets.apps.example.com.yaml +diff --git a/apps/shop/_cluster/apiextensions.k8s.io/customresourcedefinitions/widgets.apps.example.com.yaml b/apps/shop/_cluster/apiextensions.k8s.io/customresourcedefinitions/widgets.apps.example.com.yaml +new file mode 100644 +--- /dev/null ++++ b/apps/shop/_cluster/apiextensions.k8s.io/customresourcedefinitions/widgets.apps.example.com.yaml +@@ -0,0 +1,31 @@ ++apiVersion: apiextensions.k8s.io/v1 ++kind: CustomResourceDefinition ++metadata: ++ name: widgets.apps.example.com ++ labels: ++ app.kubernetes.io/managed-by: Helm ++ annotations: ++ meta.helm.sh/release-name: widget-operator ++ meta.helm.sh/release-namespace: widget-system ++spec: ++ group: apps.example.com ++ scope: Namespaced ++ names: ++ plural: widgets ++ singular: widget ++ kind: Widget ++ versions: ++ - name: v1alpha1 ++ served: true ++ storage: true ++ schema: ++ openAPIV3Schema: ++ type: object ++ properties: ++ spec: ++ type: object ++ properties: ++ replicas: {type: integer} ++ variant: {type: string} ++# ... and the rest of a real schema, which is where the megabyte goes. diff --git a/docs/layout/examples/crd-closure/input/crd-widgets.yaml b/docs/layout/examples/crd-closure/input/crd-widgets.yaml new file mode 100644 index 00000000..80737781 --- /dev/null +++ b/docs/layout/examples/crd-closure/input/crd-widgets.yaml @@ -0,0 +1,40 @@ +# The live CustomResourceDefinition backing apps.example.com/v1alpha1 Widget, +# as the operator reads it from the source cluster. Schema trimmed; a real one +# runs to hundreds of lines, and Argo CD's Application CRD to about a megabyte. +# +# Note what it carries. The Helm ownership metadata is NOT stripped by sanitize +# (internal/sanitize/types.go strips Flux, kro, applyset and two exact Argo keys), +# so a vendored copy arrives in Git still claiming to belong to a release in +# another cluster. The Flux label IS stripped, which is why provenance has to be +# captured here, live, and can never be read back out of the repository. +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: widgets.apps.example.com + labels: + app.kubernetes.io/managed-by: Helm + kustomize.toolkit.fluxcd.io/name: widget-operator + kustomize.toolkit.fluxcd.io/namespace: flux-system + annotations: + meta.helm.sh/release-name: widget-operator + meta.helm.sh/release-namespace: widget-system +spec: + group: apps.example.com + scope: Namespaced + names: + plural: widgets + singular: widget + kind: Widget + versions: + - name: v1alpha1 + served: true + storage: true + schema: + openAPIV3Schema: + type: object + properties: + spec: + type: object + properties: + replicas: {type: integer} + variant: {type: string} diff --git a/docs/layout/examples/crd-closure/input/widget-search.yaml b/docs/layout/examples/crd-closure/input/widget-search.yaml new file mode 100644 index 00000000..0c5f3011 --- /dev/null +++ b/docs/layout/examples/crd-closure/input/widget-search.yaml @@ -0,0 +1,18 @@ +# A second Widget, created in the intent cluster. It is an ordinary placement: +# the folder's one kustomize root takes it, and the CRD question below is about +# what ELSE the folder needs to be applied anywhere but the cluster it came from. +apiVersion: apps.example.com/v1alpha1 +kind: Widget +metadata: + name: search + namespace: shop + uid: 6d1b0e4c-6f0e-4a2f-9a0e-2c1b7f0a5d31 + resourceVersion: "418302" + generation: 1 + creationTimestamp: "2026-08-29T09:12:44Z" +spec: + replicas: 3 + variant: faceted +status: + observedGeneration: 1 + readyReplicas: 3 diff --git a/docs/layout/examples/crd-closure/repository/configmap-storefront.yaml b/docs/layout/examples/crd-closure/repository/configmap-storefront.yaml new file mode 100644 index 00000000..40ad3cd9 --- /dev/null +++ b/docs/layout/examples/crd-closure/repository/configmap-storefront.yaml @@ -0,0 +1,6 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: storefront +data: + currency: EUR diff --git a/docs/layout/examples/crd-closure/repository/kustomization.yaml b/docs/layout/examples/crd-closure/repository/kustomization.yaml new file mode 100644 index 00000000..9d9be182 --- /dev/null +++ b/docs/layout/examples/crd-closure/repository/kustomization.yaml @@ -0,0 +1,6 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +namespace: shop +resources: + - configmap-storefront.yaml + - widget-checkout.yaml diff --git a/docs/layout/examples/crd-closure/repository/widget-checkout.yaml b/docs/layout/examples/crd-closure/repository/widget-checkout.yaml new file mode 100644 index 00000000..480048ed --- /dev/null +++ b/docs/layout/examples/crd-closure/repository/widget-checkout.yaml @@ -0,0 +1,7 @@ +apiVersion: apps.example.com/v1alpha1 +kind: Widget +metadata: + name: checkout +spec: + replicas: 2 + variant: express diff --git a/docs/layout/examples/empty-repo-bootstrap/README.md b/docs/layout/examples/empty-repo-bootstrap/README.md index b8e6547a..8636fd98 100644 --- a/docs/layout/examples/empty-repo-bootstrap/README.md +++ b/docs/layout/examples/empty-repo-bootstrap/README.md @@ -50,8 +50,9 @@ it places. The convention is **established** rather than guessed. The folder is single-namespace by construction rather than by declaration: no `{namespace}` appears in any path, because no `placement` is declared and the created root places files beside itself. A second source namespace has nowhere to go that would not collide, and nothing on the target says so. -The WatchRule feeding it is namespaced and names its own source namespace, which is the fence that -actually holds. +The WatchRule feeding it is namespaced and names its own source namespace, which is the whole fence +after +[`source-scope-simplification.md`](../../../design/source-scope-simplification.md). This example is the configuration-as-data path from [the direction review](../../../future/direction-and-configuration-surface.md). The folder is a diff --git a/docs/layout/implementation-plan.md b/docs/layout/implementation-plan.md index d6283a63..7c3ed3c1 100644 --- a/docs/layout/implementation-plan.md +++ b/docs/layout/implementation-plan.md @@ -17,15 +17,16 @@ | 3 | `status.placement`, and the post-scan validation pass | no | 2 | | 4 | `placement.serializeNamespace` and `placement.kustomizeRoot` ([#322](https://github.com/ConfigButler/gitops-reverser/issues/322)) | no | 1, 3 | | 5 | `commitWindow` and `commit.message` move off `GitProvider`, as `GitTarget.spec.commit` | **yes** | — | -| 6 | The riders: the asserted CommitRequest author, its lifecycle, `meta.LocalObjectReference`, `TooManyStreams`, the ClusterProvider default message | **yes** | — | +| 6 | The riders: the asserted CommitRequest author, its lifecycle, `meta.LocalObjectReference`, `TooManyStreams`, the ClusterProvider default message | **yes** | the source-scope change, for `TooManyStreams` only | **PRs 1 to 4 are not breaking**, which is the largest change to this plan since it was written. The model no longer replaces `spec.placement` with a discriminated union; it keeps the template and adds two members whose defaults equal today's behavior. So there is no loud rejection, no `LocateNew` rewrite, no migration, and the whole placement story ships without a coordinated consumer bump. -What remains breaking is PRs 5 and 6, neither of which is about placement. They land in one release, -reviewed as two changes and paid for as one bump. PR 6 is the trim handle: nothing depends on it. +What remains breaking is PRs 5 and 6, neither of which is about placement. They land in one release +together with the source-scope deletion, which is not this plan's work but breaks the same object in +the same release (design change 3 below). PR 6 is the trim handle: nothing depends on it. **`F9`'s envtest stays outside this order and gates it.** Its answer constrains the enum work, so run it before PR 4 is planned; the question and the fallback are in [`api-wave.md`](api-wave.md). @@ -209,12 +210,16 @@ release. types, the `TooManyStreams` cap, and the ClusterProvider default message. In the release because they are breaking and the consumer should pay once. Trim from here first. -**Size `TooManyStreams` against what actually produces the fan-out**, which is the wildcard -source-namespace expansion, not the number of rules. It is the trim handle inside the trim handle. +**`TooManyStreams` is sequenced behind the `*` decision**, the one dependency this PR has on work +outside the plan. `sourceNamespace: "*"` is being redefined as a single cluster-wide list and watch, +and that is where most of the fan-out the cap was written for goes. Either land the `*` change first +and size the cap against enumerated rules, or drop the cap from this PR: sizing it against today's +per-namespace fan-out would bake in a number that is wrong the moment `*` lands. ## Design changes this plan folds in -Each came from reviewing the worked examples. The reversal dissolved four earlier entries — the +Each came from reviewing the worked examples, except the last, which came from the source-scope +decision and lands on the same PRs. The reversal dissolved four earlier entries outright — the namespace agreement rule, `kind: Template` leaving the wave, `Auto` as the CRD default, and `interval` on two objects — because every field they amended is gone. They are not restated here. @@ -232,10 +237,28 @@ argument for observing per scan: the declaration never changed, only the folder The homelab examples put a `GitTarget` in `argocd`, `flux-system` and `homelab-config`, each needing its own `GitProvider` — three copies of one credential in a single-owner cluster. `ClusterProvider` -already has the `allowedNamespaces` and fail-closed SAR machinery a cluster-scoped Git provider would need. +already has the `accessFrom` and fail-closed SAR machinery a cluster-scoped Git provider would need. Out of scope for this order; file it, and say so in the examples' prerequisites so a reader does not read the duplication as intended design. +### 3. The source-scope simplification shares this release + +Not placement work, and folded in here because it lands on the same objects, in the same breaking +release, and moves a number this plan quotes. +[`source-scope-simplification.md`](../design/source-scope-simplification.md) deletes +`GitTarget.spec.allowedSourceNamespaces`, renames two `ClusterProvider` fields, and redefines +`sourceNamespace: "*"` as a single cluster-wide list and watch at `metav1.NamespaceAll`, rejected +while `allowAnySourceNamespace` is false. + +- **It rides PR 5's release, not PR 5.** It breaks `GitTarget`, as PR 5 does, so it must be in the + same bump; it is otherwise independent and reviews as its own change. +- **PR 6's `TooManyStreams` cap shrinks**, as recorded above. +- **The corpus is unaffected, and that is worth checking rather than assuming.** Records carry the + object's own `metadata.namespace` rather than the cell's, so a document arriving from a cluster-wide + cell is placed exactly as it is today and no expected patch moves. The corpus should nonetheless + grow one `sourceNamespace: "*"` scenario once the change lands, because "the cell is cluster-wide + but the path still carries `{namespace}`" is precisely the pair a reader gets wrong. + ## Corpus gaps to fill in PR 1 - **A refusal scenario.** Every example is a happy path, and the post-scan pass has the least