diff --git a/pkg/action/uninstall.go b/pkg/action/uninstall.go index 79156991c..b14305112 100644 --- a/pkg/action/uninstall.go +++ b/pkg/action/uninstall.go @@ -88,6 +88,73 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) if err != nil { return nil, err } + + // Verify ownership in dry-run mode to show what would actually be deleted + manifests := releaseutil.SplitManifests(r.Manifest) + _, files, err := releaseutil.SortManifests(manifests, nil, releaseutil.UninstallOrder) + if err == nil { + filesToKeep, filesToDelete := filterManifestsToKeep(files) + + var builder strings.Builder + for _, file := range filesToDelete { + builder.WriteString("\n---\n" + file.Content) + } + + resources, err := u.cfg.KubeClient.Build(strings.NewReader(builder.String()), false) + if err == nil && len(resources) > 0 { + ownedResources, unownedResources, unverifiableResources, err := verifyOwnershipBeforeDelete(resources, r.Name, r.Namespace) + if err == nil { + if len(unownedResources) > 0 { + u.cfg.Logger().Warn("dry-run: resources would be skipped because they are not owned by this release", + "release", r.Name, + "count", len(unownedResources)) + for _, info := range unownedResources { + u.cfg.Logger().Warn("dry-run: would skip resource", + "kind", info.Mapping.GroupVersionKind.Kind, + "name", info.Name, + "namespace", info.Namespace) + } + } + + if len(unverifiableResources) > 0 { + u.cfg.Logger().Warn("dry-run: resources would be skipped because their ownership could not be verified", + "release", r.Name, + "count", len(unverifiableResources)) + for _, ur := range unverifiableResources { + u.cfg.Logger().Warn("dry-run: would skip resource (ownership could not be verified)", + "kind", ur.Info.Mapping.GroupVersionKind.Kind, + "name", ur.Info.Name, + "namespace", ur.Info.Namespace, + "error", ur.Err) + } + } + + if len(ownedResources) > 0 { + u.cfg.Logger().Debug("dry-run: resources would be deleted", + "release", r.Name, + "count", len(ownedResources)) + for _, info := range ownedResources { + u.cfg.Logger().Debug("dry-run: would delete resource", + "kind", info.Mapping.GroupVersionKind.Kind, + "name", info.Name, + "namespace", info.Namespace) + } + } + } + } + + // Include kept resources in dry-run info + if len(filesToKeep) > 0 { + var kept strings.Builder + kept.WriteString("These resources were kept due to the resource policy:\n") + for _, f := range filesToKeep { + fmt.Fprintf(&kept, "[%s] %s\n", f.Head.Kind, f.Head.Metadata.Name) + } + res := &releasei.UninstallReleaseResponse{Release: r, Info: kept.String()} + return res, nil + } + } + return &releasei.UninstallReleaseResponse{Release: r}, nil } @@ -153,9 +220,6 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) return nil, fmt.Errorf("failed to delete release: %s", name) } - if kept != "" { - kept = "These resources were kept due to the resource policy:\n" + kept - } res.Info = kept if err := waiter.WaitForDelete(deletedResources, u.Timeout); err != nil { @@ -269,8 +333,11 @@ func (u *Uninstall) deleteRelease(rel *release.Release) (kube.ResourceList, stri filesToKeep, filesToDelete := filterManifestsToKeep(files) var kept strings.Builder - for _, f := range filesToKeep { - fmt.Fprintf(&kept, "[%s] %s\n", f.Head.Kind, f.Head.Metadata.Name) + if len(filesToKeep) > 0 { + kept.WriteString("These resources were kept due to the resource policy:\n") + for _, f := range filesToKeep { + fmt.Fprintf(&kept, "[%s] %s\n", f.Head.Kind, f.Head.Metadata.Name) + } } var builder strings.Builder @@ -282,13 +349,69 @@ func (u *Uninstall) deleteRelease(rel *release.Release) (kube.ResourceList, stri if err != nil { return nil, "", []error{fmt.Errorf("unable to build kubernetes objects for delete: %w", err)} } + + // Verify ownership before deleting resources + var ownedResources, unownedResources kube.ResourceList + var unverifiableResources []unverifiableResource if len(resources) > 0 { - _, errs = u.cfg.KubeClient.Delete(resources, parseCascadingFlag(u.DeletionPropagation)) + ownedResources, unownedResources, unverifiableResources, err = verifyOwnershipBeforeDelete(resources, rel.Name, rel.Namespace) + if err != nil { + return nil, "", []error{fmt.Errorf("unable to verify resource ownership: %w", err)} + } + + // Log warnings for unowned resources + if len(unownedResources) > 0 { + for _, info := range unownedResources { + u.cfg.Logger().Warn("skipping delete of resource not owned by this release", + "kind", info.Mapping.GroupVersionKind.Kind, + "name", info.Name, + "namespace", info.Namespace, + "release", rel.Name) + } + if kept.Len() > 0 { + kept.WriteString("\n") + } + fmt.Fprintf(&kept, "%d resource(s) were not deleted because they are not owned by this release:\n", len(unownedResources)) + for _, info := range unownedResources { + fmt.Fprintf(&kept, "[%s] %s\n", info.Mapping.GroupVersionKind.Kind, info.Name) + } + } + + // Log warnings for resources whose ownership could not be verified + if len(unverifiableResources) > 0 { + for _, ur := range unverifiableResources { + u.cfg.Logger().Warn("skipping delete of resource because ownership could not be verified", + "kind", ur.Info.Mapping.GroupVersionKind.Kind, + "name", ur.Info.Name, + "namespace", ur.Info.Namespace, + "release", rel.Name, + "error", ur.Err) + } + if kept.Len() > 0 { + kept.WriteString("\n") + } + fmt.Fprintf(&kept, "%d resource(s) were not deleted because their ownership could not be verified:\n", len(unverifiableResources)) + for _, ur := range unverifiableResources { + fmt.Fprintf(&kept, "[%s] %s: %s\n", ur.Info.Mapping.GroupVersionKind.Kind, ur.Info.Name, ur.Err) + } + } + + // Delete only owned resources + if len(ownedResources) > 0 { + for _, info := range ownedResources { + u.cfg.Logger().Debug("deleting resource owned by this release", + "kind", info.Mapping.GroupVersionKind.Kind, + "name", info.Name, + "namespace", info.Namespace, + "release", rel.Name) + } + _, errs = u.cfg.KubeClient.Delete(ownedResources, parseCascadingFlag(u.DeletionPropagation, u.cfg.Logger())) + } } - return resources, kept.String(), errs + return ownedResources, kept.String(), errs } -func parseCascadingFlag(cascadingFlag string) v1.DeletionPropagation { +func parseCascadingFlag(cascadingFlag string, logger *slog.Logger) v1.DeletionPropagation { switch cascadingFlag { case "orphan": return v1.DeletePropagationOrphan @@ -297,7 +420,7 @@ func parseCascadingFlag(cascadingFlag string) v1.DeletionPropagation { case "background": return v1.DeletePropagationBackground default: - slog.Debug("uninstall: given cascade value, defaulting to delete propagation background", "value", cascadingFlag) + logger.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 aeac98142..90f69d639 100644 --- a/pkg/action/uninstall_test.go +++ b/pkg/action/uninstall_test.go @@ -17,9 +17,10 @@ limitations under the License. package action import ( - "context" + "bytes" "errors" "io" + "log/slog" "testing" "github.com/stretchr/testify/assert" @@ -147,9 +148,16 @@ func TestUninstallRelease_Cascade(t *testing.T) { } }` require.NoError(t, unAction.cfg.Releases.Create(rel)) + + // Create dummy resources with Mapping but no Client - this skips ownership verification + // (nil Client is treated as owned) and goes directly to delete + dummyResources := kube.ResourceList{ + newDeploymentResource("secret", "", ""), + } + failer := unAction.cfg.KubeClient.(*kubefake.FailingKubeClient) failer.DeleteError = errors.New("Uninstall with cascade failed") - failer.BuildDummy = true + failer.DummyResources = dummyResources unAction.cfg.KubeClient = failer _, err := unAction.Run(rel.Name) require.Error(t, err) @@ -170,39 +178,177 @@ func TestUninstallRun_UnreachableKubeClient(t *testing.T) { assert.ErrorContains(t, err, "connection refused") } -func TestUninstall_WaitOptionsPassedDownstream(t *testing.T) { +func TestUninstallRelease_OwnershipVerification(t *testing.T) { is := assert.New(t) - unAction := uninstallAction(t) + // Create a buffer to capture log output + logBuffer := &bytes.Buffer{} + handler := slog.NewTextHandler(logBuffer, &slog.HandlerOptions{Level: slog.LevelDebug}) + + config := actionConfigFixture(t) + config.SetLogger(handler) + + unAction := NewUninstall(config) unAction.DisableHooks = true unAction.DryRun = false - unAction.WaitStrategy = kube.StatusWatcherStrategy + unAction.KeepHistory = true + + rel := releaseStub() + rel.Name = "ownership-test" + rel.Namespace = "default" + rel.Manifest = `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-configmap + labels: + app.kubernetes.io/managed-by: Helm + annotations: + meta.helm.sh/release-name: ownership-test + meta.helm.sh/release-namespace: default +data: + key: value` + require.NoError(t, config.Releases.Create(rel)) + + // Create dummy resources with proper ownership metadata + labels := map[string]string{ + "app.kubernetes.io/managed-by": "Helm", + } + annotations := map[string]string{ + "meta.helm.sh/release-name": "ownership-test", + "meta.helm.sh/release-namespace": "default", + } + dummyResources := kube.ResourceList{ + newDeploymentWithOwner("owned-deploy", "default", labels, annotations), + } + failer := config.KubeClient.(*kubefake.FailingKubeClient) + failer.DummyResources = dummyResources - // Use WithWaitContext as a marker WaitOption that we can track - ctx := context.Background() - unAction.WaitOptions = []kube.WaitOption{kube.WithWaitContext(ctx)} + resi, err := unAction.Run(rel.Name) + is.NoError(err) + is.NotNil(resi) + res, err := releaserToV1Release(resi.Release) + is.NoError(err) + is.Equal(common.StatusUninstalled, res.Info.Status) + + // Verify log contains debug message about deleting owned resource + logOutput := logBuffer.String() + is.Contains(logOutput, "deleting resource owned by this release") + is.Contains(logOutput, "owned-deploy") + is.Contains(logOutput, "Deployment") +} + +func TestUninstallRelease_OwnershipVerification_WithKeepPolicy(t *testing.T) { + is := assert.New(t) + + // Create a buffer to capture log output + logBuffer := &bytes.Buffer{} + handler := slog.NewTextHandler(logBuffer, &slog.HandlerOptions{Level: slog.LevelWarn}) + + config := actionConfigFixture(t) + config.SetLogger(handler) + + unAction := NewUninstall(config) + unAction.DisableHooks = true + unAction.DryRun = false + unAction.KeepHistory = true rel := releaseStub() - rel.Name = "wait-options-uninstall" - rel.Manifest = `{ - "apiVersion": "v1", - "kind": "Secret", - "metadata": { - "name": "secret" - }, - "type": "Opaque", - "data": { - "password": "password" - } - }` - require.NoError(t, unAction.cfg.Releases.Create(rel)) + rel.Name = "keep-and-ownership" + rel.Namespace = "default" + rel.Manifest = `apiVersion: v1 +kind: Secret +metadata: + name: kept-secret + annotations: + helm.sh/resource-policy: keep + meta.helm.sh/release-name: keep-and-ownership + meta.helm.sh/release-namespace: default + labels: + app.kubernetes.io/managed-by: Helm +type: Opaque +data: + password: cGFzc3dvcmQ= +--- +apiVersion: v1 +kind: ConfigMap +metadata: + name: deleted-configmap + labels: + app.kubernetes.io/managed-by: Helm + annotations: + meta.helm.sh/release-name: keep-and-ownership + meta.helm.sh/release-namespace: default +data: + key: value` + require.NoError(t, config.Releases.Create(rel)) - // Access the underlying FailingKubeClient to check recorded options - failer := unAction.cfg.KubeClient.(*kubefake.FailingKubeClient) + // Create dummy resources - one unowned to test logging + dummyResources := kube.ResourceList{ + newDeploymentWithOwner("unowned-deploy", "default", nil, nil), + } + failer := config.KubeClient.(*kubefake.FailingKubeClient) + failer.DummyResources = dummyResources - _, err := unAction.Run(rel.Name) + res, err := unAction.Run(rel.Name) + is.NoError(err) + is.NotNil(res) + // Should contain info about kept resources + is.Contains(res.Info, "kept due to the resource policy") + + // Verify log contains warning about skipped unowned resource + logOutput := logBuffer.String() + is.Contains(logOutput, "skipping delete of resource not owned by this release") + is.Contains(logOutput, "unowned-deploy") +} + +func TestUninstallRelease_DryRun_OwnershipVerification(t *testing.T) { + is := assert.New(t) + + // Create a buffer to capture log output + logBuffer := &bytes.Buffer{} + handler := slog.NewTextHandler(logBuffer, &slog.HandlerOptions{Level: slog.LevelWarn}) + + config := actionConfigFixture(t) + config.SetLogger(handler) + + unAction := NewUninstall(config) + unAction.DisableHooks = true + unAction.DryRun = true + + rel := releaseStub() + rel.Name = "dryrun-ownership" + rel.Namespace = "default" + rel.Manifest = `apiVersion: v1 +kind: ConfigMap +metadata: + name: test-configmap + labels: + app.kubernetes.io/managed-by: Helm + annotations: + meta.helm.sh/release-name: dryrun-ownership + meta.helm.sh/release-namespace: default +data: + key: value` + require.NoError(t, config.Releases.Create(rel)) + + // Create dummy resources - one unowned to test dry-run logging + dummyResources := kube.ResourceList{ + newDeploymentWithOwner("dryrun-unowned-deploy", "default", nil, nil), + } + failer := config.KubeClient.(*kubefake.FailingKubeClient) + failer.DummyResources = dummyResources + + resi, err := unAction.Run(rel.Name) + is.NoError(err) + is.NotNil(resi) + is.NotNil(resi.Release) + res, err := releaserToV1Release(resi.Release) is.NoError(err) + is.Equal("dryrun-ownership", res.Name) - // Verify that WaitOptions were passed to GetWaiter - is.NotEmpty(failer.RecordedWaitOptions, "WaitOptions should be passed to GetWaiter") + // Verify log contains dry-run warning about resources that would be skipped + logOutput := logBuffer.String() + is.Contains(logOutput, "dry-run: would skip resource") + is.Contains(logOutput, "dryrun-unowned-deploy") + is.Contains(logOutput, "Deployment") } diff --git a/pkg/action/validate.go b/pkg/action/validate.go index 94bf4906b..cce8e32ee 100644 --- a/pkg/action/validate.go +++ b/pkg/action/validate.go @@ -95,7 +95,7 @@ func existingResourceConflict(resources kube.ResourceList, releaseName, releaseN if err := checkOwnership(existing, releaseName, releaseNamespace); err != nil { return fmt.Errorf("%s exists and cannot be imported into the current release: %s", resourceString(info), err) } - + // Resources that are not found are skipped because they are already deleted and do not need deletion. infoCopy := *info requireUpdate.Append(&infoCopy) return nil @@ -104,6 +104,69 @@ func existingResourceConflict(resources kube.ResourceList, releaseName, releaseN return requireUpdate, err } +// unverifiableResource pairs a resource with the error encountered while attempting +// to verify its ownership (for example, RBAC or network failures). +type unverifiableResource struct { + Info *resource.Info + Err error +} + +// verifyOwnershipBeforeDelete checks that resources in the list are owned by the specified release. +// It returns three lists: +// - owned: resources confirmed to be owned by the release (safe to delete). +// - unowned: resources that exist but are not owned by the release (should be skipped). +// - unverifiable: resources whose ownership could not be determined due to a fetch +// error (e.g. RBAC or network issues), paired with the underlying error. +// +// Resources that are not found on the server are excluded from all returned lists, +// since they have already been deleted and require no further action. +func verifyOwnershipBeforeDelete(resources kube.ResourceList, releaseName, releaseNamespace string) (kube.ResourceList, kube.ResourceList, []unverifiableResource, error) { + var owned kube.ResourceList + var unowned kube.ResourceList + var unverifiable []unverifiableResource + + err := resources.Visit(func(info *resource.Info, err error) error { + if err != nil { + return err + } + + // If client is not available, skip verification (test scenario or build failure) + if info.Client == nil { + infoCopy := *info + owned.Append(&infoCopy) + return nil + } + + helper := resource.NewHelper(info.Client, info.Mapping) + existing, err := helper.Get(info.Namespace, info.Name) + if err != nil { + if apierrors.IsNotFound(err) { + // Resource already deleted; nothing to do. + return nil + } + // Cannot fetch resource (network/permission issue); ownership unverifiable. + infoCopy := *info + unverifiable = append(unverifiable, unverifiableResource{Info: &infoCopy, Err: err}) + return nil + } + + // Verify ownership of the existing resource + if err := checkOwnership(existing, releaseName, releaseNamespace); err != nil { + // Resource not owned by this release, cannot delete + infoCopy := *info + unowned.Append(&infoCopy) + return nil + } + + // Resource is owned by this release, can delete + infoCopy := *info + owned.Append(&infoCopy) + return nil + }) + + return owned, unowned, unverifiable, err +} + func checkOwnership(obj runtime.Object, releaseName, releaseNamespace string) error { lbls, err := accessor.Labels(obj) if err != nil { diff --git a/pkg/action/validate_test.go b/pkg/action/validate_test.go index 2223f57d5..8e983e9ff 100644 --- a/pkg/action/validate_test.go +++ b/pkg/action/validate_test.go @@ -211,6 +211,98 @@ func TestCheckOwnership(t *testing.T) { assert.EqualError(t, err, `invalid ownership metadata; label validation error: key "app.kubernetes.io/managed-by" must equal "Helm": current value is "helm"`) } +func TestVerifyOwnershipBeforeDelete(t *testing.T) { + var ( + releaseName = "rel-a" + releaseNamespace = "ns-a" + labels = map[string]string{ + appManagedByLabel: appManagedByHelm, + } + annotations = map[string]string{ + helmReleaseNameAnnotation: releaseName, + helmReleaseNamespaceAnnotation: releaseNamespace, + } + wrongAnnotations = map[string]string{ + helmReleaseNameAnnotation: "rel-b", + helmReleaseNamespaceAnnotation: releaseNamespace, + } + ) + + // Test all resources properly owned + t.Run("all resources owned", func(t *testing.T) { + owned1 := newDeploymentWithOwner("owned1", "ns-a", labels, annotations) + owned2 := newDeploymentWithOwner("owned2", "ns-a", labels, annotations) + resources := kube.ResourceList{owned1, owned2} + + ownedList, unownedList, _, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) + assert.NoError(t, err) + assert.Len(t, ownedList, 2) + assert.Len(t, unownedList, 0) + }) + + // Test mix of owned and unowned resources + t.Run("mixed ownership", func(t *testing.T) { + owned := newDeploymentWithOwner("owned", "ns-a", labels, annotations) + unowned := newDeploymentWithOwner("unowned", "ns-a", labels, wrongAnnotations) + resources := kube.ResourceList{owned, unowned} + + ownedList, unownedList, _, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) + assert.NoError(t, err) + assert.Len(t, ownedList, 1) + assert.Len(t, unownedList, 1) + assert.Equal(t, "owned", ownedList[0].Name) + assert.Equal(t, "unowned", unownedList[0].Name) + }) + + // Test resource not found (should be skipped - not in either list) + t.Run("resource not found", func(t *testing.T) { + missing := newMissingDeployment("missing", "ns-a") + resources := kube.ResourceList{missing} + + ownedList, unownedList, _, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) + assert.NoError(t, err) + assert.Len(t, ownedList, 0) + assert.Len(t, unownedList, 0) + }) + + // Test resource with no ownership metadata + t.Run("no ownership metadata", func(t *testing.T) { + noMeta := newDeploymentWithOwner("no-meta", "ns-a", nil, nil) + resources := kube.ResourceList{noMeta} + + ownedList, unownedList, _, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) + assert.NoError(t, err) + assert.Len(t, ownedList, 0) + assert.Len(t, unownedList, 1) + }) + + // Test resource owned by different release + t.Run("owned by different release", func(t *testing.T) { + otherRelease := newDeploymentWithOwner("other", "ns-a", labels, wrongAnnotations) + resources := kube.ResourceList{otherRelease} + + ownedList, unownedList, _, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) + assert.NoError(t, err) + assert.Len(t, ownedList, 0) + assert.Len(t, unownedList, 1) + }) + + // Test mixed scenario: owned, unowned, and missing resources + t.Run("mixed with missing resources", func(t *testing.T) { + owned := newDeploymentWithOwner("owned", "ns-a", labels, annotations) + unowned := newDeploymentWithOwner("unowned", "ns-a", labels, wrongAnnotations) + missing := newMissingDeployment("missing", "ns-a") + resources := kube.ResourceList{owned, unowned, missing} + + ownedList, unownedList, _, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) + assert.NoError(t, err) + assert.Len(t, ownedList, 1) + assert.Len(t, unownedList, 1) + assert.Equal(t, "owned", ownedList[0].Name) + assert.Equal(t, "unowned", unownedList[0].Name) + }) +} + func TestSetMetadataVisitor(t *testing.T) { var ( err error