From ad108148424f1a5d607a3aa402d2e0cb6754ca38 Mon Sep 17 00:00:00 2001 From: yxxhero <11087727+yxxhero@users.noreply.github.com> Date: Mon, 29 Jun 2026 20:40:18 +0800 Subject: [PATCH] fix: replace YAML-based DeepCopy with recursive deep copy to fix #973 (#2675) Environment.DeepCopy() used a YAML marshal/unmarshal round-trip to copy values. When SOPS/KMS-encrypted secret files contained values with special characters (colons, quotes, braces such as ~masked:ab#7i7!;{'".), the YAML round-trip could mangle or silently drop adjacent keys, producing the "map has no entry for key" error reported in #973. Replace the YAML-based DeepCopy with maputil.DeepCopyMap(), a proper recursive deep copy that: - Preserves original Go types (string "true" stays string, not bool) - Never loses data due to special characters in values - Normalises map[any]any keys to strings (matching CastKeysToStrings) Signed-off-by: yxxhero --- pkg/environment/environment.go | 51 ++----------- pkg/environment/environment_test.go | 68 +++++++++++++++++ pkg/maputil/maputil.go | 38 ++++++++++ pkg/maputil/maputil_test.go | 111 ++++++++++++++++++++++++++++ 4 files changed, 225 insertions(+), 43 deletions(-) diff --git a/pkg/environment/environment.go b/pkg/environment/environment.go index faf84582..32a507d0 100644 --- a/pkg/environment/environment.go +++ b/pkg/environment/environment.go @@ -2,7 +2,6 @@ package environment import ( "github.com/helmfile/helmfile/pkg/maputil" - "github.com/helmfile/helmfile/pkg/yaml" ) type Environment struct { @@ -32,52 +31,18 @@ func New(name string) *Environment { } } +// DeepCopy returns a deep copy of the environment. +// It uses maputil.DeepCopyMap rather than a YAML marshal/unmarshal round-trip +// so that secret values containing special characters (colons, quotes, braces, +// etc.) are never mangled or lost. This fixes issue #973 where large SOPS-encrypted +// secret files caused environment values like "myValue" to silently disappear. func (e Environment) DeepCopy() Environment { - valuesBytes, err := yaml.Marshal(e.Values) - if err != nil { - panic(err) - } - var values map[string]any - if err := yaml.Unmarshal(valuesBytes, &values); err != nil { - panic(err) - } - values, err = maputil.CastKeysToStrings(values) - if err != nil { - panic(err) - } - - defaultsBytes, err := yaml.Marshal(e.Defaults) - if err != nil { - panic(err) - } - var defaults map[string]any - if err := yaml.Unmarshal(defaultsBytes, &defaults); err != nil { - panic(err) - } - defaults, err = maputil.CastKeysToStrings(defaults) - if err != nil { - panic(err) - } - - cliOverridesBytes, err := yaml.Marshal(e.CLIOverrides) - if err != nil { - panic(err) - } - var cliOverrides map[string]any - if err := yaml.Unmarshal(cliOverridesBytes, &cliOverrides); err != nil { - panic(err) - } - cliOverrides, err = maputil.CastKeysToStrings(cliOverrides) - if err != nil { - panic(err) - } - return Environment{ Name: e.Name, KubeContext: e.KubeContext, - Values: values, - Defaults: defaults, - CLIOverrides: cliOverrides, + Values: maputil.DeepCopyMap(e.Values), + Defaults: maputil.DeepCopyMap(e.Defaults), + CLIOverrides: maputil.DeepCopyMap(e.CLIOverrides), } } diff --git a/pkg/environment/environment_test.go b/pkg/environment/environment_test.go index e990995b..4d15f236 100644 --- a/pkg/environment/environment_test.go +++ b/pkg/environment/environment_test.go @@ -237,3 +237,71 @@ func TestEnvironment_GetMergedValues_Issue2527_ValuesOverrideDefaults(t *testing assert.Equal(t, true, hd["wait"]) assert.Equal(t, 300, hd["timeout"]) } + +// TestEnvironment_DeepCopy_Issue973_SecretSpecialChars verifies that DeepCopy +// preserves all keys when values contain special characters (colons, quotes, +// braces, etc.) typical of SOPS/KMS-encrypted secrets. +// Regression test for https://github.com/helmfile/helmfile/issues/973. +func TestEnvironment_DeepCopy_Issue973_SecretSpecialChars(t *testing.T) { + env := &Environment{ + Name: "myEnv", + Values: map[string]any{ + "masked1": map[string]any{ + "masked2": "xxxxxxxxxx", + "masked3": "xxxxxxxxxx", + }, + "masked85": map[string]any{ + "masked86": map[string]any{"masked87": "xxxxxxxxxx"}, + "masked88": map[string]any{"masked89": "~masked:ab#7i7!;{'\"."}, + }, + // myValue must survive the deep copy alongside the secret values. + "myValue": "valueOfMyValue", + }, + Defaults: map[string]any{ + "aDependentValue": "{{.Values.myValue}}", + }, + } + + copied := env.DeepCopy() + + // myValue must be preserved. + assert.Equal(t, "valueOfMyValue", copied.Values["myValue"], "myValue must survive DeepCopy") + // The secret value must be preserved exactly, not mangled. + masked85 := copied.Values["masked85"].(map[string]any) + masked88 := masked85["masked88"].(map[string]any) + assert.Equal(t, "~masked:ab#7i7!;{'\".", masked88["masked89"], + "secret value with special chars must survive DeepCopy unchanged") + // Defaults must be preserved too. + assert.Equal(t, "{{.Values.myValue}}", copied.Defaults["aDependentValue"]) + + // Mutating the copy must not affect the original. + copied.Values["myValue"] = "changed" + assert.Equal(t, "valueOfMyValue", env.Values["myValue"], "DeepCopy must be independent") +} + +// TestEnvironment_Merge_Issue973_NoDataLoss verifies that Merge (which calls +// DeepCopy internally) does not drop any keys when secret values with special +// characters are present. +func TestEnvironment_Merge_Issue973_NoDataLoss(t *testing.T) { + base := &Environment{ + Name: "myEnv", + Values: map[string]any{}, + } + + loaded := &Environment{ + Name: "myEnv", + Values: map[string]any{ + "masked1": map[string]any{"masked2": "xxxxxxxxxx"}, + "masked89": "~masked:ab#7i7!;{'\".", + "myValue": "valueOfMyValue", + }, + } + + merged, err := base.Merge(loaded) + require.NoError(t, err) + + assert.Equal(t, "valueOfMyValue", merged.Values["myValue"], + "myValue must survive Merge with secret values present") + assert.Equal(t, "~masked:ab#7i7!;{'\".", merged.Values["masked89"], + "secret value must survive Merge unchanged") +} diff --git a/pkg/maputil/maputil.go b/pkg/maputil/maputil.go index 168c2366..47192035 100644 --- a/pkg/maputil/maputil.go +++ b/pkg/maputil/maputil.go @@ -39,6 +39,44 @@ func CastKeysToStrings(s any) (map[string]any, error) { return new, nil } +// DeepCopyMap creates a deep copy of a map[string]any, recursively copying all +// nested maps and slices. Unlike YAML marshal/unmarshal round-trips, it preserves +// the original Go types of all values (e.g. the string "true" stays a string, +// not a bool) and never loses data due to special characters in values. +// It also normalises map[any]any keys to strings, matching CastKeysToStrings. +func DeepCopyMap(m map[string]any) map[string]any { + if m == nil { + return nil + } + result := make(map[string]any, len(m)) + for k, v := range m { + result[k] = deepCopyValue(v) + } + return result +} + +func deepCopyValue(v any) any { + switch val := v.(type) { + case map[string]any: + return DeepCopyMap(val) + case map[any]any: + copied := make(map[string]any, len(val)) + for k, item := range val { + copied[fmt.Sprintf("%v", k)] = deepCopyValue(item) + } + return copied + case []any: + copied := make([]any, len(val)) + for i, item := range val { + copied[i] = deepCopyValue(item) + } + return copied + default: + // Primitives (string, int, bool, nil, float, etc.) are copied by value. + return val + } +} + func RecursivelyStringifyMapKey(v any) (any, error) { var castedV any switch typedV := v.(type) { diff --git a/pkg/maputil/maputil_test.go b/pkg/maputil/maputil_test.go index 3dbd6f97..2558988a 100644 --- a/pkg/maputil/maputil_test.go +++ b/pkg/maputil/maputil_test.go @@ -893,3 +893,114 @@ func TestMergeMaps_ArrayStrategies(t *testing.T) { } }) } + +// TestDeepCopyMap_Issue973 verifies that DeepCopyMap preserves all values +// exactly, including strings with special characters that would break a +// YAML marshal/unmarshal round-trip. +// Regression test for https://github.com/helmfile/helmfile/issues/973. +func TestDeepCopyMap_Issue973(t *testing.T) { + tests := []struct { + name string + input map[string]any + }{ + { + name: "special_chars_from_sops_secret", + input: map[string]any{ + "masked85": map[string]any{ + "masked88": map[string]any{ + "masked89": "~masked:ab#7i7!;{'\".", + }, + }, + "myValue": "valueOfMyValue", + }, + }, + { + name: "yaml_ambiguous_strings_stay_strings", + input: map[string]any{ + "yes_val": "yes", + "null_val": "null", + "num_str": "007", + "colon_val": "key: value", + "brace_val": "{flow}", + "pipe_val": "|block", + }, + }, + { + name: "deeply_nested_structures", + input: map[string]any{ + "l1": map[string]any{ + "l2": map[string]any{ + "l3": map[string]any{ + "l4": map[string]any{ + "l5": "deep_value", + }, + }, + }, + }, + }, + }, + { + name: "arrays_with_maps", + input: map[string]any{ + "items": []any{ + map[string]any{"name": "a", "value": "1"}, + map[string]any{"name": "b", "value": "2"}, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + copied := DeepCopyMap(tt.input) + if !reflect.DeepEqual(tt.input, copied) { + t.Errorf("DeepCopyMap changed data.\nExpected: %v\nGot: %v", tt.input, copied) + } + }) + } +} + +// TestDeepCopyMap_Independence verifies that mutating the copy does not affect +// the original (i.e. it is a true deep copy, not a shallow one). +func TestDeepCopyMap_Independence(t *testing.T) { + original := map[string]any{ + "nested": map[string]any{"key": "value"}, + "array": []any{map[string]any{"k": "v"}}, + } + + copied := DeepCopyMap(original) + + // Mutate nested map + copied["nested"].(map[string]any)["key"] = "changed" + if original["nested"].(map[string]any)["key"] != "value" { + t.Error("DeepCopyMap: mutating nested map in copy affected original") + } + + // Mutate array element map + copied["array"].([]any)[0].(map[string]any)["k"] = "changed2" + if original["array"].([]any)[0].(map[string]any)["k"] != "v" { + t.Error("DeepCopyMap: mutating array element in copy affected original") + } +} + +// TestDeepCopyMap_AnyKeysNormalises verifies that map[any]any keys are +// normalised to strings during the deep copy. +func TestDeepCopyMap_AnyKeysNormalises(t *testing.T) { + // Simulate what yaml v2 produces: map[any]any with int keys. + input := map[string]any{ + "mixed": map[any]any{ + 1: "int_key", + "str": "str_key", + }, + } + + copied := DeepCopyMap(input) + mixed := copied["mixed"].(map[string]any) + + if _, ok := mixed["1"]; !ok { + t.Errorf("int key 1 should have been normalised to string key \"1\"; got: %v", mixed) + } + if mixed["str"] != "str_key" { + t.Errorf("string key should be preserved; got: %v", mixed) + } +}