Skip to content

Commit

Permalink
fix: don't remove defaulted fields and rely only on three way diff me…
Browse files Browse the repository at this point in the history 
…rge during diffing (argoproj#68)

* fix: don't remove defaulted fields and rely only on three way diff merge during diffing
  • Loading branch information
Alexander Matyushentsev authored Jun 30, 2020
1 parent ce9616a commit c23d4d7
Show file tree
Hide file tree
Showing 5 changed files with 204 additions and 12 deletions.
48 changes: 38 additions & 10 deletions pkg/diff/diff.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,19 +118,31 @@ func TwoWayDiff(config, live *unstructured.Unstructured) (*DiffResult, error) {
}
}

// Workaround for https://github.com/kubernetes/kubernetes/issues/74819.
// The volumeClaimTemplates field does not support merge strategy. Unfortunately we cannot even inject proper
// strategic merge patch metadata because merge key is `matadata.name` but `strategicpatch` package does not
// nested merge keys. So the only option is poor man merge patch using `volumeClaimTemplates` index as a merge key.
func statefulSetWorkaround(orig, live *unstructured.Unstructured) {
origTemplate, ok, err := unstructured.NestedSlice(orig.Object, "spec", "volumeClaimTemplates")
if !ok || err != nil {
return
}

liveTemplate, ok, err := unstructured.NestedSlice(live.Object, "spec", "volumeClaimTemplates")
if !ok || err != nil {
return
}

_ = unstructured.SetNestedField(live.Object, jsonutil.RemoveListFields(origTemplate, liveTemplate), "spec", "volumeClaimTemplates")
}

// ThreeWayDiff performs a diff with the understanding of how to incorporate the
// last-applied-configuration annotation in the diff.
// Inputs are assumed to be stripped of type information
func ThreeWayDiff(orig, config, live *unstructured.Unstructured) (*DiffResult, error) {
orig = removeNamespaceAnnotation(orig)
config = removeNamespaceAnnotation(config)

// Remove defaulted fields from the live object.
// This subtracts any extra fields in the live object which are not present in last-applied-configuration.
// This is needed to perform a fair comparison when we send the objects to gojsondiff
// TODO: Remove line below to fix https://github.com/argoproj/argo-cd/issues/2865 and add special case for StatefulSet
live = &unstructured.Unstructured{Object: jsonutil.RemoveMapFields(orig.Object, live.Object)}

// 1. calculate a 3-way merge patch
patchBytes, versionedObject, err := threeWayMergePatch(orig, config, live)
if err != nil {
Expand Down Expand Up @@ -223,11 +235,18 @@ func threeWayMergePatch(orig, config, live *unstructured.Unstructured) ([]byte,
if err != nil {
return nil, nil, err
}
liveBytes, err := json.Marshal(live.Object)
if err != nil {
return nil, nil, err
}

if versionedObject, err := scheme.Scheme.New(orig.GroupVersionKind()); err == nil {
gk := orig.GroupVersionKind().GroupKind()
if (gk.Group == "apps" || gk.Group == "extensions") && gk.Kind == "StatefulSet" {
statefulSetWorkaround(orig, live)
}

liveBytes, err := json.Marshal(live.Object)
if err != nil {
return nil, nil, err
}

lookupPatchMeta, err := strategicpatch.NewPatchMetaFromStruct(versionedObject)
if err != nil {
return nil, nil, err
Expand All @@ -238,6 +257,15 @@ func threeWayMergePatch(orig, config, live *unstructured.Unstructured) ([]byte,
}
return patch, versionedObject, nil
} else {
// Remove defaulted fields from the live object.
// This subtracts any extra fields in the live object which are not present in last-applied-configuration.
live = &unstructured.Unstructured{Object: jsonutil.RemoveMapFields(orig.Object, live.Object)}

liveBytes, err := json.Marshal(live.Object)
if err != nil {
return nil, nil, err
}

patch, err := jsonmergepatch.CreateThreeWayJSONMergePatch(origBytes, configBytes, liveBytes)
if err != nil {
return nil, nil, err
Expand Down
14 changes: 14 additions & 0 deletions pkg/diff/diff_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,20 @@ func TestThreeWayDiffExample2(t *testing.T) {
log.Println(ascii)
}

// Tests a real world example
func TestThreeWayDiffExample3(t *testing.T) {
configUn := unmarshalFile("testdata/elasticsearch-config.json")
liveUn := unmarshalFile("testdata/elasticsearch-live.json")

dr := diff(t, configUn, liveUn, GetDefaultDiffOptions())
assert.False(t, dr.Modified)
ascii, err := printDiff(dr, liveUn)
assert.Nil(t, err)
if ascii != "" {
log.Println(ascii)
}
}

// TestThreeWayDiffExample2WithDifference is same as TestThreeWayDiffExample2 but with differences
func TestThreeWayDiffExample2WithDifference(t *testing.T) {
configUn := unmarshalFile("testdata/elasticsearch-config.json")
Expand Down
42 changes: 42 additions & 0 deletions pkg/diff/testdata/deployment-config.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
{
"apiVersion": "apps/v1",
"kind": "Deployment",
"metadata": {
"name": "guestbook-ui"
},
"spec": {
"replicas": 1,
"revisionHistoryLimit": 3,
"selector": {
"matchLabels": {
"app": "guestbook-ui"
}
},
"template": {
"metadata": {
"labels": {
"app": "guestbook-ui"
}
},
"spec": {
"containers": [
{
"image": "gcr.io/heptio-images/ks-guestbook-demo:0.2",
"name": "guestbook-ui",
"ports": [
{
"containerPort": 80
}
],
"env": [
{
"name": "VAR1",
"value": "something"
}
]
}
]
}
}
}
}
108 changes: 108 additions & 0 deletions pkg/diff/testdata/deployment-live.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
{
"apiVersion": "apps/v1",
"kind": "Deployment",
"metadata": {
"annotations": {
"deployment.kubernetes.io/revision": "5",
"kubectl.kubernetes.io/last-applied-configuration": "{\"apiVersion\":\"apps/v1\",\"kind\":\"Deployment\",\"metadata\":{\"annotations\":{},\"labels\":{\"app.kubernetes.io/instance\":\"guestbook\"},\"name\":\"guestbook-ui\",\"namespace\":\"default\"},\"spec\":{\"replicas\":1,\"revisionHistoryLimit\":3,\"selector\":{\"matchLabels\":{\"app\":\"guestbook-ui\"}},\"template\":{\"metadata\":{\"labels\":{\"app\":\"guestbook-ui\"}},\"spec\":{\"containers\":[{\"env\":[{\"name\":\"VAR1\",\"value\":\"something\"}],\"image\":\"gcr.io/heptio-images/ks-guestbook-demo:0.2\",\"name\":\"guestbook-ui\",\"ports\":[{\"containerPort\":80}]}]}}}}\n"
},
"creationTimestamp": "2020-06-25T19:25:05Z",
"generation": 5,
"labels": {
"app.kubernetes.io/instance": "guestbook"
},
"name": "guestbook-ui",
"namespace": "default",
"resourceVersion": "1208550",
"selfLink": "/apis/apps/v1/namespaces/default/deployments/guestbook-ui",
"uid": "165ca30d-8dca-44ae-b0ba-6ea26160ac9f"
},
"spec": {
"progressDeadlineSeconds": 600,
"replicas": 1,
"revisionHistoryLimit": 3,
"selector": {
"matchLabels": {
"app": "guestbook-ui"
}
},
"strategy": {
"rollingUpdate": {
"maxSurge": "25%",
"maxUnavailable": "25%"
},
"type": "RollingUpdate"
},
"template": {
"metadata": {
"creationTimestamp": null,
"labels": {
"app": "guestbook-ui"
}
},
"spec": {
"containers": [
{
"env": [
{
"name": "VAR2",
"valueFrom": {
"fieldRef": {
"apiVersion": "v1",
"fieldPath": "metadata.name"
}
}
},
{
"name": "VAR1",
"value": "something"
}
],
"image": "gcr.io/heptio-images/ks-guestbook-demo:0.2",
"imagePullPolicy": "IfNotPresent",
"name": "guestbook-ui",
"ports": [
{
"containerPort": 80,
"protocol": "TCP"
}
],
"resources": {},
"terminationMessagePath": "/dev/termination-log",
"terminationMessagePolicy": "File"
}
],
"dnsPolicy": "ClusterFirst",
"restartPolicy": "Always",
"schedulerName": "default-scheduler",
"securityContext": {},
"terminationGracePeriodSeconds": 30
}
}
},
"status": {
"availableReplicas": 1,
"conditions": [
{
"lastTransitionTime": "2020-06-25T19:25:07Z",
"lastUpdateTime": "2020-06-25T19:25:07Z",
"message": "Deployment has minimum availability.",
"reason": "MinimumReplicasAvailable",
"status": "True",
"type": "Available"
},
{
"lastTransitionTime": "2020-06-25T19:25:05Z",
"lastUpdateTime": "2020-06-30T19:39:35Z",
"message": "ReplicaSet \"guestbook-ui-6f7c7fc4b6\" has successfully progressed.",
"reason": "NewReplicaSetAvailable",
"status": "True",
"type": "Progressing"
}
],
"observedGeneration": 5,
"readyReplicas": 1,
"replicas": 1,
"updatedReplicas": 1
}
}
4 changes: 2 additions & 2 deletions pkg/utils/json/json.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ func removeFields(config, live interface{}) interface{} {
case map[string]interface{}:
return RemoveMapFields(c, live.(map[string]interface{}))
case []interface{}:
return removeListFields(c, live.([]interface{}))
return RemoveListFields(c, live.([]interface{}))
default:
return live
}
Expand All @@ -28,7 +28,7 @@ func RemoveMapFields(config, live map[string]interface{}) map[string]interface{}
return result
}

func removeListFields(config, live []interface{}) []interface{} {
func RemoveListFields(config, live []interface{}) []interface{} {
// If live is longer than config, then the extra elements at the end of the
// list will be returned as-is so they appear in the diff.
result := make([]interface{}, 0, len(live))
Expand Down

0 comments on commit c23d4d7

Please sign in to comment.