Skip to content

Make takeOwnership a standing claim and detect ownership drift - #418

Merged
jonasz-lasut merged 1 commit into
crossplane-contrib:mainfrom
stevendborrelli:take-ownership-standing-claim
Oct 14, 2026
Merged

Make takeOwnership a standing claim and detect ownership drift#418
jonasz-lasut merged 1 commit into
crossplane-contrib:mainfrom
stevendborrelli:take-ownership-standing-claim

Conversation

@stevendborrelli

@stevendborrelli stevendborrelli commented Oct 9, 2026

Copy link
Copy Markdown
Member

Description of your changes

Previously the takeOwnership option was designed to work once: after the first successful application of the labels status is updated and it would not detect or process any other changes in ownership.

This change changes the behavior to observe the metadata of the rendered resources and take ownership in the case of drift.

  • pass TakeOwnership through on every deploy while the field is set, instead of masking it with the recorded ownershipTaken status
  • detect ownership drift in Observe: compare live ownership metadata (managed-by label, both meta.helm.sh annotations) on every rendered resource, and report the observation out of date when any differ so a deploy re-stamps them; keeps status.synced and Available untouched
  • skip the check when TakeOwnership is unset, or when spec drift already owes a deploy; fetch PartialObjectMetadata so the per-reconcile lookups only carry metadata
  • status.atProvider.ownershipTaken becomes informational; API docs and regenerated CRD descriptions updated to the new semantics

Fixes #

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

@stevendborrelli
stevendborrelli force-pushed the take-ownership-standing-claim branch from 767ae33 to 7e720ff Compare October 13, 2026 10:32
jonasz-lasut
jonasz-lasut reviewed Oct 13, 2026
View reviewed changes
Comment thread pkg/controller/cluster/release/release.go Outdated
Comment thread pkg/controller/cluster/release/release.go
Comment thread pkg/clients/helm/ownership.go
@jonasz-lasut

jonasz-lasut commented Oct 13, 2026

Copy link
Copy Markdown
Collaborator

Hi @stevendborrelli thank you for your contribution, I've added 3 comments. Also please sign the DCO

@stevendborrelli @claude
Detect ownership drift for releases with takeOwnership
61e5b18 
takeOwnership adopts a pre-existing release once, but nothing afterwards
guarded that adoption: when another actor re-stamped the ownership
metadata on an already-adopted resource, nothing in the chart, values,
or digest changed, Helm only validates ownership for resources it is
about to create, and the release sat "up to date" while somebody else
owned part of it.

- Observe compares live ownership metadata (managed-by label, both
  meta.helm.sh annotations) on every rendered resource while
  takeOwnership is set, via the new OwnershipDrifted helper, and reports
  out of date where any differ so the triggered upgrade re-stamps them;
  status.synced and Available stay untouched. The check runs only when
  no deploy is owed from other causes, and fetches PartialObjectMetadata
  so per-reconcile lookups carry only metadata
- adoption itself keeps its one-time gate on the deploy path: Helm
  force-stamps ownership metadata on every upgrade regardless of
  TakeOwnership, so the drift-triggered upgrade is the repair, while
  adopting on every deploy would silently take over pre-existing objects
  a chart bump starts rendering - the failure the one-time design exists
  to prevent
- releases still on client-side apply are exempt from detection: their
  patches derive from the manifests alone, so ownership keys the chart
  renders unchanged never change and detection would retry on every
  poll forever until ssaForceConflicts migrates them to ssa
- field docs and regenerated CRDs document the ssaForceConflicts conflict
  consequence and the behavior change for existing takeOwnership releases
- unit tests for drift detection, Observe gating (including the CSA
  exemption), and the one-shot deploy gate; verified end to end on kind
  (adoption, control, steady state) and against the stock uptest suite

Addresses PR review comments on adoption gating, the CSA retry loop, and
the provider-upgrade behavior change (kept field-gated with docs warning
per maintainer preference).

Co-Authored-By: Claude Code <noreply@anthropic.com>
Signed-off-by: Steven Borrelli <steve@borrelli.org>
@stevendborrelli
stevendborrelli force-pushed the take-ownership-standing-claim branch from 7e720ff to 61e5b18 Compare October 13, 2026 14:30
@jonasz-lasut

jonasz-lasut commented Oct 14, 2026

Copy link
Copy Markdown
Collaborator

Release notes snippet so I don;t forget to mention behavioral changes introduced in this PR

Important

Release objects with spec.forProvider.takeOwnership: true now check the Helm ownership metadata of their rendered resources on every poll. If another actor (kubectl, a GitOps controller, the Helm CLI) has re-stamped app.kubernetes.io/managed-by or the meta.helm.sh/release-name /
meta.helm.sh/release-namespace annotations on one of those resources under its own field manager, the repair upgrade fails with a server-side apply conflict and the Release turns failed on the first poll after upgrading the provider, with no change to its spec. Set
spec.forProvider.ssaForceConflicts: true to let the provider take the fields back, or remove takeOwnership to stop the retries. Releases still on client-side apply (installed before v1.4.0 and never upgraded with ssaForceConflicts) are not checked.

Highlights

  • Ownership drift detection: takeOwnership now keeps a Release's claim on its resources live instead of adopting them once and never looking again

New Features

  • spec.forProvider.takeOwnership now detects ownership drift (both cluster-scoped and namespaced APIs). On every poll the provider compares the app.kubernetes.io/managed-by label and the meta.helm.sh/release-name and meta.helm.sh/release-namespace annotations of every chart-rendered
    resource against the release, and triggers an upgrade to re-stamp any that differ. Adopting pre-existing resources stays a one-time operation on the first deploy, recorded in status.atProvider.ownershipTaken. Only releases applied server-side are checked; client-side apply releases are left
    alone until ssaForceConflicts migrates them. See the callout above for the behavior change on existing releases.

jonasz-lasut
jonasz-lasut approved these changes Oct 14, 2026
View reviewed changes

@jonasz-lasut jonasz-lasut left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution @stevendborrelli LGTM!
Thank you for discussing the options and making sure that we have the same understanding on how this feature is supposed to work

@jonasz-lasut
jonasz-lasut merged commit 864d50a into crossplane-contrib:main Oct 14, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jonasz-lasut jonasz-lasut jonasz-lasut approved these changes
@turkenh turkenh Awaiting requested review from turkenh turkenh is a code owner
@lsviben lsviben Awaiting requested review from lsviben lsviben is a code owner
@erhancagirici erhancagirici Awaiting requested review from erhancagirici erhancagirici is a code owner
@sergenyalcin sergenyalcin Awaiting requested review from sergenyalcin sergenyalcin is a code owner
@turkenf turkenf Awaiting requested review from turkenf turkenf is a code owner
@ulucinar ulucinar Awaiting requested review from ulucinar ulucinar is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@stevendborrelli @jonasz-lasut