diff --git a/pkg/action/uninstall.go b/pkg/action/uninstall.go index efbc72fef..0af47045d 100644 --- a/pkg/action/uninstall.go +++ b/pkg/action/uninstall.go @@ -276,12 +276,12 @@ func (u *Uninstall) deleteRelease(rel *release.Release) (kube.ResourceList, stri return nil, "", []error{fmt.Errorf("unable to build kubernetes objects for delete: %w", err)} } if len(resources) > 0 { - _, errs = u.cfg.KubeClient.Delete(resources, parseCascadingFlag(u.DeletionPropagation)) + _, errs = u.cfg.KubeClient.Delete(resources, u.parseCascadingFlag(u.DeletionPropagation)) } return resources, kept.String(), errs } -func parseCascadingFlag(cascadingFlag string) v1.DeletionPropagation { +func (u *Uninstall) parseCascadingFlag(cascadingFlag string) v1.DeletionPropagation { switch cascadingFlag { case "orphan": return v1.DeletePropagationOrphan @@ -290,6 +290,14 @@ func parseCascadingFlag(cascadingFlag string) v1.DeletionPropagation { case "background": return v1.DeletePropagationBackground default: + // When --wait is used with watcher or legacy strategy, default to foreground + // deletion to ensure dependent resources are cleaned up before returning. + // This prevents the issue where uninstallation with wait strategy returns before + // all resources (including controller-created dependents) are fully deleted. + if u.WaitStrategy == kube.StatusWatcherStrategy || u.WaitStrategy == kube.LegacyStrategy { + slog.Debug("uninstall: defaulting to foreground deletion due to wait strategy", "wait strategy", u.WaitStrategy, "cascading flag", cascadingFlag) + return v1.DeletePropagationForeground + } slog.Debug("uninstall: given cascade value, defaulting to delete propagation background", "value", cascadingFlag) return v1.DeletePropagationBackground } diff --git a/pkg/action/uninstall_test.go b/pkg/action/uninstall_test.go index fba1e391f..e2e895dbe 100644 --- a/pkg/action/uninstall_test.go +++ b/pkg/action/uninstall_test.go @@ -169,3 +169,72 @@ func TestUninstallRun_UnreachableKubeClient(t *testing.T) { assert.Nil(t, result) assert.ErrorContains(t, err, "connection refused") } + +func TestParseCascadingFlag(t *testing.T) { + tests := []struct { + name string + cascadingFlag string + waitStrategy kube.WaitStrategy + expectedResult string + }{ + { + name: "explicit orphan", + cascadingFlag: "orphan", + waitStrategy: kube.HookOnlyStrategy, + expectedResult: "Orphan", + }, + { + name: "explicit foreground", + cascadingFlag: "foreground", + waitStrategy: kube.HookOnlyStrategy, + expectedResult: "Foreground", + }, + { + name: "explicit background", + cascadingFlag: "background", + waitStrategy: kube.HookOnlyStrategy, + expectedResult: "Background", + }, + { + name: "default with hookOnly strategy returns background", + cascadingFlag: "", + waitStrategy: kube.HookOnlyStrategy, + expectedResult: "Background", + }, + { + name: "default with watcher strategy returns foreground", + cascadingFlag: "", + waitStrategy: kube.StatusWatcherStrategy, + expectedResult: "Foreground", + }, + { + name: "default with legacy strategy returns foreground", + cascadingFlag: "", + waitStrategy: kube.LegacyStrategy, + expectedResult: "Foreground", + }, + { + name: "unknown value with hookOnly strategy returns background", + cascadingFlag: "unknown", + waitStrategy: kube.HookOnlyStrategy, + expectedResult: "Background", + }, + { + name: "unknown value with watcher strategy returns foreground", + cascadingFlag: "unknown", + waitStrategy: kube.StatusWatcherStrategy, + expectedResult: "Foreground", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + unAction := uninstallAction(t) + unAction.WaitStrategy = tt.waitStrategy + + result := unAction.parseCascadingFlag(tt.cascadingFlag) + + assert.Equal(t, tt.expectedResult, string(result)) + }) + } +} diff --git a/pkg/cmd/uninstall.go b/pkg/cmd/uninstall.go index 4cc14ae1e..8e38fbed8 100644 --- a/pkg/cmd/uninstall.go +++ b/pkg/cmd/uninstall.go @@ -25,6 +25,7 @@ import ( "helm.sh/helm/v4/pkg/action" "helm.sh/helm/v4/pkg/cmd/require" + "helm.sh/helm/v4/pkg/kube" ) const uninstallDesc = ` @@ -50,11 +51,20 @@ func newUninstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { ValidArgsFunction: func(_ *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return compListReleases(toComplete, args, cfg) }, - RunE: func(_ *cobra.Command, args []string) error { + RunE: func(cmd *cobra.Command, args []string) error { validationErr := validateCascadeFlag(client) if validationErr != nil { return validationErr } + + // When --wait is used with a non-hookOnly strategy, default to foreground + // deletion to ensure dependent resources are cleaned up before returning. + // This prevents the issue where helm uninstall --wait returns before + // all resources (including controller-created dependents) are fully deleted. + if !cmd.Flags().Changed("cascade") && client.WaitStrategy != kube.HookOnlyStrategy { + client.DeletionPropagation = "foreground" + } + for i := range args { res, err := client.Run(args[i]) @@ -76,7 +86,7 @@ func newUninstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { f.BoolVar(&client.DisableHooks, "no-hooks", false, "prevent hooks from running during uninstallation") f.BoolVar(&client.IgnoreNotFound, "ignore-not-found", false, `Treat "release not found" as a successful uninstall`) f.BoolVar(&client.KeepHistory, "keep-history", false, "remove all associated resources and mark the release as deleted, but retain the release history") - f.StringVar(&client.DeletionPropagation, "cascade", "background", "Must be \"background\", \"orphan\", or \"foreground\". Selects the deletion cascading strategy for the dependents. Defaults to background.") + f.StringVar(&client.DeletionPropagation, "cascade", "background", "Must be \"background\", \"orphan\", or \"foreground\". Selects the deletion cascading strategy for the dependents. Defaults to background, or foreground if --wait is used.") f.DurationVar(&client.Timeout, "timeout", 300*time.Second, "time to wait for any individual Kubernetes operation (like Jobs for hooks)") f.StringVar(&client.Description, "description", "", "add a custom description") AddWaitFlag(cmd, &client.WaitStrategy) diff --git a/pkg/cmd/uninstall_test.go b/pkg/cmd/uninstall_test.go index ce436e68c..d4096a0c5 100644 --- a/pkg/cmd/uninstall_test.go +++ b/pkg/cmd/uninstall_test.go @@ -17,10 +17,19 @@ limitations under the License. package cmd import ( + "bytes" + "io" "testing" - "helm.sh/helm/v4/pkg/release/common" + "github.com/stretchr/testify/assert" + + "helm.sh/helm/v4/pkg/action" + "helm.sh/helm/v4/pkg/chart/common" + kubefake "helm.sh/helm/v4/pkg/kube/fake" + releasecommon "helm.sh/helm/v4/pkg/release/common" release "helm.sh/helm/v4/pkg/release/v1" + "helm.sh/helm/v4/pkg/storage" + "helm.sh/helm/v4/pkg/storage/driver" ) func TestUninstall(t *testing.T) { @@ -63,8 +72,8 @@ func TestUninstall(t *testing.T) { cmd: "uninstall aeneas --keep-history", golden: "output/uninstall-keep-history-earlier-deployed.txt", rels: []*release.Release{ - release.Mock(&release.MockReleaseOptions{Name: "aeneas", Version: 1, Status: common.StatusDeployed}), - release.Mock(&release.MockReleaseOptions{Name: "aeneas", Version: 2, Status: common.StatusFailed}), + release.Mock(&release.MockReleaseOptions{Name: "aeneas", Version: 1, Status: releasecommon.StatusDeployed}), + release.Mock(&release.MockReleaseOptions{Name: "aeneas", Version: 2, Status: releasecommon.StatusFailed}), }, }, { @@ -83,6 +92,111 @@ func TestUninstall(t *testing.T) { runTestCmd(t, tests) } +// TestUninstallCascadeDefaultWithWait tests that when --wait is used, +// the default cascade behavior changes to foreground to ensure +// dependent resources are cleaned up before returning. +func TestUninstallCascadeDefaultWithWait(t *testing.T) { + tests := []struct { + name string + cmd string + expectedDeletionPropagation string + }{ + { + name: "without --wait uses background cascade", + cmd: "uninstall test-release", + expectedDeletionPropagation: "Background", + }, + { + name: "--wait without --cascade uses foreground cascade", + cmd: "uninstall test-release --wait", + expectedDeletionPropagation: "Foreground", + }, + { + name: "--wait=watcher without --cascade uses foreground cascade", + cmd: "uninstall test-release --wait=watcher", + expectedDeletionPropagation: "Foreground", + }, + { + name: "--wait=legacy without --cascade uses foreground cascade", + cmd: "uninstall test-release --wait=legacy", + expectedDeletionPropagation: "Foreground", + }, + { + name: "--wait=hookOnly uses background cascade (default)", + cmd: "uninstall test-release --wait=hookOnly", + expectedDeletionPropagation: "Background", + }, + { + name: "--wait with explicit --cascade=background respects user setting", + cmd: "uninstall test-release --wait --cascade=background", + expectedDeletionPropagation: "Background", + }, + { + name: "--wait with explicit --cascade=orphan respects user setting", + cmd: "uninstall test-release --wait --cascade=orphan", + expectedDeletionPropagation: "Orphan", + }, + { + name: "--wait with explicit --cascade=foreground respects user setting", + cmd: "uninstall test-release --wait --cascade=foreground", + expectedDeletionPropagation: "Foreground", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create a capturing KubeClient to verify the deletion propagation + capturedDeletionPropagation := "" + store := storage.Init(driver.NewMemory()) + rel := release.Mock(&release.MockReleaseOptions{Name: "test-release"}) + if err := store.Create(rel); err != nil { + t.Fatal(err) + } + + capturingClient := &kubefake.CapturingKubeClient{ + FailingKubeClient: kubefake.FailingKubeClient{ + PrintingKubeClient: kubefake.PrintingKubeClient{Out: io.Discard}, + BuildDummy: true, + }, + CapturedDeletionPropagation: &capturedDeletionPropagation, + } + + actionConfig := &action.Configuration{ + Releases: store, + KubeClient: capturingClient, + Capabilities: common.DefaultCapabilities, + } + + buf := new(bytes.Buffer) + args := splitCmd(tt.cmd) + + root, err := newRootCmdWithConfig(actionConfig, buf, args, SetupLogging) + if err != nil { + t.Fatal(err) + } + + root.SetArgs(args) + root.SetOut(buf) + root.SetErr(buf) + + _ = root.Execute() + + // Verify the captured deletion propagation + assert.Equal(t, tt.expectedDeletionPropagation, capturedDeletionPropagation, + "Expected DeletionPropagation to be %q but got %q", tt.expectedDeletionPropagation, capturedDeletionPropagation) + }) + } +} + +func splitCmd(cmd string) []string { + // Simple split by space - doesn't handle quoted strings + result := []string{} + for _, part := range bytes.Fields([]byte(cmd)) { + result = append(result, string(part)) + } + return result +} + func TestUninstallCompletion(t *testing.T) { checkReleaseCompletion(t, "uninstall", true) } diff --git a/pkg/kube/fake/capturing_kube_client.go b/pkg/kube/fake/capturing_kube_client.go new file mode 100644 index 000000000..947020ff2 --- /dev/null +++ b/pkg/kube/fake/capturing_kube_client.go @@ -0,0 +1,39 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package fake + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "helm.sh/helm/v4/pkg/kube" +) + +// CapturingKubeClient is a fake KubeClient that captures internal flags such as the +// deletion propagation policy. It extends FailingKubeClient to provide dummy resources. +type CapturingKubeClient struct { + FailingKubeClient + // CapturedDeletionPropagation stores the DeletionPropagation value passed to Delete + CapturedDeletionPropagation *string +} + +// Delete captures the deletion propagation policy and returns success +func (c *CapturingKubeClient) Delete(resources kube.ResourceList, policy metav1.DeletionPropagation) (*kube.Result, []error) { + if c.CapturedDeletionPropagation != nil { + *c.CapturedDeletionPropagation = string(policy) + } + return &kube.Result{Deleted: resources}, nil +}