diff --git a/api/v1alpha3/gittarget_types.go b/api/v1alpha3/gittarget_types.go index f9d70f39..9eee0040 100644 --- a/api/v1alpha3/gittarget_types.go +++ b/api/v1alpha3/gittarget_types.go @@ -87,6 +87,52 @@ type GitTargetSpec struct { // +optional Placement *GitTargetPlacementSpec `json:"placement,omitempty"` + // Design rationale, kept out of the generated CRD description by the blank line below. + // + // It sits at the TOP LEVEL rather than inside placement, and the line between the two is + // retroactivity. spec.placement decides where a NEW document goes and never moves one already + // written; this governs the bytes of EVERY write, and it also decides how a managed document + // is FOUND — a document whose namespace is inherited is located in the file bytes by a + // namespace-less identity. A field with that blast radius nested inside a struct documented as + // "new files only" would be a trap. + // + // It is a *bool because no plain default preserves today's behavior: false breaks a flat + // folder, whose documents must carry their own namespace or they are ambiguous, and true + // writes a redundant line into every kustomize folder that already supplies one. nil means + // infer, which is what the operator has always done. + // + // The name deliberately avoids writeNamespace. "Write" is the most loaded word in this API — + // the write boundary, the write jail, WriteBoundaryRefused — so writeNamespace: false invites + // the reading "never write to this namespace", a permission, which is precisely what the + // neighbouring sourceNamespace fields are. + // + // See docs/layout/model.md § "serializeNamespace". + + // SerializeNamespace declares whether a committed document carries its own + // metadata.namespace. It governs every write this target makes, not just the first one, and it + // applies to NAMESPACED resources only — a cluster-scoped document has no namespace, so the + // field is ignored for it rather than being an error. + // + // Omitted, the namespace is INFERRED per document, which is the behavior a target that says + // nothing has always had: metadata.namespace is omitted only when the kustomization governing + // the document's path sets namespace: to that resource's own namespace, and written explicitly + // in every other case. Leave it unset for a folder that is legitimately non-uniform — a tree of + // nested kustomize roots, each supplying its own namespace — because inference resolves each + // document against the root governing its own path. + // + // true always writes it: the setting for a flat folder applied directly, where nothing + // downstream supplies a namespace and a document without one is ambiguous. + // + // false never writes it, and is a claim about the whole folder: something outside this + // repository — a Flux Kustomization's targetNamespace, an Argo Application's + // destination.namespace, or a kustomization this target maintains itself — supplies the + // namespace instead. Because a namespace-less document takes its namespace from a single + // supplier, an explicit false admits exactly ONE source namespace: a second WatchRule bringing + // another namespace to this target is refused, with GitPathAccepted=False and reason + // MultipleSourceNamespaces. + // +optional + SerializeNamespace *bool `json:"serializeNamespace,omitempty"` + // Design rationale, kept out of the generated CRD description by the blank line below. // // It defaults to a concrete {name: "default"} rather than an implicit nil so a target that omits @@ -228,6 +274,52 @@ type GitTargetPlacementSpec struct { // — give every sensitive type an explicit identity-complete ByType entry. // +optional Default string `json:"default,omitempty"` + + // Design rationale, kept out of the generated CRD description by the blank line below. + // + // It has exactly ONE job, and the name says less than the field does. Registering a new file + // with the kustomization that already governs its directory is an INVARIANT rather than a + // setting (#295, fixed by #319): a file no kustomization lists is a file nothing renders, so + // that happens in both columns. What this flag decides is only what to do when there is no + // root at all. + // + // So useKustomize: false does not mean "leave kustomize alone". If a folder's root must not be + // touched, do not point a GitTarget at that folder: the ancestor walk is bounded by the write + // jail, so a kustomization ABOVE spec.path is never edited, and rooting the target lower is + // the existing, better-tested way to say it. + // + // It belongs inside placement, unlike spec.serializeNamespace, because it is retroactive in + // the same way the rest of this struct is: it decides whether a NEW file's directory has a + // root to join, and creates one if not. Nothing already written moves or changes. + // + // See docs/layout/model.md § "useKustomize". + + // UseKustomize declares that this folder is a kustomize folder whose root the operator + // maintains. It controls one thing: what happens when NO kustomization governs the path a new + // document lands at. + // + // Omitted or false, the document is written and nothing else is touched. True, a + // kustomization.yaml is created at spec.path and the new document is registered in it as part + // of the same commit. + // + // A created root ADOPTS the folder: its resources: lists every managed document already there + // as well as the new one. Turning a folder into a kustomize folder means the folder, and a + // root naming one file would leave every other file in Git but out of every render. + // + // A created root carries NO namespace:. It holds an apiVersion, a kind and the resources: list + // and nothing else, so the namespace still comes from the documents (serializeNamespace unset + // or true) or from whatever installs the folder (serializeNamespace false). + // + // A folder that already has a kustomize render root never gains a second one, because two + // render roots is an ambiguous folder that stops accepting new documents altogether. If a + // byType or default template places a document outside the existing root, that placement is + // REFUSED rather than committed as a file no kustomization would render. + // + // It has NO bearing on a folder that already has a root. A new file is always registered with + // the nearest kustomization governing it, whatever chose its path, because a file no + // kustomization lists is a file kustomize never builds. + // +optional + UseKustomize bool `json:"useKustomize,omitempty"` } // GitTargetStatus defines the observed state of GitTarget. diff --git a/api/v1alpha3/zz_generated.deepcopy.go b/api/v1alpha3/zz_generated.deepcopy.go index 4a43f048..53c820dd 100644 --- a/api/v1alpha3/zz_generated.deepcopy.go +++ b/api/v1alpha3/zz_generated.deepcopy.go @@ -811,6 +811,11 @@ func (in *GitTargetSpec) DeepCopyInto(out *GitTargetSpec) { *out = new(GitTargetPlacementSpec) (*in).DeepCopyInto(*out) } + if in.SerializeNamespace != nil { + in, out := &in.SerializeNamespace, &out.SerializeNamespace + *out = new(bool) + **out = **in + } if in.ClusterProviderRef != nil { in, out := &in.ClusterProviderRef, &out.ClusterProviderRef *out = new(ClusterProviderReference) diff --git a/charts/gitops-reverser/templates/validate-operator-types-webhook.yaml b/charts/gitops-reverser/templates/validate-operator-types-webhook.yaml index 0fd57a40..3d382ebf 100644 --- a/charts/gitops-reverser/templates/validate-operator-types-webhook.yaml +++ b/charts/gitops-reverser/templates/validate-operator-types-webhook.yaml @@ -1,11 +1,16 @@ {{- if .Values.servers.admission.enabled }} --- -# Captures the authenticated submitter of our own command kinds (a CommitRequest today) -# at admission, into the command-author Redis corner the controller reads back with no -# wait. Narrow by construction: one rule per command kind. failurePolicy is Ignore and -# the handler always allows, so a user's CommitRequest never depends on this webhook — -# a miss leaves the request without a claimed actor (AuthorAttributed=False). -# See docs/spec/commitrequest-admission-authorship.md. +# Two unrelated jobs on one endpoint, both narrow by construction. +# +# It captures the authenticated submitter of our own command kinds (a CommitRequest today) +# into the command-author Redis corner the controller reads back with no wait; a miss +# leaves the request without a claimed actor (AuthorAttributed=False). See +# docs/spec/commitrequest-admission-authorship.md. +# +# It also rejects a WatchRule that would bring a second source namespace to a GitTarget +# declaring spec.serializeNamespace: false. That rejection is FEEDBACK, not enforcement: +# the writer refuses the same configuration at the write whether or not this webhook ran, +# which is why failurePolicy stays Ignore. See docs/layout/model.md. apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: @@ -27,7 +32,8 @@ webhooks: path: /validate-operator-types port: {{ .Values.servers.admission.port }} # Ignore, not Fail: a missed author capture must leave the request unnamed, never - # reject a user's CommitRequest. + # reject a user's CommitRequest, and the WatchRule check is feedback the write path + # already enforces on its own. failurePolicy: Ignore matchPolicy: Equivalent rules: @@ -40,6 +46,18 @@ webhooks: resources: - commitrequests scope: Namespaced + # UPDATE as well as CREATE: a rule can grow a second sourceNamespace long after it + # was created, and that edit is the same mistake. + - apiGroups: + - configbutler.ai + apiVersions: + - v1alpha3 + operations: + - CREATE + - UPDATE + resources: + - watchrules + scope: Namespaced # Redis write on real requests; nothing on dry-run (the handler honors this). sideEffects: NoneOnDryRun timeoutSeconds: {{ .Values.servers.admission.timeoutSeconds }} diff --git a/cmd/main.go b/cmd/main.go index 085893d5..d9c49414 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -1155,7 +1155,8 @@ func newManager( // setupAdmissionWebhooks registers both handlers on the one admission server: the // always-allow observer (a future-policy extension point) and the validate-operator-types -// handler that captures the submitter of our own command kinds into commandAuthorStore. +// handler, which captures the submitter of our own command kinds into commandAuthorStore and +// validates a WatchRule's source namespaces against the GitTarget it names. func setupAdmissionWebhooks(mgr ctrl.Manager, commandAuthorStore *queue.CommandAuthorStore) { mgr.GetWebhookServer().Register( webhookhandler.ValidateAllPath, @@ -1163,7 +1164,7 @@ func setupAdmissionWebhooks(mgr ctrl.Manager, commandAuthorStore *queue.CommandA ) // Leave Store as a nil interface when there is no Redis-backed store, so the handler // no-ops rather than dereferencing a typed-nil *CommandAuthorStore. - operatorTypesHandler := &webhookhandler.ValidateOperatorTypesHandler{} + operatorTypesHandler := &webhookhandler.ValidateOperatorTypesHandler{Client: mgr.GetClient()} if commandAuthorStore != nil { operatorTypesHandler.Store = commandAuthorStore } diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index 1df8f7e7..cabfebf3 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -330,6 +330,33 @@ spec: such as "all.yaml") is only valid when a sensitive resource can never reach it — give every sensitive type an explicit identity-complete ByType entry. type: string + useKustomize: + description: |- + UseKustomize declares that this folder is a kustomize folder whose root the operator + maintains. It controls one thing: what happens when NO kustomization governs the path a new + document lands at. + + Omitted or false, the document is written and nothing else is touched. True, a + kustomization.yaml is created at spec.path and the new document is registered in it as part + of the same commit. + + A created root ADOPTS the folder: its resources: lists every managed document already there + as well as the new one. Turning a folder into a kustomize folder means the folder, and a + root naming one file would leave every other file in Git but out of every render. + + A created root carries NO namespace:. It holds an apiVersion, a kind and the resources: list + and nothing else, so the namespace still comes from the documents (serializeNamespace unset + or true) or from whatever installs the folder (serializeNamespace false). + + A folder that already has a kustomize render root never gains a second one, because two + render roots is an ambiguous folder that stops accepting new documents altogether. If a + byType or default template places a document outside the existing root, that placement is + REFUSED rather than committed as a file no kustomization would render. + + It has NO bearing on a folder that already has a root. A new file is always registered with + the nearest kustomization governing it, whatever chose its path, because a file no + kustomization lists is a file kustomize never builds. + type: boolean type: object providerRef: description: |- @@ -377,6 +404,31 @@ spec: - Always type: string type: object + serializeNamespace: + description: |- + SerializeNamespace declares whether a committed document carries its own + metadata.namespace. It governs every write this target makes, not just the first one, and it + applies to NAMESPACED resources only — a cluster-scoped document has no namespace, so the + field is ignored for it rather than being an error. + + Omitted, the namespace is INFERRED per document, which is the behavior a target that says + nothing has always had: metadata.namespace is omitted only when the kustomization governing + the document's path sets namespace: to that resource's own namespace, and written explicitly + in every other case. Leave it unset for a folder that is legitimately non-uniform — a tree of + nested kustomize roots, each supplying its own namespace — because inference resolves each + document against the root governing its own path. + + true always writes it: the setting for a flat folder applied directly, where nothing + downstream supplies a namespace and a document without one is ambiguous. + + false never writes it, and is a claim about the whole folder: something outside this + repository — a Flux Kustomization's targetNamespace, an Argo Application's + destination.namespace, or a kustomization this target maintains itself — supplies the + namespace instead. Because a namespace-less document takes its namespace from a single + supplier, an explicit false admits exactly ONE source namespace: a second WatchRule bringing + another namespace to this target is refused, with GitPathAccepted=False and reason + MultipleSourceNamespaces. + type: boolean suspend: description: |- Suspend stops this target from writing to Git, without deleting it. It is the knob to turn diff --git a/config/webhook/validating-webhook.yaml b/config/webhook/validating-webhook.yaml index 2d0b2c17..37830cbc 100644 --- a/config/webhook/validating-webhook.yaml +++ b/config/webhook/validating-webhook.yaml @@ -81,12 +81,18 @@ webhooks: # reject a user's CommitRequest. The handler also returns Allowed even when the Redis # write errors, so the command's success never depends on this webhook at all — # Ignore only covers the webhook being entirely unreachable. + # + # It stays Ignore now that the endpoint can also REJECT a WatchRule, because that + # rejection is feedback rather than enforcement: the writer's own precondition refuses + # the same configuration whether or not this webhook ran. Fail here would make every + # WatchRule write depend on the operator being up, to gate something already gated. failurePolicy: Ignore matchPolicy: Equivalent name: validate-operator-types.configbutler.ai - # Narrow: only our own command kinds (one rule per command kind). The submitter is - # captured into the command-author Redis corner and read back by the CommitRequest - # controller with no wait. + # Narrow: our own command kinds (one rule per command kind), plus the one config kind + # this endpoint validates. The submitter of a command is captured into the command-author + # Redis corner and read back by the CommitRequest controller with no wait; a WatchRule is + # checked against the GitTarget it names and may be rejected. rules: - apiGroups: - configbutler.ai @@ -97,6 +103,18 @@ webhooks: resources: - commitrequests scope: Namespaced + # UPDATE as well as CREATE: a rule can grow a second sourceNamespace long after it was + # created, and that edit is the same mistake. + - apiGroups: + - configbutler.ai + apiVersions: + - v1alpha3 + operations: + - CREATE + - UPDATE + resources: + - watchrules + scope: Namespaced # Redis write on real requests; nothing on dry-run (the handler honors NoneOnDryRun). sideEffects: NoneOnDryRun timeoutSeconds: 2 diff --git a/docs/INDEX.md b/docs/INDEX.md index adff1e47..71c4c625 100644 --- a/docs/INDEX.md +++ b/docs/INDEX.md @@ -104,6 +104,7 @@ Eighteen other open items: |---|---| | [`open-asks-priority.md`](design/open-asks-priority.md) | **the work queue.** Merges three overlapping backlogs — the gitops-api consumer asks, the API-surface block left unbuilt by the status and configuration-model review, and the config-surface proposal (B1–B6) — into one ordered queue under four stated tests, and says where we deliberately do **not** do what was asked. The standing caveat narrowed once the layout model reversed: a Tier 2 entry belongs to postponed [#294](https://github.com/ConfigButler/gitops-reverser/issues/294) only if it breaks a `GitTarget` field, and everything else is independently schedulable. Makes one design call against what was asked: **delete Option C sibling inference** rather than ship an off-switch for it, because it let a human's edit to the repository change operator behavior with nothing in status recording the move. That deletion has shipped, and "what the deletion taught" records what building it found. **F9 is Tier 1**: the only item whose answer is unknown rather than whose work is unscheduled, and it gates planning the enum work | | [`placement-visibility-and-declared-defaults.md`](design/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 [`layout/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 | +| [`created-root-namespace.md`](design/created-root-namespace.md) | **design, decided.** One question with five answers: what namespace a `kustomization.yaml` the operator CREATES should carry. Decided **B, never write one** — `spec.serializeNamespace: false` means the artifact does not encode its deployment namespace, and adding a root must not quietly change that contract; the namespace comes from the documents when the field is unset or `true`, and from the installer (Flux `targetNamespace`, Argo `destination.namespace`) when it is `false`. Records the three facts an earlier draft got wrong (a namespace-less root is ordinary, both installers supply one, and what refused it was our own fidelity gate rather than kustomize), and carries the scoped fidelity rule that follows: the namespace is ignored in the render comparison ONLY when the governing root sets none, so a root that declares `namespace: shop` still rejects a live `billing` object. Also the sibling call: under `useKustomize` a placement no `resources:` list would name is refused rather than committed unrendered | | [`build-order.md`](design/build-order.md) | **design**, and the one page that is only about sequencing. Five in-flight changes resolve to **three tracks that do not block each other** — additive placement, the breaking source-scope wave, and patch authoring — with the two real couplings named (`*` is defined in terms of the field the wave deletes; `useKustomize`'s created root depends on the one-source-namespace rule) and three couplings people keep assuming that do not exist. Holds no design: every item is specified elsewhere and the specification wins | | [`gittarget-api-wave.md`](design/gittarget-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 layout model reversed and left it: 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 | | [`target-watch-plan.md`](design/target-watch-plan.md) | **built.** The companion to [`watch-manager-ownership.md`](design/watch-manager-ownership.md): the ownership page says WHO applies a plan, this one says WHAT a plan is and what changing it may touch. A cell — group, resource, namespace, deliberately no served version — is the one identity the watch stream, the render-fidelity scope and the mark-and-sweep boundary all agree on, because a key that does not round-trip to the scope it sweeps under is the class of error that deletes user data. The plan is diffed into `keep`/`start`/`restart`/`stop` and applied per cell, so adding one WatchRule stops replaying every unrelated cell into a queue shared with other tenants; a `restart` is a served-version change, which is why the version is spec DATA rather than identity. Readiness and the fidelity revision are per scope, so a KEPT cell holds the result its own replay produced rather than being asked to prove itself again over an unrelated edit. `stop` never touches files — removal is a Git-side sweep under the target's existing `spec.prune.mode`, not a watch-layer delete. "Cut at the producer" is the accepted consequence: nothing fences the queue, so a deselected cell may leave a short tail of writes, bounded by the queue and converged afterwards. Still open: the `stop` classification wants a settled `TypeRemoved` from `typeset` (see TODO), and removal on INTENT is undecided. | diff --git a/docs/configuration.md b/docs/configuration.md index fb657f04..38843008 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -480,6 +480,12 @@ The important fields are: - `spec.placement`: optional policy for where **new** resources are written (see [Where new resources are written](#where-new-resources-are-written-specplacement)); omit it and a new resource takes the folder's one kustomization root, or the built-in canonical path +- `spec.placement.useKustomize`: whether the operator maintains a `kustomization.yaml` for this + folder, creating one when the folder has none (see + [Keeping the folder a kustomize folder](#keeping-the-folder-a-kustomize-folder-specplacementusekustomize)) +- `spec.serializeNamespace`: whether written documents carry their own `metadata.namespace` (see + [Whether documents carry their namespace](#whether-documents-carry-their-namespace-specserializenamespace)); + omit it and each document's namespace is inferred from the folder - `spec.prune`: which deletion paths may remove documents from this target's folder (see [Deletion policy](#deletion-policy-specprunemode)); omit it for the safe default @@ -867,6 +873,169 @@ co-mingled with a plaintext document. Two consequences for your templates: resource is **skipped fail-safe** (logged and counted in the resync summary as `placementSkipped`) rather than written unsafely. It is not surfaced as a dedicated status condition today. +### Choosing between the two layout fields + +`spec.serializeNamespace` and `spec.placement.useKustomize` answer two independent questions, and a +target that sets neither behaves exactly as it did before they existed. Start here, then read the +section for whichever you set. + +**Question 1: where does the namespace of a mirrored object live?** + +| You want | Set | What the folder looks like | +|---|---|---| +| Each document to say which namespace it is in. Someone can `kubectl apply -f` the folder and land everything where it came from | `serializeNamespace: true` | every namespaced document carries `metadata.namespace` | +| The folder to be installable into a namespace chosen at install time, by a Flux `targetNamespace`, an Argo `destination.namespace`, or a `kustomization.yaml` you wrote | `serializeNamespace: false` | no document carries `metadata.namespace`, and nothing in the folder pins one | +| Neither claim, because the folder is a tree of nested kustomize roots that each supply their own namespace | leave it **unset** | each document is resolved against the root governing its own path | + +**Question 2: does the operator maintain this folder's `kustomization.yaml`?** + +| You want | Set | What the operator does | +|---|---|---| +| The folder to become a kustomize folder, including from empty | `placement.useKustomize: true` | creates `kustomization.yaml` at `spec.path` if there is none, adopting the files already there, and registers every new document in it | +| Plain files, or a `kustomization.yaml` only you create | leave it **unset** | writes the document; registers it in a root that already governs it, and creates nothing | + +Registering a new file with a kustomization that **already** governs it happens either way. That is +not a setting: a file no `resources:` list names is a file kustomize never builds. + +**The three combinations in practice:** + +| Shape | Spec | Who supplies the namespace | +|---|---|---| +| A mirror you can apply back | `serializeNamespace: true`, no `useKustomize` | the documents | +| A portable artifact | `serializeNamespace: false` **+** `useKustomize: true` | whatever installs the folder | +| An existing kustomize repository | leave both unset | the `kustomization.yaml` files already there | + +One combination to avoid: `serializeNamespace: false` on a folder nothing installs. The documents +carry no namespace and nothing supplies one, so `kubectl apply -f` lands them all in `default`. +Nothing can detect that for you, because the installer lives outside the repository. See +[why nothing checks the supplier](#whether-documents-carry-their-namespace-specserializenamespace). + +### Whether documents carry their namespace (`spec.serializeNamespace`) + +A path decides where a file sits; it cannot decide what is inside it. `spec.serializeNamespace` +does: whether the committed document carries its own `metadata.namespace`. + +```yaml +spec: + path: apps/checkout + serializeNamespace: false +``` + +| Value | What is written | +|---|---| +| omitted (default) | inferred per document, which is today's behavior | +| `true` | every namespaced document carries `metadata.namespace` | +| `false` | no document carries it; something outside the folder supplies it | + +It governs **every** write the target makes, not only the first one. That is why it sits at the top +level of the spec rather than inside `spec.placement`, which decides where *new* documents go and +never moves one already written. It applies to **namespaced resources only**: a `ClusterRole` has no +namespace, so the field is ignored for it rather than being an error. + +**Omitted is not the same as `false`, and it is the right answer more often than either.** Inference +omits `metadata.namespace` only when the kustomization governing that document's path sets +`namespace:` to *this* resource's own namespace, and writes it explicitly otherwise, because +omitting it anywhere else would hand the document to a different namespace than the object it +mirrors. Leave the field unset for a folder that is legitimately non-uniform, such as a tree of +nested kustomize roots each supplying its own namespace: inference resolves every document against +the root governing its own path, which no single folder-wide value can do. + +The two explicit values are for the two shapes people declare: + +- **`true` for a flat folder applied directly.** Nothing downstream supplies a namespace, so a + document without one is ambiguous. It also keeps the document portable: it means the same thing + pasted anywhere. +- **`false` for a folder whose namespace comes from outside it**: a Flux `Kustomization`'s + `spec.targetNamespace`, an Argo CD `Application`'s `spec.destination.namespace`, or a + hand-written `kustomization.yaml` in the folder that sets `namespace:`. A root the operator + creates never sets one. + +**Nothing checks that the supplier exists, and nothing can.** For a raw namespace-free folder the +supplier lives in the cluster that *consumes* the repository, and there may be more than one of +them: two deployers may land the same folder in two different namespaces, both correctly. That +portability is the point of the shape, so a rule demanding proof inside the folder would report a +fault against a folder doing exactly what it was built for. + +**One thing is checked, and it refuses.** An explicit `serializeNamespace: false` admits exactly +**one source namespace**. A namespace-free document takes its namespace from a single supplier, so +two source namespaces reaching the folder contradict the setting itself, and the failure is silent: +`shop/config` and `billing/config` both resolve to one `config.yaml` whose bytes name no namespace, +so they are not two documents that collide but one document two live objects take turns +overwriting. A second `WatchRule` bringing another source namespace to such a target is refused with +`GitPathAccepted=False`, reason `MultipleSourceNamespaces`. A rule naming `sourceNamespace: "*"` is +refused too, without enumerating anything, because a wildcard cannot be shown to be one namespace. +The set counted is the target's own namespace plus the explicit `rules[].sourceNamespace` of every +`WatchRule` pointing at it. It is unrelated to `spec.allowedSourceNamespaces`, which answers who may +write here rather than what the folder means. + +Inference is never fenced this way. A folder that is truly multi-namespace and namespace-free is +what leaving the field **unset** is for. + +**What the render check does with the namespace.** Every write is compared against the live object +it mirrors, and `metadata.namespace` is the one field allowed not to match. It is ignored only when +the folder renders the document with no namespace at all, which is `serializeNamespace: false` with +nothing in the folder supplying one. A `kustomization.yaml` that declares `namespace:` makes a +concrete claim, so it is still compared: a root declaring `namespace: shop` rendering an object that +lives in `billing` is a relocation, and it is refused whatever `serializeNamespace` says. + +### Keeping the folder a kustomize folder (`spec.placement.useKustomize`) + +```yaml +spec: + path: apps/checkout + serializeNamespace: false + placement: + useKustomize: true +``` + +It controls exactly one thing: **what happens when no `kustomization.yaml` governs the path a new +document lands at.** + +| | A kustomization governs the path | Nothing governs the path | +|---|---|---| +| omitted / `false` (default) | the new file joins its `resources:` list | the file is written and nothing else is touched | +| `true` | the new file joins its `resources:` list | a `kustomization.yaml` is **created** at `spec.path`, and the file joins it in the same commit | + +**Registering a new file with the kustomization that already governs it is not what this flag +controls.** It happens in both rows, because a file no kustomization lists is a file kustomize never +builds. The flag is only about the empty case, which is what makes an empty repository +bootstrappable. + +**A created root adopts the whole folder.** Its `resources:` lists every managed document +already in the folder alongside the new one, at the paths those files already have. Nothing is +moved, rewritten or re-encoded. A root naming only the new file would leave every other file +sitting in Git and out of every render: the moment a consumer ran `kustomize build` against the +folder, they would stop being applied, with nothing to show what happened. + +**A folder that already has a render root never gains a second one, and a document it would not +render is refused.** If a `byType` or `default` template puts the new document somewhere the +existing root does not govern, the placement is refused with `GitPathAccepted=False`, reason +`UnrenderedPlacement`. Two render roots in one target's folder is the ambiguous case, and an +ambiguous folder stops accepting new documents entirely; committing the file unregistered instead +would leave a document sitting in Git looking mirrored while nothing applies it. The fix is a +template that keeps its documents inside the root, or a target pointed at the folder that root +governs. Without `useKustomize` nothing changes: a target that made no claim about kustomize keeps +its current behavior. + +**The created root carries no `namespace:`.** It is an `apiVersion`, a `kind` and the `resources:` +list, and nothing else. `serializeNamespace: false` says the artifact does not encode its deployment +namespace, and creating a root must not re-encode it one file up where an installer cannot override +it, so the namespace still comes from the documents (`serializeNamespace` unset or `true`) or from +whatever installs the folder. The reasoning, and the four other answers that were considered, is in +[`design/created-root-namespace.md`](design/created-root-namespace.md). That last line is the point of the pairing with +[`serializeNamespace: false`](#whether-documents-carry-their-namespace-specserializenamespace): on +an empty folder there is nothing to infer from, and the operator owns the file the omission depends +on, so the omission is provable rather than trusted. + +The new document is placed **beside the root**, exactly as it would be beside a root that was +already there, unless a `byType` or `default` template says otherwise. A declared template still +decides the path; the created root lists the document wherever the template put it. + +The name says less than the field does, so one reading is worth ruling out: `useKustomize: false` +does not mean "leave kustomize alone". If a folder's `kustomization.yaml` must never be touched, do +not point a `GitTarget` at that folder. A kustomization **above** `spec.path` is never edited (the +ancestor walk stops at the write jail), so rooting the target lower is the way to say it. + ### Additional sensitive resources Core Kubernetes `Secret` resources always use the encrypted Git write path. For a Secret-shaped diff --git a/docs/design/created-root-namespace.md b/docs/design/created-root-namespace.md new file mode 100644 index 00000000..a9bdc5c2 --- /dev/null +++ b/docs/design/created-root-namespace.md @@ -0,0 +1,109 @@ +# What namespace does a created `kustomization.yaml` carry? + +> **design**: decided, and being built. Index: [`../INDEX.md`](../INDEX.md) +> Date: 2026-09-01. +> +> One question, five answers, and the reason the obvious one is wrong. It came out of review of +> [#328](https://github.com/ConfigButler/gitops-reverser/pull/328), which shipped +> `spec.placement.useKustomize` writing `namespace:` into every root it created. This page records +> the choice so the next reader does not have to re-derive it from a diff. + +## The question + +`spec.placement.useKustomize: true` creates a `kustomization.yaml` when a folder has none. +`spec.serializeNamespace: false` says the documents in that folder carry no `metadata.namespace`. +Set both and something has to decide: does the root the operator writes carry `namespace: `, or nothing at all? + +It is not a formatting question. It decides whether the folder is an artifact anyone can install +anywhere, or a mirror pinned to the namespace it was captured from. + +## What is not in question + +Three facts, because the first draft of this argument got them wrong: + +- **A `kustomization.yaml` with no `namespace:` is ordinary.** It is what a kustomize *base* is. + Three already ship in our own corpus, and + [shape 6's](../layout/shapes/6-kustomize-base-and-overlays/repository/apps/checkout/base/kustomization.yaml) + carries the comment "No namespace: the base is written to be deployable into any of them". +- **Both installers supply one downstream.** Flux's `Kustomization.spec.targetNamespace` and Argo + CD's `Application.spec.destination.namespace` each apply over the built output, so a + namespace-less folder lands wherever the installer says. +- **Nothing in kustomize, Flux or Argo refused the namespace-less root.** What refused it was *our* + render-fidelity gate, comparing a rendered object with no namespace against a live object in + `shop`. That is discussed under [the fidelity rule](#the-fidelity-rule-that-goes-with-it). + +## The options + +Likelihood is how often we expect a real user to want that behavior, given the two things this +operator is used for: mirroring a live cluster so it can be reviewed and re-applied, and publishing +a folder other clusters install. + +| | What the created root carries | Who it serves | Likelihood useful | What it costs | +|---|---|---|---|---| +| **A** | always `namespace: ` | someone mirroring one cluster into a folder only that cluster installs | **Medium.** Real, but it is the case that needs no flag: leave `serializeNamespace` unset and the documents carry their own namespaces | Contradicts the field the user set. They asked us not to serialize the namespace and we serialized it one file up, where it is harder to see | +| **B** | nothing, ever *(decided)* | anyone publishing a folder an installer places: one base, several environments, or a cluster whose namespace is chosen at install time | **High.** It is the standard kustomize base convention, and the only shape that survives being installed twice into two namespaces | The folder stops recording which namespace the objects were mirrored from. Applied with no installer, they land in `default` | +| **C** | it, but only when `serializeNamespace` is unset or `true` | nobody, on inspection | **Low.** When the documents carry their own namespaces a root-level `namespace:` is redundant at best; where it disagrees it silently relocates every document | The worst of both: a transformer nobody asked for, over documents that already answered the question | +| **D** | a namespace from a new explicit field | someone who wants an operator-owned root pinned to a namespace that is not the source namespace | **Low today.** No one has asked, and it is additive later if they do | A third field on a two-field model, and a second way to say something `serializeNamespace` already implies | +| **E** | refuse `serializeNamespace: false` + `useKustomize: true` outright | nobody | **Very low.** It is the one pairing where each half answers the other's question | Deletes the shape the empty-folder bootstrap exists for | + +**The decision is B: a created root never writes `namespace:`.** The rule that falls out of it is +one sentence, and it is the field's own meaning: *the namespace comes from the documents when +`serializeNamespace` is unset or `true`, and from the installer when it is `false`.* Nothing the +operator writes pins it anywhere else. + +A was shipped first and is wrong for one reason worth stating plainly: `serializeNamespace: false` +means *this artifact does not encode its deployment namespace*, and adding a root must not quietly +change that contract. Where the namespace is written is exactly what the user was configuring. + +## The fidelity rule that goes with it + +The render gate compares what the folder renders with the live object it mirrors. Under B the +rendered object has no namespace and the live one does, so the gate has to be told when that +difference is the point rather than a fault. + +**It is scoped by the governing root, not by the target's field alone.** A pre-existing root that +declares `namespace: shop` has a concrete render contract, and a live `billing` object written into +it really is being relocated: relaxing that would hide the exact failure the gate was built for. + +| `spec.serializeNamespace` | The root governing the path | `metadata.namespace` in the comparison | +|---|---|---| +| unset or `true` | any | **checked** | +| `false` | sets `namespace:` | **checked** — the folder makes a concrete claim and must keep it | +| `false` | sets none, or there is no root | **ignored**, and every other field still compared | + +The third row is not a weakening; it is the removal of an inconsistency. A namespace-free flat +folder ([shape 2](../layout/shapes/2-flat-namespace-free/README.md)) is never namespace-checked +today, because the gate only arms when a flush touched a kustomization. The same declaration got a +different answer in a kustomize folder purely because a root file existed. Now both answer the same +way. + +The [one-source-namespace rule](../layout/model.md#the-second-guard-one-source-namespace-and-this-one-refuses) +is untouched by this and keeps its job. It protects **source** identity: two source namespaces +collapsing onto one namespace-less document that two live objects take turns overwriting. It never +tried to constrain the installer's **destination**, which is what this page is about. + +## The other half: a file nobody renders is refused + +`useKustomize: true` declares that the folder is a kustomize folder the operator maintains. Under +that declaration, committing a document that no `resources:` list names is committing a file that +looks mirrored and is applied by nothing. + +It arises in one case, and creating a second root is not the answer: a folder that already has a +render root, with a `byType` or `default` template placing the document outside it. A second root +would make the folder cover two render roots, which is +[`Ambiguous`](../layout/model.md#statusplacement-and-the-post-scan-pass), and an ambiguous folder +stops placing new documents at all. So the existing root is left alone and **the placement is +refused** rather than written unrendered. The fix is the user's: point the template inside the root, +or point the target at the folder the root governs. + +Without `useKustomize` nothing changes. A target that made no claim about kustomize keeps today's +behavior, and this is not a new refusal for folders that never asked for one. + +## Where this is written down + +[`../layout/model.md`](../layout/model.md) is the layout model and stays the specification; this +page is only the argument behind one of its sentences. The behavior a user reads is in +[`../configuration.md`](../configuration.md), and +[shape 5](../layout/shapes/5-kustomize-single-folder/README.md) is the worked example the corpus +executes. diff --git a/docs/layout/model.md b/docs/layout/model.md index dbcfd0d0..b64f3b26 100644 --- a/docs/layout/model.md +++ b/docs/layout/model.md @@ -149,10 +149,13 @@ The two settings exist for the two shapes a user actually declares: - **`true` for a flat folder.** Nothing downstream supplies a namespace, so every namespaced document has to carry one. It also keeps a document portable: it means the same thing pasted anywhere. -- **`false` beside a root that supplies it.** With `useKustomize: true` the operator owns that root - and writes `namespace:` into it, so the omission is **provable** rather than trusted. That is the - difference between establishing a convention and guessing one, and it is what inference - structurally cannot do on an empty folder — there is nothing there to infer from. +- **`false` for a folder whose namespace is chosen where it is installed.** The supplier is a Flux + `Kustomization.spec.targetNamespace`, an Argo `Application.spec.destination.namespace`, or a + `kustomization.yaml` in the folder that a person wrote. `useKustomize: true` does not change that: + a root the OPERATOR creates carries no `namespace:`, because the setting says this artifact does + not encode its deployment namespace and creating a root must not silently re-encode it one file + up. See [`../design/created-root-namespace.md`](../design/created-root-namespace.md), which + reverses an earlier revision of this bullet. The name deliberately avoids `writeNamespace`. "Write" is the most loaded word in this API — the write boundary, the write jail, `WriteBoundaryRefused` — so `writeNamespace: false` invites the diff --git a/docs/layout/shapes/2-flat-namespace-free/README.md b/docs/layout/shapes/2-flat-namespace-free/README.md index a591120a..43f6d894 100644 --- a/docs/layout/shapes/2-flat-namespace-free/README.md +++ b/docs/layout/shapes/2-flat-namespace-free/README.md @@ -75,8 +75,8 @@ copy of the information that would have told the two objects apart. **This is decided, and it is a rule rather than a field: an explicit `serializeNamespace: false` admits exactly one source namespace, and the second is refused.** The argument is in [the shapes README](../README.md#a-namespace-free-folder-needs-a-fence-around-one-namespace) and in -[`model.md`](../../model.md#the-second-guard-one-source-namespace-and-this-one-refuses); it ships -with the field, in PR 2. Unlike the supplier question above, it is answerable entirely inside the +[`model.md`](../../model.md#the-second-guard-one-source-namespace-and-this-one-refuses). Unlike +the supplier question above, it is answerable entirely inside the cluster: the set of source namespaces reaching a target comes from the rules that name it, not from the folder — so this shape gets a real fence even though its *supplier* stays unverifiable. @@ -85,7 +85,7 @@ It is a fixture rather than only an argument. [`config/watchrule-second-namespace.yaml`](config/watchrule-second-namespace.yaml) are this folder with the mistake made, and [`expected-second-namespace-status.yaml`](expected-second-namespace-status.yaml) is the refusal. -The corpus runs it and skips it, naming PR 2. +The corpus runs it. ## Empty folder diff --git a/docs/layout/shapes/2-flat-namespace-free/expected-checkout-config.patch b/docs/layout/shapes/2-flat-namespace-free/expected-checkout-config.patch index 22ab17e4..da02aeca 100644 --- a/docs/layout/shapes/2-flat-namespace-free/expected-checkout-config.patch +++ b/docs/layout/shapes/2-flat-namespace-free/expected-checkout-config.patch @@ -8,4 +8,4 @@ new file mode 100644 +metadata: + name: checkout-config +data: -+ timeout: "15m" ++ timeout: 15m diff --git a/docs/layout/shapes/4-tree-namespace-free/expected-checkout-config.patch b/docs/layout/shapes/4-tree-namespace-free/expected-checkout-config.patch index 508523d3..f2a021c8 100644 --- a/docs/layout/shapes/4-tree-namespace-free/expected-checkout-config.patch +++ b/docs/layout/shapes/4-tree-namespace-free/expected-checkout-config.patch @@ -8,4 +8,4 @@ new file mode 100644 +metadata: + name: checkout-config +data: -+ timeout: "15m" ++ timeout: 15m diff --git a/docs/layout/shapes/5-kustomize-single-folder/README.md b/docs/layout/shapes/5-kustomize-single-folder/README.md index de722d78..d4355934 100644 --- a/docs/layout/shapes/5-kustomize-single-folder/README.md +++ b/docs/layout/shapes/5-kustomize-single-folder/README.md @@ -52,16 +52,21 @@ and the document together: ```text apps/checkout/ - kustomization.yaml # namespace: shop, resources: [checkout-config.yaml] + kustomization.yaml # resources: [checkout-config.yaml], and no namespace: checkout-config.yaml # no metadata.namespace ``` -**This is the one pairing in the model that closes its own loop.** `serializeNamespace: false` is -honest only when something guarantees the namespace, and on an empty folder there is nothing to -inspect — inference structurally cannot answer. `useKustomize: true` supplies the missing half: the -operator writes the supplier, so the omission is provable rather than trusted, and the post-scan -guard is satisfied by construction. It is also what makes the created root **meaningful** rather -than an empty file. +**The created root carries no `namespace:`, and that is the decision rather than an omission.** +`serializeNamespace: false` says this artifact does not encode its deployment namespace, and +creating a root must not quietly change that contract by pinning the namespace one file up, where it +is harder to see and impossible for an installer to override. The folder is a portable artifact: a +Flux `targetNamespace` or an Argo `destination.namespace` places it, exactly as it places +[shape 2](../2-flat-namespace-free/README.md). The full argument, and the four other answers that +were considered, is in +[`../../../design/created-root-namespace.md`](../../../design/created-root-namespace.md). + +What the flag buys here is *structure*: an empty folder becomes a kustomize folder with the first +commit, and every later document joins the same root instead of scattering. Contrast [shape 2](../2-flat-namespace-free/README.md), which is the same omission with the guarantee in another cluster, and [shape 6](../6-kustomize-base-and-overlays/README.md), where diff --git a/docs/layout/shapes/5-kustomize-single-folder/expected-empty-folder-first-write.patch b/docs/layout/shapes/5-kustomize-single-folder/expected-empty-folder-first-write.patch index 3e9101ef..60767d9b 100644 --- a/docs/layout/shapes/5-kustomize-single-folder/expected-empty-folder-first-write.patch +++ b/docs/layout/shapes/5-kustomize-single-folder/expected-empty-folder-first-write.patch @@ -1,13 +1,3 @@ -diff --git a/apps/checkout/kustomization.yaml b/apps/checkout/kustomization.yaml -new file mode 100644 ---- /dev/null -+++ b/apps/checkout/kustomization.yaml -@@ -0,0 +1,5 @@ -+apiVersion: kustomize.config.k8s.io/v1beta1 -+kind: Kustomization -+namespace: shop -+resources: -+ - checkout-config.yaml diff --git a/apps/checkout/checkout-config.yaml b/apps/checkout/checkout-config.yaml new file mode 100644 --- /dev/null @@ -18,4 +8,13 @@ new file mode 100644 +metadata: + name: checkout-config +data: -+ timeout: "15m" ++ timeout: 15m +diff --git a/apps/checkout/kustomization.yaml b/apps/checkout/kustomization.yaml +new file mode 100644 +--- /dev/null ++++ b/apps/checkout/kustomization.yaml +@@ -0,0 +1,4 @@ ++apiVersion: kustomize.config.k8s.io/v1beta1 ++kind: Kustomization ++resources: ++ - checkout-config.yaml diff --git a/docs/layout/shapes/README.md b/docs/layout/shapes/README.md index 0470936d..a5f58e35 100644 --- a/docs/layout/shapes/README.md +++ b/docs/layout/shapes/README.md @@ -1,8 +1,9 @@ # The folder shapes, and the configuration each one needs -> **design**: a specification by example for the layout model proposed in -> [`../model.md`](../model.md). The two booleans shown here — `spec.serializeNamespace` and -> `spec.placement.useKustomize` — do not exist in the current release. +> **design**: a specification by example for the layout model in +> [`../model.md`](../model.md). Both booleans shown here, `spec.serializeNamespace` and +> `spec.placement.useKustomize`, are shipped fields, and every folder below is executed against +> the write path by the layout corpus. > Date: 2026-08-31. > Index: [`../../INDEX.md`](../../INDEX.md) @@ -125,10 +126,13 @@ so every shape that relied on inference has to be *declared* instead. Three findings come out of that column, and they are the substance of this document. -**Shape 5 is the case `useKustomize` was designed for, and it closes its own loop.** An empty folder -plus `useKustomize: true` plus `serializeNamespace: false` is the one combination where the omission -is *provable*: the operator writes the `kustomization.yaml`, puts `namespace: shop` in it, and then -legitimately leaves `metadata.namespace` out of every document it places. Nothing is trusted. +**Shape 5 is the case `useKustomize` was designed for.** An empty folder plus `useKustomize: true` +plus `serializeNamespace: false` produces a kustomize folder from the first commit: the operator +writes the `kustomization.yaml`, adopts whatever is already there into its `resources:`, and leaves +`metadata.namespace` out of every document it places. The created root carries no `namespace:` +either — the artifact does not encode its deployment namespace, and the installer supplies it, the +same way it does for shapes 2 and 4 (see +[`../../design/created-root-namespace.md`](../../design/created-root-namespace.md)). [`5-kustomize-single-folder`](5-kustomize-single-folder/README.md) shows both halves — the same folder adopted and created — and they differ by two lines of spec. @@ -241,7 +245,7 @@ namespace-less document. [Shape 2](2-flat-namespace-free/README.md#what-if-two-n works both through on its own fixtures. **The answer is a rule, not a field: an explicit `serializeNamespace: false` admits exactly one -source namespace, and the second is refused.** It ships with the field in PR 2. The argument — why +source namespace, and the second is refused.** The argument — why no third boolean, why explicit `false` only, why `useKustomize: true` makes it mandatory rather than optional, and where the refusal lives — is in [`model.md`](../model.md#the-second-guard-one-source-namespace-and-this-one-refuses) and is not diff --git a/docs/layout/specific-examples/README.md b/docs/layout/specific-examples/README.md index d9b8a7cc..cf6aa542 100644 --- a/docs/layout/specific-examples/README.md +++ b/docs/layout/specific-examples/README.md @@ -1,8 +1,8 @@ # Specific examples: two ecosystems, and the shared prerequisites > **design**: worked scenarios for the layout model in [`../model.md`](../model.md). The -> `GitTarget` files use `spec.serializeNamespace` and `spec.placement.useKustomize`, neither of -> which exists in the current release. +> `GitTarget` files use `spec.serializeNamespace` and `spec.placement.useKustomize`, and both are +> shipped fields. > Date: 2026-08-31. > Index: [`../../INDEX.md`](../../INDEX.md) diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index 47cb57ce..851f06d3 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -84,7 +84,20 @@ const ( // planned write escaping spec.path (L1), or an in-place edit of a source file more than // one kustomize render root reaches (L2, write-fan-in > 1). Nothing was committed. The // string must stay in sync with the watch package's gitPathRefusalReason. - GitTargetReasonWriteBoundaryRefused = "WriteBoundaryRefused" + GitTargetReasonWriteBoundaryRefused = "WriteBoundaryRefused" + // GitTargetReasonMultipleSourceNamespaces is the terminal reason for a target that declared + // its folder namespace-free (spec.serializeNamespace: false) and that more than one source + // namespace reaches. Two namespaces produce documents whose identities are equal once the + // namespace is stripped, so each write flips one document between two live objects; the writer + // refuses the flush before any byte is written. The remedy is a GitTarget or WatchRule edit. + // The string must stay in sync with manifestanalyzer.GitPathRefusalReason. + GitTargetReasonMultipleSourceNamespaces = "MultipleSourceNamespaces" + // GitTargetReasonUnrenderedPlacement is the terminal reason for a placement under + // spec.placement.useKustomize that no kustomization would render: the folder already has a + // render root and the resolved path is outside it. Nothing is written, because a document in + // Git that nothing applies looks mirrored and is not. The remedy is the target's own template + // or path. The string must stay in sync with manifestanalyzer.GitPathRefusalReason. + GitTargetReasonUnrenderedPlacement = "UnrenderedPlacement" GitTargetReasonRenderMatchesLive = "RenderMatchesLive" GitTargetReasonRenderDoesNotMatchLive = "RenderDoesNotMatchLive" GitTargetReasonRenderRechecking = "Rechecking" diff --git a/internal/controller/gittarget_layout_test.go b/internal/controller/gittarget_layout_test.go index a861de30..94157ab7 100644 --- a/internal/controller/gittarget_layout_test.go +++ b/internal/controller/gittarget_layout_test.go @@ -183,6 +183,7 @@ func TestPublishLayout_AmbiguousMatchesTheCorpusFixture(t *testing.T) { // as a transient — otherwise a target that will never converge reads as one that is still trying. func TestGitTargetReadiness_StalledFollowsGitPathAccepted(t *testing.T) { for _, fixture := range []struct{ dir, file string }{ + {"2-flat-namespace-free", "expected-second-namespace-status.yaml"}, {"6-kustomize-base-and-overlays", "expected-app-root-status.yaml"}, {"8-base-owned-field-edit", "expected-env-change-status.yaml"}, } { diff --git a/internal/controller/stream_status.go b/internal/controller/stream_status.go index 232a5f30..3548d510 100644 --- a/internal/controller/stream_status.go +++ b/internal/controller/stream_status.go @@ -114,6 +114,8 @@ func gitTargetReadyReasonIsStalled(reason string) bool { GitTargetReasonUnsupportedContent, GitTargetReasonIgnoreShadowsManagedPath, GitTargetReasonWriteBoundaryRefused, + GitTargetReasonMultipleSourceNamespaces, + GitTargetReasonUnrenderedPlacement, GitTargetReasonRenderDoesNotMatchLive, GitTargetReadyReasonValidationFailed, GitTargetReadyReasonEncryptionNotConfigured, diff --git a/internal/git/acceptance_gate_test.go b/internal/git/acceptance_gate_test.go index 8a266262..9e3926d9 100644 --- a/internal/git/acceptance_gate_test.go +++ b/internal/git/acceptance_gate_test.go @@ -43,7 +43,15 @@ func TestPlanFlush_RefusesUnsupportedKustomizeFolder(t *testing.T) { w := &BranchWorker{contentWriter: writer} event := cmEvent("CREATE", "fresh", "green") - _, err := w.flushEventsToWorktree(context.Background(), worktree, "", []Event{event}, nil, v1alpha3.PruneOnEvent) + _, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "", + []Event{event}, + nil, + namespacePolicy{}, + v1alpha3.PruneOnEvent, + ) var refused *manifestanalyzer.AcceptanceRefusedError require.ErrorAs(t, err, &refused, "flush must refuse with *AcceptanceRefusedError") @@ -68,7 +76,15 @@ func TestPlanFlush_AcceptsPlainKustomizeFolder(t *testing.T) { w := &BranchWorker{contentWriter: writer} create := []Event{cmEvent("CREATE", "fresh", "green")} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", create, nil, v1alpha3.PruneOnEvent) + changed, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "", + create, + nil, + namespacePolicy{}, + v1alpha3.PruneOnEvent, + ) require.NoError(t, err, "a plain kustomization must not be refused") assert.True(t, changed, "the ConfigMap must be written beside the retained kustomization") } @@ -83,7 +99,15 @@ func TestPlanFlush_DoesNotRefuseOwnSopsConfig(t *testing.T) { w := &BranchWorker{contentWriter: writer} create := []Event{cmEvent("CREATE", "fresh", "green")} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", create, nil, v1alpha3.PruneOnEvent) + changed, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "", + create, + nil, + namespacePolicy{}, + v1alpha3.PruneOnEvent, + ) require.NoError(t, err, ".sops.yaml is the operator's own config and must not be refused") assert.True(t, changed, "the ConfigMap must still be written beside .sops.yaml") } diff --git a/internal/git/commit_executor.go b/internal/git/commit_executor.go index c792f43d..b34128ef 100644 --- a/internal/git/commit_executor.go +++ b/internal/git/commit_executor.go @@ -206,6 +206,7 @@ func (w *BranchWorker) applyPendingWriteEvents( base, byBase[base], placementPolicyForBase(targets, base), + namespacePolicyForBase(targets, base), pruneModeForBase(targets, base), ) if err != nil { diff --git a/internal/git/fieldpatch_flush_test.go b/internal/git/fieldpatch_flush_test.go index 79c0b985..8b5a2433 100644 --- a/internal/git/fieldpatch_flush_test.go +++ b/internal/git/fieldpatch_flush_test.go @@ -55,7 +55,15 @@ func deploymentsMapper() typeset.Lookup { func applyScalePatch(t *testing.T, writer *contentWriter, worktree *gogit.Worktree, events ...Event) bool { t.Helper() w := &BranchWorker{contentWriter: writer, mapper: deploymentsMapper()} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, nil, v1alpha3.PruneOnEvent) + changed, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "", + events, + nil, + namespacePolicy{}, + v1alpha3.PruneOnEvent, + ) require.NoError(t, err) return changed } diff --git a/internal/git/inplace_edit_test.go b/internal/git/inplace_edit_test.go index 7e46ebf5..e95df07d 100644 --- a/internal/git/inplace_edit_test.go +++ b/internal/git/inplace_edit_test.go @@ -54,7 +54,15 @@ func newWorktreeForTest(t *testing.T) *gogit.Worktree { func applyEventsViaPlanFlush(t *testing.T, writer *contentWriter, worktree *gogit.Worktree, events ...Event) bool { t.Helper() w := &BranchWorker{contentWriter: writer} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, nil, v1alpha3.PruneOnEvent) + changed, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "", + events, + nil, + namespacePolicy{}, + v1alpha3.PruneOnEvent, + ) require.NoError(t, err) return changed } @@ -68,7 +76,15 @@ func applyEventsViaPlanFlushWithMapper( ) bool { t.Helper() w := &BranchWorker{contentWriter: writer, mapper: mapper} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, nil, v1alpha3.PruneOnEvent) + changed, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "", + events, + nil, + namespacePolicy{}, + v1alpha3.PruneOnEvent, + ) require.NoError(t, err) return changed } diff --git a/internal/git/inplace_overrides_test.go b/internal/git/inplace_overrides_test.go index 4a804bf3..241b09c1 100644 --- a/internal/git/inplace_overrides_test.go +++ b/internal/git/inplace_overrides_test.go @@ -210,7 +210,8 @@ func TestApplyOverrideEdits_SkipLeavesBuffersUntouched(t *testing.T) { scan := manifestanalyzer.FolderScan{YAMLFiles: []manifestedit.FileContent{ {Path: "kustomization.yaml", Content: []byte(kust)}, }} - wb := newWriteBatch(context.Background(), newContentWriter(types.SensitiveResourcePolicy{}), nil, scan, nil, "") + wb := newWriteBatch( + context.Background(), newContentWriter(types.SensitiveResourcePolicy{}), nil, scan, nil, namespacePolicy{}, "") fieldMissing := manifestanalyzer.OverrideEdit{ KustomizationPath: "kustomization.yaml", diff --git a/internal/git/kustomization_bootstrap.go b/internal/git/kustomization_bootstrap.go new file mode 100644 index 00000000..136d3237 --- /dev/null +++ b/internal/git/kustomization_bootstrap.go @@ -0,0 +1,172 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "fmt" + "path" + "sort" + "strings" + + "sigs.k8s.io/controller-runtime/pkg/log" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" +) + +// createdKustomizationHeader is the root the operator writes when spec.placement.useKustomize asks +// for one. It is deliberately the smallest thing kustomize will build: an apiVersion, a kind, the +// namespace when there is one to write, and the resources: list the new document joins. +const ( + createdKustomizationAPIVersion = "kustomize.config.k8s.io/v1beta1" + createdKustomizationKind = "Kustomization" +) + +// bootstrapKustomization creates the folder's kustomization.yaml when nothing governs the path a +// new document is landing at and the GitTarget asked for one. It returns the root the caller must +// register the document in, or nil when this write needs no such root. +// +// This is the only place the operator writes a file nobody asked for by name, which is why it is +// narrow on every axis: +// +// - Only when spec.placement.useKustomize is true. Registering into a root that already exists is +// an invariant and happens regardless (#319); this is exclusively the empty case. +// - Only when NO kustomization governs the resolved path. The walk is bounded by the write jail, +// so a root above spec.path is a read-only ancestor and its absence from the answer is not a +// licence to write a competing root inside the jail. +// - Only when the folder has no render root AT ALL (LayoutNone). A folder that already has one, +// with this document landing outside it, must not gain a second: two render roots is +// Ambiguous, and the target would stop placing new documents entirely. That placement is +// REFUSED instead of written unrendered — see unrenderedPlacementRefusal. +// - Only once per batch. The second new document in the same flush joins the root the first one +// created, through the ordinary resources: append. +// +// The root it writes ADOPTS the folder, rather than listing only the document that triggered it. +// Enabling kustomize on a folder that already holds manifests and then writing a root that names +// one file would make every other file stop rendering the moment a consumer ran kustomize build: +// the files would still be in Git, and nothing would apply them. See adoptableEntries. +// +// The created root carries NO namespace:, ever. spec.serializeNamespace: false means the artifact +// does not encode its deployment namespace, and creating a root must not quietly change that +// contract by pinning the namespace one file up, where it is harder to see and impossible to +// override. The namespace comes from the documents when serializeNamespace is unset or true, and +// from the installer — a Flux Kustomization's targetNamespace, an Argo Application's +// destination.namespace — when it is false. See docs/design/created-root-namespace.md. +func (wb *writeBatch) bootstrapKustomization( + ctx context.Context, + placement manifestanalyzer.PlacementResult, +) *manifestanalyzer.KustomizationInfo { + if wb.policy == nil || !wb.policy.UseKustomize || placement.Kustomization != nil { + return nil + } + if wb.createdRoot != nil { + return wb.createdRoot + } + if manifestanalyzer.GoverningKustomization(wb.store, wb.writeSubdir, placement.Path) != nil { + // A root governs the path and simply already lists it, which is the case + // PlacementResult.Kustomization cannot be told apart from "no root at all". + return nil + } + if wb.layout.Reason != manifestanalyzer.LayoutNone { + // The folder HAS a render root; this document just landed somewhere that root does not + // govern, which a declared template can do. Writing a second root beside the first would + // make the folder cover two render roots — Ambiguous — and the target would stop placing + // new documents at all. One unregistered file is a smaller fault than a folder that has + // stopped accepting writes, and the ancestor walk (#319) already registers everything the + // existing root does govern. + return nil + } + + rootPath := path.Join(orRootDir(wb.writeSubdir), "kustomization.yaml") + buf := wb.buffer(rootPath) + if buf.current != nil { + // Something is already at the path we would write. Leave it alone: overwriting a file the + // scan did not model as a kustomization would destroy content on a guess. + return nil + } + entries := wb.adoptableEntries(rootPath, placement.Path) + buf.current = []byte(renderCreatedKustomization(entries)) + + created := &manifestanalyzer.KustomizationInfo{Path: rootPath, Resources: entries} + wb.createdRoot = created + for range entries { + recordKustomizationEntry(ctx, wb.target, kustomizationEntryAdded) + } + log.FromContext(ctx).Info("Created kustomization.yaml for a folder that had none", + "kustomization", rootPath, "entries", entries, "adopted", len(entries)-1) + // The document is registered in the bytes just written, so the caller must not append it + // again; returning nil says "no further registration needed" for this first document. + return nil +} + +// adoptableEntries is the created root's resources: list — the document being placed, plus every +// managed document already in the folder that no kustomization governs. +// +// The adoption is the point. A root that listed only the new document would silently unrender every +// file already in the folder: they stay in Git, they look mirrored, and the moment a consumer runs +// kustomize build against the folder they are gone from the output. Turning a folder into a +// kustomize folder means the folder, not the one document that happened to arrive first. +// +// A file some OTHER kustomization already governs is left out. Listing it here would render it +// twice, once through each root, which kustomize reports as a duplicate resource and refuses to +// build. The caller only creates a root for a folder with no render root at all, so this is +// defensive rather than routine — a kustomization inside the jail that is not a render root of it +// (a base something above spec.path reads) is the shape that reaches it. +// +// Paths are relative to the created root and sorted, so the file reads in a stable order and two +// runs of the same flush produce the same bytes. +func (wb *writeBatch) adoptableEntries(rootPath, documentPath string) []string { + entries := []string{kustomizationEntryFor(rootPath, documentPath)} + for filePath := range wb.store.FilesByPath { + if filePath == documentPath || !pathWithinWriteScope(wb.writeSubdir, filePath) { + continue + } + if manifestanalyzer.GoverningKustomization(wb.store, wb.writeSubdir, filePath) != nil { + continue + } + entries = append(entries, kustomizationEntryFor(rootPath, filePath)) + } + sort.Strings(entries) + return entries +} + +// pathWithinWriteScope reports whether a scanned path is inside the write jail. With no jail the +// whole scanned subtree is the target's own. +func pathWithinWriteScope(writeSubdir, filePath string) bool { + if writeSubdir == "" { + return true + } + return strings.HasPrefix(filePath, writeSubdir+"/") +} + +// renderCreatedKustomization is the created root's bytes: an apiVersion, a kind, and the resources: +// list. Nothing else — no namespace:, and no stanza for a field the operator does not set. They are +// assembled as text rather than marshalled from a struct so the file reads the way a person would +// have written it: block sequence, two-space indent. +func renderCreatedKustomization(entries []string) string { + var b strings.Builder + fmt.Fprintf(&b, "apiVersion: %s\nkind: %s\n", createdKustomizationAPIVersion, createdKustomizationKind) + b.WriteString("resources:\n") + for _, entry := range entries { + fmt.Fprintf(&b, " - %s\n", entry) + } + return b.String() +} + +// kustomizationEntryFor expresses a document's path relative to the kustomization that lists it, +// which is how kustomize reads a resources: entry. +func kustomizationEntryFor(rootPath, documentPath string) string { + dir := path.Dir(rootPath) + if dir == "." { + return documentPath + } + return strings.TrimPrefix(documentPath, dir+"/") +} + +// orRootDir reads the write jail as a directory: an empty jail is the scan root itself. +func orRootDir(writeSubdir string) string { + if writeSubdir == "" { + return "." + } + return writeSubdir +} diff --git a/internal/git/kustomization_bootstrap_test.go b/internal/git/kustomization_bootstrap_test.go new file mode 100644 index 00000000..0ccd4a01 --- /dev/null +++ b/internal/git/kustomization_bootstrap_test.go @@ -0,0 +1,175 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "strings" + "testing" + + gogit "github.com/go-git/go-git/v6" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// spec.placement.useKustomize is the only thing in this operator that writes a file nobody asked +// for by name, so what it does NOT do matters as much as what it does. The corpus +// (docs/layout/shapes/5-kustomize-single-folder) pins the bytes of the one commit it produces; +// these pin its boundaries. + +func useKustomizePolicy() *manifestanalyzer.PlacementPolicy { + return &manifestanalyzer.PlacementPolicy{UseKustomize: true} +} + +func flushWithPlacement( + t *testing.T, + worktree *gogit.Worktree, + policy *manifestanalyzer.PlacementPolicy, + namespaces namespacePolicy, + events ...Event, +) error { + t.Helper() + w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: namespaceProbeMapper()} + _, err := w.flushEventsToWorktree(t.Context(), worktree, "", events, policy, namespaces, v1alpha3.PruneOnEvent) + return err +} + +func TestUseKustomize_CreatesTheRootAndRegistersTheDocument(t *testing.T) { + worktree := newWorktreeForTest(t) + + require.NoError(t, flushWithPlacement(t, worktree, useKustomizePolicy(), + serializeNamespacePolicy(false, "shop"), + namespaceProbeEvent("shop", "checkout-config", "green"))) + + root := readWorktreeFile(t, worktree, "kustomization.yaml") + assert.Equal(t, "apiVersion: kustomize.config.k8s.io/v1beta1\nkind: Kustomization\n"+ + "resources:\n - checkout-config.yaml\n", root, + "an apiVersion, a kind and the resources: list — and no namespace:, ever") + assert.NotContains(t, readWorktreeFile(t, worktree, "checkout-config.yaml"), "namespace:", + "serializeNamespace: false, so the installer supplies the namespace and the folder encodes none") +} + +// The second document in the same flush joins the root the first one created. The store was built +// before the batch, so nothing in it knows that file exists. +func TestUseKustomize_SecondDocumentJoinsTheRootTheFirstCreated(t *testing.T) { + worktree := newWorktreeForTest(t) + + require.NoError(t, flushWithPlacement(t, worktree, useKustomizePolicy(), + serializeNamespacePolicy(false, "shop"), + namespaceProbeEvent("shop", "checkout-config", "green"), + namespaceProbeEvent("shop", "checkout-flags", "blue"))) + + root := readWorktreeFile(t, worktree, "kustomization.yaml") + assert.Contains(t, root, "- checkout-config.yaml") + assert.Contains(t, root, "- checkout-flags.yaml") + assert.Equal(t, 1, strings.Count(root, "kind: Kustomization"), "one root, not one per document") +} + +// The flag's ONE job is the empty case. A folder that already has a root is registered into +// either way, which is #319's invariant and not this setting. +func TestUseKustomize_LeavesAnExistingRootAlone(t *testing.T) { + worktree := newWorktreeForTest(t) + existing := "apiVersion: kustomize.config.k8s.io/v1beta1\nkind: Kustomization\n" + + "namespace: shop\nresources:\n - web.yaml\n" + seedFile(t, worktree.Filesystem().Root(), "kustomization.yaml", existing) + seedFile(t, worktree.Filesystem().Root(), "web.yaml", + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: web\n") + + require.NoError(t, flushWithPlacement(t, worktree, useKustomizePolicy(), + serializeNamespacePolicy(false, "shop"), + namespaceProbeEvent("shop", "checkout-config", "green"))) + + root := readWorktreeFile(t, worktree, "kustomization.yaml") + assert.Contains(t, root, "- web.yaml", "the root the user wrote keeps its own entries") + assert.Contains(t, root, "- checkout-config.yaml") + assert.Equal(t, 1, strings.Count(root, "kind: Kustomization")) +} + +// The finding this test exists for: enabling kustomize on a folder that ALREADY holds manifests +// and then writing a root that lists only the new document would unrender every other file. They +// stay in Git, they look mirrored, and the first `kustomize build` drops them from the output. +// +// So the created root adopts the folder. Nothing is rewritten, moved or re-encoded: the existing +// files are named in resources: exactly where they already are. +func TestUseKustomize_CreatedRootAdoptsTheFilesAlreadyInTheFolder(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem().Root() + seedFile(t, root, "web.yaml", + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: web\n namespace: shop\n") + seedFile(t, root, "configmaps/cache.yaml", + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: cache\n namespace: shop\n") + + require.NoError(t, flushWithPlacement(t, worktree, useKustomizePolicy(), + serializeNamespacePolicy(false, "shop"), + namespaceProbeEvent("shop", "checkout-config", "green"))) + + assert.Equal(t, "apiVersion: kustomize.config.k8s.io/v1beta1\nkind: Kustomization\n"+ + "resources:\n"+ + " - checkout-config.yaml\n - configmaps/cache.yaml\n - web.yaml\n", + readWorktreeFile(t, worktree, "kustomization.yaml"), + "every managed document in the folder is listed, at the path it already lives at") + assert.Contains(t, readWorktreeFile(t, worktree, "web.yaml"), "namespace: shop", + "adopting a file names it in resources:; it does not rewrite its bytes") +} + +// A folder that already has a render root never gains a SECOND one, even when a declared template +// puts the new document outside it: two render roots is an Ambiguous folder, and an ambiguous +// folder stops accepting new documents altogether. +// +// What is left is to write the document unrendered or to refuse, and under useKustomize the target +// has declared this folder is a kustomize folder it maintains. A file no resources: list names is +// one that sits in Git looking mirrored while nothing applies it, so the placement is REFUSED and +// the user is told which root does not reach it. +func TestUseKustomize_RefusesAPlacementNoRootWouldRender(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem().Root() + seedFile(t, root, "media/kustomization.yaml", + "apiVersion: kustomize.config.k8s.io/v1beta1\nkind: Kustomization\n"+ + "namespace: shop\nresources:\n - web.yaml\n") + seedFile(t, root, "media/web.yaml", + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: web\n") + policy := &manifestanalyzer.PlacementPolicy{Default: "flags/{name}.yaml", UseKustomize: true} + + err := flushWithPlacement(t, worktree, policy, namespacePolicy{}, + namespaceProbeEvent("shop", "checkout-config", "green")) + + require.Error(t, err, "a document nothing renders is refused, not committed") + assert.Contains(t, err.Error(), "governed by no kustomization") + assert.NoFileExists(t, root+"/kustomization.yaml", + "a second render root would make the folder ambiguous and stop every later placement") + assert.NoFileExists(t, root+"/flags/checkout-config.yaml", + "and nothing is written: a file in Git that no resources: list names looks mirrored and is not") + assert.Contains(t, readWorktreeFile(t, worktree, "media/kustomization.yaml"), "- web.yaml", + "the root that was already there is untouched") +} + +// Without the flag nothing is created, and the document lands at the canonical path. That is the +// default and it is the whole difference between adopting a kustomize folder and creating one. +func TestUseKustomize_UnsetWritesNoRoot(t *testing.T) { + worktree := newWorktreeForTest(t) + + require.NoError(t, flushWithPlacement(t, worktree, nil, namespacePolicy{}, + namespaceProbeEvent("shop", "checkout-config", "green"))) + + assert.NoFileExists(t, worktree.Filesystem().Root()+"/kustomization.yaml") + assert.FileExists(t, worktree.Filesystem().Root()+"/shop/configmaps/checkout-config.yaml") +} + +// A declared template still decides the path; useKustomize only decides whether the folder gets a +// root. The created root lists the document wherever the template put it, which is what makes a +// subdirectory template renderable at all. +func TestUseKustomize_RegistersADeclaredSubdirectoryPath(t *testing.T) { + worktree := newWorktreeForTest(t) + policy := &manifestanalyzer.PlacementPolicy{Default: "configmaps/{name}.yaml", UseKustomize: true} + + require.NoError(t, flushWithPlacement(t, worktree, policy, + serializeNamespacePolicy(false, "shop"), + namespaceProbeEvent("shop", "checkout-config", "green"))) + + assert.Contains(t, readWorktreeFile(t, worktree, "kustomization.yaml"), + "- configmaps/checkout-config.yaml") + assert.FileExists(t, worktree.Filesystem().Root()+"/configmaps/checkout-config.yaml") +} diff --git a/internal/git/kustomize_delete_test.go b/internal/git/kustomize_delete_test.go index 4c6fdf3c..f1dc9c10 100644 --- a/internal/git/kustomize_delete_test.go +++ b/internal/git/kustomize_delete_test.go @@ -162,7 +162,7 @@ func TestPlanFlush_DeleteInsideARenderRootIsVerified(t *testing.T) { scan, err := scanWorktreeSubtree(root) require.NoError(t, err) - batch := newWriteBatch(context.Background(), writer, configMapMapper(), scan, nil, "") + batch := newWriteBatch(context.Background(), writer, configMapMapper(), scan, nil, namespacePolicy{}, "") batch.applyDelete(context.Background(), deleteConfigMapEvent("delete-me")) assert.True(t, batch.putToKustomize, diff --git a/internal/git/kustomize_oracle_test.go b/internal/git/kustomize_oracle_test.go index c8643c03..93493c34 100644 --- a/internal/git/kustomize_oracle_test.go +++ b/internal/git/kustomize_oracle_test.go @@ -33,7 +33,15 @@ func flushEventsForTest( ) (bool, error) { t.Helper() w := &BranchWorker{contentWriter: writer, mapper: mapper} - return w.flushEventsToWorktree(context.Background(), worktree, "", events, nil, v1alpha3.PruneOnEvent) + return w.flushEventsToWorktree( + context.Background(), + worktree, + "", + events, + nil, + namespacePolicy{}, + v1alpha3.PruneOnEvent, + ) } // Before a kustomize-governed write is committed, the repository is re-rendered WITH it diff --git a/internal/git/layout_corpus_test.go b/internal/git/layout_corpus_test.go index 5820ea40..bb7c5207 100644 --- a/internal/git/layout_corpus_test.go +++ b/internal/git/layout_corpus_test.go @@ -41,11 +41,10 @@ import ( // finished when every skip naming PR 2 is gone. Not every skip is PR 2's — shape 8's // `images:` authoring belongs to track C and outlives it — so the rule is deliberately "its // own skips" rather than "the last skip". -// - `config/gittarget.yaml` parses into corpusGitTarget below, a HARNESS-LOCAL struct, -// for exactly as long as it names fields the API does not have. Every field it holds -// that v1alpha3.GitTargetSpec also holds is asserted against the real type by -// TestLayoutCorpus_ConfigParsesAgainstTheRealAPI, so the examples cannot quietly -// describe an API nobody built. +// - `config/gittarget.yaml` decodes into v1alpha3.GitTarget, the REAL type. It parsed into a +// harness-local struct for as long as the examples named fields the API did not have, and +// deleting that struct was PR 2's own definition of done: the worked examples and the shipped +// API are now the same API, and a field either exists or the corpus stops decoding. // - Refusals are fixtures too. A scenario set where every write succeeds is // advertising rather than specification, so the refusal halves assert an // `expected-status.yaml` instead of a patch. @@ -60,47 +59,11 @@ var updateLayoutCorpus = flag.Bool("update", false, // illustrates, and the drift would be invisible in review. const layoutCorpusRoot = layoutfixture.Root -// corpusGitTarget is the harness-local reading of a scenario's config/gittarget.yaml. -// -// It is deliberately NOT v1alpha3.GitTarget. The examples use spec.serializeNamespace -// and spec.placement.useKustomize, which PR 2 introduces, so decoding into the real type -// would either fail or silently drop them. The mapping is temporary by design and PR 2 -// deletes it — the pointer-typed booleans below are the fields that keep it alive, and -// when they move to the real spec this struct has nothing left to hold. -type corpusGitTarget struct { - Metadata struct { - Name string `json:"name"` - Namespace string `json:"namespace"` - } `json:"metadata"` - Spec struct { - Path string `json:"path"` - Branch string `json:"branch"` - // SerializeNamespace is PR 2's spec.serializeNamespace: unset means infer. - SerializeNamespace *bool `json:"serializeNamespace"` - Placement *struct { - ByType map[string]string `json:"byType"` - Default string `json:"default"` - // UseKustomize is PR 2's placement.useKustomize. - UseKustomize *bool `json:"useKustomize"` - } `json:"placement"` - } `json:"spec"` -} - -// policy projects the parsed config onto the flush policy the write path actually takes -// today. Only the two shipped rungs — byType and default — cross over; the two booleans -// have no consumer until PR 2, which is precisely why the scenarios that depend on them -// are skipped rather than asserted. -func (c corpusGitTarget) policy() *manifestanalyzer.PlacementPolicy { - if c.Spec.Placement == nil { - return nil - } - if len(c.Spec.Placement.ByType) == 0 && c.Spec.Placement.Default == "" { - return nil - } - return &manifestanalyzer.PlacementPolicy{ - ByType: c.Spec.Placement.ByType, - Default: c.Spec.Placement.Default, - } +// corpusNamespaces projects a scenario onto the namespace policy the write path takes: the +// target's own spec.serializeNamespace, and the source namespaces the fixture's WatchRules bring +// to it. +func corpusNamespaces(target v1alpha3.GitTarget, sources []string, wildcard bool) namespacePolicy { + return namespacePolicyFor(target.Spec, sources, wildcard) } // corpusScenario is one executable row of the corpus: a fixture folder, which config in @@ -129,6 +92,11 @@ type corpusScenario struct { // TestGitTargetReadiness_StalledFollowsGitPathAccepted), so the whole file is covered even // though no single test covers all of it. status string + // emptyRepository seeds NOTHING, for a scenario whose whole subject is a folder that does not + // exist yet. It is a flag rather than an empty `repository-empty/` fixture directory because + // Git cannot hold an empty directory, and a .gitkeep inside one would show up in the diff the + // scenario asserts. + emptyRepository bool // skip names the PR that unskips this scenario, and is the whole reason the row is // written before the behavior exists. An empty skip is a scenario that runs today. skip string @@ -169,8 +137,6 @@ func layoutCorpus() []corpusScenario { dir: "shapes/2-flat-namespace-free", input: "checkout-config.yaml", patch: "expected-checkout-config.patch", - skip: "PR 2: needs spec.serializeNamespace: false. Today inference writes " + - "metadata.namespace because no kustomization in the folder supplies it", }, { // The fence around "one namespace": an explicit serializeNamespace: false admits @@ -180,8 +146,6 @@ func layoutCorpus() []corpusScenario { config: "gittarget-second-namespace.yaml", input: "checkout-config.yaml", status: "expected-second-namespace-status.yaml", - skip: "PR 2: the one-source-namespace refusal ships with spec.serializeNamespace " + - "(the write-plan precondition first, then the WatchRule admission check)", }, { dir: "shapes/3-tree-serialized", @@ -192,8 +156,6 @@ func layoutCorpus() []corpusScenario { dir: "shapes/4-tree-namespace-free", input: "checkout-config.yaml", patch: "expected-checkout-config.patch", - skip: "PR 2: needs spec.serializeNamespace: false. Today inference writes " + - "metadata.namespace because no kustomization in the folder supplies it", }, { dir: "shapes/5-kustomize-single-folder", @@ -201,12 +163,14 @@ func layoutCorpus() []corpusScenario { patch: "expected-checkout-config.patch", }, { - dir: "shapes/5-kustomize-single-folder", - config: "gittarget-empty-folder.yaml", - input: "checkout-config.yaml", - patch: "expected-empty-folder-first-write.patch", - skip: "PR 2: needs placement.useKustomize to create the kustomization.yaml this " + - "empty folder has none of", + dir: "shapes/5-kustomize-single-folder", + // The same folder before it exists. `repository/` is shape 5 ALREADY adopted, which is + // the other scenario in this folder; this one seeds nothing, because "there is nothing + // to infer from" is the whole subject. + config: "gittarget-empty-folder.yaml", + input: "checkout-config.yaml", + patch: "expected-empty-folder-first-write.patch", + emptyRepository: true, }, { dir: "shapes/6-kustomize-base-and-overlays", @@ -279,13 +243,16 @@ func runCorpusScenario(t *testing.T, sc corpusScenario) { target := readCorpusGitTarget(t, filepath.Join(folder, "config", sc.configFile())) obj := readCorpusInput(t, filepath.Join(folder, "input", sc.input)) - worktree, seeded := seedCorpusWorktree(t, filepath.Join(folder, "repository")) + worktree, seeded := seedCorpusWorktree(t, folder, sc) event := corpusEvent(t, obj, target) + sources, wildcard := readCorpusSourceNamespaces(t, folder, sc.configFile(), target) + worker := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: corpusMapper()} _, err := worker.flushEventsToWorktree( t.Context(), worktree, sanitizePath(target.Spec.Path), - []Event{event}, target.policy(), v1alpha3.PruneOnEvent) + []Event{event}, resolvePlacementPolicy(target.Spec.Placement), + corpusNamespaces(target, sources, wildcard), v1alpha3.PruneOnEvent) if sc.patch == "" { requireCorpusRefusal(t, err, filepath.Join(folder, sc.status), worktree, seeded) @@ -340,17 +307,69 @@ func assertCorpusPatch(t *testing.T, path, got string) { "the write path and %s disagree; re-run with -update if the new diff is the intended one", path) } -// readCorpusGitTarget decodes one scenario's GitTarget through the harness-local struct. -func readCorpusGitTarget(t *testing.T, path string) corpusGitTarget { +// readCorpusGitTarget decodes one scenario's GitTarget. It is the SHIPPED type: a config naming a +// field the API does not have fails here, which is the check the harness-local struct used to buy +// with a whole second parser and an assertion test beside it. +func readCorpusGitTarget(t *testing.T, path string) v1alpha3.GitTarget { t.Helper() raw, err := os.ReadFile(path) require.NoError(t, err) - var target corpusGitTarget - require.NoError(t, yaml.Unmarshal(raw, &target), "parsing %s", path) + var target v1alpha3.GitTarget + require.NoError(t, yaml.UnmarshalStrict(raw, &target), "parsing %s", path) require.NotEmpty(t, target.Spec.Path, "%s: spec.path is what the corpus writes into", path) return target } +// readCorpusSourceNamespaces derives the source namespaces a scenario's target is reached by, from +// the WatchRules the fixture folder holds — the same set resolveSourceNamespaces computes from the +// cluster: the target's own namespace plus every explicit rules[].sourceNamespace. +// +// Which rules belong to a scenario is decided by the config's own name, because a folder can hold +// two configs that differ only in the rules pointing at them: shape 2's gittarget.yaml and +// gittarget-second-namespace.yaml are the same target with the same flag, and the ONLY difference +// between the write and the refusal is that a second WatchRule exists. So a config +// `gittarget[-].yaml` is served by `watchrule.yaml` plus `watchrule-.yaml`, +// whichever of the two the folder has, and every rule that matches must name the target — a +// fixture whose rules point somewhere else is a fixture that is not saying what it looks like it +// says. +func readCorpusSourceNamespaces( + t *testing.T, + folder, configFile string, + target v1alpha3.GitTarget, +) ([]string, bool) { + t.Helper() + variant := strings.TrimSuffix(strings.TrimPrefix(configFile, "gittarget"), ".yaml") + + seen := map[string]struct{}{target.Namespace: {}} + wildcard := false + for _, name := range []string{"watchrule.yaml", "watchrule" + variant + ".yaml"} { + path := filepath.Join(folder, "config", name) + raw, err := os.ReadFile(path) + if os.IsNotExist(err) { + continue + } + require.NoError(t, err) + var rule v1alpha3.WatchRule + require.NoError(t, yaml.Unmarshal(raw, &rule), "parsing %s", path) + require.Equal(t, target.Name, rule.Spec.TargetRef.Name, + "%s points at a different GitTarget than the scenario's config", path) + for _, item := range rule.Spec.Rules { + if item.IsSourceNamespaceWildcard() { + wildcard = true + continue + } + seen[item.EffectiveSourceNamespace(rule.Namespace)] = struct{}{} + } + } + + namespaces := make([]string, 0, len(seen)) + for ns := range seen { + namespaces = append(namespaces, ns) + } + sort.Strings(namespaces) + return namespaces, wildcard +} + // readCorpusInput decodes the live object a scenario receives. It is deliberately the // object as the API server serves it — uid, resourceVersion, managedFields and all — // because the difference between it and the expected patch IS the sanitization assertion. @@ -364,7 +383,7 @@ func readCorpusInput(t *testing.T, path string) *unstructured.Unstructured { } // corpusEvent builds the write event for a scenario's live object. -func corpusEvent(t *testing.T, obj *unstructured.Unstructured, target corpusGitTarget) Event { +func corpusEvent(t *testing.T, obj *unstructured.Unstructured, target v1alpha3.GitTarget) Event { t.Helper() gvk := obj.GroupVersionKind() entry, ok := corpusTypes[gvk] @@ -375,8 +394,8 @@ func corpusEvent(t *testing.T, obj *unstructured.Unstructured, target corpusGitT gvk.Group, gvk.Version, entry.Resource, obj.GetNamespace(), obj.GetName()), Operation: "CREATE", Path: target.Spec.Path, - GitTargetName: target.Metadata.Name, - GitTargetNamespace: target.Metadata.Namespace, + GitTargetName: target.Name, + GitTargetNamespace: target.Namespace, } } @@ -386,10 +405,14 @@ func corpusEvent(t *testing.T, obj *unstructured.Unstructured, target corpusGitT // `repository/` is always rooted at the REPOSITORY root, never at spec.path, so a fixture // shows where the target sits as well as what it holds. Shapes 6 to 8 depend on that: their // target is a leaf overlay whose base lives outside spec.path but inside the render scope. -func seedCorpusWorktree(t *testing.T, repositoryDir string) (*gogit.Worktree, *object.Commit) { +func seedCorpusWorktree(t *testing.T, folder string, sc corpusScenario) (*gogit.Worktree, *object.Commit) { t.Helper() worktree := newWorktreeForTest(t) root := worktree.Filesystem().Root() + if sc.emptyRepository { + return worktree, commitCorpusWorktree(t, worktree, "seed an empty repository") + } + repositoryDir := filepath.Join(folder, "repository") require.NoError(t, filepath.WalkDir(repositoryDir, func(path string, entry os.DirEntry, err error) error { if err != nil || entry.IsDir() { @@ -499,73 +522,6 @@ func corpusMapper() typeset.Lookup { return typeset.NewSnapshotRegistry(typeset.Snapshot{Entries: entries}) } -// TestLayoutCorpus_ConfigParsesAgainstTheRealAPI is the check the harness-local struct -// buys. corpusGitTarget exists because the examples name fields the API does not have -// yet; the risk that creates is the opposite one — a field the examples and the API BOTH -// have, spelled differently — so every shipped field a scenario config sets is decoded -// into the real v1alpha3.GitTarget too, and must survive the round trip. -// -// PR 2 deletes corpusGitTarget and this test with it: once serializeNamespace and -// useKustomize are real fields, the scenarios decode into v1alpha3.GitTarget directly and -// there is nothing left to keep honest. -func TestLayoutCorpus_ConfigParsesAgainstTheRealAPI(t *testing.T) { - for _, sc := range layoutCorpus() { - t.Run(sc.name(), func(t *testing.T) { - path := filepath.Join(layoutCorpusRoot, sc.dir, "config", sc.configFile()) - harness := readCorpusGitTarget(t, path) - - raw, err := os.ReadFile(path) - require.NoError(t, err) - // The unbuilt fields are stripped, not tolerated: decoding them into the real - // type is exactly the thing that must start working in PR 2, and letting the - // decoder ignore them here would hide the day it does. - var shipped v1alpha3.GitTarget - require.NoError(t, yaml.Unmarshal(withoutUnbuiltFields(t, raw), &shipped), "parsing %s", path) - - require.Equal(t, harness.Spec.Path, shipped.Spec.Path, "spec.path") - require.Equal(t, harness.Metadata.Name, shipped.Name, "metadata.name") - require.Equal(t, harness.Metadata.Namespace, shipped.Namespace, "metadata.namespace") - require.Equal(t, harness.Spec.Branch, shipped.Spec.Branch, "spec.branch") - if policy := harness.policy(); policy != nil { - require.NotNil(t, shipped.Spec.Placement, "spec.placement") - require.Equal(t, policy.ByType, shipped.Spec.Placement.ByType, "spec.placement.byType") - require.Equal(t, policy.Default, shipped.Spec.Placement.Default, "spec.placement.default") - } - }) - } -} - -// withoutUnbuiltFields removes the fields PR 2 introduces from a scenario config, so what -// is left is the API as it stands today. `suspend:` was on this list and is not any more: it -// ships in PR 1, and no worked example sets it — an example exists to show what gets WRITTEN, -// and previewing that is a scratch branch rather than a suspended target. It is a line filter rather than a re-marshal -// because the configs are commented documents and the comments are half of what they say. -func withoutUnbuiltFields(t *testing.T, raw []byte) []byte { - t.Helper() - unbuilt := []string{"serializeNamespace:", "useKustomize:"} - var kept []string - for _, line := range strings.Split(string(raw), "\n") { - trimmed := strings.TrimSpace(line) - if strings.HasPrefix(trimmed, "#") { - continue - } - if slicesContainsPrefix(trimmed, unbuilt) { - continue - } - kept = append(kept, line) - } - return []byte(strings.Join(kept, "\n")) -} - -func slicesContainsPrefix(s string, prefixes []string) bool { - for _, p := range prefixes { - if strings.HasPrefix(s, p) { - return true - } - } - return false -} - // TestLayoutCorpus_EveryFixtureFolderIsExecuted closes the corpus over the filesystem: a // scenario folder that nothing in layoutCorpus() names is a fixture nobody runs, which is // the state this whole file exists to leave behind. diff --git a/internal/git/namespace_context_refusal_test.go b/internal/git/namespace_context_refusal_test.go index e45515ed..816f62fa 100644 --- a/internal/git/namespace_context_refusal_test.go +++ b/internal/git/namespace_context_refusal_test.go @@ -83,7 +83,7 @@ func TestPlanFlush_RefusesWhenTransformerOverridesExplicitNamespace(t *testing.T worker := &BranchWorker{contentWriter: writer, mapper: namespaceProbeMapper()} changed, err := worker.flushEventsToWorktree( t.Context(), worktree, "", - []Event{namespaceProbeEvent("beta", "cm", "green")}, nil, v1alpha3.PruneOnEvent) + []Event{namespaceProbeEvent("beta", "cm", "green")}, nil, namespacePolicy{}, v1alpha3.PruneOnEvent) require.Error(t, err, "the folder renders alpha/cm while the mirror holds beta/cm; the write must refuse") assert.Contains(t, err.Error(), "does not render to the live object") @@ -117,7 +117,7 @@ func TestPlanFlush_RefusesWhenNestedRootsBothSetNamespace(t *testing.T) { worker := &BranchWorker{contentWriter: writer, mapper: namespaceProbeMapper()} changed, err := worker.flushEventsToWorktree( t.Context(), worktree, "", - []Event{namespaceProbeEvent("outer", "m", "green")}, nil, v1alpha3.PruneOnEvent) + []Event{namespaceProbeEvent("outer", "m", "green")}, nil, namespacePolicy{}, v1alpha3.PruneOnEvent) require.Error(t, err, "a second document for the same rendered object must not be committed") assert.False(t, changed) diff --git a/internal/git/namespace_fidelity_test.go b/internal/git/namespace_fidelity_test.go new file mode 100644 index 00000000..bed7cf1b --- /dev/null +++ b/internal/git/namespace_fidelity_test.go @@ -0,0 +1,131 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "strings" + "testing" + + gogit "github.com/go-git/go-git/v6" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The render gate compares what a folder renders with the live object it mirrors, and +// metadata.namespace is the one field that is allowed not to match. This file is the whole +// boundary, one test per row of the table in docs/design/created-root-namespace.md: +// +// serializeNamespace governing root metadata.namespace in the comparison +// unset or true any checked +// false sets namespace: checked +// false sets none / no root ignored, every other field still compared +// +// The relaxation is scoped by the RENDER, not by the setting alone. A folder that declares +// namespace: makes a concrete claim and has to keep it; a folder that declares none has said the +// namespace comes from the installer, and comparing it against the source namespace would be +// comparing something the folder deliberately does not express. + +func namespacelessRoot() string { + return "apiVersion: kustomize.config.k8s.io/v1beta1\nkind: Kustomization\nresources:\n - web.yaml\n" +} + +func rootWithNamespace(namespace string) string { + return "apiVersion: kustomize.config.k8s.io/v1beta1\nkind: Kustomization\n" + + "namespace: " + namespace + "\nresources:\n - web.yaml\n" +} + +func seedKustomizeFolder(t *testing.T, worktree *gogit.Worktree, root string) { + t.Helper() + dir := worktree.Filesystem().Root() + seedFile(t, dir, "kustomization.yaml", root) + seedFile(t, dir, "web.yaml", "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: web\n") +} + +// Row 3, the created root. The operator wrote the root itself and wrote no namespace into it, so +// the document renders with none while the live object is in shop. That difference is the shape +// working as declared. +func TestNamespaceFidelity_CreatedRootSuppliesNoNamespaceAndTheWriteIsAccepted(t *testing.T) { + worktree := newWorktreeForTest(t) + + require.NoError(t, flushWithPlacement(t, worktree, useKustomizePolicy(), + serializeNamespacePolicy(false, "shop"), + namespaceProbeEvent("shop", "checkout-config", "green"))) + + root := readWorktreeFile(t, worktree, "kustomization.yaml") + assert.NotContains(t, root, "namespace:", + "serializeNamespace: false means the artifact does not encode its deployment namespace") + assert.NotContains(t, readWorktreeFile(t, worktree, "checkout-config.yaml"), "namespace:") +} + +// Row 3 again, and the point of scoping the rule by the render rather than by who wrote the root: +// a namespace-less root the USER wrote behaves exactly like one the operator created. +func TestNamespaceFidelity_ExistingNamespacelessRootBehavesIdentically(t *testing.T) { + worktree := newWorktreeForTest(t) + seedKustomizeFolder(t, worktree, namespacelessRoot()) + + require.NoError(t, flushWithPlacement(t, worktree, nil, + serializeNamespacePolicy(false, "shop"), + namespaceProbeEvent("shop", "checkout-config", "green"))) + + assert.NotContains(t, readWorktreeFile(t, worktree, "checkout-config.yaml"), "namespace:") + assert.Contains(t, readWorktreeFile(t, worktree, "kustomization.yaml"), "- checkout-config.yaml") +} + +// Row 2, and the refinement that keeps the relaxation honest: a root declaring namespace: shop has +// a concrete render contract, so a live billing object written under it really is being relocated. +// Relaxing there would hide exactly the failure this gate exists to catch. +func TestNamespaceFidelity_RootThatDeclaresANamespaceStillRejectsAnotherNamespacesObject(t *testing.T) { + worktree := newWorktreeForTest(t) + seedKustomizeFolder(t, worktree, rootWithNamespace("shop")) + + err := flushWithPlacement(t, worktree, nil, + serializeNamespacePolicy(false, "billing"), + namespaceProbeEvent("billing", "checkout-config", "green")) + + require.Error(t, err, "the folder renders this document into shop, and the object lives in billing") + assert.Contains(t, err.Error(), "does not render to the live object") +} + +// Row 1. Neither unset nor true relaxes anything: the namespace stays part of the comparison, so a +// transformer that would move the object is still refused. +func TestNamespaceFidelity_TrueAndUnsetStayStrict(t *testing.T) { + for _, tc := range []struct { + name string + policy namespacePolicy + }{ + {"unset infers per document", namespacePolicy{SourceNamespaces: []string{"shop"}}}, + {"true always writes the namespace", serializeNamespacePolicy(true, "shop")}, + } { + t.Run(tc.name, func(t *testing.T) { + worktree := newWorktreeForTest(t) + seedKustomizeFolder(t, worktree, rootWithNamespace("billing")) + + err := flushWithPlacement(t, worktree, nil, tc.policy, + namespaceProbeEvent("shop", "checkout-config", "green")) + + require.Error(t, err, "the root's transformer renders this shop object into billing") + assert.Contains(t, err.Error(), "does not render to the live object") + }) + } +} + +// The relaxation is namespace-shaped and nothing wider: every other field is still compared, so a +// folder that cannot express the object it mirrors is still refused under serializeNamespace: false. +func TestNamespaceFidelity_FalseStillComparesEveryOtherField(t *testing.T) { + worktree := newWorktreeForTest(t) + seedFile(t, worktree.Filesystem().Root(), "kustomization.yaml", + "apiVersion: kustomize.config.k8s.io/v1beta1\nkind: Kustomization\n"+ + "resources:\n - web.yaml\ncommonLabels:\n team: platform\n") + seedFile(t, worktree.Filesystem().Root(), "web.yaml", + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: web\n") + + err := flushWithPlacement(t, worktree, nil, + serializeNamespacePolicy(false, "shop"), + namespaceProbeEvent("shop", "checkout-config", "green")) + + require.Error(t, err, "a label transformer the live object does not carry is still a divergence") + assert.True(t, + strings.Contains(err.Error(), "does not render to the live object") || + strings.Contains(err.Error(), "Git path refused"), + "unexpected refusal: %v", err) +} diff --git a/internal/git/namespace_policy.go b/internal/git/namespace_policy.go new file mode 100644 index 00000000..13280656 --- /dev/null +++ b/internal/git/namespace_policy.go @@ -0,0 +1,83 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +// namespacePolicy is everything a GitTarget declares about the namespace of the documents it +// writes. It is deliberately separate from manifestanalyzer.PlacementPolicy, which mirrors +// spec.placement field-for-field: placement decides WHERE a new document goes and never moves one +// already written, while this decides what is INSIDE every document the target writes and how an +// existing one is found. See docs/layout/model.md, "serializeNamespace". +// +// The zero value is the behavior of a GitTarget that declares nothing: infer per document, and no +// source-namespace fence. Every caller that has no GitTarget to read — the CLI, most tests — passes +// it, so "declares nothing" is spelled out rather than reached by accident. +type namespacePolicy struct { + // Serialize is spec.serializeNamespace: nil infers per document, true always writes + // metadata.namespace, false never does. + Serialize *bool + // SourceNamespaces are the source namespaces reaching this target, sorted — the target's own + // namespace plus the explicit rules[].sourceNamespace of every WatchRule naming it (see + // resolveSourceNamespaces). It is read only when Serialize is explicitly false, because that + // is the only setting whose meaning depends on how many namespaces there are. + SourceNamespaces []string + // SourceNamespaceWildcard records that some rule names "*", so the set above is not the whole + // answer. It is never expanded: a wildcard cannot be proven to be one namespace from the spec, + // which is all the one-source-namespace rule needs to refuse it. + SourceNamespaceWildcard bool +} + +// namespacePolicyFor reads the policy off a GitTarget spec and the source namespaces resolved for +// it. +func namespacePolicyFor(spec v1alpha3.GitTargetSpec, sources []string, wildcard bool) namespacePolicy { + return namespacePolicy{ + Serialize: spec.SerializeNamespace, + SourceNamespaces: sources, + SourceNamespaceWildcard: wildcard, + } +} + +// omitNamespace reports whether the bytes this target writes must leave metadata.namespace out, +// given what inference concluded about the document's own destination. +// +// inferred is the answer the operator has always used: true when the kustomization governing the +// document's path supplies exactly this resource's namespace. An explicit spec.serializeNamespace +// overrides it in both directions — that is what the field is, an override of a correctness rule — +// and nil leaves inference alone. +// +// It is asked for namespaced documents only. A cluster-scoped document has no namespace to write +// or omit, so both answers are the same for it and the field is ignored rather than being an error. +func (p namespacePolicy) omitNamespace(inferred bool) bool { + if p.Serialize == nil { + return inferred + } + return !*p.Serialize +} + +// declaresNamespaceFree reports whether the target explicitly declared that no document it writes +// carries its own namespace. It is the EXPLICIT half only: inference reaching the same answer for +// one document says nothing about the folder, and the rules keyed on this one are folder-wide +// claims. +func (p namespacePolicy) declaresNamespaceFree() bool { + return p.Serialize != nil && !*p.Serialize +} + +// declaredNamespace is the namespace every document in this folder belongs to, or "" when the +// folder makes no such claim. It is non-empty only for a target that declared the folder +// namespace-free AND that exactly one source namespace reaches: the declaration says the namespace +// is not in the bytes, and the single source namespace says which one it is. +// +// The store needs it to READ the folder back. A namespace-less document that no kustomization +// governs otherwise belongs to no namespace at all, so the live object it mirrors would never +// match it and the next write would append a second copy beside it. Where the answer is not +// single — two namespaces, or a wildcard — nothing is attributed and the write is refused instead +// (see docs/layout/model.md, "The second guard"). +func (p namespacePolicy) declaredNamespace() string { + if !p.declaresNamespaceFree() || p.SourceNamespaceWildcard || len(p.SourceNamespaces) != 1 { + return "" + } + return p.SourceNamespaces[0] +} diff --git a/internal/git/pending_writes.go b/internal/git/pending_writes.go index 42249a00..92cc9d02 100644 --- a/internal/git/pending_writes.go +++ b/internal/git/pending_writes.go @@ -161,6 +161,14 @@ func (w *BranchWorker) resolveTargetMetadata( return ResolvedTargetMetadata{}, fmt.Errorf("failed to resolve target encryption configuration: %w", err) } + // Resolved here rather than at the write, with the rest of the target's mutable state: the set + // is a fact about the config cluster at planning time, and a write replayed after a rebase must + // be judged against the policy it was planned under. + sourceNamespaces, wildcard, err := resolveSourceNamespaces(ctx, w.Client, target) + if err != nil { + return ResolvedTargetMetadata{}, err + } + return ResolvedTargetMetadata{ Name: target.Name, Namespace: target.Namespace, @@ -168,6 +176,7 @@ func (w *BranchWorker) resolveTargetMetadata( BootstrapOptions: buildBootstrapOptions(encryptionConfig), EncryptionConfig: encryptionConfig, Placement: resolvePlacementPolicy(target.Spec.Placement), + Namespaces: namespacePolicyFor(target.Spec, sourceNamespaces, wildcard), PruneMode: target.EffectivePruneMode(), SourceCluster: target.SourceCluster(), Suspend: target.Spec.Suspend, @@ -201,8 +210,9 @@ func resolvePlacementPolicy(spec *v1alpha3.GitTargetPlacementSpec) *manifestanal return nil } return &manifestanalyzer.PlacementPolicy{ - ByType: spec.ByType, - Default: spec.Default, + ByType: spec.ByType, + Default: spec.Default, + UseKustomize: spec.UseKustomize, } } @@ -216,6 +226,23 @@ func resolvePlacementPolicy(spec *v1alpha3.GitTargetPlacementSpec) *manifestanal // base with no matching target (e.g. an event whose target metadata could not be // resolved) gets no declared policy, falling through to the kustomize root and then the // canonical path. +// namespacePolicyForBase finds the namespace policy for the GitTarget that owns base among +// targets, matching exactly as placementPolicyForBase does. A base with no matching target gets +// the zero policy, which is "declares nothing" — the same fallback an unresolvable target gets for +// placement, and the only safe one: reading a missing target as having declared its folder +// namespace-free would strip namespaces nobody asked to have stripped. +func namespacePolicyForBase( + targets map[pendingTargetKey]ResolvedTargetMetadata, + base string, +) namespacePolicy { + for _, md := range targets { + if sanitizePath(md.Path) == base { + return md.Namespaces + } + } + return namespacePolicy{} +} + func placementPolicyForBase( targets map[pendingTargetKey]ResolvedTargetMetadata, base string, diff --git a/internal/git/placement_metrics_test.go b/internal/git/placement_metrics_test.go index 62322d0e..e13ba2c1 100644 --- a/internal/git/placement_metrics_test.go +++ b/internal/git/placement_metrics_test.go @@ -74,7 +74,15 @@ func flushWithPolicy( ) { t.Helper() w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} - _, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, policy, v1alpha3.PruneOnEvent) + _, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "", + events, + policy, + namespacePolicy{}, + v1alpha3.PruneOnEvent, + ) require.NoError(t, err) } @@ -259,6 +267,7 @@ func TestPlacementMetrics_MixedSensitivityNewFileCountsARefusal(t *testing.T) { "", []Event{targetedConfigMapEvent(), targetedSecretEvent("api-token", "app")}, policy, + namespacePolicy{}, v1alpha3.PruneOnEvent, ) require.NoError(t, err, "a co-mingling refusal is skipped, not returned as a batch error") diff --git a/internal/git/placement_test.go b/internal/git/placement_test.go index 1ee25c8b..0bb0ab7a 100644 --- a/internal/git/placement_test.go +++ b/internal/git/placement_test.go @@ -42,7 +42,15 @@ func applyEventsWithPolicy( ) bool { t.Helper() w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, policy, v1alpha3.PruneOnEvent) + changed, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "", + events, + policy, + namespacePolicy{}, + v1alpha3.PruneOnEvent, + ) require.NoError(t, err) return changed } @@ -167,6 +175,7 @@ func TestPlacement_SensitiveCollision_SkipsWithoutCrashing(t *testing.T) { "", []Event{event}, policy, + namespacePolicy{}, v1alpha3.PruneOnEvent, ) @@ -261,7 +270,13 @@ func TestPlacement_UndecodableKustomization_RefusesTheFlush(t *testing.T) { w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} _, err := w.flushEventsToWorktree( - context.Background(), worktree, "", []Event{newConfigMapEvent("cache", "app")}, nil, v1alpha3.PruneOnEvent, + context.Background(), + worktree, + "", + []Event{newConfigMapEvent("cache", "app")}, + nil, + namespacePolicy{}, + v1alpha3.PruneOnEvent, ) require.Error(t, err, "a kustomization kustomize cannot build must refuse the folder, not be written into") } @@ -290,7 +305,7 @@ func TestPlacement_ExternalBaseOverlay_NewObject(t *testing.T) { w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} changed, err := w.flushEventsToWorktree( context.Background(), worktree, "overlays/test", - []Event{newConfigMapEvent("cache", "podinfo-test")}, nil, v1alpha3.PruneOnEvent, + []Event{newConfigMapEvent("cache", "podinfo-test")}, nil, namespacePolicy{}, v1alpha3.PruneOnEvent, ) require.NoError(t, err, "the overlay new-object flush must pass the render oracle") require.True(t, changed) @@ -360,6 +375,7 @@ func flushOverlayDeployment(t *testing.T, worktree *gogit.Worktree, event Event) "overlays/test", []Event{event}, nil, + namespacePolicy{}, v1alpha3.PruneOnEvent, ) return err @@ -449,6 +465,7 @@ func TestOverlayAuthors_DeletePatch_ForInheritedObject(t *testing.T) { "overlays/test", []Event{del}, nil, + namespacePolicy{}, v1alpha3.PruneOnEvent, ) require.NoError(t, err, "deleting an inherited object must author a $patch: delete, not refuse") @@ -497,6 +514,7 @@ func TestOverlayAuthors_DeletePatch_SkipsOnPathCollision(t *testing.T) { "overlays/test", []Event{del}, nil, + namespacePolicy{}, v1alpha3.PruneOnEvent, ) require.NoError(t, err, "a patch-path collision must be skipped, not error") @@ -509,7 +527,7 @@ func TestOverlayAuthors_DeletePatch_SkipsOnPathCollision(t *testing.T) { func newTestWriteBatch(t *testing.T) *writeBatch { t.Helper() writer := newContentWriter(types.SensitiveResourcePolicy{}) - return newWriteBatch(context.Background(), writer, nil, manifestanalyzer.FolderScan{}, nil, "") + return newWriteBatch(context.Background(), writer, nil, manifestanalyzer.FolderScan{}, nil, namespacePolicy{}, "") } func TestAppendYAMLDocument(t *testing.T) { @@ -638,6 +656,7 @@ func TestPlacement_ColdBundleCollision_SensitiveNeverMerged(t *testing.T) { changed, err := w.flushEventsToWorktree( context.Background(), worktree, "", []Event{newSecretEvent("first"), newSecretEvent("second")}, policy, + namespacePolicy{}, v1alpha3.PruneOnEvent, ) @@ -682,7 +701,13 @@ func TestPlacement_ColdBundleCollision_SensitiveAndPlaintextNeverMix(t *testing. secretFirst := newWorktreeForTest(t) wsf := &BranchWorker{contentWriter: newWriter()} _, err := wsf.flushEventsToWorktree( - context.Background(), secretFirst, "", []Event{secretEvent, configMapEvent}, policy, v1alpha3.PruneOnEvent, + context.Background(), + secretFirst, + "", + []Event{secretEvent, configMapEvent}, + policy, + namespacePolicy{}, + v1alpha3.PruneOnEvent, ) require.NoError(t, err) secretFirstBody, readErr := os.ReadFile(filepath.Join(secretFirst.Filesystem().Root(), "all.yaml")) @@ -695,7 +720,13 @@ func TestPlacement_ColdBundleCollision_SensitiveAndPlaintextNeverMix(t *testing. configMapFirst := newWorktreeForTest(t) wcf := &BranchWorker{contentWriter: newWriter()} _, err = wcf.flushEventsToWorktree( - context.Background(), configMapFirst, "", []Event{configMapEvent, secretEvent}, policy, v1alpha3.PruneOnEvent, + context.Background(), + configMapFirst, + "", + []Event{configMapEvent, secretEvent}, + policy, + namespacePolicy{}, + v1alpha3.PruneOnEvent, ) require.NoError(t, err) configMapFirstBody, readErr := os.ReadFile(filepath.Join(configMapFirst.Filesystem().Root(), "all.yaml")) diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index 71812c24..a00c62ec 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -71,6 +71,7 @@ func (w *BranchWorker) flushEventsToWorktree( base string, events []Event, policy *manifestanalyzer.PlacementPolicy, + namespaces namespacePolicy, pruneMode v1alpha3.PruneMode, ) (bool, error) { root := worktree.Filesystem().Root() @@ -82,7 +83,7 @@ func (w *BranchWorker) flushEventsToWorktree( // Every event in a base shares one GitTarget (events are grouped by base), so they share // one source cluster; resolve this subtree's GVK->GVR against that cluster's registry. mapper := w.mapperForCluster(clusterIDForEvents(events)) - batch := newWriteBatch(ctx, w.contentWriter, mapper, scoped.scan, policy, scoped.writeSubdir) + batch := newWriteBatch(ctx, w.contentWriter, mapper, scoped.scan, policy, namespaces, scoped.writeSubdir) batch.pruneMode = pruneMode batch.target = placementTargetForEvents(events) if err := batch.refusal(); err != nil { @@ -94,6 +95,12 @@ func (w *BranchWorker) flushEventsToWorktree( // above on purpose: a folder the operator has refused to manage is one whose layout it should // not be making claims about, and GitPathAccepted=False already says why. w.scanLayout(ctx, batch, worktree) + // The one write-plan precondition that is not about the folder at all, which is why it is + // raised AFTER the layout is published: the folder is fine and its shape is still worth + // reporting; what is wrong is the configuration pointed at it. + if err := batch.sourceNamespaceRefusal(); err != nil { + return false, err + } for _, event := range events { if err := batch.applyEvent(ctx, event); err != nil { return false, err @@ -136,6 +143,11 @@ type writeBatch struct { // only for a resource with no existing document. nil means no declared policy — // placement falls through to the folder's one kustomize root and then the canonical path. policy *manifestanalyzer.PlacementPolicy + // namespaces is the GitTarget's declared namespace behavior — spec.serializeNamespace — which + // decides whether the bytes this batch writes carry metadata.namespace at all. The zero value + // is "declare nothing", i.e. infer per document, which is what every caller with no GitTarget + // to read (the CLI, most tests) gets. + namespaces namespacePolicy // pruneMode is the GitTarget's effective spec.prune.mode, gating the EXPLICIT delete // path only (applyDelete). The inferred mark-and-sweep is gated a layer up, in the // planner, so a suppressed drop never becomes an action in the first place. @@ -153,6 +165,12 @@ type writeBatch struct { // stay within it. The store and every path in it are keyed relative to renderBase, so a // writable path is one under writeSubdir. See internal/git/render_scope.go. writeSubdir string + // createdRoot is the kustomization.yaml this batch WROTE, for a folder that had none and a + // target that declared spec.placement.useKustomize. It is nil in every other case, including + // the ordinary one where a root was already there. It exists so the second new document in a + // batch joins the root the first one created: the store was built before the batch, so nothing + // in it knows the file exists. + createdRoot *manifestanalyzer.KustomizationInfo // layout is what the scan resolved about this folder's shape. It is published as // status.placement, and createNew reads it: a folder covering several render roots has no // single one to place a new document into, so placing one is refused rather than guessed. @@ -191,6 +209,7 @@ func newWriteBatch( mapper typeset.Lookup, scan manifestanalyzer.FolderScan, policy *manifestanalyzer.PlacementPolicy, + namespaces namespacePolicy, writeSubdir string, ) *writeBatch { // The writer allowlist retains build directives (kustomization.yaml) and the operator's @@ -200,7 +219,8 @@ func newWriteBatch( // placement. The scan also carries the foreign-content view and the active // .gittargetignore, so the structure-only acceptance gate (run by writeBatch.refusal) and // the write-plan precondition (run by writeBatch.flush) read both from the store. - store := manifestanalyzer.BuildStoreFromScan(ctx, scan, mapper, manifestanalyzer.WriterAllowlist()) + store := manifestanalyzer.BuildStoreFromScan(ctx, scan, mapper, manifestanalyzer.WriterAllowlist(), + manifestanalyzer.WithDeclaredNamespace(namespaces.declaredNamespace())) // Surface the store's build-time warnings (ambiguous namespace or override // context, scope mismatches) once per batch: these drive silent fallbacks — // e.g. an ambiguous override chain falls back to write-through — and without @@ -219,6 +239,7 @@ func newWriteBatch( contentByPath: contentByPath, buffers: map[string]*fileBuffer{}, policy: policy, + namespaces: namespaces, writeSubdir: writeSubdir, } // Resolved with the store rather than by each caller, so no write path can reach createNew @@ -243,6 +264,28 @@ func (wb *writeBatch) refusal() error { return manifestanalyzer.RefusalError(manifestanalyzer.AcceptStructureOnly(wb.store)) } +// sourceNamespaceRefusal is the write-plan precondition for the one-source-namespace rule: a target +// that declared its folder namespace-free admits exactly one source namespace, and the second is +// refused before a byte moves. +// +// It is the CORRECTNESS layer, and it holds whatever admission did. The WatchRule admission check +// is atomic feedback at the moment the mistake is made, but it is one-shot: it cannot see a +// serializeNamespace flipped to false after the rules were created, and it is a fail-open webhook +// that a cluster need not be running at all. Everything that must be true of the bytes is decided +// here. See docs/spec/where-validation-lives.md. +func (wb *writeBatch) sourceNamespaceRefusal() error { + issues := manifestanalyzer.MultipleSourceNamespacesRefusal( + wb.namespaces.declaresNamespaceFree(), + wb.namespaces.SourceNamespaces, + wb.namespaces.SourceNamespaceWildcard, + wb.writeSubdir, + ) + if len(issues) == 0 { + return nil + } + return manifestanalyzer.RefusalError(manifestanalyzer.Acceptance{Accepted: false, Issues: issues}) +} + // fileBuffer is the commit-scoped, hydrated working copy of one file under the // GitTarget base path. original is the worktree bytes (nil for a file the batch // creates); current is the bytes after applying actions (nil means the file should @@ -402,8 +445,9 @@ func (wb *writeBatch) createNew(ctx context.Context, event Event) (upsertOutcome // namespace: transformer) must keep metadata.namespace out of the written bytes, // exactly as patchExisting already does for an in-place edit of an existing // document in the same context — otherwise the new document would silently break - // the convention every sibling in that directory follows. - if placement.NamespaceInherited && event.Object != nil { + // the convention every sibling in that directory follows. An explicit + // spec.serializeNamespace overrides that inference in both directions. + if wb.namespaces.omitNamespace(placement.NamespaceInherited) && event.Object != nil { event.Object = event.Object.DeepCopy() event.Object.SetNamespace("") } @@ -431,6 +475,9 @@ func (wb *writeBatch) createNew(ctx context.Context, event Event) (upsertOutcome // foreign content we declined to own, added to resources: on our say-so; and either way it // counted as outcome="added", the value that is supposed to mean "the file we just wrote will // build". Pinned by TestPlacementMetrics_RefusedPlacementLeavesTheKustomizationAlone. + if err := wb.resolveKustomizeRootForNew(ctx, &placement); err != nil { + return upsertNoChange, err + } if placement.Kustomization != nil { wb.appendKustomizationResource(ctx, event, placement) } @@ -445,11 +492,49 @@ func (wb *writeBatch) createNew(ctx context.Context, event Event) (upsertOutcome // non-converging commit into a reported refusal naming the file and the object. It does not // make the write work — that needs attribution for a document that does not exist yet — but // "we cannot express this here" is an answer, and quietly writing a lie is not. - wb.putToKustomize = wb.putToKustomize || placement.Kustomization != nil + wb.putToKustomize = wb.putToKustomize || placement.Kustomization != nil || wb.createdRoot != nil wb.intend(markUnchecked(intentFor(live, placement.Path, false), sensitive)) return outcome, nil } +// resolveKustomizeRootForNew settles what renders a new document, for a target that declared +// spec.placement.useKustomize. It creates the folder's root when there is none — with this document +// already registered in the bytes it writes, so a LATER document in the same batch joins it through +// the ordinary resources: append — and otherwise refuses a placement no kustomization would render. +// +// Both halves are here rather than in createNew because they answer one question between them: is +// this document going to be rendered at all? +func (wb *writeBatch) resolveKustomizeRootForNew( + ctx context.Context, + placement *manifestanalyzer.PlacementResult, +) error { + if created := wb.bootstrapKustomization(ctx, *placement); created != nil { + placement.Kustomization = created + } + issues := wb.unrenderedPlacementRefusal(*placement) + if len(issues) == 0 { + return nil + } + return manifestanalyzer.RefusalError(manifestanalyzer.Acceptance{Accepted: false, Issues: issues}) +} + +// unrenderedPlacementRefusal reports the refusal for a placement under spec.placement.useKustomize +// that no kustomization would render. A document is rendered when a kustomization governs its path, +// and there are three ways for that to be true: one already governed it and needs the entry +// (LocateNew set Kustomization), one already governed it and already lists the path, or this batch +// created the root — which registers the first document in the bytes it writes, so it reports no +// Kustomization to append to. +func (wb *writeBatch) unrenderedPlacementRefusal( + placement manifestanalyzer.PlacementResult, +) []manifestanalyzer.AcceptanceIssue { + governed := placement.Kustomization != nil || + wb.createdRoot != nil || + manifestanalyzer.GoverningKustomization(wb.store, wb.writeSubdir, placement.Path) != nil + useKustomize := wb.policy != nil && wb.policy.UseKustomize + return manifestanalyzer.UnrenderedPlacementRefusal( + useKustomize, governed, placement.Path, wb.layout.RenderRoot) +} + // placeNewDocument writes the new document at its resolved placement: appended to an existing // accepted bundle, folded into a same-batch cold bundle, or as a file of its own. // @@ -798,7 +883,7 @@ func (wb *writeBatch) patchExisting( } gitDoc, _ := manifestedit.NewDocumentAt(filePath, buf.current, idx) desired := event.Object - if dm.NamespaceInheritedFromContext() && desired != nil { + if wb.namespaces.omitNamespace(dm.NamespaceAbsentFromFile()) && desired != nil { desired = desired.DeepCopy() desired.SetNamespace("") } @@ -990,7 +1075,10 @@ func (wb *writeBatch) renderPrecondition() error { } var refused *manifestanalyzer.RenderRefusedError - if err := manifestanalyzer.VerifyBatchRenders(before, wb.files(), wb.intents); err != nil { + verifyOptions := manifestanalyzer.VerifyOptions{ + NamespaceSuppliedDownstream: wb.namespaces.declaresNamespaceFree(), + } + if err := manifestanalyzer.VerifyBatchRenders(before, wb.files(), wb.intents, verifyOptions); err != nil { if errors.As(err, &refused) { issues := make([]manifestanalyzer.AcceptanceIssue, 0, len(refused.Reasons)) for _, reason := range refused.Reasons { @@ -1458,14 +1546,19 @@ func (wb *writeBatch) resolveDelete(event Event) (deleteTarget, bool) { } // rawManifestIDForCurrentBytes maps an effective manifest identity back to the raw -// identity as written in the file: when the namespace was inherited from kustomization -// context, the file bytes carry no metadata.namespace, so the document is located by a +// identity as written in the file: when the namespace came from anywhere but the file — a +// kustomization's namespace: transformer, or the GitTarget's declaration that this folder's +// documents carry none — the bytes hold no metadata.namespace, so the document is located by a // namespace-less identity. +// +// It reads the DOCUMENT, never spec.serializeNamespace. The setting says what the next write will +// contain; this asks what the file already contains, and a folder written before the setting +// changed still has to be found. func rawManifestIDForCurrentBytes( id manifestedit.Identity, dm *manifestanalyzer.DocumentModel, ) manifestedit.Identity { - if dm != nil && dm.NamespaceInheritedFromContext() { + if dm != nil && dm.NamespaceAbsentFromFile() { id.Namespace = "" } return id diff --git a/internal/git/plan_flush_test.go b/internal/git/plan_flush_test.go index d411edda..9a88c08b 100644 --- a/internal/git/plan_flush_test.go +++ b/internal/git/plan_flush_test.go @@ -126,6 +126,7 @@ func TestPlanFlush_DeleteByGVROnlyFollowsMovedManifestViaMapper(t *testing.T) { "", []Event{del}, nil, + namespacePolicy{}, v1alpha3.PruneOnEvent, ) require.NoError(t, err) diff --git a/internal/git/prune_mode_test.go b/internal/git/prune_mode_test.go index 0744afce..2bdd25fd 100644 --- a/internal/git/prune_mode_test.go +++ b/internal/git/prune_mode_test.go @@ -56,7 +56,7 @@ func deleteUnder(t *testing.T, worktree *gogit.Worktree, mode v1alpha3.PruneMode t.Helper() w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} changed, err := w.flushEventsToWorktree( - context.Background(), worktree, "", []Event{deleteEventFor(name)}, nil, mode) + context.Background(), worktree, "", []Event{deleteEventFor(name)}, nil, namespacePolicy{}, mode) require.NoError(t, err) return changed } diff --git a/internal/git/render_fidelity_test.go b/internal/git/render_fidelity_test.go index 82b4bd7b..b7da7322 100644 --- a/internal/git/render_fidelity_test.go +++ b/internal/git/render_fidelity_test.go @@ -60,7 +60,7 @@ func TestRenderFidelityRefusal_BlocksLiveAndResyncWrites(t *testing.T) { name: "live event", run: func(worker *BranchWorker, worktree *gogit.Worktree) error { _, err := worker.flushEventsToWorktree( - context.Background(), worktree, "", []Event{postBuildTokenEvent()}, nil, v1alpha3.PruneOnEvent) + context.Background(), worktree, "", []Event{postBuildTokenEvent()}, nil, namespacePolicy{}, v1alpha3.PruneOnEvent) return err }, }, diff --git a/internal/git/render_scope_test.go b/internal/git/render_scope_test.go index 68ca0515..e457859f 100644 --- a/internal/git/render_scope_test.go +++ b/internal/git/render_scope_test.go @@ -131,7 +131,15 @@ func flushAtBase( ) (bool, error) { t.Helper() w := &BranchWorker{contentWriter: writer, mapper: mapper} - return w.flushEventsToWorktree(context.Background(), worktree, base, events, nil, v1alpha3.PruneOnEvent) + return w.flushEventsToWorktree( + context.Background(), + worktree, + base, + events, + nil, + namespacePolicy{}, + v1alpha3.PruneOnEvent, + ) } // The read scope of a pure overlay re-roots at the base's parent, keeps every scanned path diff --git a/internal/git/resync_flush.go b/internal/git/resync_flush.go index dbee1e53..8754aa75 100644 --- a/internal/git/resync_flush.go +++ b/internal/git/resync_flush.go @@ -314,7 +314,7 @@ func (w *BranchWorker) refuseUnsafeWorktree( // scan does, so the target's declared policy is carried in rather than passed as nil. batch := newWriteBatch( ctx, w.contentWriter, w.mapperForCluster(target.SourceCluster), - scoped.scan, target.Placement, scoped.writeSubdir) + scoped.scan, target.Placement, target.Namespaces, scoped.writeSubdir) batch.target = placementTarget{namespace: target.Namespace, name: target.Name} if err := batch.refusal(); err != nil { return err @@ -378,6 +378,7 @@ func (w *BranchWorker) applyResyncToWorktree( w.mapperForCluster(target.SourceCluster), scoped.scan, target.Placement, + target.Namespaces, scoped.writeSubdir, ) // The resync's events are synthesised from the desired snapshot and carry no GitTarget @@ -394,6 +395,12 @@ func (w *BranchWorker) applyResyncToWorktree( if err := batch.refusal(); err != nil { return ResyncStats{}, false, err } + // The same precondition the live path applies: a folder declared namespace-free that two + // source namespaces reach cannot tell its own documents apart, and a resync writes exactly + // what the live path writes. + if err := batch.sourceNamespaceRefusal(); err != nil { + return ResyncStats{}, false, err + } // The store is built from the same files the planner reads, so the plan and the apply // 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 diff --git a/internal/git/serialize_namespace_test.go b/internal/git/serialize_namespace_test.go new file mode 100644 index 00000000..e44e23f7 --- /dev/null +++ b/internal/git/serialize_namespace_test.go @@ -0,0 +1,214 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "os" + "path/filepath" + "strings" + "testing" + + gogit "github.com/go-git/go-git/v6" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// spec.serializeNamespace overrides the inference at every site that decides whether +// metadata.namespace is in the bytes. The corpus (docs/layout/shapes/2-flat-namespace-free and +// 4-tree-namespace-free) pins the FIRST write of a namespace-free folder; what it cannot pin is +// everything after it — an update, a second write of the same object, and the folder that already +// supplies the namespace being overridden the other way. Those are here. + +func serializeNamespacePolicy(serialize bool, sources ...string) namespacePolicy { + return namespacePolicy{Serialize: &serialize, SourceNamespaces: sources} +} + +func flushWithNamespacePolicy( + t *testing.T, + worktree *gogit.Worktree, + policy namespacePolicy, + events ...Event, +) error { + t.Helper() + w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: namespaceProbeMapper()} + _, err := w.flushEventsToWorktree(t.Context(), worktree, "", events, nil, policy, v1alpha3.PruneOnEvent) + return err +} + +func readWorktreeFile(t *testing.T, worktree *gogit.Worktree, rel string) string { + t.Helper() + body, err := os.ReadFile(filepath.Join(worktree.Filesystem().Root(), rel)) + require.NoError(t, err) + return string(body) +} + +// TestSerializeNamespace_FalseOmitsItWhereInferenceWouldWriteIt is the flat namespace-free folder: +// nothing in it supplies a namespace, so inference writes one, and the declaration is what stops it. +func TestSerializeNamespace_FalseOmitsItWhereInferenceWouldWriteIt(t *testing.T) { + worktree := newWorktreeForTest(t) + + require.NoError(t, flushWithNamespacePolicy(t, worktree, + serializeNamespacePolicy(false, "shop"), + namespaceProbeEvent("shop", "checkout-config", "green"))) + + body := readWorktreeFile(t, worktree, "shop/configmaps/checkout-config.yaml") + assert.NotContains(t, body, "namespace:", "an explicit serializeNamespace: false writes no namespace") + assert.Contains(t, body, "name: checkout-config") +} + +// TestSerializeNamespace_TrueWritesItWhereInferenceWouldOmitIt is the override in the other +// direction: the folder's kustomization supplies exactly this namespace, so inference would leave +// metadata.namespace out, and true puts it back. +func TestSerializeNamespace_TrueWritesItWhereInferenceWouldOmitIt(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem().Root() + seedFile(t, root, "kustomization.yaml", strings.Join([]string{ + "apiVersion: kustomize.config.k8s.io/v1beta1", + "kind: Kustomization", + "namespace: shop", + "resources: []", + "", + }, "\n")) + + require.NoError(t, flushWithNamespacePolicy(t, worktree, + serializeNamespacePolicy(true, "shop"), + namespaceProbeEvent("shop", "checkout-config", "green"))) + + assert.Contains(t, readWorktreeFile(t, worktree, "checkout-config.yaml"), "namespace: shop", + "an explicit serializeNamespace: true writes the namespace even where the folder supplies it") +} + +// TestSerializeNamespace_UnsetKeepsInference is the default, and the whole reason the field is a +// *bool: an object that says nothing behaves exactly as it did before the field existed. +func TestSerializeNamespace_UnsetKeepsInference(t *testing.T) { + worktree := newWorktreeForTest(t) + + require.NoError(t, flushWithNamespacePolicy(t, worktree, namespacePolicy{}, + namespaceProbeEvent("shop", "checkout-config", "green"))) + + assert.Contains(t, readWorktreeFile(t, worktree, "shop/configmaps/checkout-config.yaml"), + "namespace: shop", "unset infers, and nothing in this folder supplies a namespace") +} + +// TestSerializeNamespace_FalseUpdatesTheDocumentItAlreadyWrote is the case no first-write fixture +// can show, and the one that makes the setting shippable at all. +// +// A namespace-free document that no kustomization governs belongs, as far as the folder is +// concerned, to no namespace. Read back that way it does not match the live object it mirrors, so +// the second write of the SAME object would find nothing, place it as new, and append a second +// copy of it into the file holding the first. The declared-namespace attribution +// (manifestanalyzer.WithDeclaredNamespace) is what closes that loop. +func TestSerializeNamespace_FalseUpdatesTheDocumentItAlreadyWrote(t *testing.T) { + worktree := newWorktreeForTest(t) + policy := serializeNamespacePolicy(false, "shop") + + require.NoError(t, flushWithNamespacePolicy(t, worktree, policy, + namespaceProbeEvent("shop", "checkout-config", "green"))) + require.NoError(t, flushWithNamespacePolicy(t, worktree, policy, + namespaceProbeEvent("shop", "checkout-config", "blue"))) + + body := readWorktreeFile(t, worktree, "shop/configmaps/checkout-config.yaml") + assert.Equal(t, 1, strings.Count(body, "name: checkout-config"), + "the second write must edit the document the first one wrote, not append a second copy of it") + assert.Contains(t, body, "color: blue", "the edit must land") + assert.NotContains(t, body, "namespace:", "and it must still carry no namespace") +} + +// TestSerializeNamespace_FalseAttributesNothingWhenTwoNamespacesReachTheTarget is the boundary of +// that attribution: with two source namespaces there is no single namespace a namespace-free +// document could belong to, so nothing is attributed. The write is refused instead — see +// TestSerializeNamespace_SecondSourceNamespaceIsRefused — and this test pins that the READ side +// does not quietly pick one in the meantime. +func TestSerializeNamespace_FalseAttributesNothingWhenTwoNamespacesReachTheTarget(t *testing.T) { + policy := serializeNamespacePolicy(false, "billing", "shop") + assert.Empty(t, policy.declaredNamespace(), "two namespaces have no single answer") + + wildcard := serializeNamespacePolicy(false, "shop") + wildcard.SourceNamespaceWildcard = true + assert.Empty(t, wildcard.declaredNamespace(), "a wildcard cannot be proven to be one namespace") + + assert.Equal(t, "shop", serializeNamespacePolicy(false, "shop").declaredNamespace()) + assert.Empty(t, serializeNamespacePolicy(true, "shop").declaredNamespace(), + "a folder whose documents carry their own namespace needs no attribution") + assert.Empty(t, namespacePolicy{SourceNamespaces: []string{"shop"}}.declaredNamespace(), + "inference is never a folder-wide claim, so unset attributes nothing") +} + +// TestSerializeNamespace_SecondSourceNamespaceIsRefused is the fence around "one namespace", and +// the reason it refuses where the rest of the model reports: two namespaces reaching a +// namespace-free folder do not collide, they MATCH. shop/checkout-config and +// billing/checkout-config both resolve to bytes carrying no namespace, so one document ends up +// flipping between two live objects and Git holds no record that it happened. +// +// docs/layout/shapes/2-flat-namespace-free asserts the same refusal end to end, against the +// condition a user reads. This asserts the halves that fixture cannot: that the refusal is a +// precondition (nothing is written, whatever the events were), and that a wildcard is refused +// without enumerating anything. +func TestSerializeNamespace_SecondSourceNamespaceIsRefused(t *testing.T) { + t.Run("a second namespace refuses before a byte moves", func(t *testing.T) { + worktree := newWorktreeForTest(t) + + err := flushWithNamespacePolicy(t, worktree, + serializeNamespacePolicy(false, "billing", "shop"), + namespaceProbeEvent("shop", "checkout-config", "green")) + + require.Error(t, err) + assert.Contains(t, err.Error(), "admits exactly one source namespace") + assert.NoFileExists(t, filepath.Join(worktree.Filesystem().Root(), "shop/configmaps/checkout-config.yaml"), + "a refused flush writes nothing") + }) + + t.Run("a wildcard is refused without enumerating anything", func(t *testing.T) { + worktree := newWorktreeForTest(t) + policy := serializeNamespacePolicy(false, "shop") + policy.SourceNamespaceWildcard = true + + err := flushWithNamespacePolicy(t, worktree, policy, + namespaceProbeEvent("shop", "checkout-config", "green")) + + require.Error(t, err) + assert.Contains(t, err.Error(), `sourceNamespace "*"`) + }) + + t.Run("one namespace writes", func(t *testing.T) { + worktree := newWorktreeForTest(t) + require.NoError(t, flushWithNamespacePolicy(t, worktree, + serializeNamespacePolicy(false, "shop"), + namespaceProbeEvent("shop", "checkout-config", "green"))) + }) + + t.Run("inference is never fenced", func(t *testing.T) { + worktree := newWorktreeForTest(t) + require.NoError(t, flushWithNamespacePolicy(t, worktree, + namespacePolicy{SourceNamespaces: []string{"billing", "shop"}}, + namespaceProbeEvent("shop", "checkout-config", "green")), + "a folder that declared nothing resolves each document against the root governing it") + }) + + t.Run("serializeNamespace: true is not fenced either", func(t *testing.T) { + worktree := newWorktreeForTest(t) + require.NoError(t, flushWithNamespacePolicy(t, worktree, + serializeNamespacePolicy(true, "billing", "shop"), + namespaceProbeEvent("shop", "checkout-config", "green")), + "every document carries its own namespace, so two of them are still two documents") + }) +} + +// TestSerializeNamespace_RefusalPublishesItsOwnReason pins the reason the refusal reaches status +// under. The message is asserted by the corpus fixture; this is the mapping, which is what a +// consumer alerts on and what Stalled carries. +func TestSerializeNamespace_RefusalPublishesItsOwnReason(t *testing.T) { + worktree := newWorktreeForTest(t) + + err := flushWithNamespacePolicy(t, worktree, + serializeNamespacePolicy(false, "billing", "shop"), + namespaceProbeEvent("shop", "checkout-config", "green")) + + var refused *manifestanalyzer.AcceptanceRefusedError + require.ErrorAs(t, err, &refused) + assert.Equal(t, "MultipleSourceNamespaces", manifestanalyzer.GitPathRefusalReason(refused)) +} diff --git a/internal/git/source_namespaces.go b/internal/git/source_namespaces.go new file mode 100644 index 00000000..8b51a27f --- /dev/null +++ b/internal/git/source_namespaces.go @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "fmt" + "sort" + + "sigs.k8s.io/controller-runtime/pkg/client" + + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +// resolveSourceNamespaces returns the source namespaces that reach a GitTarget, as +// docs/layout/model.md defines the set: the target's OWN namespace, plus the explicit +// rules[].sourceNamespace of every WatchRule pointing at it. It reads the config cluster and +// nothing else — no scan, no repository state. +// +// Three things it deliberately is not: +// +// - It is not spec.allowedSourceNamespaces. That field is an authorization fence — who MAY write +// here — and this is a question about what the folder means. They are computed from different +// inputs and the wave that deletes the field leaves this untouched. +// - It does not read ClusterWatchRules. Those watch cluster-scoped resources, which have no +// namespace to contribute. +// - It does not enumerate a wildcard. A rules[] item naming "*" makes the set unknowable from the +// spec, so it is reported as such (wildcard=true) and the caller refuses rather than expands: +// that holds under both readings of "*", which is all any caller of this needs. +// +// A rule that cannot be listed is an error, never an empty set: silently reading "one namespace" +// off a failed List is how a fence becomes a no-op. +func resolveSourceNamespaces( + ctx context.Context, + c client.Client, + target *v1alpha3.GitTarget, +) ([]string, bool, error) { + var rules v1alpha3.WatchRuleList + if err := c.List(ctx, &rules, client.InNamespace(target.Namespace)); err != nil { + return nil, false, fmt.Errorf("failed to list WatchRules for GitTarget %s/%s: %w", + target.Namespace, target.Name, err) + } + + wildcard := false + seen := map[string]struct{}{target.Namespace: {}} + for i := range rules.Items { + rule := &rules.Items[i] + if rule.Spec.TargetRef.Name != target.Name { + continue + } + for _, item := range rule.Spec.Rules { + if item.IsSourceNamespaceWildcard() { + wildcard = true + continue + } + seen[item.EffectiveSourceNamespace(rule.Namespace)] = struct{}{} + } + } + + namespaces := make([]string, 0, len(seen)) + for ns := range seen { + namespaces = append(namespaces, ns) + } + sort.Strings(namespaces) + return namespaces, wildcard, nil +} diff --git a/internal/git/source_namespaces_test.go b/internal/git/source_namespaces_test.go new file mode 100644 index 00000000..4ab9626a --- /dev/null +++ b/internal/git/source_namespaces_test.go @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + configv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +func sourceNamespaceRule(name, targetName, sourceNamespace string) *configv1alpha3.WatchRule { + return &configv1alpha3.WatchRule{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "shop"}, + Spec: configv1alpha3.WatchRuleSpec{ + TargetRef: configv1alpha3.LocalTargetReference{Name: targetName}, + Rules: []configv1alpha3.ResourceRule{{ + Resources: []string{"configmaps"}, + SourceNamespace: sourceNamespace, + }}, + }, + } +} + +func sourceNamespaceClient(t *testing.T, objects ...client.Object) client.Client { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, configv1alpha3.AddToScheme(scheme)) + return fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build() +} + +// TestResolveSourceNamespaces is the set the one-source-namespace rule is decided on. Every case +// here is a claim docs/layout/model.md makes about it, and the whole set is answerable from the +// config cluster — no scan, no repository state. +func TestResolveSourceNamespaces(t *testing.T) { + target := &configv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "checkout-artifact", Namespace: "shop"}, + } + + t.Run("the target's own namespace is always in the set", func(t *testing.T) { + c := sourceNamespaceClient(t) + namespaces, wildcard, err := resolveSourceNamespaces(t.Context(), c, target) + require.NoError(t, err) + assert.Equal(t, []string{"shop"}, namespaces) + assert.False(t, wildcard) + }) + + t.Run("a rule that names no sourceNamespace watches its own", func(t *testing.T) { + c := sourceNamespaceClient(t, sourceNamespaceRule("content", "checkout-artifact", "")) + namespaces, _, err := resolveSourceNamespaces(t.Context(), c, target) + require.NoError(t, err) + assert.Equal(t, []string{"shop"}, namespaces) + }) + + t.Run("an explicit sourceNamespace is a second namespace", func(t *testing.T) { + c := sourceNamespaceClient(t, + sourceNamespaceRule("content", "checkout-artifact", ""), + sourceNamespaceRule("content-billing", "checkout-artifact", "billing")) + namespaces, _, err := resolveSourceNamespaces(t.Context(), c, target) + require.NoError(t, err) + assert.Equal(t, []string{"billing", "shop"}, namespaces, "sorted, so the message a user reads is stable") + }) + + t.Run("a rule pointing at another target contributes nothing", func(t *testing.T) { + c := sourceNamespaceClient(t, sourceNamespaceRule("elsewhere", "other-artifact", "billing")) + namespaces, _, err := resolveSourceNamespaces(t.Context(), c, target) + require.NoError(t, err) + assert.Equal(t, []string{"shop"}, namespaces) + }) + + t.Run("a wildcard is reported, never enumerated", func(t *testing.T) { + c := sourceNamespaceClient(t, + sourceNamespaceRule("everything", "checkout-artifact", configv1alpha3.SourceNamespaceWildcard)) + namespaces, wildcard, err := resolveSourceNamespaces(t.Context(), c, target) + require.NoError(t, err) + assert.True(t, wildcard, "a wildcard cannot be proven to be one namespace from the spec") + assert.Equal(t, []string{"shop"}, namespaces, "and it adds no name of its own") + }) +} diff --git a/internal/git/types.go b/internal/git/types.go index def5b4b5..7b78866a 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -243,6 +243,12 @@ type ResolvedTargetMetadata struct { // resources are placed beside the folder's one kustomize root, if it has exactly one, // and otherwise at the canonical path. Placement *manifestanalyzer.PlacementPolicy + // Namespaces is the GitTarget's declared namespace behavior — spec.serializeNamespace and the + // source namespaces reaching the target — which decides whether the documents this target + // writes carry metadata.namespace, and which namespace a namespace-free one belongs to. It is + // resolved with the rest of the metadata so a replayed write honours the policy it was planned + // under, exactly as PruneMode and Suspend are. + Namespaces namespacePolicy // PruneMode is the GitTarget's EFFECTIVE spec.prune.mode — always a concrete value, // because it is resolved through EffectivePruneMode and an omitted policy is onEvent. // It gates both deletion paths: the resync mark-and-sweep (through the planner's diff --git a/internal/git/write_boundary_precondition_test.go b/internal/git/write_boundary_precondition_test.go index 76289d0f..8088e743 100644 --- a/internal/git/write_boundary_precondition_test.go +++ b/internal/git/write_boundary_precondition_test.go @@ -146,8 +146,17 @@ func TestFanInPrecondition_RefusesAmbiguousOverrideWriteThrough(t *testing.T) { seedDiamond(t, root) w := &BranchWorker{contentWriter: writer, mapper: deploymentMapper()} - _, err := w.flushEventsToWorktree(context.Background(), worktree, "", - []Event{overridesDeploymentEvent("ghcr.io/example/podinfo:9.9.9", 3)}, nil, configv1alpha3.PruneOnEvent) + _, err := w.flushEventsToWorktree( + context.Background(), + worktree, + "", + []Event{ + overridesDeploymentEvent("ghcr.io/example/podinfo:9.9.9", 3), + }, + nil, + namespacePolicy{}, + configv1alpha3.PruneOnEvent, + ) issues := refusalIssues(t, err) assert.Contains(t, issueKinds(issues), manifestanalyzer.IssueWriteFanIn, "an ambiguous-override write-through must be refused, not written through") diff --git a/internal/manifestanalyzer/acceptance_refusal.go b/internal/manifestanalyzer/acceptance_refusal.go index d33b658e..f7686bac 100644 --- a/internal/manifestanalyzer/acceptance_refusal.go +++ b/internal/manifestanalyzer/acceptance_refusal.go @@ -73,6 +73,10 @@ func GitPathRefusalReason(refused *AcceptanceRefusedError) string { return "IgnoreShadowsManagedPath" case refused.AllIssuesOfKinds(IssueAmbiguousLayout): return "AmbiguousLayout" + case refused.AllIssuesOfKinds(IssueMultipleSourceNamespaces): + return "MultipleSourceNamespaces" + case refused.AllIssuesOfKinds(IssueUnrenderedPlacement): + return "UnrenderedPlacement" case refused.AllIssuesOfKinds( IssueWriteEscapesScope, IssueWriteFanIn, diff --git a/internal/manifestanalyzer/analyzer_test.go b/internal/manifestanalyzer/analyzer_test.go index 2965dc45..a8158749 100644 --- a/internal/manifestanalyzer/analyzer_test.go +++ b/internal/manifestanalyzer/analyzer_test.go @@ -115,6 +115,10 @@ func TestAnalyze_Issues(t *testing.T) { IssueUnsupportedKustomize: 0, IssueRenderDoesNotMatchLive: 0, IssueAmbiguousLayout: 0, + // A configuration fact rather than a folder one: nothing about a tree on disk can make + // two source namespaces reach it, so Analyze never raises it. + IssueMultipleSourceNamespaces: 0, + IssueUnrenderedPlacement: 0, // Foreign-content, ignore-shadow, and the write-boundary refusals are // acceptance-gate / write-plan facts, not part of the structure-only Analyze report, // so they never surface here. IssueRenderRefused is the strongest case of that: it is diff --git a/internal/manifestanalyzer/placement.go b/internal/manifestanalyzer/placement.go index 03cca4c8..9a193540 100644 --- a/internal/manifestanalyzer/placement.go +++ b/internal/manifestanalyzer/placement.go @@ -31,6 +31,10 @@ import ( type PlacementPolicy struct { ByType map[string]string Default string + // UseKustomize is spec.placement.useKustomize: create a kustomization.yaml at the write jail's + // root when nothing governs a new document's path, and register the document in it. It is read + // by the writer rather than by LocateNew, because creating a file is not a path decision. + UseKustomize bool } // PlacementRequest describes a resource with no existing document in Git — the @@ -195,6 +199,10 @@ func LocateNew(store *ManifestStore, policy *PlacementPolicy, req PlacementReque return finishPlacement(store, req, path, PlacementSourceKustomizeRoot) } + if path, ok := resolveDeclaredKustomizeFolder(store, policy, req); ok { + return finishPlacement(store, req, path, PlacementSourceKustomizeRoot) + } + return finishPlacement(store, req, canonicalPath(req), PlacementSourceCanonical) } @@ -229,6 +237,40 @@ func resolveKustomizeRoot(store *ManifestStore, req PlacementRequest) (string, b return cleanJoin(slashDir(only.Path), name), true } +// resolveDeclaredKustomizeFolder is the rung above for a folder that has no root YET. A target +// declaring spec.placement.useKustomize keeps this folder as a kustomize folder, and the writer +// creates the missing root at the jail's own directory in the same commit, so a new document +// belongs beside it exactly as it would beside a root that was already there. +// +// Without this the first document of a bootstrapped folder would land at the canonical +// {namespaceOrCluster}/{group}/{resource}/{name}.yaml path, which is a tree no resources: graph +// reaches: the operator would create a root and then place the document outside it. +// +// It reports the same PlacementSource as the rung it stands in for, because it IS that rung: the +// source label names the mechanism a reader can act on, and "beside the folder's one kustomize +// root" is what happened. The label set is a public observability contract and this adds no member +// to it. +// +// It runs only when the folder has NO writable root: exactly one is the rung above, and several is +// refused before placement is asked (an ambiguous folder has no single root to create beside). +func resolveDeclaredKustomizeFolder( + store *ManifestStore, + policy *PlacementPolicy, + req PlacementRequest, +) (string, bool) { + if policy == nil || !policy.UseKustomize { + return "", false + } + if len(writableRenderRoots(store, req.WriteScope)) != 0 { + return "", false + } + name := req.Identifier.Name + ".yaml" + if req.Sensitive { + name = req.Identifier.Name + ".sops.yaml" + } + return cleanJoin(req.WriteScope, name), true +} + // finishPlacement fills in the parts of a PlacementResult that depend only on the // resolved path (whether it already exists, and whether its directory needs a // kustomize resources: entry), and enforces the "sensitive never appends" rule. @@ -390,6 +432,14 @@ func governingKustomization(store *ManifestStore, writeScope, resolvedPath strin } } +// GoverningKustomization is governingKustomization for the writer, which has to ask a question +// LocateNew's result cannot answer: PlacementResult.Kustomization is set only when a governing root +// exists AND does not already list the path, so a nil there means either "no root" or "already +// listed". Creating a root is only correct for the first of those. +func GoverningKustomization(store *ManifestStore, writeScope, resolvedPath string) *KustomizationInfo { + return governingKustomization(store, writeScope, resolvedPath) +} + func kustomizationListsResource(k *KustomizationInfo, resolvedPath string) bool { dir := slashDir(k.Path) for _, entry := range k.Resources { diff --git a/internal/manifestanalyzer/render_verify.go b/internal/manifestanalyzer/render_verify.go index 7541ec45..f61061c1 100644 --- a/internal/manifestanalyzer/render_verify.go +++ b/internal/manifestanalyzer/render_verify.go @@ -89,6 +89,16 @@ func (e *RenderRefusedError) Error() string { return "kustomize render refused the write: " + strings.Join(e.Reasons, "; ") } +// VerifyOptions is what the write path knows about the GitTarget that the render comparison cannot +// see in the files. +type VerifyOptions struct { + // NamespaceSuppliedDownstream is spec.serializeNamespace: false — the target declared that its + // documents do not encode a deployment namespace, so something outside this repository supplies + // one. It relaxes the comparison for a rendered object that HAS no namespace, and only for + // that object: see compareRoot. + NamespaceSuppliedDownstream bool +} + // VerifyBatchRenders re-renders every root of the subtree twice — as the flush found it, // and as the flush would leave it — and proves both halves of the oracle: // @@ -103,22 +113,38 @@ func (e *RenderRefusedError) Error() string { // // before and after are complete file trees. The cost is two builds per render root, once per // flush, and it is only paid when the flush routed something through a kustomization. -func VerifyBatchRenders(before, after []manifestedit.FileContent, intents []WriteIntent) error { +func VerifyBatchRenders( + before, after []manifestedit.FileContent, + intents []WriteIntent, + opts VerifyOptions, +) error { byKey := make(map[chainKey]WriteIntent, len(intents)) for _, in := range intents { byKey[in.key()] = in } seen := map[chainKey]struct{}{} + // A root the flush CREATED has no before-state to build, and that is not the same fault as a + // root that was there and did not build. spec.placement.useKustomize writes a kustomization + // into a folder that had none, so its baseline render is empty rather than unbuildable — + // which is exactly what "this folder rendered nothing before" means. + rootsBefore := parseKustomizations(before) + var reasons []string for _, root := range renderTargets(parseKustomizations(after)) { - was, err := renderRoot(before, root) - if err != nil { - // The tree did not build BEFORE we touched it. The acceptance gate refuses - // such a folder, so we should never be writing into one — but an unverifiable - // root is not a verified root, so say so rather than skip it. - reasons = append(reasons, fmt.Sprintf("render root %s did not build before the write: %v", root, err)) - continue + var was []renderedObject + _, existed := rootsBefore[root] + if existed { + built, err := renderRoot(before, root) + if err != nil { + // The tree did not build BEFORE we touched it. The acceptance gate refuses + // such a folder, so we should never be writing into one — but an unverifiable + // root is not a verified root, so say so rather than skip it. + reasons = append(reasons, + fmt.Sprintf("render root %s did not build before the write: %v", root, err)) + continue + } + was = built } now, err := renderRoot(after, root) if err != nil { @@ -128,7 +154,9 @@ func VerifyBatchRenders(before, after []manifestedit.FileContent, intents []Writ ) continue } - reasons = append(reasons, compareRoot(root, byKey, seen, renderedByKey(was), renderedByKey(now))...) + reasons = append(reasons, compareRoot( + rootComparison{root: root, existedBefore: existed, opts: opts}, + byKey, seen, renderedByKey(was), renderedByKey(now))...) } for _, in := range intents { @@ -146,13 +174,22 @@ func VerifyBatchRenders(before, after []manifestedit.FileContent, intents []Writ return &RenderRefusedError{Reasons: reasons} } +// rootComparison is which root is being checked and whether it is one this flush CREATED. The +// second half changes what the blast-radius rule can honestly claim; see compareRoot. +type rootComparison struct { + root string + existedBefore bool + opts VerifyOptions +} + // compareRoot checks one render root's before/after pair against the flush's intents. func compareRoot( - root string, + rc rootComparison, intents map[chainKey]WriteIntent, seen map[chainKey]struct{}, was, now map[chainKey]renderedObject, ) []string { + root := rc.root var reasons []string for key := range unionKeys(was, now) { before, existed := was[key] @@ -166,6 +203,16 @@ func compareRoot( case !intended: // The blast radius. This object is nobody's target, so the flush has no // business changing it — in this root or any other. + // + // "Appears" means something different under a root this flush CREATED, and reading it + // as a blast radius would be wrong. spec.placement.useKustomize adopts the folder's + // existing documents into the root it writes, so every one of them renders for the + // first time — not because the flush touched the object (it did not touch a byte of + // those files) but because nothing rendered them before. The check that matters there + // is that the root builds at all, and it is enforced above. + if !rc.existedBefore { + continue + } if !existed || !exists { reasons = append(reasons, fmt.Sprintf( "the write adds or removes %s/%s in render root %s, which it never set out to write", @@ -190,7 +237,7 @@ func compareRoot( case !exists: reasons = append(reasons, fmt.Sprintf( "%s/%s was written, but render root %s no longer renders it", key.kind, key.name, root)) - case !sameObject(after.Object, intent.Desired): + case !rc.rendersAsIntended(after.Object, intent.Desired): reasons = append(reasons, fmt.Sprintf( "in render root %s, %s/%s (from %s) does not render to the live object after the write", root, key.kind, key.name, key.originPath)) @@ -199,6 +246,38 @@ func compareRoot( return reasons } +// rendersAsIntended compares what the folder renders with the live object the write meant to +// mirror, and holds the one place the namespace is allowed not to match. +// +// The relaxation is scoped by the RENDER, not by the setting alone. spec.serializeNamespace: false +// says the artifact does not encode its deployment namespace, so when the render agrees — the +// object comes out with no namespace, because no kustomization in its chain supplies one — the +// difference against a live object in some namespace is the shape working as declared, and every +// other field is still compared. +// +// When the render DOES produce a namespace, some kustomization set one, and that is a concrete +// claim the folder makes: a root declaring `namespace: shop` rendering a live `billing` object is a +// relocation, which is exactly the failure this gate exists to catch. So the namespace is compared +// there whatever the target declared. +// +// Asking the render rather than re-reading the kustomization files is deliberate: the chain can +// have several roots and kustomize decides which transformer wins, so the built output is the only +// answer that cannot disagree with what will actually be applied. +func (rc rootComparison) rendersAsIntended(rendered, desired *unstructured.Unstructured) bool { + if sameObject(rendered, desired) { + return true + } + if !rc.opts.NamespaceSuppliedDownstream || rendered == nil || desired == nil { + return false + } + if rendered.GetNamespace() != "" { + return false + } + withoutNamespace := desired.DeepCopy() + withoutNamespace.SetNamespace("") + return sameObject(rendered, withoutNamespace) +} + func unionKeys(a, b map[chainKey]renderedObject) map[chainKey]struct{} { out := make(map[chainKey]struct{}, len(a)+len(b)) for k := range a { diff --git a/internal/manifestanalyzer/solvable_test.go b/internal/manifestanalyzer/solvable_test.go index f229ad71..d5f185ab 100644 --- a/internal/manifestanalyzer/solvable_test.go +++ b/internal/manifestanalyzer/solvable_test.go @@ -45,18 +45,25 @@ import ( // whose answer depends on the branch that raised it lists every branch it can produce, so // this file never claims a single answer where the code has two. var classificationByKind = map[IssueKind][]Classification{ - IssueInvalidYAML: {{Solvable: true, Actor: ActorRepositoryAuthor}}, - IssueDuplicate: {{Solvable: true, Actor: ActorRepositoryAuthor}}, - IssueImpureManagedFile: {{Solvable: true, Actor: ActorRepositoryAuthor}}, - IssueMixedFile: {{Solvable: true, Actor: ActorRepositoryAuthor}}, - IssueIgnoreShadowsManaged: {{Solvable: true, Actor: ActorRepositoryAuthor}}, - IssueNonKRM: {{Solvable: true, Actor: ActorRepositoryAuthor}}, - IssueForeignFile: {{Solvable: true, Actor: ActorRepositoryAuthor}}, - IssueForeignSymlink: {{Solvable: true, Actor: ActorRepositoryAuthor}}, - IssueForeignSubmodule: {{Solvable: true, Actor: ActorRepositoryAuthor}}, - IssueOutOfScope: {{Solvable: true, Actor: ActorPlatformOperator}}, - IssueWriteEscapesScope: {{Solvable: true, Actor: ActorPlatformOperator}}, - IssueAmbiguousLayout: {{Solvable: true, Actor: ActorPlatformOperator}}, + IssueInvalidYAML: {{Solvable: true, Actor: ActorRepositoryAuthor}}, + IssueDuplicate: {{Solvable: true, Actor: ActorRepositoryAuthor}}, + IssueImpureManagedFile: {{Solvable: true, Actor: ActorRepositoryAuthor}}, + IssueMixedFile: {{Solvable: true, Actor: ActorRepositoryAuthor}}, + IssueIgnoreShadowsManaged: {{Solvable: true, Actor: ActorRepositoryAuthor}}, + IssueNonKRM: {{Solvable: true, Actor: ActorRepositoryAuthor}}, + IssueForeignFile: {{Solvable: true, Actor: ActorRepositoryAuthor}}, + IssueForeignSymlink: {{Solvable: true, Actor: ActorRepositoryAuthor}}, + IssueForeignSubmodule: {{Solvable: true, Actor: ActorRepositoryAuthor}}, + IssueOutOfScope: {{Solvable: true, Actor: ActorPlatformOperator}}, + IssueWriteEscapesScope: {{Solvable: true, Actor: ActorPlatformOperator}}, + IssueAmbiguousLayout: {{Solvable: true, Actor: ActorPlatformOperator}}, + // The only kind here that is not about the repository at all: the folder is fine and the + // configuration pointed at it is not, so the actor is the one who owns both objects the fix + // touches (the GitTarget and the WatchRule). + IssueMultipleSourceNamespaces: {{Solvable: true, Actor: ActorPlatformOperator}}, + // Same shape: the folder is fine, the template aimed at it is not, and the platform operator + // owns the template. + IssueUnrenderedPlacement: {{Solvable: true, Actor: ActorPlatformOperator}}, IssueRenderDoesNotMatchLive: {{Solvable: true, Actor: ActorPlatformOperator}}, IssueWriteFanIn: {{Solvable: false}}, IssueUnplaceableEdit: {{Solvable: false}}, diff --git a/internal/manifestanalyzer/source_namespace_fence.go b/internal/manifestanalyzer/source_namespace_fence.go new file mode 100644 index 00000000..c13726af --- /dev/null +++ b/internal/manifestanalyzer/source_namespace_fence.go @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "fmt" +) + +// IssueMultipleSourceNamespaces marks a GitTarget that declared its folder namespace-free +// (spec.serializeNamespace: false) and that more than one source namespace reaches. +// +// Unlike every other kind in this package it is a property of the CONFIGURATION rather than of the +// observed folder: it is decided from the GitTarget and the WatchRules naming it, needs no scan and +// no repository state, and is raised here only because this is where the writer's refusals and +// their status reasons live together. +const IssueMultipleSourceNamespaces IssueKind = "multiple-source-namespaces" + +// MultipleSourceNamespacesRefusal is the write-plan precondition behind +// docs/layout/model.md § "The second guard": an explicit serializeNamespace: false admits exactly +// one source namespace, and the second is refused. +// +// What follows from two is not a collision but a MATCH. shop/config and billing/config both resolve +// to a config.yaml whose bytes carry no namespace, so their manifest identities are equal, the +// bundling rule never fires, and each write flips one document between two live objects. Everywhere +// else in this model losing a distinction produces a refusal or a bundle; only here does it produce +// a match, which is why this guard refuses where the others report. +// +// It is derived from one setting rather than inferred from two, so it needs no field of its own. +// The path plays no part either: a deployer applies bytes rather than filenames, so the rule keys +// on what serializeNamespace means and not on whether the template happens to omit {namespace}. +// +// Only an EXPLICIT false is fenced. Inference is never constrained by it: a tree of nested roots is +// legitimately multi-namespace AND namespace-free in its documents, and it is exactly the case +// unset exists for, so the refusal costs that user nothing beyond the setting that was already +// correct for them. +// +// wildcard says a rule names "*". It is refused without enumerating anything, because neither +// reading of "*" can be proven to be one namespace from the spec alone. +// +// It returns no issue for a target that is not fenced, so a caller can raise it unconditionally. +func MultipleSourceNamespacesRefusal( + declaredNamespaceFree bool, + namespaces []string, + wildcard bool, + specPath string, +) []AcceptanceIssue { + if !declaredNamespaceFree || (!wildcard && len(namespaces) <= 1) { + return nil + } + reach := fmt.Sprintf("%v reach this target", namespaces) + if wildcard { + reach = fmt.Sprintf( + "a rule names sourceNamespace %q, which cannot be shown to be one namespace", SourceNamespaceWildcard) + } + return []AcceptanceIssue{{ + Kind: IssueMultipleSourceNamespaces, + // Every path in a refusal is relative to the write jail, so the folder's own name is ".". + Path: orDot(specPath), + Message: fmt.Sprintf( + "spec.serializeNamespace is false, which admits exactly one source namespace, but %s; "+ + "set serializeNamespace to unset so each document takes the namespace of the root "+ + "governing it, or split the target", reach), + // The PLATFORM OPERATOR fixes it: the remedy is a GitTarget or WatchRule edit, and both are + // this actor's own objects. Nothing in the repository is wrong. + Solvable: true, + Actor: ActorPlatformOperator, + }} +} + +// SourceNamespaceWildcard is the "every namespace" spelling of a rule's sourceNamespace. It is +// duplicated from api/v1alpha3 rather than imported, because this package is deliberately free of +// any Kubernetes API type dependency; the value is part of the CRD's user-facing contract and +// changing it would be a breaking API change either way. +const SourceNamespaceWildcard = "*" + +// IssueUnrenderedPlacement marks a new document a kustomize folder would hold but never render: +// spec.placement.useKustomize declares the operator maintains this folder's root, and the path the +// document resolved to is governed by no resources: list. +// +// Like IssueMultipleSourceNamespaces it is not a property of the folder's CONTENT — the folder is +// perfectly good kustomize — but of the configuration aimed at it, and it is raised at the write +// because only the write knows where the document was about to land. +const IssueUnrenderedPlacement IssueKind = "unrendered-placement" + +// UnrenderedPlacementRefusal refuses a placement that would commit a document nothing renders. +// +// It arises in one shape. The folder already has a render root, a byType or default template puts +// the new document outside it, and creating a second root is not available: two render roots is +// Ambiguous, and an ambiguous folder stops placing new documents at all. What is left is to write +// the file unrendered or to refuse, and under useKustomize the target has declared this folder is a +// kustomize folder — a file no resources: list names is one that looks mirrored in Git and is +// applied by nothing, which is the failure #295 was and #319 made an invariant. +// +// It is raised ONLY under useKustomize. A target that made no claim about kustomize keeps today's +// behavior, so this is not a new refusal for folders that never asked for one. +func UnrenderedPlacementRefusal(useKustomize, governed bool, resolvedPath, renderRoot string) []AcceptanceIssue { + if !useKustomize || governed { + return nil + } + return []AcceptanceIssue{{ + Kind: IssueUnrenderedPlacement, + Path: resolvedPath, + Message: fmt.Sprintf( + "placement.useKustomize is set, but %q is governed by no kustomization: render root %q is "+ + "already there and does not reach it, and a second root would make the folder cover two "+ + "render roots. Place the document inside that root, or point the GitTarget at the folder "+ + "the root governs", + resolvedPath, renderRoot), + // The PLATFORM OPERATOR fixes it: the remedy is the target's own template or path. + Solvable: true, + Actor: ActorPlatformOperator, + }} +} diff --git a/internal/manifestanalyzer/store.go b/internal/manifestanalyzer/store.go index 43189fb8..de460cc8 100644 --- a/internal/manifestanalyzer/store.go +++ b/internal/manifestanalyzer/store.go @@ -281,11 +281,24 @@ const ( // than guessed; an ambiguous case also emits a reasonAmbiguousNamespace diagnostic // for the repository-validity layer. NamespaceNone NamespaceSourceKind = "None" + // NamespaceDeclared means metadata.namespace is absent from the file and no kustomization + // supplies one, but the GitTarget DECLARED the folder namespace-free + // (spec.serializeNamespace: false) and exactly one source namespace reaches it, so that + // namespace is the document's. Like Kustomize, the namespace must stay out of the file and + // the document is located in the bytes by a namespace-less identity. + // + // It is what keeps a namespace-free folder mirrorable at all. Without it the operator writes + // shop/config as a namespace-less document, reads it back as belonging to no namespace, and + // the NEXT write of the same object matches nothing and appends a second copy of it. The + // one-source-namespace refusal is what makes the attribution safe: with two namespaces + // reaching the folder there is no single answer, and the write is refused rather than guessed. + NamespaceDeclared NamespaceSourceKind = "Declared" ) // NamespaceSource records where a document's effective namespace came from. Kind // drives the one write-time decision the live writer makes (keep metadata.namespace -// out of the file and locate by raw identity only when Kind is Kustomize); Path is the +// out of the file and locate by raw identity only when the namespace came from +// somewhere other than the file — see DocumentModel.NamespaceAbsentFromFile); Path is the // kustomization file that supplied the namespace, set only for NamespaceKustomize. type NamespaceSource struct { Kind NamespaceSourceKind @@ -300,6 +313,15 @@ func (dm *DocumentModel) NamespaceInheritedFromContext() bool { return dm.NamespaceSource.Kind == NamespaceKustomize } +// NamespaceAbsentFromFile reports whether the document's namespace is NOT in its own bytes — it +// came from a kustomization's namespace: transformer, or from the GitTarget's declaration that +// this folder's documents carry none. The writer uses it for the two decisions that follow from +// the bytes rather than from any setting: keep metadata.namespace out of the file on an update, +// and locate the document by its raw (namespace-less) identity. +func (dm *DocumentModel) NamespaceAbsentFromFile() bool { + return dm.NamespaceSource.Kind == NamespaceKustomize || dm.NamespaceSource.Kind == NamespaceDeclared +} + // MappingOutcome records why a document's ResourceIdentity is or is not set, derived // from the followability registry. It is the analyzer's view of the single // followability question — there is no status vocabulary to interpret, only three @@ -410,7 +432,12 @@ func buildStore( scan FolderScan, lookup typeset.Lookup, allowlist Allowlist, + opts ...StoreOption, ) *ManifestStore { + var settings storeSettings + for _, opt := range opts { + opt(&settings) + } if lookup == nil { // A nil lookup is the structure-only mode: an unpublished registry is never // ready, so it judges nothing. @@ -449,11 +476,12 @@ func buildStore( } hasNamedRecord := store.materializeRecords(ctx, inv.Records, materializeInputs{ - lookup: lookup, - allowlist: allowlist, - patchFiles: patchFilesOf(kusts, reachedResourceFiles(kusts)), - nsAssignments: nsAssignments, - ovAssignments: ovAssignments, + lookup: lookup, + allowlist: allowlist, + patchFiles: patchFilesOf(kusts, reachedResourceFiles(kusts)), + nsAssignments: nsAssignments, + ovAssignments: ovAssignments, + declaredNamespace: settings.declaredNamespace, }) // Record every allowlisted file with no named record as a whole-file retention, @@ -522,8 +550,27 @@ func BuildStoreFromScan( scan FolderScan, lookup typeset.Lookup, allowlist Allowlist, + opts ...StoreOption, ) *ManifestStore { - return buildStore(ctx, scan, lookup, allowlist) + return buildStore(ctx, scan, lookup, allowlist, opts...) +} + +// StoreOption is a scan-wide fact the store cannot read out of the repository, supplied by the +// caller that knows it. There is one today, and it exists because the GitTarget's declaration is +// not in the folder: see WithDeclaredNamespace. +type StoreOption func(*storeSettings) + +type storeSettings struct { + declaredNamespace string +} + +// WithDeclaredNamespace attributes every namespace-less document that no kustomization governs to +// ns. Pass it only when the GitTarget declares spec.serializeNamespace: false AND exactly one +// source namespace reaches it — the two conditions together are what make the attribution a fact +// rather than a guess, and the write path refuses the second namespace precisely so this stays +// true. An empty ns is the default: attribute nothing. +func WithDeclaredNamespace(ns string) StoreOption { + return func(st *storeSettings) { st.declaredNamespace = ns } } // DocumentLocations returns the (file path, document index) of every managed @@ -557,6 +604,9 @@ type materializeInputs struct { patchFiles map[string]struct{} nsAssignments map[string]namespaceAssignment ovAssignments map[chainKey]*overrideAssignment + // declaredNamespace is the GitTarget's declared single source namespace, set only for a + // target that declared the folder namespace-free. See WithDeclaredNamespace. + declaredNamespace string } // materializeRecords sorts every KRM document into one of three fates — retained as a build @@ -600,7 +650,7 @@ func (s *ManifestStore) materializeRecords( } retained[r.Location.Path] = true default: - s.materialize(ctx, r, in.lookup, in.nsAssignments, in.ovAssignments) + s.materialize(ctx, r, in.lookup, in.nsAssignments, in.ovAssignments, in.declaredNamespace) } } return retained @@ -614,9 +664,11 @@ func (s *ManifestStore) materialize( lookup typeset.Lookup, nsAssignments map[string]namespaceAssignment, ovAssignments map[chainKey]*overrideAssignment, + declaredNamespace string, ) { gvk := gvkOf(r.Identity) - identity, nsSource, diag := resolveNamespaceContext(ctx, r.Identity, gvk, lookup, r.Location, nsAssignments) + identity, nsSource, diag := resolveNamespaceContext( + ctx, r.Identity, gvk, lookup, r.Location, nsAssignments, declaredNamespace) if diag != nil { s.Diagnostics = append(s.Diagnostics, *diag) } @@ -695,7 +747,9 @@ func sortRetained(retained []RetainedDocument) { // resources graph: exactly one assigning namespace is inherited (Kustomize); zero or // conflicting assignments leave the document namespace-less (None), with an ambiguity // diagnostic in the conflict case. It never guesses by filesystem proximity, so a file -// is only given a namespace by a kustomization that actually references it. +// is only given a namespace by a kustomization that actually references it. declaredNamespace is +// the one exception, and it comes from the GitTarget rather than the folder: see +// WithDeclaredNamespace. func resolveNamespaceContext( ctx context.Context, id manifestedit.Identity, @@ -703,6 +757,7 @@ func resolveNamespaceContext( lookup typeset.Lookup, loc manifestedit.Location, assignments map[string]namespaceAssignment, + declaredNamespace string, ) (manifestedit.Identity, NamespaceSource, *manifestedit.Diagnostic) { if id.Namespace != "" { return id, NamespaceSource{Kind: NamespaceExplicit}, nil @@ -719,6 +774,13 @@ func resolveNamespaceContext( a := assignments[filepathToSlash(loc.Path)] switch len(a.namespaces) { case 0: + // Nothing in the folder supplies a namespace. A GitTarget that declared the folder + // namespace-free supplies one from outside it; without that declaration the document is + // left namespace-less rather than guessed. + if declaredNamespace != "" { + id.Namespace = declaredNamespace + return id, NamespaceSource{Kind: NamespaceDeclared}, nil + } return id, NamespaceSource{Kind: NamespaceNone}, nil case 1: ns := a.namespaces[0] diff --git a/internal/manifestanalyzer/store_test.go b/internal/manifestanalyzer/store_test.go index 5fce0eb2..d9e1c488 100644 --- a/internal/manifestanalyzer/store_test.go +++ b/internal/manifestanalyzer/store_test.go @@ -584,3 +584,48 @@ func keysOf(m map[string]*FileModel) []string { } return out } + +// TestBuildStore_DeclaredNamespaceAttributesNamespaceLessDocuments pins WithDeclaredNamespace: the +// GitTarget declared its folder namespace-free, so a namespace-less document that no kustomization +// governs belongs to the one source namespace reaching the target rather than to no namespace at +// all. Without it the operator cannot find the documents it wrote itself. +// +// A kustomization's namespace: still wins where there is one — the declaration says where the +// namespace comes from when the folder supplies none, not that the folder is ignored. +func TestBuildStore_DeclaredNamespaceAttributesNamespaceLessDocuments(t *testing.T) { + mapper := typeset.NewSnapshotRegistry(sampleClusterSnapshot()) + files := []manifestedit.FileContent{ + {Path: "app.yaml", Content: []byte("apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: app\n")}, + {Path: "own.yaml", Content: []byte( + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: own\n namespace: other\n")}, + } + scan := FolderScan{YAMLFiles: files} + + plain := BuildStoreFromScan(context.Background(), scan, mapper, Allowlist{}) + if plain.ByManifestIdentity[manifestedit.Identity{ + APIVersion: "v1", Kind: "ConfigMap", Namespace: "shop", Name: "app"}] != nil { + t.Fatalf("without a declaration a namespace-less document belongs to no namespace") + } + + declared := BuildStoreFromScan(context.Background(), scan, mapper, Allowlist{}, WithDeclaredNamespace("shop")) + app := declared.ByManifestIdentity[manifestedit.Identity{ + APIVersion: "v1", Kind: "ConfigMap", Namespace: "shop", Name: "app"}] + if app == nil { + t.Fatalf("a declared namespace-free folder attributes its namespace-less documents to shop") + } + if app.NamespaceSource.Kind != NamespaceDeclared { + t.Errorf("app NamespaceSource = %+v, want Declared", app.NamespaceSource) + } + if !app.NamespaceAbsentFromFile() { + t.Errorf("a declared namespace is not in the file, so the writer must keep it out") + } + if app.NamespaceInheritedFromContext() { + t.Errorf("a declared namespace does not come from build context") + } + + own := declared.ByManifestIdentity[manifestedit.Identity{ + APIVersion: "v1", Kind: "ConfigMap", Namespace: "other", Name: "own"}] + if own == nil || own.NamespaceSource.Kind != NamespaceExplicit { + t.Errorf("a document that names its own namespace keeps it: %+v", own) + } +} diff --git a/internal/webhook/validate_operator_types_handler.go b/internal/webhook/validate_operator_types_handler.go index 6637876f..ef09336b 100644 --- a/internal/webhook/validate_operator_types_handler.go +++ b/internal/webhook/validate_operator_types_handler.go @@ -10,6 +10,7 @@ import ( authnv1 "k8s.io/api/authentication/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" @@ -55,29 +56,44 @@ type CommandAuthorRecorder interface { RecordCommandAuthor(ctx context.Context, uid types.UID, author queue.CommandAuthor) error } -// ValidateOperatorTypesHandler is the admission handler for our operator CRDs. Today it -// does one thing: for a command kind (a CommitRequest) it captures the authenticated -// submitter into the CommandAuthorStore and always allows — pure observation with a -// single side effect (a Redis upsert), never a rejection, so a user's command never -// depends on it succeeding (a missed capture leaves the request without a claimed actor; -// see docs/spec/commitrequest-admission-authorship.md). It dispatches on the -// resource (isCommandKind today), so a future config-validation branch for non-command -// kinds slots in alongside without disturbing this one. +// ValidateOperatorTypesHandler is the admission handler for our operator CRDs. It dispatches on the +// resource under review and does two unrelated things: +// +// - For a COMMAND kind (a CommitRequest) it captures the authenticated submitter into the +// CommandAuthorStore and always allows — pure observation with a single side effect (a Redis +// upsert), never a rejection, so a user's command never depends on it succeeding (a missed +// capture leaves the request without a claimed actor; see +// docs/spec/commitrequest-admission-authorship.md). +// - For a WatchRule it validates the one cross-object rule admission can usefully give feedback +// on: a second source namespace against a GitTarget that declared its folder namespace-free. +// That one CAN reject, and it is the only thing here that does. +// +// The two branches share an endpoint and nothing else. Validation runs on dry-run (it has no side +// effects, and a server-side dry-run must report the rejection it would get), while the capture +// branch honours NoneOnDryRun. type ValidateOperatorTypesHandler struct { Store CommandAuthorRecorder + // Client reads the GitTarget a WatchRule names, and that target's other rules. Nil leaves the + // WatchRule branch evaluating nothing and allowing, which is the same degradation the + // fail-open failurePolicy already provides. + Client client.Client } -// Handle records {uid → author} for an admitted command CREATE before the object -// persists (the authorship invariant, §2), then allows. Every early return still -// allows: a non-command kind, a dry-run, a missing uid, or an unauthenticated request -// simply records nothing and leaves the request without a claimed actor downstream. +// Handle dispatches on the resource under review: a WatchRule is validated (and may be rejected), +// and a command CREATE has its {uid → author} recorded before the object persists (the authorship +// invariant, §2) and is then allowed. Every early return on the command path still allows: an +// unrecognised kind, a dry-run, a missing uid, or an unauthenticated request simply records nothing +// and leaves the request without a claimed actor downstream. func (h *ValidateOperatorTypesHandler) Handle(ctx context.Context, req admission.Request) admission.Response { log := logf.FromContext(ctx).WithName("validate-operator-types") gr := metav1.GroupResource{Group: req.Resource.Group, Resource: req.Resource.Resource} + if isWatchRuleKind(gr) { + return h.validateWatchRuleSourceNamespaces(ctx, req) + } if !isCommandKind(gr) { - // Belt-and-suspenders; the webhook rules already scope us to command kinds. - return admission.Allowed("not a command kind") + // Belt-and-suspenders; the webhook rules already scope us to the kinds above. + return admission.Allowed("not a kind this endpoint judges") } // Dry-run never persists, so the controller will never read this record — and we // declare sideEffects: NoneOnDryRun, so we must honor it. diff --git a/internal/webhook/watchrule_source_namespace_admission.go b/internal/webhook/watchrule_source_namespace_admission.go new file mode 100644 index 00000000..c9dbde46 --- /dev/null +++ b/internal/webhook/watchrule_source_namespace_admission.go @@ -0,0 +1,138 @@ +// SPDX-License-Identifier: Apache-2.0 + +package webhook + +import ( + "context" + "encoding/json" + "fmt" + "sort" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8stypes "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +// isWatchRuleKind reports whether gr is the namespaced WatchRule. ClusterWatchRule is deliberately +// absent: it watches cluster-scoped resources, which have no namespace to bring to a target. +func isWatchRuleKind(gr metav1.GroupResource) bool { + return gr == metav1.GroupResource{Group: "configbutler.ai", Resource: "watchrules"} +} + +// validateWatchRuleSourceNamespaces is the FEEDBACK half of the one-source-namespace rule +// (docs/layout/model.md § "The second guard"): a GitTarget that declared its folder namespace-free +// admits exactly one source namespace, and a rule bringing a second one is rejected here, at the +// moment the mistake is made, instead of leaving a target that refuses every write until someone +// reads its status. +// +// It is not the rule's enforcement. The writer's own precondition is, and it holds whatever this +// returns: this check is one-shot, so it cannot see a serializeNamespace flipped to false after the +// rules were created, the webhook is fail-open (failurePolicy: Ignore), and a cluster can run the +// operator without the admission server at all. Everything that must be true of the bytes is +// decided at the write. See docs/spec/where-validation-lives.md. +// +// Every failure to evaluate ALLOWS, for the same reason: a rejection this handler cannot justify +// is a rejection of the user's object on the strength of an unread GitTarget. +func (h *ValidateOperatorTypesHandler) validateWatchRuleSourceNamespaces( + ctx context.Context, + req admission.Request, +) admission.Response { + log := logf.FromContext(ctx).WithName("watchrule-source-namespaces") + if h.Client == nil { + return admission.Allowed("no client: source namespaces not evaluated") + } + + var rule v1alpha3.WatchRule + if err := json.Unmarshal(req.Object.Raw, &rule); err != nil { + log.Error(err, "could not decode the WatchRule under review", "namespace", req.Namespace, "name", req.Name) + return admission.Allowed("undecodable: source namespaces not evaluated") + } + // A CREATE carries no namespace on the object itself until the API server defaults it. + if rule.Namespace == "" { + rule.Namespace = req.Namespace + } + + var target v1alpha3.GitTarget + targetKey := k8stypes.NamespacedName{Namespace: rule.Namespace, Name: rule.Spec.TargetRef.Name} + if err := h.Client.Get(ctx, targetKey, &target); err != nil { + if !apierrors.IsNotFound(err) { + log.Error(err, "could not read the GitTarget this rule names", "gitTarget", targetKey) + } + // A rule naming a target that does not exist yet is ordinary: rules and targets are + // applied in whatever order the manifest happens to list them, and the reconciler holds + // such a rule unready. It is not this check's business. + return admission.Allowed("GitTarget unreadable: source namespaces not evaluated") + } + if target.Spec.SerializeNamespace == nil || *target.Spec.SerializeNamespace { + return admission.Allowed("the target declares no namespace-free folder") + } + + admitted, err := h.admittedSourceNamespaces(ctx, &target, rule.Name) + if err != nil { + log.Error(err, "could not list the sibling WatchRules", "gitTarget", targetKey) + return admission.Allowed("sibling rules unreadable: source namespaces not evaluated") + } + + for _, item := range rule.Spec.Rules { + if item.IsSourceNamespaceWildcard() { + return admission.Denied(fmt.Sprintf( + "GitTarget %s/%s sets spec.serializeNamespace: false, which admits exactly one source "+ + "namespace, and sourceNamespace %q cannot be shown to be one namespace", + target.Namespace, target.Name, v1alpha3.SourceNamespaceWildcard)) + } + admitted[item.EffectiveSourceNamespace(rule.Namespace)] = struct{}{} + } + if len(admitted) <= 1 { + return admission.Allowed("one source namespace") + } + + names := make([]string, 0, len(admitted)) + for ns := range admitted { + names = append(names, ns) + } + sort.Strings(names) + return admission.Denied(fmt.Sprintf( + "GitTarget %s/%s sets spec.serializeNamespace: false, which admits exactly one source namespace, "+ + "but this rule would make %v reach it; the documents it writes carry no metadata.namespace, so "+ + "two namespaces produce one document two objects overwrite in turn. Unset serializeNamespace on "+ + "the target so each document takes the namespace of the root governing it, or point this rule at "+ + "a target of its own", + target.Namespace, target.Name, names)) +} + +// admittedSourceNamespaces is the set already reaching the target, excluding the rule under review +// so an UPDATE is judged on what it would become rather than on what it is. It mirrors +// (internal/git).resolveSourceNamespaces, which computes the same set at the write. +// +// A wildcard among the OTHER rules is not this request's fault, so it adds nothing here: the write +// refuses that target anyway, and rejecting an unrelated edit for it would leave the user unable to +// fix the rule that caused it. +func (h *ValidateOperatorTypesHandler) admittedSourceNamespaces( + ctx context.Context, + target *v1alpha3.GitTarget, + excludeRule string, +) (map[string]struct{}, error) { + var rules v1alpha3.WatchRuleList + if err := h.Client.List(ctx, &rules, client.InNamespace(target.Namespace)); err != nil { + return nil, err + } + admitted := map[string]struct{}{target.Namespace: {}} + for i := range rules.Items { + sibling := &rules.Items[i] + if sibling.Name == excludeRule || sibling.Spec.TargetRef.Name != target.Name { + continue + } + for _, item := range sibling.Spec.Rules { + if item.IsSourceNamespaceWildcard() { + continue + } + admitted[item.EffectiveSourceNamespace(sibling.Namespace)] = struct{}{} + } + } + return admitted, nil +} diff --git a/internal/webhook/watchrule_source_namespace_admission_test.go b/internal/webhook/watchrule_source_namespace_admission_test.go new file mode 100644 index 00000000..b05d89cb --- /dev/null +++ b/internal/webhook/watchrule_source_namespace_admission_test.go @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: Apache-2.0 + +package webhook + +import ( + "encoding/json" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + admissionv1 "k8s.io/api/admission/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + ctrladmission "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +// The feedback half of the one-source-namespace rule. Everything asserted here is a rejection or a +// deliberate refusal to judge; the rule itself is enforced at the write, and +// internal/git asserts that half. + +func namespaceFreeTarget(serialize *bool) *v1alpha3.GitTarget { + return &v1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "checkout-artifact", Namespace: "shop"}, + Spec: v1alpha3.GitTargetSpec{ + Path: "apps/checkout", + SerializeNamespace: serialize, + }, + } +} + +func watchRuleFor(name, sourceNamespace string) *v1alpha3.WatchRule { + return &v1alpha3.WatchRule{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "shop"}, + Spec: v1alpha3.WatchRuleSpec{ + TargetRef: v1alpha3.LocalTargetReference{Name: "checkout-artifact"}, + Rules: []v1alpha3.ResourceRule{{ + Resources: []string{"configmaps"}, + SourceNamespace: sourceNamespace, + }}, + }, + } +} + +func watchRuleReview(t *testing.T, rule *v1alpha3.WatchRule, operation admissionv1.Operation) ctrladmission.Request { + t.Helper() + raw, err := json.Marshal(rule) + require.NoError(t, err) + return ctrladmission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Resource: metav1.GroupVersionResource{ + Group: "configbutler.ai", Version: "v1alpha3", Resource: "watchrules"}, + Operation: operation, + Namespace: rule.Namespace, + Name: rule.Name, + Object: runtime.RawExtension{Raw: raw}, + }, + } +} + +func watchRuleHandler(t *testing.T, objects ...client.Object) *ValidateOperatorTypesHandler { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, v1alpha3.AddToScheme(scheme)) + return &ValidateOperatorTypesHandler{ + Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(objects...).Build(), + } +} + +func TestWatchRuleAdmission_SecondSourceNamespaceIsRejected(t *testing.T) { + no := false + handler := watchRuleHandler(t, namespaceFreeTarget(&no), watchRuleFor("content", "")) + + response := handler.Handle(t.Context(), + watchRuleReview(t, watchRuleFor("content-billing", "billing"), admissionv1.Create)) + + require.False(t, response.Allowed) + assert.Contains(t, response.Result.Message, "admits exactly one source namespace") + assert.Contains(t, response.Result.Message, "[billing shop]") +} + +func TestWatchRuleAdmission_FirstSourceNamespaceIsAdmitted(t *testing.T) { + no := false + handler := watchRuleHandler(t, namespaceFreeTarget(&no)) + + response := handler.Handle(t.Context(), watchRuleReview(t, watchRuleFor("content", ""), admissionv1.Create)) + + assert.True(t, response.Allowed, "the target's own namespace is the one namespace it admits") +} + +// A wildcard is rejected statically: neither reading of "*" can be shown to be one namespace, so +// nothing is enumerated to find out. +func TestWatchRuleAdmission_WildcardIsRejectedWithoutEnumerating(t *testing.T) { + no := false + handler := watchRuleHandler(t, namespaceFreeTarget(&no)) + + response := handler.Handle(t.Context(), + watchRuleReview(t, watchRuleFor("everything", v1alpha3.SourceNamespaceWildcard), admissionv1.Create)) + + require.False(t, response.Allowed) + assert.Contains(t, response.Result.Message, "cannot be shown to be one namespace") +} + +// An UPDATE is judged on what the rule would BECOME. Re-submitting the rule that already holds the +// second namespace must not be rejected for its own existing value, or the only edit that could fix +// it would be blocked by it. +func TestWatchRuleAdmission_UpdateExcludesTheRuleUnderReview(t *testing.T) { + no := false + existing := watchRuleFor("content", "billing") + handler := watchRuleHandler(t, namespaceFreeTarget(&no), existing) + + narrowed := watchRuleFor("content", "shop") + response := handler.Handle(t.Context(), watchRuleReview(t, narrowed, admissionv1.Update)) + + assert.True(t, response.Allowed, "an edit that narrows the rule back to one namespace must land") +} + +func TestWatchRuleAdmission_UnfencedTargetsAreNotJudged(t *testing.T) { + yes := true + for _, tc := range []struct { + name string + serialize *bool + }{ + {"unset infers per document and is never fenced", nil}, + {"true means every document carries its own namespace", &yes}, + } { + t.Run(tc.name, func(t *testing.T) { + handler := watchRuleHandler(t, namespaceFreeTarget(tc.serialize), watchRuleFor("content", "")) + + response := handler.Handle(t.Context(), + watchRuleReview(t, watchRuleFor("content-billing", "billing"), admissionv1.Create)) + + assert.True(t, response.Allowed) + }) + } +} + +// Every way of failing to evaluate ALLOWS. A rejection this handler cannot justify is a rejection +// of the user's object on the strength of a GitTarget nobody read. +func TestWatchRuleAdmission_UnevaluatableRequestsAreAllowed(t *testing.T) { + no := false + + t.Run("no client", func(t *testing.T) { + handler := &ValidateOperatorTypesHandler{} + response := handler.Handle(t.Context(), + watchRuleReview(t, watchRuleFor("content-billing", "billing"), admissionv1.Create)) + assert.True(t, response.Allowed) + }) + + t.Run("the GitTarget does not exist yet", func(t *testing.T) { + handler := watchRuleHandler(t) + response := handler.Handle(t.Context(), + watchRuleReview(t, watchRuleFor("content-billing", "billing"), admissionv1.Create)) + assert.True(t, response.Allowed, "rules and targets are applied in whatever order the manifest lists them") + }) + + t.Run("an undecodable object", func(t *testing.T) { + handler := watchRuleHandler(t, namespaceFreeTarget(&no)) + review := watchRuleReview(t, watchRuleFor("content", ""), admissionv1.Create) + review.Object.Raw = []byte("{not json") + assert.True(t, handler.Handle(t.Context(), review).Allowed) + }) +} + +// A rule pointing at a different GitTarget contributes nothing, so a target fenced to one namespace +// is not rejected because of somebody else's rule. +func TestWatchRuleAdmission_OtherTargetsRulesAreNotCounted(t *testing.T) { + no := false + elsewhere := watchRuleFor("elsewhere", "billing") + elsewhere.Spec.TargetRef.Name = "other-artifact" + handler := watchRuleHandler(t, namespaceFreeTarget(&no), elsewhere) + + response := handler.Handle(t.Context(), watchRuleReview(t, watchRuleFor("content", ""), admissionv1.Create)) + + assert.True(t, response.Allowed) +} diff --git a/pkg/manifestanalyzer/folder.go b/pkg/manifestanalyzer/folder.go index 9ee6677c..0c76c446 100644 --- a/pkg/manifestanalyzer/folder.go +++ b/pkg/manifestanalyzer/folder.go @@ -59,6 +59,17 @@ const ( // folder rather than of the spec, so nothing can reject it before the folder is read; the // fix is to point the GitTarget at one of the roots it covers. IssueAmbiguousLayout IssueKind = "ambiguous-layout" + // IssueMultipleSourceNamespaces marks a GitTarget that declared its folder namespace-free + // (spec.serializeNamespace: false) and that more than one source namespace reaches. With the + // namespace stripped, two live objects of the same name produce one indistinguishable + // document, so the write is refused. It is the one kind here that is a property of the + // CONFIGURATION rather than of the folder: no repository content can cause it or clear it. + IssueMultipleSourceNamespaces IssueKind = "multiple-source-namespaces" + // IssueUnrenderedPlacement marks a new document a kustomize folder would hold but never + // render: spec.placement.useKustomize declares the operator maintains the folder's root, and + // the path the document resolved to is governed by no resources: list. Like the kind above it + // is a property of the configuration rather than of the folder's content. + IssueUnrenderedPlacement IssueKind = "unrendered-placement" // IssueWriteFanIn marks an in-place edit of a source file that more than one kustomize // render root reaches. IssueWriteFanIn IssueKind = "write-fan-in"