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
This commit is contained in:
Yusuke Kuoka 2021-12-18 17:44:55 +09:00 committed by GitHub
parent 3d7b4287b3
commit 9efb7afb47
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 289 additions and 74 deletions

2
go.mod
View File

@ -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

2
go.sum
View File

@ -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=

View File

@ -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{}

View File

@ -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=<nil>
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=<nil>
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=<nil>
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"
`,
},
}

View File

@ -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=<nil>
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"?
`,
},
}

View File

@ -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=<nil>
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=<nil>
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"?
`,
},
}

View File

@ -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))
}
}

View File

@ -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
}