From c3864a45d31a9fe00efcde6ff536ad66f15a4e11 Mon Sep 17 00:00:00 2001 From: yxxhero <11087727+yxxhero@users.noreply.github.com> Date: Mon, 28 Apr 2025 00:57:41 +0800 Subject: [PATCH] feat: add --take-ownership flag to helm diff and related config (#1992) * feat: add --take-ownership flag to helm diff and related config Signed-off-by: yxxhero * fix: nil issue Signed-off-by: yxxhero * fix tests Signed-off-by: yxxhero * fix tests Signed-off-by: yxxhero * fix more issue Signed-off-by: yxxhero * fix more issues Signed-off-by: yxxhero * fix tests Signed-off-by: yxxhero * add more tests Signed-off-by: yxxhero --------- Signed-off-by: yxxhero --- cmd/diff.go | 1 + pkg/app/app.go | 2 + pkg/app/config.go | 1 + pkg/app/diff_test.go | 4 ++ pkg/config/apply.go | 1 + pkg/config/diff.go | 8 +++ pkg/state/helmx.go | 25 ++++++++- pkg/state/helmx_test.go | 29 +++++++--- pkg/state/state.go | 55 +++++++++++++++++-- pkg/state/temp_test.go | 12 ++-- .../output.yaml | 3 - 11 files changed, 117 insertions(+), 24 deletions(-) diff --git a/cmd/diff.go b/cmd/diff.go index 020bc6f1..c2238704 100644 --- a/cmd/diff.go +++ b/cmd/diff.go @@ -52,6 +52,7 @@ func NewDiffCmd(globalCfg *config.GlobalImpl) *cobra.Command { f.StringArrayVar(&diffOptions.Suppress, "suppress", nil, "suppress specified Kubernetes objects in the output. Can be provided multiple times. For example: --suppress KeycloakClient --suppress VaultSecret") f.BoolVar(&diffOptions.ReuseValues, "reuse-values", false, `Override helmDefaults.reuseValues "helm diff upgrade --install --reuse-values"`) f.BoolVar(&diffOptions.ResetValues, "reset-values", false, `Override helmDefaults.reuseValues "helm diff upgrade --install --reset-values"`) + f.BoolVar(&diffOptions.TakeOwnership, "take-ownership", false, "add --take-ownership flag to helm") f.StringVar(&diffOptions.PostRenderer, "post-renderer", "", `pass --post-renderer to "helm template" or "helm upgrade --install"`) f.StringArrayVar(&diffOptions.PostRendererArgs, "post-renderer-args", nil, `pass --post-renderer-args to "helm template" or "helm upgrade --install"`) f.StringArrayVar(&diffOptions.SuppressOutputLineRegex, "suppress-output-line-regex", nil, "a list of regex patterns to suppress output lines from the diff output") diff --git a/pkg/app/app.go b/pkg/app/app.go index 65751681..16a9c720 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -1390,6 +1390,7 @@ func (a *App) apply(r *Run, c ApplyConfigProvider) (bool, bool, []error) { PostRendererArgs: c.PostRendererArgs(), SkipSchemaValidation: c.SkipSchemaValidation(), SuppressOutputLineRegex: c.SuppressOutputLineRegex(), + TakeOwnership: c.TakeOwnership(), } infoMsg, releasesToBeUpdated, releasesToBeDeleted, errs := r.diff(false, detailedExitCode, c, diffOpts) @@ -1634,6 +1635,7 @@ func (a *App) diff(r *Run, c DiffConfigProvider) (*string, bool, bool, []error) PostRendererArgs: c.PostRendererArgs(), SkipSchemaValidation: c.SkipSchemaValidation(), SuppressOutputLineRegex: c.SuppressOutputLineRegex(), + TakeOwnership: c.TakeOwnership(), } filtered := &Run{ diff --git a/pkg/app/config.go b/pkg/app/config.go index 34dff76d..e699d86f 100644 --- a/pkg/app/config.go +++ b/pkg/app/config.go @@ -157,6 +157,7 @@ type DiffConfigProvider interface { NoColor() bool Context() int DiffOutput() string + TakeOwnership() bool concurrencyConfig valuesControlMode diff --git a/pkg/app/diff_test.go b/pkg/app/diff_test.go index 2e60b3df..18dde7fc 100644 --- a/pkg/app/diff_test.go +++ b/pkg/app/diff_test.go @@ -46,6 +46,7 @@ type diffConfig struct { skipSchemaValidation bool reuseValues bool logger *zap.SugaredLogger + takeOwnership bool } func (a diffConfig) Args() string { @@ -183,6 +184,9 @@ func (a diffConfig) SkipSchemaValidation() bool { func (a diffConfig) SuppressOutputLineRegex() []string { return a.suppressOutputLineRegex } +func (a diffConfig) TakeOwnership() bool { + return a.takeOwnership +} func TestDiff(t *testing.T) { type flags struct { diff --git a/pkg/config/apply.go b/pkg/config/apply.go index 7cfc6d3c..81a9a195 100644 --- a/pkg/config/apply.go +++ b/pkg/config/apply.go @@ -269,6 +269,7 @@ func (a *ApplyImpl) TakeOwnership() bool { return a.ApplyOptions.TakeOwnership } +// SyncReleaseLabels returns the SyncReleaseLabels. func (a *ApplyImpl) SyncReleaseLabels() bool { return a.ApplyOptions.SyncReleaseLabels } diff --git a/pkg/config/diff.go b/pkg/config/diff.go index ac8018b9..d8830fad 100644 --- a/pkg/config/diff.go +++ b/pkg/config/diff.go @@ -49,6 +49,8 @@ type DiffOptions struct { // SuppressOutputLineRegex is a list of regexes to suppress output lines SuppressOutputLineRegex []string SkipSchemaValidation bool + // TakeOwnership is true if the ownership should be taken + TakeOwnership bool } // NewDiffOptions creates a new Apply @@ -201,6 +203,12 @@ func (t *DiffImpl) SuppressOutputLineRegex() []string { return t.DiffOptions.SuppressOutputLineRegex } +// SkipSchemaValidation returns the SkipSchemaValidation. func (t *DiffImpl) SkipSchemaValidation() bool { return t.DiffOptions.SkipSchemaValidation } + +// TakeOwnership returns the TakeOwnership. +func (t *DiffImpl) TakeOwnership() bool { + return t.DiffOptions.TakeOwnership +} diff --git a/pkg/state/helmx.go b/pkg/state/helmx.go index 00d29cac..1a46b1d5 100644 --- a/pkg/state/helmx.go +++ b/pkg/state/helmx.go @@ -54,7 +54,22 @@ func formatLabels(labels map[string]string) string { // append labels flags to helm flags, starting from helm v3.13.0 func (st *HelmState) appendLabelsFlags(flags []string, helm helmexec.Interface, release *ReleaseSpec, syncReleaseLabels bool) []string { - if helm.IsVersionAtLeast("3.13.0") && (syncReleaseLabels || release.SyncReleaseLabels) { + if !helm.IsVersionAtLeast("3.13.0") { + return flags + } + isSyncReleaseLabels := false + switch { + // Check if SyncReleaseLabels is true in the release spec. + case release.SyncReleaseLabels != nil && *release.SyncReleaseLabels: + isSyncReleaseLabels = true + // Check if syncReleaseLabels argument is true. + case syncReleaseLabels: + isSyncReleaseLabels = true + // Check if SyncReleaseLabels is true in HelmDefaults. + case st.HelmDefaults.SyncReleaseLabels != nil && *st.HelmDefaults.SyncReleaseLabels: + isSyncReleaseLabels = true + } + if isSyncReleaseLabels { labels := formatLabels(release.Labels) if labels != "" { flags = append(flags, "--labels", labels) @@ -203,13 +218,17 @@ func (st *HelmState) appendHideNotesFlags(flags []string, helm helmexec.Interfac } // append take-ownership flags to helm flags -func (st *HelmState) appendTakeOwnershipFlags(flags []string, helm helmexec.Interface, ops *SyncOpts) []string { +func (st *HelmState) appendTakeOwnershipFlagsForUpgrade(flags []string, helm helmexec.Interface, release *ReleaseSpec, takeOwnership bool) []string { // see https://github.com/helm/helm/releases/tag/v3.17.0 if !helm.IsVersionAtLeast("3.17.0") { return flags } switch { - case ops.TakeOwnership: + case release.TakeOwnership != nil && *release.TakeOwnership: + flags = append(flags, "--take-ownership") + case takeOwnership: + flags = append(flags, "--take-ownership") + case st.HelmDefaults.TakeOwnership != nil && *st.HelmDefaults.TakeOwnership: flags = append(flags, "--take-ownership") } return flags diff --git a/pkg/state/helmx_test.go b/pkg/state/helmx_test.go index 76f4300d..90bfb83e 100644 --- a/pkg/state/helmx_test.go +++ b/pkg/state/helmx_test.go @@ -387,12 +387,13 @@ func TestAppendHideNotesFlags(t *testing.T) { } } -func TestAppendTakeOwnershipFlags(t *testing.T) { +func TestAppendTakeOwnershipFlagsForUpgrade(t *testing.T) { type args struct { flags []string helm helmexec.Interface helmSpec HelmSpec opt *SyncOpts + release *ReleaseSpec expected []string } tests := []struct { @@ -402,8 +403,9 @@ func TestAppendTakeOwnershipFlags(t *testing.T) { { name: "no take-ownership when helm less than 3.17.0", args: args{ - flags: []string{}, - helm: testutil.NewVersionHelmExec("3.16.0"), + flags: []string{}, + release: &ReleaseSpec{}, + helm: testutil.NewVersionHelmExec("3.16.0"), opt: &SyncOpts{ TakeOwnership: true, }, @@ -413,21 +415,34 @@ func TestAppendTakeOwnershipFlags(t *testing.T) { { name: "take-ownership from cmd flag", args: args{ - flags: []string{}, - helm: testutil.NewVersionHelmExec("3.17.0"), + flags: []string{}, + helm: testutil.NewVersionHelmExec("3.17.0"), + release: &ReleaseSpec{}, opt: &SyncOpts{ TakeOwnership: true, }, expected: []string{"--take-ownership"}, }, }, + { + name: "take-ownership from release", + args: args{ + flags: []string{}, + helm: testutil.NewVersionHelmExec("3.17.0"), + release: &ReleaseSpec{ + TakeOwnership: &[]bool{true}[0], + }, + opt: &SyncOpts{}, + expected: []string{"--take-ownership"}, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { st := &HelmState{} st.HelmDefaults = tt.args.helmSpec - got := st.appendTakeOwnershipFlags(tt.args.flags, tt.args.helm, tt.args.opt) - require.Equalf(t, tt.args.expected, got, "appendTakeOwnershipFlags() = %v, want %v", got, tt.args.expected) + got := st.appendTakeOwnershipFlagsForUpgrade(tt.args.flags, tt.args.helm, tt.args.release, tt.args.opt.TakeOwnership) + require.Equalf(t, tt.args.expected, got, "appendTakeOwnershipFlagsForUpgrade() = %v, want %v", got, tt.args.expected) }) } } diff --git a/pkg/state/state.go b/pkg/state/state.go index ef93ce67..b167fa88 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -204,11 +204,13 @@ type HelmSpec struct { // PlainHttp is true if the remote charte should be fetched using HTTP and not HTTPS PlainHttp bool `yaml:"plainHttp,omitempty"` // Wait, if set to true, will wait until all resources are deleted before mark delete command as successful - DeleteWait bool `yaml:"deleteWait"` + DeleteWait bool `yaml:"deleteWait,omitempty"` // Timeout is the time in seconds to wait for helmfile delete command (default 300) - DeleteTimeout int `yaml:"deleteTimeout"` + DeleteTimeout int `yaml:"deleteTimeout,omitempty"` // SyncReleaseLabels is true if the release labels should be synced with the helmfile labels - SyncReleaseLabels bool `yaml:"syncReleaseLabels"` + SyncReleaseLabels *bool `yaml:"syncReleaseLabels,omitempty"` + // TakeOwnership is true if the helmfile should take ownership of the release + TakeOwnership *bool `yaml:"takeOwnership,omitempty"` } // RepositorySpec that defines values for a helm repo @@ -412,7 +414,9 @@ type ReleaseSpec struct { // Timeout is the time in seconds to wait for helmfile delete command (default 300) DeleteTimeout *int `yaml:"deleteTimeout,omitempty"` // SyncReleaseLabels is true if the release labels should be synced with the helmfile labels - SyncReleaseLabels bool `yaml:"syncReleaseLabels"` + SyncReleaseLabels *bool `yaml:"syncReleaseLabels,omitempty"` + // TakeOwnership is true if the release should take ownership of the resources + TakeOwnership *bool `yaml:"takeOwnership,omitempty"` } func (r *Inherits) UnmarshalYAML(unmarshal func(any) error) error { @@ -2040,6 +2044,7 @@ type DiffOpts struct { PostRendererArgs []string SuppressOutputLineRegex []string SkipSchemaValidation bool + TakeOwnership bool } func (o *DiffOpts) Apply(opts *DiffOpts) { @@ -2797,9 +2802,11 @@ func (st *HelmState) flagsForUpgrade(helm helmexec.Interface, release *ReleaseSp postRenderer := "" syncReleaseLabels := false + takeOwnership := false if opt != nil { postRenderer = opt.PostRenderer syncReleaseLabels = opt.SyncReleaseLabels + takeOwnership = opt.TakeOwnership } flags = st.appendConnectionFlags(flags, release) @@ -2828,7 +2835,7 @@ func (st *HelmState) flagsForUpgrade(helm helmexec.Interface, release *ReleaseSp flags = st.appendHideNotesFlags(flags, helm, opt) // append take-ownership flag - flags = st.appendTakeOwnershipFlags(flags, helm, opt) + flags = st.appendTakeOwnershipFlagsForUpgrade(flags, helm, release, takeOwnership) flags = st.appendExtraSyncFlags(flags, opt) @@ -2969,6 +2976,17 @@ func (st *HelmState) flagsForDiff(helm helmexec.Interface, release *ReleaseSpec, flags = st.appendSuppressOutputLineRegexFlags(flags, release, suppressOutputLineRegex) } + takeOwnership := false + if opt != nil { + takeOwnership = opt.TakeOwnership + } + + var err error + flags, err = st.appendTakeOwnershipFlagsForDiff(flags, release, takeOwnership) + if err != nil { + return nil, nil, err + } + common, files, err := st.namespaceAndValuesFlags(helm, release, workerIndex) if err != nil { return nil, files, err @@ -2977,6 +2995,33 @@ func (st *HelmState) flagsForDiff(helm helmexec.Interface, release *ReleaseSpec, return append(flags, common...), files, nil } +func (st *HelmState) appendTakeOwnershipFlagsForDiff(flags []string, release *ReleaseSpec, takeOwnership bool) ([]string, error) { + settings := cli.New() + isAppendTakeOwnership := false + + switch { + case release.TakeOwnership != nil && *release.TakeOwnership: + isAppendTakeOwnership = true + case takeOwnership: + isAppendTakeOwnership = true + case st.HelmDefaults.TakeOwnership != nil && *st.HelmDefaults.TakeOwnership: + isAppendTakeOwnership = true + } + if isAppendTakeOwnership { + diffVersion, err := helmexec.GetPluginVersion("diff", settings.PluginsDirectory) + if err != nil { + return flags, err + } + dv, _ := semver.NewVersion("v3.11.0") + + if diffVersion.LessThan(dv) { + return flags, fmt.Errorf("take-ownership is not supported by helm-diff plugin version %s, please use at least v3.11.0", diffVersion) + } + flags = append(flags, "--take-ownership") + } + return flags, nil +} + func (st *HelmState) appendChartVersionFlags(flags []string, release *ReleaseSpec) []string { if release.Version != "" { flags = append(flags, "--version", release.Version) diff --git a/pkg/state/temp_test.go b/pkg/state/temp_test.go index f0eed882..90ef78a9 100644 --- a/pkg/state/temp_test.go +++ b/pkg/state/temp_test.go @@ -38,39 +38,39 @@ func TestGenerateID(t *testing.T) { run(testcase{ subject: "baseline", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, - want: "foo-values-54f5f6cdb5", + want: "foo-values-57ff559cd5", }) run(testcase{ subject: "different bytes content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: []byte(`{"k":"v"}`), - want: "foo-values-6bc8f7944b", + want: "foo-values-75cd785b8b", }) run(testcase{ subject: "different map content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: map[string]any{"k": "v"}, - want: "foo-values-dcffdcb8", + want: "foo-values-5b44fdc768", }) run(testcase{ subject: "different chart", release: ReleaseSpec{Name: "foo", Chart: "stable/envoy"}, - want: "foo-values-6d4c6fd548", + want: "foo-values-6ddb5db94d", }) run(testcase{ subject: "different name", release: ReleaseSpec{Name: "bar", Chart: "incubator/raw"}, - want: "bar-values-76974767c8", + want: "bar-values-79bbb8df74", }) run(testcase{ subject: "specific ns", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw", Namespace: "myns"}, - want: "myns-foo-values-77bd9cc6fb", + want: "myns-foo-values-c9457ccfb", }) for id, n := range ids { diff --git a/test/e2e/template/helmfile/testdata/snapshot/issue_2098_release_template_needs/output.yaml b/test/e2e/template/helmfile/testdata/snapshot/issue_2098_release_template_needs/output.yaml index c7d30230..25dd4428 100644 --- a/test/e2e/template/helmfile/testdata/snapshot/issue_2098_release_template_needs/output.yaml +++ b/test/e2e/template/helmfile/testdata/snapshot/issue_2098_release_template_needs/output.yaml @@ -19,7 +19,6 @@ releases: name: default-shared-resources namespace: default service: shared-resources - syncReleaseLabels: false - chart: aservo/util version: 0.0.1 needs: @@ -31,10 +30,8 @@ releases: name: default-release-resources namespace: default service: release-resources - syncReleaseLabels: false templates: defaults: name: default-{{ .Release.Labels.service }} namespace: default - syncReleaseLabels: false renderedvalues: {}