From 73d1e7f635948dd7224adea2bb6b08ed05611ba4 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 28 Aug 2026 11:24:45 +0000 Subject: [PATCH] fix(placement): register a new file with its nearest ancestor kustomization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A declared path into a subdirectory of a kustomize folder produced a document that no kustomization lists, so `kubectl apply -k` never rendered it: one byType line was enough to put a file in Git that nothing applies. governingKustomization looked only in the file's own directory, plus a special case for the write scope's root, so registration depended on whether render-root scoping happened to be in force — a difference the user cannot see. It now walks up from the file's own directory to the nearest kustomization, bounded by the write jail, because a kustomization above the write scope is a read-only base whose resources: list is not ours to edit. That also reaches namespaceIsInheritedFromContext, which was unreachable for these paths: the document used to carry a metadata.namespace line the folder's own documents omit. Drop the {version} requirement from IdentityCompletePlacementTemplate. The built-in canonical path is deliberately versionless, because two served versions of one group/resource are the same object, so a version segment separates no identities. Requiring it judged the canonical shape as not identity-complete, which is what would make any spec-level default fail our own validation gate. Fixes #295 Co-Authored-By: Claude Opus 5 --- .../gittarget-new-file-placement-rules.md | 22 ++++++++ .../gittarget_placement_validation_test.go | 7 +++ internal/git/placement_metrics_test.go | 34 ++++++++++++ internal/git/placement_test.go | 32 ++++++++++++ internal/manifestanalyzer/placement.go | 52 ++++++++++++++----- internal/manifestanalyzer/placement_test.go | 7 +++ 6 files changed, 140 insertions(+), 14 deletions(-) diff --git a/docs/spec/gittarget-new-file-placement-rules.md b/docs/spec/gittarget-new-file-placement-rules.md index b44efa3c..d7186793 100644 --- a/docs/spec/gittarget-new-file-placement-rules.md +++ b/docs/spec/gittarget-new-file-placement-rules.md @@ -623,6 +623,28 @@ one root, not from picking the largest matching cohort of similar documents. Con That is invisible in the folder, so it is counted: `placement_kustomization_entries_total{outcome="failed"}`. +### Registration follows the nearest ancestor kustomization + +Registration is a property of **where the file lands**, so it applies to every resolved path +rather than to the fallback that produced one. The kustomization a new file joins is the +nearest one at or above its own directory, bounded by the write jail: a path in the file's own +directory first, then each parent up to `spec.path` (or, under render-root scoping, up to the +write scope, because a kustomization above it is a read-only base whose `resources:` list is +not ours to edit). + +The walk is what makes a declared path behave like the fallback. A single line — + +```yaml +placement: + byType: + v1/configmaps: "configmaps/{name}.yaml" +``` + +— in a folder whose root has a `kustomization.yaml` used to commit a document that no +kustomization lists, which `kubectl apply -k` therefore never renders, and which the namespace +rule below also skipped. The two cases differed by whether render-root scoping happened to be +in force, which is not something the user can see. + ### Namespace style follows the governing kustomization A document in a directory whose kustomization sets a `namespace:` transformer does not carry diff --git a/internal/controller/gittarget_placement_validation_test.go b/internal/controller/gittarget_placement_validation_test.go index a5ab6abb..91afde22 100644 --- a/internal/controller/gittarget_placement_validation_test.go +++ b/internal/controller/gittarget_placement_validation_test.go @@ -41,6 +41,13 @@ func TestValidatePlacementPolicy(t *testing.T) { }, true, }, + { + "the versionless canonical default is accepted (#295)", + &configbutleraiv1alpha3.GitTargetPlacementSpec{ + Default: "{namespaceOrCluster}/{groupPath}/{resource}/{name}.yaml", + }, + true, + }, { "bundling default with no Secret route is rejected", &configbutleraiv1alpha3.GitTargetPlacementSpec{ diff --git a/internal/git/placement_metrics_test.go b/internal/git/placement_metrics_test.go index 8b638b2e..62322d0e 100644 --- a/internal/git/placement_metrics_test.go +++ b/internal/git/placement_metrics_test.go @@ -347,3 +347,37 @@ func TestPlacementMetrics_RefusedPlacementLeavesTheKustomizationAlone(t *testing }) assert.Zero(t, added, "a refused resource must never count as an added resources: entry") } + +// The metric's own regression test for #295: a declared path into a subdirectory of a +// kustomize folder used to produce no entry at all — the document was committed, the +// resources: list never mentioned it, and the counter recorded nothing, so the one signal +// that a file is rendered by nothing stayed silent. The ancestor walk makes it an `added`. +func TestPlacementMetrics_DeclaredSubdirectoryEntryIsAdded(t *testing.T) { + reader, err := telemetry.InitTestExporter() + require.NoError(t, err) + worktree := newWorktreeForTest(t) + seedPlacedManifest(t, worktree, "kustomization.yaml", + "namespace: app\nresources:\n - deployment.yaml\n") + seedPlacedManifest(t, worktree, "deployment.yaml", + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: web\n") + policy := &manifestanalyzer.PlacementPolicy{ + ByType: map[string]string{"v1/configmaps": "configmaps/{name}.yaml"}, + } + + flushWithPolicy(t, worktree, policy, targetedConfigMapEvent()) + + entries, ok := telemetry.CollectInt64Sum(reader, kustomizationEntriesMetric, map[string]string{ + "gittarget_namespace": metricsTestGitTargetNamespace, + "gittarget_name": metricsTestGitTargetName, + "outcome": "added", + }) + require.True(t, ok, "a declared subdirectory path must still be registered with its ancestor root") + assert.Equal(t, int64(1), entries) + + failed, hasFailed := telemetry.CollectInt64Sum(reader, kustomizationEntriesMetric, map[string]string{ + "gittarget_namespace": metricsTestGitTargetNamespace, + "gittarget_name": metricsTestGitTargetName, + "outcome": "failed", + }) + assert.False(t, hasFailed && failed > 0, "no entry may be counted as failed") +} diff --git a/internal/git/placement_test.go b/internal/git/placement_test.go index 4d7ff578..1ee25c8b 100644 --- a/internal/git/placement_test.go +++ b/internal/git/placement_test.go @@ -745,3 +745,35 @@ func TestPlacement_ColdBundleCollision_ViaResync(t *testing.T) { require.NoError(t, readErr) assert.Equal(t, 2, strings.Count(string(got), "kind: ConfigMap"), "both resync creates must survive") } + +// A declared path into a SUBDIRECTORY of a kustomize folder must still be registered +// with the root that governs it (#295). Before the ancestor walk, governingKustomization +// looked only in the new file's own directory (plus the write scope's root under +// render-root scoping), so one byType line put a document in Git that no kustomization +// lists — committed, and rendered by nothing. +func TestPlacement_DeclaredSubdirectory_RegistersWithTheAncestorKustomization(t *testing.T) { + worktree := newWorktreeForTest(t) + root := worktree.Filesystem().Root() + seedPlacedManifest(t, worktree, "kustomization.yaml", + "namespace: app\nresources:\n - deployment.yaml\n") + seedPlacedManifest(t, worktree, "deployment.yaml", + "apiVersion: apps/v1\nkind: Deployment\nmetadata:\n name: web\n") + policy := &manifestanalyzer.PlacementPolicy{ + ByType: map[string]string{"v1/configmaps": "configmaps/{name}.yaml"}, + } + + changed := applyEventsWithPolicy(t, worktree, policy, newConfigMapEvent("cache", "app")) + require.True(t, changed) + + newFile, err := os.ReadFile(filepath.Join(root, "configmaps/cache.yaml")) + require.NoError(t, err, "the declared path must be honoured") + assert.Contains(t, string(newFile), "name: cache") + + kust, err := os.ReadFile(filepath.Join(root, "kustomization.yaml")) + require.NoError(t, err) + assert.Contains(t, string(kust), "configmaps/cache.yaml", + "the ancestor kustomization must list the new file, or kustomize never renders it") + + assert.NotContains(t, string(newFile), "namespace:", + "the governing kustomization's namespace: transformer supplies the namespace") +} diff --git a/internal/manifestanalyzer/placement.go b/internal/manifestanalyzer/placement.go index e7a4a28b..8919f6b6 100644 --- a/internal/manifestanalyzer/placement.go +++ b/internal/manifestanalyzer/placement.go @@ -364,20 +364,40 @@ func namespaceIsInheritedFromContext(k *KustomizationInfo, req PlacementRequest) } // governingKustomization returns the kustomization whose resources: list a new file at -// resolvedPath must join to render: the one in its own directory, or — under render-root -// scoping, when the file lands in a subdirectory of the overlay that has no kustomization of -// its own — the write scope's own render root, which reaches the file by a relative resources -// entry. Without this an overlay's new object would be committed to a file no kustomization -// includes, so it would never render and the oracle (armed only for governed writes) would not -// catch it — a silent divergence. +// resolvedPath must join to render: the NEAREST one at or above the file's own directory, +// bounded by the write jail. Without this an overlay's new object would be committed to a +// file no kustomization includes, so it would never render and the oracle (armed only for +// governed writes) would not catch it — a silent divergence. +// +// The walk is what makes a DECLARED path into a subdirectory behave like every other path +// (#295). Before it, the lookup was the file's own directory plus a special case for the +// write scope's root, so `byType: {v1/configmaps: "configmaps/{name}.yaml"}` in a kustomize +// folder committed a document nothing renders — and the two cases differed by render-root +// scoping, which the user cannot see. It also silently skipped +// namespaceIsInheritedFromContext, writing a namespace: line the folder's own documents omit. +// +// The jail is the bound, and it is load-bearing: writeScope is where the target may write, so +// a kustomization ABOVE it is a read-only ancestor (a base pulled into the scan by render-root +// scoping) whose resources: list is not ours to edit. With no scope the whole scanned subtree +// is the target's own, so the walk may reach its root. func governingKustomization(store *ManifestStore, writeScope, resolvedPath string) *KustomizationInfo { - if k := store.Kustomizations[slashDir(resolvedPath)]; k != nil { - return k + jail := path.Clean(writeScope) + if writeScope == "" { + jail = "." } - if writeScope != "" { - return store.Kustomizations[writeScope] + for dir := slashDir(resolvedPath); ; { + if k := store.Kustomizations[dir]; k != nil { + return k + } + if dir == jail || dir == "." { + return nil + } + parent := slashDir(dir) + if parent == dir { + return nil + } + dir = parent } - return nil } func kustomizationListsResource(k *KustomizationInfo, resolvedPath string) bool { @@ -642,6 +662,12 @@ func ValidPlacementTemplatePath(tmpl string) error { // itself already names one exact type); a Default template must additionally carry // the type variables since it applies across every type the class does not name // explicitly. +// +// The type variables are {groupPath} and {resource}, and deliberately NOT {version} +// (#295): the built-in canonical path is versionless because two served versions of one +// group/resource are the SAME object, so a version segment separates no identities — it +// splits one. Requiring it also judged the canonical shape we would default to as not +// identity-complete, which is what made a spec-level default fail our own validation gate. func IdentityCompletePlacementTemplate(tmpl string, narrowedToOneType bool) bool { hasName := strings.Contains(tmpl, "{name}") hasScope := strings.Contains(tmpl, "{namespace}") || strings.Contains(tmpl, "{namespaceOrCluster}") @@ -651,9 +677,7 @@ func IdentityCompletePlacementTemplate(tmpl string, narrowedToOneType bool) bool if narrowedToOneType { return true } - return strings.Contains(tmpl, "{groupPath}") && - strings.Contains(tmpl, "{version}") && - strings.Contains(tmpl, "{resource}") + return strings.Contains(tmpl, "{groupPath}") && strings.Contains(tmpl, "{resource}") } // --- Write-safety helpers for an already-occupied destination ------------------ diff --git a/internal/manifestanalyzer/placement_test.go b/internal/manifestanalyzer/placement_test.go index 30bca795..d67bb558 100644 --- a/internal/manifestanalyzer/placement_test.go +++ b/internal/manifestanalyzer/placement_test.go @@ -664,7 +664,14 @@ func TestIdentityCompletePlacementTemplate(t *testing.T) { want bool }{ {"full identity", "{groupPath}/{version}/{resource}/{namespaceOrCluster}/{name}.yaml", false, true}, + { + "versionless canonical shape", + "{namespaceOrCluster}/{groupPath}/{resource}/{name}.yaml", + false, + true, + }, {"missing resource for default", "{groupPath}/{version}/{namespaceOrCluster}/{name}.yaml", false, false}, + {"missing group for default", "{version}/{resource}/{namespaceOrCluster}/{name}.yaml", false, false}, {"narrowed type needs only scope+name", "{namespace}/secret-{name}.sops.yaml", true, true}, {"narrowed type missing name", "{namespace}/secret.sops.yaml", true, false}, {"narrowed type missing scope", "secret-{name}.sops.yaml", true, false},