Use three way merge for unstructured objects

Based on work of wxdao <waxiadao@gmail.com> in the same issue,
supersedes #9938. Basically just adds tests.

closes #9937

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

cmd/helm/root.go

@@ -161,6 +161,8 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string
 	}
 	actionConfig.RegistryClient = registryClient
 
+	actionConfig.ThreeWayMergeForUnstructured = settings.ThreeWayMergeForUnstructured
+
 	// Add subcommands
 	cmd.AddCommand(
 		// chart commands

pkg/action/action.go

@@ -94,6 +94,11 @@ type Configuration struct {
 	// Capabilities describes the capabilities of the Kubernetes cluster.
 	Capabilities *chartutil.Capabilities
 
+	// ThreeWayMergeForUnstructured controls whether to use three way merge
+	// patch for unstructured objects (custom resource, custom definitions,
+	// etc).
+	ThreeWayMergeForUnstructured bool
+
 	Log func(string, ...interface{})
 }
 
@@ -373,6 +378,7 @@ func (cfg *Configuration) recordRelease(r *release.Release) {
 func (cfg *Configuration) Init(getter genericclioptions.RESTClientGetter, namespace, helmDriver string, log DebugLog) error {
 	kc := kube.New(getter)
 	kc.Log = log
+	kc.ThreeWayMergeForUnstructured = cfg.ThreeWayMergeForUnstructured
 
 	lazyClient := &lazyClient{
 		namespace: namespace,

pkg/cli/environment.go

@@ -89,6 +89,10 @@ type EnvSettings struct {
 	BurstLimit int
 	// QPS is queries per second which may be used to avoid throttling.
 	QPS float32
+	// ThreeWayMergeForUnstructured controls whether to use three way merge
+	// patch for unstructured objects (custom resource, custom definitions,
+	// etc).
+	ThreeWayMergeForUnstructured bool
 }
 
 func New() *EnvSettings {
@@ -111,6 +115,7 @@ func New() *EnvSettings {
 		QPS:                       envFloat32Or("HELM_QPS", defaultQPS),
 	}
 	env.Debug, _ = strconv.ParseBool(os.Getenv("HELM_DEBUG"))
+	env.ThreeWayMergeForUnstructured, _ = strconv.ParseBool(os.Getenv("HELM_THREE_WAY_MERGE_FOR_UNSTRUCTURED"))
 
 	// bind to kubernetes config flags
 	config := &genericclioptions.ConfigFlags{
@@ -160,6 +165,7 @@ func (s *EnvSettings) AddFlags(fs *pflag.FlagSet) {
 	fs.StringVar(&s.RepositoryCache, "repository-cache", s.RepositoryCache, "path to the directory containing cached repository indexes")
 	fs.IntVar(&s.BurstLimit, "burst-limit", s.BurstLimit, "client-side default throttling limit")
 	fs.Float32Var(&s.QPS, "qps", s.QPS, "queries per second used when communicating with the Kubernetes API, not including bursting")
+	fs.BoolVar(&s.ThreeWayMergeForUnstructured, "three-way-merge-for-unstructured", s.ThreeWayMergeForUnstructured, "use a three way merge patch for unstructured objects (custom resources, custom resource definitions, etc.)")
 }
 
 func envOr(name, def string) string {

pkg/kube/client.go

@@ -46,6 +46,8 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"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/apimachinery/pkg/watch"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
@@ -83,6 +85,11 @@ type Client struct {
 	// Namespace allows to bypass the kubeconfig file for the choice of the namespace
 	Namespace string
 
+	// ThreeWayMergeForUnstructured controls whether to use three way merge
+	// patch for unstructured objects (custom resource, custom definitions,
+	// etc).
+	ThreeWayMergeForUnstructured bool
+
 	kubeClient *kubernetes.Clientset
 }
 
@@ -617,7 +624,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")
@@ -645,7 +652,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)
 
@@ -653,6 +660,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
@@ -683,7 +703,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, c.ThreeWayMergeForUnstructured)
 		if err != nil {
 			return errors.Wrap(err, "failed to create patch")
 		}

pkg/kube/client_test.go

@@ -24,9 +24,15 @@ import (
 	"testing"
 	"time"
 
+	corev1 "k8s.io/api/core/v1"
 	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"
 	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest/fake"
@@ -682,6 +688,153 @@ func TestReal(t *testing.T) {
 	}
 }
 
+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()
+	corev1.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)
+}
+
 const testServiceManifest = `
 kind: Service
 apiVersion: v1