fix(uninstall): honor resource-policy keep set on the live object by ruslan-shaydullin · Pull Request #32769 · helm/helm · GitHub
Skip to content

fix(uninstall): honor resource-policy keep set on the live object - #32769

Open
ruslan-shaydullin wants to merge 1 commit into
helm:mainfrom
ruslan-shaydullin:fix/uninstall-live-resource-policy
Open

ruslan-shaydullin wants to merge 1 commit into
helm:mainfrom
ruslan-shaydullin:fix/uninstall-live-resource-policy

Conversation

@ruslan-shaydullin

Copy link
Copy Markdown
Contributor

What this PR does / why we need it:

helm uninstall only checks helm.sh/resource-policy in the manifest stored with the release. If you add keep to an object that is already deployed, for example with kubectl annotate, uninstall ignores it and deletes the object.

$ helm install demo ./chart
$ kubectl annotate configmap demo-data helm.sh/resource-policy=keep

# main
$ helm uninstall demo
release "demo" uninstalled
$ kubectl get configmap demo-data
Error from server (NotFound): configmaps "demo-data" not found

# this branch
$ helm uninstall demo
These resources were kept due to the resource policy:
[ConfigMap] demo-data

release "demo" uninstalled
$ kubectl get configmap demo-data
NAME        DATA   AGE
demo-data   1      1s

helm upgrade already honors the same live annotation when a resource is dropped from the chart, so right now the two commands disagree.

verifyOwnershipBeforeDelete (#31584) already reads every resource from the cluster before uninstall deletes it, so this adds no requests. Owned resources now carry that live object, and uninstall keeps the ones whose live annotation is keep, using the same value check as for the stored manifest. They show up in the existing kept list, also with --dry-run, and they are not passed to Delete or waited on with --wait.

A resource is kept if keep is set in either place:

keep in chart keep on live object before after
yes removed kept kept
no yes deleted kept
no no deleted deleted

No new flag. As @joejulian said in #8132, "the act of adding the annotation is the opt-in".

Fixes #10890
Related to #8132

Special notes for your reviewer:

I didn't put the check in Client.Delete like #10893 did, because Client.Delete is also used for hooks and for --cleanup-on-fail. Hooks are created with server-side apply, so a hook Job left in place by a live keep would be patched instead of recreated on the next run, and an already completed Job would not run again. Doing it in the uninstall path also gives the kept output and --dry-run, and NotFound is already handled by the ownership check (the point @dmvolod raised on #10893). Thanks to @ml0renz0 for the first attempt.

Ownership is still checked first, so a resource of another release with a live keep stays in the "not owned" list.

One more case changes: keep on an item of a kind: List template. filterManifestsToKeep only reads the annotations of the List itself, so main deletes the item; with this change the live object has the annotation and the item is kept.

The docs already say uninstall skips resources with this annotation, but since uninstall now keeps something it used to delete, I'd suggest 4.4.0 rather than a patch release. Objects that got keep from some automation and never had it removed will now survive uninstall (kubectl annotate <kind> <name> helm.sh/resource-policy- removes it). I can open a short helm-www PR once this is merged.

Left out on purpose: upgrade still compares the value exactly (a live ' Keep ' is kept by uninstall but deleted by upgrade), and the sequenced uninstall in #32314 would need the same filter in verifyOwnedForDelete. No Helm 3 backport, since dev-v3 no longer takes bug fixes.

Tests cover the new filter, the live object in owned resources, and deleteRelease and --dry-run with a live keep. I also compared main and this branch on k3s v1.35.5, including --dry-run, --wait, --wait=legacy and an annotation left by a second field manager after the chart dropped it.

If applicable:

  • this PR contains user facing changes (the docs needed label should be applied if so)
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

helm uninstall decided what to keep only from the annotations in the
manifest stored with the release. A helm.sh/resource-policy: keep
annotation added to the live object, for example with kubectl
annotate, was ignored and the resource was deleted. helm upgrade
already reads the same annotation from the live object when a
resource is removed from the chart, and keeps it.

verifyOwnershipBeforeDelete already fetches each resource before
uninstall deletes it. Owned resources now carry that live object, and
uninstall keeps the ones whose live annotation is keep, using the same
value rule as for the stored manifest. A resource is kept when either
source says keep. Live kept resources are listed with the other kept
resources, also with --dry-run, and are left out of the delete and
wait lists, so --wait does not wait for them.

The check is not done in Client.Delete. Client.Delete also deletes
hooks and, with --cleanup-on-fail, resources created by a failed
upgrade or rollback, so honoring a live keep there would change those
paths too.

Signed-off-by: Ruslan Shaydullin <shaydullin.r.d@outlook.com>
@pull-request-size pull-request-size Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 16, 2026
@github-actions github-actions Bot added the v4.x Issues and Pull Requests related to the major version v4 label Oct 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files. v4.x Issues and Pull Requests related to the major version v4

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Annotation "helm.sh/resource-policy": keep not honored when uninstalling

1 participant