Merge pull request #39324 from wojtek-t/change_patch_api

Automatic merge from submit-queue

Prepare for using optimized conversion to/from map[string]interface{} in Patch operation

Ref #39017

pkg/client/record/events_cache.go

@@ -244,7 +244,7 @@ func (e *eventLogger) eventObserve(newEvent *v1.Event) (*v1.Event, []byte, error
 
 		newData, _ := json.Marshal(event)
 		oldData, _ := json.Marshal(eventCopy2)
-		patch, err = strategicpatch.CreateStrategicMergePatch(oldData, newData, event)
+		patch, err = strategicpatch.CreateTwoWayMergePatch(oldData, newData, event)
 	}
 
 	// record our new observation

pkg/controller/volume/attachdetach/statusupdater/node_status_updater.go

@@ -111,10 +111,10 @@ func (nsu *nodeStatusUpdater) UpdateNodeStatuses() error {
 		}
 
 		patchBytes, err :=
-			strategicpatch.CreateStrategicMergePatch(oldData, newData, node)
+			strategicpatch.CreateTwoWayMergePatch(oldData, newData, node)
 		if err != nil {
 			return fmt.Errorf(
-				"failed to CreateStrategicMergePatch for node %q. %v",
+				"failed to CreateTwoWayMergePatch for node %q. %v",
 				nodeName,
 				err)
 		}

pkg/genericapiserver/api/handlers/BUILD

@@ -36,6 +36,7 @@ go_library(
     srcs = [
         "discovery.go",
         "doc.go",
+        "patch.go",
         "proxy.go",
         "rest.go",
         "watch.go",

pkg/genericapiserver/api/handlers/patch.go

@@ -0,0 +1,131 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package handlers
+
+import (
+	"encoding/json"
+	"fmt"
+
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/kubernetes/pkg/util/strategicpatch"
+
+	"github.com/evanphx/json-patch"
+)
+
+// patchObjectJSON patches the <originalObject> with <patchJS> and stores
+// the result in <objToUpdate>.
+// Currently it also returns the original and patched objects serialized to
+// JSONs (this may not be needed once we can apply patches at the
+// map[string]interface{} level).
+func patchObjectJSON(
+	patchType types.PatchType,
+	codec runtime.Codec,
+	originalObject runtime.Object,
+	patchJS []byte,
+	objToUpdate runtime.Object,
+	versionedObj runtime.Object,
+) (originalObjJS []byte, patchedObjJS []byte, retErr error) {
+	js, err := runtime.Encode(codec, originalObject)
+	if err != nil {
+		return nil, nil, err
+	}
+	originalObjJS = js
+
+	switch patchType {
+	case types.JSONPatchType:
+		patchObj, err := jsonpatch.DecodePatch(patchJS)
+		if err != nil {
+			return nil, nil, err
+		}
+		if patchedObjJS, err = patchObj.Apply(originalObjJS); err != nil {
+			return nil, nil, err
+		}
+	case types.MergePatchType:
+		if patchedObjJS, err = jsonpatch.MergePatch(originalObjJS, patchJS); err != nil {
+			return nil, nil, err
+		}
+	case types.StrategicMergePatchType:
+		if patchedObjJS, err = strategicpatch.StrategicMergePatch(originalObjJS, patchJS, versionedObj); err != nil {
+			return nil, nil, err
+		}
+	default:
+		// only here as a safety net - go-restful filters content-type
+		return nil, nil, fmt.Errorf("unknown Content-Type header for patch: %v", patchType)
+	}
+	if err := runtime.DecodeInto(codec, patchedObjJS, objToUpdate); err != nil {
+		return nil, nil, err
+	}
+	return
+}
+
+// strategicPatchObject applies a strategic merge patch of <patchJS> to
+// <originalObject> and stores the result in <objToUpdate>.
+// It additionally returns the map[string]interface{} representation of the
+// <originalObject> and <patchJS>.
+func strategicPatchObject(
+	codec runtime.Codec,
+	originalObject runtime.Object,
+	patchJS []byte,
+	objToUpdate runtime.Object,
+	versionedObj runtime.Object,
+) (originalObjMap map[string]interface{}, patchMap map[string]interface{}, retErr error) {
+	// TODO: This should be one-step conversion that doesn't require
+	// json marshaling and unmarshaling once #39017 is fixed.
+	data, err := runtime.Encode(codec, originalObject)
+	if err != nil {
+		return nil, nil, err
+	}
+	originalObjMap = make(map[string]interface{})
+	if err := json.Unmarshal(data, &originalObjMap); err != nil {
+		return nil, nil, err
+	}
+
+	patchMap = make(map[string]interface{})
+	if err := json.Unmarshal(patchJS, &patchMap); err != nil {
+		return nil, nil, err
+	}
+
+	if err := applyPatchToObject(codec, originalObjMap, patchMap, objToUpdate, versionedObj); err != nil {
+		return nil, nil, err
+	}
+	return
+}
+
+// applyPatchToObject applies a strategic merge patch of <patchMap> to
+// <originalMap> and stores the result in <objToUpdate>, though it operates
+// on versioned map[string]interface{} representations.
+func applyPatchToObject(
+	codec runtime.Codec,
+	originalMap map[string]interface{},
+	patchMap map[string]interface{},
+	objToUpdate runtime.Object,
+	versionedObj runtime.Object,
+) error {
+	patchedObjMap, err := strategicpatch.StrategicMergeMapPatch(originalMap, patchMap, versionedObj)
+	if err != nil {
+		return err
+	}
+
+	// TODO: This should be one-step conversion that doesn't require
+	// json marshaling and unmarshaling once #39017 is fixed.
+	data, err := json.Marshal(patchedObjMap)
+	if err != nil {
+		return err
+	}
+	return runtime.DecodeInto(codec, data, objToUpdate)
+}

pkg/genericapiserver/api/handlers/rest.go

@@ -45,7 +45,6 @@ import (
 	"k8s.io/kubernetes/pkg/util/strategicpatch"
 
 	"github.com/emicklei/go-restful"
-	"github.com/evanphx/json-patch"
 	"github.com/golang/glog"
 )
 
@@ -556,6 +555,8 @@ func patchResource(
 	var (
 		originalObjJS        []byte
 		originalPatchedObjJS []byte
+		originalObjMap       map[string]interface{}
+		originalPatchMap     map[string]interface{}
 		lastConflictErr      error
 	)
 
@@ -570,25 +571,30 @@ func patchResource(
 		}
 
 		switch {
-		case len(originalObjJS) == 0 || len(originalPatchedObjJS) == 0:
+		case originalObjJS == nil && originalObjMap == nil:
 			// first time through,
 			// 1. apply the patch
-			// 2. save the originalJS and patchedJS to detect whether there were conflicting changes on retries
-			if js, err := runtime.Encode(codec, currentObject); err != nil {
-				return nil, err
-			} else {
-				originalObjJS = js
-			}
-
-			if js, err := getPatchedJS(patchType, originalObjJS, patchJS, versionedObj); err != nil {
-				return nil, err
-			} else {
-				originalPatchedObjJS = js
-			}
+			// 2. save the original and patched to detect whether there were conflicting changes on retries
 
 			objToUpdate := patcher.New()
-			if err := runtime.DecodeInto(codec, originalPatchedObjJS, objToUpdate); err != nil {
-				return nil, err
+
+			// For performance reasons, in case of strategicpatch, we avoid json
+			// marshaling and unmarshaling and operate just on map[string]interface{}.
+			// In case of other patch types, we still have to operate on JSON
+			// representations.
+			switch patchType {
+			case types.JSONPatchType, types.MergePatchType:
+				originalJS, patchedJS, err := patchObjectJSON(patchType, codec, currentObject, patchJS, objToUpdate, versionedObj)
+				if err != nil {
+					return nil, err
+				}
+				originalObjJS, originalPatchedObjJS = originalJS, patchedJS
+			case types.StrategicMergePatchType:
+				originalMap, patchMap, err := strategicPatchObject(codec, currentObject, patchJS, objToUpdate, versionedObj)
+				if err != nil {
+					return nil, err
+				}
+				originalObjMap, originalPatchMap = originalMap, patchMap
 			}
 			if err := checkName(objToUpdate, name, namespace, namer); err != nil {
 				return nil, err
@@ -603,33 +609,62 @@ func patchResource(
 			// 2. build a strategic merge patch from originalJS and the currentJS
 			// 3. ensure no conflicts between the two patches
 			// 4. apply the #1 patch to the currentJS object
-			currentObjectJS, err := runtime.Encode(codec, currentObject)
+
+			// TODO: This should be one-step conversion that doesn't require
+			// json marshaling and unmarshaling once #39017 is fixed.
+			data, err := runtime.Encode(codec, currentObject)
 			if err != nil {
 				return nil, err
 			}
-			currentPatch, err := strategicpatch.CreateStrategicMergePatch(originalObjJS, currentObjectJS, versionedObj)
-			if err != nil {
-				return nil, err
-			}
-			originalPatch, err := strategicpatch.CreateStrategicMergePatch(originalObjJS, originalPatchedObjJS, versionedObj)
-			if err != nil {
+			currentObjMap := make(map[string]interface{})
+			if err := json.Unmarshal(data, &currentObjMap); err != nil {
 				return nil, err
 			}
 
-			diff1 := make(map[string]interface{})
-			if err := json.Unmarshal(originalPatch, &diff1); err != nil {
-				return nil, err
+			var currentPatchMap map[string]interface{}
+			if originalObjMap != nil {
+				var err error
+				currentPatchMap, err = strategicpatch.CreateTwoWayMergeMapPatch(originalObjMap, currentObjMap, versionedObj)
+				if err != nil {
+					return nil, err
+				}
+			} else {
+				if originalPatchMap == nil {
+					// Compute original patch, if we already didn't do this in previous retries.
+					originalPatch, err := strategicpatch.CreateTwoWayMergePatch(originalObjJS, originalPatchedObjJS, versionedObj)
+					if err != nil {
+						return nil, err
+					}
+					originalPatchMap = make(map[string]interface{})
+					if err := json.Unmarshal(originalPatch, &originalPatchMap); err != nil {
+						return nil, err
+					}
+				}
+				// Compute current patch.
+				currentObjJS, err := runtime.Encode(codec, currentObject)
+				if err != nil {
+					return nil, err
+				}
+				currentPatch, err := strategicpatch.CreateTwoWayMergePatch(originalObjJS, currentObjJS, versionedObj)
+				if err != nil {
+					return nil, err
+				}
+				currentPatchMap = make(map[string]interface{})
+				if err := json.Unmarshal(currentPatch, &currentPatchMap); err != nil {
+					return nil, err
+				}
 			}
-			diff2 := make(map[string]interface{})
-			if err := json.Unmarshal(currentPatch, &diff2); err != nil {
-				return nil, err
-			}
-			hasConflicts, err := strategicpatch.HasConflicts(diff1, diff2)
+
+			hasConflicts, err := strategicpatch.HasConflicts(originalPatchMap, currentPatchMap)
 			if err != nil {
 				return nil, err
 			}
 			if hasConflicts {
-				glog.V(4).Infof("patchResource failed for resource %s, because there is a meaningful conflict.\n diff1=%v\n, diff2=%v\n", name, diff1, diff2)
+				if glog.V(4) {
+					diff1, _ := json.Marshal(currentPatchMap)
+					diff2, _ := json.Marshal(originalPatchMap)
+					glog.Infof("patchResource failed for resource %s, because there is a meaningful conflict.\n diff1=%v\n, diff2=%v\n", name, diff1, diff2)
+				}
 				// Return the last conflict error we got if we have one
 				if lastConflictErr != nil {
 					return nil, lastConflictErr
@@ -638,14 +673,11 @@ func patchResource(
 				return nil, errors.NewConflict(resource.GroupResource(), name, nil)
 			}
 
-			newlyPatchedObjJS, err := getPatchedJS(types.StrategicMergePatchType, currentObjectJS, originalPatch, versionedObj)
-			if err != nil {
-				return nil, err
-			}
 			objToUpdate := patcher.New()
-			if err := runtime.DecodeInto(codec, newlyPatchedObjJS, objToUpdate); err != nil {
+			if err := applyPatchToObject(codec, currentObjMap, originalPatchMap, objToUpdate, versionedObj); err != nil {
 				return nil, err
 			}
+
 			return objToUpdate, nil
 		}
 	}
@@ -1079,24 +1111,6 @@ func setListSelfLink(obj runtime.Object, req *restful.Request, namer ScopeNamer)
 	return count, err
 }
 
-func getPatchedJS(patchType types.PatchType, originalJS, patchJS []byte, obj runtime.Object) ([]byte, error) {
-	switch patchType {
-	case types.JSONPatchType:
-		patchObj, err := jsonpatch.DecodePatch(patchJS)
-		if err != nil {
-			return nil, err
-		}
-		return patchObj.Apply(originalJS)
-	case types.MergePatchType:
-		return jsonpatch.MergePatch(originalJS, patchJS)
-	case types.StrategicMergePatchType:
-		return strategicpatch.StrategicMergePatchData(originalJS, patchJS, obj)
-	default:
-		// only here as a safety net - go-restful filters content-type
-		return nil, fmt.Errorf("unknown Content-Type header for patch: %v", patchType)
-	}
-}
-
 func summarizeData(data []byte, maxLength int) string {
 	switch {
 	case len(data) == 0:

pkg/genericapiserver/api/handlers/rest_test.go

@@ -55,16 +55,27 @@ type TestPatchSubType struct {
 func (obj *testPatchType) GetObjectKind() schema.ObjectKind { return &obj.TypeMeta }
 
 func TestPatchAnonymousField(t *testing.T) {
-	originalJS := `{"kind":"testPatchType","theField":"my-value"}`
-	patch := `{"theField": "changed!"}`
-	expectedJS := `{"kind":"testPatchType","theField":"changed!"}`
+	testGV := schema.GroupVersion{Group: "", Version: "v"}
+	api.Scheme.AddKnownTypes(testGV, &testPatchType{})
+	codec := api.Codecs.LegacyCodec(testGV)
 
-	actualBytes, err := getPatchedJS(types.StrategicMergePatchType, []byte(originalJS), []byte(patch), &testPatchType{})
+	original := &testPatchType{
+		TypeMeta:         metav1.TypeMeta{Kind: "testPatchType", APIVersion: "v"},
+		TestPatchSubType: TestPatchSubType{StringField: "my-value"},
+	}
+	patch := `{"theField": "changed!"}`
+	expected := &testPatchType{
+		TypeMeta:         metav1.TypeMeta{Kind: "testPatchType", APIVersion: "v"},
+		TestPatchSubType: TestPatchSubType{StringField: "changed!"},
+	}
+
+	actual := &testPatchType{}
+	_, _, err := strategicPatchObject(codec, original, []byte(patch), actual, &testPatchType{})
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	if string(actualBytes) != expectedJS {
-		t.Errorf("expected %v, got %v", expectedJS, string(actualBytes))
+	if !api.Semantic.DeepEqual(actual, expected) {
+		t.Errorf("expected %#v, got %#v", expected, actual)
 	}
 }
 
@@ -215,7 +226,7 @@ func (tc *patchTestCase) Run(t *testing.T) {
 			continue
 
 		case types.StrategicMergePatchType:
-			patch, err = strategicpatch.CreateStrategicMergePatch(originalObjJS, changedJS, versionedObj)
+			patch, err = strategicpatch.CreateTwoWayMergePatch(originalObjJS, changedJS, versionedObj)
 			if err != nil {
 				t.Errorf("%s: unexpected error: %v", tc.name, err)
 				return

pkg/kubectl/cmd/patch.go

@@ -261,7 +261,7 @@ func getPatchedJSON(patchType types.PatchType, originalJS, patchJS []byte, obj r
 		return jsonpatch.MergePatch(originalJS, patchJS)
 
 	case types.StrategicMergePatchType:
-		return strategicpatch.StrategicMergePatchData(originalJS, patchJS, obj)
+		return strategicpatch.StrategicMergePatch(originalJS, patchJS, obj)
 
 	default:
 		// only here as a safety net - go-restful filters content-type

pkg/util/strategicpatch/patch.go

@@ -48,6 +48,13 @@ const (
 	deleteFromPrimitiveListDirectivePrefix = "$deleteFromPrimitiveList"
 )
 
+// JSONMap is a representations of JSON object encoded as map[string]interface{}
+// where the children can be either map[string]interface{}, []interface{} or
+// primitive type).
+// Operating on JSONMap representation is much faster as it doesn't require any
+// json marshaling and/or unmarshaling operations.
+type JSONMap map[string]interface{}
+
 // IsPreconditionFailed returns true if the provided error indicates
 // a precondition failed.
 func IsPreconditionFailed(err error) bool {
@@ -136,11 +143,6 @@ func RequireMetadataKeyUnchanged(key string) PreconditionFunc {
 	}
 }
 
-// Deprecated: Use the synonym CreateTwoWayMergePatch, instead.
-func CreateStrategicMergePatch(original, modified []byte, dataStruct interface{}) ([]byte, error) {
-	return CreateTwoWayMergePatch(original, modified, dataStruct)
-}
-
 // CreateTwoWayMergePatch creates a patch that can be passed to StrategicMergePatch from an original
 // document and a modified document, which are passed to the method as json encoded content. It will
 // return a patch that yields the modified document when applied to the original document, or an error
@@ -160,12 +162,24 @@ func CreateTwoWayMergePatch(original, modified []byte, dataStruct interface{}, f
 		}
 	}
 
+	patchMap, err := CreateTwoWayMergeMapPatch(originalMap, modifiedMap, dataStruct, fns...)
+	if err != nil {
+		return nil, err
+	}
+
+	return json.Marshal(patchMap)
+}
+
+// CreateTwoWayMergeMapPatch creates a patch from an original and modified JSON objects,
+// encoded JSONMap.
+// The serialized version of the map can then be passed to StrategicMergeMapPatch.
+func CreateTwoWayMergeMapPatch(original, modified JSONMap, dataStruct interface{}, fns ...PreconditionFunc) (JSONMap, error) {
 	t, err := getTagStructType(dataStruct)
 	if err != nil {
 		return nil, err
 	}
 
-	patchMap, err := diffMaps(originalMap, modifiedMap, t, false, false)
+	patchMap, err := diffMaps(original, modified, t, false, false)
 	if err != nil {
 		return nil, err
 	}
@@ -177,7 +191,7 @@ func CreateTwoWayMergePatch(original, modified []byte, dataStruct interface{}, f
 		}
 	}
 
-	return json.Marshal(patchMap)
+	return patchMap, nil
 }
 
 // Returns a (recursive) strategic merge patch that yields modified when applied to original.
@@ -494,12 +508,6 @@ loopB:
 	return patch, nil
 }
 
-// Deprecated: StrategicMergePatchData is deprecated. Use the synonym StrategicMergePatch,
-// instead, which follows the naming convention of evanphx/json-patch.
-func StrategicMergePatchData(original, patch []byte, dataStruct interface{}) ([]byte, error) {
-	return StrategicMergePatch(original, patch, dataStruct)
-}
-
 // StrategicMergePatch applies a strategic merge patch. The patch and the original document
 // must be json encoded content. A patch can be created from an original and a modified document
 // by calling CreateStrategicMergePatch.
@@ -524,12 +532,7 @@ func StrategicMergePatch(original, patch []byte, dataStruct interface{}) ([]byte
 		return nil, errBadJSONDoc
 	}
 
-	t, err := getTagStructType(dataStruct)
-	if err != nil {
-		return nil, err
-	}
-
-	result, err := mergeMap(originalMap, patchMap, t, true)
+	result, err := StrategicMergeMapPatch(originalMap, patchMap, dataStruct)
 	if err != nil {
 		return nil, err
 	}
@@ -537,6 +540,17 @@ func StrategicMergePatch(original, patch []byte, dataStruct interface{}) ([]byte
 	return json.Marshal(result)
 }
 
+// StrategicMergePatch applies a strategic merge patch. The original and patch documents
+// must be JSONMap. A patch can be created from an original and modified document by
+// calling CreateTwoWayMergeMapPatch.
+func StrategicMergeMapPatch(original, patch JSONMap, dataStruct interface{}) (JSONMap, error) {
+	t, err := getTagStructType(dataStruct)
+	if err != nil {
+		return nil, err
+	}
+	return mergeMap(original, patch, t, true)
+}
+
 func getTagStructType(dataStruct interface{}) (reflect.Type, error) {
 	if dataStruct == nil {
 		return nil, fmt.Errorf(errBadArgTypeFmt, "struct", "nil")