Visitar URL original
fix: check ownership before delete in upgrade/rollback/install (#32218) by silence-coding · Pull Request #32793 · helm/helm · GitHub
Skip to content

fix: check ownership before delete in upgrade/rollback/install (#32218) - #32793

Open
silence-coding wants to merge 1 commit into
helm:mainfrom
silence-coding:fix/ownership-check-delete-path
Open

silence-coding wants to merge 1 commit into
helm:mainfrom
silence-coding:fix/ownership-check-delete-path

Conversation

@silence-coding

@silence-coding silence-coding commented Oct 21, 2026 •

Copy link
Copy Markdown

What this PR does / why we need it:

Fixes #32218.

The delete phase of Update (pkg/kube/client.go) is annotation-blind: a resource present in the original release but absent from the target is deleted without consulting meta.helm.sh/release-name. When a resource has been adopted by another release via --take-ownership, the source release still deletes it on its next upgrade/rollback, corrupting the adopter's state.

The uninstall path already guards against this via verifyOwnershipBeforeDelete (#31584). This PR extends the same principle to the shared update() delete loop, which is reached by upgrade, rollback, and install.

Changes:

  • Add opt-in ClientUpdateOptionOwnership(releaseName) to pkg/kube/client.go. When set, the delete loop reads the live resource's meta.helm.sh/release-name annotation and skips deletion when it belongs to a different release. Resources owned by the operating release, or carrying no annotation, are deleted as before. When releaseName is empty the safeguard is disabled and behavior is unchanged.
  • Wire the option into all three Update call sites: upgrade.go (releasingUpgrade), rollback.go (performRollback), install.go (performInstall).
  • Only release-name is checked (not namespace) to avoid edge-case false skips across namespaces. The existing private constant in pkg/action/validate.go is left untouched; pkg/kube uses its own package-private constant since it cannot import pkg/action.

Special notes for your reviewer:

This is an alternative to the stalled draft #32257 (upgrade-only). Differences:

  • Covers all three Update call sites — upgrade, rollback, and install — not just upgrade. They share the same update() delete loop.
  • Checks only meta.helm.sh/release-name (not release-namespace) to avoid edge-case false skips for cluster-scoped or cross-namespace resources.
  • Single-param ClientUpdateOptionOwnership(releaseName) option.

The three action call sites always pass the release name, so the safeguard is always active in practice. Keeping it an explicit option preserves the pkg/kube API contract (HIP-0004): Update's option list is additive, and clientUpdateOptions.releaseName defaults to empty (disabled), so any direct caller of pkg/kube.Client.Update sees no behavior change.

A true non-destructive transfer (adopt-before-delete, no delete/recreate) is a larger enhancement and is out of scope for this fix. This PR targets the correctness goal in #32218: a release must not delete a resource owned by another release.

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

User-facing change: helm upgrade/rollback/install no longer deletes a resource that has been adopted by another release (via --take-ownership), and logs skipping delete of resource not owned by this release when skipping. No docs update needed — the existing --take-ownership documentation already describes the intended behavior ("Subsequent upgrades and uninstalls then treat those objects as part of the release"); this fix makes the actual behavior match that description for the source release.

Test plan
  • go build ./... passes
  • go vet ./pkg/kube/ ./pkg/action/ passes
  • go test ./pkg/kube/... ./pkg/action/... passes
  • New unit test in pkg/kube/client_test.go:
    • TestUpdateOwnershipCheck: foreign-owned resource skipped; owned + no-annotation resources deleted; option omitted → all deleted (regression)
  • Integration tests on k3s (v1.36.5), fixed vs. unmodified build:
    • One-way transfer: A installs shared-cm, B adopts via --take-ownership, upgrade A removes it. Fixed build: shared-cm survives (owned by b). Unmodified: deleted.
    • Regular upgrade (regression): A removes its own resource (no transfer). Fixed build: resource deleted as before; own resources retained.
    • Bidirectional swap (A↔B, both via upgrade): cr1 from A→B, cr2 from B→A. Fixed build: both resources survive with correct ownership. Unmodified: the resource adopted by the first-upgrading release is deleted by the second-upgrading release.

Backward compatibility: the pkg/kube.Client.Update signature is unchanged; only a new ClientUpdateOption is added. Direct callers that do not pass ClientUpdateOptionOwnership see identical behavior. CLI flags and workflows are unchanged. This conforms to HIP-0004.

@pull-request-size pull-request-size Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 21, 2026
@github-actions github-actions Bot added the v4.x Issues and Pull Requests related to the major version v4 label Oct 21, 2026
@silence-coding
silence-coding force-pushed the fix/ownership-check-delete-path branch 2 times, most recently from 58ad34b to 9d59131 Compare October 21, 2026 14:17
The delete phase of Update (pkg/kube/client.go) is annotation-blind: a
resource present in the original release but absent from the target is
deleted without consulting meta.helm.sh/release-name. When a resource has
been adopted by another release (via --take-ownership), the source release
still deletes it on its next upgrade/rollback, corrupting the adopter's
state. This is helm#32218.

The uninstall path already guards against this via verifyOwnershipBeforeDelete
(helm#31584). This extends the same principle to the shared update() delete loop,
which is reached by upgrade, rollback, and install.

Add an opt-in ClientUpdateOptionOwnership(releaseName). When set, the delete
loop reads the live resource's release-name annotation and skips deletion
when it belongs to a different release. Resources owned by the operating
release, or carrying no annotation, are deleted as before. When releaseName
is empty the safeguard is disabled and behavior is unchanged.

Wire the option into all three Update call sites:
  - pkg/action/upgrade.go   (releasingUpgrade)
  - pkg/action/rollback.go  (performRollback)
  - pkg/action/install.go   (performInstall)

Compared to the stalled draft helm#32257 (upgrade-only), this also covers
rollback and install, and checks only release-name (not namespace) to
avoid edge-case false skips across namespaces.

Verified on a k3s cluster with a cross-release transfer scenario: after
release B adopts a resource from A via --take-ownership, upgrading A no
longer deletes it. The unmodified build deletes it. A bidirectional swap
(A->B and B->A via upgrade) keeps both resources alive with correct
ownership on the fixed build; the unmodified build loses one. Regular
upgrades that remove a release's own resources still delete them.

Signed-off-by: silence-coding <32766901+silence-coding@users.noreply.github.com>
@silence-coding
silence-coding force-pushed the fix/ownership-check-delete-path branch from 9d59131 to 71f840c Compare October 21, 2026 14:20
@silence-coding

Copy link
Copy Markdown
Author

@TerryHowe Could you take a look at this when you have a moment? I figured you'd be familiar with this area. This addresses #32218.

I'm happy to make any changes based on your feedback.

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.

Helm ignores ownership during upgrade

1 participant