From 923bd54db05007ddb966b2e87f2e545e95bd07ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Victor=20No=C3=ABl?= Date: Sat, 2 May 2020 02:35:51 +0200 Subject: [PATCH] Use namespace for release unicity with helm3 (#1213) (#1235) This is for #1213 so that the releases are considered scoped to their namespace and not to tiller namespace. Fixes #1213 --- pkg/app/app.go | 18 ++-- pkg/app/app_test.go | 185 +++++++++++++++++++++++++++++++++++++++++- pkg/app/mocks_test.go | 9 ++ 3 files changed, 204 insertions(+), 8 deletions(-) diff --git a/pkg/app/app.go b/pkg/app/app.go index 73227991..8cc18155 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -699,7 +699,7 @@ func (a *App) visitStatesWithSelectorsAndRemoteSupport(fileOrDir string, converg return a.visitStates(fileOrDir, opts, converge) } -func processFilteredReleases(st *state.HelmState, converge func(st *state.HelmState) []error) (bool, []error) { +func processFilteredReleases(st *state.HelmState, helm helmexec.Interface, converge func(st *state.HelmState) []error) (bool, []error) { if len(st.Selectors) > 0 { err := st.FilterReleases() if err != nil { @@ -713,11 +713,15 @@ func processFilteredReleases(st *state.HelmState, converge func(st *state.HelmSt releaseNameCounts := map[Key]int{} for _, r := range st.Releases { - tillerNamespace := st.HelmDefaults.TillerNamespace - if r.TillerNamespace != "" { - tillerNamespace = r.TillerNamespace + namespace := r.Namespace + if !helm.IsHelm3() { + if r.TillerNamespace != "" { + namespace = r.TillerNamespace + } else { + namespace = st.HelmDefaults.TillerNamespace + } } - releaseNameCounts[Key{tillerNamespace, r.Name}]++ + releaseNameCounts[Key{namespace, r.Name}]++ } for name, c := range releaseNameCounts { if c > 1 { @@ -734,7 +738,7 @@ func processFilteredReleases(st *state.HelmState, converge func(st *state.HelmSt func (a *App) Wrap(converge func(*state.HelmState, helmexec.Interface) []error) func(st *state.HelmState, helm helmexec.Interface) (bool, []error) { return func(st *state.HelmState, helm helmexec.Interface) (bool, []error) { - return processFilteredReleases(st, func(st *state.HelmState) []error { + return processFilteredReleases(st, helm, func(st *state.HelmState) []error { return converge(st, helm) }) } @@ -742,7 +746,7 @@ func (a *App) Wrap(converge func(*state.HelmState, helmexec.Interface) []error) func (a *App) VisitDesiredStatesWithReleasesFiltered(fileOrDir string, converge func(*state.HelmState) []error, o ...LoadOption) error { return a.visitStatesWithSelectorsAndRemoteSupport(fileOrDir, func(st *state.HelmState) (bool, []error) { - return processFilteredReleases(st, converge) + return processFilteredReleases(st, a.getHelm(st), converge) }, o...) } diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 86582bc3..555b928c 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -45,12 +45,16 @@ func injectFs(app *App, fs *testhelper.TestFs) *App { } func expectNoCallsToHelm(app *App) { + expectNoCallsToHelmVersion(app, false) +} + +func expectNoCallsToHelmVersion(app *App, isHelm3 bool) { if app.helms != nil { panic("invalid call to expectNoCallsToHelm") } app.helms = map[helmKey]helmexec.Interface{ - createHelmKey(app.OverrideHelmBinary, app.OverrideKubeContext): &noCallHelmExec{}, + createHelmKey(app.OverrideHelmBinary, app.OverrideKubeContext): &versionOnlyHelmExec{isHelm3: isHelm3}, } } @@ -1243,6 +1247,178 @@ releases: } } +// See https://github.com/roboll/helmfile/issues/1213 +func TestVisitDesiredStatesWithReleases_DuplicateReleasesHelm2(t *testing.T) { + files := map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: foo + namespace: foo + chart: charts/foo +- name: foo + namespace: bar + chart: charts/foo +`, + } + + actual := []state.ReleaseSpec{} + + collectReleases := func(st *state.HelmState) []error { + for _, r := range st.Releases { + actual = append(actual, r) + } + return []error{} + } + app := appWithFs(&App{ + OverrideHelmBinary: DefaultHelmBinary, + OverrideKubeContext: "default", + Logger: helmexec.NewLogger(os.Stderr, "debug"), + Namespace: "", + Env: "default", + }, files) + + expectNoCallsToHelmVersion(app, false) + + err := app.VisitDesiredStatesWithReleasesFiltered( + "helmfile.yaml", collectReleases, + ) + + expected := "in ./helmfile.yaml: duplicate release \"foo\" found in \"\": there were 2 releases named \"foo\" matching specified selector" + if err == nil { + t.Errorf("error expected but not happened") + } else if err.Error() != expected { + t.Errorf("unexpected error message: expected=\"%s\", actual=\"%s\"", expected, err.Error()) + } +} + +// See https://github.com/roboll/helmfile/issues/1213 +func TestVisitDesiredStatesWithReleases_NoDuplicateReleasesHelm2(t *testing.T) { + files := map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: foo + namespace: foo + tillerNamespace: tns1 + chart: charts/foo +- name: foo + namespace: bar + tillerNamespace: tns2 + chart: charts/foo +`, + } + + actual := []state.ReleaseSpec{} + + collectReleases := func(st *state.HelmState) []error { + for _, r := range st.Releases { + actual = append(actual, r) + } + return []error{} + } + app := appWithFs(&App{ + OverrideHelmBinary: DefaultHelmBinary, + OverrideKubeContext: "default", + Logger: helmexec.NewLogger(os.Stderr, "debug"), + Namespace: "", + Env: "default", + }, files) + + expectNoCallsToHelmVersion(app, false) + + err := app.VisitDesiredStatesWithReleasesFiltered( + "helmfile.yaml", collectReleases, + ) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// See https://github.com/roboll/helmfile/issues/1213 +func TestVisitDesiredStatesWithReleases_NoDuplicateReleasesHelm3(t *testing.T) { + files := map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: foo + namespace: foo + chart: charts/foo +- name: foo + namespace: bar + chart: charts/foo +`, + } + + actual := []state.ReleaseSpec{} + + collectReleases := func(st *state.HelmState) []error { + for _, r := range st.Releases { + actual = append(actual, r) + } + return []error{} + } + app := appWithFs(&App{ + OverrideHelmBinary: DefaultHelmBinary, + OverrideKubeContext: "default", + Logger: helmexec.NewLogger(os.Stderr, "debug"), + Namespace: "", + Env: "default", + }, files) + + expectNoCallsToHelmVersion(app, true) + + err := app.VisitDesiredStatesWithReleasesFiltered( + "helmfile.yaml", collectReleases, + ) + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +// See https://github.com/roboll/helmfile/issues/1213 +func TestVisitDesiredStatesWithReleases_DuplicateReleasesHelm3(t *testing.T) { + files := map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: foo + namespace: foo + chart: charts/foo +- name: foo + namespace: foo + chart: charts/foo +`, + } + + actual := []state.ReleaseSpec{} + + collectReleases := func(st *state.HelmState) []error { + for _, r := range st.Releases { + actual = append(actual, r) + } + return []error{} + } + app := appWithFs(&App{ + OverrideHelmBinary: DefaultHelmBinary, + OverrideKubeContext: "default", + Logger: helmexec.NewLogger(os.Stderr, "debug"), + Namespace: "", + Env: "default", + }, files) + + expectNoCallsToHelmVersion(app, true) + + err := app.VisitDesiredStatesWithReleasesFiltered( + "helmfile.yaml", collectReleases, + ) + + expected := "in ./helmfile.yaml: duplicate release \"foo\" found in \"foo\": there were 2 releases named \"foo\" matching specified selector" + if err == nil { + t.Errorf("error expected but not happened") + } else if err.Error() != expected { + t.Errorf("unexpected error message: expected=\"%s\", actual=\"%s\"", expected, err.Error()) + } +} + func TestLoadDesiredStateFromYaml_DuplicateReleaseName(t *testing.T) { yamlFile := "example/path/to/yaml/file" yamlContent := []byte(`releases: @@ -3751,6 +3927,8 @@ releases: Namespace: "testNamespace", }, files) + expectNoCallsToHelm(app) + out := captureStdout(func() { err := app.PrintState(configImpl{}) assert.NilError(t, err) @@ -3795,6 +3973,9 @@ releases: Logger: logger, Namespace: "testNamespace", }, files) + + expectNoCallsToHelm(app) + out := captureStdout(func() { err := app.PrintState(configImpl{}) assert.NilError(t, err) @@ -3951,6 +4132,8 @@ releases: Env: "default", }, files) + expectNoCallsToHelm(app) + var specs []state.ReleaseSpec collectReleases := func(st *state.HelmState) []error { specs = append(specs, st.Releases...) diff --git a/pkg/app/mocks_test.go b/pkg/app/mocks_test.go index fdd45d4d..8d8e7cd0 100644 --- a/pkg/app/mocks_test.go +++ b/pkg/app/mocks_test.go @@ -5,6 +5,15 @@ import "github.com/roboll/helmfile/pkg/helmexec" type noCallHelmExec struct { } +type versionOnlyHelmExec struct { + *noCallHelmExec + isHelm3 bool +} + +func (helm *versionOnlyHelmExec) IsHelm3() bool { + return helm.isHelm3 +} + func (helm *noCallHelmExec) doPanic() { panic("unexpected call to helm") }