From 052a2a82195035f227df9bc4f4e5a4c87496d579 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 11 Sep 2025 23:07:14 +0000 Subject: [PATCH] Fix values templating bug where mergeOverwrite mutated global values Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com> --- pkg/state/helmx.go | 6 +- pkg/state/issue_2182_test.go | 130 ++++++++++++++++++++ pkg/state/state.go | 14 ++- pkg/state/state_exec_tmpl.go | 37 +++++- pkg/state/values_isolation_test.go | 76 ++++++++++++ pkg/state/values_template_isolation_test.go | 102 +++++++++++++++ pkg/tmpl/values_isolation_test.go | 57 +++++++++ 7 files changed, 413 insertions(+), 9 deletions(-) create mode 100644 pkg/state/issue_2182_test.go create mode 100644 pkg/state/values_isolation_test.go create mode 100644 pkg/state/values_template_isolation_test.go create mode 100644 pkg/tmpl/values_isolation_test.go diff --git a/pkg/state/helmx.go b/pkg/state/helmx.go index c906f61d..260875e7 100644 --- a/pkg/state/helmx.go +++ b/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 diff --git a/pkg/state/issue_2182_test.go b/pkg/state/issue_2182_test.go new file mode 100644 index 00000000..76c13cfa --- /dev/null +++ b/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") +} \ No newline at end of file diff --git a/pkg/state/state.go b/pkg/state/state.go index 85f8dee9..b313d7f6 100644 --- a/pkg/state/state.go +++ b/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) diff --git a/pkg/state/state_exec_tmpl.go b/pkg/state/state_exec_tmpl.go index 2a8e3877..fe4e2dd0 100644 --- a/pkg/state/state_exec_tmpl.go +++ b/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 { diff --git a/pkg/state/values_isolation_test.go b/pkg/state/values_isolation_test.go new file mode 100644 index 00000000..34abffd7 --- /dev/null +++ b/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") +} \ No newline at end of file diff --git a/pkg/state/values_template_isolation_test.go b/pkg/state/values_template_isolation_test.go new file mode 100644 index 00000000..2cefbcd3 --- /dev/null +++ b/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") +} \ No newline at end of file diff --git a/pkg/tmpl/values_isolation_test.go b/pkg/tmpl/values_isolation_test.go new file mode 100644 index 00000000..d703137a --- /dev/null +++ b/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. +} \ No newline at end of file