Merge pull request #41685 from liggitt/edit-refactor-unknown-field

Automatic merge from submit-queue (batch tested with PRs 41709, 41685, 41754, 41759, 37237)

Tolerate unknown fields in strategic merge patch

When using `apply` or `edit` with an object that has a compiled-in struct, if an unknown server-side field is sent, or is present in a provided file, the strategic merge patch computation fails looking up type info from the go struct

If the field only exists in one side of the patch (is being added or removed), or is identical in both sides of the patch, we should tolerate missing type info, since it doesn't affect the 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",
     ],
 )