From b3f4586ed8dfd5645da1112fd7651de6413a8ee2 Mon Sep 17 00:00:00 2001 From: KUOKA Yusuke Date: Thu, 30 May 2019 15:41:00 +0900 Subject: [PATCH] Fix panic related to env values from files and sprig dict funcs (#625) * fix: 0.68.0: go panic w/ multiple bases/layers Fixes #623 * fix: 0.64.1+: getOrNil/hasKey fails on Environment.Values with nested maps Fixes #624 --- environment/environment.go | 11 ++- pkg/app/app_test.go | 135 +++++++++++++++++++++++++++++ pkg/app/two_pass_renderer.go | 2 +- pkg/maputil/maputil.go | 50 +++++++++++ state/environment_values_loader.go | 14 ++- state/state.go | 2 +- 6 files changed, 200 insertions(+), 14 deletions(-) create mode 100644 pkg/maputil/maputil.go diff --git a/environment/environment.go b/environment/environment.go index f10b5252..69bc59d7 100644 --- a/environment/environment.go +++ b/environment/environment.go @@ -1,8 +1,9 @@ package environment import ( - "encoding/json" "github.com/imdario/mergo" + "github.com/roboll/helmfile/pkg/maputil" + "gopkg.in/yaml.v2" ) type Environment struct { @@ -13,12 +14,16 @@ type Environment struct { var EmptyEnvironment Environment func (e Environment) DeepCopy() Environment { - bytes, err := json.Marshal(e.Values) + bytes, err := yaml.Marshal(e.Values) if err != nil { panic(err) } var values map[string]interface{} - if err := json.Unmarshal(bytes, &values); err != nil { + if err := yaml.Unmarshal(bytes, &values); err != nil { + panic(err) + } + values, err = maputil.CastKeysToStrings(values) + if err != nil { panic(err) } return Environment{ diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 97c23c64..40f03dbd 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -1356,3 +1356,138 @@ releases: t.Errorf("unexpected releases[0].chart: expected=BAR, got=%s", st.Releases[0].Chart) } } + +// See https://github.com/roboll/helmfile/issues/623 +func TestLoadDesiredStateFromYaml_MultiPartTemplate_MergeMapsVariousKeys(t *testing.T) { + type testcase struct { + overrideValues interface{} + expected string + } + testcases := []testcase{ + {map[interface{}]interface{}{"foo": "FOO"}, `FOO`}, + {map[interface{}]interface{}{"foo": map[interface{}]interface{}{"foo": "FOO"}}, `map[foo:FOO]`}, + {map[interface{}]interface{}{"foo": map[string]interface{}{"foo": "FOO"}}, `map[foo:FOO]`}, + {map[interface{}]interface{}{"foo": []interface{}{"foo"}}, `[foo]`}, + {map[interface{}]interface{}{"foo": "FOO"}, `FOO`}, + } + for i := range testcases { + tc := testcases[i] + statePath := "/path/to/helmfile.yaml" + stateContent := ` +environments: + default: + values: + - 1.yaml + - 2.yaml +--- +releases: +- name: {{ .Environment.Values.foo | quote }} + chart: {{ .Environment.Values.bar | quote }} +` + testFs := state.NewTestFs(map[string]string{ + statePath: stateContent, + "/path/to/1.yaml": `bar: ["bar"]`, + "/path/to/2.yaml": `bar: ["BAR"]`, + }) + app := &App{ + readFile: testFs.ReadFile, + glob: testFs.Glob, + abs: testFs.Abs, + Env: "default", + Logger: helmexec.NewLogger(os.Stderr, "debug"), + Reverse: true, + } + opts := LoadOpts{ + CalleePath: statePath, + Environment: state.SubhelmfileEnvironmentSpec{ + OverrideValues: []interface{}{tc.overrideValues}, + }, + } + st, err := app.loadDesiredStateFromYaml(statePath, opts) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if st.Releases[0].Name != tc.expected { + t.Errorf("unexpected releases[0].name: expected=%s, got=%s", tc.expected, st.Releases[0].Name) + } + if st.Releases[0].Chart != "[BAR]" { + t.Errorf("unexpected releases[0].chart: expected=BAR, got=%s", st.Releases[0].Chart) + } + } +} + +func TestLoadDesiredStateFromYaml_MultiPartTemplate_SprigDictFuncs(t *testing.T) { + type testcase struct { + state string + expr string + expected string + } + stateInline := ` +environments: + default: + values: + - foo: FOO + bar: { "baz": "BAZ" } +--- +releases: +- name: %s + chart: stable/nginx +` + stateExternal := ` +environments: + default: + values: + - 1.yaml + - 2.yaml +--- +releases: +- name: %s + chart: stable/nginx +` + testcases := []testcase{ + {stateInline, `{{ getOrNil "foo" .Environment.Values }}`, `FOO`}, + {stateInline, `{{ getOrNil "baz" (getOrNil "bar" .Environment.Values) }}`, `BAZ`}, + {stateInline, `{{ if hasKey .Environment.Values "foo" }}{{ .Environment.Values.foo }}{{ end }}`, `FOO`}, + {stateInline, `{{ if hasKey .Environment.Values "bar" }}{{ .Environment.Values.bar.baz }}{{ end }}`, `BAZ`}, + {stateInline, `{{ if (keys .Environment.Values | has "foo") }}{{ .Environment.Values.foo }}{{ end }}`, `FOO`}, + // See https://github.com/roboll/helmfile/issues/624 + // This fails when .Environment.Values.bar is not map[string]interface{}. At the time of #624 it was map[interface{}]interface{}, which sprig's dict funcs don't support. + {stateInline, `{{ if (keys .Environment.Values | has "bar") }}{{ if (keys .Environment.Values.bar | has "baz") }}{{ .Environment.Values.bar.baz }}{{ end }}{{ end }}`, `BAZ`}, + {stateExternal, `{{ getOrNil "foo" .Environment.Values }}`, `FOO`}, + {stateExternal, `{{ getOrNil "baz" (getOrNil "bar" .Environment.Values) }}`, `BAZ`}, + {stateExternal, `{{ if hasKey .Environment.Values "foo" }}{{ .Environment.Values.foo }}{{ end }}`, `FOO`}, + {stateExternal, `{{ if hasKey .Environment.Values "bar" }}{{ .Environment.Values.bar.baz }}{{ end }}`, `BAZ`}, + {stateExternal, `{{ if (keys .Environment.Values | has "foo") }}{{ .Environment.Values.foo }}{{ end }}`, `FOO`}, + // See https://github.com/roboll/helmfile/issues/624 + {stateExternal, `{{ if (keys .Environment.Values | has "bar") }}{{ if (keys .Environment.Values.bar | has "baz") }}{{ .Environment.Values.bar.baz }}{{ end }}{{ end }}`, `BAZ`}, + } + for i := range testcases { + tc := testcases[i] + statePath := "/path/to/helmfile.yaml" + stateContent := fmt.Sprintf(tc.state, tc.expr) + testFs := state.NewTestFs(map[string]string{ + statePath: stateContent, + "/path/to/1.yaml": `foo: FOO`, + "/path/to/2.yaml": `bar: { "baz": "BAZ" }`, + }) + app := &App{ + readFile: testFs.ReadFile, + glob: testFs.Glob, + abs: testFs.Abs, + Env: "default", + Logger: helmexec.NewLogger(os.Stderr, "debug"), + Reverse: true, + } + st, err := app.loadDesiredStateFromYaml(statePath) + + if err != nil { + t.Fatalf("unexpected error at %d: %v", i, err) + } + + if st.Releases[0].Name != tc.expected { + t.Errorf("unexpected releases[0].name at %d: expected=%s, got=%s", i, tc.expected, st.Releases[0].Name) + } + } +} diff --git a/pkg/app/two_pass_renderer.go b/pkg/app/two_pass_renderer.go index 60090d77..015fc230 100644 --- a/pkg/app/two_pass_renderer.go +++ b/pkg/app/two_pass_renderer.go @@ -80,7 +80,7 @@ func (r *desiredStateLoader) twoPassRenderTemplateToYaml(inherited, overrode *en renderedEnv := r.renderEnvironment(initEnv, baseDir, filename, content) if r.logger != nil { - r.logger.Debugf("first-pass produced: %v", initEnv) + r.logger.Debugf("first-pass produced: %v", renderedEnv) } finalEnv, err := renderedEnv.Merge(overrode) diff --git a/pkg/maputil/maputil.go b/pkg/maputil/maputil.go new file mode 100644 index 00000000..c26a207c --- /dev/null +++ b/pkg/maputil/maputil.go @@ -0,0 +1,50 @@ +package maputil + +import "fmt" + +func CastKeysToStrings(s interface{}) (map[string]interface{}, error) { + new := map[string]interface{}{} + switch src := s.(type) { + case map[interface{}]interface{}: + for k, v := range src { + var str_k string + switch typed_k := k.(type) { + case string: + str_k = typed_k + default: + return nil, fmt.Errorf("unexpected type of key in map: expected string, got %T: value=%v, map=%v", typed_k, typed_k, src) + } + + var casted_v interface{} + switch typed_v := v.(type) { + case map[interface{}]interface{}: + tmp, err := CastKeysToStrings(typed_v) + if err != nil { + return nil, err + } + casted_v = tmp + default: + casted_v = typed_v + } + + new[str_k] = casted_v + } + case map[string]interface{}: + for k, v := range src { + var casted_v interface{} + switch typed_v := v.(type) { + case map[interface{}]interface{}: + tmp, err := CastKeysToStrings(typed_v) + if err != nil { + return nil, err + } + casted_v = tmp + default: + casted_v = typed_v + } + + new[k] = casted_v + } + } + return new, nil +} diff --git a/state/environment_values_loader.go b/state/environment_values_loader.go index befecc51..cf16a18e 100644 --- a/state/environment_values_loader.go +++ b/state/environment_values_loader.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/imdario/mergo" "github.com/roboll/helmfile/environment" + "github.com/roboll/helmfile/pkg/maputil" "github.com/roboll/helmfile/tmpl" "gopkg.in/yaml.v2" "path/filepath" @@ -53,21 +54,16 @@ func (ld *EnvironmentValuesLoader) LoadEnvironmentValues(missingFileHandler *str } } case map[interface{}]interface{}: - m := map[string]interface{}{} - for k, v := range typedValue { - switch typedKey := k.(type) { - case string: - m[typedKey] = v - default: - return nil, fmt.Errorf("unexpected type of key in inline environment values %v: expected string, got %T", typedValue, typedKey) - } + m, err := maputil.CastKeysToStrings(typedValue) + if err != nil { + return nil, err } if err := mergo.Merge(&envVals, &m, mergo.WithOverride); err != nil { return nil, fmt.Errorf("failed to merge %v: %v", typedValue, err) } continue default: - return nil, fmt.Errorf("unexpected type of values entry: %T", typedValue) + return nil, fmt.Errorf("unexpected type of value: value=%v, type=%T", typedValue, typedValue) } } diff --git a/state/state.go b/state/state.go index 8587ac6e..e625b527 100644 --- a/state/state.go +++ b/state/state.go @@ -1319,7 +1319,7 @@ func (st *HelmState) generateTemporaryValuesFiles(values []interface{}, missingF } generatedFiles = append(generatedFiles, valfile.Name()) default: - return nil, fmt.Errorf("unexpected type of values entry: %T", typedValue) + return nil, fmt.Errorf("unexpected type of value: value=%v, type=%T", typedValue, typedValue) } } return generatedFiles, nil