diff --git a/pkg/action/uninstall.go b/pkg/action/uninstall.go index 73927ed9d..b14305112 100644 --- a/pkg/action/uninstall.go +++ b/pkg/action/uninstall.go @@ -102,7 +102,7 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) resources, err := u.cfg.KubeClient.Build(strings.NewReader(builder.String()), false) if err == nil && len(resources) > 0 { - ownedResources, unownedResources, err := verifyOwnershipBeforeDelete(resources, r.Name, r.Namespace) + 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", @@ -116,6 +116,19 @@ func (u *Uninstall) Run(name string) (*releasei.UninstallReleaseResponse, error) } } + 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, @@ -207,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 { @@ -323,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 @@ -339,8 +352,9 @@ func (u *Uninstall) deleteRelease(rel *release.Release) (kube.ResourceList, stri // Verify ownership before deleting resources var ownedResources, unownedResources kube.ResourceList + var unverifiableResources []unverifiableResource if len(resources) > 0 { - ownedResources, unownedResources, err = verifyOwnershipBeforeDelete(resources, rel.Name, rel.Namespace) + 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)} } @@ -354,12 +368,34 @@ func (u *Uninstall) deleteRelease(rel *release.Release) (kube.ResourceList, stri "namespace", info.Namespace, "release", rel.Name) } - kept.WriteString(fmt.Sprintf("\n%d resource(s) were not deleted because they are not owned by this release:\n", len(unownedResources))) + 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 { diff --git a/pkg/action/uninstall_test.go b/pkg/action/uninstall_test.go index 2bf3d3878..3b6d09c33 100644 --- a/pkg/action/uninstall_test.go +++ b/pkg/action/uninstall_test.go @@ -148,7 +148,7 @@ func TestUninstallRelease_Cascade(t *testing.T) { "password": "password" } }` - unAction.cfg.Releases.Create(rel) + 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 @@ -208,7 +208,7 @@ metadata: meta.helm.sh/release-namespace: default data: key: value` - config.Releases.Create(rel) + require.NoError(t, config.Releases.Create(rel)) // Create dummy resources with proper ownership metadata labels := map[string]string{ @@ -281,7 +281,7 @@ metadata: meta.helm.sh/release-namespace: default data: key: value` - config.Releases.Create(rel) + require.NoError(t, config.Releases.Create(rel)) // Create dummy resources - one unowned to test logging dummyResources := kube.ResourceList{ @@ -330,7 +330,7 @@ metadata: meta.helm.sh/release-namespace: default data: key: value` - config.Releases.Create(rel) + require.NoError(t, config.Releases.Create(rel)) // Create dummy resources - one unowned to test dry-run logging dummyResources := kube.ResourceList{ diff --git a/pkg/action/validate.go b/pkg/action/validate.go index 88771053b..bc6cb4e9c 100644 --- a/pkg/action/validate.go +++ b/pkg/action/validate.go @@ -104,12 +104,26 @@ 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 two lists: owned resources (safe to delete) and unowned resources (should skip). -// Resources that are not found are considered owned (already deleted, safe to attempt delete). -func verifyOwnershipBeforeDelete(resources kube.ResourceList, releaseName, releaseNamespace string) (kube.ResourceList, kube.ResourceList, error) { +// 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 { @@ -127,12 +141,12 @@ func verifyOwnershipBeforeDelete(resources kube.ResourceList, releaseName, relea existing, err := helper.Get(info.Namespace, info.Name) if err != nil { if apierrors.IsNotFound(err) { - // Resource already deleted, skip deletion + // Resource already deleted; nothing to do. return nil } - // Cannot fetch resource (network/permission issue), cannot verify ownership + // Cannot fetch resource (network/permission issue); ownership unverifiable. infoCopy := *info - unowned.Append(&infoCopy) + unverifiable = append(unverifiable, unverifiableResource{Info: &infoCopy, Err: err}) return nil } @@ -150,7 +164,7 @@ func verifyOwnershipBeforeDelete(resources kube.ResourceList, releaseName, relea return nil }) - return owned, unowned, err + return owned, unowned, unverifiable, err } func checkOwnership(obj runtime.Object, releaseName, releaseNamespace string) error { diff --git a/pkg/action/validate_test.go b/pkg/action/validate_test.go index b425299f5..1cb1002d6 100644 --- a/pkg/action/validate_test.go +++ b/pkg/action/validate_test.go @@ -234,7 +234,7 @@ func TestVerifyOwnershipBeforeDelete(t *testing.T) { owned2 := newDeploymentWithOwner("owned2", "ns-a", labels, annotations) resources := kube.ResourceList{owned1, owned2} - ownedList, unownedList, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) + ownedList, unownedList, _, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) assert.NoError(t, err) assert.Len(t, ownedList, 2) assert.Len(t, unownedList, 0) @@ -246,7 +246,7 @@ func TestVerifyOwnershipBeforeDelete(t *testing.T) { unowned := newDeploymentWithOwner("unowned", "ns-a", labels, wrongAnnotations) resources := kube.ResourceList{owned, unowned} - ownedList, unownedList, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) + ownedList, unownedList, _, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) assert.NoError(t, err) assert.Len(t, ownedList, 1) assert.Len(t, unownedList, 1) @@ -259,7 +259,7 @@ func TestVerifyOwnershipBeforeDelete(t *testing.T) { missing := newMissingDeployment("missing", "ns-a") resources := kube.ResourceList{missing} - ownedList, unownedList, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) + ownedList, unownedList, _, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) assert.NoError(t, err) assert.Len(t, ownedList, 0) assert.Len(t, unownedList, 0) @@ -270,7 +270,7 @@ func TestVerifyOwnershipBeforeDelete(t *testing.T) { noMeta := newDeploymentWithOwner("no-meta", "ns-a", nil, nil) resources := kube.ResourceList{noMeta} - ownedList, unownedList, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) + ownedList, unownedList, _, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) assert.NoError(t, err) assert.Len(t, ownedList, 0) assert.Len(t, unownedList, 1) @@ -281,7 +281,7 @@ func TestVerifyOwnershipBeforeDelete(t *testing.T) { otherRelease := newDeploymentWithOwner("other", "ns-a", labels, wrongAnnotations) resources := kube.ResourceList{otherRelease} - ownedList, unownedList, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) + ownedList, unownedList, _, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) assert.NoError(t, err) assert.Len(t, ownedList, 0) assert.Len(t, unownedList, 1) @@ -294,7 +294,7 @@ func TestVerifyOwnershipBeforeDelete(t *testing.T) { missing := newMissingDeployment("missing", "ns-a") resources := kube.ResourceList{owned, unowned, missing} - ownedList, unownedList, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) + ownedList, unownedList, _, err := verifyOwnershipBeforeDelete(resources, releaseName, releaseNamespace) assert.NoError(t, err) assert.Len(t, ownedList, 1) assert.Len(t, unownedList, 1)