diff --git a/pkg/bundler/deployer/argocdhelm/argocdhelm.go b/pkg/bundler/deployer/argocdhelm/argocdhelm.go index a28a2451b..c442addc7 100644 --- a/pkg/bundler/deployer/argocdhelm/argocdhelm.go +++ b/pkg/bundler/deployer/argocdhelm/argocdhelm.go @@ -696,11 +696,23 @@ func injectValuesIntoSingleSource(app map[string]any, overrideKey string) error // in single quotes — that's the only YAML scalar style that doesn't // require escaping the embedded double quotes inside `required "..."`, // which would corrupt Helm's parsing of the template. + // + // Path-based child Applications have no `chart` field — Argo CD's + // generic OCI source uses `repoURL` directly as the full artifact + // reference and then resolves `path` inside it. We therefore append + // .Chart.Name here ourselves so the rendered value is + // `/` (e.g. `oci://reg/org/my-bundle`), + // matching the artifact the parent Application's `repoURL/chart:tag` + // triple resolves to. Without the append, --set repoURL= (the contract every other site in this file documents) + // produces a child source pointing at `oci://reg/org:tag` — an + // artifact that does not exist — and the child Application fails + // to sync. See issue #1034. source["repoURL"] = &yaml.Node{ Kind: yaml.ScalarNode, Tag: yamlStringTag, Style: yaml.SingleQuotedStyle, - Value: `{{ required "repoURL is required: pass --set repoURL= (e.g., oci:///) — do NOT include the chart name; the parent App appends .Chart.Name to assemble the full OCI artifact reference" .Values.repoURL }}`, + Value: `{{ required "repoURL is required: pass --set repoURL= (e.g., oci:///) — do NOT include the chart name; this template appends .Chart.Name to assemble the full OCI artifact reference" .Values.repoURL }}/{{ .Chart.Name }}`, } source["targetRevision"] = &yaml.Node{ Kind: yaml.ScalarNode, @@ -939,12 +951,20 @@ func writeChartYAML(outputDir, name, version string) (string, int64, error) { return "", 0, err } + // Quote name and version so OCI artifact paths whose last segment is + // a YAML reserved scalar ("null", "true", "false", "yes", "no", + // "123", etc.) round-trip as strings instead of getting reinterpreted + // by Helm's YAML parser as the underlying scalar type and producing + // a chart with an empty Metadata.Name (or a type-mismatch error). + // fmt %q emits a Go-quoted string that is also valid YAML for all + // printable ASCII; OCI artifact path segments are constrained to + // that charset by the docker reference grammar. See issue #1034. var buf strings.Builder buf.WriteString("apiVersion: v2\n") - fmt.Fprintf(&buf, "name: %s\n", name) + fmt.Fprintf(&buf, "name: %q\n", name) buf.WriteString("description: AICR deployment bundle with dynamic install-time values\n") buf.WriteString("type: application\n") - fmt.Fprintf(&buf, "version: %s\n", version) + fmt.Fprintf(&buf, "version: %q\n", version) content := buf.String() if writeErr := os.WriteFile(chartPath, []byte(content), 0600); writeErr != nil { diff --git a/pkg/bundler/deployer/argocdhelm/argocdhelm_test.go b/pkg/bundler/deployer/argocdhelm/argocdhelm_test.go index 161bfeb85..5bdbb16fe 100644 --- a/pkg/bundler/deployer/argocdhelm/argocdhelm_test.go +++ b/pkg/bundler/deployer/argocdhelm/argocdhelm_test.go @@ -199,8 +199,11 @@ func TestGenerate(t *testing.T) { if err != nil { t.Fatalf("failed to read Chart.yaml: %v", err) } - if !strings.Contains(string(content), "version: 2.5.0") { - t.Error("Chart.yaml should contain version: 2.5.0") + // writeChartYAML quotes the version scalar so YAML + // reserved scalars (e.g. "1.0", "null") round-trip as + // strings; see issue #1034. + if !strings.Contains(string(content), `version: "2.5.0"`) { + t.Errorf("Chart.yaml should contain version: \"2.5.0\", got:\n%s", string(content)) } }, }, @@ -1044,12 +1047,19 @@ func TestHelmTemplate_RendersWithSetRepoURL(t *testing.T) { t.Fatalf("Generate() error = %v", err) } - const wantRepoURL = "oci://example.test/myorg/aicr-bundle" + // Under the post-#1032 contract, --set repoURL carries the parent + // namespace only — the parent App appends .Chart.Name via its + // source.chart field, and (per #1034) the path-based child template + // appends / directly into its source.repoURL so Argo + // CD's generic OCI source resolves to the same artifact. + const setRepoURL = "oci://example.test/myorg" + const wantParentRepoURL = setRepoURL + const wantChildRepoURL = setRepoURL + "/" + DefaultChartName const wantTagName = "v9.9.9-render-test" cmdCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() cmd := exec.CommandContext(cmdCtx, "helm", "template", "test-release", outputDir, //nolint:gosec // controlled args - "--set", "repoURL="+wantRepoURL, + "--set", "repoURL="+setRepoURL, "--set", "targetRevision="+wantTagName, ) out, err := cmd.CombinedOutput() @@ -1090,8 +1100,8 @@ func TestHelmTemplate_RendersWithSetRepoURL(t *testing.T) { if !ok { t.Fatalf("rendered output missing parent Application 'aicr-stack'\noutput:\n%s", out) } - if parent.Spec.Source.RepoURL != wantRepoURL { - t.Errorf("parent App repoURL: got %q, want %q", parent.Spec.Source.RepoURL, wantRepoURL) + if parent.Spec.Source.RepoURL != wantParentRepoURL { + t.Errorf("parent App repoURL: got %q, want %q", parent.Spec.Source.RepoURL, wantParentRepoURL) } if parent.Spec.Source.Chart != DefaultChartName { t.Errorf("parent App chart: got %q, want %q", parent.Spec.Source.Chart, DefaultChartName) @@ -1104,8 +1114,8 @@ func TestHelmTemplate_RendersWithSetRepoURL(t *testing.T) { if !ok { t.Fatalf("rendered output missing path-based child 'nodewright-customizations'") } - if child.Spec.Source.RepoURL != wantRepoURL { - t.Errorf("child path-based repoURL: got %q, want %q", child.Spec.Source.RepoURL, wantRepoURL) + if child.Spec.Source.RepoURL != wantChildRepoURL { + t.Errorf("child path-based repoURL: got %q, want %q (parent namespace + chart name; see #1034)", child.Spec.Source.RepoURL, wantChildRepoURL) } if child.Spec.Source.TargetRevision != wantTagName { t.Errorf("child path-based targetRevision: got %q, want %q", child.Spec.Source.TargetRevision, wantTagName) @@ -1163,8 +1173,10 @@ func TestGenerate_CustomChartName(t *testing.T) { if err != nil { t.Fatalf("read Chart.yaml: %v", err) } - if !strings.Contains(string(chartBytes), "name: "+customName+"\n") { - t.Errorf("Chart.yaml missing custom name %q; got:\n%s", customName, chartBytes) + // writeChartYAML quotes the name so OCI artifact paths whose last + // segment is a YAML reserved scalar round-trip as strings; see #1034. + if !strings.Contains(string(chartBytes), `name: "`+customName+"\"\n") { + t.Errorf("Chart.yaml missing quoted custom name %q; got:\n%s", customName, chartBytes) } cmdCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) @@ -1272,3 +1284,100 @@ func assertGolden(t *testing.T, outDir, goldenDir, relPath string) { t.Errorf("%s differs from golden:\n--- got ---\n%s\n--- want ---\n%s", relPath, got, want) } } + +// TestInjectValuesIntoSingleSource_AppendsChartName pins the +// post-#1032 path-based-children fix from issue #1034: path-based +// child Applications have no `chart` field, so Argo CD's generic OCI +// source uses `repoURL` directly as the full artifact reference. The +// rendered template must therefore append .Chart.Name to the +// install-time --set repoURL value so the assembled URL matches the +// artifact the parent Application's `repoURL/chart:tag` triple +// resolves to. Without the append, --set repoURL=oci://reg/org (the +// contract documented elsewhere in this file) produces a child +// source pointing at `oci://reg/org:tag` — an artifact that does not +// exist — and the child Application fails to sync. +func TestInjectValuesIntoSingleSource_AppendsChartName(t *testing.T) { + app := map[string]any{ + "apiVersion": "argoproj.io/v1alpha1", + "kind": "Application", + "spec": map[string]any{ + "source": map[string]any{ + "repoURL": "https://github.com/myorg/myrepo.git", + "targetRevision": "main", + "path": "003-nodewright-customizations", + }, + }, + } + + if err := injectValuesIntoSingleSource(app, "nodewrightcustomizations"); err != nil { + t.Fatalf("injectValuesIntoSingleSource error: %v", err) + } + + out, err := yaml.Marshal(app) + if err != nil { + t.Fatalf("yaml.Marshal error: %v", err) + } + str := string(out) + + if !strings.Contains(str, `.Values.repoURL }}/{{ .Chart.Name }}`) { + t.Errorf("path-based child repoURL should append /{{ .Chart.Name }} after .Values.repoURL so the rendered value is the full OCI artifact reference; got:\n%s", str) + } + // The error message must direct callers to pass the parent + // namespace (the same contract the parent Application uses) so + // users don't try to bake the chart name into --set repoURL. + if !strings.Contains(str, "do NOT include the chart name") { + t.Errorf("required-message must instruct callers not to include the chart name in --set repoURL; got:\n%s", str) + } +} + +// TestWriteChartYAML_QuotesYAMLReservedScalarsAsName documents the +// chart-name quoting fix from issue #1034. Valid OCI artifact paths +// whose last segment is a YAML reserved scalar ("null", "true", +// "false", numeric strings, etc.) must round-trip as strings; if +// emitted unquoted, Helm's YAML parser reinterprets `name: null` as +// YAML null, chart.Metadata.Name becomes empty, and the chart is +// rejected by `helm package` / `helm push` with "chart.metadata.name +// is required". +func TestWriteChartYAML_QuotesYAMLReservedScalarsAsName(t *testing.T) { + tests := []struct { + name string + chartName string + wantUnmarsh string + }{ + {"YAML null literal", "null", "null"}, + {"YAML true literal", "true", "true"}, + {"YAML false literal", "false", "false"}, + {"numeric-looking", "123", "123"}, + {"YAML yes literal", "yes", "yes"}, + {"hyphenated normal name", "my-bundle", "my-bundle"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + outputDir := t.TempDir() + if _, _, err := writeChartYAML(outputDir, tt.chartName, "1.0.0"); err != nil { + t.Fatalf("writeChartYAML: %v", err) + } + content, err := os.ReadFile(filepath.Join(outputDir, "Chart.yaml")) + if err != nil { + t.Fatalf("read Chart.yaml: %v", err) + } + var parsed struct { + APIVersion string `yaml:"apiVersion"` + Name string `yaml:"name"` + Version string `yaml:"version"` + } + if err := yaml.Unmarshal(content, &parsed); err != nil { + t.Fatalf("Chart.yaml does not unmarshal cleanly for name=%q:\n%s\nerror: %v", + tt.chartName, content, err) + } + if parsed.Name != tt.wantUnmarsh { + t.Errorf("Chart.yaml name = %q (after YAML unmarshal), want %q\nraw:\n%s", + parsed.Name, tt.wantUnmarsh, content) + } + if parsed.Version != "1.0.0" { + t.Errorf("Chart.yaml version = %q, want \"1.0.0\"\nraw:\n%s", + parsed.Version, content) + } + }) + } +} diff --git a/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/Chart.yaml b/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/Chart.yaml index 01df27e33..26edba3a1 100644 --- a/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/Chart.yaml +++ b/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/Chart.yaml @@ -1,5 +1,5 @@ apiVersion: v2 -name: aicr-bundle +name: "aicr-bundle" description: AICR deployment bundle with dynamic install-time values type: application -version: 1.0.0 +version: "1.0.0" diff --git a/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yaml b/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yaml index 27f8e2408..abe17b2a1 100644 --- a/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yaml +++ b/pkg/bundler/deployer/argocdhelm/testdata/helm_and_manifest_only/templates/nodewright-customizations.yaml @@ -16,7 +16,7 @@ spec: {{- $dynamic := index .Values "nodewrightcustomizations" | default dict -}} {{- toYaml $dynamic | nindent 16 }} path: 002-nodewright-customizations - repoURL: '{{ required "repoURL is required: pass --set repoURL= (e.g., oci:///) — do NOT include the chart name; the parent App appends .Chart.Name to assemble the full OCI artifact reference" .Values.repoURL }}' + repoURL: '{{ required "repoURL is required: pass --set repoURL= (e.g., oci:///) — do NOT include the chart name; this template appends .Chart.Name to assemble the full OCI artifact reference" .Values.repoURL }}/{{ .Chart.Name }}' targetRevision: '{{ .Values.targetRevision | default .Chart.Version }}' syncPolicy: automated: diff --git a/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/Chart.yaml b/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/Chart.yaml index 01df27e33..26edba3a1 100644 --- a/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/Chart.yaml +++ b/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/Chart.yaml @@ -1,5 +1,5 @@ apiVersion: v2 -name: aicr-bundle +name: "aicr-bundle" description: AICR deployment bundle with dynamic install-time values type: application -version: 1.0.0 +version: "1.0.0" diff --git a/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yaml b/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yaml index f6379756a..3f52f0ce4 100644 --- a/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yaml +++ b/pkg/bundler/deployer/argocdhelm/testdata/mixed_component/templates/gpu-operator-post.yaml @@ -16,7 +16,7 @@ spec: {{- $dynamic := index .Values "gpuoperator" | default dict -}} {{- toYaml $dynamic | nindent 16 }} path: 002-gpu-operator-post - repoURL: '{{ required "repoURL is required: pass --set repoURL= (e.g., oci:///) — do NOT include the chart name; the parent App appends .Chart.Name to assemble the full OCI artifact reference" .Values.repoURL }}' + repoURL: '{{ required "repoURL is required: pass --set repoURL= (e.g., oci:///) — do NOT include the chart name; this template appends .Chart.Name to assemble the full OCI artifact reference" .Values.repoURL }}/{{ .Chart.Name }}' targetRevision: '{{ .Values.targetRevision | default .Chart.Version }}' syncPolicy: automated: