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