Tolerate unknown fields in strategic merge patch

pkg/kubectl/cmd/testdata/edit/testcase-unknown-field-known-group-kind/0.response

@@ -0,0 +1,25 @@
+{
+	"kind": "StorageClass",
+	"apiVersion": "storage.k8s.io/v1beta1",
+	"metadata": {
+		"name": "foo",
+		"selfLink": "/apis/storage.k8s.io/v1beta1/storageclassesfoo",
+		"uid": "b2287558-f190-11e6-b041-acbc32c1ca87",
+		"resourceVersion": "21388",
+		"creationTimestamp": "2017-02-13T02:04:04Z",
+		"labels": {
+			"label1": "value1"
+		}
+	},
+	"provisioner": "foo",
+	"parameters": {
+		"baz": "qux",
+		"foo": "bar"
+	},
+	"unknownServerField1": {
+		"data": true
+	},
+	"unknownServerField2": {
+		"data": true
+	}
+}

pkg/kubectl/cmd/testdata/edit/testcase-unknown-field-known-group-kind/1.edited

@@ -0,0 +1,23 @@
+# Please edit the object below. Lines beginning with a '#' will be ignored,
+# and an empty file will abort the edit. If an error occurs while saving this file will be
+# reopened with the relevant failures.
+#
+apiVersion: storage.k8s.io/v1beta1
+kind: StorageClass
+metadata:
+  creationTimestamp: 2017-02-13T02:04:04Z
+  labels:
+    label1: value1
+    label2: value2
+  name: foo
+  resourceVersion: "21388"
+  selfLink: /apis/storage.k8s.io/v1beta1/storageclassesfoo
+  uid: b2287558-f190-11e6-b041-acbc32c1ca87
+parameters:
+  baz: qux
+  foo: bar
+provisioner: foo
+unknownClientField:
+  clientdata: true
+unknownServerField1:
+  data: true

pkg/kubectl/cmd/testdata/edit/testcase-unknown-field-known-group-kind/1.original

@@ -0,0 +1,22 @@
+# Please edit the object below. Lines beginning with a '#' will be ignored,
+# and an empty file will abort the edit. If an error occurs while saving this file will be
+# reopened with the relevant failures.
+#
+apiVersion: storage.k8s.io/v1beta1
+kind: StorageClass
+metadata:
+  creationTimestamp: 2017-02-13T02:04:04Z
+  labels:
+    label1: value1
+  name: foo
+  resourceVersion: "21388"
+  selfLink: /apis/storage.k8s.io/v1beta1/storageclassesfoo
+  uid: b2287558-f190-11e6-b041-acbc32c1ca87
+parameters:
+  baz: qux
+  foo: bar
+provisioner: foo
+unknownServerField1:
+  data: true
+unknownServerField2:
+  data: true

pkg/kubectl/cmd/testdata/edit/testcase-unknown-field-known-group-kind/2.request

@@ -0,0 +1,12 @@
+{
+	"metadata": {
+		"labels": {
+			"label2": "value2"
+		},
+		"namespace": ""
+	},
+	"unknownClientField": {
+		"clientdata": true
+	},
+	"unknownServerField2": null
+}

pkg/kubectl/cmd/testdata/edit/testcase-unknown-field-known-group-kind/2.response

@@ -0,0 +1,26 @@
+{
+	"kind": "StorageClass",
+	"apiVersion": "storage.k8s.io/v1beta1",
+	"metadata": {
+		"name": "foo",
+		"selfLink": "/apis/storage.k8s.io/v1beta1/storageclassesfoo",
+		"uid": "b2287558-f190-11e6-b041-acbc32c1ca87",
+		"resourceVersion": "21431",
+		"creationTimestamp": "2017-02-13T02:04:04Z",
+		"labels": {
+			"label1": "value1",
+			"label2": "value2"
+		}
+	},
+	"provisioner": "foo",
+	"parameters": {
+		"baz": "qux",
+		"foo": "bar"
+	},
+	"unknownClientField": {
+		"clientdata": true
+	},
+	"unknownServerField1": {
+		"data": true
+	}
+}

pkg/kubectl/cmd/testdata/edit/testcase-unknown-field-known-group-kind/test.yaml

@@ -0,0 +1,25 @@
+description: edit an unknown version of a known group/kind
+mode: edit
+args:
+- storageclasses.v1beta1.storage.k8s.io/foo
+namespace: default
+expectedStdout:
+- "storageclass \"foo\" edited"
+expectedExitCode: 0
+steps:
+- type: request
+  expectedMethod: GET
+  expectedPath: /apis/storage.k8s.io/v1beta1/storageclasses/foo
+  expectedInput: 0.request
+  resultingStatusCode: 200
+  resultingOutput: 0.response
+- type: edit
+  expectedInput: 1.original
+  resultingOutput: 1.edited
+- type: request
+  expectedMethod: PATCH
+  expectedPath: /apis/storage.k8s.io/v1beta1/storageclasses/foo
+  expectedContentType: application/strategic-merge-patch+json
+  expectedInput: 2.request
+  resultingStatusCode: 200
+  resultingOutput: 2.response

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch.go

@@ -160,6 +160,12 @@ func diffMaps(original, modified map[string]interface{}, t reflect.Type, ignoreC
 			modifiedValueTyped := modifiedValue.(map[string]interface{})
 			fieldType, fieldPatchStrategy, _, err := forkedjson.LookupPatchMetadata(t, key)
 			if err != nil {
+				// We couldn't look up metadata for the field
+				// If the values are identical, this doesn't matter, no patch is needed
+				if reflect.DeepEqual(originalValue, modifiedValue) {
+					continue
+				}
+				// Otherwise, return the error
 				return nil, err
 			}
 
@@ -184,6 +190,12 @@ func diffMaps(original, modified map[string]interface{}, t reflect.Type, ignoreC
 			modifiedValueTyped := modifiedValue.([]interface{})
 			fieldType, fieldPatchStrategy, fieldPatchMergeKey, err := forkedjson.LookupPatchMetadata(t, key)
 			if err != nil {
+				// We couldn't look up metadata for the field
+				// If the values are identical, this doesn't matter, no patch is needed
+				if reflect.DeepEqual(originalValue, modifiedValue) {
+					continue
+				}
+				// Otherwise, return the error
 				return nil, err
 			}
 

staging/src/k8s.io/apimachinery/pkg/util/strategicpatch/patch_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/ghodss/yaml"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/mergepatch"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 type SortMergeListTestCases struct {
@@ -2625,3 +2626,117 @@ func testPatchApplicationWithoutSorting(t *testing.T, original, patch, expected
 		return
 	}
 }
+
+func TestUnknownField(t *testing.T) {
+	testcases := map[string]struct {
+		Original string
+		Current  string
+		Modified string
+
+		ExpectedTwoWay         string
+		ExpectedTwoWayErr      string
+		ExpectedTwoWayResult   string
+		ExpectedThreeWay       string
+		ExpectedThreeWayErr    string
+		ExpectedThreeWayResult string
+	}{
+		// cases we can successfully strategically merge
+		"no diff": {
+			Original: `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`,
+			Current:  `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`,
+			Modified: `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`,
+
+			ExpectedTwoWay:         `{}`,
+			ExpectedTwoWayResult:   `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`,
+			ExpectedThreeWay:       `{}`,
+			ExpectedThreeWayResult: `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`,
+		},
+		"added only": {
+			Original: `{"name":"foo"}`,
+			Current:  `{"name":"foo"}`,
+			Modified: `{"name":"foo","scalar":true,"complex":{"nested":true},"array":[1,2,3]}`,
+
+			ExpectedTwoWay:         `{"array":[1,2,3],"complex":{"nested":true},"scalar":true}`,
+			ExpectedTwoWayResult:   `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`,
+			ExpectedThreeWay:       `{"array":[1,2,3],"complex":{"nested":true},"scalar":true}`,
+			ExpectedThreeWayResult: `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`,
+		},
+		"removed only": {
+			Original: `{"name":"foo","scalar":true,"complex":{"nested":true}}`,
+			Current:  `{"name":"foo","scalar":true,"complex":{"nested":true},"array":[1,2,3]}`,
+			Modified: `{"name":"foo"}`,
+
+			ExpectedTwoWay:         `{"complex":null,"scalar":null}`,
+			ExpectedTwoWayResult:   `{"name":"foo"}`,
+			ExpectedThreeWay:       `{"complex":null,"scalar":null}`,
+			ExpectedThreeWayResult: `{"array":[1,2,3],"name":"foo"}`,
+		},
+
+		// cases we cannot successfully strategically merge (expect errors)
+		"diff": {
+			Original: `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`,
+			Current:  `{"array":[1,2,3],"complex":{"nested":true},"name":"foo","scalar":true}`,
+			Modified: `{"array":[1,2,3],"complex":{"nested":false},"name":"foo","scalar":true}`,
+
+			ExpectedTwoWayErr:   `unable to find api field`,
+			ExpectedThreeWayErr: `unable to find api field`,
+		},
+	}
+
+	for _, k := range sets.StringKeySet(testcases).List() {
+		tc := testcases[k]
+		func() {
+			twoWay, err := CreateTwoWayMergePatch([]byte(tc.Original), []byte(tc.Modified), &MergeItem{})
+			if err != nil {
+				if len(tc.ExpectedTwoWayErr) == 0 {
+					t.Errorf("%s: error making two-way patch: %v", k, err)
+				}
+				if !strings.Contains(err.Error(), tc.ExpectedTwoWayErr) {
+					t.Errorf("%s: expected error making two-way patch to contain '%s', got %s", k, tc.ExpectedTwoWayErr, err)
+				}
+				return
+			}
+
+			if string(twoWay) != tc.ExpectedTwoWay {
+				t.Errorf("%s: expected two-way patch:\n\t%s\ngot\n\t%s", k, string(tc.ExpectedTwoWay), string(twoWay))
+				return
+			}
+
+			twoWayResult, err := StrategicMergePatch([]byte(tc.Original), twoWay, MergeItem{})
+			if err != nil {
+				t.Errorf("%s: error applying two-way patch: %v", k, err)
+				return
+			}
+			if string(twoWayResult) != tc.ExpectedTwoWayResult {
+				t.Errorf("%s: expected two-way result:\n\t%s\ngot\n\t%s", k, string(tc.ExpectedTwoWayResult), string(twoWayResult))
+				return
+			}
+		}()
+
+		func() {
+			threeWay, err := CreateThreeWayMergePatch([]byte(tc.Original), []byte(tc.Modified), []byte(tc.Current), &MergeItem{}, false)
+			if err != nil {
+				if len(tc.ExpectedThreeWayErr) == 0 {
+					t.Errorf("%s: error making three-way patch: %v", k, err)
+				} else if !strings.Contains(err.Error(), tc.ExpectedThreeWayErr) {
+					t.Errorf("%s: expected error making three-way patch to contain '%s', got %s", k, tc.ExpectedThreeWayErr, err)
+				}
+				return
+			}
+
+			if string(threeWay) != tc.ExpectedThreeWay {
+				t.Errorf("%s: expected three-way patch:\n\t%s\ngot\n\t%s", k, string(tc.ExpectedThreeWay), string(threeWay))
+				return
+			}
+
+			threeWayResult, err := StrategicMergePatch([]byte(tc.Current), threeWay, MergeItem{})
+			if err != nil {
+				t.Errorf("%s: error applying three-way patch: %v", k, err)
+				return
+			} else if string(threeWayResult) != tc.ExpectedThreeWayResult {
+				t.Errorf("%s: expected three-way result:\n\t%s\ngot\n\t%s", k, string(tc.ExpectedThreeWayResult), string(threeWayResult))
+				return
+			}
+		}()
+	}
+}

vendor/BUILD

@@ -9143,6 +9143,7 @@ go_test(
         "//vendor:github.com/ghodss/yaml",
         "//vendor:k8s.io/apimachinery/pkg/runtime",
         "//vendor:k8s.io/apimachinery/pkg/util/mergepatch",
+        "//vendor:k8s.io/apimachinery/pkg/util/sets",
     ],
 )