Fix values templating bug where mergeOverwrite mutated global values

Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>

pkg/state/helmx.go

@@ -396,7 +396,11 @@ func (st *HelmState) PrepareChartify(helm helmexec.Interface, release *ReleaseSp
 			return nil, clean, fmt.Errorf("rendering set value entry for release %s: %v", release.Name, err)
 		}
 		c.Opts.SetFlags = setFlags
-		c.Opts.TemplateData = st.newReleaseTemplateData(release)
+		templateData, err := st.newReleaseTemplateData(release)
+		if err != nil {
+			return nil, clean, fmt.Errorf("creating template data for release %s: %v", release.Name, err)
+		}
+		c.Opts.TemplateData = templateData
 		c.Opts.TemplateFuncs = st.newReleaseTemplateFuncMap(dir)
 
 		return c, clean, nil

pkg/state/issue_2182_test.go

@@ -0,0 +1,130 @@
+package state
+
+import (
+	"testing"
+
+	"github.com/helmfile/helmfile/pkg/filesystem"
+	"github.com/stretchr/testify/require"
+)
+
+// TestIssue2182_ValuesTemplatingBugFix is an integration test that reproduces
+// the exact scenario described in https://github.com/helmfile/helmfile/issues/2182
+// and verifies that our fix works correctly.
+func TestIssue2182_ValuesTemplatingBugFix(t *testing.T) {
+	// Simulate the exact scenario from the issue:
+	// environments:
+	//   default:
+	//     values:
+	//       - values.yaml
+	// ---
+	// releases:
+	//   - name: foo
+	//     chart: charts/foo
+	//     valuesTemplate:
+	//       - {{ .Values | get "foo" (dict) | mergeOverwrite .Values | toYaml | nindent 8 }}
+	//   - name: bar
+	//     chart: charts/bar
+	//     valuesTemplate:
+	//       - {{ .Values | get "bar" (dict) | mergeOverwrite .Values | toYaml | nindent 8 }}
+
+	// Create test filesystem
+	fs := &filesystem.FileSystem{
+		Glob: func(pattern string) ([]string, error) {
+			return nil, nil
+		},
+		ReadFile: func(filename string) ([]byte, error) {
+			return nil, nil
+		},
+	}
+
+	// Simulate values.yaml content
+	valuesYaml := map[string]any{
+		"global":    "shared-config",
+		"commonKey": "commonValue",
+		"foo": map[string]any{
+			"enabled":     true,
+			"replicaCount": 2,
+			"image":       "foo:1.0.0",
+		},
+		"bar": map[string]any{
+			"enabled":     true,
+			"replicaCount": 1,
+			"image":       "bar:2.0.0",
+		},
+	}
+
+	st := &HelmState{
+		fs:             fs,
+		basePath:       "/tmp",
+		FilePath:       "helmfile.yaml",
+		RenderedValues: valuesYaml,
+	}
+
+	// Define the releases as they would appear in helmfile.yaml
+	fooRelease := &ReleaseSpec{
+		Name:  "foo",
+		Chart: "charts/foo",
+		ValuesTemplate: []any{
+			`{{ .Values | get "foo" (dict) | mergeOverwrite .Values | toYaml | nindent 8 }}`,
+		},
+	}
+
+	barRelease := &ReleaseSpec{
+		Name:  "bar",
+		Chart: "charts/bar",
+		ValuesTemplate: []any{
+			`{{ .Values | get "bar" (dict) | mergeOverwrite .Values | toYaml | nindent 8 }}`,
+		},
+	}
+
+	// Simulate ExecuteTemplates processing releases in order: foo then bar
+	releases1 := []ReleaseSpec{*fooRelease, *barRelease}
+	st.Releases = releases1
+
+	result1, err := st.ExecuteTemplates()
+	require.NoError(t, err, "ExecuteTemplates should succeed with foo then bar")
+
+	// Simulate ExecuteTemplates processing releases in reverse order: bar then foo  
+	releases2 := []ReleaseSpec{*barRelease, *fooRelease}
+	st.Releases = releases2
+
+	result2, err := st.ExecuteTemplates()
+	require.NoError(t, err, "ExecuteTemplates should succeed with bar then foo")
+
+	// Extract the processed releases from both executions
+	fooRelease1 := result1.Releases[0] // foo from first execution (foo, bar)
+	barRelease1 := result1.Releases[1] // bar from first execution (foo, bar)
+
+	barRelease2 := result2.Releases[0] // bar from second execution (bar, foo)
+	fooRelease2 := result2.Releases[1] // foo from second execution (bar, foo)
+
+	// The critical assertion: Order should not matter!
+	// Before the fix, the second release would see modified values from the first release
+	require.Equal(t, fooRelease1.Values, fooRelease2.Values, 
+		"foo release values should be identical regardless of processing order")
+	require.Equal(t, barRelease1.Values, barRelease2.Values,
+		"bar release values should be identical regardless of processing order")
+
+	// Verify that each release gets the expected merged values
+	// foo release should have foo-specific values merged into the root
+	fooVals1 := fooRelease1.Values[0]
+	require.Contains(t, fooVals1, "enabled")
+	require.Contains(t, fooVals1, "replicaCount") 
+	require.Contains(t, fooVals1, "image")
+	require.Contains(t, fooVals1, "global")    // Should preserve global values
+	require.Contains(t, fooVals1, "commonKey") // Should preserve common values
+
+	// bar release should have bar-specific values merged into the root  
+	barVals1 := barRelease1.Values[0]
+	require.Contains(t, barVals1, "enabled")
+	require.Contains(t, barVals1, "replicaCount")
+	require.Contains(t, barVals1, "image") 
+	require.Contains(t, barVals1, "global")    // Should preserve global values
+	require.Contains(t, barVals1, "commonKey") // Should preserve common values
+
+	// Verify that the original values were not mutated
+	originalVals := st.Values()
+	require.Equal(t, valuesYaml, originalVals, "original values should remain unchanged")
+
+	t.Log("✅ Fix verified: Release order no longer affects values templating results")
+}

pkg/state/state.go

@@ -3167,11 +3167,14 @@ func (st *HelmState) flagsForLint(helm helmexec.Interface, release *ReleaseSpec,
 	return st.appendHelmXFlags(flags, release), files, nil
 }
 
-func (st *HelmState) newReleaseTemplateData(release *ReleaseSpec) releaseTemplateData {
+func (st *HelmState) newReleaseTemplateData(release *ReleaseSpec) (releaseTemplateData, error) {
 	vals := st.Values()
-	templateData := st.createReleaseTemplateData(release, vals)
+	templateData, err := st.createReleaseTemplateData(release, vals)
+	if err != nil {
+		return releaseTemplateData{}, err
+	}
 
-	return templateData
+	return templateData, nil
 }
 
 func (st *HelmState) newReleaseTemplateFuncMap(dir string) template.FuncMap {
@@ -3181,7 +3184,10 @@ func (st *HelmState) newReleaseTemplateFuncMap(dir string) template.FuncMap {
 }
 
 func (st *HelmState) RenderReleaseValuesFileToBytes(release *ReleaseSpec, path string) ([]byte, error) {
-	templateData := st.newReleaseTemplateData(release)
+	templateData, err := st.newReleaseTemplateData(release)
+	if err != nil {
+		return nil, fmt.Errorf("failed to create template data for release %q: %v", release.Name, err)
+	}
 
 	r := tmpl.NewFileRenderer(st.fs, filepath.Dir(path), templateData)
 	rawBytes, err := r.RenderToBytes(path)

pkg/state/state_exec_tmpl.go

@@ -20,13 +20,39 @@ func (st *HelmState) Values() map[string]any {
 	return st.RenderedValues
 }
 
-func (st *HelmState) createReleaseTemplateData(release *ReleaseSpec, vals map[string]any) releaseTemplateData {
+// deepCopyValues creates a deep copy of a values map using YAML marshal/unmarshal.
+// This ensures that template operations don't mutate the original values.
+func deepCopyValues(vals map[string]any) (map[string]any, error) {
+	if vals == nil {
+		return nil, nil
+	}
+	
+	serialized, err := yaml.Marshal(vals)
+	if err != nil {
+		return nil, fmt.Errorf("failed to deep copy values: %v", err)
+	}
+
+	var deserialized map[string]any
+	if err := yaml.Unmarshal(serialized, &deserialized); err != nil {
+		return nil, fmt.Errorf("failed to deep copy values: %v", err)
+	}
+
+	return deserialized, nil
+}
+
+func (st *HelmState) createReleaseTemplateData(release *ReleaseSpec, vals map[string]any) (releaseTemplateData, error) {
+	// Create a deep copy of values to prevent template mutations from affecting global state
+	valuesCopy, err := deepCopyValues(vals)
+	if err != nil {
+		return releaseTemplateData{}, fmt.Errorf("failed to copy values for release %q: %v", release.Name, err)
+	}
+	
 	tmplData := releaseTemplateData{
 		Environment: st.Env,
 		KubeContext: st.OverrideKubeContext,
 		Namespace:   st.OverrideNamespace,
 		Chart:       st.OverrideChart,
-		Values:      vals,
+		Values:      valuesCopy,
 		Release: releaseTemplateDataRelease{
 			Name:         release.Name,
 			Chart:        release.Chart,
@@ -37,7 +63,7 @@ func (st *HelmState) createReleaseTemplateData(release *ReleaseSpec, vals map[st
 		},
 	}
 	tmplData.StateValues = &tmplData.Values
-	return tmplData
+	return tmplData, nil
 }
 
 func getBoolRefFromStringTemplate(templateRef string) (*bool, error) {
@@ -112,7 +138,10 @@ func (st *HelmState) ExecuteTemplates() (*HelmState, error) {
 
 		successFlag := false
 		for it, prev := 0, release; it < 6; it++ {
-			tmplData := st.createReleaseTemplateData(prev, vals)
+			tmplData, err := st.createReleaseTemplateData(prev, vals)
+			if err != nil {
+				return nil, fmt.Errorf("failed creating template data for release \"%s\".\"%s\": %v", st.FilePath, release.Name, err)
+			}
 			renderer := tmpl.NewFileRenderer(st.fs, st.basePath, tmplData)
 			r, err := release.ExecuteTemplateExpressions(renderer)
 			if err != nil {

pkg/state/values_isolation_test.go

@@ -0,0 +1,76 @@
+package state
+
+import (
+	"testing"
+
+	"github.com/helmfile/helmfile/pkg/filesystem"
+	"github.com/stretchr/testify/require"
+)
+
+// TestValuesMutationFix reproduces and tests the fix for the issue described in
+// https://github.com/helmfile/helmfile/issues/2182
+// where mergeOverwrite modifies the global .Values object instead of creating a local copy
+func TestValuesMutationFix(t *testing.T) {
+	// Create test filesystem with no files
+	fs := &filesystem.FileSystem{
+		Glob: func(pattern string) ([]string, error) {
+			return nil, nil
+		},
+		ReadFile: func(filename string) ([]byte, error) {
+			return nil, nil
+		},
+	}
+
+	st := &HelmState{
+		fs:       fs,
+		basePath: "/tmp",
+		RenderedValues: map[string]any{
+			"common": "value",
+			"foo": map[string]any{
+				"specific": "foo-value",
+			},
+			"bar": map[string]any{
+				"specific": "bar-value",
+			},
+		},
+	}
+
+	release := &ReleaseSpec{
+		Name: "test-release",
+		Chart: "test/chart",
+	}
+
+	// Create template data twice to simulate two different releases
+	vals1 := st.Values()
+	tmplData1, err := st.createReleaseTemplateData(release, vals1)
+	require.NoError(t, err, "first createReleaseTemplateData should not fail")
+
+	vals2 := st.Values()
+	tmplData2, err := st.createReleaseTemplateData(release, vals2)
+	require.NoError(t, err, "second createReleaseTemplateData should not fail")
+
+	// Verify that both template data have the same initial values
+	require.Equal(t, tmplData1.Values, tmplData2.Values, "both template data should start with identical values")
+
+	// Simulate mergeOverwrite operation on first template data
+	// This should not affect the second template data after our fix
+	fooSection, ok := tmplData1.Values["foo"].(map[string]any)
+	require.True(t, ok, "foo section should be a map")
+	
+	// Manually perform what mergeOverwrite would do - add values from foo section to the root
+	for k, v := range fooSection {
+		tmplData1.Values[k] = v
+	}
+
+	// Verify that the modification only affected tmplData1, not tmplData2
+	_, hasSpecificInTmpl1 := tmplData1.Values["specific"]
+	_, hasSpecificInTmpl2 := tmplData2.Values["specific"]
+
+	require.True(t, hasSpecificInTmpl1, "tmplData1 should have the merged 'specific' key")
+	require.False(t, hasSpecificInTmpl2, "tmplData2 should NOT have the merged 'specific' key (values should be isolated)")
+
+	// Also verify that the original values are not affected
+	originalVals := st.Values()
+	_, hasSpecificInOriginal := originalVals["specific"]
+	require.False(t, hasSpecificInOriginal, "original Values should NOT be affected")
+}

pkg/state/values_template_isolation_test.go

@@ -0,0 +1,102 @@
+package state
+
+import (
+	"testing"
+
+	"github.com/helmfile/helmfile/pkg/filesystem"
+	"github.com/helmfile/helmfile/pkg/tmpl"
+	"github.com/stretchr/testify/require"
+)
+
+// TestValuesTemplateIsolation tests the fix for the helmfile values templating bug
+// where changing the order of releases resulted in different values being used
+func TestValuesTemplateIsolation(t *testing.T) {
+	// Create test filesystem
+	fs := &filesystem.FileSystem{
+		Glob: func(pattern string) ([]string, error) {
+			return nil, nil
+		},
+		ReadFile: func(filename string) ([]byte, error) {
+			return nil, nil
+		},
+	}
+
+	// Create test environment values
+	envValues := map[string]any{
+		"common": "shared-value",
+		"foo": map[string]any{
+			"name": "foo-chart",
+		},
+		"bar": map[string]any{
+			"name": "bar-chart",
+		},
+	}
+
+	st := &HelmState{
+		fs:             fs,
+		basePath:       "/tmp",
+		RenderedValues: envValues,
+	}
+
+	// Create two releases that use valuesTemplate with mergeOverwrite
+	fooRelease := &ReleaseSpec{
+		Name:  "foo",
+		Chart: "charts/foo",
+		ValuesTemplate: []any{
+			"{{ .Values | get \"foo\" (dict) | mergeOverwrite .Values | toYaml | nindent 8 }}",
+		},
+	}
+
+	barRelease := &ReleaseSpec{
+		Name:  "bar",
+		Chart: "charts/bar",
+		ValuesTemplate: []any{
+			"{{ .Values | get \"bar\" (dict) | mergeOverwrite .Values | toYaml | nindent 8 }}",
+		},
+	}
+
+	// Test: process foo first, then bar
+	vals1 := st.Values()
+	tmplData1, err := st.createReleaseTemplateData(fooRelease, vals1)
+	require.NoError(t, err)
+	
+	renderer1 := tmpl.NewFileRenderer(st.fs, st.basePath, tmplData1)
+	processedFooFirst, err := fooRelease.ExecuteTemplateExpressions(renderer1)
+	require.NoError(t, err)
+
+	vals2 := st.Values()
+	tmplData2, err := st.createReleaseTemplateData(barRelease, vals2)
+	require.NoError(t, err)
+	
+	renderer2 := tmpl.NewFileRenderer(st.fs, st.basePath, tmplData2)
+	processedBarSecond, err := barRelease.ExecuteTemplateExpressions(renderer2)
+	require.NoError(t, err)
+
+	// Test: process bar first, then foo (reverse order)
+	vals3 := st.Values()
+	tmplData3, err := st.createReleaseTemplateData(barRelease, vals3)
+	require.NoError(t, err)
+	
+	renderer3 := tmpl.NewFileRenderer(st.fs, st.basePath, tmplData3)
+	processedBarFirst, err := barRelease.ExecuteTemplateExpressions(renderer3)
+	require.NoError(t, err)
+
+	vals4 := st.Values()
+	tmplData4, err := st.createReleaseTemplateData(fooRelease, vals4)
+	require.NoError(t, err)
+	
+	renderer4 := tmpl.NewFileRenderer(st.fs, st.basePath, tmplData4)
+	processedFooSecond, err := fooRelease.ExecuteTemplateExpressions(renderer4)
+	require.NoError(t, err)
+
+	// Verify that the order doesn't matter - results should be consistent
+	require.Equal(t, processedFooFirst.Values, processedFooSecond.Values, 
+		"foo release should produce same values regardless of processing order")
+	require.Equal(t, processedBarSecond.Values, processedBarFirst.Values,
+		"bar release should produce same values regardless of processing order")
+
+	// Also verify that the original values are not modified
+	originalVals := st.Values()
+	require.Equal(t, envValues, originalVals, 
+		"original values should remain unchanged")
+}

pkg/tmpl/values_isolation_test.go

@@ -0,0 +1,57 @@
+package tmpl
+
+import (
+	"testing"
+
+	"github.com/helmfile/helmfile/pkg/filesystem"
+)
+
+// TestValuesIsolation reproduces the issue described in https://github.com/helmfile/helmfile/issues/2182
+// where mergeOverwrite modifies the global .Values object instead of creating a local copy
+// This test demonstrates that the issue is fixed when using the state layer (which provides isolation)
+func TestValuesIsolation(t *testing.T) {
+	ctx := &Context{
+		fs: &filesystem.FileSystem{
+			Glob: func(pattern string) ([]string, error) {
+				return nil, nil
+			},
+		},
+	}
+
+	// Template that simulates the problematic helmfile.yaml content
+	// NOTE: This test shows that the template layer itself doesn't provide isolation,
+	// but the fix is implemented at the state layer where values are copied before templating
+	template := `
+{{- $originalValues := .Values }}
+{{- $fooValues := .Values | get "foo" (dict) | mergeOverwrite .Values }}
+{{- $barValues := .Values | get "bar" (dict) | mergeOverwrite .Values }}
+First render (should use original + foo): {{ $fooValues | toYaml }}
+Second render (should use original + bar): {{ $barValues | toYaml }}
+Final Values (should be original): {{ .Values | toYaml }}
+Original Values (should be same as final): {{ $originalValues | toYaml }}
+`
+
+	data := map[string]any{
+		"Values": map[string]any{
+			"common": "value",
+			"foo": map[string]any{
+				"specific": "foo-value",
+			},
+			"bar": map[string]any{
+				"specific": "bar-value",
+			},
+		},
+	}
+
+	buf, err := ctx.RenderTemplateToBuffer(template, data)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	result := buf.String()
+	t.Logf("Template result:\n%s", result)
+
+	// This test still demonstrates the raw template behavior (without our fix at state layer),
+	// but the actual bug is fixed at the state layer where we copy values before templating.
+	// See TestValuesTemplateIsolation in pkg/state for the proper integration test.
+}