Merge 2d1b37a3bd into 62c7e845d1

internal/release/v2/util/manifest.go

@@ -34,18 +34,37 @@ type SimpleHead struct {
 	} `json:"metadata,omitempty"`
 }
 
-var sep = regexp.MustCompile(`(?m)^---[ \t]*`)
+// sep matches YAML document separators. A separator is `---` at the start of
+// a line (or start of the stream), followed by horizontal whitespace or by a
+// newline/end of stream. The whitespace branch allows same-line comments or
+// content while leaving their suffix in the following manifest. The multiline
+// anchor leaves the preceding document's trailing whitespace and newlines
+// intact.
+//
+// This is intentionally stricter than the Chart v1/v2 regex in
+// pkg/release/v1/util/manifest.go. The v1/v2 version tolerates
+// `---<non-whitespace>` (e.g. `---apiVersion: v1`) by treating it as a
+// separator glued to content, a silent correction for Go template whitespace
+// trimming (`{{-`) eating the newline after `---`. Chart v3 does not carry
+// that workaround forward; see the function comment below.
+var sep = regexp.MustCompile(`(?m)^---(?:[ \t]+|\r?$)`)
 
 // SplitManifests takes a manifest string and returns a map containing individual manifests.
 //
-// **Note for Chart API v3**: This function (due to the regex above) has allowed _WRONG_
+// Chart API v3 note: unlike Chart v1/v2, this implementation does NOT silently
-// Go templates to be defined inside charts across the years. The generated text from Go
+// repair YAML document separators glued to content by Go template whitespace
-// templates may contain `---apiVersion: v1`, and this function magically splits this back
+// trimming. A template such as
-// to `---\napiVersion: v1`. This has caused issues recently after Helm 4 introduced
+//
-// kio.ParseAll to inject annotations when post-renderers are used. In Chart API v3,
+//	---
-// we should kill this regex with fire (or change it) and expose charts doing the wrong
+//	{{- include "mychart.service" . }}
-// thing Go template-wise. Helm should say a big _NO_ to charts doing the wrong thing,
+//
-// with or without post-renderers.
+// renders `---apiVersion: v1\n...` because `{{-` strips the newline after
+// `---`. In Chart v1/v2, SplitManifests detects this and splits the input as
+// if the newline were still there; in Chart v3, the glued `---` is left as
+// part of the document body rather than being silently repaired. Downstream
+// validation can then reject the malformed manifest. Chart authors should
+// drop the dash (`{{ include ... }}`) or omit the explicit `---` separator;
+// Helm inserts one between templates on its own. See helm/helm#32036.
 func SplitManifests(bigFile string) map[string]string {
 	// Basically, we're quickly splitting a stream of YAML documents into an
 	// array of YAML docs. The file name is just a place holder, but should be

internal/release/v2/util/manifest_test.go

@@ -28,6 +28,37 @@ func TestSplitManifests(t *testing.T) {
 		input    string
 		expected map[string]string
 	}{
+		{
+			name:  "commented separator after existing document (LF)",
+			input: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n  name: cm1\n--- # next document\napiVersion: v1\nkind: Service\nmetadata:\n  name: svc\n",
+			expected: map[string]string{
+				"manifest-0": "apiVersion: v1\nkind: ConfigMap\nmetadata:\n  name: cm1\n",
+				"manifest-1": "# next document\napiVersion: v1\nkind: Service\nmetadata:\n  name: svc\n",
+			},
+		},
+		{
+			name:  "commented separator after existing document (CRLF)",
+			input: "apiVersion: v1\r\nkind: ConfigMap\r\nmetadata:\r\n  name: cm1\r\n--- # next document\r\napiVersion: v1\r\nkind: Service\r\nmetadata:\r\n  name: svc\r\n",
+			expected: map[string]string{
+				"manifest-0": "apiVersion: v1\r\nkind: ConfigMap\r\nmetadata:\r\n  name: cm1\r\n",
+				"manifest-1": "# next document\r\napiVersion: v1\r\nkind: Service\r\nmetadata:\r\n  name: svc\r\n",
+			},
+		},
+		{
+			name:  "whitespace-separated same-line content is a document boundary",
+			input: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n  name: cm1\n--- apiVersion: v1\nkind: Service\nmetadata:\n  name: svc\n",
+			expected: map[string]string{
+				"manifest-0": "apiVersion: v1\nkind: ConfigMap\nmetadata:\n  name: cm1\n",
+				"manifest-1": "apiVersion: v1\nkind: Service\nmetadata:\n  name: svc\n",
+			},
+		},
+		{
+			name:  "no-whitespace marker remains attached",
+			input: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n  name: cm1\n---#comment\napiVersion: v1\nkind: Service\nmetadata:\n  name: svc\n",
+			expected: map[string]string{
+				"manifest-0": "apiVersion: v1\nkind: ConfigMap\nmetadata:\n  name: cm1\n---#comment\napiVersion: v1\nkind: Service\nmetadata:\n  name: svc\n",
+			},
+		},
 		{
 			name: "single doc with leading separator and whitespace",
 			input: `
@@ -401,12 +432,13 @@ metadata:
 			},
 		},
 
-		// **Note for Chart API v3**: The following tests exercise the lenient
+		// Chart API v3 behaviour: separators glued to content (as produced by
-		// regex that splits `---apiVersion` back into separate documents.
+		// `{{-` trimming the newline after `---`) are NOT split apart. The
-		// In Chart API v3, these inputs should return an _ERROR_ instead.
+		// glued `---` stays on the document body so downstream YAML parsing
-		// See the comment on the SplitManifests function for more details.
+		// can surface the problem instead of Helm silently correcting it.
+		// See helm/helm#32036 and the SplitManifests doc comment.
 		{
-			name: "leading glued separator (---apiVersion)",
+			name: "leading glued separator stays with content",
 			input: `
 ---apiVersion: v1
 kind: ConfigMap
@@ -414,7 +446,7 @@ metadata:
   name: cm1
 `,
 			expected: map[string]string{
-				"manifest-0": `apiVersion: v1
+				"manifest-0": `---apiVersion: v1
 kind: ConfigMap
 metadata:
   name: cm1
@@ -422,7 +454,7 @@ metadata:
 			},
 		},
 		{
-			name: "mid-content glued separator (---apiVersion)",
+			name: "mid-content glued separator stays with first doc",
 			input: `
 apiVersion: v1
 kind: ConfigMap
@@ -438,8 +470,7 @@ metadata:
 kind: ConfigMap
 metadata:
   name: cm1
-`,
+---apiVersion: v1
-				"manifest-1": `apiVersion: v1
 kind: ConfigMap
 metadata:
   name: cm2
@@ -447,7 +478,7 @@ metadata:
 			},
 		},
 		{
-			name: "multiple glued separators",
+			name: "multiple glued separators produce a single doc",
 			input: `
 ---apiVersion: v1
 kind: ConfigMap
@@ -463,17 +494,15 @@ metadata:
   name: cm3
 `,
 			expected: map[string]string{
-				"manifest-0": `apiVersion: v1
+				"manifest-0": `---apiVersion: v1
 kind: ConfigMap
 metadata:
   name: cm1
-`,
+---apiVersion: v1
-				"manifest-1": `apiVersion: v1
 kind: ConfigMap
 metadata:
   name: cm2
-`,
+---apiVersion: v1
-				"manifest-2": `apiVersion: v1
 kind: ConfigMap
 metadata:
   name: cm3
@@ -481,7 +510,7 @@ metadata:
 			},
 		},
 		{
-			name: "mixed glued and proper separators",
+			name: "proper separators split, glued ones do not",
 			input: `
 apiVersion: v1
 kind: ConfigMap
@@ -507,14 +536,50 @@ metadata:
 kind: ConfigMap
 metadata:
   name: cm2
-`,
+---apiVersion: v1
-				"manifest-2": `apiVersion: v1
 kind: ConfigMap
 metadata:
   name: cm3
 `,
 			},
 		},
+		{
+			name:  "block scalar trailing whitespace survives a following separator",
+			input: "apiVersion: v1\nkind: ConfigMap\ndata:\n  value: |+\n    hello 	\n\n---\napiVersion: v1\nkind: Service\n",
+			expected: map[string]string{
+				"manifest-0": "apiVersion: v1\nkind: ConfigMap\ndata:\n  value: |+\n    hello 	\n\n",
+				"manifest-1": "apiVersion: v1\nkind: Service\n",
+			},
+		},
+		{
+			name:  "CRLF document endings survive a following separator",
+			input: "apiVersion: v1\r\nkind: ConfigMap\r\n\r\n---	 \r\napiVersion: v1\r\nkind: Service\r\n",
+			expected: map[string]string{
+				"manifest-0": "apiVersion: v1\r\nkind: ConfigMap\r\n\r\n",
+				"manifest-1": "apiVersion: v1\r\nkind: Service\r\n",
+			},
+		},
+		{
+			name:  "trailing separator with no newline is still a separator",
+			input: "apiVersion: v1\nkind: ConfigMap\nmetadata:\n  name: cm1\n---",
+			expected: map[string]string{
+				"manifest-0": "apiVersion: v1\nkind: ConfigMap\nmetadata:\n  name: cm1\n",
+			},
+		},
+		{
+			name:  "separator with trailing spaces and tabs is still a separator",
+			input: "---\t \napiVersion: v1\nkind: ConfigMap\nmetadata:\n  name: cm1\n",
+			expected: map[string]string{
+				"manifest-0": "apiVersion: v1\nkind: ConfigMap\nmetadata:\n  name: cm1\n",
+			},
+		},
+		{
+			name:  "CRLF line endings still split",
+			input: "---\r\napiVersion: v1\r\nkind: ConfigMap\r\nmetadata:\r\n  name: cm1\r\n",
+			expected: map[string]string{
+				"manifest-0": "apiVersion: v1\r\nkind: ConfigMap\r\nmetadata:\r\n  name: cm1\r\n",
+			},
+		},
 	}
 
 	for _, tt := range tests {