From 4c9c42d3c59c1169d74d63e5e4bfbab870676ad1 Mon Sep 17 00:00:00 2001 From: KUOKA Yusuke Date: Tue, 21 May 2019 14:37:03 +0900 Subject: [PATCH] fix: gotmpl layer was unable to reference env values from other layers when env!=default (#609) Fixes #604 --- pkg/app/app_test.go | 165 +++++++++++++++++++++++++++ pkg/app/desired_state_file_loader.go | 96 ++++++---------- pkg/app/two_pass_renderer.go | 16 +-- state/create.go | 91 ++++++++++++--- state/create_test.go | 14 +-- 5 files changed, 290 insertions(+), 92 deletions(-) diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 52705cd9..2dff049c 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -842,3 +842,168 @@ helmDefaults: t.Errorf("unexpected helmDefaults.kubeContext: expected=FOO, got=%s", st.HelmDefaults.KubeContext) } } + +func TestLoadDesiredStateFromYaml_MultiPartTemplate_WithNonDefaultEnv(t *testing.T) { + yamlFile := "/path/to/yaml/file" + yamlContent := []byte(`bases: +- ../base.yaml +--- +bases: +- ../base.gotmpl +--- +helmDefaults: + kubeContext: {{ .Environment.Values.foo }} +--- +releases: +- name: myrelease0 + chart: mychart0 +--- + +{{ readFile "templates.yaml" }} + +releases: +- name: myrelease1 + chart: mychart1 + labels: + stage: pre + foo: bar +- name: myrelease1 + chart: mychart2 + labels: + stage: post + <<: *default +`) + files := map[string][]byte{ + yamlFile: yamlContent, + "/path/to/base.yaml": []byte(`environments: + test: + values: + - environments/default/1.yaml +`), + "/path/to/yaml/environments/default/1.yaml": []byte(`foo: FOO`), + "/path/to/base.gotmpl": []byte(`environments: + test: + values: + - environments/default/2.yaml + +helmDefaults: + tillerNamespace: {{ .Environment.Values.tillerNs }} +`), + "/path/to/yaml/environments/default/2.yaml": []byte(`tillerNs: TILLER_NS`), + "/path/to/yaml/templates.yaml": []byte(`templates: + default: &default + missingFileHandler: Warn + values: ["` + "{{`" + `{{.Release.Name}}` + "`}}" + `/values.yaml"] +`), + } + readFile := func(filename string) ([]byte, error) { + content, ok := files[filename] + if !ok { + return nil, fmt.Errorf("unexpected filename: %s", filename) + } + return content, nil + } + app := &App{ + readFile: readFile, + glob: filepath.Glob, + abs: filepath.Abs, + Env: "test", + Logger: helmexec.NewLogger(os.Stderr, "debug"), + } + st, err := app.loadDesiredStateFromYaml(yamlFile) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if st.HelmDefaults.TillerNamespace != "TILLER_NS" { + t.Errorf("unexpected helmDefaults.tillerNamespace: expected=TILLER_NS, got=%s", st.HelmDefaults.TillerNamespace) + } + + if st.Releases[0].Name != "myrelease0" { + t.Errorf("unexpected releases[0].name: expected=myrelease0, got=%s", st.Releases[0].Name) + } + if st.Releases[1].Name != "myrelease1" { + t.Errorf("unexpected releases[1].name: expected=myrelease1, got=%s", st.Releases[1].Name) + } + if st.Releases[2].Name != "myrelease1" { + t.Errorf("unexpected releases[2].name: expected=myrelease1, got=%s", st.Releases[2].Name) + } + if st.Releases[2].Values[0] != "{{`{{.Release.Name}}`}}/values.yaml" { + t.Errorf("unexpected releases[2].missingFileHandler: expected={{`{{.Release.Name}}`}}/values.yaml, got=%s", st.Releases[1].Values[0]) + } + if *st.Releases[2].MissingFileHandler != "Warn" { + t.Errorf("unexpected releases[2].missingFileHandler: expected=Warn, got=%s", *st.Releases[1].MissingFileHandler) + } + + if st.Releases[2].Values[0] != "{{`{{.Release.Name}}`}}/values.yaml" { + t.Errorf("unexpected releases[2].missingFileHandler: expected={{`{{.Release.Name}}`}}/values.yaml, got=%s", st.Releases[1].Values[0]) + } + + if st.HelmDefaults.KubeContext != "FOO" { + t.Errorf("unexpected helmDefaults.kubeContext: expected=FOO, got=%s", st.HelmDefaults.KubeContext) + } +} + +func TestLoadDesiredStateFromYaml_MultiPartTemplate_WithReverse(t *testing.T) { + yamlFile := "/path/to/yaml/file" + yamlContent := []byte(` +{{ readFile "templates.yaml" }} + +releases: +- name: myrelease0 + chart: mychart0 +- name: myrelease1 + chart: mychart1 + <<: *default +--- + +{{ readFile "templates.yaml" }} + +releases: +- name: myrelease2 + chart: mychart2 +- name: myrelease3 + chart: mychart3 + <<: *default +`) + files := map[string][]byte{ + yamlFile: yamlContent, + "/path/to/yaml/templates.yaml": []byte(`templates: + default: &default + missingFileHandler: Warn + values: ["` + "{{`" + `{{.Release.Name}}` + "`}}" + `/values.yaml"] +`), + } + readFile := func(filename string) ([]byte, error) { + content, ok := files[filename] + if !ok { + return nil, fmt.Errorf("unexpected filename: %s", filename) + } + return content, nil + } + app := &App{ + readFile: readFile, + glob: filepath.Glob, + abs: filepath.Abs, + Env: "default", + Logger: helmexec.NewLogger(os.Stderr, "debug"), + Reverse: true, + } + st, err := app.loadDesiredStateFromYaml(yamlFile) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if st.Releases[0].Name != "myrelease3" { + t.Errorf("unexpected releases[0].name: expected=myrelease3, got=%s", st.Releases[0].Name) + } + if st.Releases[1].Name != "myrelease2" { + t.Errorf("unexpected releases[0].name: expected=myrelease2, got=%s", st.Releases[1].Name) + } + if st.Releases[2].Name != "myrelease1" { + t.Errorf("unexpected releases[0].name: expected=myrelease1, got=%s", st.Releases[2].Name) + } + if st.Releases[3].Name != "myrelease0" { + t.Errorf("unexpected releases[0].name: expected=myrelease0, got=%s", st.Releases[3].Name) + } +} diff --git a/pkg/app/desired_state_file_loader.go b/pkg/app/desired_state_file_loader.go index c8010d6a..5f3c5e01 100644 --- a/pkg/app/desired_state_file_loader.go +++ b/pkg/app/desired_state_file_loader.go @@ -8,8 +8,6 @@ import ( "github.com/roboll/helmfile/environment" "github.com/roboll/helmfile/state" "go.uber.org/zap" - "log" - "os" "path/filepath" "sort" ) @@ -29,7 +27,34 @@ type desiredStateLoader struct { } func (ld *desiredStateLoader) Load(f string) (*state.HelmState, error) { - return ld.loadFile(filepath.Dir(f), filepath.Base(f), true) + st, err := ld.loadFile(filepath.Dir(f), filepath.Base(f), true) + if err != nil { + return nil, err + } + + if ld.Reverse { + rev := func(i, j int) bool { + return j < i + } + sort.Slice(st.Releases, rev) + sort.Slice(st.Helmfiles, rev) + } + + if ld.KubeContext != "" { + if st.HelmDefaults.KubeContext != "" { + return nil, errors.New("err: Cannot use option --kube-context and set attribute helmDefaults.kubeContext.") + } + st.HelmDefaults.KubeContext = ld.KubeContext + } + + if ld.namespace != "" { + if st.Namespace != "" { + return nil, errors.New("err: Cannot use option --namespace and set attribute namespace.") + } + st.Namespace = ld.namespace + } + + return st, nil } func (ld *desiredStateLoader) loadFile(baseDir, file string, evaluateBases bool) (*state.HelmState, error) { @@ -62,15 +87,21 @@ func (ld *desiredStateLoader) loadFile(baseDir, file string, evaluateBases bool) baseDir, file, evaluateBases, + nil, ) } return self, err } -func (a *desiredStateLoader) load(yaml []byte, baseDir, file string, evaluateBases bool) (*state.HelmState, error) { +func (a *desiredStateLoader) underlying() *state.StateCreator { c := state.NewCreator(a.logger, a.readFile, a.abs) - st, err := c.ParseAndLoadEnv(yaml, baseDir, file, a.env) + c.LoadFile = a.loadFile + return c +} + +func (a *desiredStateLoader) load(yaml []byte, baseDir, file string, evaluateBases bool, env *environment.Environment) (*state.HelmState, error) { + st, err := a.underlying().ParseAndLoad(yaml, baseDir, file, a.env, evaluateBases, env) if err != nil { return nil, err } @@ -94,62 +125,10 @@ func (a *desiredStateLoader) load(yaml []byte, baseDir, file string, evaluateBas newHelmfile.Path = match helmfiles = append(helmfiles, newHelmfile) } - } st.Helmfiles = helmfiles - if a.Reverse { - rev := func(i, j int) bool { - return j < i - } - sort.Slice(st.Releases, rev) - sort.Slice(st.Helmfiles, rev) - } - - if a.KubeContext != "" { - if st.HelmDefaults.KubeContext != "" { - log.Printf("err: Cannot use option --kube-context and set attribute helmDefaults.kubeContext.") - os.Exit(1) - } - st.HelmDefaults.KubeContext = a.KubeContext - } - if a.namespace != "" { - if st.Namespace != "" { - log.Printf("err: Cannot use option --namespace and set attribute namespace.") - os.Exit(1) - } - st.Namespace = a.namespace - } - - if err != nil { - return nil, err - } - - if !evaluateBases { - if len(st.Bases) > 0 { - return nil, errors.New("nested `base` helmfile is unsupported. please submit a feature request if you need this!") - } - - return st, nil - } - - layers := []*state.HelmState{} - for _, b := range st.Bases { - base, err := a.loadFile(baseDir, b, false) - if err != nil { - return nil, err - } - layers = append(layers, base) - } - layers = append(layers, st) - - for i := 1; i < len(layers); i++ { - if err := mergo.Merge(layers[0], layers[i], mergo.WithAppendSlice); err != nil { - return nil, err - } - } - - return layers[0], nil + return st, nil } func (ld *desiredStateLoader) renderAndLoad(baseDir, filename string, content []byte, evaluateBases bool) (*state.HelmState, error) { @@ -181,6 +160,7 @@ func (ld *desiredStateLoader) renderAndLoad(baseDir, filename string, content [] baseDir, filename, evaluateBases, + env, ) if err != nil { return nil, err diff --git a/pkg/app/two_pass_renderer.go b/pkg/app/two_pass_renderer.go index 8c372b4b..26095842 100644 --- a/pkg/app/two_pass_renderer.go +++ b/pkg/app/two_pass_renderer.go @@ -3,7 +3,6 @@ package app import ( "bytes" "fmt" - "github.com/imdario/mergo" "github.com/roboll/helmfile/environment" "github.com/roboll/helmfile/state" "github.com/roboll/helmfile/tmpl" @@ -32,10 +31,10 @@ func (r *desiredStateLoader) renderEnvironment(firstPassEnv environment.Environm return firstPassEnv } } - c := state.NewCreator(r.logger, r.readFile, r.abs) + c := r.underlying() c.Strict = false // create preliminary state, as we may have an environment. Tolerate errors. - prestate, err := c.ParseAndLoadEnv(yamlBuf.Bytes(), baseDir, filename, r.env) + prestate, err := c.ParseAndLoad(yamlBuf.Bytes(), baseDir, filename, r.env, false, &firstPassEnv) if err != nil && r.logger != nil { switch err.(type) { case *state.StateLoadError: @@ -45,16 +44,7 @@ func (r *desiredStateLoader) renderEnvironment(firstPassEnv environment.Environm } if prestate != nil { - intEnv := environment.Environment{Name: firstPassEnv.Name} - if err := mergo.Merge(&intEnv, &firstPassEnv, mergo.WithAppendSlice); err != nil { - r.logger.Debugf("error in first-pass rendering: result of \"%s\": %v", filename, err) - return firstPassEnv - } - if err := mergo.Merge(&intEnv, &prestate.Env, mergo.WithAppendSlice); err != nil { - r.logger.Debugf("error in first-pass rendering: result of \"%s\": %v", filename, err) - return firstPassEnv - } - firstPassEnv = intEnv + firstPassEnv = prestate.Env } return firstPassEnv } diff --git a/state/create.go b/state/create.go index b68b0c00..3f122522 100644 --- a/state/create.go +++ b/state/create.go @@ -2,6 +2,7 @@ package state import ( "bytes" + "errors" "fmt" "io" "os" @@ -32,16 +33,18 @@ func (e *UndefinedEnvError) Error() string { return e.msg } -type creator struct { +type StateCreator struct { logger *zap.SugaredLogger readFile func(string) ([]byte, error) abs func(string) (string, error) Strict bool + + LoadFile func(baseDir, file string, evaluateBases bool) (*HelmState, error) } -func NewCreator(logger *zap.SugaredLogger, readFile func(string) ([]byte, error), abs func(string) (string, error)) *creator { - return &creator{ +func NewCreator(logger *zap.SugaredLogger, readFile func(string) ([]byte, error), abs func(string) (string, error)) *StateCreator { + return &StateCreator{ logger: logger, readFile: readFile, abs: abs, @@ -49,8 +52,8 @@ func NewCreator(logger *zap.SugaredLogger, readFile func(string) ([]byte, error) } } -// Parses YAML into HelmState, while loading environment values files relative to the `cwd` -func (c *creator) ParseAndLoadEnv(content []byte, baseDir, file string, env string) (*HelmState, error) { +// Parse parses YAML into HelmState +func (c *StateCreator) Parse(content []byte, baseDir, file string) (*HelmState, error) { var state HelmState state.FilePath = file @@ -94,12 +97,6 @@ func (c *creator) ParseAndLoadEnv(content []byte, baseDir, file string, env stri state.logger = c.logger - e, err := state.loadEnv(env, c.readFile) - if err != nil { - return nil, &StateLoadError{fmt.Sprintf("failed to read %s", file), err} - } - state.Env = *e - state.readFile = c.readFile state.removeFile = os.Remove state.fileExists = func(path string) (bool, error) { @@ -117,7 +114,61 @@ func (c *creator) ParseAndLoadEnv(content []byte, baseDir, file string, env stri return &state, nil } -func (st *HelmState) loadEnv(name string, readFile func(string) ([]byte, error)) (*environment.Environment, error) { +// LoadEnvValues loads environment values files relative to the `baseDir` +func (c *StateCreator) LoadEnvValues(target *HelmState, env string, ctxEnv *environment.Environment) (*HelmState, error) { + state := *target + + e, err := state.loadEnvValues(env, ctxEnv, c.readFile) + if err != nil { + return nil, &StateLoadError{fmt.Sprintf("failed to read %s", state.FilePath), err} + } + state.Env = *e + + return &state, nil +} + +// Parses YAML into HelmState, while loading environment values files relative to the `baseDir` +func (c *StateCreator) ParseAndLoad(content []byte, baseDir, file string, envName string, evaluateBases bool, envValues *environment.Environment) (*HelmState, error) { + state, err := c.Parse(content, baseDir, file) + if err != nil { + return nil, err + } + + if !evaluateBases { + if len(state.Bases) > 0 { + return nil, errors.New("nested `base` helmfile is unsupported. please submit a feature request if you need this!") + } + } + + state, err = c.loadBases(state, baseDir) + if err != nil { + return nil, err + } + + return c.LoadEnvValues(state, envName, envValues) +} + +func (c *StateCreator) loadBases(st *HelmState, baseDir string) (*HelmState, error) { + layers := []*HelmState{} + for _, b := range st.Bases { + base, err := c.LoadFile(baseDir, b, false) + if err != nil { + return nil, err + } + layers = append(layers, base) + } + layers = append(layers, st) + + for i := 1; i < len(layers); i++ { + if err := mergo.Merge(layers[0], layers[i], mergo.WithAppendSlice); err != nil { + return nil, err + } + } + + return layers[0], nil +} + +func (st *HelmState) loadEnvValues(name string, ctxEnv *environment.Environment, readFile func(string) ([]byte, error)) (*environment.Environment, error) { envVals := map[string]interface{}{} envSpec, ok := st.Environments[name] if ok { @@ -172,9 +223,21 @@ func (st *HelmState) loadEnv(name string, readFile func(string) ([]byte, error)) } } } - } else if name != DefaultEnv { + } else if ctxEnv == nil && name != DefaultEnv { return nil, &UndefinedEnvError{msg: fmt.Sprintf("environment \"%s\" is not defined", name)} } - return &environment.Environment{Name: name, Values: envVals}, nil + newEnv := &environment.Environment{Name: name, Values: envVals} + + if ctxEnv != nil { + intEnv := *ctxEnv + + if err := mergo.Merge(&intEnv, newEnv, mergo.WithAppendSlice); err != nil { + return nil, fmt.Errorf("error while merging environment values for \"%s\": %v", name, err) + } + + newEnv = &intEnv + } + + return newEnv, nil } diff --git a/state/create_test.go b/state/create_test.go index 9dfac579..bb934d2a 100644 --- a/state/create_test.go +++ b/state/create_test.go @@ -13,13 +13,13 @@ import ( ) func createFromYaml(content []byte, file string, env string, logger *zap.SugaredLogger) (*HelmState, error) { - c := &creator{ - logger, - ioutil.ReadFile, - filepath.Abs, - true, + c := &StateCreator{ + logger: logger, + readFile: ioutil.ReadFile, + abs: filepath.Abs, + Strict: true, } - return c.ParseAndLoadEnv(content, filepath.Dir(file), file, env) + return c.ParseAndLoad(content, filepath.Dir(file), file, env, false, nil) } func TestReadFromYaml(t *testing.T) { @@ -113,7 +113,7 @@ bar: {{ readFile "bar.txt" }} return nil, fmt.Errorf("unexpected filename: %s", filename) } - state, err := NewCreator(logger, readFile, filepath.Abs).ParseAndLoadEnv(yamlContent, filepath.Dir(yamlFile), yamlFile, "production") + state, err := NewCreator(logger, readFile, filepath.Abs).ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", false, nil) if err != nil { t.Errorf("unexpected error: %v", err) }