From 1ef9b29f6dce765e3a083e90467581ad2e6fccc6 Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Fri, 11 Dec 2020 09:09:35 +0900 Subject: [PATCH] Improve handling of releases being newly installed by helmfile-apply (#1618) This improves helmfile-apply with two things: - Some users had timing-out issues or annoyed by huge output from helm-diff run as part of helmfile-apply on first install. `--skip-diff-on-install` skips running helm-diff for releases being newly installed, so that you can avoid those issues. - Some users had difficultly or found it not straight-forward to install CRDs and custom resources from separate charts in one helmfile-apply (#1353). The new helmfile.yaml release field `disableValidationOnInstall: true` adds `--disable-validation` to helm-diff only for releases being newly released, which should mostly resolve the issue. Resolves #1353 --- main.go | 8 ++ pkg/app/app.go | 9 +- pkg/app/app_test.go | 251 +++++++++++++++++++++++++++++++++++++++++--- pkg/app/config.go | 1 + pkg/state/state.go | 59 +++++++++-- 5 files changed, 301 insertions(+), 27 deletions(-) diff --git a/main.go b/main.go index ebb8c741..7e04e0ce 100644 --- a/main.go +++ b/main.go @@ -397,6 +397,10 @@ func main() { Name: "skip-cleanup", Usage: "Stop cleaning up temporary values generated by helmfile and helm-secrets. Useful for debugging. Don't use in production for security", }, + cli.BoolFlag{ + Name: "skip-diff-on-install", + Usage: "Skips running helm-diff on releases being newly installed on this apply. Useful when the release manifests are too huge to be reviewed, or it's too time-consuming to diff at all", + }, cli.BoolFlag{ Name: "include-tests", Usage: "enable the diffing of the helm test hooks", @@ -738,6 +742,10 @@ func (c configImpl) SkipCleanup() bool { return c.c.Bool("skip-cleanup") } +func (c configImpl) SkipDiffOnInstall() bool { + return c.c.Bool("skip-diff-on-install") +} + func (c configImpl) EmbedValues() bool { return c.c.Bool("embed-values") } diff --git a/pkg/app/app.go b/pkg/app/app.go index 64eb8b45..65bc998d 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1084,10 +1084,11 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) { detailedExitCode := true diffOpts := &state.DiffOpts{ - NoColor: c.NoColor(), - Context: c.Context(), - Set: c.Set(), - SkipCleanup: c.RetainValuesFiles() || c.SkipCleanup(), + NoColor: c.NoColor(), + Context: c.Context(), + Set: c.Set(), + SkipCleanup: c.RetainValuesFiles() || c.SkipCleanup(), + SkipDiffOnInstall: c.SkipDiffOnInstall(), } infoMsg, releasesToBeUpdated, releasesToBeDeleted, errs := r.diff(false, detailedExitCode, c, diffOpts) diff --git a/pkg/app/app_test.go b/pkg/app/app_test.go index 13dd3cac..af215a45 100644 --- a/pkg/app/app_test.go +++ b/pkg/app/app_test.go @@ -2313,6 +2313,7 @@ type applyConfig struct { concurrency int detailedExitcode bool interactive bool + skipDiffOnInstall bool logger *zap.SugaredLogger } @@ -2380,6 +2381,10 @@ func (a applyConfig) RetainValuesFiles() bool { return a.retainValuesFiles } +func (a applyConfig) SkipDiffOnInstall() bool { + return a.skipDiffOnInstall +} + type depsConfig struct { skipRepos bool } @@ -2651,18 +2656,19 @@ releases: func TestApply(t *testing.T) { testcases := []struct { - name string - loc string - ns string - concurrency int - error string - files map[string]string - selectors []string - lists map[exectest.ListKey]string - diffs map[exectest.DiffKey]error - upgraded []exectest.Release - deleted []exectest.Release - log string + name string + loc string + ns string + concurrency int + skipDiffOnInstall bool + error string + files map[string]string + selectors []string + lists map[exectest.ListKey]string + diffs map[exectest.DiffKey]error + upgraded []exectest.Release + deleted []exectest.Release + log string }{ // // complex test cases for smoke testing @@ -3071,6 +3077,222 @@ baz stable/mychart3 bar stable/mychart2 foo stable/mychart1 +`, + }, + // + // install with upgrade + // + { + name: "install-with-upgrade-with-validation-control", + loc: location(), + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: baz + chart: stable/mychart3 + disableValidationOnInstall: true +- name: foo + chart: stable/mychart1 + disableValidationOnInstall: true + needs: + - bar +- name: bar + chart: stable/mychart2 + disableValidation: true +`, + }, + diffs: map[exectest.DiffKey]error{ + exectest.DiffKey{Name: "baz", Chart: "stable/mychart3", Flags: "--kube-contextdefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + exectest.DiffKey{Name: "foo", Chart: "stable/mychart1", Flags: "--disable-validation--kube-contextdefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + exectest.DiffKey{Name: "bar", Chart: "stable/mychart2", Flags: "--disable-validation--kube-contextdefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + lists: map[exectest.ListKey]string{ + exectest.ListKey{Filter: "^foo$", Flags: "--kube-contextdefault--deployed--failed--pending"}: ``, + exectest.ListKey{Filter: "^bar$", Flags: "--kube-contextdefault--deployed--failed--pending"}: `NAME REVISION UPDATED STATUS CHART APP VERSION NAMESPACE +bar 4 Fri Nov 1 08:40:07 2019 DEPLOYED mychart2-3.1.0 3.1.0 default +`, + exectest.ListKey{Filter: "^baz$", Flags: "--kube-contextdefault--deployed--failed--pending"}: `NAME REVISION UPDATED STATUS CHART APP VERSION NAMESPACE +baz 4 Fri Nov 1 08:40:07 2019 DEPLOYED mychart3-3.1.0 3.1.0 default +`, + }, + upgraded: []exectest.Release{ + {Name: "baz", Flags: []string{"--kube-context", "default"}}, + {Name: "bar", Flags: []string{"--kube-context", "default"}}, + {Name: "foo", Flags: []string{"--kube-context", "default"}}, + }, + deleted: []exectest.Release{}, + concurrency: 1, + 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: baz + 3: chart: stable/mychart3 + 4: disableValidationOnInstall: true + 5: - name: foo + 6: chart: stable/mychart1 + 7: disableValidationOnInstall: true + 8: needs: + 9: - bar +10: - name: bar +11: chart: stable/mychart2 +12: disableValidation: true +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: baz + 3: chart: stable/mychart3 + 4: disableValidationOnInstall: true + 5: - name: foo + 6: chart: stable/mychart1 + 7: disableValidationOnInstall: true + 8: needs: + 9: - bar +10: - name: bar +11: chart: stable/mychart2 +12: disableValidation: true +13: + +merged environment: &{default map[] map[]} +3 release(s) found in helmfile.yaml + +Affected releases are: + bar (stable/mychart2) UPDATED + baz (stable/mychart3) UPDATED + foo (stable/mychart1) UPDATED + +processing 2 groups of releases in this order: +GROUP RELEASES +1 baz, bar +2 foo + +processing releases in group 1/2: baz, bar +processing releases in group 2/2: foo +getting deployed release version failed:Failed to get the version for:mychart1 + +UPDATED RELEASES: +NAME CHART VERSION +baz stable/mychart3 3.1.0 +bar stable/mychart2 3.1.0 +foo stable/mychart1 + +`, + }, + // + // install with upgrade and --skip-diff-on-install + // + { + name: "install-with-upgrade-with-skip-diff-on-install", + loc: location(), + skipDiffOnInstall: true, + files: map[string]string{ + "/path/to/helmfile.yaml": ` +releases: +- name: baz + chart: stable/mychart3 + disableValidationOnInstall: true +- name: foo + chart: stable/mychart1 + disableValidationOnInstall: true + needs: + - bar +- name: bar + chart: stable/mychart2 + disableValidation: true +`, + }, + diffs: map[exectest.DiffKey]error{ + exectest.DiffKey{Name: "baz", Chart: "stable/mychart3", Flags: "--kube-contextdefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + exectest.DiffKey{Name: "bar", Chart: "stable/mychart2", Flags: "--disable-validation--kube-contextdefault--detailed-exitcode"}: helmexec.ExitError{Code: 2}, + }, + lists: map[exectest.ListKey]string{ + exectest.ListKey{Filter: "^foo$", Flags: "--kube-contextdefault--deployed--failed--pending"}: ``, + exectest.ListKey{Filter: "^bar$", Flags: "--kube-contextdefault--deployed--failed--pending"}: `NAME REVISION UPDATED STATUS CHART APP VERSION NAMESPACE +bar 4 Fri Nov 1 08:40:07 2019 DEPLOYED mychart2-3.1.0 3.1.0 default +`, + exectest.ListKey{Filter: "^baz$", Flags: "--kube-contextdefault--deployed--failed--pending"}: `NAME REVISION UPDATED STATUS CHART APP VERSION NAMESPACE +baz 4 Fri Nov 1 08:40:07 2019 DEPLOYED mychart3-3.1.0 3.1.0 default +`, + }, + upgraded: []exectest.Release{ + {Name: "baz", Flags: []string{"--kube-context", "default"}}, + {Name: "bar", Flags: []string{"--kube-context", "default"}}, + {Name: "foo", Flags: []string{"--kube-context", "default"}}, + }, + deleted: []exectest.Release{}, + concurrency: 1, + 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: baz + 3: chart: stable/mychart3 + 4: disableValidationOnInstall: true + 5: - name: foo + 6: chart: stable/mychart1 + 7: disableValidationOnInstall: true + 8: needs: + 9: - bar +10: - name: bar +11: chart: stable/mychart2 +12: disableValidation: true +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: baz + 3: chart: stable/mychart3 + 4: disableValidationOnInstall: true + 5: - name: foo + 6: chart: stable/mychart1 + 7: disableValidationOnInstall: true + 8: needs: + 9: - bar +10: - name: bar +11: chart: stable/mychart2 +12: disableValidation: true +13: + +merged environment: &{default map[] map[]} +3 release(s) found in helmfile.yaml + +Affected releases are: + bar (stable/mychart2) UPDATED + baz (stable/mychart3) UPDATED + foo (stable/mychart1) UPDATED + +processing 2 groups of releases in this order: +GROUP RELEASES +1 baz, bar +2 foo + +processing releases in group 1/2: baz, bar +processing releases in group 2/2: foo +getting deployed release version failed:Failed to get the version for:mychart1 + +UPDATED RELEASES: +NAME CHART VERSION +baz stable/mychart3 3.1.0 +bar stable/mychart2 3.1.0 +foo stable/mychart1 + `, }, // @@ -3919,8 +4141,9 @@ err: "foo" depends on nonexistent release "bar" applyErr := app.Apply(applyConfig{ // if we check log output, concurrency must be 1. otherwise the test becomes non-deterministic. - concurrency: tc.concurrency, - logger: logger, + concurrency: tc.concurrency, + logger: logger, + skipDiffOnInstall: tc.skipDiffOnInstall, }) if tc.error == "" && applyErr != nil { t.Fatalf("unexpected error for data defined at %s: %v", tc.loc, applyErr) diff --git a/pkg/app/config.go b/pkg/app/config.go index 9ba448e1..853507dc 100644 --- a/pkg/app/config.go +++ b/pkg/app/config.go @@ -52,6 +52,7 @@ type ApplyConfigProvider interface { RetainValuesFiles() bool SkipCleanup() bool + SkipDiffOnInstall() bool concurrencyConfig interactive diff --git a/pkg/state/state.go b/pkg/state/state.go index 24b99498..854b5692 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -211,6 +211,10 @@ type ReleaseSpec struct { // FYI, such diff without `--disable-validation` fails on first install because the K8s cluster doesn't have CRDs registered yet. DisableValidation *bool `yaml:"disableValidation,omitempty"` + // DisableValidationOnInstall disables the K8s API validation while running helm-diff on the release being newly installed on helmfile-apply. + // It is useful when any release contains custom resources for CRDs that is not yet installed onto the cluster. + DisableValidationOnInstall *bool `yaml:"disableValidationOnInstall,omitempty"` + // MissingFileHandler is set to either "Error" or "Warn". "Error" instructs helmfile to fail when unable to find a values or secrets file. When "Warn", it prints the file and continues. // The default value for MissingFileHandler is "Error". MissingFileHandler *string `yaml:"missingFileHandler,omitempty"` @@ -1418,10 +1422,11 @@ type diffResult struct { } type diffPrepareResult struct { - release *ReleaseSpec - flags []string - errors []*ReleaseError - files []string + release *ReleaseSpec + flags []string + errors []*ReleaseError + files []string + upgradeDueToSkippedDiff bool } func (st *HelmState) prepareDiffReleases(helm helmexec.Interface, additionalValues []string, concurrency int, detailedExitCode, includeTests, suppressSecrets bool, opt ...DiffOpt) ([]diffPrepareResult, []error) { @@ -1430,6 +1435,29 @@ func (st *HelmState) prepareDiffReleases(helm helmexec.Interface, additionalValu o.Apply(opts) } + mu := &sync.Mutex{} + installedReleases := map[string]bool{} + + isInstalled := func(r *ReleaseSpec) bool { + mu.Lock() + defer mu.Unlock() + + id := ReleaseToID(r) + + if v, ok := installedReleases[id]; ok { + return v + } + + v, err := st.isReleaseInstalled(st.createHelmContext(r, 0), helm, *r) + if err != nil { + st.logger.Warnf("confirming if the release is already installed or not: %v", err) + } else { + installedReleases[id] = v + } + + return v + } + releases := []*ReleaseSpec{} for i, _ := range st.Releases { if !st.Releases[i].Desired() { @@ -1465,10 +1493,17 @@ func (st *HelmState) prepareDiffReleases(helm helmexec.Interface, additionalValu st.ApplyOverrides(release) + if opts.SkipDiffOnInstall && !isInstalled(release) { + results <- diffPrepareResult{release: release, upgradeDueToSkippedDiff: true} + continue + } + + disableValidation := release.DisableValidationOnInstall != nil && *release.DisableValidationOnInstall && !isInstalled(release) + // TODO We need a long-term fix for this :) // See https://github.com/roboll/helmfile/issues/737 mut.Lock() - flags, files, err := st.flagsForDiff(helm, release, workerIndex) + flags, files, err := st.flagsForDiff(helm, release, disableValidation, workerIndex) mut.Unlock() if err != nil { errs = append(errs, err) @@ -1571,6 +1606,8 @@ type DiffOpts struct { Set []string SkipCleanup bool + + SkipDiffOnInstall bool } func (o *DiffOpts) Apply(opts *DiffOpts) { @@ -1609,6 +1646,9 @@ func (st *HelmState) DiffReleases(helm helmexec.Interface, additionalValues []st rs := []ReleaseSpec{} errs := []error{} + // The exit code returned by helm-diff when it detected any changes + HelmDiffExitCodeChanged := 2 + st.scatterGather( workerLimit, len(preps), @@ -1622,7 +1662,9 @@ func (st *HelmState) DiffReleases(helm helmexec.Interface, additionalValues []st for prep := range jobQueue { flags := prep.flags release := prep.release - if err := helm.DiffRelease(st.createHelmContext(release, workerIndex), release.Name, normalizeChart(st.basePath, release.Chart), suppressDiff, flags...); err != nil { + if prep.upgradeDueToSkippedDiff { + results <- diffResult{&ReleaseError{ReleaseSpec: release, err: nil, Code: HelmDiffExitCodeChanged}} + } else if err := helm.DiffRelease(st.createHelmContext(release, workerIndex), release.Name, normalizeChart(st.basePath, release.Chart), suppressDiff, flags...); err != nil { switch e := err.(type) { case helmexec.ExitError: // Propagate any non-zero exit status from the external command like `helm` that is failed under the hood @@ -1647,7 +1689,7 @@ func (st *HelmState) DiffReleases(helm helmexec.Interface, additionalValues []st res := <-results if res.err != nil { errs = append(errs, res.err) - if res.err.Code == 2 { + if res.err.Code == HelmDiffExitCodeChanged { rs = append(rs, *res.err.ReleaseSpec) } } @@ -2151,7 +2193,7 @@ func (st *HelmState) flagsForTemplate(helm helmexec.Interface, release *ReleaseS return append(flags, common...), files, nil } -func (st *HelmState) flagsForDiff(helm helmexec.Interface, release *ReleaseSpec, workerIndex int) ([]string, []string, error) { +func (st *HelmState) flagsForDiff(helm helmexec.Interface, release *ReleaseSpec, disableValidation bool, workerIndex int) ([]string, []string, error) { flags := st.chartVersionFlags(release) disableOpenAPIValidation := false @@ -2165,7 +2207,6 @@ func (st *HelmState) flagsForDiff(helm helmexec.Interface, release *ReleaseSpec, flags = append(flags, "--disable-openapi-validation") } - disableValidation := false if release.DisableValidation != nil { disableValidation = *release.DisableValidation } else if st.HelmDefaults.DisableValidation != nil {