Fix original object mutation on patch retry

Kubernetes-commit: c415e4aeabe5e5514dcdbf2c01c533533c25f4c1

pkg/endpoints/handlers/patch.go

@@ -86,21 +86,21 @@ func strategicPatchObject(
 	patchJS []byte,
 	objToUpdate runtime.Object,
 	versionedObj runtime.Object,
-) (originalObjMap map[string]interface{}, patchMap map[string]interface{}, retErr error) {
+) error {
-	originalObjMap = make(map[string]interface{})
+	originalObjMap := make(map[string]interface{})
 	if err := unstructured.DefaultConverter.ToUnstructured(originalObject, &originalObjMap); err != nil {
-		return nil, nil, err
+		return err
 	}
 
-	patchMap = make(map[string]interface{})
+	patchMap := make(map[string]interface{})
 	if err := json.Unmarshal(patchJS, &patchMap); err != nil {
-		return nil, nil, err
+		return err
 	}
 
 	if err := applyPatchToObject(codec, defaulter, originalObjMap, patchMap, objToUpdate, versionedObj); err != nil {
-		return nil, nil, err
+		return err
 	}
-	return
+	return nil
 }
 
 // applyPatchToObject applies a strategic merge patch of <patchMap> to

pkg/endpoints/handlers/rest.go

@@ -570,7 +570,7 @@ func patchResource(
 		originalObjJS           []byte
 		originalPatchedObjJS    []byte
 		originalObjMap          map[string]interface{}
-		originalPatchMap        map[string]interface{}
+		getOriginalPatchMap     func() (map[string]interface{}, error)
 		lastConflictErr         error
 		originalResourceVersion string
 	)
@@ -610,6 +610,26 @@ func patchResource(
 					return nil, err
 				}
 				originalObjJS, originalPatchedObjJS = originalJS, patchedJS
+
+				// Make a getter that can return a fresh strategic patch map if needed for conflict retries
+				// We have to rebuild it each time we need it, because the map gets mutated when being applied
+				var originalPatchBytes []byte
+				getOriginalPatchMap = func() (map[string]interface{}, error) {
+					if originalPatchBytes == nil {
+						// Compute once
+						originalPatchBytes, err = strategicpatch.CreateTwoWayMergePatch(originalObjJS, originalPatchedObjJS, versionedObj)
+						if err != nil {
+							return nil, err
+						}
+					}
+					// Return a fresh map every time
+					originalPatchMap := make(map[string]interface{})
+					if err := json.Unmarshal(originalPatchBytes, &originalPatchMap); err != nil {
+						return nil, err
+					}
+					return originalPatchMap, nil
+				}
+
 			case types.StrategicMergePatchType:
 				// Since the patch is applied on versioned objects, we need to convert the
 				// current object to versioned representation first.
@@ -621,8 +641,12 @@ func patchResource(
 				if err != nil {
 					return nil, err
 				}
-				originalMap, patchMap, err := strategicPatchObject(codec, defaulter, currentVersionedObject, patchJS, versionedObjToUpdate, versionedObj)
+				// Capture the original object map and patch for possible retries.
-				if err != nil {
+				originalMap := make(map[string]interface{})
+				if err := unstructured.DefaultConverter.ToUnstructured(currentVersionedObject, &originalMap); err != nil {
+					return nil, err
+				}
+				if err := strategicPatchObject(codec, defaulter, currentVersionedObject, patchJS, versionedObjToUpdate, versionedObj); err != nil {
 					return nil, err
 				}
 				// Convert the object back to unversioned.
@@ -632,8 +656,17 @@ func patchResource(
 					return nil, err
 				}
 				objToUpdate = unversionedObjToUpdate
-				// Store unstructured representations for possible retries.
+				// Store unstructured representation for possible retries.
-				originalObjMap, originalPatchMap = originalMap, patchMap
+				originalObjMap = originalMap
+				// Make a getter that can return a fresh strategic patch map if needed for conflict retries
+				// We have to rebuild it each time we need it, because the map gets mutated when being applied
+				getOriginalPatchMap = func() (map[string]interface{}, error) {
+					patchMap := make(map[string]interface{})
+					if err := json.Unmarshal(patchJS, &patchMap); err != nil {
+						return nil, err
+					}
+					return patchMap, nil
+				}
 			}
 			if err := checkName(objToUpdate, name, namespace, namer); err != nil {
 				return nil, err
@@ -669,17 +702,6 @@ func patchResource(
 					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 {
@@ -695,6 +717,12 @@ func patchResource(
 				}
 			}
 
+			// Get a fresh copy of the original strategic patch each time through, since applying it mutates the map
+			originalPatchMap, err := getOriginalPatchMap()
+			if err != nil {
+				return nil, err
+			}
+
 			hasConflicts, err := mergepatch.HasConflicts(originalPatchMap, currentPatchMap)
 			if err != nil {
 				return nil, err

pkg/endpoints/handlers/rest_test.go

@@ -74,7 +74,7 @@ func TestPatchAnonymousField(t *testing.T) {
 	}
 
 	actual := &testPatchType{}
-	_, _, err := strategicPatchObject(codec, defaulter, original, []byte(patch), actual, &testPatchType{})
+	err := strategicPatchObject(codec, defaulter, original, []byte(patch), actual, &testPatchType{})
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -314,7 +314,7 @@ func TestNumberConversion(t *testing.T) {
 
 	patchJS := []byte(`{"spec":{"ports":[{"port":80,"nodePort":31789}]}}`)
 
-	_, _, err := strategicPatchObject(codec, defaulter, currentVersionedObject, patchJS, versionedObjToUpdate, versionedObj)
+	err := strategicPatchObject(codec, defaulter, currentVersionedObject, patchJS, versionedObjToUpdate, versionedObj)
 	if err != nil {
 		t.Fatal(err)
 	}