fix(bundler): compare the resolved chart name, not the registry default

Review found gatekeeper never emitted CreateReplace despite being enrolled in
ownsCRDs, and reproduced it on all three enrolled components.

ApplyRegistryDefaults strips a defaultChart to the segment after its last "/"
when defaulting ref.Chart. gatekeeper's registry entry is
"gatekeeper/gatekeeper", so a stock recipe resolves ref.Chart="gatekeeper"
while usesRegistryChart compared against the unstripped value. One of the
three headline components was silently inert, and the PR body, the registry
comment, and the component catalog all claimed otherwise. It failed into the
safe direction -- Skip, no destructive replace -- but it did not do what was
documented.

usesRegistryChart now normalizes the registry value the same way, and version
comparison runs both sides through deployer.NormalizeVersion so an overlay
re-pinning "v1.3.0" against a registry "1.3.0" does not silently disable the
policy for an audited chart.

The test could not have caught this. It built refs by copying registry fields
directly, which produces a shape the resolver never emits, and its comment
claimed that shape was "what a stock recipe resolves to". Refs are now built by
running ApplyRegistryDefaults over an empty ref, and the owner cases are
discovered from the registry rather than hardcoded, so gatekeeper and
nvsentinel are covered and a future enrollment is covered without editing the
test. Only k8s-aibom had been exercised.

The chartRef render assertion was replaced by a direct usesRegistryChart test.
Attempting the render route produced a sourceRef HelmRelease, so a render test
would have passed without exercising the path -- the ambiguous-condition shape
this suite avoids elsewhere. The property test asserts refs without registry
coordinates are rejected, with a resolved ref as the control so the negative
cases cannot pass vacuously.

Adds TestOwnsCRDsPinsMatchAuditedVersions, which pins the chart versions the
CRD-ownership audit actually ran against. usesRegistryChart covers a recipe
overriding coordinates; nothing covered the other direction, where bumping
defaultVersion in the registry carries ownsCRDs forward to a chart nobody
re-checked, which may by then share a CRD or use webhook conversion. The guard
caught two wrong versions on its first run.

Related: #2264
Signed-off-by: Mark Chmarny <mark@chmarny.com>

Author: mchmarny

pkg/bundler/deployer/flux/flux.go

@@ -1018,8 +1018,23 @@ func usesRegistryChart(ref recipe.ComponentRef, cfg *recipe.ComponentConfig) boo
 		return false
 	}
 	return ref.Source == cfg.Helm.DefaultRepository &&
-		ref.EffectiveChart() == cfg.Helm.DefaultChart &&
-		ref.Version == cfg.Helm.DefaultVersion
+		ref.EffectiveChart() == registryChartName(cfg.Helm.DefaultChart) &&
+		deployer.NormalizeVersion(ref.Version) == deployer.NormalizeVersion(cfg.Helm.DefaultVersion)
+}
+
+// registryChartName reduces a registry defaultChart to the form a resolved
+// ComponentRef actually carries.
+//
+// ApplyRegistryDefaults strips everything before the last "/" when defaulting
+// ref.Chart, so a registry entry like "gatekeeper/gatekeeper" resolves to
+// "gatekeeper". Comparing against the unstripped value silently fails for every
+// component whose defaultChart carries a repo-alias prefix, which is how
+// gatekeeper was enrolled in ownsCRDs and never emitted the policy.
+func registryChartName(defaultChart string) string {
+	if idx := strings.LastIndex(defaultChart, "/"); idx >= 0 {
+		return defaultChart[idx+1:]
+	}
+	return defaultChart
 }
 
 // ownsCRDs reports whether the named component may replace its CRDs on

pkg/bundler/deployer/flux/flux_test.go

@@ -3113,134 +3113,201 @@ func TestGenerate_SourceOnlyRefChartFallsBackToName(t *testing.T) {
 // base.yaml and all ship the NodeFeature CRDs, so replacing unconditionally
 // would have several HelmReleases rewrite the same CRD every reconcile.
 //
-// Three directions are asserted, and each fails for a different reason:
+// Refs are built by running ApplyRegistryDefaults over an empty ref, which is
+// what a stock recipe actually resolves to. Constructing them by copying
+// registry fields directly is what an earlier revision did, and it silently
+// diverged: ApplyRegistryDefaults strips a defaultChart to its bare name, so a
+// hand-built ref carrying "gatekeeper/gatekeeper" never matched the real
+// resolved "gatekeeper" and the test could not see that gatekeeper emitted no
+// policy at all. Do not hand-build these refs.
 //
-//	owner            -> emits the policy
-//	sharer           -> must not, because replacing would race another release
-//	coordinates moved -> must not, because ownsCRDs records an audit of the
-//	                    registry's pinned chart and a ref that overrides
-//	                    source/chart/version points somewhere unaudited
-//
-// A positive-only test would pass against an unconditional template, which is
-// the defect this replaced, so the negative cases are the load-bearing ones.
+// The owner cases are discovered from the registry rather than hardcoded, so a
+// component enrolled in ownsCRDs later is covered without touching this test.
 //
 // NOTE ON COVERAGE: this exercises the sourceRef template only. An upstream
 // Helm component renders spec.chart.spec.sourceRef regardless of
-// OCISourceName; the chartRef template is reached by local, vendored and
-// manifest-backed components, and those carry no registry chart coordinates,
-// so they cannot satisfy usesRegistryChart and cannot emit the policy today.
-// Earlier revisions of this test claimed chartRef coverage by setting
-// OCISourceName on a normal Helm ref, which renders sourceRef and tested the
-// same path twice. Do not reintroduce that shape.
+// OCISourceName. TestUsesRegistryChartRejectsRefsWithoutCoordinates covers
+// why the chartRef shape cannot emit the policy today.
 func TestGenerate_HelmReleaseCRDUpgradePolicyIsOptIn(t *testing.T) {
 	registry, regErr := recipe.GetComponentRegistry()
 	if regErr != nil {
 		t.Fatalf("GetComponentRegistry: %v", regErr)
 	}
 
-	// refFor builds a ref at the component's exact registry coordinates, which
-	// is what a stock recipe resolves to.
-	refFor := func(t *testing.T, name string) recipe.ComponentRef {
+	// resolvedRef mirrors stock resolution: an empty ref, then registry
+	// defaults. Anything else risks testing a shape the resolver never emits.
+	resolvedRef := func(t *testing.T, name string) recipe.ComponentRef {
 		t.Helper()
 		cfg := registry.Get(name)
 		if cfg == nil {
 			t.Fatalf("%s missing from registry", name)
 		}
-		return recipe.ComponentRef{
-			Name:      name,
-			Namespace: name,
-			Type:      recipe.ComponentTypeHelm,
-			Source:    cfg.Helm.DefaultRepository,
-			Chart:     cfg.Helm.DefaultChart,
-			Version:   cfg.Helm.DefaultVersion,
+		ref := recipe.ComponentRef{Name: name, Namespace: name, Type: recipe.ComponentTypeHelm}
+		ref.ApplyRegistryDefaults(cfg)
+		return ref
+	}
+
+	// Every enrolled owner, discovered rather than listed.
+	var owners []string
+	for _, name := range registry.Names() {
+		if cfg := registry.Get(name); cfg != nil && cfg.OwnsCRDs {
+			owners = append(owners, name)
+		}
+	}
+	if len(owners) == 0 {
+		t.Fatal("no ownsCRDs components in the registry; this test would prove nothing")
+	}
+
+	t.Run("owners emit the policy", func(t *testing.T) {
+		for _, name := range owners {
+			t.Run(name, func(t *testing.T) {
+				assertCRDPolicy(t, resolvedRef(t, name), name, "CreateReplace")
+			})
 		}
+	})
+
+	// nfd shares the NodeFeature CRDs with gpu-operator and network-operator,
+	// all three of which co-exist in base.yaml.
+	t.Run("sharer does not", func(t *testing.T) {
+		assertCRDPolicy(t, resolvedRef(t, "nfd"), "nfd", "")
+	})
+
+	// ownsCRDs records an audit of the registry's pinned chart. A ref pointing
+	// anywhere else is unaudited, so the policy must not carry over.
+	t.Run("coordinate overrides disable it", func(t *testing.T) {
+		owner := owners[0]
+		for _, tc := range []struct {
+			name   string
+			mutate func(*recipe.ComponentRef)
+		}{
+			{"version", func(r *recipe.ComponentRef) { r.Version = "0.0.1-unaudited" }},
+			{"chart", func(r *recipe.ComponentRef) { r.Chart = "some-fork" }},
+			{"source", func(r *recipe.ComponentRef) { r.Source = "oci://example.invalid/charts" }},
+		} {
+			t.Run(tc.name, func(t *testing.T) {
+				ref := resolvedRef(t, owner)
+				tc.mutate(&ref)
+				assertCRDPolicy(t, ref, owner, "")
+			})
+		}
+	})
+}
+
+// assertCRDPolicy renders one component and asserts spec.upgrade.crds.
+//
+// Decoded as a map, not a typed struct: a string field cannot tell an absent
+// crds key from one explicitly rendered as "", so a negative case would pass
+// against a template emitting an empty value. Presence is the assertion.
+func assertCRDPolicy(t *testing.T, ref recipe.ComponentRef, component, want string) {
+	t.Helper()
+	outputDir := t.TempDir()
+
+	recipeResult := &recipe.RecipeResult{}
+	recipeResult.Metadata.Version = testVersion
+	recipeResult.ComponentRefs = []recipe.ComponentRef{ref}
+
+	g := &Generator{
+		RecipeResult:    recipeResult,
+		ComponentValues: map[string]map[string]any{component: {}},
+		Version:         "v0.9.0",
+	}
+	if _, err := g.Generate(context.Background(), outputDir); err != nil {
+		t.Fatalf("Generate() error = %v", err)
+	}
+
+	raw := readFile(t, filepath.Join(outputDir, component, "helmrelease.yaml"))
+	var doc struct {
+		Spec map[string]any `yaml:"spec"`
+	}
+	if err := yaml.Unmarshal([]byte(raw), &doc); err != nil {
+		t.Fatalf("parse HelmRelease: %v", err)
+	}
+	upgrade, hasUpgrade := doc.Spec["upgrade"]
+
+	if want == "" {
+		if hasUpgrade {
+			t.Errorf("spec.upgrade present (%v), want the key absent entirely\n%s", upgrade, raw)
+		}
+		return
+	}
+	if !hasUpgrade {
+		t.Fatalf("spec.upgrade absent, want crds = %q\n%s", want, raw)
+	}
+	upgradeMap, ok := upgrade.(map[string]any)
+	if !ok {
+		t.Fatalf("spec.upgrade is %T, want a mapping\n%s", upgrade, raw)
+	}
+	crds, hasCRDs := upgradeMap["crds"]
+	if !hasCRDs {
+		t.Fatalf("spec.upgrade.crds absent, want %q\n%s", want, raw)
+	}
+	if crds != want {
+		t.Errorf("spec.upgrade.crds = %v, want %q\n%s", crds, want, raw)
+	}
+}
+
+// TestUsesRegistryChartRejectsRefsWithoutCoordinates pins the property that
+// makes the chartRef template's UpgradeCRDs block unreachable today.
+//
+// chartRef is reached by local, vendored and manifest-backed components. Those
+// carry no registry chart coordinates, so usesRegistryChart is false for them
+// and the policy cannot render. Asserting that directly is preferable to a
+// render test: an attempt at the render route produced a sourceRef HelmRelease
+// instead, so such a test would have passed without exercising anything, which
+// is the ambiguous-condition shape this suite already avoids elsewhere.
+//
+// If a future chartRef path does carry registry coordinates, this test still
+// holds but the block becomes reachable — at which point the chartRef template
+// needs its own render coverage.
+func TestUsesRegistryChartRejectsRefsWithoutCoordinates(t *testing.T) {
+	registry, regErr := recipe.GetComponentRegistry()
+	if regErr != nil {
+		t.Fatalf("GetComponentRegistry: %v", regErr)
+	}
+	cfg := registry.Get("k8s-aibom")
+	if cfg == nil || !cfg.OwnsCRDs {
+		t.Fatal("k8s-aibom must be an ownsCRDs component for this test to mean anything")
 	}
 
 	tests := []struct {
-		name      string
-		component string
-		mutate    func(*recipe.ComponentRef)
-		want      string
+		name string
+		ref  recipe.ComponentRef
+		want bool
 	}{
 		{
-			// k8s-aibom is registry ownsCRDs: sole owner, no webhook conversion.
-			name: "owner at registry coordinates", component: "k8s-aibom", want: "CreateReplace",
-		},
-		{
-			// nfd shares the NodeFeature CRDs with gpu-operator and
-			// network-operator, all three of which co-exist in base.yaml.
-			name: "sharer", component: "nfd", want: "",
-		},
-		{
-			name: "owner with overridden version", component: "k8s-aibom",
-			mutate: func(r *recipe.ComponentRef) { r.Version = "1.2.0" }, want: "",
+			name: "manifest-only ref has no chart or source",
+			ref: recipe.ComponentRef{
+				Name: "k8s-aibom", Type: recipe.ComponentTypeHelm,
+				ManifestFiles: []string{"components/k8s-aibom/values.yaml"},
+			},
+			want: false,
 		},
 		{
-			name: "owner with overridden chart", component: "k8s-aibom",
-			mutate: func(r *recipe.ComponentRef) { r.Chart = "some-fork" }, want: "",
+			name: "local chart path is not the registry chart",
+			ref: recipe.ComponentRef{
+				Name: "k8s-aibom", Type: recipe.ComponentTypeHelm,
+				Chart: "./k8s-aibom", Version: cfg.Helm.DefaultVersion,
+			},
+			want: false,
 		},
 		{
-			name: "owner with overridden source", component: "k8s-aibom",
-			mutate: func(r *recipe.ComponentRef) { r.Source = "oci://example.invalid/charts" }, want: "",
+			// The control: without this, every case above could pass because
+			// usesRegistryChart always returns false.
+			name: "fully resolved registry ref matches",
+			ref: func() recipe.ComponentRef {
+				r := recipe.ComponentRef{Name: "k8s-aibom", Type: recipe.ComponentTypeHelm}
+				r.ApplyRegistryDefaults(cfg)
+				return r
+			}(),
+			want: true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			outputDir := t.TempDir()
-			ref := refFor(t, tt.component)
-			if tt.mutate != nil {
-				tt.mutate(&ref)
-			}
-
-			recipeResult := &recipe.RecipeResult{}
-			recipeResult.Metadata.Version = testVersion
-			recipeResult.ComponentRefs = []recipe.ComponentRef{ref}
-
-			g := &Generator{
-				RecipeResult:    recipeResult,
-				ComponentValues: map[string]map[string]any{tt.component: {}},
-				Version:         "v0.9.0",
-			}
-			if _, err := g.Generate(context.Background(), outputDir); err != nil {
-				t.Fatalf("Generate() error = %v", err)
-			}
-
-			raw := readFile(t, filepath.Join(outputDir, tt.component, "helmrelease.yaml"))
-
-			// Decoded as a map, not a typed struct: a struct field of type
-			// string cannot tell an absent `crds` key from one explicitly
-			// rendered as "", so a negative case would pass against a
-			// template that emits an empty value. Presence is the assertion.
-			var doc struct {
-				Spec map[string]any `yaml:"spec"`
-			}
-			if err := yaml.Unmarshal([]byte(raw), &doc); err != nil {
-				t.Fatalf("parse HelmRelease: %v", err)
-			}
-			upgrade, hasUpgrade := doc.Spec["upgrade"]
-
-			if tt.want == "" {
-				if hasUpgrade {
-					t.Errorf("spec.upgrade present (%v), want the key absent entirely\n%s",
-						upgrade, raw)
-				}
-				return
-			}
-
-			if !hasUpgrade {
-				t.Fatalf("spec.upgrade absent, want crds = %q\n%s", tt.want, raw)
-			}
-			upgradeMap, ok := upgrade.(map[string]any)
-			if !ok {
-				t.Fatalf("spec.upgrade is %T, want a mapping\n%s", upgrade, raw)
-			}
-			crds, hasCRDs := upgradeMap["crds"]
-			if !hasCRDs {
-				t.Fatalf("spec.upgrade.crds absent, want %q\n%s", tt.want, raw)
-			}
-			if crds != tt.want {
-				t.Errorf("spec.upgrade.crds = %v, want %q\n%s", crds, tt.want, raw)
+			if got := usesRegistryChart(tt.ref, cfg); got != tt.want {
+				t.Errorf("usesRegistryChart() = %v, want %v (chart=%q source=%q version=%q)",
+					got, tt.want, tt.ref.EffectiveChart(), tt.ref.Source, tt.ref.Version)
 			}
 		})
 	}