fix(merge): ensure to have same data type while performing 3-way merge

[mittachaitu]

This PR adds the check to perform 3-way merge only if datatype matches to observed state data types else error will be returned.

Bug Description: merge func silently allows to merge when different data types are provided for 3-way merge and this leads merge data will not match to expected data. Following is an example(not an expected behaviour):

observed := map[string]interface{}{
	"key1": "old-value1",
	"key2": "old-value2",
}

desired := map[string]string{
	"key1": "value1",
	"key2": "value2",
}

got := map[string]interface{}{
	"key1": "old-value1",
	"key2": "old-value2",
}

The following error will be reported to the caller after this patch:

Can't merge desired changes: [data]: Expecting last applied as map[string]interface{}, got map[string]string

Signed-off-by: mittachaitu [email protected]

[grzesuav]

hi @mittachaitu , do you have maybe any example when it causes an issue ? It would be good to know some example. I will try to add the same to https://github.com/metacontroller/metacontroller

[mittachaitu]

hi @mittachaitu , do you have maybe any example when it causes an issue ? It would be good to know some example. I will try to add the same to https://github.com/metacontroller/metacontroller

Hi @grzesuav, I faced an issue when trying to merge K8s configmap. Following is the code snippet to reproduce the issue

observed := map[string]interface{}{
				"apiVersion": "v1",
				"kind":       "ConfigMap",
				"metadata": map[string]interface{}{
					"name":      "node-cm1",
					"namespace": "metac",
				},
				"data": map[string]interface{}{
					"node.properties": "data1",
				},
                    }

desired := map[string]interface{}{
				"apiVersion": "v1",
				"kind":       "ConfigMap",
				"metadata": map[string]interface{}{
					"name":      "node-cm1",
					"namespace": "metac",
				},
				"data": map[string]string{
					"node.properties": "data2",
				},
		 }

// Calling Merge will return observed state instead of observed
got, err := Merge(observed, nil, desired)
if err != nil {
   // return error
}

// Output will be the same as an observed state which is not expected
 got := 	map[string]interface{}{
	            "apiVersion": "v1",
		    "kind":       "ConfigMap",
		    "metadata": map[string]interface{}{
			 "name":      "node-cm1",
			 "namespace": "metac",
		     },
		     "data": map[string]string{
			 "node.properties": "data1",
		     },
              }

UnitTest case also available to test the same changes

[grzesuav]

@mittachaitu thanks :) will try to test it onmetacontroller :)

[grzesuav]

@mittachaitu may I ask where the issue arises ? As I understand it is because json data is differently serialized in golang ? Do you have maybe json example of kubernetes ConfigMap ?

[mittachaitu]

@mittachaitu may I ask where the issue arises ? As I understand it is because json data is differently serialized in golang ? Do you have maybe json example of kubernetes ConfigMap ?

@grzesuav Basically we are using metac as k8s library code for d-operators.

How we reached here: Since we are using metac as a library in d-operators to update any kind of k8s resource we are using merge to get data(k8s update call will be made using this data). Here data might not be serialized always there might be chances the d-operators may invoke directly by calling Apply(via NewApplier.Run())

Note: When we call merge func by unmarshaling serialized data then there won't be any issues. Issue arises only when we directly use it as a library call.

Sample JSON example:

apiVersion: v1
data:
  node.properties: |
    key1: value1
kind: ConfigMap
metadata:
  creationTimestamp: "2021-03-17T07:30:57Z"
  name: propel-node-cm1
  namespace: propel
  resourceVersion: "54933120"
  selfLink: /api/v1/namespaces/propel/configmaps/propel-node-cm1
  uid: c7d3870f-92bd-45ae-81f1-ae680cb35025