Fix --take-ownership

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 #30622

Signed-off-by: Patrick Seidensal <pseidensal@suse.com>

pkg/action/install.go

@@ -465,7 +465,11 @@ func (i *Install) performInstall(rel *release.Release, toBeAdopted kube.Resource
 	if len(toBeAdopted) == 0 && len(resources) > 0 {
 		_, err = i.cfg.KubeClient.Create(resources)
 	} else if len(resources) > 0 {
-		_, err = i.cfg.KubeClient.Update(toBeAdopted, resources, i.Force)
+		if i.TakeOwnership {
+			_, err = i.cfg.KubeClient.(kube.InterfaceThreeWayMerge).UpdateThreeWayMerge(toBeAdopted, resources, i.Force)
+		} else {
+			_, err = i.cfg.KubeClient.Update(toBeAdopted, resources, i.Force)
+		}
 	}
 	if err != nil {
 		return rel, err

pkg/kube/client.go

@@ -43,6 +43,8 @@ import (
 	metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/jsonmergepatch"
+	"k8s.io/apimachinery/pkg/util/mergepatch"
 	"k8s.io/apimachinery/pkg/util/strategicpatch"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 	"k8s.io/cli-runtime/pkg/resource"
@@ -399,14 +401,7 @@ func (c *Client) BuildTable(reader io.Reader, validate bool) (ResourceList, erro
 	return result, scrubValidationError(err)
 }
 
-// Update takes the current list of objects and target list of objects and
+func (c *Client) update(original, target ResourceList, force, threeWayMerge bool) (*Result, error) {
-// creates resources that don't already exist, updates resources that have been
-// modified in the target configuration, and deletes resources from the current
-// configuration that are not present in the target configuration. If an error
-// occurs, a Result will still be returned with the error, containing all
-// resource updates, creations, and deletions that were attempted. These can be
-// used for cleanup or other logging purposes.
-func (c *Client) Update(original, target ResourceList, force bool) (*Result, error) {
 	updateErrors := []string{}
 	res := &Result{}
 
@@ -441,7 +436,7 @@ func (c *Client) Update(original, target ResourceList, force bool) (*Result, err
 			return errors.Errorf("no %s with the name %q found", kind, info.Name)
 		}
 
-		if err := updateResource(c, info, originalInfo.Object, force); err != nil {
+		if err := updateResource(c, info, originalInfo.Object, force, threeWayMerge); err != nil {
 			c.Log("error updating the resource %q:\n\t %v", info.Name, err)
 			updateErrors = append(updateErrors, err.Error())
 		}
@@ -482,6 +477,31 @@ func (c *Client) Update(original, target ResourceList, force bool) (*Result, err
 	return res, nil
 }
 
+// Update takes the current list of objects and target list of objects and
+// creates resources that don't already exist, updates resources that have been
+// modified in the target configuration, and deletes resources from the current
+// configuration that are not present in the target configuration. If an error
+// occurs, a Result will still be returned with the error, containing all
+// resource updates, creations, and deletions that were attempted. These can be
+// used for cleanup or other logging purposes.
+//
+// The difference to Update is that UpdateThreeWayMerge does a three-way-merge
+// for unstructured objects.
+func (c *Client) UpdateThreeWayMerge(original, target ResourceList, force bool) (*Result, error) {
+	return c.update(original, target, force, true)
+}
+
+// Update takes the current list of objects and target list of objects and
+// creates resources that don't already exist, updates resources that have been
+// modified in the target configuration, and deletes resources from the current
+// configuration that are not present in the target configuration. If an error
+// occurs, a Result will still be returned with the error, containing all
+// resource updates, creations, and deletions that were attempted. These can be
+// used for cleanup or other logging purposes.
+func (c *Client) Update(original, target ResourceList, force bool) (*Result, error) {
+	return c.update(original, target, force, false)
+}
+
 // Delete deletes Kubernetes resources specified in the resources list with
 // background cascade deletion. It will attempt to delete all resources even
 // if one or more fail and collect any errors. All successfully deleted items
@@ -591,7 +611,7 @@ func deleteResource(info *resource.Info, policy metav1.DeletionPropagation) erro
 		})
 }
 
-func createPatch(target *resource.Info, current runtime.Object) ([]byte, types.PatchType, error) {
+func createPatch(target *resource.Info, current runtime.Object, threeWayMergeForUnstructured bool) ([]byte, types.PatchType, error) {
 	oldData, err := json.Marshal(current)
 	if err != nil {
 		return nil, types.StrategicMergePatchType, errors.Wrap(err, "serializing current configuration")
@@ -619,7 +639,7 @@ func createPatch(target *resource.Info, current runtime.Object) ([]byte, types.P
 
 	// Unstructured objects, such as CRDs, may not have a not registered error
 	// returned from ConvertToVersion. Anything that's unstructured should
-	// use the jsonpatch.CreateMergePatch. Strategic Merge Patch is not supported
+	// use generic JSON merge patch. Strategic Merge Patch is not supported
 	// on objects like CRDs.
 	_, isUnstructured := versionedObject.(runtime.Unstructured)
 
@@ -627,6 +647,19 @@ func createPatch(target *resource.Info, current runtime.Object) ([]byte, types.P
 	_, isCRD := versionedObject.(*apiextv1beta1.CustomResourceDefinition)
 
 	if isUnstructured || isCRD {
+		if threeWayMergeForUnstructured {
+			// from https://github.com/kubernetes/kubectl/blob/b83b2ec7d15f286720bccf7872b5c72372cb8e80/pkg/cmd/apply/patcher.go#L129
+			preconditions := []mergepatch.PreconditionFunc{
+				mergepatch.RequireKeyUnchanged("apiVersion"),
+				mergepatch.RequireKeyUnchanged("kind"),
+				mergepatch.RequireMetadataKeyUnchanged("name"),
+			}
+			patch, err := jsonmergepatch.CreateThreeWayJSONMergePatch(oldData, newData, currentData, preconditions...)
+			if err != nil && mergepatch.IsPreconditionFailed(err) {
+				err = fmt.Errorf("%w: at least one field was changed: apiVersion, kind or name", err)
+			}
+			return patch, types.MergePatchType, err
+		}
 		// fall back to generic JSON merge patch
 		patch, err := jsonpatch.CreateMergePatch(oldData, newData)
 		return patch, types.MergePatchType, err
@@ -641,7 +674,7 @@ func createPatch(target *resource.Info, current runtime.Object) ([]byte, types.P
 	return patch, types.StrategicMergePatchType, err
 }
 
-func updateResource(c *Client, target *resource.Info, currentObj runtime.Object, force bool) error {
+func updateResource(c *Client, target *resource.Info, currentObj runtime.Object, force, threeWayMergeForUnstructured bool) error {
 	var (
 		obj    runtime.Object
 		helper = resource.NewHelper(target.Client, target.Mapping).WithFieldManager(getManagedFieldsManager())
@@ -657,7 +690,7 @@ func updateResource(c *Client, target *resource.Info, currentObj runtime.Object,
 		}
 		c.Log("Replaced %q with kind %s for kind %s", target.Name, currentObj.GetObjectKind().GroupVersionKind().Kind, kind)
 	} else {
-		patch, patchType, err := createPatch(target, currentObj)
+		patch, patchType, err := createPatch(target, currentObj, threeWayMergeForUnstructured)
 		if err != nil {
 			return errors.Wrap(err, "failed to create patch")
 		}

pkg/kube/client_test.go

@@ -27,8 +27,13 @@ import (
 	"github.com/stretchr/testify/assert"
 
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	jsonserializer "k8s.io/apimachinery/pkg/runtime/serializer/json"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/cli-runtime/pkg/resource"
 	k8sfake "k8s.io/client-go/kubernetes/fake"
 	"k8s.io/client-go/kubernetes/scheme"
@@ -208,7 +213,7 @@ func TestCreate(t *testing.T) {
 	})
 }
 
-func TestUpdate(t *testing.T) {
+func testUpdate(t *testing.T, threeWayMerge bool) {
 	listA := newPodList("starfish", "otter", "squid")
 	listB := newPodList("starfish", "otter", "dolphin")
 	listC := newPodList("starfish", "otter", "dolphin")
@@ -279,7 +284,12 @@ func TestUpdate(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	result, err := c.Update(first, second, false)
+	var result *Result
+	if threeWayMerge {
+		result, err = c.UpdateThreeWayMerge(first, second, false)
+	} else {
+		result, err = c.Update(first, second, false)
+	}
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -328,6 +338,14 @@ func TestUpdate(t *testing.T) {
 	}
 }
 
+func TestUpdate(t *testing.T) {
+	testUpdate(t, false)
+}
+
+func TestUpdateThreeWayMerge(t *testing.T) {
+	testUpdate(t, true)
+}
+
 func TestBuild(t *testing.T) {
 	tests := []struct {
 		name      string
@@ -913,3 +931,150 @@ spec:
 
 var resourceQuotaConflict = []byte(`
 {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"Operation cannot be fulfilled on resourcequotas \"quota\": the object has been modified; please apply your changes to the latest version and try again","reason":"Conflict","details":{"name":"quota","kind":"resourcequotas"},"code":409}`)
+
+type createPatchTestCase struct {
+	name string
+
+	// The target state.
+	target *unstructured.Unstructured
+	// The current state as it exists in the release.
+	current *unstructured.Unstructured
+	// The actual state as it exists in the cluster.
+	actual *unstructured.Unstructured
+
+	threeWayMergeForUnstructured bool
+	// The patch is supposed to transfer the current state to the target state,
+	// thereby preserving the actual state, wherever possible.
+	expectedPatch     string
+	expectedPatchType types.PatchType
+}
+
+func (c createPatchTestCase) run(t *testing.T) {
+	scheme := runtime.NewScheme()
+	v1.AddToScheme(scheme)
+	encoder := jsonserializer.NewSerializerWithOptions(
+		jsonserializer.DefaultMetaFactory, scheme, scheme, jsonserializer.SerializerOptions{
+			Yaml: false, Pretty: false, Strict: true,
+		},
+	)
+	objBody := func(obj runtime.Object) io.ReadCloser {
+		return io.NopCloser(bytes.NewReader([]byte(runtime.EncodeOrDie(encoder, obj))))
+	}
+	header := make(http.Header)
+	header.Set("Content-Type", runtime.ContentTypeJSON)
+	restClient := &fake.RESTClient{
+		NegotiatedSerializer: unstructuredSerializer,
+		Resp: &http.Response{
+			StatusCode: 200,
+			Body:       objBody(c.actual),
+			Header:     header,
+		},
+	}
+
+	targetInfo := &resource.Info{
+		Client:    restClient,
+		Namespace: "default",
+		Name:      "test-obj",
+		Object:    c.target,
+		Mapping: &meta.RESTMapping{
+			Resource: schema.GroupVersionResource{
+				Group:    "crd.com",
+				Version:  "v1",
+				Resource: "datas",
+			},
+			Scope: meta.RESTScopeNamespace,
+		},
+	}
+
+	patch, patchType, err := createPatch(targetInfo, c.current, c.threeWayMergeForUnstructured)
+	if err != nil {
+		t.Fatalf("Failed to create patch: %v", err)
+	}
+
+	if c.expectedPatch != string(patch) {
+		t.Errorf("Unexpected patch.\nTarget:\n%s\nCurrent:\n%s\nActual:\n%s\n\nExpected:\n%s\nGot:\n%s",
+			c.target,
+			c.current,
+			c.actual,
+			c.expectedPatch,
+			string(patch),
+		)
+	}
+
+	if patchType != types.MergePatchType {
+		t.Errorf("Expected patch type %s, got %s", types.MergePatchType, patchType)
+	}
+}
+
+func newTestCustomResourceData(metadata map[string]string, spec map[string]interface{}) *unstructured.Unstructured {
+	if metadata == nil {
+		metadata = make(map[string]string)
+	}
+	if _, ok := metadata["name"]; !ok {
+		metadata["name"] = "test-obj"
+	}
+	if _, ok := metadata["namespace"]; !ok {
+		metadata["namespace"] = "default"
+	}
+	o := map[string]interface{}{
+		"apiVersion": "crd.com/v1",
+		"kind":       "Data",
+		"metadata":   metadata,
+	}
+	if len(spec) > 0 {
+		o["spec"] = spec
+	}
+	return &unstructured.Unstructured{
+		Object: o,
+	}
+}
+
+func TestCreatePatchCustomResourceMetadata(t *testing.T) {
+	target := newTestCustomResourceData(map[string]string{
+		"meta.helm.sh/release-name":      "foo-simple",
+		"meta.helm.sh/release-namespace": "default",
+		"objectset.rio.cattle.io/id":     "default-foo-simple",
+	}, nil)
+	testCase := createPatchTestCase{
+		name:    "take ownership of resource",
+		target:  target,
+		current: target,
+		actual: newTestCustomResourceData(nil, map[string]interface{}{
+			"color": "red",
+		}),
+		threeWayMergeForUnstructured: true,
+		expectedPatch:                `{"metadata":{"meta.helm.sh/release-name":"foo-simple","meta.helm.sh/release-namespace":"default","objectset.rio.cattle.io/id":"default-foo-simple"}}`,
+		expectedPatchType:            types.MergePatchType,
+	}
+	t.Run(testCase.name, testCase.run)
+
+	// Previous behavior.
+	testCase.threeWayMergeForUnstructured = false
+	testCase.expectedPatch = `{}`
+	t.Run(testCase.name, testCase.run)
+}
+
+func TestCreatePatchCustomResourceSpec(t *testing.T) {
+	target := newTestCustomResourceData(nil, map[string]interface{}{
+		"color": "red",
+		"size":  "large",
+	})
+	testCase := createPatchTestCase{
+		name:    "merge with spec of existing custom resource",
+		target:  target,
+		current: target,
+		actual: newTestCustomResourceData(nil, map[string]interface{}{
+			"color":  "red",
+			"weight": "heavy",
+		}),
+		threeWayMergeForUnstructured: true,
+		expectedPatch:                `{"spec":{"size":"large"}}`,
+		expectedPatchType:            types.MergePatchType,
+	}
+	t.Run(testCase.name, testCase.run)
+
+	// Previous behavior.
+	testCase.threeWayMergeForUnstructured = false
+	testCase.expectedPatch = `{}`
+	t.Run(testCase.name, testCase.run)
+}

pkg/kube/fake/fake.go

@@ -123,6 +123,14 @@ func (f *FailingKubeClient) Update(r, modified kube.ResourceList, ignoreMe bool)
 	return f.PrintingKubeClient.Update(r, modified, ignoreMe)
 }
 
+// Update returns the configured error if set or prints
+func (f *FailingKubeClient) UpdateThreeWayMerge(r, modified kube.ResourceList, ignoreMe bool) (*kube.Result, error) {
+	if f.UpdateError != nil {
+		return &kube.Result{}, f.UpdateError
+	}
+	return f.PrintingKubeClient.Update(r, modified, ignoreMe)
+}
+
 // Build returns the configured error if set or prints
 func (f *FailingKubeClient) Build(r io.Reader, _ bool) (kube.ResourceList, error) {
 	if f.BuildError != nil {

pkg/kube/interface.go

@@ -53,6 +53,13 @@ type Interface interface {
 	GetWaiter(ws WaitStrategy) (Waiter, error)
 }
 
+// InterfaceThreeWayMerge was introduced to avoid breaking backwards compatibility for Interface implementers.
+//
+// TODO Helm 4: Remove InterfaceThreeWayMerge and integrate its method(s) into the Interface.
+type InterfaceThreeWayMerge interface {
+	UpdateThreeWayMerge(original, target ResourceList, force bool) (*Result, error)
+}
+
 // Waiter defines methods related to waiting for resource states.
 type Waiter interface {
 	// Wait waits up to the given timeout for the specified resources to be ready.
@@ -118,6 +125,7 @@ type InterfaceResources interface {
 }
 
 var _ Interface = (*Client)(nil)
+var _ InterfaceThreeWayMerge = (*Client)(nil)
 var _ InterfaceLogs = (*Client)(nil)
 var _ InterfaceDeletionPropagation = (*Client)(nil)
 var _ InterfaceResources = (*Client)(nil)