diff --git a/pkg/endpoints/handlers/patch.go b/pkg/endpoints/handlers/patch.go index c8b889070..48c20e5e4 100644 --- a/pkg/endpoints/handlers/patch.go +++ b/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) { - originalObjMap = make(map[string]interface{}) +) error { + 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 to diff --git a/pkg/endpoints/handlers/rest.go b/pkg/endpoints/handlers/rest.go index 276183d34..0d5093742 100644 --- a/pkg/endpoints/handlers/rest.go +++ b/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) - if err != nil { + // Capture the original object map and patch for possible retries. + 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. - originalObjMap, originalPatchMap = originalMap, patchMap + // Store unstructured representation for possible retries. + 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 diff --git a/pkg/endpoints/handlers/rest_test.go b/pkg/endpoints/handlers/rest_test.go index bc0d2c3d9..06c295719 100644 --- a/pkg/endpoints/handlers/rest_test.go +++ b/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) }