Fix --take-ownership for custom resources - closes #30622 by p-se · Pull Request #30697 · helm/helm

p-se · 2025-03-21T11:23:30Z

If a resource exists in the cluster and is to be adopted by helm install --take-ownership, it is left unchanged while helm reports the installation to have succeeded.

This is due to CRs and CRDs being merged without three-way-merge, which results in an empty patch.

By using a three-way-merge transparently when --take-ownership is used, helm behaves as expected without breaking previous behavior.

Fixes #30622

What this PR does / why we need it:

Special notes for your reviewer:

If applicable:

this PR contains user facing changes (the docs needed label should be applied if so)
--take-ownership does not appear to be documented at all, so perhaps yes from that perspective, but no from the perspective of this PR.
this PR contains unit tests
this PR has been tested for backwards compatibility
The changes in this PR are meant to not change any existing behavior, except for how helm behaves when called with --take-ownership.

p-se · 2025-03-21T12:50:40Z

The YAML after a successful --take-ownership with the changes of this PR:

apiVersion: crd.com/v1
kind: Data
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"crd.com/v1","kind":"Data","metadata":{"annotations":{},"labels":{"test":"adopt"},"name":"crd-test","namespace":"kube-system"},"spec":{"color":"blue","size":"large"}}
    meta.helm.sh/release-name: foo
    meta.helm.sh/release-namespace: default
  creationTimestamp: "2025-03-21T12:37:34Z"
  generation: 2
  labels:
    app.kubernetes.io/managed-by: Helm
    test: adopt
  name: crd-test
  namespace: kube-system
  resourceVersion: "374259"
  uid: 4e87ddf2-2012-4ee7-a804-a8a7406c72b7
spec:
  color: blue
  size: large
  weight: heavy

banjoh · 2025-03-21T13:38:17Z

@p-se the PR needs to be merged to main first, then another PR cherry picking commits into dev-v3 needs to be created. Please have the PR point to main branch

banjoh

I tested a happy path test using the example in #30622 and it works. Testing a real use case would be needed.

banjoh · 2025-03-21T18:01:40Z

pkg/kube/interface.go

 	// Update updates one or more resources or creates the resource
 	// if it doesn't exist.
-	Update(original, target ResourceList, force bool) (*Result, error)
+	Update(original, target ResourceList, force bool, threeWayMergeForUnstructured bool) (*Result, error)


Since we are targeting to fix this in v3, this API change will break backward compatibility

Would you like me to add a method to the interface and use that new method instead of adapting the existing method to preserve backward compatibility?

I'll bring this PR up in the developer call tomorrow. Depending on the outcome, you might not need to make any change to this function.

Notes: https://docs.google.com/document/d/1d-6xJEx0C78csIYSPKJzRPeWaHG_8W1Hjl72OJggwdc/edit?tab=t.0#heading=h.cafxi2yfgqg1

If we can default to 3-way merging, the additional threeWayMergeForUnstructured parameter may not be needed. If we need the parameter, the way to extend the API is to create a new interface which kube Client implements. Take a look at here and here. In golang, extending an interface is a breaking change because any type implementing that interface will break

@p-se remember to add // TODO Helm 4: Remove this... in the new interface, similar to the other extensions

@p-se from today's developer call, it was agreed not to have to 3-way merging for CRDs and unstructured resources as the default behaviour so as not to surprise users with a change in behaviour.

We will keep the threeWayMergeForUnstructured boolean that will be true whenever --take-ownership flag is set.

Please create a new interface like below (or similar) that kube.Client implements, then revert the previous change you made. Once that's done this PR should be good to go

// TODO Helm 4: Remove InterfaceThreeWayMerge and integrate its method(s) into the Interface. type InterfaceThreeWayMerge interface { UpdateThreeWayMerge(original, target ResourceList, force bool, threeWayMergeForUnstructured bool) (*Result, error) }

robertsirc

Hello, thank you for your PR could you please update and ensure you have test.

pkg/action/install.go

banjoh · 2025-03-24T11:51:50Z

pkg/kube/client.go

 	_, isCRD := versionedObject.(*apiextv1beta1.CustomResourceDefinition)

 	if isUnstructured || isCRD {
+		if threeWayMergeForUnstructured {


@mattfarina in light of #13411 (review), is there any objection defaulting to 3-way merge for CRDs and unstructured resources in Helm v3, or should we have a CLI flag to opt into the merge strategy? I do not see any backward compatibility risk, but I might be missing something

For v4 I'm leaning towards having 3-way merge as default.

The result of the conversation in last Helm dev meeting is that it's ok to use 3-way merge for the CR during --take-ownership (given that it fixes the bug, which didn't work previously). But we should NOT change the behavior for CRs everywhere apart from take-ownership in Helm v3.

Perhaps we may want to do that in Helm v4 if we don't implement Server Side Apply before then, but let's hold off on that in this PR.

banjoh

LGTM

banjoh · 2025-04-07T06:32:44Z

pkg/action/install.go

 	} else if len(resources) > 0 {
-		_, err = i.cfg.KubeClient.Update(toBeAdopted, resources, i.Force)
+		if i.TakeOwnership {
+			if kubeClient, ok := i.cfg.KubeClient.(kube.InterfaceThreeWayMerge); ok {


nit: You could remove this check since there is var _ InterfaceThreeWayMerge = (*Client)(nil) check that ensures the client implements this API. If you want to keep this check, then return an error in an else block

If a resource exists in the cluster and is to be adopted by helm install --take-ownership, it is left unchanged while helm reports the installation to have succeeded. This is due to CRs and CRDs being merged without three-way-merge, which results in an empty patch. By using a three-way-merge transparently when --take-ownership is used, the helm behaves as expected without breaking previous behavior. Fixes helm#30622 Signed-off-by: Patrick Seidensal <pseidensal@suse.com>

scottrigby

@p-se nice work 👍 LGTM

Re @robertsirc #30697 (comment) about tests. I agree we need to add tests to PRs whenever possible. Currently this PR fixes a glaring bug for a feature that did not have tests for the feature, but as @banjoh noted there are several open PRs for that already: #30697 (comment). However, this PR does add tests for code that is changed—specifically the merge strategies—which I think is important and covers what we need for this PR.

scottrigby · 2025-04-11T20:19:04Z

Needs to remove the empty test commit (will also fix missing DCO) 250c69b and now needs to resolve conflicts in upstream pkg/kube/client.go.

ivelichkovich · 2025-04-14T20:43:02Z

Thanks for adding this, lgtm

Signed-off-by: Evans Mungai <mbuevans@gmail.com>

robertsirc

LGTM

pkg/action/install.go

pkg/kube/client.go

Signed-off-by: Evans Mungai <mbuevans@gmail.com>

scottrigby

Nice work. Approving again to allow merge

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 21, 2025

p-se changed the title ~~Fix --take-ownership~~ Fix --take-ownership for custom resources Mar 21, 2025

p-se marked this pull request as ready for review March 21, 2025 12:51

banjoh requested changes Mar 21, 2025

View reviewed changes

p-se force-pushed the fix-take-ownership branch from 0e4042d to c551686 Compare March 21, 2025 19:57

p-se changed the base branch from release-3.17 to main March 21, 2025 19:58

robertsirc requested changes Mar 21, 2025

View reviewed changes

pkg/action/install.go Outdated Show resolved Hide resolved

p-se changed the title ~~Fix --take-ownership for custom resources~~ Fix --take-ownership for custom resources - closes #30622 Mar 23, 2025

banjoh reviewed Mar 24, 2025

View reviewed changes

p-se mentioned this pull request Mar 24, 2025

[SURE-8849] fleet incorrectly reports pre-existing resources as missing when setting helm.takeOwnership: true rancher/fleet#2134

Closed

1 task

scottrigby added the bug Categorizes issue or PR as related to a bug. label Mar 25, 2025

p-se force-pushed the fix-take-ownership branch from c551686 to d166355 Compare April 4, 2025 09:58

banjoh approved these changes Apr 7, 2025

View reviewed changes

p-se force-pushed the fix-take-ownership branch 3 times, most recently from 01b94b6 to d360800 Compare April 7, 2025 07:29

p-se force-pushed the fix-take-ownership branch from d360800 to e55707b Compare April 7, 2025 11:55

scottrigby previously approved these changes Apr 7, 2025

View reviewed changes

scottrigby added the Has One Approval This PR has one approval. It still needs a second approval to be merged. label Apr 7, 2025

banjoh dismissed scottrigby’s stale review via a130d01 April 15, 2025 08:54

banjoh force-pushed the fix-take-ownership branch 3 times, most recently from 9dc6fab to f52e42d Compare April 15, 2025 09:33

Add install test for TakeOwnership flag

7c37a10

Signed-off-by: Evans Mungai <mbuevans@gmail.com>

banjoh force-pushed the fix-take-ownership branch from b3a8b60 to 7c37a10 Compare April 15, 2025 10:24

Merge branch 'main' into fix-take-ownership

e8e79cc

Signed-off-by: Evans Mungai <mbuevans@gmail.com>

robertsirc previously approved these changes Apr 16, 2025

View reviewed changes

pkg/action/install.go Show resolved Hide resolved

pkg/kube/client.go Show resolved Hide resolved

robertsirc added approved Indicates a PR has been approved by the required number of approvers and removed Has One Approval This PR has one approval. It still needs a second approval to be merged. labels Apr 16, 2025

fix formatting errors

1f5605a

Signed-off-by: Evans Mungai <mbuevans@gmail.com>

banjoh dismissed robertsirc’s stale review via 1f5605a April 17, 2025 14:33

scottrigby approved these changes Apr 17, 2025

View reviewed changes

scottrigby added the Needs v3 backport Label PRs for v4/main, which are still applicable to v3 so need a separate backport PR label Apr 17, 2025

scottrigby merged commit 599fad1 into helm:main Apr 17, 2025
5 checks passed

banjoh mentioned this pull request Apr 17, 2025

v3 backport - fix --take-ownership #30768

Merged

scottrigby removed the Needs v3 backport Label PRs for v4/main, which are still applicable to v3 so need a separate backport PR label Apr 21, 2025

scottrigby added v3 port complete For completed v2->v3 ports and removed approved Indicates a PR has been approved by the required number of approvers labels Nov 6, 2025

Conversation

p-se commented Mar 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

p-se commented Mar 21, 2025

Uh oh!

banjoh commented Mar 21, 2025

Uh oh!

banjoh left a comment

Choose a reason for hiding this comment

Uh oh!

banjoh Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

p-se Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

banjoh Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

banjoh Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

banjoh Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

banjoh Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

robertsirc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

banjoh Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottrigby Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

banjoh left a comment

Choose a reason for hiding this comment

Uh oh!

banjoh Apr 7, 2025

Choose a reason for hiding this comment

Uh oh!

scottrigby left a comment

Choose a reason for hiding this comment

Uh oh!

scottrigby commented Apr 11, 2025

Uh oh!

ivelichkovich commented Apr 14, 2025

Uh oh!

robertsirc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

scottrigby left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

p-se commented Mar 21, 2025 •

edited

Loading

p-se Mar 25, 2025 •

edited

Loading

banjoh Mar 24, 2025 •

edited

Loading

scottrigby Apr 1, 2025 •

edited

Loading

scottrigby left a comment •

edited

Loading