msb-public
/
helm
mirror of https://github.com/helm/helm
1
0
Code Issues Projects Releases Wiki Activity

fix: address Copilot review on ownership-verified uninstall

Browse Source 
- Distinguish unowned resources from those whose ownership could not be
verified (e.g. RBAC/network errors). Unverifiable resources are now
returned in a separate list with the underlying error so log output
and the "kept" summary describe what actually happened.
- Only prefix the kept summary with the resource-policy header when
there are policy-kept resources, so users with only ownership-skipped
resources don't see a misleading header.
- Update verifyOwnershipBeforeDelete doc comment to reflect that
not-found resources are excluded from all returned lists.
- Assert success of Releases.Create in uninstall tests so fixture
setup failures are surfaced immediately.

Signed-off-by: Evans Mungai <mbuevans@gmail.com>
pull/31584/head
Evans Mungai 6 days ago
parent 9728c5af31
commit 007713087b
No known key found for this signature in database
GPG Key ID: BBEB812143DD14E1
4 changed files with 75 additions and 25 deletions
Show Stats Download Patch File Download Diff File

52
pkg/action/uninstall.go
Unescape View File

@ -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 {

8
pkg/action/uninstall_test.go
Unescape View File

@ -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{

28
pkg/action/validate.go
Unescape View File

@ -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 {

12
pkg/action/validate_test.go
Unescape View File

@ -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)

Write Preview
Loading…
Cancel
Save