pkg/helm/release/manager.go: refactor to use ThreeWayMergePatch for native k8s objects (#2869)

A follow-up PR to #2808 and #2777. @joelanford

In this PR we are using the Kubernetes strategic three-way merge patch for patches by the helm-operator for native Kubernetes objects.
This ensures that we are using the correct merge strategy that is defined in the OpenAPI schema of the object. #2808

However, we do not have the required schema information for custom resources(CRs). So we need to fall back to the previous manual json-patch.
To detect CRs we are using the same method as helm https://github.com/helm/helm/blob/b21b00978539ea8270e6b672bc1e6bc07af61475/pkg/kube/client.go#L391

Co-authored-by: Joe Lanford <[email protected]>
Co-authored-by: Henning Surmeier <[email protected]>

Author: 3 people

changelog/fragments/2869-helm-three-way.yaml

@@ -0,0 +1,19 @@
+# entries is a list of entries to include in
+# release notes and/or the migration guide
+entries:
+  - description: >
+      In Helm-based operators, reconcile logic now uses three-way strategic merge patches
+      for native kubernetes objects so that array patch strategies are correctly honored
+      and applied.
+
+    # kind is one of:
+    # - addition
+    # - change
+    # - deprecation
+    # - removal
+    # - bugfix
+    kind: change
+
+    # Is this a breaking change?
+    breaking: false
+

pkg/helm/release/manager.go

@@ -23,6 +23,9 @@ import (
 	"strings"
 
 	"gomodules.xyz/jsonpatch/v3"
+	helmkube "helm.sh/helm/v3/pkg/kube"
+	apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1"
+
 	"helm.sh/helm/v3/pkg/action"
 	cpb "helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/kube"
@@ -33,6 +36,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	apitypes "k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/strategicpatch"
 	"k8s.io/cli-runtime/pkg/resource"
 	"k8s.io/client-go/rest"
 
@@ -241,52 +245,87 @@ func reconcileRelease(ctx context.Context, kubeClient kube.Interface, expectedMa
 	}
 	return expectedInfos.Visit(func(expected *resource.Info, err error) error {
 		if err != nil {
-			return err
+			return fmt.Errorf("visit error: %w", err)
 		}
 
 		expectedClient := resource.NewClientWithOptions(expected.Client, func(r *rest.Request) {
 			*r = *r.Context(ctx)
 		})
 		helper := resource.NewHelper(expectedClient, expected.Mapping)
 
-		existing, err := helper.Get(expected.Namespace, expected.Name, false)
+		existing, err := helper.Get(expected.Namespace, expected.Name, expected.Export)
 		if apierrors.IsNotFound(err) {
 			if _, err := helper.Create(expected.Namespace, true, expected.Object,
 				&metav1.CreateOptions{}); err != nil {
 				return fmt.Errorf("create error: %s", err)
 			}
 			return nil
 		} else if err != nil {
-			return err
+			return fmt.Errorf("could not get object: %w", err)
 		}
 
-		patch, err := generatePatch(existing, expected.Object)
+		// Replicate helm's patch creation, which will create a Three-Way-Merge patch for
+		// native kubernetes Objects and fall back to a JSON merge patch for unstructured Objects such as CRDs
+		// We also extend the JSON merge patch by ignoring "remove" operations for fields added by kubernetes
+		// Reference in the helm source code:
+		// https://github.com/helm/helm/blob/1c9b54ad7f62a5ce12f87c3ae55136ca20f09c98/pkg/kube/client.go#L392
+		patch, patchType, err := createPatch(existing, expected)
 		if err != nil {
-			return fmt.Errorf("failed to marshal JSON patch: %w", err)
+			return fmt.Errorf("error creating patch: %w", err)
 		}
 
 		if patch == nil {
+			// nothing to do
 			return nil
 		}
 
-		_, err = helper.Patch(expected.Namespace, expected.Name, apitypes.JSONPatchType, patch, &metav1.PatchOptions{})
+		_, err = helper.Patch(expected.Namespace, expected.Name, patchType, patch,
+			&metav1.PatchOptions{})
 		if err != nil {
 			return fmt.Errorf("patch error: %w", err)
 		}
 		return nil
 	})
 }
 
-func generatePatch(existing, expected runtime.Object) ([]byte, error) {
+func createPatch(existing runtime.Object, expected *resource.Info) ([]byte, apitypes.PatchType, error) {
 	existingJSON, err := json.Marshal(existing)
 	if err != nil {
-		return nil, err
+		return nil, apitypes.StrategicMergePatchType, err
 	}
-	expectedJSON, err := json.Marshal(expected)
+	expectedJSON, err := json.Marshal(expected.Object)
 	if err != nil {
-		return nil, err
+		return nil, apitypes.StrategicMergePatchType, err
 	}
 
+	// Get a versioned object
+	versionedObject := helmkube.AsVersioned(expected)
+
+	// Unstructured objects, such as CRDs, may not have an not registered error
+	// returned from ConvertToVersion. Anything that's unstructured should
+	// use the jsonpatch.CreateMergePatch. Strategic Merge Patch is not supported
+	// on objects like CRDs.
+	_, isUnstructured := versionedObject.(runtime.Unstructured)
+
+	// On newer K8s versions, CRDs aren't unstructured but has this dedicated type
+	_, isCRD := versionedObject.(*apiextv1beta1.CustomResourceDefinition)
+
+	if isUnstructured || isCRD {
+		// fall back to generic JSON merge patch
+		patch, err := createJSONMergePatch(existingJSON, expectedJSON)
+		return patch, apitypes.JSONPatchType, err
+	}
+
+	patchMeta, err := strategicpatch.NewPatchMetaFromStruct(versionedObject)
+	if err != nil {
+		return nil, apitypes.StrategicMergePatchType, err
+	}
+
+	patch, err := strategicpatch.CreateThreeWayMergePatch(expectedJSON, expectedJSON, existingJSON, patchMeta, true)
+	return patch, apitypes.StrategicMergePatchType, err
+}
+
+func createJSONMergePatch(existingJSON, expectedJSON []byte) ([]byte, error) {
 	ops, err := jsonpatch.CreatePatch(existingJSON, expectedJSON)
 	if err != nil {
 		return nil, err

pkg/helm/release/manager_test.go

@@ -15,18 +15,25 @@
 package release
 
 import (
-	"encoding/json"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	apitypes "k8s.io/apimachinery/pkg/types"
+	"k8s.io/cli-runtime/pkg/resource"
+
+	appsv1 "k8s.io/api/apps/v1"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/runtime"
 )
 
-func newTestDeployment(containers []interface{}) *unstructured.Unstructured {
+func newTestUnstructured(containers []interface{}) *unstructured.Unstructured {
 	return &unstructured.Unstructured{
 		Object: map[string]interface{}{
-			"kind":       "Deployment",
-			"apiVersion": "apps/v1",
+			"kind":       "MyResource",
+			"apiVersion": "myApi",
 			"metadata": map[string]interface{}{
 				"name":      "test",
 				"namespace": "ns",
@@ -42,99 +49,167 @@ func newTestDeployment(containers []interface{}) *unstructured.Unstructured {
 	}
 }
 
-func TestManagerGeneratePatch(t *testing.T) {
+func newTestDeployment(containers []v1.Container) *appsv1.Deployment {
+	return &appsv1.Deployment{
+		TypeMeta:   metav1.TypeMeta{Kind: "Deployment", APIVersion: "apps/v1"},
+		ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"},
+		Spec: appsv1.DeploymentSpec{
+			Template: v1.PodTemplateSpec{
+				Spec: v1.PodSpec{
+					Containers: containers,
+				},
+			},
+		},
+	}
+}
+
+func TestManagerGenerateStrategicMergePatch(t *testing.T) {
 
 	tests := []struct {
-		o1    *unstructured.Unstructured
-		o2    *unstructured.Unstructured
-		patch []map[string]interface{}
+		o1        runtime.Object
+		o2        runtime.Object
+		patch     string
+		patchType apitypes.PatchType
 	}{
 		{
-			o1: newTestDeployment([]interface{}{
+			o1: newTestUnstructured([]interface{}{
 				map[string]interface{}{
 					"name": "test1",
 				},
 				map[string]interface{}{
 					"name": "test2",
 				},
 			}),
-			o2: newTestDeployment([]interface{}{
+			o2: newTestUnstructured([]interface{}{
 				map[string]interface{}{
 					"name": "test1",
 				},
 			}),
-			patch: []map[string]interface{}{},
+			patch:     ``,
+			patchType: apitypes.JSONPatchType,
 		},
 		{
-			o1: newTestDeployment([]interface{}{
+			o1: newTestUnstructured([]interface{}{
 				map[string]interface{}{
 					"name": "test1",
 				},
 			}),
-			o2: newTestDeployment([]interface{}{
+			o2: newTestUnstructured([]interface{}{
 				map[string]interface{}{
 					"name": "test1",
 				},
 				map[string]interface{}{
 					"name": "test2",
 				},
 			}),
-			patch: []map[string]interface{}{
-				{
-					"op":   "add",
-					"path": "/spec/template/spec/containers/1",
-					"value": map[string]interface{}{
-						"name": string("test2"),
-					},
-				},
-			},
+			patch:     `[{"op":"add","path":"/spec/template/spec/containers/1","value":{"name":"test2"}}]`,
+			patchType: apitypes.JSONPatchType,
 		},
 		{
-			o1: newTestDeployment([]interface{}{
+			o1: newTestUnstructured([]interface{}{
 				map[string]interface{}{
 					"name": "test1",
 				},
 			}),
-			o2: newTestDeployment([]interface{}{
+			o2: newTestUnstructured([]interface{}{
 				map[string]interface{}{
 					"name": "test1",
 					"test": nil,
 				},
 			}),
-			patch: []map[string]interface{}{},
+			patch:     ``,
+			patchType: apitypes.JSONPatchType,
 		},
 		{
-			o1: newTestDeployment([]interface{}{
+			o1: newTestUnstructured([]interface{}{
 				map[string]interface{}{
 					"name": "test1",
 				},
 			}),
-			o2: newTestDeployment([]interface{}{
+			o2: newTestUnstructured([]interface{}{
 				map[string]interface{}{
 					"name": "test2",
 				},
 			}),
-			patch: []map[string]interface{}{
-				{
-					"op":    "replace",
-					"path":  "/spec/template/spec/containers/0/name",
-					"value": "test2",
+			patch:     `[{"op":"replace","path":"/spec/template/spec/containers/0/name","value":"test2"}]`,
+			patchType: apitypes.JSONPatchType,
+		},
+		{
+			o1: newTestDeployment([]v1.Container{
+				{Name: "test1"},
+				{Name: "test2"},
+			}),
+			o2: newTestDeployment([]v1.Container{
+				{Name: "test1"},
+			}),
+			patch:     `{"spec":{"template":{"spec":{"$setElementOrder/containers":[{"name":"test1"}]}}}}`, //nolint:lll
+			patchType: apitypes.StrategicMergePatchType,
+		},
+		{
+			o1: newTestDeployment([]v1.Container{
+				{Name: "test1"},
+			}),
+			o2: newTestDeployment([]v1.Container{
+				{Name: "test1"},
+				{Name: "test2"},
+			}),
+			patch:     `{"spec":{"template":{"spec":{"$setElementOrder/containers":[{"name":"test1"},{"name":"test2"}],"containers":[{"name":"test2","resources":{}}]}}}}`, //nolint:lll
+			patchType: apitypes.StrategicMergePatchType,
+		},
+		{
+			o1: newTestDeployment([]v1.Container{
+				{Name: "test1"},
+			}),
+			o2: newTestDeployment([]v1.Container{
+				{Name: "test1", LivenessProbe: nil},
+			}),
+			patch:     `{}`,
+			patchType: apitypes.StrategicMergePatchType,
+		},
+		{
+			o1: newTestDeployment([]v1.Container{
+				{Name: "test1"},
+			}),
+			o2: newTestDeployment([]v1.Container{
+				{Name: "test2"},
+			}),
+			patch:     `{"spec":{"template":{"spec":{"$setElementOrder/containers":[{"name":"test2"}],"containers":[{"name":"test2","resources":{}}]}}}}`, //nolint:lll
+			patchType: apitypes.StrategicMergePatchType,
+		},
+		{
+			o1: &appsv1.Deployment{
+				TypeMeta: metav1.TypeMeta{Kind: "Deployment", APIVersion: "apps/v1"},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "ns",
+					Annotations: map[string]string{
+						"testannotation": "testvalue",
+					},
 				},
+				Spec: appsv1.DeploymentSpec{},
 			},
+			o2: &appsv1.Deployment{
+				TypeMeta: metav1.TypeMeta{Kind: "Deployment", APIVersion: "apps/v1"},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "ns",
+				},
+				Spec: appsv1.DeploymentSpec{},
+			},
+			patch:     `{}`, //nolint:lll
+			patchType: apitypes.StrategicMergePatchType,
 		},
 	}
 
 	for _, test := range tests {
-		diff, err := generatePatch(test.o1, test.o2)
-		assert.NoError(t, err)
 
-		if len(test.patch) == 0 {
-			assert.Equal(t, 0, len(test.patch))
-		} else {
-			x := []map[string]interface{}{}
-			err = json.Unmarshal(diff, &x)
-			assert.NoError(t, err)
-			assert.Equal(t, test.patch, x)
+		o2Info := &resource.Info{
+			Object: test.o2,
 		}
+
+		diff, patchType, err := createPatch(test.o1, o2Info)
+		assert.NoError(t, err)
+		assert.Equal(t, test.patchType, patchType)
+		assert.Equal(t, test.patch, string(diff))
 	}
 }