Merge 9f84dea13d into 62c7e845d1

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)

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))
+}

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

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