Merge pull request #13154 from carloslima/post-render-everything

Allow post-renderer to process hooks

go.mod

@@ -45,6 +45,7 @@ require (
 	k8s.io/kubectl v0.33.2
 	oras.land/oras-go/v2 v2.6.0
 	sigs.k8s.io/controller-runtime v0.21.0
+	sigs.k8s.io/kustomize/kyaml v0.19.0
 	sigs.k8s.io/yaml v1.5.0
 )
 
@@ -175,7 +176,6 @@ require (
 	k8s.io/utils v0.0.0-20250321185631-1f6e0b77f77e // indirect
 	sigs.k8s.io/json v0.0.0-20241014173422-cfa47c3a1cc8 // indirect
 	sigs.k8s.io/kustomize/api v0.19.0 // indirect
-	sigs.k8s.io/kustomize/kyaml v0.19.0 // indirect
 	sigs.k8s.io/randfill v1.0.0 // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.6.0 // indirect
 )

pkg/action/action.go

@@ -22,9 +22,11 @@ import (
 	"fmt"
 	"io"
 	"log/slog"
+	"maps"
 	"os"
 	"path"
 	"path/filepath"
+	"slices"
 	"strings"
 	"sync"
 	"text/template"
@@ -34,6 +36,8 @@ import (
 	"k8s.io/client-go/discovery"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/rest"
+	"sigs.k8s.io/kustomize/kyaml/kio"
+	kyaml "sigs.k8s.io/kustomize/kyaml/yaml"
 
 	chart "helm.sh/helm/v4/pkg/chart/v2"
 	chartutil "helm.sh/helm/v4/pkg/chart/v2/util"
@@ -91,6 +95,81 @@ type Configuration struct {
 	mutex sync.Mutex
 }
 
+const (
+	// filenameAnnotation is the annotation key used to store the original filename
+	// information in manifest annotations for post-rendering reconstruction.
+	filenameAnnotation = "postrenderer.helm.sh/postrender-filename"
+)
+
+// annotateAndMerge combines multiple YAML files into a single stream of documents,
+// adding filename annotations to each document for later reconstruction.
+func annotateAndMerge(files map[string]string) (string, error) {
+	var combinedManifests []*kyaml.RNode
+
+	// Get sorted filenames to ensure result is deterministic
+	fnames := slices.Sorted(maps.Keys(files))
+
+	for _, fname := range fnames {
+		content := files[fname]
+		// Skip partials and empty files.
+		if strings.HasPrefix(path.Base(fname), "_") || strings.TrimSpace(content) == "" {
+			continue
+		}
+
+		manifests, err := kio.ParseAll(content)
+		if err != nil {
+			return "", fmt.Errorf("parsing %s: %w", fname, err)
+		}
+		for _, manifest := range manifests {
+			if err := manifest.PipeE(kyaml.SetAnnotation(filenameAnnotation, fname)); err != nil {
+				return "", fmt.Errorf("annotating %s: %w", fname, err)
+			}
+			combinedManifests = append(combinedManifests, manifest)
+		}
+	}
+
+	merged, err := kio.StringAll(combinedManifests)
+	if err != nil {
+		return "", fmt.Errorf("writing merged docs: %w", err)
+	}
+	return merged, nil
+}
+
+// splitAndDeannotate reconstructs individual files from a merged YAML stream,
+// removing filename annotations and grouping documents by their original filenames.
+func splitAndDeannotate(postrendered string) (map[string]string, error) {
+	manifests, err := kio.ParseAll(postrendered)
+	if err != nil {
+		return nil, fmt.Errorf("error parsing YAML: %w", err)
+	}
+
+	manifestsByFilename := make(map[string][]*kyaml.RNode)
+	for i, manifest := range manifests {
+		meta, err := manifest.GetMeta()
+		if err != nil {
+			return nil, fmt.Errorf("getting metadata: %w", err)
+		}
+		fname := meta.Annotations[filenameAnnotation]
+		if fname == "" {
+			fname = fmt.Sprintf("generated-by-postrender-%d.yaml", i)
+		}
+		if err := manifest.PipeE(kyaml.ClearAnnotation(filenameAnnotation)); err != nil {
+			return nil, fmt.Errorf("clearing filename annotation: %w", err)
+		}
+		manifestsByFilename[fname] = append(manifestsByFilename[fname], manifest)
+	}
+
+	reconstructed := make(map[string]string, len(manifestsByFilename))
+	for fname, docs := range manifestsByFilename {
+		fileContents, err := kio.StringAll(docs)
+		if err != nil {
+			return nil, fmt.Errorf("re-writing %s: %w", fname, err)
+		}
+		reconstructed[fname] = fileContents
+	}
+	return reconstructed, nil
+}
+
 // renderResources renders the templates in a chart
 //
 // TODO: This function is badly in need of a refactor.
@@ -160,6 +239,33 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Valu
 	}
 	notes := notesBuffer.String()
 
+	if pr != nil {
+		// We need to send files to the post-renderer before sorting and splitting
+		// hooks from manifests. The post-renderer interface expects a stream of
+		// manifests (similar to what tools like Kustomize and kubectl expect), whereas
+		// the sorter uses filenames.
+		// Here, we merge the documents into a stream, post-render them, and then split
+		// them back into a map of filename -> content.
+
+		// Merge files as stream of documents for sending to post renderer
+		merged, err := annotateAndMerge(files)
+		if err != nil {
+			return hs, b, notes, fmt.Errorf("error merging manifests: %w", err)
+		}
+
+		// Run the post renderer
+		postRendered, err := pr.Run(bytes.NewBufferString(merged))
+		if err != nil {
+			return hs, b, notes, fmt.Errorf("error while running post render on files: %w", err)
+		}
+
+		// Use the file list and contents received from the post renderer
+		files, err = splitAndDeannotate(postRendered.String())
+		if err != nil {
+			return hs, b, notes, fmt.Errorf("error while parsing post rendered output: %w", err)
+		}
+	}
+
 	// Sort hooks, manifests, and partials. Only hooks and manifests are returned,
 	// as partials are not used after renderer.Render. Empty manifests are also
 	// removed here.
@@ -220,13 +326,6 @@ func (cfg *Configuration) renderResources(ch *chart.Chart, values chartutil.Valu
 		}
 	}
 
-	if pr != nil {
-		b, err = pr.Run(b)
-		if err != nil {
-			return hs, b, notes, fmt.Errorf("error while running post render on files: %w", err)
-		}
-	}
-
 	return hs, b, notes, nil
 }
 

pkg/action/action_test.go

@@ -16,13 +16,17 @@ limitations under the License.
 package action
 
 import (
+	"bytes"
+	"errors"
 	"flag"
 	"fmt"
 	"io"
 	"log/slog"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	fakeclientset "k8s.io/client-go/kubernetes/fake"
 
 	"helm.sh/helm/v4/internal/logging"
@@ -368,3 +372,577 @@ func TestGetVersionSet(t *testing.T) {
 		t.Error("Non-existent version is reported found.")
 	}
 }
+
+// Mock PostRenderer for testing
+type mockPostRenderer struct {
+	shouldError bool
+	transform   func(string) string
+}
+
+func (m *mockPostRenderer) Run(renderedManifests *bytes.Buffer) (*bytes.Buffer, error) {
+	if m.shouldError {
+		return nil, errors.New("mock post-renderer error")
+	}
+
+	content := renderedManifests.String()
+	if m.transform != nil {
+		content = m.transform(content)
+	}
+
+	return bytes.NewBufferString(content), nil
+}
+
+func TestAnnotateAndMerge(t *testing.T) {
+	tests := []struct {
+		name          string
+		files         map[string]string
+		expectedError string
+		expected      string
+	}{
+		{
+			name:     "no files",
+			files:    map[string]string{},
+			expected: "",
+		},
+		{
+			name: "single file with single manifest",
+			files: map[string]string{
+				"templates/configmap.yaml": `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm
+data:
+  key: value`,
+			},
+			expected: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm
+  annotations:
+    postrenderer.helm.sh/postrender-filename: 'templates/configmap.yaml'
+data:
+  key: value
+`,
+		},
+		{
+			name: "multiple files with multiple manifests",
+			files: map[string]string{
+				"templates/configmap.yaml": `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm
+data:
+  key: value`,
+				"templates/secret.yaml": `apiVersion: v1
+kind: Secret
+metadata:
+  name: test-secret
+data:
+  password: dGVzdA==`,
+			},
+			expected: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm
+  annotations:
+    postrenderer.helm.sh/postrender-filename: 'templates/configmap.yaml'
+data:
+  key: value
+---
+apiVersion: v1
+kind: Secret
+metadata:
+  name: test-secret
+  annotations:
+    postrenderer.helm.sh/postrender-filename: 'templates/secret.yaml'
+data:
+  password: dGVzdA==
+`,
+		},
+		{
+			name: "file with multiple manifests",
+			files: map[string]string{
+				"templates/multi.yaml": `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm1
+data:
+  key: value1
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm2
+data:
+  key: value2`,
+			},
+			expected: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm1
+  annotations:
+    postrenderer.helm.sh/postrender-filename: 'templates/multi.yaml'
+data:
+  key: value1
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm2
+  annotations:
+    postrenderer.helm.sh/postrender-filename: 'templates/multi.yaml'
+data:
+  key: value2
+`,
+		},
+		{
+			name: "partials and empty files are removed",
+			files: map[string]string{
+				"templates/cm.yaml": `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm1
+`,
+				"templates/_partial.tpl": `
+{{-define name}}
+  {{- "abracadabra"}}
+{{- end -}}`,
+				"templates/empty.yaml": ``,
+			},
+			expected: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm1
+  annotations:
+    postrenderer.helm.sh/postrender-filename: 'templates/cm.yaml'
+`,
+		},
+		{
+			name: "empty file",
+			files: map[string]string{
+				"templates/empty.yaml": "",
+			},
+			expected: ``,
+		},
+		{
+			name: "invalid yaml",
+			files: map[string]string{
+				"templates/invalid.yaml": `invalid: yaml: content:
+  - malformed`,
+			},
+			expectedError: "parsing templates/invalid.yaml",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			merged, err := annotateAndMerge(tt.files)
+
+			if tt.expectedError != "" {
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), tt.expectedError)
+			} else {
+				assert.NoError(t, err)
+				assert.NotNil(t, merged)
+				assert.Equal(t, tt.expected, merged)
+			}
+		})
+	}
+}
+
+func TestSplitAndDeannotate(t *testing.T) {
+	tests := []struct {
+		name          string
+		input         string
+		expectedFiles map[string]string
+		expectedError string
+	}{
+		{
+			name: "single annotated manifest",
+			input: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm
+  annotations:
+    postrenderer.helm.sh/postrender-filename: templates/configmap.yaml
+data:
+  key: value`,
+			expectedFiles: map[string]string{
+				"templates/configmap.yaml": `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm
+data:
+  key: value
+`,
+			},
+		},
+		{
+			name: "multiple manifests with different filenames",
+			input: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm
+  annotations:
+    postrenderer.helm.sh/postrender-filename: templates/configmap.yaml
+data:
+  key: value
+---
+apiVersion: v1
+kind: Secret
+metadata:
+  name: test-secret
+  annotations:
+    postrenderer.helm.sh/postrender-filename: templates/secret.yaml
+data:
+  password: dGVzdA==`,
+			expectedFiles: map[string]string{
+				"templates/configmap.yaml": `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm
+data:
+  key: value
+`,
+				"templates/secret.yaml": `apiVersion: v1
+kind: Secret
+metadata:
+  name: test-secret
+data:
+  password: dGVzdA==
+`,
+			},
+		},
+		{
+			name: "multiple manifests with same filename",
+			input: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm1
+  annotations:
+    postrenderer.helm.sh/postrender-filename: templates/multi.yaml
+data:
+  key: value1
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm2
+  annotations:
+    postrenderer.helm.sh/postrender-filename: templates/multi.yaml
+data:
+  key: value2`,
+			expectedFiles: map[string]string{
+				"templates/multi.yaml": `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm1
+data:
+  key: value1
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm2
+data:
+  key: value2
+`,
+			},
+		},
+		{
+			name: "manifest with other annotations",
+			input: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm
+  annotations:
+    postrenderer.helm.sh/postrender-filename: templates/configmap.yaml
+    other-annotation: should-remain
+data:
+  key: value`,
+			expectedFiles: map[string]string{
+				"templates/configmap.yaml": `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm
+  annotations:
+    other-annotation: should-remain
+data:
+  key: value
+`,
+			},
+		},
+		{
+			name:          "invalid yaml input",
+			input:         "invalid: yaml: content:",
+			expectedError: "error parsing YAML: MalformedYAMLError",
+		},
+		{
+			name: "manifest without filename annotation",
+			input: `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm
+data:
+  key: value`,
+			expectedFiles: map[string]string{
+				"generated-by-postrender-0.yaml": `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm
+data:
+  key: value
+`,
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			files, err := splitAndDeannotate(tt.input)
+
+			if tt.expectedError != "" {
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), tt.expectedError)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, len(tt.expectedFiles), len(files))
+
+				for expectedFile, expectedContent := range tt.expectedFiles {
+					actualContent, exists := files[expectedFile]
+					assert.True(t, exists, "Expected file %s not found", expectedFile)
+					assert.Equal(t, expectedContent, actualContent)
+				}
+			}
+		})
+	}
+}
+
+func TestAnnotateAndMerge_SplitAndDeannotate_Roundtrip(t *testing.T) {
+	// Test that merge/split operations are symmetric
+	originalFiles := map[string]string{
+		"templates/configmap.yaml": `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm
+data:
+  key: value`,
+		"templates/secret.yaml": `apiVersion: v1
+kind: Secret
+metadata:
+  name: test-secret
+data:
+  password: dGVzdA==`,
+		"templates/multi.yaml": `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm1
+data:
+  key: value1
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-cm2
+data:
+  key: value2`,
+	}
+
+	// Merge and annotate
+	merged, err := annotateAndMerge(originalFiles)
+	require.NoError(t, err)
+
+	// Split and deannotate
+	reconstructed, err := splitAndDeannotate(merged)
+	require.NoError(t, err)
+
+	// Compare the results
+	assert.Equal(t, len(originalFiles), len(reconstructed))
+	for filename, originalContent := range originalFiles {
+		reconstructedContent, exists := reconstructed[filename]
+		assert.True(t, exists, "File %s should exist in reconstructed files", filename)
+
+		// Normalize whitespace for comparison since YAML processing might affect formatting
+		normalizeContent := func(content string) string {
+			return strings.TrimSpace(strings.ReplaceAll(content, "\r\n", "\n"))
+		}
+
+		assert.Equal(t, normalizeContent(originalContent), normalizeContent(reconstructedContent))
+	}
+}
+
+func TestRenderResources_PostRenderer_Success(t *testing.T) {
+	cfg := actionConfigFixture(t)
+
+	// Create a simple mock post-renderer
+	mockPR := &mockPostRenderer{
+		transform: func(content string) string {
+			content = strings.ReplaceAll(content, "hello", "yellow")
+			content = strings.ReplaceAll(content, "goodbye", "foodpie")
+			return strings.ReplaceAll(content, "test-cm", "test-cm-postrendered")
+		},
+	}
+
+	ch := buildChart(withSampleTemplates())
+	values := map[string]interface{}{}
+
+	hooks, buf, notes, err := cfg.renderResources(
+		ch, values, "test-release", "", false, false, false,
+		mockPR, false, false, false,
+	)
+
+	assert.NoError(t, err)
+	assert.NotNil(t, hooks)
+	assert.NotNil(t, buf)
+	assert.Equal(t, "", notes)
+	expectedBuf := `---
+# Source: yellow/templates/foodpie
+foodpie: world
+---
+# Source: yellow/templates/with-partials
+yellow: Earth
+---
+# Source: yellow/templates/yellow
+yellow: world
+`
+	expectedHook := `kind: ConfigMap
+metadata:
+  name: test-cm-postrendered
+  annotations:
+    "helm.sh/hook": post-install,pre-delete,post-upgrade
+data:
+  name: value`
+
+	assert.Equal(t, expectedBuf, buf.String())
+	assert.Len(t, hooks, 1)
+	assert.Equal(t, expectedHook, hooks[0].Manifest)
+}
+
+func TestRenderResources_PostRenderer_Error(t *testing.T) {
+	cfg := actionConfigFixture(t)
+
+	// Create a post-renderer that returns an error
+	mockPR := &mockPostRenderer{
+		shouldError: true,
+	}
+
+	ch := buildChart(withSampleTemplates())
+	values := map[string]interface{}{}
+
+	_, _, _, err := cfg.renderResources(
+		ch, values, "test-release", "", false, false, false,
+		mockPR, false, false, false,
+	)
+
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "error while running post render on files")
+}
+
+func TestRenderResources_PostRenderer_MergeError(t *testing.T) {
+	cfg := actionConfigFixture(t)
+
+	// Create a mock post-renderer
+	mockPR := &mockPostRenderer{}
+
+	// Create a chart with invalid YAML that would cause AnnotateAndMerge to fail
+	ch := &chart.Chart{
+		Metadata: &chart.Metadata{
+			APIVersion: "v1",
+			Name:       "test-chart",
+			Version:    "0.1.0",
+		},
+		Templates: []*chart.File{
+			{Name: "templates/invalid", Data: []byte("invalid: yaml: content:")},
+		},
+	}
+	values := map[string]interface{}{}
+
+	_, _, _, err := cfg.renderResources(
+		ch, values, "test-release", "", false, false, false,
+		mockPR, false, false, false,
+	)
+
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "error merging manifests")
+}
+
+func TestRenderResources_PostRenderer_SplitError(t *testing.T) {
+	cfg := actionConfigFixture(t)
+
+	// Create a post-renderer that returns invalid YAML
+	mockPR := &mockPostRenderer{
+		transform: func(_ string) string {
+			return "invalid: yaml: content:"
+		},
+	}
+
+	ch := buildChart(withSampleTemplates())
+	values := map[string]interface{}{}
+
+	_, _, _, err := cfg.renderResources(
+		ch, values, "test-release", "", false, false, false,
+		mockPR, false, false, false,
+	)
+
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "error while parsing post rendered output: error parsing YAML: MalformedYAMLError:")
+}
+
+func TestRenderResources_PostRenderer_Integration(t *testing.T) {
+	cfg := actionConfigFixture(t)
+
+	mockPR := &mockPostRenderer{
+		transform: func(content string) string {
+			return strings.ReplaceAll(content, "metadata:", "color: blue\nmetadata:")
+		},
+	}
+
+	ch := buildChart(withSampleTemplates())
+	values := map[string]interface{}{}
+
+	hooks, buf, notes, err := cfg.renderResources(
+		ch, values, "test-release", "", false, false, false,
+		mockPR, false, false, false,
+	)
+
+	assert.NoError(t, err)
+	assert.NotNil(t, hooks)
+	assert.NotNil(t, buf)
+	assert.Equal(t, "", notes) // Notes should be empty for this test
+
+	// Verify that the post-renderer modifications are present in the output
+	output := buf.String()
+	expected := `---
+# Source: hello/templates/goodbye
+goodbye: world
+color: blue
+---
+# Source: hello/templates/hello
+hello: world
+color: blue
+---
+# Source: hello/templates/with-partials
+hello: Earth
+color: blue
+`
+	assert.Contains(t, output, "color: blue")
+	assert.Equal(t, 3, strings.Count(output, "color: blue"))
+	assert.Equal(t, expected, output)
+}
+
+func TestRenderResources_NoPostRenderer(t *testing.T) {
+	cfg := actionConfigFixture(t)
+
+	ch := buildChart(withSampleTemplates())
+	values := map[string]interface{}{}
+
+	hooks, buf, notes, err := cfg.renderResources(
+		ch, values, "test-release", "", false, false, false,
+		nil, false, false, false,
+	)
+
+	assert.NoError(t, err)
+	assert.NotNil(t, hooks)
+	assert.NotNil(t, buf)
+	assert.Equal(t, "", notes)
+}