From 9efb7afb4771d60b1a819a495c3f0f725ab33c41 Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Sat, 18 Dec 2021 17:44:55 +0900 Subject: [PATCH] Do fail on a possible typo in `needs` entries (#2026) * Do fail on a possible typo in `needs` entries Helmfile kindly fails with a friendly error when you made a typo in a `needs` entry, i.e. a `needs` entry included a reference to a release that is not defined in the helmfile config. Example Output: ``` in ./helmfile.needs.yaml: release(s) "app" depend(s) on an undefined release "infrastructure/cert-manager2". Perhaps you made a typo in `needs` or forgot defining a release named "cert-manager2" with appropriate `namespace` and `kubeContext`? ``` This prevents issues like #1959 * Fix regression in helmfile-diff (This may break when you had two or more duplicated releases that are intended to be de-duplicated before DAG calculation using selectors * Fix regression when you used selector to deduplicate releases before DAG calculation * Comments * Fix regressions in helmfile-apply and helmfile-sync * Fix regression in duplicate release detection --- go.mod | 2 +- go.sum | 2 + pkg/app/app.go | 108 ++++++++++++------------ pkg/app/app_test.go | 147 +++++++++++++++++++++++++++++++-- pkg/app/diff_nokubectx_test.go | 4 +- pkg/app/diff_test.go | 66 ++++++++++++++- pkg/state/state.go | 5 +- pkg/state/state_run.go | 29 +++++-- 8 files changed, 289 insertions(+), 74 deletions(-) diff --git a/go.mod b/go.mod index 4a226e0d..f5bdf2a4 100644 --- a/go.mod +++ b/go.mod @@ -29,7 +29,7 @@ require ( github.com/tatsushid/go-prettytable v0.0.0-20141013043238-ed2d14c29939 github.com/urfave/cli v1.22.5 github.com/variantdev/chartify v0.9.1 - github.com/variantdev/dag v1.0.0 + github.com/variantdev/dag v1.1.0 github.com/variantdev/vals v0.14.0 go.uber.org/multierr v1.6.0 go.uber.org/zap v1.16.0 diff --git a/go.sum b/go.sum index 559805a8..ea287922 100644 --- a/go.sum +++ b/go.sum @@ -602,6 +602,8 @@ github.com/variantdev/chartify v0.9.1 h1:Mkq2BvtHr42BYf2a14YWkfeSoY/+d1wL2xfap4q github.com/variantdev/chartify v0.9.1/go.mod h1:qF4XzQlkfH/6k2jAi1hLas+lK4zSCa8kY+r5JdmLA68= github.com/variantdev/dag v1.0.0 h1:7SFjATxHtrYV20P3tx53yNDBMegz6RT4jv8JPHqAHdM= github.com/variantdev/dag v1.0.0/go.mod h1:pH1TQsNSLj2uxMo9NNl9zdGy01Wtn+/2MT96BrKmVyE= +github.com/variantdev/dag v1.1.0 h1:xodYlSng33KWGvIGMpKUyLcIZRXKiNUx612mZJqYrDg= +github.com/variantdev/dag v1.1.0/go.mod h1:pH1TQsNSLj2uxMo9NNl9zdGy01Wtn+/2MT96BrKmVyE= github.com/variantdev/vals v0.14.0 h1:J+zX1DDLTQ35A198HChVkCwLfA/4OeKhSuZ5zjmgV0Q= github.com/variantdev/vals v0.14.0/go.mod h1:zoPna6p+fM7pC2Lo17dl5sRpamAV2a0j2VDslkMLW0M= github.com/vektra/mockery v1.1.2/go.mod h1:VcfZjKaFOPO+MpN4ZvwPjs4c48lkq1o3Ym8yHZJu0jU= diff --git a/pkg/app/app.go b/pkg/app/app.go index 7b93c838..4b0beeb1 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1149,29 +1149,61 @@ func (a *App) getSelectedReleases(r *Run, includeTransitiveNeeds bool) ([]state. return nil, nil, err } - selectedIds := map[string]struct{}{} + selectedIds := map[string]state.ReleaseSpec{} + selectedCounts := map[string]int{} for _, r := range selected { - selectedIds[state.ReleaseToID(&r)] = struct{}{} + r := r + id := state.ReleaseToID(&r) + selectedIds[id] = r + selectedCounts[id]++ + + if dupCount := selectedCounts[id]; dupCount > 1 { + return nil, nil, fmt.Errorf("found %d duplicate releases with ID %q", dupCount, id) + } } allReleases := r.state.GetReleasesWithOverrides() - needed := map[string]struct{}{} - for _, r := range selected { - collectNeeds(r, selectedIds, needed, allReleases, includeTransitiveNeeds) - } - - var releases []state.ReleaseSpec - - releases = append(releases, selected...) - + groupsByID := map[string][]*state.ReleaseSpec{} for _, r := range allReleases { - if _, ok := needed[state.ReleaseToID(&r)]; ok { - releases = append(releases, r) - } + r := r + groupsByID[state.ReleaseToID(&r)] = append(groupsByID[state.ReleaseToID(&r)], &r) } - if err := checkDuplicates(r.helm, r.state, releases); err != nil { + var deduplicated []state.ReleaseSpec + + dedupedBefore := map[string]struct{}{} + + // We iterate over allReleases rather than groupsByID + // to preserve the order of releases + for _, seq := range allReleases { + id := state.ReleaseToID(&seq) + + rs := groupsByID[id] + + if len(rs) == 1 { + deduplicated = append(deduplicated, *rs[0]) + continue + } + + if _, ok := dedupedBefore[id]; ok { + continue + } + + // We keep the selected one only when there were two or more duplicate + // releases in the helmfile config. + // Otherwise we can't compute the DAG of releases correctly. + r, deduped := selectedIds[id] + if !deduped { + panic(fmt.Errorf("assertion error: release %q has never been selected. This shouldn't happen!", id)) + } + + deduplicated = append(deduplicated, r) + + dedupedBefore[id] = struct{}{} + } + + if err := checkDuplicates(r.helm, r.state, deduplicated); err != nil { return nil, nil, err } @@ -1183,42 +1215,7 @@ func (a *App) getSelectedReleases(r *Run, includeTransitiveNeeds bool) ([]state. a.Logger.Debugf("%d release(s)%s found in %s\n", len(selected), extra, r.state.FilePath) - return selected, releases, nil -} - -func collectNeeds(release state.ReleaseSpec, selectedIds map[string]struct{}, needed map[string]struct{}, allReleases []state.ReleaseSpec, includeTransitiveNeeds bool) { - for _, id := range release.Needs { - // Avoids duplicating a release that is selected AND also needed by another selected release - if _, ok := selectedIds[id]; !ok { - if _, ok := needed[id]; !ok { - needed[id] = struct{}{} - if includeTransitiveNeeds { - releaseParts := strings.Split(id, "/") - releasePartsCount := len(releaseParts) - releaseName := releaseParts[releasePartsCount-1] - releaseNamespace := "" - releaseKubeContext := "" - if releasePartsCount > 1 { - releaseNamespace = releaseParts[releasePartsCount-2] - } - if releasePartsCount > 2 { - releaseKubeContext = releaseParts[releasePartsCount-3] - } - for _, r := range allReleases { - if len(releaseNamespace) > 0 && r.Namespace != releaseNamespace { - continue - } - if len(releaseKubeContext) > 0 && r.KubeContext != releaseKubeContext { - continue - } - if r.Name == releaseName { - collectNeeds(r, selectedIds, needed, allReleases, includeTransitiveNeeds) - } - } - } - } - } - } + return selected, deduplicated, nil } func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) { @@ -1323,6 +1320,9 @@ Do you really want to apply? affectedReleases := state.AffectedReleases{} + // Traverse DAG of all the releases so that we don't suffer from false-positive missing dependencies + st.Releases = selectedAndNeededReleases + if !interactive || interactive && r.askForConfirmation(confMsg) { r.helm.SetExtraArgs(argparser.GetArgs(c.Args(), r.state)...) @@ -1458,7 +1458,7 @@ Do you really want to delete? func (a *App) diff(r *Run, c DiffConfigProvider) (*string, bool, bool, []error) { st := r.state - selectedReleases, selectedAndNeededReleases, err := a.getSelectedReleases(r, false) + selectedReleases, deduplicatedReleases, err := a.getSelectedReleases(r, false) if err != nil { return nil, false, false, []error{err} } @@ -1477,7 +1477,7 @@ func (a *App) diff(r *Run, c DiffConfigProvider) (*string, bool, bool, []error) SkipDiffOnInstall: c.SkipDiffOnInstall(), } - st.Releases = selectedAndNeededReleases + st.Releases = deduplicatedReleases plan, err := st.PlanReleases(state.PlanOptions{Reverse: false, SelectedReleases: selectedReleases, SkipNeeds: c.SkipNeeds(), IncludeNeeds: c.IncludeNeeds(), IncludeTransitiveNeeds: false}) if err != nil { @@ -1731,7 +1731,7 @@ func (a *App) sync(r *Run, c SyncConfigProvider) (bool, []error) { r.helm.SetExtraArgs(argparser.GetArgs(c.Args(), r.state)...) // Traverse DAG of all the releases so that we don't suffer from false-positive missing dependencies - st.Releases = toSyncWithNeeds + st.Releases = selectedAndNeededReleases affectedReleases := state.AffectedReleases{} diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 47d4c07e..905f4894 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -4213,6 +4213,69 @@ merged environment: &{default map[] map[]} // // error cases // + { + name: "unselected release in needs", + loc: location(), + selectors: []string{"name=foo"}, + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: bar + namespace: ns1 + chart: mychart3 +- name: foo + chart: mychart1 + needs: + - ns1/bar +`, + }, + diffs: map[exectest.DiffKey]error{ + exectest.DiffKey{Name: "baz", Chart: "mychart3", Flags: "--kube-contextdefault--namespacens1--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + exectest.DiffKey{Name: "foo", Chart: "mychart1", Flags: "--kube-contextdefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + lists: map[exectest.ListKey]string{}, + upgraded: []exectest.Release{}, + deleted: []exectest.Release{}, + concurrency: 1, + error: `in ./helmfile.yaml: release "default//foo" depends on "default/ns1/bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies`, + log: `processing file "helmfile.yaml" in directory "." +first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= +first-pass uses: &{default map[] map[]} +first-pass rendering output of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: bar + 3: namespace: ns1 + 4: chart: mychart3 + 5: - name: foo + 6: chart: mychart1 + 7: needs: + 8: - ns1/bar + 9: + +first-pass produced: &{default map[] map[]} +first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]} +vals: +map[] +defaultVals:[] +second-pass rendering result of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: bar + 3: namespace: ns1 + 4: chart: mychart3 + 5: - name: foo + 6: chart: mychart1 + 7: needs: + 8: - ns1/bar + 9: + +merged environment: &{default map[] map[]} +1 release(s) matching name=foo found in helmfile.yaml + +err: release "default//foo" depends on "default/ns1/bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies +`, + }, { name: "non-existent release in needs", loc: location(), @@ -4225,18 +4288,18 @@ releases: - name: foo chart: mychart1 needs: - - bar + - ns1/bar `, }, diffs: map[exectest.DiffKey]error{ - exectest.DiffKey{Name: "baz", Chart: "mychart3", Flags: "--kube-contextdefault--namespacens1--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + exectest.DiffKey{Name: "bar", Chart: "mychart3", Flags: "--kube-contextdefault--namespacens1--detailed-exitcode"}: helmexec.ExitError{Code: 2}, exectest.DiffKey{Name: "foo", Chart: "mychart1", Flags: "--kube-contextdefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, }, lists: map[exectest.ListKey]string{}, upgraded: []exectest.Release{}, deleted: []exectest.Release{}, concurrency: 1, - error: `in ./helmfile.yaml: release "default//foo" depends on "default//bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies`, + error: "in ./helmfile.yaml: release(s) \"default//foo\" depend(s) on an undefined release \"default/ns1/bar\". Perhaps you made a typo in \"needs\" or forgot defining a release named \"bar\" with appropriate \"namespace\" and \"kubeContext\"?", log: `processing file "helmfile.yaml" in directory "." first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= first-pass uses: &{default map[] map[]} @@ -4249,7 +4312,7 @@ first-pass rendering output of "helmfile.yaml.part.0": 5: - name: foo 6: chart: mychart1 7: needs: - 8: - bar + 8: - ns1/bar 9: first-pass produced: &{default map[] map[]} @@ -4266,13 +4329,85 @@ second-pass rendering result of "helmfile.yaml.part.0": 5: - name: foo 6: chart: mychart1 7: needs: - 8: - bar + 8: - ns1/bar 9: merged environment: &{default map[] map[]} 2 release(s) found in helmfile.yaml -err: release "default//foo" depends on "default//bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies +err: release(s) "default//foo" depend(s) on an undefined release "default/ns1/bar". Perhaps you made a typo in "needs" or forgot defining a release named "bar" with appropriate "namespace" and "kubeContext"? +`, + }, + { + name: "duplicate releases", + loc: location(), + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: bar + namespace: ns1 + chart: mychart3 +- name: foo + chart: mychart2 + needs: + - ns1/bar +- name: foo + chart: mychart1 + needs: + - ns1/bar +`, + }, + diffs: map[exectest.DiffKey]error{ + exectest.DiffKey{Name: "bar", Chart: "mychart3", Flags: "--kube-contextdefault--namespacens1--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + exectest.DiffKey{Name: "foo", Chart: "mychart1", Flags: "--kube-contextdefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + lists: map[exectest.ListKey]string{}, + upgraded: []exectest.Release{}, + deleted: []exectest.Release{}, + concurrency: 1, + error: "in ./helmfile.yaml: found 2 duplicate releases with ID \"default//foo\"", + log: `processing file "helmfile.yaml" in directory "." +first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= +first-pass uses: &{default map[] map[]} +first-pass rendering output of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: bar + 3: namespace: ns1 + 4: chart: mychart3 + 5: - name: foo + 6: chart: mychart2 + 7: needs: + 8: - ns1/bar + 9: - name: foo +10: chart: mychart1 +11: needs: +12: - ns1/bar +13: + +first-pass produced: &{default map[] map[]} +first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]} +vals: +map[] +defaultVals:[] +second-pass rendering result of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: bar + 3: namespace: ns1 + 4: chart: mychart3 + 5: - name: foo + 6: chart: mychart2 + 7: needs: + 8: - ns1/bar + 9: - name: foo +10: chart: mychart1 +11: needs: +12: - ns1/bar +13: + +merged environment: &{default map[] map[]} +err: found 2 duplicate releases with ID "default//foo" `, }, } diff --git a/pkg/app/diff_nokubectx_test.go b/pkg/app/diff_nokubectx_test.go index 83e9dc14..5b31a874 100644 --- a/pkg/app/diff_nokubectx_test.go +++ b/pkg/app/diff_nokubectx_test.go @@ -1227,7 +1227,7 @@ releases: upgraded: []exectest.Release{}, deleted: []exectest.Release{}, concurrency: 1, - error: `in ./helmfile.yaml: release "foo" depends on "bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies`, + error: `in ./helmfile.yaml: release(s) "foo" depend(s) on an undefined release "bar". Perhaps you made a typo in "needs" or forgot defining a release named "bar" with appropriate "namespace" and "kubeContext"?`, log: `processing file "helmfile.yaml" in directory "." first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= first-pass uses: &{default map[] map[]} @@ -1263,7 +1263,7 @@ second-pass rendering result of "helmfile.yaml.part.0": merged environment: &{default map[] map[]} 2 release(s) found in helmfile.yaml -err: release "foo" depends on "bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies +err: release(s) "foo" depend(s) on an undefined release "bar". Perhaps you made a typo in "needs" or forgot defining a release named "bar" with appropriate "namespace" and "kubeContext"? `, }, } diff --git a/pkg/app/diff_test.go b/pkg/app/diff_test.go index 6209d9dd..2ceedc9f 100644 --- a/pkg/app/diff_test.go +++ b/pkg/app/diff_test.go @@ -1313,6 +1313,68 @@ merged environment: &{default map[] map[]} // // error cases // + { + name: "unselected release in needs", + loc: location(), + flags: flags{skipNeeds: false}, + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: bar + chart: mychart3 +- name: foo + chart: mychart1 + needs: + - bar +`, + }, + detailedExitcode: true, + selectors: []string{"name=foo"}, + diffs: map[exectest.DiffKey]error{ + exectest.DiffKey{Name: "bar", Chart: "mychart3", Flags: "--kube-contextdefault--namespacens1--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + exectest.DiffKey{Name: "foo", Chart: "mychart1", Flags: "--kube-contextdefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + lists: map[exectest.ListKey]string{}, + upgraded: []exectest.Release{}, + deleted: []exectest.Release{}, + concurrency: 1, + error: `in ./helmfile.yaml: release "default//foo" depends on "default//bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies`, + log: `processing file "helmfile.yaml" in directory "." +first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= +first-pass uses: &{default map[] map[]} +first-pass rendering output of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: bar + 3: chart: mychart3 + 4: - name: foo + 5: chart: mychart1 + 6: needs: + 7: - bar + 8: + +first-pass produced: &{default map[] map[]} +first-pass rendering result of "helmfile.yaml.part.0": {default map[] map[]} +vals: +map[] +defaultVals:[] +second-pass rendering result of "helmfile.yaml.part.0": + 0: + 1: releases: + 2: - name: bar + 3: chart: mychart3 + 4: - name: foo + 5: chart: mychart1 + 6: needs: + 7: - bar + 8: + +merged environment: &{default map[] map[]} +1 release(s) matching name=foo found in helmfile.yaml + +err: release "default//foo" depends on "default//bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies +`, + }, { name: "non-existent release in needs", loc: location(), @@ -1337,7 +1399,7 @@ releases: upgraded: []exectest.Release{}, deleted: []exectest.Release{}, concurrency: 1, - error: `in ./helmfile.yaml: release "default//foo" depends on "default//bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies`, + error: `in ./helmfile.yaml: release(s) "default//foo" depend(s) on an undefined release "default//bar". Perhaps you made a typo in "needs" or forgot defining a release named "bar" with appropriate "namespace" and "kubeContext"?`, log: `processing file "helmfile.yaml" in directory "." first-pass rendering starting for "helmfile.yaml.part.0": inherited=&{default map[] map[]}, overrode= first-pass uses: &{default map[] map[]} @@ -1373,7 +1435,7 @@ second-pass rendering result of "helmfile.yaml.part.0": merged environment: &{default map[] map[]} 2 release(s) found in helmfile.yaml -err: release "default//foo" depends on "default//bar" which does not match the selectors. Please add a selector like "--selector name=bar", or indicate whether to skip (--skip-needs) or include (--include-needs) these dependencies +err: release(s) "default//foo" depend(s) on an undefined release "default//bar". Perhaps you made a typo in "needs" or forgot defining a release named "bar" with appropriate "namespace" and "kubeContext"? `, }, } diff --git a/pkg/state/state.go b/pkg/state/state.go index 74622238..04f8fcf5 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -1888,8 +1888,11 @@ func (st *HelmState) DiffReleases(helm helmexec.Interface, additionalValues []st ) for _, p := range preps { - if stdout, ok := outputs[ReleaseToID(p.release)]; ok { + id := ReleaseToID(p.release) + if stdout, ok := outputs[id]; ok { fmt.Print(stdout.String()) + } else { + panic(fmt.Sprintf("missing output for release %s", id)) } } diff --git a/pkg/state/state_run.go b/pkg/state/state_run.go index d4695a93..038620e8 100644 --- a/pkg/state/state_run.go +++ b/pkg/state/state_run.go @@ -150,15 +150,12 @@ func GroupReleasesByDependency(releases []Release, opts PlanOptions) ([][]Releas idToReleases[id] = append(idToReleases[id], r) idToIndex[id] = i - // Only compute dependencies from non-filtered releases - if !r.Filtered { - var needs []string - for i := 0; i < len(r.Needs); i++ { - n := r.Needs[i] - needs = append(needs, n) - } - d.Add(id, dag.Dependencies(needs)) + var needs []string + for i := 0; i < len(r.Needs); i++ { + n := r.Needs[i] + needs = append(needs, n) } + d.Add(id, dag.Dependencies(needs)) } var ids []string @@ -218,6 +215,22 @@ func GroupReleasesByDependency(releases []Release, opts PlanOptions) ([][]Releas msgs[i] = msg } return nil, errors.New(msgs[0]) + } else if ude, ok := err.(*dag.UndefinedDependencyError); ok { + var quotedReleaseNames []string + for _, d := range ude.Dependents { + quotedReleaseNames = append(quotedReleaseNames, fmt.Sprintf("%q", d)) + } + + idComponents := strings.Split(ude.UndefinedNode, "/") + name := idComponents[len(idComponents)-1] + humanReadableUndefinedReleaseInfo := fmt.Sprintf(`named %q with appropriate "namespace" and "kubeContext"`, name) + + return nil, fmt.Errorf( + `release(s) %s depend(s) on an undefined release %q. Perhaps you made a typo in "needs" or forgot defining a release %s?`, + strings.Join(quotedReleaseNames, ", "), + ude.UndefinedNode, + humanReadableUndefinedReleaseInfo, + ) } return nil, err }