diff --git a/.coverage-baseline b/.coverage-baseline index 1927d47b..903ec006 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -74.5 +75.2 diff --git a/README.md b/README.md index 9d126666..3c7aed37 100644 --- a/README.md +++ b/README.md @@ -51,9 +51,6 @@ simpler for other teams. > managed control planes (EKS/GKE/AKS) generally do not expose. Without it the operator still mirrors > state, with commits authored by the configured committer. **Valkey/Redis is required either way** — it > holds each GitTarget's watch resume state so work is re-picked up after a restart or reconnect. -> -> If exact per-user authorship matters but you do not want to own kube-apiserver audit delivery yourself, -> I welcome conversations about a managed ConfigButler path. ## How it works @@ -122,12 +119,8 @@ Directions we may revisit later live in [docs/TODO.md](docs/TODO.md) and [docs/f ## Quick start This quick start sets up **committer-only mode**: the operator mirrors watched Kubernetes state into Git, -and commits are authored by the configured committer identity. That is the easiest way to prove the -workflow works. - -Valkey/Redis is still required: it holds each GitTarget's watch resume state so the operator re-picks up -where it left off after a restart or reconnect. Named commit authors can be added later with -kube-apiserver audit delivery. +commits are authored by the configured committer identity, and named authors can be added later. It is the +easiest way to prove the workflow works. **Prerequisites** diff --git a/api/v1alpha3/gittarget_types.go b/api/v1alpha3/gittarget_types.go index 1f4bc194..5c8c8b4d 100644 --- a/api/v1alpha3/gittarget_types.go +++ b/api/v1alpha3/gittarget_types.go @@ -73,6 +73,48 @@ type GitTargetSpec struct { // Encryption defines encryption settings for Secret resource writes. // +optional Encryption *EncryptionSpec `json:"encryption,omitempty"` + + // Placement declares where NEW resources are written. It has no effect on a + // resource that already has a document in Git — that document is always + // updated in place at its existing location, wherever that is. Mutable: a + // change only affects resources created after the change. + // +optional + Placement *GitTargetPlacementSpec `json:"placement,omitempty"` +} + +// GitTargetPlacementSpec declares where NEW resources are written when no document +// for their identity exists yet in Git — one exact-type map plus a fallback +// default template (Option B2 of +// docs/design/manifest/version2/gittarget-new-file-placement-rules.md). There is +// deliberately no separate "sensitive" placement block: sensitivity is a +// write-safety classification the controller owns (encrypt the content, keep the +// path identity-complete, never append or co-mingle), not a second placement +// namespace the user has to configure. A user routes Secrets the same way they +// route anything else — by naming their type in ByType. When a resource's type +// has no ByType entry and no Default, placement falls back to following the layout +// already established by sibling resources in the repository, and finally to the +// canonical, versionless {namespaceOrCluster}/{group}/{resource}/{name}.yaml path +// when there is nothing to follow. Because that fallback omits the API version, +// objects that differ only by version share a file; a target that watches several +// versions of the same group/resource and wants them separated must use a +// ByType/Default template that includes {version}. +type GitTargetPlacementSpec struct { + // ByType maps an exact resource type key ("{group}/{version}/{resource}", e.g. + // "v1/configmaps", "apps/v1/deployments", or "v1/secrets"; core resources omit + // the group) to the path template used for a new resource of that type. A path + // selected for a sensitive resource (Secrets, plus any operator-configured + // sensitive type) must be identity-complete so it cannot collide two distinct + // sensitive resources onto one file. + // +optional + ByType map[string]string `json:"byType,omitempty"` + + // Default is the path template used for a new resource whose type has no ByType + // entry. Omitted, it falls through to sibling-layout inference and then the + // built-in canonical path. A bundling default (one that is not identity-complete, + // 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. + // +optional + Default string `json:"default,omitempty"` } // GitTargetStatus defines the observed state of GitTarget. diff --git a/api/v1alpha3/watchrule_types.go b/api/v1alpha3/watchrule_types.go index 56e60f8a..8619cfe2 100644 --- a/api/v1alpha3/watchrule_types.go +++ b/api/v1alpha3/watchrule_types.go @@ -88,6 +88,11 @@ type ResourceRule struct { // - ["v1", "v1beta1"] matches both versions // - ["*"] matches all served versions // - [] matches the preferred served version + // + // Multi-version note: the built-in cold-start Git path is versionless, so two + // objects that differ only by API version resolve to the same file. To watch + // several versions of a group/resource and keep them in separate files, give the + // GitTarget a placement template that includes {version} (see GitTargetPlacementSpec). // +optional APIVersions []string `json:"apiVersions,omitempty"` diff --git a/api/v1alpha3/zz_generated.deepcopy.go b/api/v1alpha3/zz_generated.deepcopy.go index 1ae7f952..86340c09 100644 --- a/api/v1alpha3/zz_generated.deepcopy.go +++ b/api/v1alpha3/zz_generated.deepcopy.go @@ -580,6 +580,28 @@ func (in *GitTargetList) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *GitTargetPlacementSpec) DeepCopyInto(out *GitTargetPlacementSpec) { + *out = *in + if in.ByType != nil { + in, out := &in.ByType, &out.ByType + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GitTargetPlacementSpec. +func (in *GitTargetPlacementSpec) DeepCopy() *GitTargetPlacementSpec { + if in == nil { + return nil + } + out := new(GitTargetPlacementSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GitTargetSpec) DeepCopyInto(out *GitTargetSpec) { *out = *in @@ -589,6 +611,11 @@ func (in *GitTargetSpec) DeepCopyInto(out *GitTargetSpec) { *out = new(EncryptionSpec) (*in).DeepCopyInto(*out) } + if in.Placement != nil { + in, out := &in.Placement, &out.Placement + *out = new(GitTargetPlacementSpec) + (*in).DeepCopyInto(*out) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GitTargetSpec. diff --git a/config/crd/bases/configbutler.ai_gittargets.yaml b/config/crd/bases/configbutler.ai_gittargets.yaml index aef08751..dbdaac72 100644 --- a/config/crd/bases/configbutler.ai_gittargets.yaml +++ b/config/crd/bases/configbutler.ai_gittargets.yaml @@ -162,6 +162,33 @@ spec: Immutable: delete and recreate the GitTarget to change its destination. minLength: 1 type: string + placement: + description: |- + Placement declares where NEW resources are written. It has no effect on a + resource that already has a document in Git — that document is always + updated in place at its existing location, wherever that is. Mutable: a + change only affects resources created after the change. + properties: + byType: + additionalProperties: + type: string + description: |- + ByType maps an exact resource type key ("{group}/{version}/{resource}", e.g. + "v1/configmaps", "apps/v1/deployments", or "v1/secrets"; core resources omit + the group) to the path template used for a new resource of that type. A path + selected for a sensitive resource (Secrets, plus any operator-configured + sensitive type) must be identity-complete so it cannot collide two distinct + sensitive resources onto one file. + type: object + default: + description: |- + Default is the path template used for a new resource whose type has no ByType + entry. Omitted, it falls through to sibling-layout inference and then the + built-in canonical path. A bundling default (one that is not identity-complete, + 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 + type: object providerRef: description: |- ProviderRef references the GitProvider that backs this target. diff --git a/config/crd/bases/configbutler.ai_watchrules.yaml b/config/crd/bases/configbutler.ai_watchrules.yaml index 1a085f43..d47d05f4 100644 --- a/config/crd/bases/configbutler.ai_watchrules.yaml +++ b/config/crd/bases/configbutler.ai_watchrules.yaml @@ -105,6 +105,11 @@ spec: - ["v1", "v1beta1"] matches both versions - ["*"] matches all served versions - [] matches the preferred served version + + Multi-version note: the built-in cold-start Git path is versionless, so two + objects that differ only by API version resolve to the same file. To watch + several versions of a group/resource and keep them in separate files, give the + GitTarget a placement template that includes {version} (see GitTargetPlacementSpec). items: type: string type: array diff --git a/docs/architecture.md b/docs/architecture.md index 4cfd85fa..bda28990 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -422,24 +422,26 @@ local SHA from before the retry. ## What It Writes to Git -A `GitTarget` owns one subtree (`spec.path`) on one branch. New objects are placed at a canonical REST -like path; once a document exists it is edited in place wherever it already lives. A populated target -looks like: +A `GitTarget` owns one subtree (`spec.path`) on one branch. A new object follows the layout the folder +already has (or a declared policy); once a document exists it is edited in place wherever it already +lives. A populated target seeded from empty looks like: ```text team-a-config/ # GitTarget spec.path ├── README.md # operator-managed bootstrap file ├── .sops.yaml # present only when encryption is configured -├── v1/ -│ ├── configmaps/team-a/app-config.yaml -│ └── secrets/team-a/db-creds.sops.yaml # sensitive types are SOPS/age encrypted -└── apps/ - └── v1/deployments/team-a/api.yaml +├── team-a/ +│ ├── configmaps/app-config.yaml +│ ├── secrets/db-creds.sops.yaml # sensitive types are SOPS/age encrypted +│ └── apps/deployments/api.yaml ``` -The path shape is `{spec.path}/{group}/{version}/{resource}/{namespace}/{name}.yaml` (the empty core -group is omitted, sensitive resources get a `.sops.yaml` suffix). Details and the placement policy are in -[Git Write Architecture](#git-write-architecture). +The **built-in default** path is `{spec.path}/{namespace}/{group}/{resource}/{name}.yaml` — namespace +first, the API group omitted for core resources, no version segment, and a `.sops.yaml` suffix for +sensitive resources; a cluster-scoped resource uses the literal `cluster/` in place of the namespace. +But that default is only the cold-start seed: a new resource first follows its **siblings'** existing +layout, and a `GitTarget` can declare its own placement policy. Details and the placement policy are in +[File Placement](#file-placement). *** @@ -901,7 +903,8 @@ hydrates only touched files into buffers for the commit, and flushes only change * **Upserts:** if a managed document for the resource already exists, patch it in place (preserving siblings in a multi document file); if it is sensitive, encrypt the whole document again at its existing - path; if no document exists, create a new file at the canonical placement path. + path; if no document exists, place a new file per [File Placement](#file-placement) (declared policy, + then sibling inference, then the canonical default). * **Kustomize override edit-through:** a live value produced by a well-formed `images:` or `replicas:` entry in the document's kustomization chain is written back to that entry (comment-preserving, only fields the entry already declares); the source manifest keeps its bytes. Anything the inversion cannot @@ -915,11 +918,31 @@ hydrates only touched files into buffers for the commit, and flushes only change ### File Placement -New resources use the canonical REST like path -`{spec.path}/{group}/{version}/{resource}/{namespace}/{name}.yaml`. The core group's empty segment is -omitted (`{spec.path}/v1/configmaps/ns/name.yaml`), and sensitive resources use a `.sops.yaml` suffix. -Existing resources are **match first**: once a document exists in Git, updates and deletes use its current -location instead of recomputing placement. +Placement runs **only for a resource with no existing document** in the target. Existing resources are +**match first**: once a document exists in Git, updates and deletes use its current location — found by +manifest identity, not by path — instead of recomputing placement. So a change to how new files are +placed never moves a file already in Git. A new resource is placed by the first of these that applies +([internal/manifestanalyzer/placement.go](../internal/manifestanalyzer/placement.go), +[design](design/manifest/version2/gittarget-new-file-placement-rules.md)): + +1. **Declared policy (`spec.placement`).** A `GitTarget` can declare a `byType` map (exact + `[group/]version/resource` → path template) plus a `default` template, rendered from a small + brace-variable path language (`{namespace}`, `{group}`, `{resource}`, `{name}`, …). +2. **Sibling inference.** With no matching declared template, the new resource follows the layout its + siblings already use — appended to the bundle its type shares, or placed one-per-file beside them — + so pointing a target at an existing folder just continues that folder's convention. When the whole + subtree is governed by one supported kustomization and the type is brand new, the file lands beside + that kustomization and gets a `resources:` entry. +3. **Canonical fallback.** With nothing to follow (an empty repo, a brand-new type), the built-in default + `{spec.path}/{namespace}/{group}/{resource}/{name}.yaml` — namespace-first, group omitted for core, no + version, `_cluster/` for cluster-scoped, `.sops.yaml` for sensitive — so a fresh target is deterministic + and self-propagating. + +Sensitivity is a write-safety classifier, not a placement input: whatever path is chosen, a sensitive +resource is written encrypted, is never appended to an existing file, and is never co-mingled with a +plaintext document. When those guarantees cannot be honoured (e.g. a bundling `default` would route a +sensitive resource into a shared file), the resource is **skipped fail-safe** — logged per-resource and +counted in the resync summary (`placementSkipped`) — rather than written unsafely. ### Bootstrap, Encryption, and Signing @@ -1062,7 +1085,9 @@ Current limitations: * **No multi cluster routing;** cluster ID path segments on `/audit-webhook` are rejected. * **`deletecollection`** is reconciled by the watch (each item arrives as its own `DELETED`, or the mark-and-sweep reconciles them on replay). -* **New file placement** is the canonical path, not user configurable. +* **A fail-safe placement skip has no dedicated status condition.** A resource the writer refuses to + place unsafely is logged per-resource and counted in the resync summary (`placementSkipped`), but is + not (yet) surfaced as a distinct GitTarget status condition. *** diff --git a/docs/configuration.md b/docs/configuration.md index 81d706bc..fd873f83 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -324,6 +324,9 @@ The important fields are: - `spec.path`: required relative path inside the repository; use `.` only when you deliberately want the repository root - `spec.encryption`: how `Secret` resources should be encrypted before commit +- `spec.placement`: optional policy for where **new** resources are written (see + [Where new resources are written](#where-new-resources-are-written-specplacement)); omit it to follow + the repository's existing layout Example: @@ -366,6 +369,133 @@ The most useful status fields are: Use conditions for automation. +### Where new resources are written (`spec.placement`) + +Placement decides the file path for a resource that has **no document in Git yet**. Once a document +exists, updates and deletes always edit it in place at its current location (found by manifest identity, +not path), so changing placement never moves an existing file — it only affects resources created after +the change. + +#### How a path is chosen (the resolution ladder) + +For each new resource the operator walks this order and stops at the first that produces a path: + +1. **`spec.placement.byType[]`** — an explicit template for that resource's type, if you + declared one. +2. **`spec.placement.default`** — your explicit catch-all template, if you declared one. +3. **Sibling inference** — follow the layout the repository already uses for resources like this one + (described next). +4. **Built-in canonical path** — `{namespace}/{group}/{resource}/{name}.yaml`: namespace first, the group + omitted for core resources, no version segment, `_cluster/` in place of the namespace for + cluster-scoped resources (an illegal namespace name, so it can never clash with a real one), and a + `.sops.yaml` suffix for sensitive resources. + +If you set **no** `spec.placement`, only steps 3 and 4 run — which is why pointing a target at an +existing repository "just continues" that repo's conventions, and a brand-new empty repo gets the tidy +canonical layout. + +#### Following the existing layout (sibling inference) + +This is the part that looks like magic but isn't: the operator never reverse-engineers a template. It +reads the files already in the target and makes two **observed** decisions for the new resource — *which +directory* (the nearest cohort of resources like it — same type, then same type in any namespace) and +*one-file-or-bundle* (does that cohort keep one resource per file, or share a multi-document file?). + +Worked example — a target at `spec.path: clusters/prod` already looks like: + +```text +clusters/prod/ + all.yaml # 9 ConfigMaps in one multi-document file (a "bundle") + team-a/secrets/db.sops.yaml # one Secret, encrypted, one file per Secret +``` + +- A new **ConfigMap** `cache` arrives: its type-cohort (ConfigMaps) lives entirely in the `all.yaml` + bundle → the new document is **appended to `all.yaml`**. No new file, no canonical tree is created. +- A new **Secret** `api-token` arrives: it is sensitive, so plaintext siblings are ignored; the only + encrypted cohort is `team-a/secrets/` (one-per-file) → a new encrypted file + **`team-a/secrets/api-token.sops.yaml`**. +- A new ConfigMap in a **brand-new namespace** `billing`: the ConfigMap cohort is still the `all.yaml` + bundle, which is namespace-agnostic, so it is **appended to `all.yaml`** too — the new namespace needs + no new segment. + +The boundaries that keep it predictable: + +- A **sensitive** resource never infers from (or is appended into) a plaintext file; it only follows + encrypted siblings, otherwise it uses the secure canonical path. +- When a type genuinely lives in two layouts at once, the tie-break is deterministic (the cohort with the + most members wins, then the lexically smallest path) — never a coin-flip. +- Inference can only **continue** a layout that already exists. It cannot invent a greenfield one — "I + want all ConfigMaps bundled even though none exist yet" is a job for `byType` below. + +The full ladder, tie-break rules, and edge cases are in +[design/manifest/version2/gittarget-new-file-placement-rules.md](design/manifest/version2/gittarget-new-file-placement-rules.md); +the vision behind it is [design/manifest/file-agnostic-placement.md](design/manifest/file-agnostic-placement.md). + +#### Declaring a layout (`byType` / `default`) + +Set `spec.placement` when you want to **prescribe** a layout rather than follow the repo (for example a +greenfield repo, or a convention inference can't reach): + +```yaml +spec: + placement: + byType: + v1/configmaps: "{namespace}/configmaps.yaml" # bundle every ConfigMap of a namespace into one file + v1/secrets: "{namespace}/secrets/{name}.yaml" # one file per Secret + default: "{namespaceOrCluster}/{group}/{resource}/{name}.yaml" +``` + +- **`byType`** maps an exact `[group/]version/resource` key (core resources omit the group, e.g. + `v1/configmaps`; grouped resources include it, e.g. `apps/v1/deployments`) to a path template. +- **`default`** is the template for any type with no `byType` entry. Omit it to fall through to sibling + inference and then the built-in path. +- Templates are small **brace-variable path templates** (see the table below), validated statically as + part of the `Validated` gate — an unknown variable, a path that escapes `spec.path` (a leading `/` or + `..`), or a non-`.yaml`/`.yml` suffix fails the target *before* any write. + +#### Template variables + +Every value is sanitized for use as a single path segment. An **empty** segment (an omitted variable, +e.g. `{group}` for a core resource) is dropped from the final path, so `{group}/{resource}/{name}.yaml` +renders `configmaps/app.yaml`, not `/configmaps/app.yaml`. Example values are for an `apps/v1` Deployment +named `api` in namespace `team-a`: + +| Variable | Renders | Example | +|---|---|---| +| `{name}` | resource name | `api` | +| `{namespace}` | the resource's namespace; **empty** for a cluster-scoped resource | `team-a` | +| `{namespaceOrCluster}` | the namespace, or the literal `_cluster` for a cluster-scoped resource | `team-a` (a Node → `_cluster`) | +| `{resource}` | plural resource name | `deployments` | +| `{group}` | API group; **empty** for core resources | `apps` (a ConfigMap → empty) | +| `{groupPath}` | the API group as a path segment; equivalent to `{group}` today (the empty core-group segment is dropped either way) | `apps` | +| `{version}` | API version | `v1` | +| `{apiVersion}` | manifest `apiVersion` — `group/version`, or just `version` for core | `apps/v1` (a ConfigMap → `v1`) | +| `{kind}` | manifest kind | `Deployment` | +| `{scope}` | `namespaced` or `cluster` (a readable label — not a namespace-position value) | `namespaced` | +| `{sensitiveSuffix}` | `.sops.yaml` for a sensitive resource, `.yaml` otherwise | `.yaml` (a Secret → `.sops.yaml`) | + +> **`{namespace}` vs `{namespaceOrCluster}` — the one to get right.** For a cluster-scoped resource +> `{namespace}` is **empty**, so its whole path segment vanishes: a template `{namespace}/{resource}/{name}.yaml` +> renders `clusterroles/admin.yaml` for a ClusterRole (no scope folder at all). Use `{namespaceOrCluster}` +> when a single template must also place cluster-scoped resources — it keeps a stable `_cluster/` segment +> (`_cluster/clusterroles/admin.yaml`) so namespaced and cluster-scoped resources stay cleanly separated. +> `{scope}` is a *descriptor* (`cluster`/`namespaced`), not a substitute — don't use it as the folder for +> cluster resources. + +#### Sensitivity is a write-safety rule, not a placement setting + +Sensitivity is enforced by the operator whatever path is chosen. A `Secret` (and any operator-configured +sensitive type) is always written encrypted, is never appended to an existing file, and is never +co-mingled with a plaintext document. Two consequences for your templates: + +- A `byType` route for a sensitive type must be **identity-complete** — it must contain `{name}` and a + scope such as `{namespace}` — so two of them can never collide onto one file. +- A **bundling `default`** that is not identity-complete (e.g. `"all.yaml"`) is rejected unless every + sensitive type has its own identity-complete `byType` entry, so a Secret can never fall through into a + shared file. If an operator-configured sensitive type still reaches such a path at write time, that + resource is **skipped fail-safe** — logged and counted in the resync summary (`placementSkipped`) — + rather than written unsafely. It is not surfaced as a dedicated status condition today. + ### Additional sensitive resources Core Kubernetes `Secret` resources always use the encrypted Git write path. For a Secret-shaped diff --git a/docs/design/gitops-api/README.md b/docs/design/gitops-api/README.md index a089e195..cccb5c1f 100644 --- a/docs/design/gitops-api/README.md +++ b/docs/design/gitops-api/README.md @@ -3,6 +3,7 @@ > Status: active workstream > Captured: 2026-07-06 > Related: +> [kustomize-support-boundary-and-product-model.md](kustomize-support-boundary-and-product-model.md), > [../../future/idea-application-editing.md](../../future/idea-application-editing.md), > [../manifest/contextual-namespace-and-kustomize-folder-editing.md](../manifest/contextual-namespace-and-kustomize-folder-editing.md), > [../manifest/file-agnostic-placement.md](../manifest/file-agnostic-placement.md), @@ -11,10 +12,12 @@ ## Goal Enable a product layer on top of GitOps Reverser where an end user points the -operator at a Git folder holding **raw YAML or simple Kustomize**, edits those -resources through the Kubernetes API, and the operator writes the changes to a -branch. The product layer then opens the pull request through the Git host's -API. +operator at a Git folder holding **Kubernetes Resource Model (KRM) documents** +— core resources, custom resources, simple Kustomize folders, and higher-level +control-plane objects such as Flux `HelmRelease`, Argo CD `Application`, or +KRO resources — edits those resources through the Kubernetes API, and the +operator writes the changes to a branch. The product layer then opens the pull +request through the Git host's API. The division of responsibility is deliberate and already a recorded decision ([file-agnostic-placement.md](../manifest/file-agnostic-placement.md)): @@ -31,24 +34,102 @@ The operator never gains Git-host knowledge (no GitHub/GitLab API calls). ## The governing rule Every feature in this workstream must keep the repository **round-trippable**: -one source document owns one live object, and every edit the operator writes -must be expressible in both directions (live → Git and Git → live via the -GitOps tool's render). Constructs that are lossy or one-way (arbitrary patches, -generators with hash suffixes, name prefixes, Helm inflation) stay refused — -that refusal is the product's defensible support contract, not a gap. +every edit the operator writes has exactly one writable destination in Git, +and the result must be expressible in both directions (live → Git and Git → +live via the GitOps tool's render). Source documents that are shared by more +than one render root are read-only context; lossy or one-way constructs +(arbitrary patches, generators with hash suffixes, name prefixes, Helm +inflation) stay refused. That refusal is the product's defensible support +contract, not a gap. + +## Launch use cases and the practical path + +The product launches on the things non-Git-experts and busy platform engineers +do every day, not on the hardest repository shapes: + +1. **"Add something to the test environment"** — a test deployment, a test + database, an extra tool, or a product-specific abstraction. In KRM terms + this is adding ordinary documents: a `Deployment`, a Flux `HelmRelease` + plus its source, an Argo CD `Application`, or a KRO resource. In an + explicit Kustomize environment folder, the operator must also add the file + to that overlay's `resources:`. +2. **"Roll out a new version of an installation"** — bump an image tag + (F1's `images:` entry, or a plain in-place edit), a chart version on a + Flux `HelmRelease`, a revision on an Argo CD `Application`, or any other + version field on an ordinary KRM document. +3. **"Change the boring knobs without learning the repo"** — replicas, values + held in a CR spec, simple ConfigMap data, or other fields on documents the + environment owns directly. Fields owned by a shared base are explicitly + deferred to F3 patch authoring or a normal Git PR. + +Two consequences shape the plan: + +**Higher-level products come into scope as KRM, not as special cases.** +"No Helm" stays true for *inflation*: `helmCharts` remains refused and we +never render a chart. But a Flux `HelmRelease`, Argo CD `Application`, KRO +resource, or plain `Deployment` is a KRM document. Editing KRM documents is +the product surface; Flux, Argo, KRO, and core Kubernetes are examples of +controllers whose intent can be expressed that way. + +**Both standard layouts are on the launch path — scoped to these use +cases.** The day-to-day repertoire is whole-document adds plus governed +version bumps, and that slice is expressible in *both* shapes people +actually have in their GitOps repos: + +- **One plain folder per environment** (no shared base; no explicit + `kustomization.yaml` required — many GitOps tools can apply a directory of + KRM documents directly). Adding a file to test is just + adding a file; promotion is a product-level copy/diff between sibling + folders. Fully supported today. + + ```text + apps/podinfo/ + ├── test/ # one GitTarget per environment; + │ ├── deployment.yaml # core KRM is intent too + │ ├── helmrelease-postgres.yaml # Flux example + │ └── application-observability.yaml # Argo CD example + ├── acceptance/ + └── production/ + ``` + +- **Base + environment overlays, kept un-fancy** — day-one support, and the + reference layout in + [kustomize-support-boundary-and-product-model.md §6](kustomize-support-boundary-and-product-model.md): + each overlay sets `namespace:` and uses only + `resources`/`images`/`replicas`. This needs **F2** (render-root scoping): + use case 1 lands as an overlay-local file + `resources:` entry, use case 2 + as the overlay's `images:` entry or an overlay-local KRM document edit. + The base is read-only, always. + +The scoping move that keeps this launchable: **F2 + F4 are day-one +Kustomize support; F3 is the deferred hard part.** Adding overlay-local KRM +and bumping governed versions do not need patch authoring. A per-environment +edit of a base-owned *field* (the `kubectl set env` case) has no destination +until F3 — at launch it is honestly reported as unreflected and reverted by +hydration +([unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md)), +never silently lost. Tier-2 metrics on how often users hit that wall are +exactly what prices F3. ## Feature ladder -Ordered by value-per-risk. Each feature gets its own design doc in this folder -when work starts. +Ordered by delivery priority for the launch path above (F-numbers are stable +identifiers, not an ordering). Each feature gets its own design doc in this +folder when work starts. The cross-cutting scope decisions behind the ladder — the +kustomization field taxonomy, the supported-layout allowlist, who runs +kustomize, the multi-environment product model (promotion, "factor into +base"), and the mirror-mode vs. intent-cluster topology — live in +[kustomize-support-boundary-and-product-model.md](kustomize-support-boundary-and-product-model.md). | # | Feature | Design doc | Status | |---|---------|-----------|--------| | F1 | Kustomize `images:` / `replicas:` edit-through — a live change produced by an override entry is written back to that entry, never through into the source manifest | [f1-images-replicas-edit-through.md](f1-images-replicas-edit-through.md) | implemented ([#198](https://github.com/ConfigButler/gitops-reverser/pull/198)) | -| F2 | Render-root scoping — a GitTarget declares its render root (e.g. `overlays/env1`); base files reached through `../../base` become read-only context, dissolving overlay fan-out ambiguity | — | not designed | -| F3 | Restricted patch authoring — write/update scalar-field strategic-merge patches in an overlay ("live drift lands in the environment's overlay") | — | not designed | -| F4 | New-file placement rules — sibling inference + `spec.newFilePath` template so new resources land in the folder's convention, not the canonical REST path | designed in [version2/gittarget-new-file-placement-rules.md](../manifest/version2/gittarget-new-file-placement-rules.md) | designed, unbuilt | -| F5 | Branch/session ergonomics — base-branch selection, opt-in remote branch cleanup, a GitTarget-level quiescence condition | — | not designed | +| F7 | Higher-level KRM objects as first-class documents — corpus + e2e pinning that Flux `HelmRelease`/`Kustomization`, Argo CD `Application`, KRO resources, and core resources mirror and edit like any KRM document (they should already; F7 *proves* it), plus "install an app = add KRM" user docs | — | not designed (expected small) — **launch-critical** | +| F2 | Render-root scoping — a GitTarget declares its render root (e.g. `overlays/env1`); base files reached through `../../base` become read-only context, dissolving overlay fan-out ambiguity. Launch scope: overlay `images:`/`replicas:` entries + overlay-local documents, shipped **with** the per-edit `FullyReflected` accounting | — | not designed — **launch-critical** | +| F4 | New-file placement rules — sibling inference + `spec.placement` template so new resources land in the folder's convention, not the canonical REST path; includes creating the `resources:` entry when the target folder carries a kustomization | designed in [version2/gittarget-new-file-placement-rules.md](../manifest/version2/gittarget-new-file-placement-rules.md) | implemented (v1: declared policy + sibling inference steps 1/2/4 + kustomize `resources:` entry; step 3 and ordered-rule Option A deferred) | +| F5 | Branch/session ergonomics — base-branch selection, opt-in remote branch cleanup, a GitTarget-level quiescence condition | — | not designed — **launch-critical** | +| F3 | Restricted patch authoring — write/update scalar-field strategic-merge patches in an overlay ("live drift lands in the environment's overlay"); turns the launch-time unreflected class (per-env edits of base-owned fields) into reflected edits | — | post-launch; priced by tier-2 metrics | +| F6 | Admission preflight gate — opt-in intent-cluster webhook rejecting edits that cannot be reflected into the folder (fail-open; the underlying per-edit `FullyReflected` accounting ships with the F2/F4 launch unit) | [unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md) | designed, unbuilt — value highest while F3 is absent | ## What already works (baseline, 2026-07-06) @@ -69,5 +150,13 @@ Classic base + per-env overlay repositories are refused today on two independent gates: any overlay feature outside the supported subset (`patches*`, generators, `components`, `namePrefix`/`nameSuffix`, Helm, remote bases), and multi-root namespace fan-out over a shared base -(`ambiguous-namespace`). F1 does not change that boundary; F2/F3 are the +(`ambiguous-namespace`). F1 does not change that boundary; F2/F4/F3 are the features that would move it. + +F2+F4 move the *layout* half of this boundary **at launch**: un-fancy +base+overlay folders (per-overlay `namespace` + `resources`/`images`/ +`replicas`) become accepted, new overlay-local KRM can be added to +`resources:`, and the base stays read-only. The *feature* half stays: +`patches*` (until F3 authors its own), generators, `components`, name +prefixes, Helm rendering, and remote bases keep refusing the folder — that +refusal remains the support contract. diff --git a/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md b/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md new file mode 100644 index 00000000..477b3688 --- /dev/null +++ b/docs/design/gitops-api/kustomize-support-boundary-and-product-model.md @@ -0,0 +1,432 @@ +# Kustomize support boundary and product model + +> Status: direction-setting (no code change; feeds F2/F3 design) +> Captured: 2026-07-06 +> Related: +> [README.md](README.md), +> [f1-images-replicas-edit-through.md](f1-images-replicas-edit-through.md), +> [../manifest/contextual-namespace-and-kustomize-folder-editing.md](../manifest/contextual-namespace-and-kustomize-folder-editing.md), +> [../unsupported-folder-refusal-plan.md](../unsupported-folder-refusal-plan.md), +> [../manifest/version2/gittarget-new-file-placement-rules.md](../manifest/version2/gittarget-new-file-placement-rules.md) + +## Purpose + +The per-feature docs in this folder answer "how do we build X." This one takes +a step back and answers the questions that decide what X should be: + +1. Which part of the kustomization surface can this product **ever** support, + and which part is permanently out? +2. Which **folder layouts** do we promise to support, and in what order? +3. Who **runs kustomize** — us, or the GitOps tool? +4. How do the multi-environment product questions resolve: how does an app get + into *all* environments, and how does a change **promote** from test to + production? +5. Where does the reverser **run** — inside the workload cluster it mirrors, + or in a separate **intent cluster** whose only job is editing (§8)? + +The governing rule from [README.md](README.md) is the measuring stick for the +first four: the repository must stay **round-trippable** — every edit the +operator writes has exactly one writable Git destination, and the result must +be expressible in both directions (live → Git, and Git → live via the GitOps +tool's render). Shared source documents are read-only context. The fifth is a +deployment axis that leaves that rule — and everything derived from it — +untouched (§8). + +## 1. The kustomization surface: an invertibility taxonomy + +Measured against round-trippability, the ~29 documented `kustomization.yaml` +fields ([reference](https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/)) +fall into four buckets. The boundary is not a temporary implementation gap — +most of the "never" bucket is *structurally* non-invertible, and saying so is +the support contract. + +| Bucket | Fields | Why | +|---|---|---| +| **Invertible — supported** | `resources`/`bases` (local), `namespace`, `images`, `replicas` | Today's subset: namespace inference + F1 edit-through | +| **Invertible — planned** | `patches` (scalar strategic-merge, **operator-authored only**) | The one crossing worth building (F3); env drift needs a per-environment destination | +| **Invertible — possible, not planned** | `labels`, `commonLabels`, `commonAnnotations`, `buildMetadata` (subtractable in projection); `namePrefix`/`nameSuffix` (identity arithmetic cascades into every name reference); `configMapGenerator` limited to literals + `disableNameSuffixHash` | Cost/value is poor; prefixes especially buy little and touch everything | +| **Never — structurally non-invertible** | `helmCharts`/`helmGlobals` (live state does not determine chart+values inputs); generators with hash suffixes (the hash couples content to rollout — inverting means authoring rename cascades); `replacements`/`vars` (the value's source of truth is another field; a change to the target is semantically ambiguous); `generators`/`transformers`/`validators` plugins (arbitrary code = unknowable render); `components` (patch bundles composed across variants); remote bases (cannot edit what we do not own) | +| **Never — policy** | `secretGenerator` | Plaintext secrets in Git contradict the SOPS stance regardless of invertibility | +| **Refuse rather than tolerate** | `configurations`, `openapi`, `crds` | They silently change merge-key semantics, so the editor's assumptions become wrong with no visible signal | +| **Tolerable ignorables** | `sortOptions` | Does not change object state | + +Two nuances found while auditing the current gate +(`hasUnsupportedKustomizeFeature`, `internal/manifestanalyzer/store.go`): + +- **Tolerated metadata transformers leak.** `commonLabels`, `labels`, + `commonAnnotations`, and `buildMetadata` pass the gate silently, but a live + object can carry transformer-supplied metadata its source file lacks — the + writer patches it into the file as "drift." The render stays correct + (idempotent), so this is pollution rather than corruption, but it is the + same "dead text shadowed by a transformer" pathology F1 fixed for `images:`. + A future projection-side subtraction is the F1-style fix; until then the + support statement should name the limitation. +- **The fan-out fallback is safe only emergently.** Ambiguous override chains + (`ambiguous-images`, `diamond-images` corpora) emit a *warning* and fall + back to plain write-through. Write-through into a file consumed by two + render roots is the one edit that must never happen. Today the parallel + namespace ambiguity (`NamespaceNone`) prevents the live-object match in + practice, so no write occurs — but that safety is a side effect, not a + stated rule. See [§4](#4-the-invariant). + +## 2. Supported layouts: an allowlist, not field caveats + +The public support statement should enumerate **layouts we accept**, not +fields we reject. Three layouts, in delivery order: + +1. **Plain manifest folder** — raw YAML, explicit namespaces. *Shipped.* +2. **Single-context kustomize folder** — one render root; `namespace`, + `resources` (local files / child dirs), `images`, `replicas`. *Shipped + (F1).* +3. **Base + environment overlays** — one shared base, N overlay roots, one + Kubernetes namespace per overlay. *The day-one F2+F4 target, completed by + F3; see §5.* This is the canonical layout in the kustomize documentation + and common GitOps examples — a kustomize story without it reads as toy + support. + +**Launch posture:** all three layouts are on the launch path. Layouts 1–2 +apply *per environment* (one plain folder per env, each its own GitTarget); +layout 3 launches in its **F2+F4 scope** — per-overlay `namespace` + +`resources`/`images`/`replicas`, overlay-local documents, base read-only, and +new overlay-local KRM added to that overlay's `resources:`. The day-to-day +use cases (add something to test, bump a version, edit an overlay-local +object) need exactly that slice. Per-environment edits of base-owned *fields* +are the deferred hard part (F3) and are honestly reported as unreflected until +then. See the launch path in [README.md](README.md). + +Explicitly **out of scope**, and worth saying in user docs: + +- **Fleet / cluster-root repositories** (`clusters/` + `apps/` + `infra/` + layouts). A GitTarget points at an *app subtree*, never a cluster root. + This composes fine with fleet repos — a Flux `Kustomization`, Argo CD + `Application`, or another product object can point at the same app folder + we do — and keeps us out of infrastructure folders full of Helm rendering + and components. +- **Helm rendering in any form** (`helmCharts`, hybrid repos): permanently + refused. Flux `HelmRelease`, Argo CD `Application`, KRO resources, and other + control-plane CRs are ordinary KRM documents and fully in scope — the + boundary is chart inflation, not the CRs that request it. +- **Folders transformed outside the folder.** Flux `postBuild` substitutions + and `targetNamespace`, Argo CD Application-level kustomize overrides, or any + controller-side transform that is not written in the folder: the folder + alone no longer determines the render, so the round-trip promise cannot + hold. Our contract is "we mirror what the folder alone renders." (Reading + controller CRs in-cluster to learn those transforms is a possible + much-later feature, not part of this boundary.) + +## 3. Why overlays are a different animal + +Fan-out is exactly why the governing rule is "one writable destination per +edit," not "one source document owns one live object": a base +`deployment.yaml` can produce N live objects, one per environment. Two +consequences: + +- **Read side** — which overlay produced the object we are watching? We need + a variant → namespace mapping. +- **Write side** — an edit observed in the test environment must land in the + test overlay, *never* in the base. Writing to base changes production + because someone touched test. + +So overlay support is not an incremental extension of F1; it changes the +write model from "edit the document that produced the object" to "synthesize +the minimal expression of the change **in the right variant**." + +## 4. The invariant + +> **The operator never writes to a file consumed by more than one render +> root.** (Write fan-in = 1. Base files and shared components are read-only +> context, always.) + +This single sentence: + +- makes "operator edits base" structurally impossible — nothing observed in + one environment can ever change what another environment renders; +- explains the `diamond-images` refusal (two paths from one root); +- decides the multi-environment product questions in §9 (the operator cannot + "add to all environments" *by design*); +- must be promoted from today's emergent behavior to an explicit, tested rule + **before** the F2/F4 launch unit ships (see the fan-out fallback nuance in + §1). + +## 5. The overlay model (F2+F4 at launch, F3 completes it) + +With the invariant in place, every observed change in environment X has +exactly one legal destination: + +| Observed change in env X | Lands in | +|---|---| +| image tag / replica count | overlay X's `images:`/`replicas:` entry (F1 machinery, scoped per render root) | +| new object | new overlay-local file in overlay X **+ a `resources:` entry** (F4 placement plus entry creation — F1's "never add entries" boundary moves here) | +| any other spec field (env var, resource limits, args…) | an overlay-X strategic-merge patch — **F3, or nowhere** | +| delete of a base-owned object in one env | unrepresentable without a `$patch: delete` patch — refuse the write, or cover it in F3 | + +The third row is the honesty condition on shipping F2+F4 first: an arbitrary +field edit on a base-owned object has no legal destination without patch +authoring (in-place would be the base). Day-one Kustomize support therefore +covers the common slice — overlay entries, overlay-local documents, and adding +overlay-local KRM to `resources:` — **and must ship with the per-edit +`FullyReflected` accounting**, so an +out-of-scope edit is reported and reverted, never silently lost. That is a +scoped promise, not a gap: the launch use cases (add to test, bump a +version) never hit the third row, and tier-2 metrics on how often real +users *do* hit it are what price F3. + +F3's scope stays narrow and safe: the operator only creates/updates patches +**it authored** (scalar fields, one patch file per object per overlay); +pre-existing hand-written patches still refuse the folder. The gate keeps its +shape — we accept exactly the structure we fully model, which now includes +our own patches. + +What happens when an edit has **no** legal destination — the "or nowhere" +rows above — and how the user finds out, is designed separately in +[unreflectable-edits-and-write-gating.md](unreflectable-edits-and-write-gating.md) +(per-edit `FullyReflected` accounting, self-healing via hydration, and the +optional F6 admission preflight). + +### Variant identification: self-describing folders over configuration + +Prefer reading the mapping from the folder to declaring it on the GitTarget. +The analyzer already computes render roots (kustomizations no other +kustomization references); if each overlay sets `namespace:`, the +variant → namespace mapping is free and the repository documents itself. + +- **Rule:** every overlay root must set `namespace:`, distinct per root. + Refusal message: "add `namespace:` to each overlay." +- Namespace injected out-of-band (Flux `targetNamespace`, Argo destination) + stays refused (§2). An explicit GitTarget-side mapping + (`spec.variants: [{root, namespace}]`) is a fallback we add only if real + folders cannot be made self-describing. + +### Scoping: one GitTarget per overlay root + +The natural shape (already sketched as F2 in [README.md](README.md)): a +GitTarget's `spec.path` names the overlay (`…/overlays/test`), so +environment = GitTarget = watch scope = write scope, and multiple GitTargets +share the branch through the existing BranchWorker serialization. + +Structural consequence the F2 design must solve: the base sits *outside* +`spec.path`, so the analyzer needs a **read scope** (the repo subtree +reachable via `../../base`) wider than the **write scope** (the overlay +directory). The GitTarget path-overlap rejection must learn that shared +*read-only* context between targets is legal while overlapping *write* +scopes stay forbidden. + +## 6. The reference layout + +The blessed shape for docs, examples, and the test corpus: + +```text +apps/podinfo/ +├── base/ # read-only to the operator, always +│ ├── kustomization.yaml # resources: [deployment.yaml, service.yaml] +│ ├── deployment.yaml +│ └── service.yaml +└── overlays/ + ├── test/ # GitTarget A → namespace podinfo-test + │ ├── kustomization.yaml # namespace: podinfo-test + │ │ # resources: [../../base, debug-toolbox.yaml] + │ │ # images: [{name: podinfo, newTag: 6.6.0-rc1}] + │ │ # replicas: [{name: podinfo, count: 1}] + │ │ # patches: [{path: podinfo-deployment.patch.yaml}] # F3 + │ ├── debug-toolbox.yaml # an extra installed only in test + │ └── podinfo-deployment.patch.yaml # operator-authored env drift (F3) + ├── acceptance/ + │ └── kustomization.yaml # namespace: podinfo-acc, resources: [../../base], images/replicas + └── production/ + └── kustomization.yaml # namespace: podinfo-prod, resources: [../../base], images/replicas +``` + +"Install extras in test" and "add files in test" are the same mechanism: an +overlay-local resource file plus its `resources:` entry, created by the +operator when a new object appears in that environment's namespace. + +## 7. Who runs kustomize + +Deployment stays with the user's GitOps controller — Flux, Argo CD, or +anything else that consumes KRM — because that keeps the operator out of the +deployment business and out of drift-ownership fights. But "running +kustomize" means two different things: + +- **Deploying** (Git → cluster): theirs, always. +- **Understanding** (folder → expected render): ours, necessarily — the + writer must know which file supplied each live value. + +We keep *re-implementing the narrow transformer subset* rather than embedding +kustomize as the renderer: it is what keeps the refusal boundary honest (we +refuse exactly what we do not model). The worthwhile upgrade is kustomize's +Go API (`krusty`) as a **verification oracle**, not a renderer: in the +acceptance gate or in CI, build each render root in-memory and compare +against our own projection; mismatch → refuse. That buys kustomize's ground +truth without ever depending on semantics we have not modeled — the cheap, +high-confidence version of F1's parked "Option D." + +## 8. Where the reverser runs: mirror mode and intent mode + +Everything above is deliberately **topology-independent** — and that is the +point of this section. The same operator supports two deployment shapes: + +- **Mirror mode (in-cluster) — shipped today.** The reverser runs inside the + workload cluster it watches. The Kubernetes API is the source of truth and + Git is the continuously updated mirror + ([architecture.md](../../architecture.md)); writing the mirror branch + directly is legitimate. +- **Intent mode (separate intent cluster) — the product direction.** The + reverser runs in a dedicated — possibly public-facing — *intent cluster* + that runs no workloads. That cluster's API is an **editing surface over the + Git folder**: `main` is hydrated into it by normal GitOps tooling, people + edit through the Kubernetes API (kubectl, CI, or a product UI on top), the + reverser materializes the edits onto a session branch, and the product + opens the PR. **Merging the PR is the deploy** — the real clusters pick up + `main` through their own GitOps controller, and the intent cluster + re-hydrates to the merged result. + +```mermaid +flowchart TB + subgraph M["Mirror mode — shipped today"] + direction LR + WC[Workload cluster
source of truth] -->|watch| R1[Reverser
in-cluster] + R1 -->|mirror commits| G1[(Git repository)] + end + subgraph I["Intent mode — the product direction"] + direction LR + U[User / product UI] -->|Kubernetes API| IC[Intent cluster
editing surface, no workloads] + IC -->|watch| R2[Reverser
in intent cluster] + R2 -->|session branch| G2[("Git repository
main = source of truth")] + G2 -->|merged main| RC[Real clusters
via GitOps controller] + G2 -.->|hydrate main| IC + end +``` + +The editing loop, in order: + +```mermaid +sequenceDiagram + autonumber + participant U as User / product UI + participant IC as Intent cluster API + participant R as GitOps Reverser + participant G as Git repository + participant P as Product layer + participant F as GitOps controller (real clusters) + + Note over G,IC: hydration — the GitOps tool applies main's render into the intent cluster + U->>IC: edit objects (kubectl apply / UI) + IC-->>R: watch events (+ audit facts naming the author) + R->>G: route each edit (overlay entry / patch / file), push session branch + U->>IC: CommitRequest ("save now") + R-->>IC: Pushed=True + status.sha / status.branch + P->>G: open PR (session branch → main) + Note over G: review + merge — the merge is the deploy decision + G-->>F: new main revision + F->>F: render + apply into each real cluster + Note over G,IC: intent cluster re-hydrates to merged main — loop closed +``` + +### What intent mode changes + +- **What a watched object means.** Mirror mode captures *actual* state; + intent mode captures *proposed* state. The session branch is the unsaved + buffer, the PR is the save dialog, the merge is the deploy. +- **A hydration arrow appears (Git → intent cluster).** The same GitOps + tooling, pointed at the intent cluster, applies **every overlay into its + namespace** — making the intent cluster the one place all environments are + visible side by side (exactly what promotion diffs want). One tension to + design for: a continuously drift-correcting hydrator would revert a user's + edits mid-session, so hydration must be session-aware (suspended while a + session is open, or one-shot per session). That lifecycle is product-layer. +- **Branch discipline replaces direct mirroring.** Intent mode writes only + session branches; `main` moves exclusively by merge. + `GitProvider.spec.allowedBranches` already expresses this. +- **Attribution becomes first-class instead of best-effort.** The product + owns the intent cluster's apiserver configuration, so the audit webhook and + OIDC claim mappings are *guaranteed* wired — every commit, and therefore + every PR, names the real human. In customer-owned workload clusters that + wiring can only be recommended; here it is part of the product. +- **Kubernetes RBAC becomes the edit-permission model for Git.** With + namespace-per-overlay (§5), "may propose changes to test, read-only on + production" is a RoleBinding — and a contributor needs **no access to any + real cluster** to propose a production change. +- **The cluster can be hollow.** Nothing needs to run, so a + control-plane-only cluster (k3s server-only, vcluster, kwok) suffices; + status stays empty, which costs nothing (the sanitizer strips status + anyway). Schema validation, admission, and RBAC still run — that is the + "easy API" being bought: typed, validated, access-controlled editing of a + Git folder. +- **CRD parity is a prerequisite.** Editing a custom resource requires its + CRD installed in the intent cluster; a multi-tenant intent cluster also + needs namespace uniqueness across the repos it hosts. Product-layer + bookkeeping, not operator work. + +### What it does not change + +The taxonomy (§1), the layout allowlist (§2), the fan-in invariant (§4), the +routing table (§5), and the who-runs-kustomize split (§7) apply **verbatim** +— the operator neither knows nor cares whether the cluster it watches runs +real workloads. Intent mode is a deployment pattern plus product-layer +lifecycle, not a fork of the operator. + +The modes also compose: an organization can run mirror mode inside its real +clusters (capturing drift and incident-time edits) and intent mode as the +front door for planned changes — both write the same repository through the +same routing rules. + +## 9. The product model: three arrows + +Every multi-environment question resolves once each direction of data flow +has exactly one owner: + +| Arrow | Owner | Content | +|---|---|---| +| **API → Git** (per environment) | GitOps Reverser | Mirror each environment into *its own overlay*; never cross environments (§4 invariant) | +| **Git → API** | GitOps controller | Render and apply, unchanged | +| **Git → Git** | Product layer | Promotion, "factor into base," PR creation, branch/session policy | + +The arrows are topology-independent (§8): in intent mode the API → Git arrow +originates in the intent cluster, and Git → API runs twice — the hydrator +converging the intent cluster and the customer's GitOps controller converging +the real clusters — but every arrow keeps its single owner. + +Consequences: + +- **"Add the same app to all environments" is deliberately not an operator + capability.** The story is: the app appears in test (live apply, or via the + product) → the operator lands it as `overlays/test/whatever.yaml` → the + product offers **promote**, a pure Git computation (copy/diff between + overlays, open a PR). No component ever writes to base automatically. +- **Promotion is a first-class product verb, not a social convention.** F1 + already makes the highest-frequency case trivial: a tag bump in test + materializes as a one-line change to `overlays/test/kustomization.yaml`, so + "promote to acceptance" is proposing the same one-line edit in the next + overlay's file. Small, safe, demo-able — and it falls out of shipped work. +- **"Factor into base"** (dedupe N overlay copies into base + minimal + patches) is a later, fancier Git → Git refactor — also product-layer, and + computable precisely *because* the operator keeps every environment's + effective state expressible in the folder. + +The operator's contribution to promotion is that it becomes a deterministic +diff instead of an archaeology exercise. + +## 10. Consequences for the feature ladder + +- **Intent mode adds no write-model feature, but raises F5.** The topology's + deltas (hydration lifecycle, PR flow, RBAC mapping) are product-layer; the + operator-side enabler is F5 — branch/session ergonomics (base-branch + selection, remote branch cleanup, a quiescence condition) — whose priority + intent mode raises from "ergonomics" to "product prerequisite." +- **F2+F4 ship at launch; F3 completes them.** Render-root scoping, overlay + new-file placement, `resources:` entry creation, and the unreflected-set + accounting are an honest launch unit scoped to adds, bumps, and + overlay-local edits; the first `kubectl set env` against a base-owned + object in test is *reported and reverted*, not silently lost. F3 turns + that report into a reflected edit, priced by how often the report fires. +- **Prerequisite hardening (pre-F2/F4):** make the write-fan-in-=-1 + invariant (§4) explicit and tested — the ambiguity fallback must provably + never write-through into a multi-consumer file, rather than being blocked as + a side effect of namespace ambiguity. +- **F4 moves a boundary:** overlay new-object placement needs `resources:` + entry *creation*, which F1 explicitly excluded. The exclusion was per-F1 + policy, not architecture; F4's design should own it. +- **Docs follow the allowlist:** the public support statement enumerates the + three layouts of §2 (with the overlay rules of §5), names the + label/annotation leak as a known limitation, and states the Helm / + generator / fleet-root non-goals as the product's support contract. diff --git a/docs/design/gitops-api/unreflectable-edits-and-write-gating.md b/docs/design/gitops-api/unreflectable-edits-and-write-gating.md new file mode 100644 index 00000000..974c611b --- /dev/null +++ b/docs/design/gitops-api/unreflectable-edits-and-write-gating.md @@ -0,0 +1,244 @@ +# Unreflectable edits: honest accounting and the admission preflight gate + +> Status: direction-setting (no code change; extends +> [kustomize-support-boundary-and-product-model.md](kustomize-support-boundary-and-product-model.md) +> §5/§8, defines F6) +> Captured: 2026-07-06 +> Related: +> [README.md](README.md), +> [f1-images-replicas-edit-through.md](f1-images-replicas-edit-through.md), +> [../unsupported-folder-refusal-plan.md](../unsupported-folder-refusal-plan.md) + +## Problem + +The write-fan-in invariant (never write a file consumed by more than one +render root) makes base files and all other shared context **read-only**. +That is the right call — it is what keeps an edit in test from changing what +production renders. But it has an unavoidable consequence: **some fields of a +hydrated object can no longer be changed through the Kubernetes API** — or +more precisely, the change persists in the cluster but has no legal +destination in Git. + +With the reference layout (base + test/acceptance/production overlays, the +product-launch target) and F1 shipped + F2/F3 as scoped, the unreflectable +classes are concrete and enumerable: + +1. **Out-of-scope field edits on a base-owned object.** F3 v1 authors + scalar-field strategic-merge patches; anything beyond that scope + (structural list surgery on unkeyed lists, atomic-list semantics, + map-key removal if excluded from v1) has no destination. Until F3 lands, + *every* per-environment field edit on a base-owned object is in this + class — at launch it is the largest one. +2. **Per-environment delete of a base-owned object.** Expressible only as a + `$patch: delete` patch — in or out of F3 v1 is a scope decision; until it + is in, unreflectable. (A rename is a delete plus a create: the create + half is always fine, the delete half is this class.) +3. **Divergent consumers of a shared override entry** (F1's known + limitation): one `images:` entry cannot hold two tags. +4. **Transformer-supplied metadata**: a label or annotation owned by a + tolerated-but-unmodeled metadata transformer (`commonLabels`, `labels`, + `commonAnnotations`, `buildMetadata`) cannot be edited per environment. +5. **Read-only context beyond the sibling base.** In-repo shared bases + anywhere in the repository (e.g. `platform/common-base`) are reachable + read scope, never write scope. Remote URL bases stay refused at + onboarding — we cannot even *read* them deterministically. + +The question this document answers: **what happens when a user makes one of +these edits anyway?** + +## Principles + +1. **Never write through.** The invariant is absolute; a fallback that edits + shared context is worse than any amount of refusal. +2. **Never drop silently.** An edit the operator cannot reflect must become + a visible fact, not a quiet divergence. +3. **Never punish the folder for one edit.** Structural facts about the + *folder* may refuse the whole GitTarget (that gate exists today); a + single runtime *edit* must never stall every session on the target. +4. **Feedback belongs near the actor.** The best place to learn "this cannot + be saved" is at `kubectl apply` time; the second best is on the + CommitRequest the product already polls; a GitTarget condition is the + backstop, not the primary surface. +5. **Correctness must not depend on optional layers.** Any admission-time + gate can be down, stale, or disabled; the system underneath it must + already be honest. + +## Three tiers of "no" + +The two ideas on the table — an admission webhook that refuses unsavable +writes, and a GitTarget state that reports misuse — are not competitors. +They are different tiers of one escalation, each scoped to what it is good +at, plus the structural gate that already exists: + +| Tier | Scope | Mechanism | Status | +|---|---|---|---| +| **1. Onboarding refusal** | whole folder, structural facts | acceptance gate: `GitPathAccepted=False`, `Stalled=True` on unsupported constructs | shipped | +| **2. Per-edit accounting** | one object/field, runtime edits | the **unreflected set** + `FullyReflected` conditions; self-healing in intent mode | the load-bearing answer — ships with the F2/F4 launch unit | +| **3. Admission preflight** | one API request, pre-persistence | opt-in validating webhook in the intent cluster; rejects writes that would leave residue; **fail-open** | F6, fast-follow | + +```mermaid +flowchart TD + E[Sanitized watch event in env X] --> D{Legal destination
in overlay X?} + D -->|images / replicas entry| K[Edit overlay kustomization] + D -->|overlay-local document| P[In-place file edit] + D -->|F3-expressible field
on base-owned object| S[Author / update overlay patch] + D -->|new object| N[New overlay file + resources entry] + D -->|none| U[Record residue in unreflected set
write nothing] + K --> C[Commit on session branch] + P --> C + S --> C + N --> C + U --> R[FullyReflected=False on
GitTarget + CommitRequest] + R --> H[Intent mode: next hydration reverts the residue
mirror mode: standing honest drift report] +``` + +Note the fifth branch: a single API write can contain both reflectable and +unreflectable fields. The writer reflects what it can (F1 already routes +mixed changes field-by-field) and records the remainder — see the atomicity +caveat under tier 3. + +## Tier 2: the unreflected set (load-bearing) + +**Definition.** The unreflected set is the standing sanitized diff between +live state and the folder's render, per GitTarget, annotated with the +supplier/reason that made each residue unwritable. It should be recomputable: +the mark-and-sweep resync rebuilds it from scratch, steady-state events add +and remove entries incrementally, and no durable store is needed (consistent +with "watch supplies state"). But this is real F2 implementation work, not a +status-only rename: the projection must know which fields came from shared +context, overlay-local files, override entries, or transformer-owned metadata. + +**Surfacing.** + +- **GitTarget** gains a `FullyReflected` condition (`True` when the set is + empty) plus a bounded status summary (count + a capped sample of + `object, field, reason`), in the style of `status.streams`. `Ready` is + unaffected — the target still works; this is a report, not a failure. +- **CommitRequest** gains the same `FullyReflected` condition scoped to the + saved window: "everything you asked to save was expressed in the commit." + This is the product's primary surface — it already polls the + CommitRequest for `Pushed` + `status.sha`, so "your session saved, but + these 2 edits could not be expressed and will be reverted" is one more + condition on an object it already reads. + +**Convergence.** What happens to the residue differs by topology, and in +both cases the honest report is our whole job: + +- **Intent mode**: the hydrator re-applies `main`'s render, reverting the + residue — the unsavable part of the buffer is discarded, like an editor + refusing to keep an invalid character. `FullyReflected` returns to `True` + on its own. The loop is safe *by construction*, gate or no gate. +- **Mirror mode**: the cluster is the source of truth and nothing reverts + (unless the customer's own GitOps tool drift-corrects). The condition is + then a standing, honest statement: "live state exists that this folder + cannot express." That is a drift report, not an error — and it must never + block the customer's cluster operations. + +**Clearing.** An entry clears when live equals render again — whether by +hydration, by the customer's drift correction, or by the user undoing the +edit. No timer, no manual ack. + +## Tier 3: the admission preflight gate (F6) + +An opt-in validating admission webhook, registered in the intent cluster on +CREATE/UPDATE/DELETE in the namespaces claimed by gated GitTargets. It +evaluates the same projection the writer uses — "would this write leave +residue?" — and rejects with an actionable message naming the supplier: + +> `spec.template.spec.containers[0].env` on `Deployment/podinfo` is supplied +> by `apps/podinfo/base/deployment.yaml`, which is shared by all +> environments. This edit cannot be saved for `podinfo-test` alone. (Bump +> images/replicas via the overlay entry; other per-env fields need patch +> support / a base change via a normal Git PR.) + +Design choices, each answering a concern raised against the webhook idea: + +- **Fail-open (`failurePolicy: Ignore`), because tier 2 backstops it.** The + gate is a UX accelerator, not a correctness layer. If it is down, edits + land, tier 2 reports them, hydration reverts them. This is what makes + "slows things down / could be annoying" acceptable: nothing depends on it. +- **Scoped, so the latency worry stays small.** It gates only claimed + namespaces in a cluster the product owns end-to-end; no customer workload + traffic ever crosses it. Evaluation is in-memory against the manifest + store — no Git round-trip. +- **Staleness is tolerated, not solved.** The decision uses a snapshot of + the analyzer store that can race a concurrent push. A wrong *allow* is + caught by tier 2; a wrong *deny* is a retry. Neither corrupts anything. +- **Free preflight via dry-run.** With `sideEffects: None` the gate runs on + `kubectl apply --dry-run=server` — "can I save this?" becomes a native + Kubernetes question the product UI can ask before the user commits. +- **The real argument for building it is atomicity, not speed.** Tier 2 + reflects field-by-field: a single apply that bumps a tag *and* edits an + unreflectable field saves the tag and loses the edit — a mixed outcome + the user never expressed. Admission is the only point where intent can be + rejected *whole*, before persistence. (This is the same pre-persistence + property that makes admission wrong for attribution — see + [architecture.md](../../architecture.md) — used here for exactly what it + is good at: prevention. Admission prevents, audit attributes, watch + supplies state.) +- **Never in mirror mode.** We do not get to reject a customer's production + operations because our mirror is lossy. The gate is an intent-cluster + product feature, off by default, enabled per GitTarget (e.g. + `spec.writeGate: Enforce | Off`). + +## Why not a GitTarget-wide "unsupported mode" + +The alternative considered — the GitTarget drops into a degraded state when +an unreflectable edit occurs — mixes two different kinds of fact: + +- **Structural facts** are stable, folder-level, and human-fixable + ("this folder uses Helm"). Whole-target refusal is right, exists today + (tier 1), and stays. +- **Runtime edits** are transient, object-level, and often self-healing + (hydration reverts them minutes later). Escalating them to a target-wide + state punishes every session for one edit, moves the feedback far from + the actor, and flaps: `Stalled` would toggle as hydration reverts the + residue. + +So the target-wide idea survives as the *reporting* half of tier 2 — a +`FullyReflected=False` condition with a bounded sample — while never +degrading `Ready` and never blocking other writes. Clear *and* +non-invasive, rather than a trade between the two. + +## Reference-layout walk-through + +Every row assumes the launch layout (`base/` + `overlays/{test,acceptance, +production}`) with F2+F4 shipped (and F3 where marked), acting in +`podinfo-test`: + +| Action in `podinfo-test` | Outcome | +|---|---| +| `kubectl set image deploy/podinfo podinfo=…:6.6.1` | reflected → `images:` entry in `overlays/test/kustomization.yaml` | +| `kubectl scale deploy/podinfo --replicas=5` | reflected → `replicas:` entry | +| set an env var / resource limit on the base-owned Deployment | with **F3**: reflected → `overlays/test/podinfo-deployment.patch.yaml`; at launch (F2+F4 only): **unreflected** — reported, reverted by hydration | +| `kubectl apply -f new-cronjob.yaml` | reflected → new overlay-local file + `resources:` entry (F4) | +| edit the test-only debug-toolbox object | reflected → in-place edit of `overlays/test/debug-toolbox.yaml` | +| delete the base-owned Service in test only | **unreflected** until `$patch: delete` lands in F3 → reported, reverted by hydration (gate rejects it up front) | +| change a label supplied by `commonLabels` | **unreflected** (transformer-owned metadata) | +| hot-bump one of two Deployments sharing one `images:` entry | **unreflected** (divergent consumers — F1's known limitation) | + +These rows are the intended residual surface for the launch layout. The F2/F4 +definition of done must prove that surface with a corpus and e2e cases; any +new residual class either gets a legal destination or joins this table with a +clear reason. That is the support statement the product can make — each +unsupported row has a designed answer (report + revert, optionally reject up +front) rather than undefined behavior. + +## Consequences for the feature ladder + +- **Tier 2 is part of the F2/F4 definition of done**, not a separate feature: + the Kustomize launch unit ships *without* F3, so overlay support without + the unreflected set would reintroduce silent divergence, which principle 2 + forbids. +- **Tier 3 is F6** — a fast-follow after the F2/F4 launch unit, independent + of F3; intent-mode only, opt-in, fail-open. Its value is *highest while F3 + is absent*: + per-env edits of base-owned fields are then the largest unreflected + class, and the gate is what tells the user so at apply time instead of + after the save. It needs the same projection tier 2 needs, so its + marginal cost is the webhook plumbing, not new analysis. +- **F3 scope decisions move rows between tables.** Each capability added to + patch authoring (`$patch: delete`, map-key removal) deletes a row from + the unreflected classes. The classes list above doubles as F3's backlog, + priced by how often each row is hit in practice — a metric tier 2's + accounting can emit. diff --git a/docs/design/manifest/version2/gittarget-new-file-placement-rules.md b/docs/design/manifest/version2/gittarget-new-file-placement-rules.md index 972a084b..2e0232d2 100644 --- a/docs/design/manifest/version2/gittarget-new-file-placement-rules.md +++ b/docs/design/manifest/version2/gittarget-new-file-placement-rules.md @@ -1,6 +1,13 @@ # GitTarget new-file placement rules -> Status: proposed +> Status: implemented (F4 v1 — Option B2: one `byType`/`default` placement map, +> with sensitivity treated as an internal write-safety classification rather than a +> separate user-facing placement namespace) + Option C sibling inference (steps +> 1/2/4) + the kustomize-root fallback documented below. The earlier B1 surface (a +> nested `sensitive:` override block) shipped first and was superseded by B2 on the +> same branch before release; Option A and C step 3 remain deferred. See +> "Sensitivity as a write-safety classifier (B2 implementation notes)" below for how +> the encryption guarantee is preserved without the API-level split. > Captured: 2026-06-05 > Related: > [file-agnostic-placement.md](../file-agnostic-placement.md) — **the vision Option C serves**, @@ -18,8 +25,10 @@ are three viable shapes: - **Option A: ordered rule lists** (`sensitiveRules` / `normalRules`), evaluated top to bottom. -- **Option B: type maps plus defaults** (`sensitive.byType` / `normal.byType`), - using exact GVR keys such as `v1/secrets` and `apps/v1/deployments`. +- **Option B: type maps plus defaults**, using exact GVR keys such as + `v1/secrets` and `apps/v1/deployments`. B has three surface variants below: + the fully nested split, B1's top-level normal plus `sensitive` override, and + B2's single map. - **Option C: follow the existing layout (sibling inference)** — no policy at all; place a new resource where resources like it already live in the repo, and only fall back to canonical placement when there is no sibling to learn from. @@ -28,10 +37,15 @@ A and B both make placement a *declared CRD policy*. C makes it a *continuation of the layout already in the repo* — zero new API surface. They are not rivals; they layer: -- **Option B is the chosen declared API.** When a user wants to *prescribe* a - layout, the nested type-map (`placement.sensitive.byType` / `.normal.byType` + - defaults) is the surface to reach for — small, exact, easy to validate. Ordered - rules (A) stay a later escape hatch only if the type map proves too limiting. +- **Option B is the declared API family, shipped in its B2 shape.** When a user + wants to *prescribe* a layout, an exact type-map plus default is the surface to + reach for — small, exact, easy to validate. The shipped shape is **one map** + (`placement.byType`, `placement.default`) where type-specific entries express + Secret placement just like any other resource placement. Sensitivity still + exists, but as a write-safety classifier: it decides encryption, + identity-completeness, and append/collision rules, not which config block to + read. Ordered rules (A) stay a later escape hatch only if the type map proves + too limiting. - **Option C is the default underneath it.** With no policy, placement *follows the layout already in the repo*; on an empty repo it falls through to today's canonical path, so default behaviour is byte-identical to now. C is what makes @@ -62,28 +76,24 @@ spec: branch: main path: clusters/prod placement: - sensitive: - byType: - v1/secrets: "{namespace}/secret-{name}.sops.yaml" - default: "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.sops.yaml" - normal: - byType: - v1/configmaps: "{namespace}/configmaps.yaml" - default: "all-else.yaml" + byType: + v1/secrets: "{namespace}/secret-{name}.yaml" + v1/configmaps: "{namespace}/configmaps.yaml" + default: "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.yaml" ``` In this example: -- sensitive resources land in one identity-complete SOPS file per resource; +- Secrets land in one identity-complete encrypted file per resource; - ConfigMaps are grouped into `clusters/prod/configmaps.yaml`; -- every other new resource is appended to `clusters/prod/all-else.yaml`. +- every other new resource uses the identity-complete canonical-style fallback. That is powerful enough to express layouts such as `namespace-{namespace}.yaml`, -per-kind bundles, Secret-only SOPS paths, and the current canonical -`group/version/resource/namespace/name.yaml` layout. Splitting sensitive and -normal placement is what keeps this from becoming too sharp: a broad normal -default cannot catch a Secret, and sensitive placement can have stricter -uniqueness rules. +per-kind bundles, Secret-specific paths, and the current canonical +`group/version/resource/namespace/name.yaml` layout. Sensitivity is still what +keeps this from becoming too sharp, but it is enforced after placement: a +sensitive write must be encrypted, identity-complete, and single-document in v1. +It does not need a separate placement namespace to say where the file belongs. The pushback: fully ordered rules may be more API than we need first. A type map is smaller, easier to validate, and still supports Secret-specific paths, @@ -106,8 +116,9 @@ The writer already uses the materialized-model direction described in managed orphans ([internal/git/resync_flush.go](../../../../internal/git/resync_flush.go)); - existing resources are found by manifest identity, so moved files are updated in place; -- new resources still fall back to `ResourceIdentifier.ToGitPath()`, with - `.sops.yaml` added for sensitive resources +- new resources still fall back to `ResourceIdentifier.ToGitPath()`; sensitive + resources are currently routed through the encrypted writer and commonly use + the built-in `.sops.yaml` naming convention ([internal/types/identifier.go](../../../../internal/types/identifier.go), [internal/git/git.go](../../../../internal/git/git.go)). @@ -297,21 +308,178 @@ This nested version is probably the cleaner type-map API. It keeps the sensitive/normal split obvious and leaves room for class-level fields later, such as `allowMultiDocument`, without inventing new top-level names. +### Option B1: one normal surface plus a sensitive override + +There is a smaller variant of the nested type-map API that may be the better +first surface: make the common, plaintext placement policy the top-level shape, +and keep `sensitive` only as the guarded override. + +```yaml +placement: + byType: + v1/configmaps: "{namespace}/configmaps.yaml" + default: "all.yaml" + sensitive: + byType: + v1/secrets: "{namespace}/secret-{name}.sops.yaml" +``` + +```go +type GitTargetPlacementSpec struct { + ByType map[string]string `json:"byType,omitempty"` + Default string `json:"default,omitempty"` + Sensitive GitTargetPlacementClass `json:"sensitive,omitempty"` +} + +type GitTargetPlacementClass struct { + ByType map[string]string `json:"byType,omitempty"` + Default string `json:"default,omitempty"` +} +``` + +The semantics are: + +- classify the resource first; +- sensitive resources consult `placement.sensitive.byType`, then + `placement.sensitive.default`, then sibling inference, then the built-in secure + canonical SOPS fallback; +- normal resources consult `placement.byType`, then `placement.default`, then + sibling inference, then the built-in canonical plaintext fallback; +- a top-level `default` never applies to sensitive resources; +- any supplied sensitive template is still strictly validated as SOPS and + identity-complete. + +This makes the ordinary case read like what users mean: "put ConfigMaps here, +and put everything else normal in `all.yaml`." They do not have to learn a +`normal` wrapper before they can express the common case, and they do not feel +invited to provide two defaults. The sensitive block remains visible only where +the user wants to override the secure default. + +Pros: + +- fewer concepts in the common path: `placement.byType` and `placement.default` + are enough for normal resources; +- the broad default is safer to explain, because it is explicitly a normal-only + default rather than a default that needs a hidden exception for Secrets; +- sensitive placement remains hard-split and cannot be caught by a plaintext + bundle such as `all.yaml`; +- it still leaves room for sensitive-specific fields later without putting them + on every placement class. + +Cons: + +- the shape is slightly asymmetric: normal is implicit at the top level, while + sensitive has an explicit block; +- future class-level fields for normal resources need top-level names, while the + sensitive class has its own namespace; +- users who expect parallel classes may ask why there is `sensitive` but no + `normal`; +- migration from the fully nested shape would require either accepting both + shapes for a while or picking this before the field ships. + +This is a useful intermediate shape: it keeps the security property that +motivated the split, but removes the awkward "two defaults" feel from the day-one +UX. Its weakness is that it still makes sensitivity part of the placement API. +That is not quite the model we want. Placement answers "where does this resource +go?"; sensitivity answers "what write rules apply once it gets there?" + +### Option B2: one type map, sensitivity as write policy + +B2 goes one step smaller: there is only one declared placement map. Users express +Secret placement by naming `v1/secrets` in `byType`, exactly like they express +ConfigMap or Deployment placement. + +```yaml +placement: + byType: + v1/secrets: "{namespace}/secrets/{name}.yaml" + v1/configmaps: "{namespace}/configmaps.yaml" + default: "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.yaml" +``` + +```go +type GitTargetPlacementSpec struct { + ByType map[string]string `json:"byType,omitempty"` + Default string `json:"default,omitempty"` +} +``` + +The semantics are: + +- resolve placement from the single map: exact `byType` first, then `default`, + then sibling inference, then canonical fallback; +- independently classify the resource as sensitive or not; +- for a sensitive resource, require the selected path to be identity-complete; +- for a sensitive resource, write encrypted content and refuse multi-document + append/collision in v1; +- for a non-sensitive resource, allow plaintext multi-document append only into + files that are not classified encrypted; +- `.sops.yaml` is not required. It is a useful convention and may still be what + the built-in canonical SOPS fallback chooses, but real GitOps repositories can + contain SOPS-encrypted files named `secret.yaml`, and the controller should + infer encryption from content/classification, not from filename alone. + +The important safety rule moves from the API shape into validation: + +- a `byType` entry for a sensitive type can be shorter because the type key + supplies type identity; it still needs scope identity and name, for example + `{namespace}/{name}.yaml`; +- a `default` that can catch sensitive resources must be identity-complete across + type, scope, and name, for example + `{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.yaml`; +- a broad bundling default such as `all.yaml` is valid only if it cannot catch + sensitive resources, for example because every sensitive type watched by the + target has an exact `byType` entry or because the target has no sensitive + writes in scope. Otherwise the policy is unsafe and should fail validation or + be reported as unused/ambiguous policy before writes start. + +Pros: + +- smallest user-facing API: one `byType`, one `default`; +- no duplicated "normal vs sensitive" mental model; +- matches how users already think about resource layouts: "Secrets go here, + ConfigMaps go there"; +- supports repositories that encrypt SOPS files without a `.sops.yaml` suffix; +- keeps safety attached to the write class, where it belongs: encrypted content, + identity-complete paths, no sensitive append, no plaintext append into + encrypted files. + +Cons: + +- validation needs to know, or conservatively approximate, which selected types + are sensitive for this GitTarget; +- a broad `default: all.yaml` becomes invalid or limited when sensitive resources + can reach it, so the error message must explain how to fix it with explicit + `byType` entries; +- without the visual `sensitive:` block, documentation must be very clear that + sensitivity still exists and still changes write rules; +- a future per-class option such as `allowMultiDocument` would need either a new + field or a later rule-list surface. + +**Decided and implemented: B2 is the declared API.** It keeps the good part of +B1 — one obvious common path — and removes the remaining API-level split. +Sensitivity stays load-bearing, but not as a second placement namespace. It is a +controller-owned write-safety contract: encrypt sensitive content, require +identity-complete placement, and refuse unsafe appends/collisions. The +`GitTargetPlacementSpec` CRD field is exactly `{byType, default}`; the earlier B1 +`sensitive:` block was removed. How the encryption guarantee survives the removal +of the API-level split is written up in "Sensitivity as a write-safety classifier +(B2 implementation notes)" below. + The validation rules are almost the same as for ordered rules: -- omitted `placement.sensitive.default` uses the built-in secure canonical SOPS - fallback; -- omitted `placement.normal.default` uses the built-in canonical plaintext - fallback; +- omitted `placement.default` uses sibling inference and then the built-in + canonical fallback; - every `byType` key must parse as a valid resolved type key; - every referenced type should be served and watched by the GitTarget, or at least reported as unused policy; -- sensitive paths must end in `.sops.yaml` or `.sops.yml`; -- sensitive paths must be identity-complete, unless the type key itself narrows - to one namespaced or cluster-scoped type and the path contains the scope - identity; -- normal paths may intentionally collide and append to plaintext multi-document - files. +- paths selected for sensitive resources must be identity-complete, unless the + exact `byType` key itself narrows to one namespaced or cluster-scoped type and + the path contains the scope identity plus name; +- paths selected for sensitive resources do **not** need to end in `.sops.yaml` + or `.sops.yml`; suffix is convention, content classification is the truth; +- paths selected for plaintext resources may intentionally collide and append to + plaintext multi-document files, but must not append to encrypted files. The main loss is expressiveness. A type map cannot say "all namespaced resources go to `namespace-{namespace}.yaml`, but cluster-scoped resources go to @@ -319,17 +487,75 @@ go to `namespace-{namespace}.yaml`, but cluster-scoped resources go to metadata such as labels. If we do not need those patterns yet, this may be a better first API than ordered rules. -My current preference is: +Implemented shape: -1. ship the nested type-map (B) as **the** declared API — it is the smallest - surface that covers the real "this type here, everything else there" need; -2. ship Option C (sibling inference, below) as the **default** that runs when B is +1. the B2 type-map is **the** declared API — the smallest surface that covers the + real "this type here, everything else there" need while keeping sensitivity as a + write-safety rule; +2. Option C (sibling inference, below) is the **default** that runs when B is absent or silent for a resource, so an unconfigured target follows the repo's own layout instead of forcing canonical; -3. keep ordered rules (A) as a future extension only if users hit the type-map +3. ordered rules (A) remain a future extension only if users hit the type-map limit; -4. keep the same template renderer, SOPS validation, and append rules across all - three, so B's templates and C's inferred locations flow through one writer. +4. one template renderer, identity validation, encryption enforcement, and append + rules serve all of it, so B's templates and C's inferred locations flow through + one writer. + +### Sensitivity as a write-safety classifier (B2 implementation notes) + +Dropping B1's `sensitive:` block only removed the ability to give sensitive +resources a *different declared path*; it removed **no** part of the encryption +guarantee, because every piece of that guarantee already lived outside the +placement API and stays there under B2: + +- **Encrypt by classification, not by path.** Whether a resource is written + through the encrypted (SOPS) writer is decided by + `types.SensitiveResourcePolicy` (core Secrets always; plus operator-configured + types), independent of which path placement chose. A Secret routed to `all.yaml` + is still encrypted. +- **Inference never crosses the encrypted boundary.** Sibling-cohort matching + reads a document's own `CauseEncrypted` classification, so a sensitive resource + only ever infers from encrypted siblings and a plaintext one only from plaintext + siblings — no config needed. +- **Sensitive never appends.** A sensitive resource whose resolved path already + holds a document is refused (`finishPlacement`), never appended. +- **Canonical stays SOPS.** The built-in fallback keeps the `.sops.yaml` suffix for + a sensitive resource. + +What B1's API split *did* additionally provide — the guarantee that a Secret could +never reach a shared/plaintext file — is preserved by moving it from "structural +(two maps)" to "one static check plus two write-time guards", so it now holds for +**every** sensitive type (core and operator-configured), not just those the user +remembered to list in a `sensitive:` block: + +- **Static (the Validated gate).** Core Secrets are always sensitive, so the + spec-only validation can name them: an explicit `byType["v1/secrets"]` route must + be identity-complete, and a bundling `default` (one that is not itself + identity-complete, e.g. `all.yaml`) is rejected unless such a route exists — a + Secret can never fall through a bundle. Additional sensitive types are operator + configuration, invisible to a spec-only gate, so they rely on the write-time + guards instead. +- **Write-time guard 1 — no cold-bundle mixing.** When several new resources in one + batch collide on a brand-new path, the writer refuses to place a sensitive and a + plaintext resource in the same file regardless of arrival order (the first wins, + the other is skipped and retried). +- **Write-time guard 2 — no append into an encrypted file.** A plaintext resource + is refused (not appended, and not overwritten) when its resolved path already + holds an encrypted document. + +The residual, deliberately accepted for v1: if an operator configures an +*additional* sensitive type and a GitTarget uses a bundling `default` without an +explicit `byType` entry for that type, resources of it are **skipped fail-safe** +rather than co-mingled — not written until the policy is fixed. As implemented, that +skip is **logged per-resource at the skip site and counted in the resync summary as +`placementSkipped`** (`ResyncStats.PlacementSkipped`); it is deliberately **not** a +dedicated GitTarget status condition in v1. So the observability claim is bounded: +the operator will not silently mirror the resource, and the skip is visible in logs +and the resync roll-up, but a reader watching only GitTarget conditions will not see +it. Core Secrets never hit this because the static gate rejects the policy up front. +Whether to teach the Validated gate about operator-configured sensitive types (so +this becomes a fast, up-front rejection there too), and whether to add a bounded +status surface for placement skips, are open questions below. ## Option C: follow the existing layout (sibling inference) @@ -387,9 +613,10 @@ free from the encryption classification already in the store: - a sensitive resource **never** infers from plaintext siblings and is **never** appended to a plaintext bundle; -- it infers only from other sensitive single-document `.sops.yaml` siblings in the - same directory, otherwise it uses the built-in **secure canonical SOPS fallback** - (identity-complete, `.sops.yaml`). +- it infers only from other encrypted/sensitive single-document siblings in the + same directory, regardless of filename suffix; otherwise it uses the built-in + **secure canonical fallback** (identity-complete and encrypted, with a + `.sops.yaml` suffix only as a convention). So the encryption guarantee never depends on the user having configured the split correctly — there is no split to configure. @@ -413,6 +640,33 @@ lands on canonical `ToGitPath()` — **byte-identical to today.** From then on t layout propagates itself. A brand-new target behaves exactly as it does now; the power only appears once a human (or a prior import) has established any layout. +**Amendment, decided during F4 implementation: a kustomize-root fallback sits +between "no sibling match" and true canonical.** The rule above is exactly right +for a flat or already-populated folder, but it silently breaks the moment the +GitTarget's one kustomization-managed folder receives its *first* resource of a +brand-new type: canonical is a `{group}/{version}/{resource}/{namespace}/{name}.yaml` +tree the kustomization's `resources:` graph can never reach, so the new file +would land outside the very folder it was meant to join — precisely what this +document exists to prevent, just for a type instead of a whole folder. + +So, when steps 1/2 (sibling inference) both miss **and** the scanned subtree is +governed by exactly one supported kustomization, the new resource is placed +beside that kustomization's other files (and gets a `resources:` entry — see +`kustomize-support-boundary-and-product-model.md` and +`unreflectable-edits-and-write-gating.md` for the product-level "add to the right +kustomize file" framing) instead of falling to canonical. This is deliberately +narrower than the shelved step 3 (same namespace, any type) above: it never joins +an existing bundle, and it only ever fires when there is exactly one supported +kustomization for the whole GitTarget to be about — the destination follows from +there being one root, not from picking the largest matching cohort — so it cannot +become the "sink that swallows every new type" risk (P5) step 3 raised. More than +one supported kustomization under the scanned root is ambiguous and falls through +to canonical rather than guessing which root the new resource belongs to. + +True canonical — no sibling of the matching type or kind, and no single +kustomization root to fall back to — remains exactly as described above: a +brand-new, unmanaged target behaves byte-identical to today. + ### Determinism and ambiguity A type can legitimately live in two layouts at once (some ConfigMaps bundled, some @@ -553,8 +807,9 @@ higher under C. - C adds **no policy to validate** — there is no template to parse in the base case. The only new runtime check is the sensitive backstop: a resolved sensitive - path must be `.sops.yaml` and identity-complete (the same invariant A/B enforce), - else fall back to canonical SOPS. + path must be identity-complete and must use the encrypted writer. If the + inferred sibling cohort cannot prove that, placement falls back to the secure + canonical path. - The resolved path still passes the existing path validation (under `spec.path`, no `..`, correct suffix, inside discovery scope) and the existing plaintext-append acceptance (never partially manage a file). @@ -570,14 +825,15 @@ higher under C. ## Sensitive placement and uniqueness -Sensitive placement should be stricter than normal placement. A normal template -may intentionally map many resources to one file because plaintext -multi-document append is supported. A sensitive template must not do that in the -first version. +Sensitive writes should be stricter than normal writes. A normal template may +intentionally map many resources to one file because plaintext multi-document +append is supported. A sensitive resource must not do that in the first version, +regardless of whether the filename contains `.sops`. The guarantee should be structural: -> Every accepted sensitive template must render an identity-complete path. +> Every placement selected for a sensitive resource must render an +> identity-complete path, and the content writer must produce encrypted content. Identity-complete means the rendered path cannot collide for two distinct sensitive resources in the GitTarget. There are two ways a template can prove @@ -590,32 +846,35 @@ that: `{namespace}` plus `{name}` for namespaced resources, or `{name}` for cluster-scoped resources. -For the type-map API, the `byType` key itself narrows to one type. For ordered -rules, "narrows to exactly one served resource type" means the rule names one API -group, one API version, and one resource, with no wildcard or omitted type field. +For the type-map API, the `byType` key itself narrows to one type. For a broad +`default`, the template must carry the full API identity if it can catch any +sensitive resource. For ordered rules, "narrows to exactly one served resource +type" means the rule names one API group, one API version, and one resource, with +no wildcard or omitted type field. This rule is intentionally conservative. A user might know that -`{namespace}/secret-{name}.sops.yaml` is unique because they only watch core -Secrets, but the controller can only rely on that if the match proves it: +`{namespace}/secret-{name}.yaml` is unique because they only watch core Secrets, +but the controller can only rely on that if the match proves it: ```yaml placement: - sensitiveRules: - - match: - apiGroups: [""] - apiVersions: ["v1"] - resources: ["secrets"] - path: "{namespace}/secret-{name}.sops.yaml" + byType: + v1/secrets: "{namespace}/secret-{name}.yaml" ``` If the match does not narrow to one type, use the full identity path: ```yaml placement: - sensitiveRules: - - path: "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.sops.yaml" + default: "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.yaml" ``` +`.sops.yaml` and `.sops.yml` remain good conventions, and the built-in secure +canonical fallback may use them, but they are not required for correctness. Some +GitOps repositories use SOPS metadata inside ordinary `*.yaml` files. The +operator should classify encryption from file content and write behavior, not +from suffix alone. + Variable expansion must also be non-lossy for identity variables. Do not use a sanitizer that turns two legal Kubernetes names into the same path segment. Percent-encoding or another reversible path encoding is safer than lossy @@ -644,35 +903,59 @@ Recommended variables: | `{kind}` | manifest kind, for example `ConfigMap` | | `{scope}` | `namespaced` or `cluster` | | `{namespace}` | metadata namespace, empty for cluster-scoped resources | -| `{namespaceOrCluster}` | namespace, or `cluster` for cluster-scoped resources | +| `{namespaceOrCluster}` | namespace, or `_cluster` (an illegal-namespace sentinel, so it never collides with a real namespace) for cluster-scoped resources | | `{name}` | metadata name | -| `{sensitiveSuffix}` | `.sops.yaml` in sensitive rules, `.yaml` in normal rules | +| `{sensitiveSuffix}` | Optional convention helper: `.sops.yaml` for sensitive writes, `.yaml` otherwise | -With those variables, the built-in canonical normal layout is: +With those variables, the built-in canonical layout is **namespace-first, no +version segment** (as implemented in `ResourceIdentifier.ToGitPath`): ```text -{groupPath}/{version}/{resource}/{namespace}/{name}{sensitiveSuffix} +{namespaceOrCluster}/{groupPath}/{resource}/{name}{sensitiveSuffix} ``` -For a core `v1` ConfigMap named `app` in namespace `default`, empty path segments -are removed, so the canonical result is: +The scope leads (a real namespace, or the literal `_cluster` for a cluster-scoped +resource — an illegal Kubernetes namespace name, so it can never collide with a real +namespace) so a repository browses namespace-first; the group is omitted for core +resources, and the API version is deliberately left out — the operator writes one +version per object, so a version segment only adds noise and would churn the path on +a preferred-version bump. For a core `v1` ConfigMap named `app` in namespace +`default`, empty segments are removed, so the canonical result is: ```text -v1/configmaps/default/app.yaml +default/configmaps/app.yaml ``` For an `apps/v1` Deployment: ```text -apps/v1/deployments/default/app.yaml +default/apps/deployments/app.yaml +``` + +For a cluster-scoped resource the scope segment is the literal `_cluster`, e.g. a +ClusterRole `admin`: + +```text +_cluster/rbac.authorization.k8s.io/clusterroles/admin.yaml ``` -For a Secret: +For a Secret under the suffix convention: ```text -v1/secrets/default/app.sops.yaml +default/secrets/app.sops.yaml ``` +The equally valid suffix-neutral form is: + +```text +default/secrets/app.yaml +``` + +(An earlier revision seeded a REST-first `{groupPath}/{version}/{resource}/ +{namespace}/{name}` path; the namespace-first, version-less shape above replaced it +before release. Because placement is match-first for existing files, the change only +affects newly-created files and never moves one already in Git.) + Optional future variables can expose selected object metadata: | Variable | Meaning | @@ -708,30 +991,34 @@ next scan would intentionally ignore that child folder. Either the placement rul must render an immediate child file such as `default-app.yaml`, or the GitTarget must enable recursive discovery. -Sensitive resources need one more invariant: if the selected resource is sensitive, -the final path must be a SOPS path. Because sensitive resources use -the sensitive placement class, the policy can validate this without guessing: +Sensitive resources need one more invariant: if the selected resource is +sensitive, the selected path must be identity-complete and the write must produce +encrypted content. The filename does not prove encryption. -- every sensitive template must render `.sops.yaml` or `.sops.yml`; -- every sensitive template must be identity-complete; -- if sensitive placement is omitted, the controller uses the built-in secure - canonical SOPS rule. +- every selected sensitive path must be identity-complete; +- a sensitive `byType` entry can rely on the type key for type identity, but must + include scope identity and `{name}`; +- a `default` that can catch sensitive resources must include type identity, + scope identity, and `{name}`; +- if no declared placement applies, sibling inference may follow existing + encrypted siblings, otherwise the controller uses the built-in secure canonical + fallback. -A Secret rule that renders `secrets/{name}.yaml` should fail validation or -reconciliation before any cleartext write is attempted. +A Secret rule that renders `secrets/{name}.yaml` is only valid for cluster-scoped +secrets (which do not exist in core Kubernetes) or for a narrowed single +namespace. For ordinary namespaced core Secrets, it is not identity-complete +because two namespaces can both contain `name`. ## Collision and append behavior -Normal placement rules intentionally allow many resources to render to the same +Plaintext placement intentionally allows many resources to render to the same file: ```yaml placement: - normalRules: - - match: - resources: ["configmaps"] - path: "configmaps.yaml" - - path: "all-else.yaml" + byType: + v1/configmaps: "configmaps.yaml" + default: "all-else.yaml" ``` That means collision is not automatically an error. It is a request to create or @@ -755,8 +1042,9 @@ Sensitive rules: - a sensitive rule that is not identity-complete is invalid; - a sensitive rule that still maps two resources to the same path is a placement error; -- a sensitive resource must not be appended to a plaintext multi-document file; -- a plaintext resource must not be appended to a SOPS file. +- a sensitive resource must not be appended to any multi-document file; +- a plaintext resource must not be appended to a file classified encrypted, + regardless of suffix. That is stricter than SOPS can theoretically support, but it keeps the current writer's invariant: encrypted documents are not patched in place and are handled @@ -780,11 +1068,11 @@ The content acceptance gate remains responsible for: Placement adds policy acceptance: - the policy must be syntactically valid; -- custom placement classes must have defaults, or use the built-in defaults; - every path template must reference only known variables; - rendered paths for the current desired snapshot must pass path validation; -- sensitive resources must render to SOPS paths; -- sensitive templates must be identity-complete; +- selected paths for sensitive resources must be identity-complete; +- selected paths for sensitive resources must be written encrypted, regardless of + filename suffix; - sensitive collisions are refused; - plaintext collisions are allowed only when they produce an accepted managed multi-document file. @@ -816,20 +1104,18 @@ This is the likely first API shape: ```yaml placement: - sensitive: - byType: - v1/secrets: "{namespace}/secret-{name}.sops.yaml" - default: "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.sops.yaml" - normal: - byType: - v1/configmaps: "{namespace}/configmaps.yaml" - default: "all.yaml" + byType: + v1/secrets: "{namespace}/secret-{name}.yaml" + v1/configmaps: "{namespace}/configmaps.yaml" + default: "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.yaml" ``` The keys are plural resource keys, so use `v1/secrets` and `v1/configmaps`, not -`v1/secret` or `v1/configmap`. A ConfigMap goes through `normal.byType` unless -the cluster/operator configuration classifies ConfigMaps as sensitive; in that -case it goes through `sensitive.byType` or `sensitive.default`. +`v1/secret` or `v1/configmap`. A ConfigMap can share +`{namespace}/configmaps.yaml`. A Secret can also be placed by the same `byType` +map, but the resolved path must be identity-complete and the writer must produce +encrypted content. The `.sops.yaml` suffix is not required; `secret-app.yaml` +can be encrypted SOPS YAML just as much as `secret-app.sops.yaml`. ### Namespace bundle with ordered rules @@ -839,7 +1125,7 @@ resources get their own bundle. ```yaml placement: sensitiveRules: - - path: "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.sops.yaml" + - path: "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.yaml" normalRules: - match: scope: Namespaced @@ -860,20 +1146,21 @@ placement: apiGroups: [""] apiVersions: ["v1"] resources: ["secrets"] - path: "{namespace}/secrets/{name}.sops.yaml" + path: "{namespace}/secrets/{name}.yaml" normalRules: - path: "{groupPath}/{version}/{resource}/{namespace}/{name}.yaml" ``` -This keeps sensitive resources one-per-file and leaves everything else in the -current canonical layout. +This keeps sensitive resources one-per-file and encrypted while leaving +everything else in the current canonical layout. A `.sops.yaml` suffix could be +used here as a repository convention, but it is not part of the safety contract. ### ConfigMaps grouped with ordered rules ```yaml placement: sensitiveRules: - - path: "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.sops.yaml" + - path: "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.yaml" normalRules: - match: apiGroups: [""] @@ -885,18 +1172,20 @@ placement: This is a reasonable middle ground: only the low-risk, plaintext resource type is bundled. -### Broad normal default +### Broad B2 default ```yaml placement: - normalRules: - - path: "all.yaml" + byType: + v1/secrets: "{namespace}/secrets/{name}.yaml" + default: "all.yaml" ``` -This affects only normal resources. It is valid for plaintext resources but -operationally heavy. Every edit touches one file, and a very large file becomes -harder to review, merge, and re-render. Sensitive resources still use the -built-in secure canonical fallback unless `sensitiveRules` is explicitly set. +This is valid because the sensitive type is explicitly covered by an +identity-complete path. The broad default then catches plaintext resources only. +If `v1/secrets` were not covered, `default: "all.yaml"` would be invalid for a +GitTarget that can write Secrets, because a sensitive resource could otherwise +land in a non-identity-complete bundle. ### Brownfield import with no policy (Option C) @@ -906,7 +1195,7 @@ folder already looks like: ```text clusters/prod/ all.yaml # 9 ConfigMaps, multi-document - v1/secrets/app/db.sops.yaml # one Secret, encrypted, one-per-file + v1/secrets/app/db.yaml # one Secret, encrypted, one-per-file ``` A new ConfigMap `cache` in namespace `app` arrives: @@ -918,7 +1207,8 @@ A new ConfigMap `cache` in namespace `app` arrives: A new Secret `api-token` in namespace `app` arrives: - it is sensitive, so plaintext siblings are ignored; the only sensitive cohort is - `v1/secrets/app/` one-per-file → a new **`v1/secrets/app/api-token.sops.yaml`**. + `v1/secrets/app/` one-per-file → a new encrypted + **`v1/secrets/app/api-token.yaml`**. A new ConfigMap in a *new* namespace `billing` arrives: @@ -927,9 +1217,9 @@ A new ConfigMap in a *new* namespace `billing` arrives: so the new namespace needs no new segment. Nothing was configured; the layout the user already had simply continued. The same -target with `placement.normal.byType: { "v1/configmaps": "{namespace}/configmaps.yaml" }` -(Option B) would instead route ConfigMaps into per-namespace files — B overriding -C where the user has an opinion. +target with `placement.byType: { "v1/configmaps": "{namespace}/configmaps.yaml" }` +in the B2 shape would instead route ConfigMaps into per-namespace files — B +overriding C where the user has an opinion. ## Keeping it small @@ -942,7 +1232,8 @@ inside these limits: extend to unseen namespaces — P4); - C infers **directory + bundle-vs-file only** — never a filename or path-segment template (that is B's job); -- separate sensitive and normal rule lists; +- keep sensitivity as an internal write-safety classifier, not a second public + placement namespace; - prefer exact type-map overrides plus defaults unless ordered matching proves necessary; - when ordered matching exists, keep it first-match-wins only; @@ -956,20 +1247,21 @@ inside these limits: - no per-resource status spam. Status should show bounded examples. This still gives enough flexibility for the use cases that motivated the idea: -Secret-specific SOPS paths, ConfigMap bundles, namespace files, and a catch-all -layout. +Secret-specific encrypted paths, ConfigMap bundles, namespace files, and a +catch-all layout. ## Implementation sketch -1. Settle the surface: **B is the declared API, C is the default.** - - B (chosen): nested type map (`placement.sensitive.byType`, - `placement.sensitive.default`, `placement.normal.byType`, - `placement.normal.default`); +1. Settle the surface: **B2 is the declared API, C is the default.** + - B2: one top-level type map (`placement.byType`) plus one + `placement.default`; sensitivity is applied as write policy after placement + resolves; - C (default): no API surface — it resolves against the content-derived store; - - A: ordered `sensitiveRules` / `normalRules`, a later escape hatch only. + - A: ordered `sensitiveRules` / `normalRules`, a later escape hatch only, not + implemented. 2. Add the CRD field: - `GitTargetSpec.Placement *GitTargetPlacementSpec` - - the chosen nested type-map shape, or the ordered-rule shape + - the B2 type-map shape (`ByType`/`Default` at top level) - policy/path validation that can be done statically. 3. Introduce a placement policy interface in the writer/manifestreport layer: @@ -990,13 +1282,14 @@ layout. - resolve a whole batch of new creates together so placement is order-independent (P2), reusing step 8's grouping; - never let a sensitive resource infer across the plaintext boundary (it uses - other `.sops.yaml` siblings or the secure canonical SOPS fallback); + other encrypted/sensitive siblings or the secure canonical fallback); - emit, for every new create, the chosen path plus the cohort and ladder step that produced it, into the scan/dry-run output (P8). 5. Parse and validate path templates once per GitTarget reconcile. Sensitive - templates must be SOPS-suffixed and identity-complete. Type-map keys must - parse to exact GVR keys. Store compiled templates in the resolved target - metadata passed to the BranchWorker. + resources must resolve to identity-complete paths and must be written through + the encrypted writer; the selected filename suffix is not the contract. + Type-map keys must parse to exact GVR keys. Store compiled templates in the + resolved target metadata passed to the BranchWorker. 6. Replace calls to `filePathForIdentifier` / `generateFilePath` for new resources with `placement.LocateNew`. 7. Leave existing-document paths unchanged. `applyUpsert` still checks the store @@ -1007,7 +1300,11 @@ layout. - write or append multi-document YAML only for accepted plaintext files. 9. Add sensitive collision checks before rendering encrypted bytes; this is a runtime backstop behind the static identity-completeness validation. -10. Feed placement errors into GitTarget status and the scan/dry-run output. +10. Surface placement skips. **Implemented as:** each fail-safe skip is logged + per-resource at the skip site and counted in the resync summary + (`ResyncStats.PlacementSkipped`, distinct from the planner's `Skipped`). A + dedicated GitTarget status condition and a scan/dry-run "why here" trace remain + future work (see open questions). 11. Update chart docs and examples after the API shape is settled. ## Tests @@ -1023,9 +1320,13 @@ Unit tests: - path validation rejects absolute paths, `..`, empty names, bad suffixes, and paths outside non-recursive discovery scope; - core group removes the empty `{groupPath}` segment; -- sensitive resources require `.sops.yaml`; -- omitted sensitive placement still uses the built-in secure canonical fallback; -- sensitive templates that are not identity-complete are rejected; +- sensitive resources do not require `.sops.yaml`; +- sensitive resources require identity-complete selected paths; +- sensitive resources are written encrypted regardless of filename suffix; +- an unmatched sensitive resource still uses the built-in secure canonical + fallback; +- a broad default such as `all.yaml` is rejected if it can catch sensitive + resources; - plaintext same-path creates produce deterministic multi-document YAML; - sensitive same-path creates fail; - existing moved manifests are updated in place and do not re-run placement; @@ -1038,8 +1339,8 @@ Option C (sibling inference) unit tests: - a new resource whose type-cohort is a bundle is appended to that bundle file; - a new resource whose type-cohort is one-per-file gets a new `{name}.yaml` beside the siblings; -- a sensitive resource never joins a plaintext bundle and uses the secure canonical - SOPS path when only plaintext siblings exist; +- a sensitive resource never joins a plaintext bundle and uses the secure + canonical encrypted path when only plaintext siblings exist; - cohort tie-break is deterministic: most members, then lexically-smallest directory, then file (P1); - a batch of new creates against an empty snapshot is order-independent — all @@ -1057,7 +1358,8 @@ Integration/e2e tests: - a GitTarget with ConfigMaps grouped into `configmaps.yaml` creates and updates multiple ConfigMaps without duplicate files; -- Secret placement writes `.sops.yaml` and never creates cleartext Secret YAML; +- Secret placement writes encrypted YAML and never creates cleartext Secret YAML, + even when the configured path ends in ordinary `.yaml`; - a namespace-bundle policy removes one document when the API resource is deleted and deletes the file only after the last managed document is gone; - an invalid policy blocks `Ready` before live events are accepted; @@ -1089,3 +1391,10 @@ Integration/e2e tests: - When B and C disagree for a resource (B names a path, C would infer another), confirm B always wins and C only fills B's gaps — and that this precedence is visible in the dry-run. +- Should the Validated gate learn about operator-configured *additional* sensitive + types (beyond core Secrets) so a bundling `default` that could catch one is + rejected up front, instead of relying on the write-time guards to skip those + resources fail-safe at commit time? Doing so means threading the operator's + sensitive-resource configuration into GitTarget validation, which today is + spec-only. For v1 the write-time guards cover the safety; this would only upgrade + a fail-safe skip into a faster, more visible up-front error. diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index 3cfd8a61..a26703fe 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -246,12 +246,23 @@ func (r *GitTargetReconciler) evaluateValidatedGate( return false, fmt.Sprintf("Validated gate failed: %s", conflictReason), &conflictResult, nil } + if placementOK, placementMsg := validatePlacementPolicy(target.Spec.Placement); !placementOK { + r.setCondition( + target, + GitTargetConditionValidated, + metav1.ConditionFalse, + GitTargetReasonInvalidConfig, + placementMsg, + ) + return false, fmt.Sprintf("Validated gate failed: %s", GitTargetReasonInvalidConfig), nil, nil + } + r.setCondition( target, GitTargetConditionValidated, metav1.ConditionTrue, GitTargetReasonOK, - "Provider and branch validation passed", + "Provider, branch, and placement policy validation passed", ) return true, "", nil, nil } diff --git a/internal/controller/gittarget_placement_validation.go b/internal/controller/gittarget_placement_validation.go new file mode 100644 index 00000000..2f0081d4 --- /dev/null +++ b/internal/controller/gittarget_placement_validation.go @@ -0,0 +1,128 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "fmt" + "strings" + + configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" +) + +// coreSecretsTypeKey is the placement byType key for core Kubernetes Secrets — the +// one resource type that is always sensitive (types.SensitiveResourcePolicy), and +// therefore the only sensitive type this static, spec-only gate can name without +// the operator's runtime sensitive-resource configuration. Additional configured +// sensitive types are caught by the write-time co-mingle guards instead. +const coreSecretsTypeKey = "v1/secrets" + +// validatePlacementPolicy statically validates a GitTarget's declared placement +// policy (F4, Option B2: +// docs/design/manifest/version2/gittarget-new-file-placement-rules.md) against the +// spec alone — no repository scan is needed, so this runs as part of the Validated +// gate, the same spec-well-formedness check that already covers provider/branch +// resolution and path-overlap conflicts. A nil spec (no declared policy) is always +// valid. +// +// B2 has one placement map for every resource, so sensitivity is not a separate +// block to validate; it is a write-safety property enforced at write time. What +// this gate still owns is the one part of that property it can prove statically: +// core Secrets (always sensitive) must never be routed to a path that could collide +// two of them onto one file. +func validatePlacementPolicy(spec *configbutleraiv1alpha3.GitTargetPlacementSpec) (bool, string) { + if spec == nil { + return true, "" + } + for key, tmpl := range spec.ByType { + if !validPlacementTypeKeySyntax(key) { + return false, fmt.Sprintf( + "placement byType key %q is not a valid \"[group/]version/resource\" type key", key, + ) + } + if msg, bad := validatePlacementTemplate(tmpl); bad { + return false, fmt.Sprintf("placement byType[%q]: %s", key, msg) + } + } + if strings.TrimSpace(spec.Default) != "" { + if msg, bad := validatePlacementTemplate(spec.Default); bad { + return false, fmt.Sprintf("placement default: %s", msg) + } + } + return validateSecretSafety(spec) +} + +// validateSecretSafety enforces, statically, that core Secrets can never be placed +// where two of them would collide onto one file: +// - an explicit byType["v1/secrets"] route must be identity-complete; and +// - a bundling default (one that is not itself identity-complete, e.g. "all.yaml") +// is rejected unless such an explicit, identity-complete Secret route exists, so +// a Secret with no byType entry can never fall through to the bundle. +// +// Additional operator-configured sensitive types are not knowable here; the +// write-time guards (finishPlacement, createNew) keep those safe. +func validateSecretSafety(spec *configbutleraiv1alpha3.GitTargetPlacementSpec) (bool, string) { + secretTmpl := strings.TrimSpace(spec.ByType[coreSecretsTypeKey]) + secretRouteComplete := secretTmpl != "" && + manifestanalyzer.IdentityCompletePlacementTemplate(secretTmpl, true) + + if secretTmpl != "" && !secretRouteComplete { + return false, fmt.Sprintf( + "placement byType[%q] %q must be identity-complete (include {name} and "+ + "{namespace}/{namespaceOrCluster}) because Secrets are sensitive and must never share a file", + coreSecretsTypeKey, secretTmpl, + ) + } + + def := strings.TrimSpace(spec.Default) + if def == "" || manifestanalyzer.IdentityCompletePlacementTemplate(def, false) { + return true, "" + } + if !secretRouteComplete { + return false, fmt.Sprintf( + "placement default %q is a bundling path that could place a Secret in a shared file; add an "+ + "identity-complete byType[%q] entry so sensitive resources are routed to their own files", + def, coreSecretsTypeKey, + ) + } + return true, "" +} + +// validatePlacementTemplate checks one template string against the two purely +// structural rules every placement template must satisfy: its variables are all +// known (ValidPlacementTemplateSyntax) and its literal text cannot escape the +// GitTarget's spec.path or carry the wrong file suffix (ValidPlacementTemplatePath). +// Secret-specific identity-completeness is handled once, centrally, in +// validateSecretSafety. +func validatePlacementTemplate(tmpl string) (string, bool) { + if err := manifestanalyzer.ValidPlacementTemplateSyntax(tmpl); err != nil { + return err.Error(), true + } + if err := manifestanalyzer.ValidPlacementTemplatePath(tmpl); err != nil { + return err.Error(), true + } + return "", false +} + +// validPlacementTypeKeySyntax reports whether key has the shape +// "[group/]version/resource" — two or three slash-separated segments, each +// non-empty and free of surrounding whitespace. It checks syntax only; whether +// the type is actually served/watched is a repository-independent question this +// static gate cannot answer. +func validPlacementTypeKeySyntax(key string) bool { + parts := strings.Split(key, "/") + if len(parts) != 2 && len(parts) != 3 { + return false + } + for _, p := range parts { + // Reject empty segments and, crucially, segments carrying surrounding + // whitespace: the resolver matches this key against the watched GVR by + // exact string equality, so "apps/v1/deployments " would clear a + // trim-then-check gate yet never match — the placement would silently + // fall through instead of erroring. Fail such keys at the Validated gate. + if p == "" || p != strings.TrimSpace(p) { + return false + } + } + return true +} diff --git a/internal/controller/gittarget_placement_validation_test.go b/internal/controller/gittarget_placement_validation_test.go new file mode 100644 index 00000000..bb20282d --- /dev/null +++ b/internal/controller/gittarget_placement_validation_test.go @@ -0,0 +1,191 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apimeta "k8s.io/apimachinery/pkg/api/meta" + 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/fake" + + configbutleraiv1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" +) + +func TestValidatePlacementPolicy(t *testing.T) { + cases := []struct { + name string + spec *configbutleraiv1alpha3.GitTargetPlacementSpec + ok bool + }{ + {"nil spec is valid", nil, true}, + {"empty spec is valid", &configbutleraiv1alpha3.GitTargetPlacementSpec{}, true}, + { + "byType plus an identity-complete default", + &configbutleraiv1alpha3.GitTargetPlacementSpec{ + ByType: map[string]string{"v1/configmaps": "{namespace}/configmaps.yaml"}, + Default: "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.yaml", + }, + true, + }, + { + "bundling default is valid when Secrets have an identity-complete route", + &configbutleraiv1alpha3.GitTargetPlacementSpec{ + ByType: map[string]string{"v1/secrets": "{namespace}/secrets/{name}.yaml"}, + Default: "all.yaml", + }, + true, + }, + { + "bundling default with no Secret route is rejected", + &configbutleraiv1alpha3.GitTargetPlacementSpec{ + Default: "all.yaml", + }, + false, + }, + { + "bundling default with a non-identity-complete Secret route is rejected", + &configbutleraiv1alpha3.GitTargetPlacementSpec{ + ByType: map[string]string{"v1/secrets": "secrets.yaml"}, + Default: "all.yaml", + }, + false, + }, + { + "Secret byType route must be identity-complete even without a default", + &configbutleraiv1alpha3.GitTargetPlacementSpec{ + ByType: map[string]string{"v1/secrets": "secrets/{name}.yaml"}, + }, + false, + }, + { + "identity-complete Secret route needs no .sops suffix", + &configbutleraiv1alpha3.GitTargetPlacementSpec{ + ByType: map[string]string{"v1/secrets": "{namespace}/secret-{name}.yaml"}, + }, + true, + }, + { + "a .sops suffix on a Secret route is still accepted", + &configbutleraiv1alpha3.GitTargetPlacementSpec{ + ByType: map[string]string{"v1/secrets": "{namespace}/secret-{name}.sops.yaml"}, + }, + true, + }, + { + "unknown template variable", + &configbutleraiv1alpha3.GitTargetPlacementSpec{ + Default: "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}-{bogus}.yaml", + }, + false, + }, + { + "template escapes spec.path with a parent traversal", + &configbutleraiv1alpha3.GitTargetPlacementSpec{ + Default: "../outside.yaml", + }, + false, + }, + { + "template does not end in a YAML suffix", + &configbutleraiv1alpha3.GitTargetPlacementSpec{ + ByType: map[string]string{"v1/configmaps": "{namespace}/{name}.txt"}, + }, + false, + }, + { + "malformed byType key", + &configbutleraiv1alpha3.GitTargetPlacementSpec{ + ByType: map[string]string{"not-a-type-key": "all.yaml"}, + }, + false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ok, msg := validatePlacementPolicy(tc.spec) + if ok != tc.ok { + t.Errorf("validatePlacementPolicy() = (%v, %q), want ok=%v", ok, msg, tc.ok) + } + if !tc.ok && msg == "" { + t.Errorf("an invalid policy must carry a message") + } + }) + } +} + +func TestValidPlacementTypeKeySyntax(t *testing.T) { + cases := []struct { + key string + want bool + }{ + {"v1/secrets", true}, + {"apps/v1/deployments", true}, + {"cert-manager.io/v1/certificates", true}, + {"", false}, + {"v1", false}, + {"a/b/c/d", false}, + {"v1//secrets", false}, + {"/v1/secrets", false}, + {"apps/v1/deployments ", false}, // trailing space can never match the resolver's exact GVR key + {" apps/v1/deployments", false}, // leading space + {"apps/v1/ deployments", false}, // padded middle segment + {"apps/v1/deployments\t", false}, // trailing tab + } + for _, tc := range cases { + if got := validPlacementTypeKeySyntax(tc.key); got != tc.want { + t.Errorf("validPlacementTypeKeySyntax(%q) = %v, want %v", tc.key, got, tc.want) + } + } +} + +// TestEvaluateValidatedGate_InvalidPlacementPolicy proves the wiring, not just the +// pure function: an otherwise-valid GitTarget (provider found, branch allowed, no +// path conflict) whose declared placement policy is invalid must fail the +// Validated gate with reason InvalidConfig, not just make validatePlacementPolicy +// return false in isolation. +func TestEvaluateValidatedGate_InvalidPlacementPolicy(t *testing.T) { + const ns = "default" + scheme := runtime.NewScheme() + require.NoError(t, clientgoscheme.AddToScheme(scheme)) + require.NoError(t, configbutleraiv1alpha3.AddToScheme(scheme)) + + provider := &configbutleraiv1alpha3.GitProvider{ + ObjectMeta: metav1.ObjectMeta{Name: "provider-a", Namespace: ns}, + Spec: configbutleraiv1alpha3.GitProviderSpec{ + AllowedBranches: []string{"main"}, + }, + } + target := &configbutleraiv1alpha3.GitTarget{ + ObjectMeta: metav1.ObjectMeta{Name: "target-a", Namespace: ns}, + Spec: configbutleraiv1alpha3.GitTargetSpec{ + ProviderRef: configbutleraiv1alpha3.GitProviderReference{Name: "provider-a"}, + Branch: "main", + Path: "apps", + Placement: &configbutleraiv1alpha3.GitTargetPlacementSpec{ + Default: "{bogus}/all.yaml", + }, + }, + } + + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(provider, target).Build() + reconciler := &GitTargetReconciler{Client: client} + + validated, msg, result, err := reconciler.evaluateValidatedGate(context.Background(), target, ns) + + require.NoError(t, err) + assert.Nil(t, result) + assert.False(t, validated) + assert.Contains(t, msg, GitTargetReasonInvalidConfig) + + cond := apimeta.FindStatusCondition(target.Status.Conditions, GitTargetConditionValidated) + require.NotNil(t, cond, "Validated condition must be set") + assert.Equal(t, metav1.ConditionFalse, cond.Status) + assert.Equal(t, GitTargetReasonInvalidConfig, cond.Reason) + assert.Contains(t, cond.Message, "bogus") +} diff --git a/internal/git/acceptance_gate_test.go b/internal/git/acceptance_gate_test.go index 1896688d..8fc22e6c 100644 --- a/internal/git/acceptance_gate_test.go +++ b/internal/git/acceptance_gate_test.go @@ -37,7 +37,7 @@ func TestPlanFlush_RefusesUnsupportedKustomizeFolder(t *testing.T) { w := &BranchWorker{contentWriter: writer} event := cmEvent("CREATE", "fresh", "green") - _, err := w.flushEventsToWorktree(context.Background(), worktree, "", []Event{event}) + _, err := w.flushEventsToWorktree(context.Background(), worktree, "", []Event{event}, nil) var refused *manifestanalyzer.AcceptanceRefusedError require.ErrorAs(t, err, &refused, "flush must refuse with *AcceptanceRefusedError") @@ -59,7 +59,7 @@ func TestPlanFlush_AcceptsPlainKustomizeFolder(t *testing.T) { w := &BranchWorker{contentWriter: writer} create := []Event{cmEvent("CREATE", "fresh", "green")} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", create) + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", create, nil) require.NoError(t, err, "a plain kustomization must not be refused") assert.True(t, changed, "the ConfigMap must be written beside the retained kustomization") } @@ -74,7 +74,7 @@ func TestPlanFlush_DoesNotRefuseOwnSopsConfig(t *testing.T) { w := &BranchWorker{contentWriter: writer} create := []Event{cmEvent("CREATE", "fresh", "green")} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", create) + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", create, nil) 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/branch_worker_split_test.go b/internal/git/branch_worker_split_test.go index 3289e146..e8c1c441 100644 --- a/internal/git/branch_worker_split_test.go +++ b/internal/git/branch_worker_split_test.go @@ -456,9 +456,9 @@ func TestBranchWorker_Replay_UsesResolvedMetadata_GitTargetDeletedMidBurst(t *te verifyPath := filepath.Join(t.TempDir(), "verify") _, _ = initLocalRepo(t, verifyPath, remoteURL, "main") - sopsPath := filepath.Join(verifyPath, "v1", "secrets", "default", "burst-secret.sops.yaml") + sopsPath := filepath.Join(verifyPath, "default", "secrets", "burst-secret.sops.yaml") assert.FileExists(t, sopsPath, "replay must reuse the resolved encryption metadata after the target disappears") - assert.NoFileExists(t, filepath.Join(verifyPath, "v1", "secrets", "default", "burst-secret.yaml")) + assert.NoFileExists(t, filepath.Join(verifyPath, "default", "secrets", "burst-secret.yaml")) assert.FileExists( t, filepath.Join(verifyPath, "OUTSIDE.md"), diff --git a/internal/git/branch_worker_test.go b/internal/git/branch_worker_test.go index e19db530..6f096205 100644 --- a/internal/git/branch_worker_test.go +++ b/internal/git/branch_worker_test.go @@ -567,7 +567,7 @@ func TestBranchWorker_CommitAndPushRequest_PreparesRepositoryBeforeFirstWrite(t "worker should prepare by pulling remote content before first write", ) - manifestPath := filepath.Join(localRepoPath, "clusters/dev", "v1", "configmaps", "default", "example.yaml") + manifestPath := filepath.Join(localRepoPath, "clusters/dev", "default", "configmaps", "example.yaml") content, err := os.ReadFile(manifestPath) require.NoError(t, err) assert.Contains(t, string(content), "name: example") @@ -684,9 +684,8 @@ func TestBranchWorker_CommitAndPushRequest_NewBranchStartsFromLatestMain(t *test manifestPath := filepath.Join( featureCheckoutPath, "clusters/dev", - "v1", - "configmaps", "default", + "configmaps", "example-feature.yaml", ) manifestContent, err := os.ReadFile(manifestPath) diff --git a/internal/git/commit_executor.go b/internal/git/commit_executor.go index a8158dad..4af0f2fb 100644 --- a/internal/git/commit_executor.go +++ b/internal/git/commit_executor.go @@ -127,7 +127,7 @@ func (w *BranchWorker) executePendingWrite( return 0, plumbing.ZeroHash, fmt.Errorf("configure secret encryptor: %w", err) } - anyChanges, err := w.applyPendingWriteEvents(ctx, repo, worktree, pendingWrite.Events) + anyChanges, err := w.applyPendingWriteEvents(ctx, repo, worktree, pendingWrite.Events, pendingWrite.Targets) if err != nil { return 0, plumbing.ZeroHash, err } @@ -162,6 +162,7 @@ func (w *BranchWorker) applyPendingWriteEvents( repo *gogit.Repository, worktree *gogit.Worktree, events []Event, + targets map[pendingTargetKey]ResolvedTargetMetadata, ) (bool, error) { // Stage path-scoped bootstrap files first, before any resource write, exactly as // the per-event path did. @@ -178,7 +179,13 @@ func (w *BranchWorker) applyPendingWriteEvents( byBase := groupEventsByBase(events) anyChanges := false for _, base := range sortedBaseKeys(byBase) { - changed, err := w.flushEventsToWorktree(ctx, worktree, base, byBase[base]) + changed, err := w.flushEventsToWorktree( + ctx, + worktree, + base, + byBase[base], + placementPolicyForBase(targets, base), + ) if err != nil { return false, err } diff --git a/internal/git/commit_executor_test.go b/internal/git/commit_executor_test.go index ce6f325c..16b37972 100644 --- a/internal/git/commit_executor_test.go +++ b/internal/git/commit_executor_test.go @@ -94,7 +94,7 @@ func TestGenerateFilePath_AdditionalSensitiveResourceUsesSOPSPath(t *testing.T) Name: "registry", }, policy) - assert.Equal(t, "core.cozystack.io/v1beta1/tenantsecrets/tenant-a/registry.sops.yaml", path) + assert.Equal(t, "tenant-a/core.cozystack.io/tenantsecrets/registry.sops.yaml", path) } func TestExecutor_GroupedSingleEvent_UsesPerEventMessageFallback(t *testing.T) { @@ -215,9 +215,9 @@ func TestExecutor_AppliesEncryptionFromPendingWrite_NotFromWorker(t *testing.T) assert.Equal(t, 1, created) assert.False(t, hash.IsZero(), "a committed write reports its commit hash") - encryptedPath := filepath.Join(repoPath, "team-secrets", "v1", "secrets", "default", "unit-secret.sops.yaml") + encryptedPath := filepath.Join(repoPath, "team-secrets", "default", "secrets", "unit-secret.sops.yaml") assert.FileExists(t, encryptedPath) - assert.NoFileExists(t, filepath.Join(repoPath, "team-secrets", "v1", "secrets", "default", "unit-secret.yaml")) + assert.NoFileExists(t, filepath.Join(repoPath, "team-secrets", "default", "secrets", "unit-secret.yaml")) content, err := os.ReadFile(encryptedPath) require.NoError(t, err) diff --git a/internal/git/commit_test.go b/internal/git/commit_test.go index 94f80470..0e0c5f2c 100644 --- a/internal/git/commit_test.go +++ b/internal/git/commit_test.go @@ -427,7 +427,7 @@ func TestIntegration_FilePathAndCommitMessage(t *testing.T) { commitMessage, err := renderEventCommitMessage(event, ResolveCommitConfig(nil)) require.NoError(t, err) - assert.Equal(t, "v1/pods/integration-test/integration-test-pod.yaml", filePath) + assert.Equal(t, "integration-test/pods/integration-test-pod.yaml", filePath) assert.Equal(t, "[CREATE] v1/pods/integration-test-pod", commitMessage) assert.Contains(t, filePath, "integration-test-pod") assert.Contains(t, commitMessage, "integration-test-pod") @@ -586,7 +586,7 @@ func TestDeleteOperation_ClusterScoped(t *testing.T) { commitMessage, err := renderEventCommitMessage(event, ResolveCommitConfig(nil)) require.NoError(t, err) - assert.Equal(t, "v1/namespaces/test-namespace.yaml", filePath) + assert.Equal(t, "_cluster/namespaces/test-namespace.yaml", filePath) assert.Equal(t, "[DELETE] v1/namespaces/test-namespace", commitMessage) } diff --git a/internal/git/fieldpatch_flush_test.go b/internal/git/fieldpatch_flush_test.go index 2456e0c9..6508d827 100644 --- a/internal/git/fieldpatch_flush_test.go +++ b/internal/git/fieldpatch_flush_test.go @@ -54,7 +54,7 @@ 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) + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, nil) require.NoError(t, err) return changed } diff --git a/internal/git/git_operations_test.go b/internal/git/git_operations_test.go index e02815b8..5f529ec3 100644 --- a/internal/git/git_operations_test.go +++ b/internal/git/git_operations_test.go @@ -349,7 +349,7 @@ func TestBranchWorker_FirstCommitOnEmptyRepo(t *testing.T) { assert.Equal(t, "main", branchName) // Verify file exists - filePath := filepath.Join(worker.repoPathForRemote("file://"+serverPath), "v1/pods/default/test-pod.yaml") + filePath := filepath.Join(worker.repoPathForRemote("file://"+serverPath), "default/pods/test-pod.yaml") assert.FileExists(t, filePath) } diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 46d19f47..e0219519 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -51,7 +51,7 @@ func TestToGitPath_NamespacedResource(t *testing.T) { group: "", version: "v1", resourcePlural: "pods", - expected: "v1/pods/default/test-pod.yaml", + expected: "default/pods/test-pod.yaml", }, { name: "my-service", @@ -59,7 +59,7 @@ func TestToGitPath_NamespacedResource(t *testing.T) { group: "", version: "v1", resourcePlural: "services", - expected: "v1/services/production/my-service.yaml", + expected: "production/services/my-service.yaml", }, { name: "app-config", @@ -67,7 +67,7 @@ func TestToGitPath_NamespacedResource(t *testing.T) { group: "", version: "v1", resourcePlural: "configmaps", - expected: "v1/configmaps/staging/app-config.yaml", + expected: "staging/configmaps/app-config.yaml", }, { name: "complex-name-with-dashes", @@ -75,7 +75,7 @@ func TestToGitPath_NamespacedResource(t *testing.T) { group: "apps", version: "v1", resourcePlural: "deployments", - expected: "apps/v1/deployments/kube-system/complex-name-with-dashes.yaml", + expected: "kube-system/apps/deployments/complex-name-with-dashes.yaml", }, } @@ -107,28 +107,28 @@ func TestToGitPath_ClusterScopedResource(t *testing.T) { group: "", version: "v1", resourcePlural: "namespaces", - expected: "v1/namespaces/my-namespace.yaml", + expected: "_cluster/namespaces/my-namespace.yaml", }, { name: "cluster-admin", group: "rbac.authorization.k8s.io", version: "v1", resourcePlural: "clusterroles", - expected: "rbac.authorization.k8s.io/v1/clusterroles/cluster-admin.yaml", + expected: "_cluster/rbac.authorization.k8s.io/clusterroles/cluster-admin.yaml", }, { name: "system-binding", group: "rbac.authorization.k8s.io", version: "v1", resourcePlural: "clusterrolebindings", - expected: "rbac.authorization.k8s.io/v1/clusterrolebindings/system-binding.yaml", + expected: "_cluster/rbac.authorization.k8s.io/clusterrolebindings/system-binding.yaml", }, { name: "my-pv", group: "", version: "v1", resourcePlural: "persistentvolumes", - expected: "v1/persistentvolumes/my-pv.yaml", + expected: "_cluster/persistentvolumes/my-pv.yaml", }, } @@ -156,7 +156,7 @@ func TestToGitPath_EmptyNamespace(t *testing.T) { Name: "test-resource", } path := identifier.ToGitPath() - assert.Equal(t, "v1/testkinds/test-resource.yaml", path) + assert.Equal(t, "_cluster/testkinds/test-resource.yaml", path) } func TestToGitPath_SpecialCharacters(t *testing.T) { @@ -170,19 +170,19 @@ func TestToGitPath_SpecialCharacters(t *testing.T) { name: "test.resource", namespace: "default", resourcePlural: "pods", - expected: "v1/pods/default/test.resource.yaml", + expected: "default/pods/test.resource.yaml", }, { name: "test_resource", namespace: "default", resourcePlural: "services", - expected: "v1/services/default/test_resource.yaml", + expected: "default/services/test_resource.yaml", }, { name: "test-resource-123", namespace: "test-ns-456", resourcePlural: "configmaps", - expected: "v1/configmaps/test-ns-456/test-resource-123.yaml", + expected: "test-ns-456/configmaps/test-resource-123.yaml", }, } @@ -315,7 +315,7 @@ func TestPathGeneration_ConsistentOutput(t *testing.T) { assert.Equal(t, path1, path2) assert.Equal(t, path2, path3) - assert.Equal(t, "v1/pods/consistent-ns/consistent-test.yaml", path1) + assert.Equal(t, "consistent-ns/pods/consistent-test.yaml", path1) } func TestToGitPath_DeleteOperation(t *testing.T) { @@ -330,7 +330,7 @@ func TestToGitPath_DeleteOperation(t *testing.T) { // File path should be same for CREATE, UPDATE, and DELETE path := identifier.ToGitPath() - expected := "v1/secrets/production/test-resource.yaml" + expected := "production/secrets/test-resource.yaml" assert.Equal(t, expected, path) } diff --git a/internal/git/gittargetignore_writer_test.go b/internal/git/gittargetignore_writer_test.go index 67eb78a8..0678179e 100644 --- a/internal/git/gittargetignore_writer_test.go +++ b/internal/git/gittargetignore_writer_test.go @@ -34,10 +34,10 @@ func TestWriter_IgnoreShadowsManagedPath(t *testing.T) { createBareRepo(t, remotePath) remoteURL := "file://" + remotePath - // "v1/" shadows the canonical write path v1/pods/default/.yaml. It is not a - // catastrophic whole-space pattern, so the acceptance gate accepts it — the danger is + // "default/pods/" shadows the canonical write path default/pods/.yaml. It is not + // a catastrophic whole-space pattern, so the acceptance gate accepts it — the danger is // caught only at write time, where the path is finally known. - simulateClientCommitOnDisk(t, remoteURL, "main", ".gittargetignore", "v1/\n") + simulateClientCommitOnDisk(t, remoteURL, "main", ".gittargetignore", "default/pods/\n") worker, err := newTestBranchWorker(remoteURL, "test-repo", "main") require.NoError(t, err) diff --git a/internal/git/inplace_edit_test.go b/internal/git/inplace_edit_test.go index 7933cb66..dddfb1c1 100644 --- a/internal/git/inplace_edit_test.go +++ b/internal/git/inplace_edit_test.go @@ -52,7 +52,7 @@ 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) + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, nil) require.NoError(t, err) return changed } @@ -66,7 +66,7 @@ func applyEventsViaPlanFlushWithMapper( ) bool { t.Helper() w := &BranchWorker{contentWriter: writer, mapper: mapper} - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events) + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, nil) require.NoError(t, err) return changed } diff --git a/internal/git/inplace_overrides_test.go b/internal/git/inplace_overrides_test.go index 94936055..b6661792 100644 --- a/internal/git/inplace_overrides_test.go +++ b/internal/git/inplace_overrides_test.go @@ -210,7 +210,7 @@ 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) + wb := newWriteBatch(context.Background(), newContentWriter(types.SensitiveResourcePolicy{}), nil, scan, nil) fieldMissing := manifestanalyzer.OverrideEdit{ KustomizationPath: "kustomization.yaml", diff --git a/internal/git/manifestedit/kustomization.go b/internal/git/manifestedit/kustomization.go index c84bc8c2..cfcd3b8a 100644 --- a/internal/git/manifestedit/kustomization.go +++ b/internal/git/manifestedit/kustomization.go @@ -4,6 +4,7 @@ package manifestedit import ( "fmt" + "strings" "gopkg.in/yaml.v3" ) @@ -35,20 +36,17 @@ type KustomizationEdit struct { Value string } -// PatchKustomization applies the edits to a single-document kustomization file, -// preserving comments, key order, and framing exactly as the manifest patch path -// does. All-or-nothing: any edit that cannot be applied (multi-document file, -// unparseable YAML, missing section/entry/field, a name mismatch at the pinned -// index) skips the whole call and returns the original content with a -// diagnostic — the writer must never guess inside a build directive. -func PatchKustomization(path string, content []byte, edits []KustomizationEdit) (EditResult, []Diagnostic) { - skip := func(format string, args ...interface{}) (EditResult, []Diagnostic) { - return EditResult{Content: content, Mode: EditSkipped}, - []Diagnostic{diag(DiagWarning, Location{Path: path}, format, args...)} - } - +// locateKustomizationDocument finds and decodes the sole document in a +// single-document kustomization.yaml, ready for an editor to mutate root in place. +// ok is false when the file cannot be safely edited — a multi-document file, +// unparseable YAML, an empty document, or a non-mapping document — in which case +// reason names why, for the caller's skip diagnostic. +func locateKustomizationDocument( + path string, + content []byte, +) ([]rawDoc, int, *yaml.Node, string, bool) { if DocumentCount(content) != 1 { - return skip("kustomization %s is not a single-document file", path) + return nil, -1, nil, fmt.Sprintf("kustomization %s is not a single-document file", path), false } docs := splitDocuments(string(content)) idx := -1 @@ -59,14 +57,32 @@ func PatchKustomization(path string, content []byte, edits []KustomizationEdit) } } if idx < 0 { - return skip("kustomization %s holds no document", path) + return nil, -1, nil, fmt.Sprintf("kustomization %s holds no document", path), false + } + decoded, empty, err := decodeDoc(docs[idx].body) + if err != nil || empty || decoded.Kind != yaml.MappingNode { + return nil, -1, nil, fmt.Sprintf("kustomization %s is not an editable mapping document", path), false + } + return docs, idx, decoded, "", true +} + +// PatchKustomization applies the edits to a single-document kustomization file, +// preserving comments, key order, and framing exactly as the manifest patch path +// does. All-or-nothing: any edit that cannot be applied (multi-document file, +// unparseable YAML, missing section/entry/field, a name mismatch at the pinned +// index) skips the whole call and returns the original content with a +// diagnostic — the writer must never guess inside a build directive. +func PatchKustomization(path string, content []byte, edits []KustomizationEdit) (EditResult, []Diagnostic) { + skip := func(format string, args ...interface{}) (EditResult, []Diagnostic) { + return EditResult{Content: content, Mode: EditSkipped}, + []Diagnostic{diag(DiagWarning, Location{Path: path}, format, args...)} } - target := docs[idx].body - root, empty, err := decodeDoc(target) - if err != nil || empty || root.Kind != yaml.MappingNode { - return skip("kustomization %s is not an editable mapping document", path) + docs, idx, root, reason, ok := locateKustomizationDocument(path, content) + if !ok { + return skip("%s", reason) } + target := docs[idx].body for _, e := range edits { if err := applyKustomizationEdit(root, e); err != nil { return skip("kustomization %s: %v", path, err) @@ -110,6 +126,58 @@ func applyKustomizationEdit(root *yaml.Node, e KustomizationEdit) error { return nil } +// AppendKustomizationResource adds one entry to an existing kustomization.yaml's +// resources: sequence — the mechanism half of F4's "add to the right kustomize +// file" (docs/design/manifest/version2/gittarget-new-file-placement-rules.md): a +// new sibling file placed inside a kustomize-governed directory must also be named +// in that directory's resources: list, or kustomize never renders it. +// +// It is idempotent: if entry already appears in the sequence, the call is a no-op +// (EditNoChange), never a duplicate append. All-or-nothing like PatchKustomization: +// a multi-document file, unparseable YAML, or a document with no existing +// resources: sequence skips the whole call with a diagnostic — the writer never +// invents a resources: key that is not already there, mirroring F1's "never +// creates a kustomization file" boundary one level down (never creates a +// resources: section either). +func AppendKustomizationResource(path string, content []byte, entry string) (EditResult, []Diagnostic) { + skip := func(format string, args ...interface{}) (EditResult, []Diagnostic) { + return EditResult{Content: content, Mode: EditSkipped}, + []Diagnostic{diag(DiagWarning, Location{Path: path}, format, args...)} + } + + docs, idx, root, reason, ok := locateKustomizationDocument(path, content) + if !ok { + return skip("%s", reason) + } + target := docs[idx].body + + section := nodeMapGet(root, "resources") + if section == nil || section.Kind != yaml.SequenceNode { + return skip("kustomization %s has no resources sequence", path) + } + for _, item := range section.Content { + if item.Kind == yaml.ScalarNode && strings.TrimSpace(item.Value) == strings.TrimSpace(entry) { + return EditResult{Content: content, Mode: EditNoChange}, nil + } + } + section.Content = append(section.Content, &yaml.Node{ + Kind: yaml.ScalarNode, + Tag: "!!str", + Value: entry, + }) + + encoded, err := encodeNode(root) + if err != nil { + return skip("kustomization %s: re-encode failed: %v", path, err) + } + body := reskinDocument(target, string(encoded)) + if body == target { + return EditResult{Content: content, Mode: EditNoChange}, nil + } + docs[idx].body = body + return EditResult{Content: []byte(joinDocuments(docs)), Mode: EditPatched}, nil +} + // setOverrideScalar writes the new value, keeping the value string-typed for the // image fields (the encoder quotes "1.29"-style values when the tag is !!str) and // integer-typed for count. An existing quoting style is kept; other styles reset diff --git a/internal/git/manifestedit/kustomization_test.go b/internal/git/manifestedit/kustomization_test.go index b976c383..520aeb3d 100644 --- a/internal/git/manifestedit/kustomization_test.go +++ b/internal/git/manifestedit/kustomization_test.go @@ -152,3 +152,59 @@ func TestPatchKustomization_RefusalsLeaveContentUntouched(t *testing.T) { }) } } + +func TestAppendKustomizationResource_AddsEntryPreservingHandAuthoring(t *testing.T) { + res, diags := AppendKustomizationResource("kustomization.yaml", []byte(kustomizationFixture), "debug-toolbox.yaml") + if res.Mode != EditPatched { + t.Fatalf("Mode = %q, want patched (diags %+v)", res.Mode, diags) + } + got := string(res.Content) + for _, want := range []string{ + "# pin the app image here", + "resources:\n - deployment.yaml\n - debug-toolbox.yaml\n", + } { + if !strings.Contains(got, want) { + t.Errorf("want %q in:\n%s", want, got) + } + } + // Unrelated sections stay put. + if !strings.Contains(got, "namespace: app") || !strings.Contains(got, "count: 3") { + t.Errorf("unrelated fields must be untouched:\n%s", got) + } +} + +func TestAppendKustomizationResource_IdempotentWhenAlreadyListed(t *testing.T) { + res, _ := AppendKustomizationResource("kustomization.yaml", []byte(kustomizationFixture), "deployment.yaml") + if res.Mode != EditNoChange { + t.Fatalf("Mode = %q, want no-change for an entry that is already listed", res.Mode) + } + if string(res.Content) != kustomizationFixture { + t.Errorf("a no-op must leave the bytes byte-identical") + } +} + +func TestAppendKustomizationResource_RefusalsLeaveContentUntouched(t *testing.T) { + cases := []struct { + name string + content string + }{ + {"no resources sequence", "namespace: app\nimages:\n - name: x\n newTag: \"1\"\n"}, + {"resources is not a sequence", "resources: not-a-list\n"}, + {"multi-document file", kustomizationFixture + "---\nnamespace: other\n"}, + {"unparseable", "resources: [::\n"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + res, diags := AppendKustomizationResource("kustomization.yaml", []byte(tc.content), "new.yaml") + if res.Mode != EditSkipped { + t.Fatalf("Mode = %q, want skipped", res.Mode) + } + if string(res.Content) != tc.content { + t.Errorf("a refused append must leave the bytes untouched") + } + if len(diags) == 0 { + t.Errorf("a refusal must carry a diagnostic") + } + }) + } +} diff --git a/internal/git/pending_writes.go b/internal/git/pending_writes.go index 067a1408..0d481d52 100644 --- a/internal/git/pending_writes.go +++ b/internal/git/pending_writes.go @@ -7,6 +7,9 @@ import ( "errors" "fmt" "strings" + + v1alpha3 "github.com/ConfigButler/gitops-reverser/api/v1alpha3" + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" ) func (w *BranchWorker) buildGroupedPendingWrite(ctx context.Context, events []Event) (*PendingWrite, error) { @@ -164,9 +167,45 @@ func (w *BranchWorker) resolveTargetMetadata( Path: target.Spec.Path, BootstrapOptions: buildBootstrapOptions(encryptionConfig), EncryptionConfig: encryptionConfig, + Placement: resolvePlacementPolicy(target.Spec.Placement), }, nil } +// resolvePlacementPolicy converts the CRD's declared placement spec into the +// package-local shape manifestanalyzer.LocateNew consumes. Kept as a plain field- +// for-field copy (not a shared type) so manifestanalyzer stays free of any +// Kubernetes API type dependency; see PlacementPolicy's doc comment. +func resolvePlacementPolicy(spec *v1alpha3.GitTargetPlacementSpec) *manifestanalyzer.PlacementPolicy { + if spec == nil { + return nil + } + return &manifestanalyzer.PlacementPolicy{ + ByType: spec.ByType, + Default: spec.Default, + } +} + +// placementPolicyForBase finds the placement policy for the GitTarget that owns +// base among targets. base is already a sanitized subtree key (groupEventsByBase +// runs it through sanitizePath), so md.Path must be sanitized the same way before +// comparing — otherwise a root target, whose spec.path is "." but whose sanitized +// base is "", would never match and would silently drop its declared placement on +// the live-write path (resync resolves target.Placement directly, so the two paths +// would diverge). GitTarget paths never overlap, so at most one target can match; a +// base with no matching target (e.g. an event whose target metadata could not be +// resolved) gets no declared policy, falling through to sibling inference. +func placementPolicyForBase( + targets map[pendingTargetKey]ResolvedTargetMetadata, + base string, +) *manifestanalyzer.PlacementPolicy { + for _, md := range targets { + if sanitizePath(md.Path) == base { + return md.Placement + } + } + return nil +} + // MessageKind is derived from the pending write's shape. func (p PendingWrite) MessageKind() CommitMessageKind { if p.Kind == PendingWriteAtomic || p.Kind == PendingWriteResync { diff --git a/internal/git/pending_writes_test.go b/internal/git/pending_writes_test.go index f285ac01..6800659f 100644 --- a/internal/git/pending_writes_test.go +++ b/internal/git/pending_writes_test.go @@ -229,3 +229,55 @@ func TestExecutor_PendingWrites_PreservesArrivalOrder(t *testing.T) { assert.Equal(t, "[UPDATE] v1/configmaps/c", second.Message) assert.Equal(t, "[UPDATE] v1/configmaps/a", first.Message) } + +// TestPlacementPolicyForBase_RootTargetMatchesSanitizedBase pins the fix for a root +// GitTarget silently dropping its declared placement on the live-write path: +// groupEventsByBase keys events by sanitizePath(event.Path), which collapses a root +// target's "." to "", so the lookup must sanitize md.Path the same way or "." would +// never equal "" and the policy would come back nil (falling back to sibling/canonical +// placement, diverging from resync which resolves target.Placement directly). +func TestPlacementPolicyForBase_RootTargetMatchesSanitizedBase(t *testing.T) { + policy := resolvePlacementPolicy(&configv1alpha3.GitTargetPlacementSpec{Default: "all.yaml"}) + targets := map[pendingTargetKey]ResolvedTargetMetadata{ + {Name: "root", Namespace: "default"}: { + Name: "root", Namespace: "default", Path: ".", Placement: policy, + }, + } + + // A root target's events group under the sanitized base "". + got := placementPolicyForBase(targets, "") + require.NotNil(t, got, "a root target's declared placement must be found on the live path") + assert.Same(t, policy, got) +} + +func TestPlacementPolicyForBase_NonRootAndNoMatch(t *testing.T) { + policy := resolvePlacementPolicy(&configv1alpha3.GitTargetPlacementSpec{Default: "all.yaml"}) + targets := map[pendingTargetKey]ResolvedTargetMetadata{ + {Name: "sub", Namespace: "default"}: { + Name: "sub", Namespace: "default", Path: "live-cluster", Placement: policy, + }, + } + + assert.Same(t, policy, placementPolicyForBase(targets, "live-cluster")) + assert.Nil(t, placementPolicyForBase(targets, "somewhere-else"), + "a base no target owns gets no declared policy") +} + +func TestResolvePlacementPolicy_NilSpec(t *testing.T) { + if got := resolvePlacementPolicy(nil); got != nil { + t.Errorf("resolvePlacementPolicy(nil) = %+v, want nil", got) + } +} + +func TestResolvePlacementPolicy_ConvertsFieldForField(t *testing.T) { + spec := &configv1alpha3.GitTargetPlacementSpec{ + ByType: map[string]string{"v1/configmaps": "{namespace}/configmaps.yaml"}, + Default: "all.yaml", + } + + got := resolvePlacementPolicy(spec) + + require.NotNil(t, got) + assert.Equal(t, spec.ByType, got.ByType) + assert.Equal(t, spec.Default, got.Default) +} diff --git a/internal/git/placement_test.go b/internal/git/placement_test.go new file mode 100644 index 00000000..794b7b32 --- /dev/null +++ b/internal/git/placement_test.go @@ -0,0 +1,458 @@ +// SPDX-License-Identifier: Apache-2.0 + +package git + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + gogit "github.com/go-git/go-git/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// newConfigMapEvent builds a CREATE event for a new ConfigMap with no existing +// document anywhere in the fixture — the only case F4 placement runs for. +func newConfigMapEvent(name, namespace string) Event { + return Event{ + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]interface{}{"name": name, "namespace": namespace}, + "data": map[string]interface{}{"color": "blue"}, + }}, + Identifier: types.NewResourceIdentifier("", "v1", "configmaps", namespace, name), + Operation: "CREATE", + } +} + +func applyEventsWithPolicy( + t *testing.T, + worktree *gogit.Worktree, + policy *manifestanalyzer.PlacementPolicy, + events ...Event, +) bool { + t.Helper() + w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", events, policy) + require.NoError(t, err) + return changed +} + +func TestPlacement_DeclaredPolicy_NewFile(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + policy := &manifestanalyzer.PlacementPolicy{ + ByType: map[string]string{"v1/configmaps": "{namespace}/configmaps.yaml"}, + } + + changed := applyEventsWithPolicy(t, worktree, policy, newConfigMapEvent("cache", "app")) + require.True(t, changed) + + got, err := os.ReadFile(filepath.Join(root, "app", "configmaps.yaml")) + require.NoError(t, err) + assert.Contains(t, string(got), "name: cache") + assert.Contains(t, string(got), "color: blue") +} + +func TestPlacement_SiblingInference_BesideExistingFile(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + seedPlacedManifest(t, worktree, "overlays/test/configmap-existing.yaml", + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: existing\n namespace: podinfo-test\ndata:\n k: v\n") + + changed := applyEventsWithPolicy(t, worktree, nil, newConfigMapEvent("cache", "podinfo-test")) + require.True(t, changed) + + got, err := os.ReadFile(filepath.Join(root, "overlays/test/cache.yaml")) + require.NoError(t, err, "the new file should land beside its sibling, not at the canonical path") + assert.Contains(t, string(got), "name: cache") +} + +func TestPlacement_BundleAppend_ExistingMultiDocFile(t *testing.T) { + worktree := newWorktreeForTest(t) + seeded := "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: a\n namespace: app\ndata:\n k: v\n" + + "---\n" + + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: b\n namespace: app\ndata:\n k: v\n" + full := seedPlacedManifest(t, worktree, "all.yaml", seeded) + + changed := applyEventsWithPolicy(t, worktree, nil, newConfigMapEvent("cache", "app")) + require.True(t, changed) + + got, err := os.ReadFile(full) + require.NoError(t, err) + assert.Contains(t, string(got), "name: a", "the first existing document must survive") + assert.Contains(t, string(got), "name: b", "the second existing document must survive") + assert.Contains(t, string(got), "name: cache", "the new document must be appended") + assert.Equal(t, 3, strings.Count(string(got), "kind: ConfigMap"), + "exactly one document must be added, not a replace") +} + +// A new resource whose siblings are in a kustomize-namespace-inferred bundle +// must not write metadata.namespace into that bundle — otherwise an incidental +// resource sharing the namespace (e.g. a cluster-injected ConfigMap watched by +// too broad a WatchRule) would break the "no namespace: in this file" +// convention every other document in the bundle already follows. +func TestPlacement_BundleAppend_OmitsNamespaceInKustomizeContext(t *testing.T) { + worktree := newWorktreeForTest(t) + kustYAML := "namespace: app\nresources:\n - all.yaml\n" + seedPlacedManifest(t, worktree, "overlays/test/kustomization.yaml", kustYAML) + seeded := "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: a\ndata:\n k: v\n" + + "---\n" + + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: b\ndata:\n k: v\n" + full := seedPlacedManifest(t, worktree, "overlays/test/all.yaml", seeded) + + changed := applyEventsWithPolicy(t, worktree, nil, newConfigMapEvent("cache", "app")) + require.True(t, changed) + + got, err := os.ReadFile(full) + require.NoError(t, err) + assert.Contains(t, string(got), "name: a", "the first existing document must survive") + assert.Contains(t, string(got), "name: b", "the second existing document must survive") + assert.Contains(t, string(got), "name: cache", "the new document must be appended") + assert.NotContains(t, string(got), "namespace:", + "the new document must not break the bundle's namespace-omitted convention") +} + +// A misconfigured declared template (missing {name}) makes two distinct secrets +// collide on the same rendered path. createNew must skip the write rather than +// crash or corrupt the existing file — the next event or resync retries once the +// policy is fixed. +func TestPlacement_SensitiveCollision_SkipsWithoutCrashing(t *testing.T) { + worktree := newWorktreeForTest(t) + existing := "apiVersion: v1\nkind: Secret\nmetadata:\n name: other\n namespace: app\nsops:\n version: \"3\"\n" + full := seedPlacedManifest(t, worktree, "secrets/app.sops.yaml", existing) + policy := &manifestanalyzer.PlacementPolicy{ + ByType: map[string]string{"v1/secrets": "secrets/{namespace}.sops.yaml"}, + } + + event := Event{ + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{"name": "api-token", "namespace": "app"}, + }}, + Identifier: types.NewResourceIdentifier("", "v1", "secrets", "app", "api-token"), + Operation: "CREATE", + } + w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{})} + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", []Event{event}, policy) + + require.NoError(t, err, "a placement conflict must be skipped, not returned as a batch error") + assert.False(t, changed, "no file should be written when placement cannot be resolved safely") + + got, readErr := os.ReadFile(full) + require.NoError(t, readErr) + assert.Equal(t, existing, string(got), "the existing secret must survive byte-for-byte") +} + +func TestPlacement_KustomizeEntryAppended_SameCommit(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + kustYAML := "# overlay for test\n" + + "namespace: podinfo-test\n" + + "resources:\n" + + " - deployment.yaml\n" + seedPlacedManifest(t, worktree, "overlays/test/kustomization.yaml", kustYAML) + seedPlacedManifest(t, worktree, "overlays/test/deployment.yaml", + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: web\n namespace: podinfo-test\n") + + changed := applyEventsWithPolicy(t, worktree, nil, newConfigMapEvent("debug-toolbox", "podinfo-test")) + require.True(t, changed) + + newFile, err := os.ReadFile(filepath.Join(root, "overlays/test/debug-toolbox.yaml")) + require.NoError(t, err, "the new resource should land inside the overlay directory") + assert.Contains(t, string(newFile), "name: debug-toolbox") + + kust, err := os.ReadFile(filepath.Join(root, "overlays/test/kustomization.yaml")) + require.NoError(t, err) + kustStr := string(kust) + assert.Contains(t, kustStr, "# overlay for test", "hand-authored comments must survive") + assert.Contains(t, kustStr, "- deployment.yaml", "the existing entry must survive") + assert.Contains(t, kustStr, "- debug-toolbox.yaml", "the new file must be added to resources:") +} + +func TestPlacement_KustomizeEntryIdempotent_OnRepeatedApply(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + kustYAML := "namespace: podinfo-test\nresources:\n - deployment.yaml\n" + seedPlacedManifest(t, worktree, "overlays/test/kustomization.yaml", kustYAML) + seedPlacedManifest(t, worktree, "overlays/test/deployment.yaml", + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: web\n namespace: podinfo-test\n") + + event := newConfigMapEvent("debug-toolbox", "podinfo-test") + require.True(t, applyEventsWithPolicy(t, worktree, nil, event)) + // A second apply of the same create (e.g. a resync re-observing the same live + // object) must not duplicate the resources: entry. + applyEventsWithPolicy(t, worktree, nil, event) + + kust, err := os.ReadFile(filepath.Join(root, "overlays/test/kustomization.yaml")) + require.NoError(t, err) + assert.Equal(t, 1, strings.Count(string(kust), "debug-toolbox.yaml"), + "the resources: entry must appear exactly once") +} + +// A kustomization whose resources: field is malformed (not a sequence) is still +// accepted by the analyzer (only specific disallowed keys, not resources: shape, +// disqualify a kustomization) — so the writer must still place the resource's own +// file, but the resources: entry add is skipped rather than corrupting the +// kustomization, leaving it exactly as it was for a human to fix. +func TestPlacement_KustomizeEntryAppendSkipped_MalformedResourcesField(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem.Root() + kustYAML := "namespace: app\nresources: not-a-list\n" + seedPlacedManifest(t, worktree, "overlays/test/kustomization.yaml", kustYAML) + + changed := applyEventsWithPolicy(t, worktree, nil, newConfigMapEvent("cache", "app")) + require.True(t, changed, "the new resource's own file must still be written") + + _, err := os.Stat(filepath.Join(root, "overlays/test/cache.yaml")) + require.NoError(t, err, "the resource itself is written even though the kustomize entry could not be added") + + kust, err := os.ReadFile(filepath.Join(root, "overlays/test/kustomization.yaml")) + require.NoError(t, err) + // Byte-exact, not YAMLEq: the guarantee under test is that a skipped edit + // returns the original content untouched, not merely a semantically + // equivalent one. + assert.Equal(t, kustYAML, string(kust), //nolint:testifylint + "a malformed resources: field must be left untouched, not corrupted") +} + +func newTestWriteBatch(t *testing.T) *writeBatch { + t.Helper() + writer := newContentWriter(types.SensitiveResourcePolicy{}) + return newWriteBatch(context.Background(), writer, nil, manifestanalyzer.FolderScan{}, nil) +} + +func TestAppendYAMLDocument(t *testing.T) { + if got := appendYAMLDocument(nil, []byte("a: 1\n")); string(got) != "a: 1\n" { + t.Errorf("empty existing should return newDoc verbatim, got %q", got) + } + if got := appendYAMLDocument([]byte("a: 1"), []byte("b: 2\n")); string(got) != "a: 1\n---\nb: 2\n" { + t.Errorf("got %q, want a newline inserted before the separator when existing lacks a trailing one", got) + } + if got := appendYAMLDocument([]byte("a: 1\n"), []byte("b: 2\n")); string(got) != "a: 1\n---\nb: 2\n" { + t.Errorf("got %q, want no extra blank line when existing already ends in a newline", got) + } +} + +func TestAppendNewDocument_BuildContentErrorIsPropagated(t *testing.T) { + wb := newTestWriteBatch(t) + event := Event{ + Object: nil, + Identifier: types.NewResourceIdentifier("", "v1", "configmaps", "app", "x"), + } + + _, err := wb.appendNewDocument(context.Background(), event, "all.yaml") + + require.Error(t, err, "a nil object cannot be marshaled, and that error must not be swallowed") +} + +// If the kustomization file was removed by an earlier delete in the same batch +// (an edge case that should not happen in practice, since kustomization.yaml is +// a retained build directive, but the writer must never panic on it), +// appendKustomizationResource must no-op rather than dereference a nil buffer. +func TestAppendKustomizationResource_VanishedBuffer_NoPanic(t *testing.T) { + wb := newTestWriteBatch(t) + kustPath := "overlays/test/kustomization.yaml" + wb.buffers[kustPath] = &fileBuffer{rel: kustPath, current: nil} + placement := manifestanalyzer.PlacementResult{ + Path: "overlays/test/new.yaml", + Kustomization: &manifestanalyzer.KustomizationInfo{Path: kustPath, Resources: []string{"deployment.yaml"}}, + } + event := Event{Identifier: types.NewResourceIdentifier("", "v1", "configmaps", "app", "new")} + + assert.NotPanics(t, func() { + wb.appendKustomizationResource(context.Background(), event, placement) + }) + assert.Nil(t, wb.buffers[kustPath].current, "a vanished buffer must not be resurrected") +} + +// Two new resources that both render to the same brand-new declared path (a +// collision LocateNew cannot see coming, since it only consults the pre-batch +// store) must form one deterministic multi-document file — never silently +// overwrite one another — regardless of which event the writer processes +// first. This is the design doc's "if several new plaintext resources in one +// plan render to the same path, write a multi-document file in deterministic +// resource-identity order". +func TestPlacement_ColdBundleCollision_BothSurviveRegardlessOfOrder(t *testing.T) { + policy := &manifestanalyzer.PlacementPolicy{Default: "all.yaml"} + first := newConfigMapEvent("alpha", "app") + second := newConfigMapEvent("beta", "app") + + forward := newWorktreeForTest(t) + changed := applyEventsWithPolicy(t, forward, policy, first, second) + require.True(t, changed) + forwardBody, err := os.ReadFile(filepath.Join(forward.Filesystem.Root(), "all.yaml")) + require.NoError(t, err) + + reversed := newWorktreeForTest(t) + changed = applyEventsWithPolicy(t, reversed, policy, second, first) + require.True(t, changed) + reversedBody, err := os.ReadFile(filepath.Join(reversed.Filesystem.Root(), "all.yaml")) + require.NoError(t, err) + + assert.Contains(t, string(forwardBody), "name: alpha", "the first resource must survive") + assert.Contains(t, string(forwardBody), "name: beta", "the second resource must survive") + assert.Equal(t, 2, strings.Count(string(forwardBody), "kind: ConfigMap"), "both must land as separate documents") + assert.Equal(t, string(forwardBody), string(reversedBody), + "the resulting file must not depend on event processing order") +} + +// A third collision on the same batch-cold path must also survive and stay +// sorted, proving the fix generalizes beyond exactly two resources. +func TestPlacement_ColdBundleCollision_ThreeResourcesAllSurvive(t *testing.T) { + worktree := newWorktreeForTest(t) + policy := &manifestanalyzer.PlacementPolicy{Default: "all.yaml"} + + changed := applyEventsWithPolicy(t, worktree, policy, + newConfigMapEvent("charlie", "app"), + newConfigMapEvent("alpha", "app"), + newConfigMapEvent("bravo", "app"), + ) + require.True(t, changed) + + got, err := os.ReadFile(filepath.Join(worktree.Filesystem.Root(), "all.yaml")) + require.NoError(t, err) + body := string(got) + assert.Equal(t, 3, strings.Count(body, "kind: ConfigMap")) + // Sorted by resource identity ("…/app/alpha" < "…/app/bravo" < "…/app/charlie"), + // independent of the arrival order above. + assert.Less(t, strings.Index(body, "name: alpha"), strings.Index(body, "name: bravo")) + assert.Less(t, strings.Index(body, "name: bravo"), strings.Index(body, "name: charlie")) +} + +// A sensitive resource must never be merged into a shared file, even when the +// collision is with another new resource within the same batch (not a +// pre-existing file, which the analyzer-level test already covers). +func TestPlacement_ColdBundleCollision_SensitiveNeverMerged(t *testing.T) { + worktree := newWorktreeForTest(t) + policy := &manifestanalyzer.PlacementPolicy{ + ByType: map[string]string{"v1/secrets": "secrets/{namespace}.sops.yaml"}, + } + newSecretEvent := func(name string) Event { + return Event{ + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{"name": name, "namespace": "app"}, + }}, + Identifier: types.NewResourceIdentifier("", "v1", "secrets", "app", name), + Operation: "CREATE", + } + } + writer := newContentWriter(types.SensitiveResourcePolicy{}) + writer.setEncryptor(&stubEncryptor{result: []byte( + "apiVersion: v1\nkind: Secret\nmetadata:\n name: first\n namespace: app\n" + + "data:\n k: ENC[AES256,data:x,iv:y,tag:z]\nsops:\n version: 3.9.0\n", + )}, "test-scope") + w := &BranchWorker{contentWriter: writer} + + changed, err := w.flushEventsToWorktree( + context.Background(), worktree, "", []Event{newSecretEvent("first"), newSecretEvent("second")}, policy, + ) + + require.NoError(t, err) + assert.True(t, changed, "the first secret must still be written") + + got, readErr := os.ReadFile(filepath.Join(worktree.Filesystem.Root(), "secrets/app.sops.yaml")) + require.NoError(t, readErr) + assert.Equal(t, 1, strings.Count(string(got), "kind: Secret"), + "the second secret must be skipped, never merged into the first's file") +} + +// A sensitive and a plaintext resource that a bundling default routes to the same +// brand-new file must never co-mingle in it, whichever event the writer processes +// first. This is the same-batch half of Option B2's write-safety guard: the single +// declared map is consulted for both classes, so a Secret and a ConfigMap can now +// resolve to one path, and the guard keeps encrypted and cleartext documents out of +// the same file (the first arrival wins; the other is skipped and retried). +func TestPlacement_ColdBundleCollision_SensitiveAndPlaintextNeverMix(t *testing.T) { + policy := &manifestanalyzer.PlacementPolicy{Default: "all.yaml"} + secretEvent := Event{ + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]interface{}{"name": "cred", "namespace": "app"}, + }}, + Identifier: types.NewResourceIdentifier("", "v1", "secrets", "app", "cred"), + Operation: "CREATE", + } + configMapEvent := newConfigMapEvent("cache", "app") + + newWriter := func() *contentWriter { + writer := newContentWriter(types.SensitiveResourcePolicy{}) + writer.setEncryptor(&stubEncryptor{result: []byte( + "apiVersion: v1\nkind: Secret\nmetadata:\n name: cred\n namespace: app\n" + + "data:\n k: ENC[AES256,data:x,iv:y,tag:z]\nsops:\n version: 3.9.0\n", + )}, "test-scope") + return writer + } + + // Secret first: the Secret wins the file, the ConfigMap is skipped. + secretFirst := newWorktreeForTest(t) + wsf := &BranchWorker{contentWriter: newWriter()} + _, err := wsf.flushEventsToWorktree( + context.Background(), secretFirst, "", []Event{secretEvent, configMapEvent}, policy, + ) + require.NoError(t, err) + secretFirstBody, readErr := os.ReadFile(filepath.Join(secretFirst.Filesystem.Root(), "all.yaml")) + require.NoError(t, readErr) + assert.Contains(t, string(secretFirstBody), "kind: Secret") + assert.NotContains(t, string(secretFirstBody), "kind: ConfigMap", + "a plaintext resource must never join a file that already holds an encrypted document") + + // ConfigMap first: the ConfigMap wins the file, the Secret is skipped. + configMapFirst := newWorktreeForTest(t) + wcf := &BranchWorker{contentWriter: newWriter()} + _, err = wcf.flushEventsToWorktree( + context.Background(), configMapFirst, "", []Event{configMapEvent, secretEvent}, policy, + ) + require.NoError(t, err) + configMapFirstBody, readErr := os.ReadFile(filepath.Join(configMapFirst.Filesystem.Root(), "all.yaml")) + require.NoError(t, readErr) + assert.Contains(t, string(configMapFirstBody), "kind: ConfigMap") + assert.NotContains(t, string(configMapFirstBody), "kind: Secret", + "a sensitive resource must never share a file with a plaintext resource") +} + +// Resync (M8) folds its whole desired snapshot through the same createNew path +// as steady-state events, so the same brand-new-path collision can occur there +// too — proving the fix covers both entry points the design doc calls out. +func TestPlacement_ColdBundleCollision_ViaResync(t *testing.T) { + worktree := newWorktreeForTest(t) + policy := &manifestanalyzer.PlacementPolicy{Default: "all.yaml"} + desired := []manifestanalyzer.DesiredResource{ + { + Resource: types.NewResourceIdentifier("", "v1", "configmaps", "app", "alpha"), + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "v1", "kind": "ConfigMap", + "metadata": map[string]interface{}{"name": "alpha", "namespace": "app"}, + }}, + }, + { + Resource: types.NewResourceIdentifier("", "v1", "configmaps", "app", "beta"), + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "v1", "kind": "ConfigMap", + "metadata": map[string]interface{}{"name": "beta", "namespace": "app"}, + }}, + }, + } + + w := &BranchWorker{contentWriter: newContentWriter(types.SensitiveResourcePolicy{}), mapper: configMapMapper()} + _, changed, err := w.applyResyncToWorktree(context.Background(), worktree, "", desired, nil, policy) + + require.NoError(t, err) + assert.True(t, changed) + + got, readErr := os.ReadFile(filepath.Join(worktree.Filesystem.Root(), "all.yaml")) + require.NoError(t, readErr) + assert.Equal(t, 2, strings.Count(string(got), "kind: ConfigMap"), "both resync creates must survive") +} diff --git a/internal/git/plan_flush.go b/internal/git/plan_flush.go index f38c9b96..0e7fdca7 100644 --- a/internal/git/plan_flush.go +++ b/internal/git/plan_flush.go @@ -19,6 +19,7 @@ import ( "github.com/ConfigButler/gitops-reverser/internal/git/manifestedit" "github.com/ConfigButler/gitops-reverser/internal/manifestanalyzer" "github.com/ConfigButler/gitops-reverser/internal/manifestreport" + "github.com/ConfigButler/gitops-reverser/internal/types" "github.com/ConfigButler/gitops-reverser/internal/typeset" ) @@ -41,6 +42,7 @@ func (w *BranchWorker) flushEventsToWorktree( worktree *gogit.Worktree, base string, events []Event, + policy *manifestanalyzer.PlacementPolicy, ) (bool, error) { root := worktree.Filesystem.Root() scan, err := scanWorktreeSubtree(filepath.Join(root, base)) @@ -48,7 +50,7 @@ func (w *BranchWorker) flushEventsToWorktree( return false, err } - batch := newWriteBatch(ctx, w.contentWriter, w.mapper, scan) + batch := newWriteBatch(ctx, w.contentWriter, w.mapper, scan, policy) if err := batch.refusal(); err != nil { return false, err } @@ -71,6 +73,36 @@ type writeBatch struct { docLoc map[*manifestanalyzer.DocumentModel]manifestanalyzer.RecordRef contentByPath map[string][]byte buffers map[string]*fileBuffer + // policy is the GitTarget's declared new-file placement policy (F4), consulted + // only for a resource with no existing document. nil means no declared policy — + // placement falls through to sibling inference and then the canonical path. + policy *manifestanalyzer.PlacementPolicy + // coldBundles tracks, per path, the new resources this batch has placed at a + // path that held no document before the batch started (keyed the same as + // buffers). It exists so several new resources that render to the same + // brand-new path — a collision LocateNew resolves against the pre-batch store + // and therefore cannot see coming — form one deterministic, resource-identity- + // sorted multi-document file instead of each writeWholeFile call silently + // discarding the one before it. See + // docs/design/manifest/version2/gittarget-new-file-placement-rules.md, + // "Collision and append behavior": "if several new plaintext resources in one + // plan render to the same path, write a multi-document file in deterministic + // resource-identity order." + coldBundles map[string][]coldBundleMember +} + +// coldBundleMember is one new document contributing to a brand-new shared bundle +// file within this batch. Retained (rather than re-parsed from buf.current) so a +// later collision on the same path can re-sort and rebuild the whole file from +// scratch, independent of which new resource's event the writer processed first. +type coldBundleMember struct { + identifier types.ResourceIdentifier + content []byte + // sensitive records whether this member is an encrypted (sensitive) resource, so + // createNew can refuse to co-mingle sensitive and plaintext documents in one + // brand-new file regardless of the order their events arrived (Option B2's + // write-safety guard — see createNew). + sensitive bool } func newWriteBatch( @@ -78,6 +110,7 @@ func newWriteBatch( writer eventContentWriter, mapper typeset.Lookup, scan manifestanalyzer.FolderScan, + policy *manifestanalyzer.PlacementPolicy, ) *writeBatch { // The writer allowlist retains build directives (kustomization.yaml) and the operator's // own .sops.yaml bootstrap config outside the managed model — these are auxiliary input, @@ -104,6 +137,7 @@ func newWriteBatch( docLoc: store.DocumentLocations(), contentByPath: contentByPath, buffers: map[string]*fileBuffer{}, + policy: policy, } } @@ -160,6 +194,13 @@ const ( upsertNoChange upsertOutcome = iota upsertCreated upsertUpdated + // upsertSkippedUnsafe is a deliberate, fail-safe refusal to write a resource: + // its placement could not be resolved safely, or writing would co-mingle a + // sensitive and a plaintext document, or would overwrite a multi-document file. + // It is distinct from upsertNoChange (a genuine no-op) so the resync path can + // count it and surface it, rather than have a not-mirrored resource vanish with + // no signal (F4 Option B2's fail-safe skips — see createNew/writeWholeFile). + upsertSkippedUnsafe ) // applyEvent folds one event into the batch: a field patch sets bounded fields on an @@ -186,8 +227,8 @@ func (wb *writeBatch) applyEvent(ctx context.Context, event Event) error { // a sensitive document is re-encrypted wholesale AT ITS EXISTING PATH (never patched in // place — that would drop the SOPS metadata and write the secret back in cleartext, and // never at the canonical path, which would orphan the moved copy). A resource with no -// existing document is a new file at the canonical placement path. It returns what it -// did to the bytes (created / updated / no change). +// existing document is placed by createNew (F4). It returns what it did to the bytes +// (created / updated / no change). func (wb *writeBatch) applyUpsert(ctx context.Context, event Event) (upsertOutcome, error) { if id, ok := manifestIdentity(event.Object); ok { if dm := wb.store.ByManifestIdentity[id]; dm != nil { @@ -198,7 +239,209 @@ func (wb *writeBatch) applyUpsert(ctx context.Context, event Event) (upsertOutco return wb.patchExisting(ctx, event, filePath, id, dm) } } - return wb.writeWholeFile(ctx, event, wb.writer.filePathForIdentifier(event.Identifier)) + return wb.createNew(ctx, event) +} + +// createNew resolves the placement of a resource with no existing document — +// declared policy (Option B), sibling inference (Option C), or the canonical +// fallback — per docs/design/manifest/version2/gittarget-new-file-placement-rules.md, +// adds the kustomize resources: entry the placement may require, and writes the new +// document: a brand-new file, or an additional document appended to an existing +// accepted plaintext bundle. A placement LocateNew cannot honour safely (today, only +// a sensitive resource whose resolved path collides with an existing file) is logged +// and left unwritten rather than risking a mis-write; the next event or resync +// retries it once the conflict is resolved (e.g. the placement policy is fixed). +func (wb *writeBatch) createNew(ctx context.Context, event Event) (upsertOutcome, error) { + kind := "" + if event.Object != nil { + kind = event.Object.GetKind() + } + sensitive := wb.writer.isSensitiveIdentifier(event.Identifier) + placement, err := manifestanalyzer.LocateNew(wb.store, wb.policy, manifestanalyzer.PlacementRequest{ + Identifier: event.Identifier, + Kind: kind, + Sensitive: sensitive, + }) + if err != nil { + log.FromContext(ctx).Info("Skipping new resource: placement could not be resolved safely", + "resource", event.Identifier.String(), "reason", err.Error()) + return upsertSkippedUnsafe, nil + } + + if placement.Kustomization != nil { + wb.appendKustomizationResource(ctx, event, placement) + } + + // A destination that infers its namespace from build context (a kustomization's + // 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 { + event.Object = event.Object.DeepCopy() + event.Object.SetNamespace("") + } + + if placement.Append { + return wb.appendNewDocument(ctx, event, placement.Path) + } + + buf := wb.buffer(placement.Path) + if buf.original == nil { + // Nothing occupied this path before the batch started, so every write + // here is a new resource: this event, or an earlier one in the same + // batch that rendered to the same path (a collision LocateNew cannot + // see coming — it only ever consults the pre-batch store). Route + // through the cold-bundle path so a collision forms a deterministic + // multi-document file instead of a second writeWholeFile silently + // discarding whichever new resource arrived first. + // + // A sensitive resource must never share a file (with anything), and a + // plaintext resource must never join a bundle that already holds a + // sensitive member — either way the file would co-mingle encrypted and + // plaintext documents. Skip rather than mix; the next event or resync + // retries once the placement policy stops routing them together. This is + // the same-batch half of Option B2's write-safety guard (the cross-batch + // half — appending into an already-encrypted file — is refused in + // LocateNew/finishPlacement). + if buf.current != nil && (sensitive || wb.coldBundleHasSensitive(placement.Path)) { + log.FromContext(ctx).Info( + "Skipping new resource: sensitive and plaintext resources must not share a new file", + "resource", event.Identifier.String(), "file", placement.Path, "sensitive", sensitive) + return upsertSkippedUnsafe, nil + } + return wb.writeColdBundleMember(ctx, event, placement.Path, sensitive) + } + return wb.writeWholeFile(ctx, event, placement.Path) +} + +// writeColdBundleMember writes a resource with no existing document to rel, a +// path nothing occupied before this batch started. Because LocateNew resolves +// every event against the pre-batch store snapshot (P2 of the design doc), +// several new resources rendering to the same brand-new path each look like the +// sole occupant to LocateNew, so a plain single-document write would let each +// one overwrite the last. Instead every member seen so far at rel (including +// this one) is re-sorted by resource identity and the file is rebuilt from +// scratch, so the result is independent of which new resource's event the +// writer processed first — see the design doc's "Collision and append +// behavior": "if several new plaintext resources in one plan render to the same +// path, write a multi-document file in deterministic resource-identity order." +// For the common single-member case this produces byte-identical output to a +// plain write. +func (wb *writeBatch) writeColdBundleMember( + ctx context.Context, + event Event, + rel string, + sensitive bool, +) (upsertOutcome, error) { + content, err := wb.writer.buildContentForWrite(ctx, event) + if err != nil { + return upsertNoChange, err + } + if wb.coldBundles == nil { + wb.coldBundles = map[string][]coldBundleMember{} + } + wb.coldBundles[rel] = append( + wb.coldBundles[rel], + coldBundleMember{identifier: event.Identifier, content: content, sensitive: sensitive}, + ) + members := wb.coldBundles[rel] + sort.Slice(members, func(i, j int) bool { + return members[i].identifier.Key() < members[j].identifier.Key() + }) + + var rebuilt []byte + for _, m := range members { + rebuilt = appendYAMLDocument(rebuilt, m.content) + } + wb.buffer(rel).current = rebuilt + return upsertCreated, nil +} + +// coldBundleHasSensitive reports whether any member already staged for the +// brand-new file at rel is an encrypted (sensitive) resource, so createNew can +// refuse to add a plaintext member that would co-mingle with it. +func (wb *writeBatch) coldBundleHasSensitive(rel string) bool { + for _, m := range wb.coldBundles[rel] { + if m.sensitive { + return true + } + } + return false +} + +// appendNewDocument adds a resource with no existing document as an additional +// document in an existing accepted plaintext file (a "bundle" placement). Unlike +// writeWholeFile it never replaces the file's existing bytes — every prior document +// in the buffer survives untouched, byte for byte; LocateNew never returns an +// Append placement for a sensitive resource (see its doc comment), so this path is +// plaintext-only. +func (wb *writeBatch) appendNewDocument(ctx context.Context, event Event, rel string) (upsertOutcome, error) { + content, err := wb.writer.buildContentForWrite(ctx, event) + if err != nil { + return upsertNoChange, err + } + buf := wb.buffer(rel) + buf.current = appendYAMLDocument(buf.current, content) + return upsertCreated, nil +} + +// appendYAMLDocument appends newDoc as an additional "---\n"-separated document +// after existing. existing is assumed to already be valid, accepted YAML (single- or +// multi-document); newDoc is assumed to be exactly one well-formed document +// (sanitize.MarshalToOrderedYAML's output, which always ends in a newline). +func appendYAMLDocument(existing, newDoc []byte) []byte { + if len(existing) == 0 { + return newDoc + } + const separator = "---\n" + out := make([]byte, 0, len(existing)+len(separator)+len(newDoc)) + out = append(out, existing...) + if out[len(out)-1] != '\n' { + out = append(out, '\n') + } + out = append(out, separator...) + out = append(out, newDoc...) + return out +} + +// appendKustomizationResource adds the new document's path to its resources: +// sequence as part of the same commit, so kustomize picks up the file createNew just +// placed inside the kustomization's directory — F4's "add to the right kustomize +// file." The entry is rendered relative to the kustomization's own directory +// (resources: entries are relative to the kustomization file, not the repo root). +// A failure here only drops the resources: entry (logged as a diagnostic); the +// resource's own file is still written, since a human can add the missing entry by +// hand and the next placement for that directory re-detects the gap. +func (wb *writeBatch) appendKustomizationResource( + ctx context.Context, + event Event, + placement manifestanalyzer.PlacementResult, +) { + k := placement.Kustomization + entry := placement.Path + if dir := path.Dir(k.Path); dir != "." { + if rel, err := filepath.Rel(dir, placement.Path); err == nil { + entry = filepath.ToSlash(rel) + } + } + + buf := wb.buffer(k.Path) + if buf.current == nil { + return // the kustomization vanished within this batch; nothing to edit + } + res, diags := manifestedit.AppendKustomizationResource(k.Path, buf.current, entry) + switch res.Mode { + case manifestedit.EditPatched: + buf.current = res.Content + log.FromContext(ctx).Info("Added resources: entry for new file", + "kustomization", k.Path, "entry", entry, "resource", event.Identifier.String()) + case manifestedit.EditNoChange: + case manifestedit.EditSkipped, manifestedit.EditDeleted, manifestedit.EditWholeReplace: + log.FromContext(ctx).Info("Could not add resources: entry for new file", + "kustomization", k.Path, "entry", entry, "resource", event.Identifier.String()) + logManifestDiagnostics(ctx, diags) + } } // applyFieldPatch folds a subresource field-patch event into the batch: it locates the @@ -470,7 +713,7 @@ func (wb *writeBatch) writeWholeFile(ctx context.Context, event Event, rel strin "file", rel, "resource", event.Identifier.String(), ) - return upsertNoChange, nil + return upsertSkippedUnsafe, nil } if bytes.Equal(buf.current, content) || manifestsAreSemanticallyEqual(buf.current, content) { return upsertNoChange, nil diff --git a/internal/git/plan_flush_test.go b/internal/git/plan_flush_test.go index 9dd64ba4..78626411 100644 --- a/internal/git/plan_flush_test.go +++ b/internal/git/plan_flush_test.go @@ -119,7 +119,7 @@ func TestPlanFlush_DeleteByGVROnlyFollowsMovedManifestViaMapper(t *testing.T) { }, Operation: "DELETE", } - changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", []Event{del}) + changed, err := w.flushEventsToWorktree(context.Background(), worktree, "", []Event{del}, nil) require.NoError(t, err) assert.True(t, changed, "the moved manifest must be deleted via the resolved resource identity") _, statErr := os.Stat(placedFull) diff --git a/internal/git/resync_flush.go b/internal/git/resync_flush.go index b082bb97..d92f5e5b 100644 --- a/internal/git/resync_flush.go +++ b/internal/git/resync_flush.go @@ -143,6 +143,7 @@ func (l *branchWorkerEventLoop) applyResync(req *ResyncRequest) { "updated", stats.Updated, "deleted", stats.Deleted, "skipped", stats.Skipped, + "placementSkipped", stats.PlacementSkipped, "pendingWrites", len(l.pendingWrites)) req.reply(ResyncResult{Stats: *stats}) } @@ -246,7 +247,9 @@ func (w *BranchWorker) executeResyncPendingWrite( return 0, fmt.Errorf("configure secret encryptor: %w", err) } - stats, anyChanges, err := w.applyResyncToWorktree(ctx, worktree, base, pendingWrite.Desired, pendingWrite.ScopeGVR) + stats, anyChanges, err := w.applyResyncToWorktree( + ctx, worktree, base, pendingWrite.Desired, pendingWrite.ScopeGVR, target.Placement, + ) if err != nil { return 0, err } @@ -281,7 +284,8 @@ func (w *BranchWorker) executeResyncPendingWrite( } log.FromContext(ctx).Info("git resync commit created", "created", stats.Created, "updated", stats.Updated, - "deleted", stats.Deleted, "skipped", stats.Skipped, "revision", pendingWrite.Revision) + "deleted", stats.Deleted, "skipped", stats.Skipped, + "placementSkipped", stats.PlacementSkipped, "revision", pendingWrite.Revision) return 1, nil } @@ -291,7 +295,8 @@ func (w *BranchWorker) refuseUnsafeWorktree(ctx context.Context, worktree *gogit if err != nil { return err } - batch := newWriteBatch(ctx, w.contentWriter, w.mapper, scan) + // The acceptance gate never places a resource, so no placement policy is needed here. + batch := newWriteBatch(ctx, w.contentWriter, w.mapper, scan, nil) return batch.refusal() } @@ -321,6 +326,7 @@ func (w *BranchWorker) applyResyncToWorktree( base string, desired []manifestanalyzer.DesiredResource, scopeGVR *schema.GroupVersionResource, + policy *manifestanalyzer.PlacementPolicy, ) (ResyncStats, bool, error) { root := worktree.Filesystem.Root() scan, err := scanWorktreeSubtree(filepath.Join(root, base)) @@ -328,7 +334,7 @@ func (w *BranchWorker) applyResyncToWorktree( return ResyncStats{}, false, err } - batch := newWriteBatch(ctx, w.contentWriter, w.mapper, scan) + batch := newWriteBatch(ctx, w.contentWriter, w.mapper, scan, policy) // First materialization is the adoption gate: refuse a subtree that holds content the // operator cannot safely manage (unsupported kustomization, duplicate identity, impure // or non-KRM files, foreign content, a catastrophic .gittargetignore) and commit nothing, @@ -381,6 +387,12 @@ func (wb *writeBatch) applyResyncPlan( stats.Created++ case upsertUpdated: stats.Updated++ + case upsertSkippedUnsafe: + // A fail-safe placement refusal (see createNew/writeWholeFile). Count it + // so a resource the resync did not mirror shows up in the summary instead + // of vanishing between Created and Skipped; the per-resource reason is + // already logged at the skip site. + stats.PlacementSkipped++ case upsertNoChange: } } diff --git a/internal/git/resync_flush_test.go b/internal/git/resync_flush_test.go index 39c14061..2ef0da78 100644 --- a/internal/git/resync_flush_test.go +++ b/internal/git/resync_flush_test.go @@ -65,7 +65,7 @@ func applyResyncViaWorktree( ) (ResyncStats, bool) { t.Helper() w := &BranchWorker{contentWriter: writer, mapper: mapper} - stats, changed, err := w.applyResyncToWorktree(context.Background(), worktree, "", desired, nil) + stats, changed, err := w.applyResyncToWorktree(context.Background(), worktree, "", desired, nil, nil) require.NoError(t, err) return stats, changed } @@ -222,7 +222,7 @@ func TestResync_ScopedSweepDropsOnlyTargetType(t *testing.T) { w := &BranchWorker{contentWriter: writer, mapper: twoTypeMapper()} scope := &schema.GroupVersionResource{Group: "", Version: "v1", Resource: "configmaps"} - stats, changed, err := w.applyResyncToWorktree(context.Background(), worktree, "", nil, scope) + stats, changed, err := w.applyResyncToWorktree(context.Background(), worktree, "", nil, scope, nil) require.NoError(t, err) require.True(t, changed, "the removed type's document is swept") assert.Equal(t, 1, stats.Deleted, "exactly the configmap is swept, not the secret") @@ -274,9 +274,52 @@ func TestResync_FoldsCreateUpdateDropTogether(t *testing.T) { _, dropErr := os.Stat(dropFull) assert.True(t, os.IsNotExist(dropErr)) - freshCanonical := filepath.Join(root, writer.filePathForIdentifier(desiredCM("fresh", "red").Resource)) - _, freshErr := os.Stat(freshCanonical) - assert.NoError(t, freshErr, "the created resource lands at its canonical path") + // F4: with existing ConfigMap siblings ("keep", "drop") each in their own file + // under apps/, a genuinely new ConfigMap follows that established layout + // (Option C sibling inference) rather than the canonical GVR-tree path. + freshInferred := filepath.Join(root, "apps", "fresh.yaml") + _, freshErr := os.Stat(freshInferred) + assert.NoError(t, freshErr, "the created resource lands beside its siblings under apps/") +} + +// A fail-safe placement refusal during resync is counted in PlacementSkipped, not +// silently swallowed between Created and the planner's Skipped view. Here a bundling +// default routes both a Secret and a ConfigMap in the same namespace to one cold +// "all.yaml"; the writer refuses to co-mingle encrypted and plaintext documents, so +// one of the two is skipped fail-safe — and that skip must show up in the stats. +func TestResync_UnsafePlacementCountsAsPlacementSkipped(t *testing.T) { + enc := &stubEncryptor{result: []byte( + "apiVersion: v1\nkind: Secret\nmetadata:\n name: cred\n namespace: app\n" + + "data:\n k: ENC[AES256,data:x,iv:y,tag:z]\nsops:\n version: 3.9.0\n")} + writer := newContentWriter(types.SensitiveResourcePolicy{}) + writer.setEncryptor(enc, "test-scope") + worktree := newWorktreeForTest(t) + + policy := &manifestanalyzer.PlacementPolicy{Default: "all.yaml"} + desired := []manifestanalyzer.DesiredResource{ + { + Resource: types.NewResourceIdentifier("", "v1", "secrets", "app", "cred"), + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "v1", "kind": "Secret", + "metadata": map[string]interface{}{"name": "cred", "namespace": "app"}, + }}, + }, + { + Resource: types.NewResourceIdentifier("", "v1", "configmaps", "app", "cache"), + Object: &unstructured.Unstructured{Object: map[string]interface{}{ + "apiVersion": "v1", "kind": "ConfigMap", + "metadata": map[string]interface{}{"name": "cache", "namespace": "app"}, + }}, + }, + } + + w := &BranchWorker{contentWriter: writer, mapper: twoTypeMapper()} + stats, _, err := w.applyResyncToWorktree(context.Background(), worktree, "", desired, nil, policy) + require.NoError(t, err) + + assert.Equal(t, 1, stats.PlacementSkipped, + "the resource refused fail-safe to avoid co-mingling must be counted, not vanish") + assert.Equal(t, 1, stats.Created, "the other resource is still written") } // A sensitive (SOPS) resource that the resync re-encrypts is counted as Updated, not @@ -322,6 +365,7 @@ func TestResync_SensitiveUpdateCountsAsUpdatedNotSkipped(t *testing.T) { "", []manifestanalyzer.DesiredResource{desired}, nil, + nil, ) require.NoError(t, err) require.True(t, changed, "the secret is re-encrypted") diff --git a/internal/git/secret_write_test.go b/internal/git/secret_write_test.go index 5f84b151..8f0ae05c 100644 --- a/internal/git/secret_write_test.go +++ b/internal/git/secret_write_test.go @@ -184,7 +184,7 @@ func TestBranchWorker_SecretWritesSOPSPath(t *testing.T) { require.NoError(t, err) require.NoError(t, worker.commitPendingWrites([]PendingWrite{*pendingWrite}, false)) - sopsPath := filepath.Join(worker.repoPathForRemote(remoteURL), "v1", "secrets", "default", "test-secret.sops.yaml") + sopsPath := filepath.Join(worker.repoPathForRemote(remoteURL), "default", "secrets", "test-secret.sops.yaml") assert.FileExists(t, sopsPath) } diff --git a/internal/git/types.go b/internal/git/types.go index 22285bbd..ba6c1c03 100644 --- a/internal/git/types.go +++ b/internal/git/types.go @@ -154,6 +154,10 @@ type ResolvedTargetMetadata struct { Path string BootstrapOptions pathBootstrapOptions EncryptionConfig *ResolvedEncryptionConfig + // Placement is the GitTarget's declared new-file placement policy (F4), resolved + // from spec.placement. Nil when the GitTarget declares none, in which case new + // resources are placed by sibling inference and then the canonical path. + Placement *manifestanalyzer.PlacementPolicy } // PendingWrite is the unit retained until a push succeeds. @@ -262,12 +266,18 @@ type ResyncResult struct { // ResyncStats summarises what a resync changed, for GitTarget status. Created, // Updated, and Deleted are the materialised create / patch+replace / managed-drop // counts; Skipped is documents present but not safely editable (e.g. encrypted or -// disallowed constructs). +// disallowed constructs). PlacementSkipped is new resources the writer refused to +// place fail-safe — placement could not be resolved safely, or the write would +// co-mingle sensitive and plaintext documents (F4 Option B2). It is counted (not +// silently swallowed) and logged per-resource so a not-mirrored resource is visible +// in the resync summary; it is not (yet) surfaced as a dedicated GitTarget status +// condition. type ResyncStats struct { - Created int - Updated int - Deleted int - Skipped int + Created int + Updated int + Deleted int + Skipped int + PlacementSkipped int } // reply delivers a result on the request's buffered channel without blocking, so a diff --git a/internal/manifestanalyzer/placement.go b/internal/manifestanalyzer/placement.go new file mode 100644 index 00000000..98d4c9c6 --- /dev/null +++ b/internal/manifestanalyzer/placement.go @@ -0,0 +1,789 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "errors" + "fmt" + "path" + "regexp" + "sort" + "strings" + + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// PlacementPolicy is a resolved GitTarget placement declaration (Option B2 of +// docs/design/manifest/version2/gittarget-new-file-placement-rules.md): a single +// exact-type map plus a fallback default template, consulted for every resource +// regardless of sensitivity. It mirrors api/v1alpha3.GitTargetPlacementSpec +// field-for-field but is defined locally so this analyzer package stays free of any +// Kubernetes API type dependency; the git package converts the CRD spec into this +// shape. +// +// There is no sensitive/normal split here: sensitivity is a write-safety property +// (encrypt the content, keep the path identity-complete, never append or +// co-mingle) enforced after resolution — in finishPlacement (sensitive never +// appends), in the writer (encrypt by classification), and in cohortMembers +// (inference never crosses the encrypted boundary) — not a second map to configure. +// +// A nil *PlacementPolicy, or one with no matching ByType entry and no Default, +// falls through to sibling inference (Option C) and then the canonical fallback. +type PlacementPolicy struct { + ByType map[string]string + Default string +} + +// PlacementRequest describes a resource with no existing document in Git — the +// only case placement runs for (an existing document is always updated in place at +// its current location; see docs/design/manifest/version2/ +// gittarget-new-file-placement-rules.md, "Existing manifests are still match-first"). +type PlacementRequest struct { + Identifier types.ResourceIdentifier + Kind string + Sensitive bool +} + +// PlacementSource names which mechanism produced a PlacementResult's Path, for +// logging and the scan/dry-run "why here" trace (P8 in the design doc). +type PlacementSource string + +const ( + // PlacementSourceDeclared is Option B: an explicit placement.byType/default + // template matched. + PlacementSourceDeclared PlacementSource = "declared" + // PlacementSourceInferred is Option C: no declared template matched, but an + // existing sibling cohort determined the destination. + PlacementSourceInferred PlacementSource = "inferred" + // PlacementSourceCanonical is the built-in, versionless + // {namespaceOrCluster}/{group}/{resource}/{name}.yaml fallback: no declared + // template and no sibling to follow (e.g. an empty repository, or the + // type/namespace is new). + PlacementSourceCanonical PlacementSource = "canonical" +) + +// PlacementResult is where a new resource should be written. +type PlacementResult struct { + // Path is the resolved file path (slash-separated), relative to the scanned + // root (the GitTarget's spec.path). + Path string + // Append is true when Path already exists as a managed file the new document + // should be appended to as an additional document; false for a brand-new file. + Append bool + // Source names which mechanism produced Path. + Source PlacementSource + // Cohort describes the sibling cohort and ladder step that produced Path; + // empty unless Source is PlacementSourceInferred. + Cohort string + // Kustomization is set when Path's directory carries a supported + // kustomization.yaml whose resources: list does not already name Path — the + // writer must add it as part of the same commit so kustomize picks the file + // up (F4's "add to the right kustomize file"). + Kustomization *KustomizationInfo + // NamespaceInherited is true when Path's destination infers its namespace + // from build context (a kustomization.yaml's namespace: transformer) rather + // than from metadata.namespace in the file — mirroring + // DocumentModel.NamespaceInheritedFromContext for a document that does not + // exist yet. The writer must keep metadata.namespace out of the written + // bytes, exactly as it already does for an in-place edit of an existing + // document in the same context (see design doc: "the new file inherits its + // sibling's NamespaceSource"). + NamespaceInherited bool +} + +// LocateNew resolves the placement of a resource with no existing document, per +// docs/design/manifest/version2/gittarget-new-file-placement-rules.md: a declared +// template (Option B) wins when present; otherwise an existing sibling cohort +// decides (Option C, steps 1/2 — same type+namespace, then same type+any +// namespace); otherwise the canonical path. +// +// store MUST be the pre-plan snapshot for the whole batch and must never be mutated +// mid-batch, so a batch of several new creates resolves order-independently +// regardless of event order — a new resource never becomes another new resource's +// sibling within the same commit (P2 of the design doc). +// +// Step 3 (same namespace, any type) is deliberately not implemented: the design +// doc's own P5 discussion flags it as the highest-risk rung (an unbounded +// namespace-wide bundle that swallows every new type sharing a namespace), and +// steps 1/2/4 already cover the launch use cases (per-type bundles, per-type files, +// canonical). A namespace-bundle layout remains reachable via Option B. +// +// An error is returned only when the resolved placement cannot be honoured safely +// — currently, a sensitive resource whose resolved path already exists (sensitive +// documents are never appended; see "Sensitive placement and uniqueness" in the +// design doc). The caller must skip creating that resource and surface the error as +// a diagnostic rather than writing into a shared or multi-document sensitive file. +func LocateNew(store *ManifestStore, policy *PlacementPolicy, req PlacementRequest) (PlacementResult, error) { + vars := placementVars(req) + + if path, ok, err := resolveDeclared(policy, req, vars); err == nil && ok { + return finishPlacement(store, req, path, PlacementSourceDeclared, "", false) + } + + if path, cohort, nsInherited, ok := resolveInferred(store, req); ok { + return finishPlacement(store, req, path, PlacementSourceInferred, cohort, nsInherited) + } + + if path, ok, nsInherited := resolveKustomizeRoot(store, req); ok { + return finishPlacement( + store, req, path, PlacementSourceInferred, "the GitTarget's one kustomization root", nsInherited, + ) + } + + return finishPlacement(store, req, canonicalPath(req), PlacementSourceCanonical, "", false) +} + +// resolveKustomizeRoot is a narrow, F4-specific fallback for when no sibling cohort +// exists (steps 1/2 both miss) — typically a resource whose type has never before +// appeared in this GitTarget. The canonical path (step 4) is a +// {group}/{version}/{resource}/{namespace}/{name}.yaml tree a kustomization's +// resources: graph can never reach, so a brand-new type in an otherwise +// kustomize-managed folder would silently land outside the folder's own +// convention — precisely the problem F4 exists to fix. When the whole scanned +// subtree is governed by exactly one supported kustomization (today's +// single-context baseline), the new resource belongs beside that kustomization's +// other files instead. +// +// This is intentionally narrower than the design doc's shelved step 3 (same +// namespace, any type): it never appends into an existing bundle file, and it only +// ever fires when there is exactly one supported kustomization for the whole +// GitTarget to be about — the destination follows from there being one root, not +// from picking the largest matching cohort — so it cannot become the "sink that +// swallows every new type" risk (P5) the doc's own step 3 raised. More than one +// supported kustomization under the scanned root is ambiguous and declines rather +// than guessing. +func resolveKustomizeRoot(store *ManifestStore, req PlacementRequest) (string, bool, bool) { + var only *KustomizationInfo + for _, k := range store.Kustomizations { + if k.Unsupported { + continue + } + if only != nil { + return "", false, false + } + only = k + } + if only == nil { + return "", false, false + } + name := req.Identifier.Name + ".yaml" + if req.Sensitive { + name = req.Identifier.Name + ".sops.yaml" + } + return cleanJoin(slashDir(only.Path), name), true, only.Namespace != "" +} + +// 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. +func finishPlacement( + store *ManifestStore, + req PlacementRequest, + resolvedPath string, + source PlacementSource, + cohort string, + namespaceInherited bool, +) (PlacementResult, error) { + // This is the one gate every resolution path — declared, inferred, the + // kustomize-root fallback, and canonical alike — funnels through before a + // byte is ever written, so a rendered path can never escape the GitTarget's + // spec.path regardless of which mechanism produced it. See "Path validation" + // in the design doc: non-empty, a clean relative path, no "..", and a YAML + // suffix. + if err := ValidateResolvedPlacementPath(resolvedPath); err != nil { + return PlacementResult{}, fmt.Errorf( + "placement for resource %s resolved to an invalid path: %w", req.Identifier.String(), err, + ) + } + res := PlacementResult{Path: resolvedPath, Source: source, Cohort: cohort, NamespaceInherited: namespaceInherited} + // A resolved path that already holds a file is only a safe append target when + // every document already in it is cleanly editable. A file that tolerates a + // non-editable construct (an anchor, alias, or other disallowed pattern) may + // hold a document that looks like a match but does not actually claim its + // identity — appending after it is not the data-loss risk that overwriting it + // would be, but treating it as an ordinary bundle is still wrong: the writer + // cannot vouch for what is already in that file. Append stays false, so the + // caller falls back to writeWholeFile, whose own multi-document guard is the + // established, tested safety net for exactly this collision. + fm, exists := store.FilesByPath[resolvedPath] + if exists && fileIsAppendSafe(fm) { + res.Append = true + } + if req.Sensitive && res.Append { + return PlacementResult{}, fmt.Errorf( + "placement for sensitive resource %s resolved to %q, which already holds a document; "+ + "sensitive resources are never appended to an existing file", + req.Identifier.String(), resolvedPath, + ) + } + // A plaintext resource must never join a file that already holds an encrypted + // document: appending would sit its cleartext beside SOPS-encrypted data (a + // partially-encrypted file), and falling through to writeWholeFile would + // instead overwrite — destroy — the encrypted document. Under Option B2 the one + // declared map is consulted for sensitive and normal resources alike, so this + // runtime guard (not a separate sensitive placement block) is what keeps the two + // classes from co-mingling for every sensitive type, core or operator-configured. + if res.Append && !req.Sensitive && fileHoldsEncryptedDocument(fm) { + return PlacementResult{}, fmt.Errorf( + "placement for resource %s resolved to %q, which already holds an encrypted document; "+ + "a plaintext resource is never appended to an encrypted file", + req.Identifier.String(), resolvedPath, + ) + } + if k := store.Kustomizations[slashDir(resolvedPath)]; k != nil && !k.Unsupported && + !kustomizationListsResource(k, resolvedPath) { + res.Kustomization = k + } + return res, nil +} + +func kustomizationListsResource(k *KustomizationInfo, resolvedPath string) bool { + dir := slashDir(k.Path) + for _, entry := range k.Resources { + if cleanJoin(dir, entry) == resolvedPath { + return true + } + } + return false +} + +// ValidateResolvedPlacementPath enforces the design doc's "Path validation" +// contract against a fully-resolved (variable-substituted) placement path, +// regardless of which mechanism produced it: non-empty, a clean relative path +// staying under the GitTarget's spec.path (no "..", not absolute, no redundant +// segments), no Windows-style backslash separators, a non-empty final file name, +// and a recognized YAML suffix (".sops.yaml"/".sops.yml" satisfy this too, since +// they end in ".yaml"/".yml"). finishPlacement runs this on every path before a +// single byte is written, so a bad declared template (F4 Option B) can never +// escape the folder the writer owns — sanitizePlacementSegment already defends +// each individual variable's value, but the template's own literal text is +// author-supplied and unconstrained without this gate. +func ValidateResolvedPlacementPath(p string) error { + if p == "" { + return errors.New("path is empty") + } + if strings.ContainsRune(p, '\\') { + return fmt.Errorf("path %q must use \"/\" separators, not \"\\\"", p) + } + if path.IsAbs(p) { + return fmt.Errorf("path %q must be relative, not absolute", p) + } + cleaned := path.Clean(p) + if cleaned != p || cleaned == "." || cleaned == ".." || strings.HasPrefix(cleaned, "../") { + return fmt.Errorf("path %q must be a clean relative path that stays under the GitTarget's spec.path", p) + } + base := path.Base(cleaned) + if base == "" || base == "." || base == "/" { + return fmt.Errorf("path %q has no file name", p) + } + if !strings.HasSuffix(cleaned, ".yaml") && !strings.HasSuffix(cleaned, ".yml") { + return fmt.Errorf("path %q must end in .yaml or .yml", p) + } + return nil +} + +// canonicalPath mirrors internal/git's generateFilePath (ResourceIdentifier.ToGitPath +// plus the .sops.yaml suffix for a sensitive resource). It is re-implemented here, +// not imported, because internal/git already imports manifestanalyzer and importing +// the other way would cycle; the duplicated logic is six lines and covered by tests +// on both sides. +func canonicalPath(req PlacementRequest) string { + base := req.Identifier.ToGitPath() + if !req.Sensitive { + return base + } + if strings.HasSuffix(base, ".yaml") { + return strings.TrimSuffix(base, ".yaml") + ".sops.yaml" + } + return base + ".sops.yaml" +} + +// --- Option B: declared type-map placement ------------------------------------- + +func resolveDeclared(policy *PlacementPolicy, req PlacementRequest, vars map[string]string) (string, bool, error) { + if policy == nil { + return "", false, nil + } + key := PlacementTypeKey(req.Identifier.Group, req.Identifier.Version, req.Identifier.Resource) + var tmpl string + switch { + case strings.TrimSpace(policy.ByType[key]) != "": + tmpl = policy.ByType[key] + case strings.TrimSpace(policy.Default) != "": + tmpl = policy.Default + default: + return "", false, nil + } + rendered, err := RenderPlacementTemplate(tmpl, vars) + if err != nil { + return "", false, err + } + return rendered, true, nil +} + +// PlacementTypeKey renders the exact-type key used by GitTargetPlacementSpec.ByType: +// "{group}/{version}/{resource}", with the group segment omitted for core resources +// ("v1/secrets", "apps/v1/deployments", "cert-manager.io/v1/certificates"). +func PlacementTypeKey(group, version, resource string) string { + if group == "" { + return fmt.Sprintf("%s/%s", version, resource) + } + return fmt.Sprintf("%s/%s/%s", group, version, resource) +} + +var placementVariablePattern = regexp.MustCompile(`\{[a-zA-Z]+\}`) + +// isKnownPlacementVariable reports whether name is one of the variables +// RenderPlacementTemplate accepts. Keep in sync with placementVars and +// placementVariableNames. +func isKnownPlacementVariable(name string) bool { + switch name { + case "group", "groupPath", "version", "apiVersion", "resource", + "kind", "scope", "namespace", "namespaceOrCluster", "name", "sensitiveSuffix": + return true + default: + return false + } +} + +// placementVariableNames lists every variable isKnownPlacementVariable accepts, +// for callers (ValidPlacementTemplateSyntax) that need the full set rather than a +// single-name membership check. +func placementVariableNames() []string { + return []string{ + "group", "groupPath", "version", "apiVersion", "resource", + "kind", "scope", "namespace", "namespaceOrCluster", "name", "sensitiveSuffix", + } +} + +func placementVars(req PlacementRequest) map[string]string { + id := req.Identifier + scope := "namespaced" + nsOrCluster := id.Namespace + if id.IsClusterScoped() { + scope = "cluster" + // The namespace-position segment uses "_cluster", an illegal Kubernetes + // namespace name (DNS-1123 forbids "_"), so it can never collide with a real + // namespace — unlike a bare "cluster", which is a legal namespace name. This + // mirrors ResourceIdentifier.ToGitPath's built-in canonical scope segment. + // {scope} above stays the readable "cluster"/"namespaced" descriptor. + nsOrCluster = "_cluster" + } + apiVersion := id.Version + if id.Group != "" { + apiVersion = id.Group + "/" + id.Version + } + sensitiveSuffix := ".yaml" + if req.Sensitive { + sensitiveSuffix = ".sops.yaml" + } + return map[string]string{ + "group": id.Group, + "groupPath": id.Group, + "version": id.Version, + "apiVersion": apiVersion, + "resource": id.Resource, + "kind": req.Kind, + "scope": scope, + "namespace": id.Namespace, + "namespaceOrCluster": nsOrCluster, + "name": id.Name, + "sensitiveSuffix": sensitiveSuffix, + } +} + +// RenderPlacementTemplate expands a brace-variable path template ("{namespace}/ +// secret-{name}.sops.yaml") against vars, then collapses empty path segments left +// behind by an omitted variable (e.g. "{groupPath}" for a core resource) so +// "{groupPath}/{version}/..." renders "v1/..." rather than "/v1/...". It returns an +// error naming any "{...}"-shaped placeholder that is not a known variable, so a +// typo in a declared template is caught rather than silently left as literal text. +func RenderPlacementTemplate(tmpl string, vars map[string]string) (string, error) { + var unknown []string + rendered := placementVariablePattern.ReplaceAllStringFunc(tmpl, func(match string) string { + name := strings.Trim(match, "{}") + if !isKnownPlacementVariable(name) { + unknown = append(unknown, match) + return match + } + return sanitizePlacementSegment(vars[name]) + }) + if len(unknown) > 0 { + return "", fmt.Errorf( + "placement template %q references unknown variable(s): %s", + tmpl, + strings.Join(unknown, ", "), + ) + } + return collapseEmptyPathSegments(rendered), nil +} + +// sanitizePlacementSegment defends the identity-completeness guarantee: a +// Kubernetes name/namespace can never legally contain "/", but a template +// variable's value is substituted verbatim, so any stray path separator is +// percent-encoded rather than allowed to silently fold two distinct resources onto +// the same rendered path. +func sanitizePlacementSegment(v string) string { + if !strings.ContainsAny(v, "/\\%") { + return v + } + v = strings.ReplaceAll(v, "%", "%25") + v = strings.ReplaceAll(v, "/", "%2F") + v = strings.ReplaceAll(v, "\\", "%5C") + return v +} + +func collapseEmptyPathSegments(p string) string { + parts := strings.Split(p, "/") + kept := make([]string, 0, len(parts)) + for _, part := range parts { + if part != "" { + kept = append(kept, part) + } + } + return strings.Join(kept, "/") +} + +// ValidPlacementTemplateSyntax reports whether tmpl references only known +// placement variables, independent of any resource identity — the check a +// GitTarget's Validated gate runs statically at reconcile time, before any +// repository scan. +func ValidPlacementTemplateSyntax(tmpl string) error { + names := placementVariableNames() + stub := make(map[string]string, len(names)) + for _, name := range names { + stub[name] = "" + } + _, err := RenderPlacementTemplate(tmpl, stub) + return err +} + +// ValidPlacementTemplatePath statically rejects a declared template whose own +// literal text (never mind any variable substitution, which sanitizePlacementSegment +// already defends per-value) could render outside the GitTarget's spec.path or +// with the wrong kind of file name: an explicit ".." path segment, a leading "/" +// (absolute), a "\" separator, or a suffix that isn't ".yaml"/".yml" (a template +// ending in the literal "{sensitiveSuffix}" placeholder is accepted without +// rendering it, since that variable only ever expands to ".yaml" or ".sops.yaml"). +// This runs at the GitTarget's Validated gate — before any repository scan, and +// before any resource can ever trigger a write — so a bad template fails fast and +// visibly instead of silently skipping (or, without ValidateResolvedPlacementPath's +// runtime backstop, escaping) resource by resource. +func ValidPlacementTemplatePath(tmpl string) error { + trimmed := strings.TrimSpace(tmpl) + if trimmed == "" { + return errors.New("placement template is empty") + } + if strings.ContainsRune(trimmed, '\\') { + return fmt.Errorf("placement template %q must use \"/\" separators, not \"\\\"", tmpl) + } + if strings.HasPrefix(trimmed, "/") { + return fmt.Errorf("placement template %q must be relative, not absolute", tmpl) + } + for _, segment := range strings.Split(trimmed, "/") { + if segment == ".." { + return fmt.Errorf("placement template %q must not contain a \"..\" path segment", tmpl) + } + } + if !strings.HasSuffix(trimmed, "{sensitiveSuffix}") && + !strings.HasSuffix(trimmed, ".yaml") && !strings.HasSuffix(trimmed, ".yml") { + return fmt.Errorf("placement template %q must end in .yaml, .yml, or {sensitiveSuffix}", tmpl) + } + return nil +} + +// IdentityCompletePlacementTemplate reports whether tmpl is guaranteed to render a +// distinct path for every distinct resource identity — the structural guarantee +// "Sensitive placement and uniqueness" in the design doc requires of every accepted +// sensitive template. narrowedToOneType is true for a ByType entry (the map key +// itself already names one exact type); a Default template must additionally carry +// the type variables since it applies across every type the class does not name +// explicitly. +func IdentityCompletePlacementTemplate(tmpl string, narrowedToOneType bool) bool { + hasName := strings.Contains(tmpl, "{name}") + hasScope := strings.Contains(tmpl, "{namespace}") || strings.Contains(tmpl, "{namespaceOrCluster}") + if !hasName || !hasScope { + return false + } + if narrowedToOneType { + return true + } + return strings.Contains(tmpl, "{groupPath}") && + strings.Contains(tmpl, "{version}") && + strings.Contains(tmpl, "{resource}") +} + +// --- Option C: sibling inference ------------------------------------------------- + +// resolveInferred implements Option C steps 1 and 2. See LocateNew's doc comment for +// why step 3 is not implemented. +func resolveInferred(store *ManifestStore, req PlacementRequest) (string, string, bool, bool) { + id := req.Identifier + + if members := cohortMembers( + store, + id.Group, + id.Version, + id.Resource, + id.Namespace, + true, + req.Sensitive, + ); len( + members, + ) > 0 { + if path, cohort, nsInherited, ok := cohortDestination( + store, + members, + req, + "same type and namespace", + false, + ); ok { + return path, cohort, nsInherited, true + } + } + // Step 2 matches across namespaces, so — unlike step 1, where every candidate + // already shares the new resource's own namespace — a candidate here must prove + // it is namespace-agnostic before it can be trusted for a namespace it has never + // seen (P4 of the design doc): a per-namespace-segmented layout (a dedicated + // bundle or directory per namespace) must NOT be extended by guessing one of the + // existing namespaces' files/directories for a brand-new namespace. cohortDestination + // disqualifies any candidate that has not demonstrated it already spans more than + // one namespace (a bundle) or lives in a single shared directory regardless of + // namespace (singleton style); an unseen namespace then correctly falls through to + // the canonical path, which builds the right namespace segment directly. + if members := cohortMembers(store, id.Group, id.Version, id.Resource, "", false, req.Sensitive); len(members) > 0 { + if path, cohort, nsInherited, ok := cohortDestination( + store, + members, + req, + "same type, any namespace", + true, + ); ok { + return path, cohort, nsInherited, true + } + } + return "", "", false, false +} + +// cohortMembers collects every existing document of the given type (optionally +// pinned to namespace) whose sensitivity matches the resource being placed. A +// document's sensitivity is read off the analyzer's own encrypted-document +// classification (CauseEncrypted) rather than a separately threaded policy, so a +// sensitive resource can never infer from a plaintext sibling or vice versa (the +// design doc's "sensitive stays hard-split — with no config"). +func cohortMembers( + store *ManifestStore, + group, version, resource, namespace string, + matchNamespace, sensitive bool, +) []*DocumentModel { + var out []*DocumentModel + for rid, dm := range store.ByResourceIdentity { + if rid.Group != group || rid.Version != version || rid.Resource != resource { + continue + } + if matchNamespace && rid.Namespace != namespace { + continue + } + if isSensitiveDocument(dm) != sensitive { + continue + } + out = append(out, dm) + } + return out +} + +func isSensitiveDocument(dm *DocumentModel) bool { + return dm.Cause.Kind == CauseEncrypted +} + +// fileIsAppendSafe reports whether every document already in fm is cleanly +// editable or an ordinary encrypted document — never a document tolerated despite +// an unsupported construct (CauseNonEditable: an anchor, alias, or other disallowed +// pattern), which does not claim its identity and so cannot be vouched for. Such a +// file is excluded from both bundle and singleton-style candidacy (cohortDestination) +// and from the append decision (finishPlacement): a genuinely new resource must +// never be joined to a file the writer cannot fully account for. +func fileIsAppendSafe(fm *FileModel) bool { + if fm == nil { + return false + } + for _, d := range fm.Documents { + if d.Cause.Kind == CauseNonEditable { + return false + } + } + return true +} + +// fileHoldsEncryptedDocument reports whether fm already contains at least one +// encrypted (sensitive) document. finishPlacement uses it to refuse appending a +// plaintext resource into an encrypted file — the write-time half of the +// "sensitivity is a write-safety classifier, not a placement namespace" contract +// (Option B2 of the design doc). +func fileHoldsEncryptedDocument(fm *FileModel) bool { + if fm == nil { + return false + } + for _, d := range fm.Documents { + if d.Cause.Kind == CauseEncrypted { + return true + } + } + return false +} + +// cohortDestination decides, for one matched cohort, whether the repository's +// established pattern is "one resource per file" or "resources of this cohort share +// a file" (a bundle), and resolves the concrete destination: +// +// - every file holding >1 document is a candidate bundle, keyed by path, weighted +// by how many cohort members it holds; +// - every file holding exactly one document is "singleton style," aggregated into +// one virtual candidate regardless of how many separate files/directories it +// spans, weighted by its total member count; +// - the candidate with the most members wins; ties (including "no bundle beats +// the singleton style") favour singleton style, the more conservative choice, +// since it never grows an existing bundle the repository's siblings do not +// clearly favour. Among multiple bundle files tied for the lead, the +// lexicographically smallest file path wins; the singleton style's directory is +// the lexicographically smallest directory among its members. +// +// This is deterministic and independent of map/walk iteration order (P1 of the +// design doc): the result depends only on the (path -> member count) shape of the +// pre-plan snapshot, never on the order LocateNew is called for other resources in +// the same batch. +// +// namespaceAgnostic is true only for step 2 (any namespace). It disqualifies a +// candidate that has not demonstrated it is independent of namespace: a bundle file +// must already hold members from more than one distinct namespace, and singleton +// style must have every member in exactly one directory (P4 — see resolveInferred). +// A step-1 candidate (namespaceAgnostic false) is never disqualified this way, +// because every member there already shares the new resource's own namespace. +func cohortDestination( + store *ManifestStore, + members []*DocumentModel, + req PlacementRequest, + step string, + namespaceAgnostic bool, +) (string, string, bool, bool) { + docLoc := store.DocumentLocations() + perFile := map[string][]*DocumentModel{} + for _, m := range members { + if p := docLoc[m].FilePath; p != "" { + perFile[p] = append(perFile[p], m) + } + } + if len(perFile) == 0 { + return "", "", false, false + } + + singletonDirs, bestPath, bestCount, dirReps, bundleReps := classifyCohortLocations( + store, + perFile, + namespaceAgnostic, + ) + if bestPath == "" && len(singletonDirs) == 0 { + return "", "", false, false + } + + cohort := fmt.Sprintf("%d sibling(s) via %s", len(members), step) + if bestCount > len(singletonDirs) { + nsInherited := bundleReps[bestPath] != nil && bundleReps[bestPath].NamespaceInheritedFromContext() + return bestPath, cohort, nsInherited, true + } + + sort.Strings(singletonDirs) + winDir := singletonDirs[0] + name := req.Identifier.Name + ".yaml" + if req.Sensitive { + name = req.Identifier.Name + ".sops.yaml" + } + nsInherited := dirReps[winDir] != nil && dirReps[winDir].NamespaceInheritedFromContext() + return cleanJoin(winDir, name), cohort, nsInherited, true +} + +// classifyCohortLocations partitions a cohort's members by where they live: +// every file holding more than one document is a bundle candidate (keyed by path, +// weighted by member count); every file holding exactly one document contributes to +// the singleton-style candidate. A tainted file (fileIsAppendSafe false) is +// excluded from both. namespaceAgnostic applies the P4 safety rule (see +// resolveInferred): a bundle must already span more than one namespace, and +// singleton style must resolve to a single shared directory, or the candidate is +// dropped. It returns the eligible singleton directories, the winning bundle +// (path/count), and one representative document per singleton directory / bundle +// path — used to decide whether the destination's namespace is inherited from +// build context (see PlacementResult.NamespaceInherited) — determined +// independently of map iteration order by scanning candidate paths in sorted order. +func classifyCohortLocations( + store *ManifestStore, + perFile map[string][]*DocumentModel, + namespaceAgnostic bool, +) ([]string, string, int, map[string]*DocumentModel, map[string]*DocumentModel) { + var singletonDirs []string + dirReps := map[string]*DocumentModel{} + bundleReps := map[string]*DocumentModel{} + bundleCounts := map[string]int{} + for p, ms := range perFile { + fm := store.FilesByPath[p] + if !fileIsAppendSafe(fm) { + continue // a tainted file is never a placement destination + } + if len(fm.Documents) > 1 { + if namespaceAgnostic && !spansMultipleNamespaces(ms) { + continue // unproven: looks like a per-namespace-segmented bundle (P4) + } + bundleCounts[p] = len(ms) + bundleReps[p] = ms[0] + continue + } + dir := slashDir(p) + singletonDirs = append(singletonDirs, dir) + if _, seen := dirReps[dir]; !seen { + dirReps[dir] = ms[0] + } + } + if namespaceAgnostic && !allSameDir(singletonDirs) { + singletonDirs = nil // unproven: directories look namespace-segmented (P4) + } + + bundlePaths := make([]string, 0, len(bundleCounts)) + for p := range bundleCounts { + bundlePaths = append(bundlePaths, p) + } + sort.Strings(bundlePaths) + bestPath, bestCount := "", 0 + for _, p := range bundlePaths { + if bundleCounts[p] > bestCount { + bestCount, bestPath = bundleCounts[p], p + } + } + return singletonDirs, bestPath, bestCount, dirReps, bundleReps +} + +// spansMultipleNamespaces reports whether ms (all documents sharing one file) +// carry more than one distinct namespace, proving the file is namespace-agnostic +// rather than one namespace's dedicated bundle. +func spansMultipleNamespaces(ms []*DocumentModel) bool { + seen := map[string]struct{}{} + for _, m := range ms { + if m.ResourceIdentity == nil { + continue + } + seen[m.ResourceIdentity.Namespace] = struct{}{} + if len(seen) > 1 { + return true + } + } + return false +} + +// allSameDir reports whether every directory in dirs is identical (trivially true +// for zero or one element). +func allSameDir(dirs []string) bool { + for i := 1; i < len(dirs); i++ { + if dirs[i] != dirs[0] { + return false + } + } + return true +} diff --git a/internal/manifestanalyzer/placement_test.go b/internal/manifestanalyzer/placement_test.go new file mode 100644 index 00000000..ce41a68d --- /dev/null +++ b/internal/manifestanalyzer/placement_test.go @@ -0,0 +1,827 @@ +// SPDX-License-Identifier: Apache-2.0 + +package manifestanalyzer + +import ( + "context" + "fmt" + "strings" + "testing" + "testing/fstest" + + "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/ConfigButler/gitops-reverser/internal/types" + "github.com/ConfigButler/gitops-reverser/internal/typeset" +) + +// placementSnapshot is like sampleClusterSnapshot, but additionally allows core +// Secrets, so a sensitive resource's ByResourceIdentity actually resolves and can +// be exercised by the placement tests below. +func placementSnapshot() typeset.Snapshot { + return typeset.Snapshot{ + Generation: 1, + Entries: []typeset.Entry{ + { + GVK: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, + GVR: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, + Namespaced: true, + Allowed: true, + }, + { + GVK: schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, + GVR: schema.GroupVersionResource{Version: "v1", Resource: "configmaps"}, + Namespaced: true, + Allowed: true, + }, + { + GVK: schema.GroupVersionKind{Version: "v1", Kind: "Secret"}, + GVR: schema.GroupVersionResource{Version: "v1", Resource: "secrets"}, + Namespaced: true, + Allowed: true, + }, + }, + } +} + +func placementStore(t *testing.T, fsys fstest.MapFS) *ManifestStore { + t.Helper() + mapper := typeset.NewSnapshotRegistry(placementSnapshot()) + return BuildStore(context.Background(), fsys, mapper) +} + +func configMapYAML(name, namespace string) string { + return fmt.Sprintf("apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: %s\n namespace: %s\n", name, namespace) +} + +func secretYAML(name, namespace string) string { + return fmt.Sprintf( + "apiVersion: v1\nkind: Secret\nmetadata:\n name: %s\n namespace: %s\nsops:\n version: \"3\"\n", + name, namespace, + ) +} + +func newConfigMapRequest(name, namespace string) PlacementRequest { + return PlacementRequest{ + Identifier: types.NewResourceIdentifier("", "v1", "configmaps", namespace, name), + Kind: "ConfigMap", + } +} + +func newSecretRequest(name string) PlacementRequest { + return PlacementRequest{ + Identifier: types.NewResourceIdentifier("", "v1", "secrets", "app", name), + Kind: "Secret", + Sensitive: true, + } +} + +func TestLocateNew_EmptyRepo_Canonical(t *testing.T) { + store := placementStore(t, fstest.MapFS{}) + req := newConfigMapRequest("cache", "app") + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + want := req.Identifier.ToGitPath() + if res.Path != want || res.Source != PlacementSourceCanonical || res.Append { + t.Fatalf("got %+v, want canonical path %q, no append", res, want) + } +} + +func TestLocateNew_BundleCohort_Appends(t *testing.T) { + fsys := fstest.MapFS{ + "all.yaml": {Data: []byte(configMapYAML("a", "app") + "---\n" + configMapYAML("b", "app"))}, + } + store := placementStore(t, fsys) + req := newConfigMapRequest("cache", "app") + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + if res.Path != "all.yaml" || !res.Append || res.Source != PlacementSourceInferred { + t.Fatalf("got %+v, want append to all.yaml via inference", res) + } +} + +func TestLocateNew_SingletonCohort_NewFileBesideSiblings(t *testing.T) { + fsys := fstest.MapFS{ + "overlays/test/configmap-a.yaml": {Data: []byte(configMapYAML("a", "app"))}, + } + store := placementStore(t, fsys) + req := newConfigMapRequest("cache", "app") + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + want := "overlays/test/cache.yaml" + if res.Path != want || res.Append || res.Source != PlacementSourceInferred { + t.Fatalf("got %+v, want a new file %q beside the sibling", res, want) + } +} + +// A sibling whose namespace is inherited from a kustomization's namespace: +// transformer (no metadata.namespace in its own bytes) means a new document +// placed beside it must also omit metadata.namespace — otherwise the write +// would silently break the convention every document in that context follows +// (this is what let an incidental resource sharing the namespace, e.g. a +// cluster-injected ConfigMap, write a namespace: line into a hand-curated +// bundle file in production; see the design doc's Option C test plan, "the new +// file inherits its sibling's NamespaceSource"). +func TestLocateNew_SiblingNamespaceInheritedFromKustomize_NewFileOmitsNamespace(t *testing.T) { + fsys := fstest.MapFS{ + "overlays/test/kustomization.yaml": { + Data: []byte("namespace: app\nresources:\n - configmap-a.yaml\n"), + }, + "overlays/test/configmap-a.yaml": { + Data: []byte("apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: a\n"), + }, + } + store := placementStore(t, fsys) + req := newConfigMapRequest("cache", "app") + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + if !res.NamespaceInherited { + t.Fatalf("got %+v, want NamespaceInherited since the sibling omits metadata.namespace", res) + } +} + +// A sibling with an explicit metadata.namespace (no kustomize context) means a +// new document beside it keeps writing its namespace explicitly too. +func TestLocateNew_SiblingNamespaceExplicit_NewFileKeepsNamespace(t *testing.T) { + fsys := fstest.MapFS{ + "overlays/test/configmap-a.yaml": {Data: []byte(configMapYAML("a", "app"))}, + } + store := placementStore(t, fsys) + req := newConfigMapRequest("cache", "app") + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + if res.NamespaceInherited { + t.Fatalf("got %+v, want NamespaceInherited false: the sibling writes its namespace explicitly", res) + } +} + +// resolveKustomizeRoot's fallback (no sibling of this type yet) must also flag +// NamespaceInherited when the one kustomization declares a namespace: +// transformer, for the same reason. +func TestLocateNew_KustomizeRootWithNamespaceTransformer_NewFileOmitsNamespace(t *testing.T) { + fsys := fstest.MapFS{ + "overlays/test/kustomization.yaml": { + Data: []byte("namespace: podinfo-test\nresources:\n - deployment.yaml\n"), + }, + "overlays/test/deployment.yaml": {Data: []byte( + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: web\n namespace: podinfo-test\n", + )}, + } + store := placementStore(t, fsys) + req := PlacementRequest{ + Identifier: types.NewResourceIdentifier("", "v1", "configmaps", "podinfo-test", "debug-toolbox"), + Kind: "ConfigMap", + } + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + if !res.NamespaceInherited { + t.Fatalf("got %+v, want NamespaceInherited since the kustomization sets namespace:", res) + } +} + +func TestLocateNew_Sensitive_NeverJoinsPlaintextBundle(t *testing.T) { + fsys := fstest.MapFS{ + "all.yaml": {Data: []byte(configMapYAML("a", "app") + "---\n" + configMapYAML("b", "app"))}, + } + store := placementStore(t, fsys) + req := newSecretRequest("api-token") + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + want := "app/secrets/api-token.sops.yaml" + if res.Path != want || res.Append || res.Source != PlacementSourceCanonical { + t.Fatalf("got %+v, want the secure canonical SOPS fallback %q", res, want) + } +} + +func TestLocateNew_Sensitive_JoinsSensitiveSiblingDirectory(t *testing.T) { + fsys := fstest.MapFS{ + "secrets/app/db.sops.yaml": {Data: []byte(secretYAML("db", "app"))}, + } + store := placementStore(t, fsys) + req := newSecretRequest("api-token") + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + want := "secrets/app/api-token.sops.yaml" + if res.Path != want || res.Append || res.Source != PlacementSourceInferred { + t.Fatalf("got %+v, want a new single-doc SOPS file beside the sensitive sibling %q", res, want) + } +} + +func TestLocateNew_TieBreak_SingletonWinsWhenAheadOrTied(t *testing.T) { + cases := []struct { + name string + singletons int + bundleSize int + wantBundle bool + }{ + {"singleton strictly ahead", 3, 2, false}, + {"tie favours singleton", 2, 2, false}, + {"bundle strictly ahead", 2, 3, true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + fsys := fstest.MapFS{} + for i := range tc.singletons { + fsys[fmt.Sprintf("solo-%d.yaml", i)] = &fstest.MapFile{ + Data: []byte(configMapYAML(fmt.Sprintf("solo-%d", i), "app")), + } + } + var bundle strings.Builder + for i := range tc.bundleSize { + if i > 0 { + bundle.WriteString("---\n") + } + bundle.WriteString(configMapYAML(fmt.Sprintf("bundled-%d", i), "app")) + } + if tc.bundleSize > 0 { + fsys["bundle.yaml"] = &fstest.MapFile{Data: []byte(bundle.String())} + } + + store := placementStore(t, fsys) + res, err := LocateNew(store, nil, newConfigMapRequest("new", "app")) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + if got := res.Path == "bundle.yaml"; got != tc.wantBundle { + t.Fatalf("path = %q (append=%v), wantBundle=%v", res.Path, res.Append, tc.wantBundle) + } + }) + } +} + +func TestLocateNew_DeclaredOutranksInferred(t *testing.T) { + fsys := fstest.MapFS{ + "all.yaml": {Data: []byte(configMapYAML("a", "app"))}, + } + store := placementStore(t, fsys) + policy := &PlacementPolicy{ + ByType: map[string]string{"v1/configmaps": "{namespace}/configmaps.yaml"}, + } + + res, err := LocateNew(store, policy, newConfigMapRequest("cache", "app")) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + want := "app/configmaps.yaml" + if res.Path != want || res.Source != PlacementSourceDeclared { + t.Fatalf("got %+v, want the declared template %q to win over inference", res, want) + } +} + +func TestLocateNew_Step2_NewNamespaceUnderPerNamespaceBundle_FallsToCanonical(t *testing.T) { + fsys := fstest.MapFS{ + "ns1/configmaps.yaml": { + Data: []byte( + configMapYAML("a", "ns1") + "---\n" + configMapYAML("b", "ns1") + "---\n" + configMapYAML("c", "ns1"), + ), + }, + } + store := placementStore(t, fsys) + req := newConfigMapRequest("cache", "ns2") + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + // P4: a per-namespace-segmented bundle must not be guessed for an unseen + // namespace; the new namespace's ConfigMap must fall through to canonical, + // never land in ns1/configmaps.yaml. + if res.Path != req.Identifier.ToGitPath() || res.Source != PlacementSourceCanonical { + t.Fatalf("got %+v, want canonical fallback (never ns1's bundle)", res) + } +} + +func TestLocateNew_Step2_NamespaceAgnosticBundle_IsReused(t *testing.T) { + fsys := fstest.MapFS{ + "all.yaml": {Data: []byte(configMapYAML("a", "ns1") + "---\n" + configMapYAML("b", "ns2"))}, + } + store := placementStore(t, fsys) + req := newConfigMapRequest("cache", "ns3") + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + if res.Path != "all.yaml" || !res.Append || res.Source != PlacementSourceInferred { + t.Fatalf("got %+v, want the namespace-agnostic bundle reused for the new namespace", res) + } +} + +func TestLocateNew_Step2_NewNamespaceUnderPerNamespaceDirectories_FallsToCanonical(t *testing.T) { + // Two distinct singleton directories, one per namespace, is what proves a + // per-namespace-segmented convention (P4) — a single existing directory would + // be indistinguishable from coincidence, so this needs at least two. + fsys := fstest.MapFS{ + "ns1/configmap-a.yaml": {Data: []byte(configMapYAML("a", "ns1"))}, + "ns2/configmap-b.yaml": {Data: []byte(configMapYAML("b", "ns2"))}, + } + store := placementStore(t, fsys) + req := newConfigMapRequest("cache", "ns3") + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + if res.Path != req.Identifier.ToGitPath() || res.Source != PlacementSourceCanonical { + t.Fatalf("got %+v, want canonical fallback, never ns1/ or ns2/ for a resource in ns3", res) + } +} + +func TestLocateNew_SensitiveCollision_Errors(t *testing.T) { + // The existing file already occupies exactly the path the declared template + // will render for the new resource (a misconfigured template lacking {name} + // would produce this in practice); LocateNew must refuse to append a sensitive + // document onto it rather than silently colliding two identities. + fsys := fstest.MapFS{ + "secrets/app/api-token-2.sops.yaml": {Data: []byte(secretYAML("other", "app"))}, + } + store := placementStore(t, fsys) + policy := &PlacementPolicy{ + ByType: map[string]string{"v1/secrets": "secrets/{namespace}/{name}.sops.yaml"}, + } + + _, err := LocateNew(store, policy, newSecretRequest("api-token-2")) + if err == nil { + t.Fatalf("expected an error placing a second identity onto the same sensitive path") + } +} + +// Under Option B2 the single declared map is consulted for sensitive and normal +// resources alike, so a plaintext resource can be routed onto a path that already +// holds an encrypted document. finishPlacement must refuse that rather than +// append the cleartext beside SOPS data (or fall through to a whole-file +// overwrite that would destroy the encrypted document) — the write-time guard +// that replaces B1's structural sensitive/normal split. +func TestLocateNew_PlaintextOntoEncryptedFile_Refused(t *testing.T) { + // The analyzer classifies a document as encrypted only for a ".sops.yaml"/ + // ".sops.yml" file carrying a sops: key, so the fixture must use that name. + fsys := fstest.MapFS{ + "bundle.sops.yaml": {Data: []byte(secretYAML("db", "app"))}, + } + store := placementStore(t, fsys) + policy := &PlacementPolicy{Default: "bundle.sops.yaml"} + + _, err := LocateNew(store, policy, newConfigMapRequest("cache", "app")) + if err == nil { + t.Fatalf("expected a refusal placing a plaintext resource onto an encrypted file") + } + if !strings.Contains(err.Error(), "encrypted") { + t.Fatalf("error should name the encrypted-file conflict, got: %v", err) + } +} + +func TestLocateNew_KustomizationEntryDetected(t *testing.T) { + kustYAML := "namespace: podinfo-test\nresources:\n - deployment.yaml\n" + fsys := fstest.MapFS{ + "overlays/test/kustomization.yaml": {Data: []byte(kustYAML)}, + "overlays/test/deployment.yaml": {Data: []byte( + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: web\n namespace: podinfo-test\n", + )}, + } + store := placementStore(t, fsys) + req := PlacementRequest{ + Identifier: types.NewResourceIdentifier("", "v1", "configmaps", "podinfo-test", "debug-toolbox"), + Kind: "ConfigMap", + } + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + if res.Kustomization == nil { + t.Fatalf("got %+v, want a Kustomization entry to add since the overlay carries one", res) + } + if res.Kustomization.Path != "overlays/test/kustomization.yaml" { + t.Errorf("Kustomization.Path = %q, want overlays/test/kustomization.yaml", res.Kustomization.Path) + } +} + +func TestLocateNew_KustomizationAlreadyListed_NoEntryNeeded(t *testing.T) { + kustYAML := "namespace: podinfo-test\nresources:\n - deployment.yaml\n - debug-toolbox.yaml\n" + fsys := fstest.MapFS{ + "overlays/test/kustomization.yaml": {Data: []byte(kustYAML)}, + "overlays/test/deployment.yaml": {Data: []byte( + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: web\n namespace: podinfo-test\n", + )}, + } + store := placementStore(t, fsys) + policy := &PlacementPolicy{ + Default: "overlays/test/debug-toolbox.yaml", + } + req := PlacementRequest{ + Identifier: types.NewResourceIdentifier("", "v1", "configmaps", "podinfo-test", "debug-toolbox"), + Kind: "ConfigMap", + } + + res, err := LocateNew(store, policy, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + if res.Kustomization != nil { + t.Fatalf("got %+v, want no Kustomization entry since debug-toolbox.yaml is already listed", res) + } +} + +func TestLocateNew_KustomizationUnsupported_NeverEdited(t *testing.T) { + kustYAML := "namespace: podinfo-test\nresources:\n - deployment.yaml\nhelmCharts:\n - name: x\n" + fsys := fstest.MapFS{ + "overlays/test/kustomization.yaml": {Data: []byte(kustYAML)}, + } + store := placementStore(t, fsys) + req := PlacementRequest{ + Identifier: types.NewResourceIdentifier("", "v1", "configmaps", "podinfo-test", "debug-toolbox"), + Kind: "ConfigMap", + } + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + if res.Kustomization != nil { + t.Fatalf("got %+v, want an unsupported kustomization never surfaced for editing", res) + } +} + +func TestLocateNew_BatchOrderIndependence(t *testing.T) { + fsys := fstest.MapFS{ + "solo.yaml": {Data: []byte(configMapYAML("solo", "app"))}, + } + // The store snapshot is built once (the batch's pre-plan snapshot) and never + // mutated; resolving two hypothetical new siblings against it must not depend on + // which is resolved first (P2) — neither becomes the other's sibling. + store := placementStore(t, fsys) + + first, err := LocateNew(store, nil, newConfigMapRequest("alpha", "app")) + if err != nil { + t.Fatalf("LocateNew(alpha): %v", err) + } + second, err := LocateNew(store, nil, newConfigMapRequest("beta", "app")) + if err != nil { + t.Fatalf("LocateNew(beta): %v", err) + } + + storeAgain := placementStore(t, fsys) + secondFirst, err := LocateNew(storeAgain, nil, newConfigMapRequest("beta", "app")) + if err != nil { + t.Fatalf("LocateNew(beta) reordered: %v", err) + } + firstSecond, err := LocateNew(storeAgain, nil, newConfigMapRequest("alpha", "app")) + if err != nil { + t.Fatalf("LocateNew(alpha) reordered: %v", err) + } + + if first.Path != firstSecond.Path || second.Path != secondFirst.Path { + t.Fatalf("resolution order changed the result: %q/%q vs %q/%q", + first.Path, second.Path, firstSecond.Path, secondFirst.Path) + } + if first.Path == second.Path { + t.Fatalf("alpha and beta must not collide onto the same new file: %q", first.Path) + } +} + +func TestRenderPlacementTemplate(t *testing.T) { + vars := map[string]string{ + "group": "", "groupPath": "", "version": "v1", "apiVersion": "v1", + "resource": "configmaps", "kind": "ConfigMap", "scope": "namespaced", + "namespace": "default", "namespaceOrCluster": "default", "name": "app", + "sensitiveSuffix": ".yaml", + } + got, err := RenderPlacementTemplate("{groupPath}/{version}/{resource}/{namespace}/{name}.yaml", vars) + if err != nil { + t.Fatalf("RenderPlacementTemplate: %v", err) + } + if want := "v1/configmaps/default/app.yaml"; got != want { + t.Errorf("got %q, want %q (empty groupPath segment collapsed)", got, want) + } +} + +func TestPlacementVars_GroupedClusterScoped(t *testing.T) { + req := PlacementRequest{ + Identifier: types.NewResourceIdentifier("rbac.authorization.k8s.io", "v1", "clusterroles", "", "admin"), + Kind: "ClusterRole", + } + vars := placementVars(req) + if vars["scope"] != "cluster" || vars["namespaceOrCluster"] != "_cluster" { + t.Errorf("got scope=%q namespaceOrCluster=%q, want scope=\"cluster\" (descriptor) and "+ + "namespaceOrCluster=\"_cluster\" (illegal-namespace sentinel) for a cluster-scoped resource", + vars["scope"], vars["namespaceOrCluster"]) + } + if want := "rbac.authorization.k8s.io/v1"; vars["apiVersion"] != want { + t.Errorf("apiVersion = %q, want %q for a grouped resource", vars["apiVersion"], want) + } +} + +// A sensitive and a normal document of the SAME type (e.g. one ConfigMap +// encrypted as .sops.yaml, one plain) must not be conflated: cohortMembers must +// skip the mismatched-sensitivity sibling rather than only relying on the type +// filter (which cannot tell them apart, since sensitivity is an encryption fact, +// not a type fact). +func TestLocateNew_MixedSensitivityConfigMapsInSameNamespace_NeverConflated(t *testing.T) { + fsys := fstest.MapFS{ + "normal.yaml": {Data: []byte(configMapYAML("a", "app") + "---\n" + configMapYAML("b", "app"))}, + "secret.sops.yaml": { + Data: []byte( + "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: sensitive-cm\n namespace: app\nsops:\n version: \"3\"\n", + ), + }, + } + store := placementStore(t, fsys) + + res, err := LocateNew(store, nil, newConfigMapRequest("cache", "app")) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + want := "normal.yaml" + if res.Path != want || !res.Append { + t.Fatalf("got %+v, want the new normal ConfigMap appended to its normal bundle %q, "+ + "never to the encrypted sibling", res, want) + } +} + +// A file tolerated despite a non-editable construct (e.g. a YAML anchor) must +// never be joined — classifyCohortLocations excludes it from both bundle and +// singleton candidacy, so a genuinely new sibling falls through past it instead +// of silently landing beside content the writer cannot vouch for. +func TestLocateNew_TaintedSiblingNeverJoined(t *testing.T) { + tainted := "apiVersion: v1\nkind: ConfigMap\nmetadata:\n name: anchored\n namespace: app\n" + + "data: &d\n color: blue\nextra:\n <<: *d\n" + fsys := fstest.MapFS{ + "tainted.yaml": {Data: []byte(tainted)}, + } + store := placementStore(t, fsys) + + res, err := LocateNew(store, nil, newConfigMapRequest("cache", "app")) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + if res.Path == "tainted.yaml" || res.Source != PlacementSourceCanonical { + t.Fatalf("got %+v, want the tainted file excluded and canonical fallback used", res) + } +} + +func TestRenderPlacementTemplate_UnknownVariable(t *testing.T) { + _, err := RenderPlacementTemplate("{namespace}/{bogus}.yaml", map[string]string{"namespace": "default"}) + if err == nil { + t.Fatalf("expected an error for an unknown template variable") + } +} + +func TestRenderPlacementTemplate_SanitizesSlashInValue(t *testing.T) { + got, err := RenderPlacementTemplate("{namespace}/{name}.yaml", map[string]string{ + "namespace": "default", "name": "weird/name", + }) + if err != nil { + t.Fatalf("RenderPlacementTemplate: %v", err) + } + if want := "default/weird%2Fname.yaml"; got != want { + t.Errorf("got %q, want %q (slash percent-encoded, not a path separator)", got, want) + } +} + +func TestIdentityCompletePlacementTemplate(t *testing.T) { + cases := []struct { + name string + tmpl string + narrowedToOneType bool + want bool + }{ + {"full identity", "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.yaml", false, true}, + {"missing resource for default", "{groupPath}/{version}/{namespaceOrCluster}/{name}.yaml", false, false}, + {"narrowed type needs only scope+name", "{namespace}/secret-{name}.sops.yaml", true, true}, + {"narrowed type missing name", "{namespace}/secret.sops.yaml", true, false}, + {"narrowed type missing scope", "secret-{name}.sops.yaml", true, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := IdentityCompletePlacementTemplate(tc.tmpl, tc.narrowedToOneType); got != tc.want { + t.Errorf("IdentityCompletePlacementTemplate(%q, %v) = %v, want %v", + tc.tmpl, tc.narrowedToOneType, got, tc.want) + } + }) + } +} + +func TestPlacementTypeKey(t *testing.T) { + if got := PlacementTypeKey("", "v1", "secrets"); got != "v1/secrets" { + t.Errorf("core key = %q, want v1/secrets", got) + } + if got := PlacementTypeKey("apps", "v1", "deployments"); got != "apps/v1/deployments" { + t.Errorf("grouped key = %q, want apps/v1/deployments", got) + } +} + +func TestValidPlacementTemplateSyntax(t *testing.T) { + if err := ValidPlacementTemplateSyntax("{namespace}/{name}.yaml"); err != nil { + t.Errorf("a template built only from known variables must be valid: %v", err) + } + if err := ValidPlacementTemplateSyntax("{namespace}/{bogus}.yaml"); err == nil { + t.Errorf("expected an error for the unknown variable {bogus}") + } +} + +// Two supported kustomizations under the scanned root is ambiguous: neither can +// safely be assumed to be "the one" the GitTarget is about, so a genuinely new +// type falls through to canonical rather than guessing. +func TestLocateNew_KustomizeRoot_AmbiguousWithTwoSupported(t *testing.T) { + fsys := fstest.MapFS{ + "overlays/a/kustomization.yaml": {Data: []byte("namespace: a\nresources:\n - deployment.yaml\n")}, + "overlays/a/deployment.yaml": {Data: []byte( + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: web\n namespace: a\n", + )}, + "overlays/b/kustomization.yaml": {Data: []byte("namespace: b\nresources:\n - deployment.yaml\n")}, + "overlays/b/deployment.yaml": {Data: []byte( + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: web\n namespace: b\n", + )}, + } + store := placementStore(t, fsys) + req := newConfigMapRequest("cache", "a") + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + if res.Path != req.Identifier.ToGitPath() || res.Source != PlacementSourceCanonical { + t.Fatalf("got %+v, want canonical fallback: two supported kustomizations is ambiguous", res) + } +} + +// The kustomize-root fallback must also work for a sensitive resource: no +// existing sibling of that type, exactly one supported kustomization, no +// namespace: transformer set (so the sensitive path keeps its explicit namespace). +func TestLocateNew_KustomizeRootSensitive(t *testing.T) { + fsys := fstest.MapFS{ + "overlays/test/kustomization.yaml": {Data: []byte("resources:\n - deployment.yaml\n")}, + "overlays/test/deployment.yaml": {Data: []byte( + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: web\n namespace: app\n", + )}, + } + store := placementStore(t, fsys) + req := newSecretRequest("api-token") + + res, err := LocateNew(store, nil, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + want := "overlays/test/api-token.sops.yaml" + if res.Path != want || res.NamespaceInherited { + t.Fatalf("got %+v, want %q with no namespace transformer set", res, want) + } +} + +// A declared template with an unknown variable is a misconfiguration LocateNew +// must not crash or write on; it falls through to sibling inference / canonical, +// exactly as if no declared template had matched. +func TestLocateNew_DeclaredTemplateUnknownVariable_FallsThrough(t *testing.T) { + store := placementStore(t, fstest.MapFS{}) + policy := &PlacementPolicy{ + Default: "{bogus}/all.yaml", + } + req := newConfigMapRequest("cache", "app") + + res, err := LocateNew(store, policy, req) + if err != nil { + t.Fatalf("LocateNew: %v", err) + } + if res.Path != req.Identifier.ToGitPath() || res.Source != PlacementSourceCanonical { + t.Fatalf("got %+v, want canonical fallback when the declared template is invalid", res) + } +} + +func TestFileIsAppendSafe(t *testing.T) { + if fileIsAppendSafe(nil) { + t.Error("a nil FileModel must never be append-safe") + } + clean := &FileModel{Documents: []*DocumentModel{{Cause: DocumentCause{Kind: CauseNone}}}} + if !fileIsAppendSafe(clean) { + t.Error("a file with only cleanly editable documents must be append-safe") + } + sensitive := &FileModel{Documents: []*DocumentModel{{Cause: DocumentCause{Kind: CauseEncrypted}}}} + if !fileIsAppendSafe(sensitive) { + t.Error("an ordinary encrypted document must not be treated as tainted") + } + tainted := &FileModel{Documents: []*DocumentModel{ + {Cause: DocumentCause{Kind: CauseNone}}, + {Cause: DocumentCause{Kind: CauseNonEditable}}, + }} + if fileIsAppendSafe(tainted) { + t.Error("a file holding a non-editable (e.g. anchor-using) document must never be append-safe") + } +} + +func TestSpansMultipleNamespaces(t *testing.T) { + if spansMultipleNamespaces(nil) { + t.Error("no members cannot span multiple namespaces") + } + unresolved := []*DocumentModel{{ResourceIdentity: nil}} + if spansMultipleNamespaces(unresolved) { + t.Error("a document with no resolved ResourceIdentity contributes no namespace") + } + oneNamespace := []*DocumentModel{ + {ResourceIdentity: &types.ResourceIdentifier{Namespace: "a"}}, + {ResourceIdentity: &types.ResourceIdentifier{Namespace: "a"}}, + } + if spansMultipleNamespaces(oneNamespace) { + t.Error("members sharing one namespace do not span multiple namespaces") + } + twoNamespaces := []*DocumentModel{ + {ResourceIdentity: &types.ResourceIdentifier{Namespace: "a"}}, + {ResourceIdentity: &types.ResourceIdentifier{Namespace: "b"}}, + } + if !spansMultipleNamespaces(twoNamespaces) { + t.Error("members in two distinct namespaces must span multiple namespaces") + } +} + +func TestValidateResolvedPlacementPath(t *testing.T) { + cases := []struct { + name string + path string + ok bool + }{ + {"clean relative yaml", "overlays/test/cache.yaml", true}, + {"clean relative yml", "overlays/test/cache.yml", true}, + {"sops path is a yaml path too", "secrets/app/db.sops.yaml", true}, + {"empty", "", false}, + {"parent traversal", "../outside.yaml", false}, + {"nested parent traversal", "overlays/../../outside.yaml", false}, + {"absolute", "/etc/passwd", false}, + {"backslash separator", "overlays\\test\\cache.yaml", false}, + {"not clean (double slash)", "overlays//cache.yaml", false}, + {"no file name", "overlays/test/", false}, + {"bad suffix", "overlays/test/cache.txt", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidateResolvedPlacementPath(tc.path) + if (err == nil) != tc.ok { + t.Errorf("ValidateResolvedPlacementPath(%q) = %v, want ok=%v", tc.path, err, tc.ok) + } + }) + } +} + +func TestValidPlacementTemplatePath(t *testing.T) { + cases := []struct { + name string + tmpl string + ok bool + }{ + {"clean relative", "{namespace}/{name}.yaml", true}, + {"sensitiveSuffix placeholder", "{namespace}/secret-{name}{sensitiveSuffix}", true}, + {"parent traversal", "../outside.yaml", false}, + {"nested parent traversal", "{namespace}/../../outside.yaml", false}, + {"absolute", "/etc/{name}.yaml", false}, + {"backslash", "{namespace}\\{name}.yaml", false}, + {"bad suffix", "{namespace}/{name}.txt", false}, + {"empty", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := ValidPlacementTemplatePath(tc.tmpl) + if (err == nil) != tc.ok { + t.Errorf("ValidPlacementTemplatePath(%q) = %v, want ok=%v", tc.tmpl, err, tc.ok) + } + }) + } +} + +// Defense in depth: even if a path-escaping template somehow reached LocateNew +// (e.g. validation were bypassed, stale, or a future bug), the runtime gate in +// finishPlacement must still refuse to write outside the GitTarget's spec.path, +// exactly like the existing sensitive-collision refusal — skip the resource, not +// escape the folder. +func TestLocateNew_DeclaredTemplateEscapingPath_Refused(t *testing.T) { + store := placementStore(t, fstest.MapFS{}) + policy := &PlacementPolicy{ + Default: "../../outside.yaml", + } + + _, err := LocateNew(store, policy, newConfigMapRequest("cache", "app")) + if err == nil { + t.Fatal("expected an error for a declared template that escapes spec.path") + } +} diff --git a/internal/manifestanalyzer/plan_test.go b/internal/manifestanalyzer/plan_test.go index 461cb634..8e334519 100644 --- a/internal/manifestanalyzer/plan_test.go +++ b/internal/manifestanalyzer/plan_test.go @@ -160,8 +160,8 @@ func TestBuildPlan_Create(t *testing.T) { if create.Desired == nil { t.Errorf("create action should carry the desired object") } - if got := create.Resource.ToGitPath(); got != "v1/configmaps/default/c.yaml" { - t.Errorf("create placement = %q, want v1/configmaps/default/c.yaml", got) + if got := create.Resource.ToGitPath(); got != "default/configmaps/c.yaml" { + t.Errorf("create placement = %q, want default/configmaps/c.yaml", got) } } diff --git a/internal/manifestanalyzer/render_test.go b/internal/manifestanalyzer/render_test.go index b19d16a4..7b8f1ce0 100644 --- a/internal/manifestanalyzer/render_test.go +++ b/internal/manifestanalyzer/render_test.go @@ -121,7 +121,7 @@ func TestRenderScanText(t *testing.T) { "kustomization.yaml", "Plan: 2 action(s)", "create", - "v1/configmaps/default/new.yaml", + "default/configmaps/new.yaml", "patch", "deploy.yaml#0", } { diff --git a/internal/manifestanalyzer/store.go b/internal/manifestanalyzer/store.go index 4638e93f..cbeb1a2f 100644 --- a/internal/manifestanalyzer/store.go +++ b/internal/manifestanalyzer/store.go @@ -70,6 +70,14 @@ type ManifestStore struct { // unless the store was built with a non-empty allowlist. Retained []RetainedDocument + // Kustomizations indexes every kustomization.yaml found under the scanned root by + // its directory (slash, relative to the root; "." for the root itself). New-file + // placement (F4) consults it to decide whether a candidate directory is + // kustomize-governed and, if so, which file's resources: list a new sibling must + // be added to. Populated independent of the allowlist — build-directive discovery + // does not depend on which files the writer materialises. + Kustomizations map[string]*KustomizationInfo + // Foreign lists the filesystem entries under spec.path that matched no recognized role // (non-YAML files, symlinks, submodules) and survived the .gittargetignore filter. The // acceptance gate refuses each one (foreignContentRefusals); the path is an @@ -367,9 +375,10 @@ func buildStore( // The foreign-content view and the active .gittargetignore travel with the store so // the acceptance gate (foreignContentRefusals + IgnoreIssues) and the writer's // write-plan precondition both read them from one place. - Foreign: scan.Foreign, - Ignore: scan.Ignore, - IgnoreIssues: scan.IgnoreIssues, + Foreign: scan.Foreign, + Ignore: scan.Ignore, + IgnoreIssues: scan.IgnoreIssues, + Kustomizations: kustomizationInfos(kusts), } // inv.Records are exactly the KRM documents (editable or not), in stable scan @@ -591,6 +600,49 @@ type namespaceAssignment struct { sourceByNamespace map[string]string // namespace -> kustomization file path that assigned it } +// KustomizationInfo is the write-relevant view of one kustomization.yaml exposed for +// new-file placement (F4): whether the directory it lives in carries a supported +// kustomization and, if so, its local resources/bases entries (raw, relative to its +// own directory) — the list a new sibling file must be added to so kustomize +// includes it. +type KustomizationInfo struct { + // Path is the kustomization.yaml's own file path (slash), relative to the + // scanned root. + Path string + + // Resources holds the resources + bases entries exactly as written (local file + // names, child-directory bases, or remote URLs), in file order. It is the raw + // text, not resolved paths — cleanJoin resolves an entry against Path's directory. + Resources []string + + // Unsupported is true when the kustomization uses a feature outside the + // supported subset (hasUnsupportedKustomizeFeature) or a remote base, or is + // unparseable. The writer must never edit an unsupported kustomization. + Unsupported bool + + // Namespace is the kustomization's namespace: transformer value, empty when + // it sets none. A new document placed directly in this kustomization's + // directory (resolveKustomizeRoot) omits metadata.namespace when this is + // set, exactly as an existing document in this context already does. + Namespace string +} + +// kustomizationInfos exports the analyzer's internal kustomization index for +// new-file placement (F4). Copying into a fresh map/slice keeps ManifestStore +// immune to any future mutation of the internal kustomizationDoc values. +func kustomizationInfos(kusts map[string]*kustomizationDoc) map[string]*KustomizationInfo { + out := make(map[string]*KustomizationInfo, len(kusts)) + for dir, doc := range kusts { + out[dir] = &KustomizationInfo{ + Path: doc.path, + Resources: append([]string(nil), doc.resources...), + Unsupported: doc.unsupported, + Namespace: doc.namespace, + } + } + return out +} + // kustomizationDoc is the parsed, write-relevant view of one kustomization.yaml: its // namespace transformer, its resources/bases graph entries, and whether it uses any // feature outside the supported contextual-namespace subset (which disqualifies it as diff --git a/internal/types/identifier.go b/internal/types/identifier.go index 2bfa3f48..7c1eef27 100644 --- a/internal/types/identifier.go +++ b/internal/types/identifier.go @@ -7,9 +7,10 @@ import ( "fmt" ) -// ResourceIdentifier encapsulates all information needed to uniquely identify -// a Kubernetes resource and generate its Git storage path following the -// Kubernetes REST API structure: {group}/{version}/{resource}/{namespace}/{name}. +// ResourceIdentifier encapsulates all information needed to uniquely identify a +// Kubernetes resource. Its Key() is the fully-qualified REST-style identity +// ({group}/{version}/{resource}/{namespace}/{name}); its ToGitPath() is the +// versionless, namespace-first Git storage path (see that method). type ResourceIdentifier struct { Group string // e.g., "apps", "" for core resources Version string // e.g., "v1" @@ -43,24 +44,34 @@ func (r ResourceIdentifier) Key() string { return fmt.Sprintf("%s/%s/%s/%s", r.Group, r.Version, r.Resource, r.Name) } -// ToGitPath generates the Git repository file path following Kubernetes API structure. +// ToGitPath generates the canonical Git file path for a new resource: +// {namespace-or-cluster}/{group}/{resource}/{name}.yaml. The scope segment leads +// (a real namespace, or the literal "_cluster" for a cluster-scoped resource) so a +// repository reads namespace-first, the way a human browses it; the API group is +// omitted for core resources, and the API version is deliberately left out — the +// operator writes one version per object, so a version segment adds noise and would +// churn the path on a preferred-version bump. This is only the cold-start fallback: +// once any layout exists in the target, sibling inference follows it, and an +// existing document is always edited in place at its current location (match-first), +// so changing this shape never moves a file that is already in Git. See +// docs/design/manifest/version2/gittarget-new-file-placement-rules.md. func (r ResourceIdentifier) ToGitPath() string { - var basePath string - - if r.Group == "" { - // Core resources (no group) - basePath = r.Version - } else { - basePath = fmt.Sprintf("%s/%s", r.Group, r.Version) + scope := r.Namespace + if scope == "" { + // Cluster-scoped resource: the scope segment is "_cluster", an illegal + // Kubernetes namespace name (DNS-1123 forbids "_"), so it can never collide + // with a real namespace and reads unambiguously as "not a namespace" — unlike + // a bare "cluster", which is itself a legal namespace name. Matches the + // {namespaceOrCluster} placement template variable. + scope = "_cluster" } - if r.Namespace != "" { - // Namespaced resource - return fmt.Sprintf("%s/%s/%s/%s.yaml", basePath, r.Resource, r.Namespace, r.Name) + if r.Group == "" { + // Core resources (no group): omit the group segment entirely. + return fmt.Sprintf("%s/%s/%s.yaml", scope, r.Resource, r.Name) } - // Cluster-scoped resource - return fmt.Sprintf("%s/%s/%s.yaml", basePath, r.Resource, r.Name) + return fmt.Sprintf("%s/%s/%s/%s.yaml", scope, r.Group, r.Resource, r.Name) } // IsClusterScoped returns true if the resource is cluster-scoped. diff --git a/internal/types/identifier_test.go b/internal/types/identifier_test.go index ef6a5309..507f71a0 100644 --- a/internal/types/identifier_test.go +++ b/internal/types/identifier_test.go @@ -23,7 +23,7 @@ func TestResourceIdentifier_ToGitPath(t *testing.T) { Namespace: "default", Name: "nginx", }, - want: "v1/pods/default/nginx.yaml", + want: "default/pods/nginx.yaml", }, { name: "core namespaced resource - ConfigMap", @@ -34,7 +34,7 @@ func TestResourceIdentifier_ToGitPath(t *testing.T) { Namespace: "default", Name: "config", }, - want: "v1/configmaps/default/config.yaml", + want: "default/configmaps/config.yaml", }, { name: "core cluster-scoped resource - Node", @@ -45,7 +45,7 @@ func TestResourceIdentifier_ToGitPath(t *testing.T) { Namespace: "", Name: "worker-1", }, - want: "v1/nodes/worker-1.yaml", + want: "_cluster/nodes/worker-1.yaml", }, { name: "non-core namespaced resource - Deployment", @@ -56,7 +56,7 @@ func TestResourceIdentifier_ToGitPath(t *testing.T) { Namespace: "production", Name: "app", }, - want: "apps/v1/deployments/production/app.yaml", + want: "production/apps/deployments/app.yaml", }, { name: "non-core namespaced resource - StatefulSet", @@ -67,7 +67,7 @@ func TestResourceIdentifier_ToGitPath(t *testing.T) { Namespace: "database", Name: "postgres", }, - want: "apps/v1/statefulsets/database/postgres.yaml", + want: "database/apps/statefulsets/postgres.yaml", }, { name: "non-core cluster-scoped resource - ClusterRole", @@ -78,7 +78,7 @@ func TestResourceIdentifier_ToGitPath(t *testing.T) { Namespace: "", Name: "admin", }, - want: "rbac.authorization.k8s.io/v1/clusterroles/admin.yaml", + want: "_cluster/rbac.authorization.k8s.io/clusterroles/admin.yaml", }, { name: "custom CRD with group", @@ -89,7 +89,7 @@ func TestResourceIdentifier_ToGitPath(t *testing.T) { Namespace: "prod", Name: "instance", }, - want: "example.com/v1alpha1/myapps/prod/instance.yaml", + want: "prod/example.com/myapps/instance.yaml", }, { name: "custom CRD cluster-scoped", @@ -100,7 +100,7 @@ func TestResourceIdentifier_ToGitPath(t *testing.T) { Namespace: "", Name: "main", }, - want: "custom.io/v1beta1/globalconfigs/main.yaml", + want: "_cluster/custom.io/globalconfigs/main.yaml", }, { name: "resource with special characters in name", @@ -111,7 +111,7 @@ func TestResourceIdentifier_ToGitPath(t *testing.T) { Namespace: "default", Name: "my-app-v2", }, - want: "apps/v1/deployments/default/my-app-v2.yaml", + want: "default/apps/deployments/my-app-v2.yaml", }, } diff --git a/test/e2e/aggregated_apiserver_e2e_test.go b/test/e2e/aggregated_apiserver_e2e_test.go index 84387b7a..ecacac09 100644 --- a/test/e2e/aggregated_apiserver_e2e_test.go +++ b/test/e2e/aggregated_apiserver_e2e_test.go @@ -159,7 +159,7 @@ spec: flunderName := fmt.Sprintf("aggregated-commit-flunder-%d", GinkgoRandomSeed()) repoPath := path.Join( aggregatedPath, - fmt.Sprintf("wardle.example.com/v1alpha1/flunders/%s/%s.yaml", testNs, flunderName), + fmt.Sprintf("%s/wardle.example.com/flunders/%s.yaml", testNs, flunderName), ) repoFile := filepath.Join(repo.CheckoutDir, repoPath) diff --git a/test/e2e/bi_directional_e2e_test.go b/test/e2e/bi_directional_e2e_test.go index ec52274c..06c884fe 100644 --- a/test/e2e/bi_directional_e2e_test.go +++ b/test/e2e/bi_directional_e2e_test.go @@ -395,18 +395,15 @@ func (r biDirectionalRun) repoPath(parts ...string) string { func (r biDirectionalRun) liveOrderPath(name string) string { return r.repoPath( r.livePath, - iceCreamInstanceDir(crdGroupBiDirectional), - r.testNs, - name+".yaml", + iceCreamInstancePath(crdGroupBiDirectional, r.testNs, name), ) } func (r biDirectionalRun) liveSecretPath(name string) string { return r.repoPath( r.livePath, - "v1", - "secrets", r.testNs, + "secrets", name+".sops.yaml", ) } @@ -419,7 +416,7 @@ func (r biDirectionalRun) writeCRDToRepo() { Expect(err).NotTo(HaveOccurred(), "failed to render IceCreamOrder CRD fixture") Expect( os.MkdirAll( - r.repoPath(r.crdPath, "apiextensions.k8s.io", "v1", "customresourcedefinitions"), + r.repoPath(r.crdPath, "_cluster", "apiextensions.k8s.io", "customresourcedefinitions"), 0o755, ), ).To(Succeed()) @@ -427,8 +424,8 @@ func (r biDirectionalRun) writeCRDToRepo() { os.WriteFile( r.repoPath( r.crdPath, + "_cluster", "apiextensions.k8s.io", - "v1", "customresourcedefinitions", iceCreamCRDMirrorFile(crdGroupBiDirectional), ), diff --git a/test/e2e/commit_author_attribution_e2e_test.go b/test/e2e/commit_author_attribution_e2e_test.go index 7adbf0a2..ac55e471 100644 --- a/test/e2e/commit_author_attribution_e2e_test.go +++ b/test/e2e/commit_author_attribution_e2e_test.go @@ -121,7 +121,7 @@ var _ = Describe("Commit Author Attribution", Label("manager"), Ordered, func() ) Expect(err).NotTo(HaveOccurred(), "failed to create impersonated ConfigMap") - repoPath := path.Join(basePath, fmt.Sprintf("v1/configmaps/%s/%s.yaml", testNs, cmName)) + repoPath := path.Join(basePath, fmt.Sprintf("%s/configmaps/%s.yaml", testNs, cmName)) expectedFile := filepath.Join(repo.CheckoutDir, repoPath) By("waiting for the commit and asserting its author is the OIDC identity") diff --git a/test/e2e/commit_request_e2e_test.go b/test/e2e/commit_request_e2e_test.go index 51370d1a..b9e4ba12 100644 --- a/test/e2e/commit_request_e2e_test.go +++ b/test/e2e/commit_request_e2e_test.go @@ -127,7 +127,7 @@ var _ = Describe("Commit Request", Label("commit-request", "audit-consumer"), Or pullLatestRepoState(g, repo.CheckoutDir) expectedFile := filepath.Join(repo.CheckoutDir, basePath, - fmt.Sprintf("apps/v1/deployments/%s/%s.yaml", testNs, deployName)) + fmt.Sprintf("%s/apps/deployments/%s.yaml", testNs, deployName)) info, statErr := os.Stat(expectedFile) g.Expect(statErr).NotTo(HaveOccurred(), fmt.Sprintf("Deployment file should exist at %s", expectedFile)) g.Expect(info.Size()).To(BeNumerically(">", 0)) @@ -198,7 +198,7 @@ var _ = Describe("Commit Request", Label("commit-request", "audit-consumer"), Or Eventually(func(g Gomega) { pullLatestRepoState(g, repo.CheckoutDir) expected := filepath.Join(repo.CheckoutDir, basePath, - fmt.Sprintf("apps/v1/deployments/%s/%s.yaml", testNs, deployName)) + fmt.Sprintf("%s/apps/deployments/%s.yaml", testNs, deployName)) _, statErr := os.Stat(expected) g.Expect(statErr).NotTo(HaveOccurred(), fmt.Sprintf("the held Deployment should exist after finalize at %s", expected)) @@ -247,7 +247,7 @@ var _ = Describe("Commit Request", Label("commit-request", "audit-consumer"), Or pullLatestRepoState(g, repo.CheckoutDir) expectedFile := filepath.Join(repo.CheckoutDir, basePath, - fmt.Sprintf("apps/v1/deployments/%s/%s.yaml", testNs, deployName)) + fmt.Sprintf("%s/apps/deployments/%s.yaml", testNs, deployName)) info, statErr := os.Stat(expectedFile) g.Expect(statErr).NotTo(HaveOccurred(), fmt.Sprintf("Deployment file should exist at %s", expectedFile)) g.Expect(info.Size()).To(BeNumerically(">", 0)) @@ -401,7 +401,7 @@ var _ = Describe("Commit Request Bundle (UC2)", Label("commit-request", "audit-c for _, name := range deployNames { expected := filepath.Join(repo.CheckoutDir, basePath, - fmt.Sprintf("apps/v1/deployments/%s/%s.yaml", testNs, name)) + fmt.Sprintf("%s/apps/deployments/%s.yaml", testNs, name)) _, statErr := os.Stat(expected) g.Expect(statErr).NotTo(HaveOccurred(), fmt.Sprintf("every bundled Deployment must be present in the one commit: %s", expected)) diff --git a/test/e2e/commit_window_batching_e2e_test.go b/test/e2e/commit_window_batching_e2e_test.go index bbfe7ba0..489c78a4 100644 --- a/test/e2e/commit_window_batching_e2e_test.go +++ b/test/e2e/commit_window_batching_e2e_test.go @@ -104,7 +104,7 @@ var _ = Describe("Commit Window Batching", Eventually(func(g Gomega) { pullLatestRepoState(g, repo.CheckoutDir) expected := filepath.Join(repo.CheckoutDir, basePath, - "v1", "configmaps", testNs, seedCM+".yaml") + testNs, "configmaps", seedCM+".yaml") _, statErr := os.Stat(expected) g.Expect(statErr).NotTo(HaveOccurred(), fmt.Sprintf("seed ConfigMap should land before the burst: %s", expected)) @@ -131,7 +131,7 @@ var _ = Describe("Commit Window Batching", pullLatestRepoState(g, repo.CheckoutDir) for _, name := range burstNames { expected := filepath.Join(repo.CheckoutDir, basePath, - "v1", "configmaps", testNs, name+".yaml") + testNs, "configmaps", name+".yaml") _, statErr := os.Stat(expected) g.Expect(statErr).NotTo(HaveOccurred(), fmt.Sprintf("ConfigMap file should exist: %s", expected)) @@ -194,7 +194,7 @@ func assertBurstFilesAreGroupedIntoLatestCommit(checkoutDir string, burstNames [ expectedPaths := make(map[string]struct{}, len(burstNames)) for _, name := range burstNames { - p := fmt.Sprintf("%s/v1/configmaps/%s/%s.yaml", basePath, namespace, name) + p := fmt.Sprintf("%s/%s/configmaps/%s.yaml", basePath, namespace, name) expectedPaths[p] = struct{}{} } diff --git a/test/e2e/crd_lifecycle_e2e_test.go b/test/e2e/crd_lifecycle_e2e_test.go index 0372ba4e..d575f8df 100644 --- a/test/e2e/crd_lifecycle_e2e_test.go +++ b/test/e2e/crd_lifecycle_e2e_test.go @@ -137,7 +137,7 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun // CRDs are cluster-scoped, so path should NOT include namespace expectedFile := filepath.Join(crdLifecycleRepo.CheckoutDir, "e2e/crd-install-test", - "apiextensions.k8s.io/v1/customresourcedefinitions/"+iceCreamCRDMirrorFile(crdGroupCRDLifecycle)) + "_cluster/apiextensions.k8s.io/customresourcedefinitions/"+iceCreamCRDMirrorFile(crdGroupCRDLifecycle)) fileInfo, statErr := os.Stat(expectedFile) g.Expect(statErr).NotTo(HaveOccurred(), fmt.Sprintf("CRD file should exist at %s", expectedFile)) g.Expect(fileInfo.Size()).To(BeNumerically(">", 0), "CRD file should not be empty") @@ -282,7 +282,7 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun expectedFile := filepath.Join(crdLifecycleRepo.CheckoutDir, "e2e/icecream-test", - fmt.Sprintf("%s/%s/%s.yaml", iceCreamInstanceDir(crdGroupCRDLifecycle), testNs, crdInstanceName)) + iceCreamInstancePath(crdGroupCRDLifecycle, testNs, crdInstanceName)) fileInfo, statErr := os.Stat(expectedFile) g.Expect(statErr). NotTo(HaveOccurred(), fmt.Sprintf("CRD instance file should exist at %s", expectedFile)) @@ -373,7 +373,7 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun // Read the file again to ensure status is still not present expectedFile := filepath.Join(crdLifecycleRepo.CheckoutDir, "e2e/icecream-test", - fmt.Sprintf("%s/%s/%s.yaml", iceCreamInstanceDir(crdGroupCRDLifecycle), testNs, crdInstanceName)) + iceCreamInstancePath(crdGroupCRDLifecycle, testNs, crdInstanceName)) content, readErr := os.ReadFile(expectedFile) g.Expect(readErr).NotTo(HaveOccurred()) g.Expect(string(content)).NotTo(ContainSubstring("status:"), @@ -441,7 +441,7 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun expectedFile := filepath.Join(crdLifecycleRepo.CheckoutDir, "e2e/icecream-test", - fmt.Sprintf("%s/%s/%s.yaml", iceCreamInstanceDir(crdGroupCRDLifecycle), testNs, crdInstanceName)) + iceCreamInstancePath(crdGroupCRDLifecycle, testNs, crdInstanceName)) content, readErr := os.ReadFile(expectedFile) g.Expect(readErr).NotTo(HaveOccurred()) g.Expect(string(content)).To(ContainSubstring("customerName: Bob")) @@ -490,7 +490,7 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun expectedFile := filepath.Join(crdLifecycleRepo.CheckoutDir, "e2e/icecream-test", - fmt.Sprintf("%s/%s/%s.yaml", iceCreamInstanceDir(crdGroupCRDLifecycle), testNs, crdInstanceName)) + iceCreamInstancePath(crdGroupCRDLifecycle, testNs, crdInstanceName)) content, readErr := os.ReadFile(expectedFile) g.Expect(readErr).NotTo(HaveOccurred()) g.Expect(string(content)).To(ContainSubstring("container: WaffleBowl"), @@ -556,7 +556,7 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun expectedFile := filepath.Join(crdLifecycleRepo.CheckoutDir, "e2e/icecream-test", - fmt.Sprintf("%s/%s/%s.yaml", iceCreamInstanceDir(crdGroupCRDLifecycle), testNs, crdInstanceName)) + iceCreamInstancePath(crdGroupCRDLifecycle, testNs, crdInstanceName)) fileInfo, statErr := os.Stat(expectedFile) g.Expect(statErr). NotTo(HaveOccurred(), fmt.Sprintf("CRD instance file should exist at %s", expectedFile)) @@ -573,7 +573,7 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun By("verifying CRD instance file is deleted from Git repository") relPath := filepath.Join( "e2e/icecream-test", - fmt.Sprintf("%s/%s/%s.yaml", iceCreamInstanceDir(crdGroupCRDLifecycle), testNs, crdInstanceName)) + iceCreamInstancePath(crdGroupCRDLifecycle, testNs, crdInstanceName)) verifyFileDeleted := func(g Gomega) { pullLatestRepoState(g, crdLifecycleRepo.CheckoutDir) @@ -629,7 +629,7 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun expectedFile := filepath.Join(crdLifecycleRepo.CheckoutDir, "e2e/crd-delete-test", - "apiextensions.k8s.io/v1/customresourcedefinitions/"+iceCreamCRDMirrorFile(crdGroupCRDLifecycle)) + "_cluster/apiextensions.k8s.io/customresourcedefinitions/"+iceCreamCRDMirrorFile(crdGroupCRDLifecycle)) _, statErr := os.Stat(expectedFile) g.Expect(statErr).NotTo(HaveOccurred(), "CRD file should exist before deletion") } @@ -641,7 +641,7 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun By("verifying CRD file is deleted from Git repository") crdRelPath := filepath.Join( "e2e/crd-delete-test", - "apiextensions.k8s.io/v1/customresourcedefinitions/"+iceCreamCRDMirrorFile(crdGroupCRDLifecycle)) + "_cluster/apiextensions.k8s.io/customresourcedefinitions/"+iceCreamCRDMirrorFile(crdGroupCRDLifecycle)) verifyFileDeleted := func(g Gomega) { pullLatestRepoState(g, crdLifecycleRepo.CheckoutDir) @@ -662,7 +662,7 @@ var _ = Describe("Manager CRD Lifecycle", Label("manager"), Serial, Ordered, fun expectedFile := filepath.Join(crdLifecycleRepo.CheckoutDir, "e2e/crd-delete-test", - "apiextensions.k8s.io/v1/customresourcedefinitions/"+iceCreamCRDMirrorFile(crdGroupCRDLifecycle)) + "_cluster/apiextensions.k8s.io/customresourcedefinitions/"+iceCreamCRDMirrorFile(crdGroupCRDLifecycle)) _, statErr := os.Stat(expectedFile) g.Expect(statErr).To(HaveOccurred(), "CRD file must stay deleted after CRD termination updates") g.Expect(os.IsNotExist(statErr)).To(BeTrue(), "CRD file must not reappear in Git") diff --git a/test/e2e/deletecollection_intent_e2e_test.go b/test/e2e/deletecollection_intent_e2e_test.go index b1991bd9..2b249ee6 100644 --- a/test/e2e/deletecollection_intent_e2e_test.go +++ b/test/e2e/deletecollection_intent_e2e_test.go @@ -325,7 +325,7 @@ func forceClearConfigMapFinalizers(client *kubernetes.Clientset, ns string) { } func configMapRepoPath(ns, name string) string { - return path.Join(dcIntentBasePath, fmt.Sprintf("v1/configmaps/%s/%s.yaml", ns, name)) + return path.Join(dcIntentBasePath, fmt.Sprintf("%s/configmaps/%s.yaml", ns, name)) } func waitForFilePresent(repo *RepoArtifacts, repoPath string) { diff --git a/test/e2e/demo_e2e_test.go b/test/e2e/demo_e2e_test.go index 3481e414..9eaf0a05 100644 --- a/test/e2e/demo_e2e_test.go +++ b/test/e2e/demo_e2e_test.go @@ -26,10 +26,9 @@ const ( var demoCoffeeConfigGitPath = strings.Join([]string{ "voter-coffee", + "voter-test", "examples.configbutler.ai", - "v1alpha1", "coffeeconfigs", - "voter-test", "testnet-coffee.yaml", }, "/") diff --git a/test/e2e/deployment_scale_author_attribution_e2e_test.go b/test/e2e/deployment_scale_author_attribution_e2e_test.go index 5e1d8304..f3ce1173 100644 --- a/test/e2e/deployment_scale_author_attribution_e2e_test.go +++ b/test/e2e/deployment_scale_author_attribution_e2e_test.go @@ -80,7 +80,7 @@ var _ = Describe("Deployment scale author attribution", Label("manager", "subres osDeploymentFile := filepath.Join( repo.CheckoutDir, targetPath, - fmt.Sprintf("apps/v1/deployments/%s/%s.yaml", testNs, deploymentName), + fmt.Sprintf("%s/apps/deployments/%s.yaml", testNs, deploymentName), ) Eventually(func(g Gomega) { g.Expect(committedDeploymentReplicas(g, repo.CheckoutDir, osDeploymentFile)).To(Equal(int64(1))) @@ -96,7 +96,7 @@ var _ = Describe("Deployment scale author attribution", Label("manager", "subres ) Expect(err).NotTo(HaveOccurred(), "impersonated kubectl scale should succeed") - repoPath := path.Join(targetPath, fmt.Sprintf("apps/v1/deployments/%s/%s.yaml", testNs, deploymentName)) + repoPath := path.Join(targetPath, fmt.Sprintf("%s/apps/deployments/%s.yaml", testNs, deploymentName)) By("verifying the parent Deployment manifest is updated AND the commit is authored by the scaler") Eventually(func(g Gomega) { diff --git a/test/e2e/deployment_scale_subresource_e2e_test.go b/test/e2e/deployment_scale_subresource_e2e_test.go index 7c568869..979cf85b 100644 --- a/test/e2e/deployment_scale_subresource_e2e_test.go +++ b/test/e2e/deployment_scale_subresource_e2e_test.go @@ -47,7 +47,7 @@ var _ = Describe("Deployment scale subresource", Label("manager", "subresource") deploymentFile := filepath.Join( repo.CheckoutDir, targetPath, - fmt.Sprintf("apps/v1/deployments/%s/%s.yaml", testNs, deploymentName), + fmt.Sprintf("%s/apps/deployments/%s.yaml", testNs, deploymentName), ) Eventually(func(g Gomega) { g.Expect(committedDeploymentReplicas(g, repo.CheckoutDir, deploymentFile)).To(Equal(int64(1))) diff --git a/test/e2e/f4_placement_e2e_test.go b/test/e2e/f4_placement_e2e_test.go new file mode 100644 index 00000000..cd419a30 --- /dev/null +++ b/test/e2e/f4_placement_e2e_test.go @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: Apache-2.0 + +package e2e + +import ( + "fmt" + "os" + "path/filepath" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// Validates F4 new-file placement end-to-end: a brand-new resource with no +// existing document in Git — the "install something extra in test" launch use +// case (docs/design/manifest/version2/gittarget-new-file-placement-rules.md, +// docs/design/gitops-api/README.md) — lands inside the kustomize-managed overlay +// directory it belongs to, not the canonical GVR-tree path, and the overlay's +// kustomization.yaml gains the resources: entry so kustomize actually renders it. +var _ = Describe("Manager F4 New-File Placement", Label("manager", "f4-placement"), Ordered, func() { + var ( + testNs string + repo *RepoArtifacts + providerName = "f4-placement-provider" + destName = "f4-placement-dest" + ruleName = "f4-placement-rule" + gitPath = "e2e/f4-placement" + ) + + const ( + fixtureRoot = "test/e2e/fixtures/f4-placement-folder" + newConfigMap = "debug-toolbox" + newFileRepoPath = "debug-toolbox.yaml" + kustRepoPath = "kustomization.yaml" + repoName = "e2e-f4-placement" + ) + + BeforeAll(func() { + By("creating the f4-placement test namespace") + testNs = testNamespaceFor("manager-f4-placement") + _, _ = kubectlRun("create", "namespace", testNs) + + By("setting up Gitea repo and credentials") + repo = SetupRepo(resolveE2EContext(), testNs, fmt.Sprintf("%s-%d", repoName, GinkgoRandomSeed())) + + _, err := kubectlRunInNamespace(testNs, "apply", "-f", repo.SecretsYAML) + Expect(err).NotTo(HaveOccurred(), "failed to apply git secrets to test namespace") + + applySOPSAgeKeyToNamespace(testNs) + + By("creating the GitProvider") + createGitProviderWithURLInNamespace(providerName, testNs, repo.GitSecretHTTP, repo.RepoURLHTTP) + verifyResourceStatus("gitprovider", providerName, testNs, "True", "Ready", "") + }) + + AfterAll(func() { + _, _ = kubectlRunInNamespace(testNs, "delete", "configmap", newConfigMap, "--ignore-not-found=true") + cleanupWatchRule(ruleName, testNs) + cleanupGitTarget(destName, testNs) + _, _ = kubectlRunInNamespace(testNs, "delete", "gitprovider", providerName, "--ignore-not-found=true") + cleanupNamespace(testNs) + }) + + It("places a brand-new resource inside its kustomize overlay and registers it in resources:", func() { + renderedFixture := renderInPlaceFixtureFolder(fixtureRoot, testNs) + DeferCleanup(func() { _ = os.RemoveAll(renderedFixture) }) + + By("seeding the Git repository with the kustomize overlay fixture") + seedRenderedFolderIntoRepo(repo, testNs, renderedFixture, gitPath) + + By("applying the rendered overlay with Kustomize") + _, err := kubectlRunInNamespace(testNs, "apply", "-k", renderedFixture) + Expect(err).NotTo(HaveOccurred(), "failed to apply rendered fixture kustomization") + + By("creating the GitTarget and ConfigMap WatchRule") + createGitTarget(destName, testNs, providerName, gitPath, "main") + err = applyFromTemplate("test/e2e/templates/manager/watchrule-configmap.tmpl", struct { + Name string + Namespace string + DestinationName string + }{Name: ruleName, Namespace: testNs, DestinationName: destName}, testNs) + Expect(err).NotTo(HaveOccurred(), "failed to apply ConfigMap WatchRule") + verifyResourceCondition("gittarget", destName, testNs, "Validated", "True", "OK", "") + verifyResourceStatus("watchrule", ruleName, testNs, "True", "Ready", "") + waitForStreamsRunning(destName, testNs) + + By("creating a brand-new ConfigMap with no existing document in Git") + _, err = kubectlRunInNamespace(testNs, "create", "configmap", newConfigMap, "--from-literal=color=blue") + Expect(err).NotTo(HaveOccurred(), "failed to create the new ConfigMap") + + By("verifying the new file landed in the overlay and the kustomization was updated") + newFileFullPath := filepath.Join(repo.CheckoutDir, gitPath, newFileRepoPath) + kustFullPath := filepath.Join(repo.CheckoutDir, gitPath, kustRepoPath) + canonicalPath := filepath.Join(repo.CheckoutDir, gitPath, testNs, "configmaps", newConfigMap+".yaml") + + Eventually(func(g Gomega) { + pullLatestRepoState(g, repo.CheckoutDir) + + newFileBody := readRepoFile(g, newFileFullPath) + g.Expect(newFileBody).To(ContainSubstring("name: " + newConfigMap)) + g.Expect(newFileBody).To(ContainSubstring("color: blue")) + g.Expect(newFileBody).NotTo(ContainSubstring("namespace:"), + "the overlay's kustomization sets namespace:, so the new file must not repeat it") + + kustBody := readRepoFile(g, kustFullPath) + g.Expect(kustBody).To(ContainSubstring("- deployment.yaml"), "the existing entry must survive") + g.Expect(kustBody).To(ContainSubstring("- " + newFileRepoPath)) + + _, statErr := os.Stat(canonicalPath) + g.Expect(os.IsNotExist(statErr)). + To(BeTrue(), "must not also create a canonical-path duplicate %s", canonicalPath) + }, 120*time.Second, 3*time.Second).Should(Succeed()) + + By("✅ new resource placed inside the kustomize overlay and registered in resources:") + }) +}) diff --git a/test/e2e/fixtures/f4-placement-folder/deployment.yaml b/test/e2e/fixtures/f4-placement-folder/deployment.yaml new file mode 100644 index 00000000..c56ae44f --- /dev/null +++ b/test/e2e/fixtures/f4-placement-folder/deployment.yaml @@ -0,0 +1,21 @@ +# A pre-existing resource in the overlay, proving the folder is already a real +# kustomize context before F4 places anything new in it. +apiVersion: apps/v1 +kind: Deployment +metadata: + name: f4-placement-web + labels: + app.kubernetes.io/part-of: f4-placement +spec: + replicas: 1 + selector: + matchLabels: + app: f4-placement-web + template: + metadata: + labels: + app: f4-placement-web + spec: + containers: + - name: web + image: nginx:1.27 diff --git a/test/e2e/fixtures/f4-placement-folder/kustomization.yaml b/test/e2e/fixtures/f4-placement-folder/kustomization.yaml new file mode 100644 index 00000000..aafea77c --- /dev/null +++ b/test/e2e/fixtures/f4-placement-folder/kustomization.yaml @@ -0,0 +1,5 @@ +apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +namespace: __E2E_NAMESPACE__ +resources: + - deployment.yaml diff --git a/test/e2e/gittarget_isolation_e2e_test.go b/test/e2e/gittarget_isolation_e2e_test.go index 77af6876..6b8ecf84 100644 --- a/test/e2e/gittarget_isolation_e2e_test.go +++ b/test/e2e/gittarget_isolation_e2e_test.go @@ -110,7 +110,7 @@ var _ = Describe("Manager GitTarget Isolation", Label("manager"), Ordered, func( applyIsolationConfigMap(cmName, testNs) By("asserting target A commits it as an event commit, not a reconcile") - relPath := path.Join(pathA, fmt.Sprintf("v1/configmaps/%s/%s.yaml", testNs, cmName)) + relPath := path.Join(pathA, fmt.Sprintf("%s/configmaps/%s.yaml", testNs, cmName)) assertEventCommit := func(g Gomega) { pullLatestRepoState(g, isoRepo.CheckoutDir) diff --git a/test/e2e/icecream.go b/test/e2e/icecream.go index efe2ceae..11001235 100644 --- a/test/e2e/icecream.go +++ b/test/e2e/icecream.go @@ -25,15 +25,17 @@ func iceCreamCRDName(group string) string { } // iceCreamCRDMirrorFile returns the filename gitops-reverser writes for the CRD -// under apiextensions.k8s.io/v1/customresourcedefinitions/. +// under _cluster/apiextensions.k8s.io/customresourcedefinitions/ (CRDs are +// cluster-scoped, so the scope segment is the literal "_cluster"). func iceCreamCRDMirrorFile(group string) string { return iceCreamCRDName(group) + ".yaml" } -// iceCreamInstanceDir returns the mirror path prefix for IceCreamOrder instances -// of the given group: "/v1/icecreamorders". -func iceCreamInstanceDir(group string) string { - return group + "/v1/icecreamorders" +// iceCreamInstancePath returns the canonical mirror path for one IceCreamOrder +// instance under the new namespace-first, version-less layout: +// "//icecreamorders/.yaml". +func iceCreamInstancePath(group, namespace, name string) string { + return namespace + "/" + group + "/icecreamorders/" + name + ".yaml" } // applyIceCreamCRD renders and applies the IceCreamOrder CRD for the given group. diff --git a/test/e2e/inplace_edit_e2e_test.go b/test/e2e/inplace_edit_e2e_test.go index 846eb690..047240fb 100644 --- a/test/e2e/inplace_edit_e2e_test.go +++ b/test/e2e/inplace_edit_e2e_test.go @@ -35,7 +35,7 @@ var _ = Describe("Manager In-Place Manifest Editing", Label("manager", "inplace- const preservedComment = "# gitops-reverser-e2e: preserve-this-comment" configMapRepoPath := func() string { - return filepath.Join(gitPath, fmt.Sprintf("v1/configmaps/%s/%s.yaml", testNs, cmName)) + return filepath.Join(gitPath, fmt.Sprintf("%s/configmaps/%s.yaml", testNs, cmName)) } BeforeAll(func() { @@ -254,7 +254,7 @@ var _ = Describe( g.Expect(kustomizationBody).To(Equal(readRepoFile(g, renderedKustomization))) for _, name := range []string{bundleConfigMapName, nestedConfigMapName} { - canonicalPath := filepath.Join(repo.CheckoutDir, gitPath, "v1", "configmaps", testNs, name+".yaml") + canonicalPath := filepath.Join(repo.CheckoutDir, gitPath, testNs, "configmaps", name+".yaml") _, statErr := os.Stat(canonicalPath) g.Expect(os.IsNotExist(statErr)). To(BeTrue(), "must not create canonical duplicate %s", canonicalPath) diff --git a/test/e2e/quickstart_framework_e2e_test.go b/test/e2e/quickstart_framework_e2e_test.go index dfbbfa14..465291f8 100644 --- a/test/e2e/quickstart_framework_e2e_test.go +++ b/test/e2e/quickstart_framework_e2e_test.go @@ -225,9 +225,8 @@ func (r *quickstartFrameworkRun) verifyQuickstartConfigMapCommits() { expectedFile := filepath.Join( r.checkoutDir, "live-cluster", - "v1", - "configmaps", ns, + "configmaps", fmt.Sprintf("%s.yaml", configMapName), ) @@ -316,9 +315,8 @@ func (r *quickstartFrameworkRun) verifyQuickstartSecretEncryption(generatedAgeKe expectedFile := filepath.Join( r.checkoutDir, "live-cluster", - "v1", - "secrets", ns, + "secrets", fmt.Sprintf("%s.sops.yaml", secretName), ) diff --git a/test/e2e/restart_reconcile_e2e_test.go b/test/e2e/restart_reconcile_e2e_test.go index 45e08797..b22d8f7e 100644 --- a/test/e2e/restart_reconcile_e2e_test.go +++ b/test/e2e/restart_reconcile_e2e_test.go @@ -123,7 +123,7 @@ var _ = Describe("Restart Reconcile Safety", Label("restart-reconcile"), Serial, expectedFiles := make([]string, 0, len(orderNames)) for _, name := range orderNames { expectedFiles = append(expectedFiles, filepath.Join( - gitTargetPath, iceCreamInstanceDir(crdGroupRestartReconcile), testNs, name+".yaml", + gitTargetPath, iceCreamInstancePath(crdGroupRestartReconcile, testNs, name), )) } diff --git a/test/e2e/signing_e2e_test.go b/test/e2e/signing_e2e_test.go index f8ac7363..534fdd31 100644 --- a/test/e2e/signing_e2e_test.go +++ b/test/e2e/signing_e2e_test.go @@ -364,7 +364,7 @@ var _ = Describe("Commit Signing", Label("signing"), Ordered, func() { // per-event ConfigMap commit. That backfill touches a different file, so scoping to the ConfigMap // file (and matching the per-event commit explicitly) asserts the per-event behaviour without // being fragile to that acceptable setup-time ordering. - cmFile := fmt.Sprintf("%s/v1/configmaps/%s/%s.yaml", commitPath, testNs, cmName) + cmFile := fmt.Sprintf("%s/%s/configmaps/%s.yaml", commitPath, testNs, cmName) Eventually(func(g Gomega) { _, pullErr := gitRun(signingRepo.CheckoutDir, "pull") g.Expect(pullErr).NotTo(HaveOccurred()) diff --git a/test/e2e/watchrule_configmap_secret_e2e_test.go b/test/e2e/watchrule_configmap_secret_e2e_test.go index 3a1db556..8f5224c6 100644 --- a/test/e2e/watchrule_configmap_secret_e2e_test.go +++ b/test/e2e/watchrule_configmap_secret_e2e_test.go @@ -170,14 +170,12 @@ spec: expectedConfigMap := filepath.Join( watchRuleRepo.CheckoutDir, gitTargetPath, - fmt.Sprintf("v1/configmaps/%s/%s.yaml", testNs, configMapName), + fmt.Sprintf("%s/configmaps/%s.yaml", testNs, configMapName), ) expectedOrder := filepath.Join( watchRuleRepo.CheckoutDir, gitTargetPath, - iceCreamInstanceDir(crdGroupWildcardRule), - testNs, - orderName+".yaml", + iceCreamInstancePath(crdGroupWildcardRule, testNs, orderName), ) Eventually(func(g Gomega) { pullLatestRepoState(g, watchRuleRepo.CheckoutDir) @@ -256,7 +254,7 @@ spec: expectedFile := filepath.Join(watchRuleRepo.CheckoutDir, "e2e/secret-encryption-test", - fmt.Sprintf("v1/secrets/%s/%s.sops.yaml", testNs, secretName)) + fmt.Sprintf("%s/secrets/%s.sops.yaml", testNs, secretName)) content, readErr := os.ReadFile(expectedFile) g.Expect(readErr).NotTo(HaveOccurred(), fmt.Sprintf("Secret file must exist at %s", expectedFile)) g.Expect(string(content)).To(ContainSubstring("sops:")) @@ -412,7 +410,7 @@ spec: expectedFile := filepath.Join(watchRuleRepo.CheckoutDir, "e2e/secret-autogen-test", - fmt.Sprintf("v1/secrets/%s/%s.sops.yaml", testNs, secretName)) + fmt.Sprintf("%s/secrets/%s.sops.yaml", testNs, secretName)) content, readErr := os.ReadFile(expectedFile) g.Expect(readErr).NotTo(HaveOccurred(), fmt.Sprintf("Secret file must exist at %s", expectedFile)) g.Expect(string(content)).To(ContainSubstring("sops:")) @@ -511,14 +509,14 @@ spec: // Check for the expected ConfigMap file (new API-aligned path) expectedFile := filepath.Join(watchRuleRepo.CheckoutDir, "e2e/configmap-test", - fmt.Sprintf("v1/configmaps/%s/%s.yaml", testNs, configMapName)) + fmt.Sprintf("%s/configmaps/%s.yaml", testNs, configMapName)) fileInfo, statErr := os.Stat(expectedFile) g.Expect(statErr).NotTo(HaveOccurred(), fmt.Sprintf("ConfigMap file should exist at %s", expectedFile)) g.Expect(fileInfo.Size()).To(BeNumerically(">", 0), "ConfigMap file should not be empty") expectedRepoPath := path.Join( "e2e/configmap-test", - fmt.Sprintf("v1/configmaps/%s/%s.yaml", testNs, configMapName), + fmt.Sprintf("%s/configmaps/%s.yaml", testNs, configMapName), ) assertLatestCommitForPathTouchesOnlyWithOptional( g, @@ -534,7 +532,7 @@ spec: assertLatestCommitTouchesNoNamespaces( g, watchRuleRepo.CheckoutDir, - "e2e/configmap-test/v1/configmaps", + "e2e/configmap-test", []string{ "gitops-reverser", "flux-system", @@ -634,7 +632,7 @@ spec: // must not appear in the repo before we apply the WatchRule. expectedRepoPath := path.Join( "e2e/backfill-rule-add", - fmt.Sprintf("v1/configmaps/%s/%s.yaml", testNs, configMapName), + fmt.Sprintf("%s/configmaps/%s.yaml", testNs, configMapName), ) expectedFile := filepath.Join(watchRuleRepo.CheckoutDir, expectedRepoPath) @@ -731,7 +729,7 @@ spec: // Check for the expected ConfigMap file (new API-aligned path) expectedFile := filepath.Join(watchRuleRepo.CheckoutDir, "e2e/delete-test", - fmt.Sprintf("v1/configmaps/%s/%s.yaml", testNs, configMapName)) + fmt.Sprintf("%s/configmaps/%s.yaml", testNs, configMapName)) fileInfo, statErr := os.Stat(expectedFile) g.Expect(statErr).NotTo(HaveOccurred(), fmt.Sprintf("ConfigMap file should exist at %s", expectedFile)) g.Expect(fileInfo.Size()).To(BeNumerically(">", 0), "ConfigMap file should not be empty") @@ -751,7 +749,7 @@ spec: // Check that the ConfigMap file no longer exists (new API-aligned path) expectedRelativePath := path.Join( "e2e/delete-test", - fmt.Sprintf("v1/configmaps/%s/%s.yaml", testNs, configMapName), + fmt.Sprintf("%s/configmaps/%s.yaml", testNs, configMapName), ) expectedFile := filepath.Join(watchRuleRepo.CheckoutDir, expectedRelativePath) _, statErr := os.Stat(expectedFile) @@ -768,7 +766,7 @@ spec: assertLatestCommitTouchesNoNamespaces( g, watchRuleRepo.CheckoutDir, - "e2e/delete-test/v1/configmaps", + "e2e/delete-test", []string{ "gitops-reverser", "flux-system",