diff --git a/pkg/bundler/deployer/helmfile/helmfile_test.go b/pkg/bundler/deployer/helmfile/helmfile_test.go index a6d1fac9b..14430433b 100644 --- a/pkg/bundler/deployer/helmfile/helmfile_test.go +++ b/pkg/bundler/deployer/helmfile/helmfile_test.go @@ -455,6 +455,111 @@ func TestBuildHelmfile_SameNamespaceNeedsBare(t *testing.T) { } } +// TestBuildHelmfile_CreateNamespaceFromFolder asserts that buildHelmfile +// emits a per-release `createNamespace: false` override when a folder's +// CreateNamespace flag is false (the Talos privileged-namespace pattern), +// and emits no override (relying on helmDefaults.createNamespace: true) +// otherwise. The localformat writer already encodes the +// chart-owns-Namespace decision; the helmfile deployer must honor it or +// helm refuses to import the namespace it created out-of-band. +func TestBuildHelmfile_CreateNamespaceFromFolder(t *testing.T) { + folders := []localformat.Folder{ + // Pre-injection folder that ships its own Namespace (Talos pattern): + // localformat would set CreateNamespace=false; expect the override. + {Index: 1, Dir: "001-gpu-operator-pre", Kind: localformat.KindLocalHelm, + Name: "gpu-operator-pre", Parent: "gpu-operator", CreateNamespace: false}, + // Primary upstream-helm folder: no Namespace conflict possible from + // AICR's perspective; expect no per-release override. + {Index: 2, Dir: "002-gpu-operator", Kind: localformat.KindUpstreamHelm, + Name: "gpu-operator", Parent: "gpu-operator", CreateNamespace: true, + Upstream: &localformat.Upstream{Chart: "gpu-operator", Repo: "https://helm.ngc.nvidia.com/nvidia", Version: "v25.3.3"}}, + } + ns := map[string]string{"gpu-operator": "privileged-gpu-operator"} + doc, err := buildHelmfile(folders, ns, nil) + if err != nil { + t.Fatalf("buildHelmfile() error = %v", err) + } + if len(doc.Releases) != 2 { + t.Fatalf("expected 2 releases, got %d", len(doc.Releases)) + } + if doc.Releases[0].CreateNamespace == nil || *doc.Releases[0].CreateNamespace { + t.Errorf("release[0] (gpu-operator-pre) CreateNamespace = %v, want pointer to false — "+ + "the chart owns the Namespace and helm must not create it out-of-band", + doc.Releases[0].CreateNamespace) + } + if doc.Releases[1].CreateNamespace != nil { + t.Errorf("release[1] (gpu-operator) CreateNamespace = %v, want nil — "+ + "helmDefaults.createNamespace: true covers the common case; no per-release override should be emitted", + doc.Releases[1].CreateNamespace) + } +} + +// TestGenerate_NamespaceOwningPreManifest is the integration counterpart: +// a pre-manifest containing a Namespace resource flows end-to-end through +// localformat.Write → buildHelmfile and surfaces as `createNamespace: false` +// on the pre-release in the rendered helmfile.yaml. This locks the +// os-talos failure mode the helmfile deployer originally hit (release +// creates namespace via --create-namespace, then the chart's Namespace +// template can't claim it). +func TestGenerate_NamespaceOwningPreManifest(t *testing.T) { + namespaceManifest := []byte("apiVersion: v1\n" + + "kind: Namespace\n" + + "metadata:\n" + + " name: privileged-gpu-operator\n" + + " labels:\n" + + " pod-security.kubernetes.io/enforce: privileged\n") + g := &Generator{ + RecipeResult: recipeWith( + recipe.ComponentRef{ + Name: "gpu-operator", + Namespace: "privileged-gpu-operator", + Chart: "gpu-operator", + Version: "v25.3.3", + Source: "https://helm.ngc.nvidia.com/nvidia", + Type: recipe.ComponentTypeHelm, + }, + ), + ComponentValues: map[string]map[string]any{"gpu-operator": {}}, + ComponentPreManifests: map[string]map[string][]byte{ + "gpu-operator": {"talos-namespace.yaml": namespaceManifest}, + }, + Version: testBundlerVersion, + } + outputDir := t.TempDir() + if _, err := g.Generate(context.Background(), outputDir); err != nil { + t.Fatalf("Generate() error = %v", err) + } + data, err := os.ReadFile(filepath.Join(outputDir, fileHelmfile)) + if err != nil { + t.Fatalf("read helmfile.yaml: %v", err) + } + var doc Helmfile + if err := yaml.Unmarshal(data, &doc); err != nil { + t.Fatalf("parse helmfile.yaml: %v\n--- content ---\n%s", err, data) + } + if len(doc.Releases) != 2 { + t.Fatalf("expected 2 releases (gpu-operator-pre + gpu-operator), got %d", len(doc.Releases)) + } + // release[0] is the injected -pre with the Namespace template. + if got := doc.Releases[0].Name; got != "gpu-operator-pre" { + t.Fatalf("release[0].name = %q, want %q", got, "gpu-operator-pre") + } + if doc.Releases[0].CreateNamespace == nil || *doc.Releases[0].CreateNamespace { + t.Errorf("release[0] (gpu-operator-pre) CreateNamespace = %v, want pointer to false — "+ + "pre-manifest ships a kind: Namespace, so helm must not create it out-of-band", + doc.Releases[0].CreateNamespace) + } + // release[1] is the primary upstream-helm; no override expected. + if doc.Releases[1].CreateNamespace != nil { + t.Errorf("release[1] (gpu-operator) CreateNamespace = %v, want nil", + doc.Releases[1].CreateNamespace) + } + // Global default is unchanged — every other release still benefits. + if !doc.HelmDefaults.CreateNamespace { + t.Error("helmDefaults.createNamespace should remain true; only the affected release overrides it") + } +} + // TestComponentOverrides_ParityWithHelmDeployScript guards against silent // drift between componentOverrides in this package and the hardcoded // ASYNC_COMPONENTS / COMPONENT_HELM_TIMEOUT case block in diff --git a/pkg/bundler/deployer/helmfile/releases.go b/pkg/bundler/deployer/helmfile/releases.go index b531d947d..bfc57be73 100644 --- a/pkg/bundler/deployer/helmfile/releases.go +++ b/pkg/bundler/deployer/helmfile/releases.go @@ -69,6 +69,12 @@ type Release struct { // Timeout is rendered only when the release overrides // helmDefaults.timeout (e.g., kai-scheduler 20m). Timeout int `yaml:"timeout,omitempty"` + // CreateNamespace is rendered only when explicitly set to false — + // the chart ships its own Namespace template, so helm must not + // create the namespace out-of-band. helmDefaults.createNamespace + // (true) applies otherwise. Pointer so a deliberate false survives + // YAML marshaling and a nil value omits the field entirely. + CreateNamespace *bool `yaml:"createNamespace,omitempty"` } // overrides carries per-component helm flag overrides. @@ -155,6 +161,20 @@ func buildHelmfile(folders []localformat.Folder, namespaceByComponent map[string } } + // Override the global helmDefaults.createNamespace when this + // folder's chart owns the target Namespace (Talos privileged- + // namespace pre-injection). Without this, helmfile passes + // --create-namespace and helm creates the namespace out-of-band; + // the chart's later Namespace template then collides because + // the existing namespace lacks the release's ownership labels. + // Only emit the override when false — the global default + // already covers the true case and we want helmfile.yaml + // minimal. + if !f.CreateNamespace { + falseVal := false + rel.CreateNamespace = &falseVal + } + releases = append(releases, rel) prevName = rel.Name prevNamespace = ns diff --git a/pkg/bundler/deployer/localformat/folder.go b/pkg/bundler/deployer/localformat/folder.go index 7234fb098..edc4aeed4 100644 --- a/pkg/bundler/deployer/localformat/folder.go +++ b/pkg/bundler/deployer/localformat/folder.go @@ -56,4 +56,14 @@ type Folder struct { Parent string // component this folder belongs to (== Name for primary) Upstream *Upstream // set iff Kind == KindUpstreamHelm Files []string // relative paths (to OutputDir) of files written in this folder + // CreateNamespace is true when the orchestration layer should pass + // --create-namespace to helm for this folder's release, false when + // the folder's chart ships its own Namespace resource (the Talos + // privileged-namespace pre-injection pattern). install.sh already + // honors this internally; the field exposes the same decision to + // out-of-band deployers (e.g., helmfile) that bypass install.sh. + // Helm 3 refuses to import a namespace it created out-of-band via + // --create-namespace because that namespace lacks the release's + // ownership annotations. + CreateNamespace bool } diff --git a/pkg/bundler/deployer/localformat/local_helm.go b/pkg/bundler/deployer/localformat/local_helm.go index 9dec41a9e..9eaa6f7b1 100644 --- a/pkg/bundler/deployer/localformat/local_helm.go +++ b/pkg/bundler/deployer/localformat/local_helm.go @@ -214,11 +214,12 @@ func writeLocalHelmFolder( files = append(files, filepath.Join(dir, "install.sh")) return Folder{ - Index: idx, - Dir: dir, - Kind: KindLocalHelm, - Name: name, - Parent: parent, - Files: files, + Index: idx, + Dir: dir, + Kind: KindLocalHelm, + Name: name, + Parent: parent, + Files: files, + CreateNamespace: effectiveCreateNamespace, }, nil } diff --git a/pkg/bundler/deployer/localformat/upstream_helm.go b/pkg/bundler/deployer/localformat/upstream_helm.go index 804a2445d..d5066c71f 100644 --- a/pkg/bundler/deployer/localformat/upstream_helm.go +++ b/pkg/bundler/deployer/localformat/upstream_helm.go @@ -109,5 +109,9 @@ func writeUpstreamHelmFolder(outputDir, dir string, idx int, c Component) (Folde filepath.Join(dir, "upstream.env"), filepath.Join(dir, "install.sh"), }, + // Upstream Helm folders don't ship AICR-rendered templates, so + // the chart-owns-Namespace detection in local_helm doesn't apply + // here. Default to true to match install.sh's --create-namespace. + CreateNamespace: true, }, nil } diff --git a/pkg/bundler/deployer/localformat/vendor_folder.go b/pkg/bundler/deployer/localformat/vendor_folder.go index cdb85dce0..a99167ef5 100644 --- a/pkg/bundler/deployer/localformat/vendor_folder.go +++ b/pkg/bundler/deployer/localformat/vendor_folder.go @@ -185,6 +185,10 @@ func writeVendoredHelmFolder( Name: c.Name, Parent: c.Name, Files: files, + // Vendored folders wrap an upstream chart that AICR does not + // render; the chart-owns-Namespace detection does not apply + // here. Default to true to match the upstream-helm path. + CreateNamespace: true, }, rec, nil }