From 9f84dea13d31dd56c533b379309263e43b7d931f Mon Sep 17 00:00:00 2001 From: Rob White Date: Tue, 1 Sep 2026 14:31:50 +0100 Subject: [PATCH] fix(action): drop the CRD file's own YAML document separator Helm prefixes every rendered file with a "---" separator and a "# Source:" comment. A CRD file that already starts with a separator was therefore emitted as an empty document holding only the comment, followed by the real manifest, which breaks consumers of the rendered output. Trim a leading separator from the CRD contents so the comment stays attached to the manifest it describes. Blank lines and comments ahead of the separator are preserved, and only a line consisting solely of "---" counts, so indented markers, "----", and separators terminating a YAML directive are left alone. Closes #12953 Signed-off-by: Rob White --- pkg/action/action.go | 39 ++++- pkg/action/action_test.go | 152 ++++++++++++++++++ .../testdata/output/template-with-crds.txt | 15 ++ .../testcharts/subchart/crds/crdB.yaml | 13 ++ 4 files changed, 217 insertions(+), 2 deletions(-) create mode 100644 pkg/cmd/testdata/testcharts/subchart/crds/crdB.yaml diff --git a/pkg/action/action.go b/pkg/action/action.go index e93d6181f..ad08dc39e 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -483,10 +483,11 @@ func (cfg *Configuration) renderResources(ctx context.Context, ch *chart.Chart, if includeCrds { for _, crd := range ch.CRDObjects() { + crdData := trimLeadingDocumentSeparator(string(crd.File.Data)) if outputDir == "" { - fmt.Fprintf(b, "---\n# Source: %s\n%s\n", crd.Filename, string(crd.File.Data)) + fmt.Fprintf(b, "---\n# Source: %s\n%s\n", crd.Filename, crdData) } else { - err = writeToFile(outputDir, crd.Filename, string(crd.File.Data), fileWritten[crd.Filename]) + err = writeToFile(outputDir, crd.Filename, crdData, fileWritten[crd.Filename]) if err != nil { return hs, b, "", err } @@ -522,6 +523,40 @@ func (cfg *Configuration) renderResources(ctx context.Context, ch *chart.Chart, return hs, b, notes, nil } +// trimLeadingDocumentSeparator removes a leading YAML document separator from +// the raw contents of a chart file. +// +// Helm writes its own "---" separator and "# Source:" comment ahead of every +// file it renders. A file that already starts with a separator is therefore +// preceded by a document holding nothing but the comment, and consumers of the +// rendered output see an extra, empty document. Removing the separator the file +// supplies keeps the comment attached to the manifest it describes. +// +// Blank lines and comments ahead of the separator are preserved. A line only +// counts as a separator when it consists solely of "---", so indented markers, +// "----", and separators that follow content or a YAML directive are left +// alone. +func trimLeadingDocumentSeparator(data string) string { + for offset := 0; offset < len(data); { + line, next := data[offset:], len(data) + if i := strings.IndexByte(line, '\n'); i >= 0 { + line, next = line[:i], offset+i+1 + } + + switch content := strings.TrimRight(line, " \t\r"); { + case content == "---": + return data[:offset] + data[next:] + case content == "" || strings.HasPrefix(strings.TrimLeft(content, " \t"), "#"): + // A separator may still follow a blank line or a comment. + offset = next + default: + return data + } + } + + return data +} + // RESTClientGetter gets the rest client type RESTClientGetter interface { ToRESTConfig() (*rest.Config, error) diff --git a/pkg/action/action_test.go b/pkg/action/action_test.go index 056c539a5..864200b60 100644 --- a/pkg/action/action_test.go +++ b/pkg/action/action_test.go @@ -22,6 +22,8 @@ import ( "fmt" "io" "log/slog" + "os" + "path/filepath" "strings" "testing" "time" @@ -2292,3 +2294,153 @@ func TestInteractWithServer(t *testing.T) { assert.False(t, interactWithServer(DryRunClient)) assert.True(t, interactWithServer(DryRunServer)) } + +func TestTrimLeadingDocumentSeparator(t *testing.T) { + for _, tc := range []struct { + name string + data string + expected string + }{ + { + name: "leading separator is removed", + data: "---\napiVersion: v1\n", + expected: "apiVersion: v1\n", + }, + { + name: "content without a separator is left alone", + data: "apiVersion: v1\n", + expected: "apiVersion: v1\n", + }, + { + name: "only the leading separator is removed", + data: "---\napiVersion: v1\n---\napiVersion: v2\n", + expected: "apiVersion: v1\n---\napiVersion: v2\n", + }, + { + name: "separator following content is left alone", + data: "apiVersion: v1\n---\napiVersion: v2\n", + expected: "apiVersion: v1\n---\napiVersion: v2\n", + }, + { + name: "comments and blank lines preceding the separator are kept", + data: "# a comment\n\n---\napiVersion: v1\n", + expected: "# a comment\n\napiVersion: v1\n", + }, + { + name: "trailing whitespace on the separator is tolerated", + data: "--- \napiVersion: v1\n", + expected: "apiVersion: v1\n", + }, + { + name: "carriage returns are tolerated", + data: "---\r\napiVersion: v1\r\n", + expected: "apiVersion: v1\r\n", + }, + { + name: "separator without a trailing newline", + data: "---", + expected: "", + }, + { + name: "indented separator is not a document separator", + data: " ---\napiVersion: v1\n", + expected: " ---\napiVersion: v1\n", + }, + { + name: "four dashes are not a document separator", + data: "----\napiVersion: v1\n", + expected: "----\napiVersion: v1\n", + }, + { + name: "separator terminating a YAML directive is left alone", + data: "%YAML 1.2\n---\napiVersion: v1\n", + expected: "%YAML 1.2\n---\napiVersion: v1\n", + }, + { + name: "empty data", + data: "", + expected: "", + }, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.expected, trimLeadingDocumentSeparator(tc.data)) + }) + } +} + +const crdManifest = `apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: testcrds.testcrdgroups.example.com +` + +// Helm prefixes every rendered file with its own document separator, so a CRD +// file that already carries one must not produce a second, empty document. +// See https://github.com/helm/helm/issues/12953. +func TestRenderResources_IncludeCRDs(t *testing.T) { + for _, tc := range []struct { + name string + crdData string + expected string + }{ + { + name: "CRD without a document separator", + crdData: crdManifest, + expected: "---\n# Source: hello/crds/crd.yaml\n" + crdManifest + "\n", + }, + { + name: "CRD with a leading document separator", + crdData: "---\n" + crdManifest, + expected: "---\n# Source: hello/crds/crd.yaml\n" + crdManifest + "\n", + }, + { + name: "CRD with a comment before the document separator", + crdData: "# generated by controller-gen\n---\n" + crdManifest, + expected: "---\n# Source: hello/crds/crd.yaml\n# generated by controller-gen\n" + crdManifest + "\n", + }, + { + name: "CRD holding several documents", + crdData: "---\n" + crdManifest + "---\n" + crdManifest, + expected: "---\n# Source: hello/crds/crd.yaml\n" + crdManifest + "---\n" + crdManifest + "\n", + }, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := actionConfigFixture(t) + ch := buildChartWithTemplates(nil, withFile(common.File{ + Name: "crds/crd.yaml", + ModTime: time.Now(), + Data: []byte(tc.crdData), + })) + + _, buf, _, err := cfg.renderResources( + t.Context(), ch, map[string]any{}, "test-release", "", false, false, true, + nil, false, false, false, PostRenderStrategyCombined, + ) + + require.NoError(t, err) + assert.Equal(t, tc.expected, buf.String()) + }) + } +} + +// The --output-dir path writes CRDs through writeToFile, which prefixes each +// file with a document separator of its own. +func TestRenderResources_IncludeCRDs_OutputDir(t *testing.T) { + cfg := actionConfigFixture(t) + ch := buildChartWithTemplates(nil, withFile(common.File{ + Name: "crds/crd.yaml", + ModTime: time.Now(), + Data: []byte("---\n" + crdManifest), + })) + + outputDir := t.TempDir() + _, _, _, err := cfg.renderResources( + t.Context(), ch, map[string]any{}, "test-release", outputDir, false, false, true, + nil, false, false, false, PostRenderStrategyCombined, + ) + require.NoError(t, err) + + written, err := os.ReadFile(filepath.Join(outputDir, "hello", "crds", "crd.yaml")) + require.NoError(t, err) + assert.Equal(t, "---\n# Source: hello/crds/crd.yaml\n"+crdManifest+"\n", string(written)) +} diff --git a/pkg/cmd/testdata/output/template-with-crds.txt b/pkg/cmd/testdata/output/template-with-crds.txt index 1d63265ec..0cac28e00 100644 --- a/pkg/cmd/testdata/output/template-with-crds.txt +++ b/pkg/cmd/testdata/output/template-with-crds.txt @@ -15,6 +15,21 @@ spec: - tc singular: authconfig +--- +# Source: subchart/crds/crdB.yaml +apiVersion: apiextensions.k8s.io/v1beta1 +kind: CustomResourceDefinition +metadata: + name: testcrdbs.testcrdgroups.example.com +spec: + group: testcrdgroups.example.com + version: v1alpha1 + names: + kind: TestCRDB + listKind: TestCRDBList + plural: testcrdbs + singular: testcrdb + --- # Source: subchart/templates/subdir/serviceaccount.yaml apiVersion: v1 diff --git a/pkg/cmd/testdata/testcharts/subchart/crds/crdB.yaml b/pkg/cmd/testdata/testcharts/subchart/crds/crdB.yaml new file mode 100644 index 000000000..793108fef --- /dev/null +++ b/pkg/cmd/testdata/testcharts/subchart/crds/crdB.yaml @@ -0,0 +1,13 @@ +--- +apiVersion: apiextensions.k8s.io/v1beta1 +kind: CustomResourceDefinition +metadata: + name: testcrdbs.testcrdgroups.example.com +spec: + group: testcrdgroups.example.com + version: v1alpha1 + names: + kind: TestCRDB + listKind: TestCRDBList + plural: testcrdbs + singular: testcrdb