Merge pull request #31584 from banjoh/em/check-ownership-before-delete

feat: add ownership verification before deleting resources during uni…

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

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

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 {

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