Merge pull request #44788 from enisoc/patch-numeric

Automatic merge from submit-queue

PATCH: Fix erroneous meaningful conflict for numeric values.

The wrong json package was used, resulting in patches being unmarshaled with numbers as float64 rather than int64. This in turn confused `HasConflicts()` which expects numeric types to match.

The end result was false positives of meaningful conflicts, such as:

```
there is a meaningful conflict (firstResourceVersion: "8517", currentResourceVersion: "8519"):
 diff1={"metadata":{"resourceVersion":"8519"},"spec":{"replicas":0},"status":"conditions":null,"fullyLabeledReplicas":null,"replicas":0}}
, diff2={"spec":{"replicas":0}}
```

This is branched from a discussion on https://github.com/kubernetes/kubernetes/pull/43469.

```release-note
Fix false positive "meaningful conflict" detection for strategic merge patch with integer values.
```

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go

@@ -18,7 +18,6 @@ package handlers
 
 import (
 	"encoding/hex"
-	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"math/rand"
@@ -38,6 +37,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/json"
 	"k8s.io/apimachinery/pkg/util/mergepatch"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/strategicpatch"

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go

@@ -193,11 +193,6 @@ func (tc *patchTestCase) Run(t *testing.T) {
 		}
 	}
 
-	testPatcher := &testPatcher{}
-	testPatcher.t = t
-	testPatcher.startingPod = tc.startingPod
-	testPatcher.updatePod = tc.updatePod
-
 	ctx := request.NewDefaultContext()
 	ctx = request.WithNamespace(ctx, namespace)
 
@@ -211,6 +206,13 @@ func (tc *patchTestCase) Run(t *testing.T) {
 	versionedObj := &v1.Pod{}
 
 	for _, patchType := range []types.PatchType{types.JSONPatchType, types.MergePatchType, types.StrategicMergePatchType} {
+		// This needs to be reset on each iteration.
+		testPatcher := &testPatcher{
+			t:           t,
+			startingPod: tc.startingPod,
+			updatePod:   tc.updatePod,
+		}
+
 		// TODO SUPPORT THIS!
 		if patchType == types.JSONPatchType {
 			continue
@@ -220,12 +222,12 @@ func (tc *patchTestCase) Run(t *testing.T) {
 		originalObjJS, err := runtime.Encode(codec, tc.startingPod)
 		if err != nil {
 			t.Errorf("%s: unexpected error: %v", tc.name, err)
-			return
+			continue
 		}
 		changedJS, err := runtime.Encode(codec, tc.changedPod)
 		if err != nil {
 			t.Errorf("%s: unexpected error: %v", tc.name, err)
-			return
+			continue
 		}
 
 		patch := []byte{}
@@ -237,14 +239,14 @@ func (tc *patchTestCase) Run(t *testing.T) {
 			patch, err = strategicpatch.CreateTwoWayMergePatch(originalObjJS, changedJS, versionedObj)
 			if err != nil {
 				t.Errorf("%s: unexpected error: %v", tc.name, err)
-				return
+				continue
 			}
 
 		case types.MergePatchType:
 			patch, err = jsonpatch.CreateMergePatch(originalObjJS, changedJS)
 			if err != nil {
 				t.Errorf("%s: unexpected error: %v", tc.name, err)
-				return
+				continue
 			}
 
 		}
@@ -253,12 +255,12 @@ func (tc *patchTestCase) Run(t *testing.T) {
 		if len(tc.expectedError) != 0 {
 			if err == nil || err.Error() != tc.expectedError {
 				t.Errorf("%s: expected error %v, but got %v", tc.name, tc.expectedError, err)
-				return
+				continue
 			}
 		} else {
 			if err != nil {
 				t.Errorf("%s: unexpected error: %v", tc.name, err)
-				return
+				continue
 			}
 		}
 
@@ -266,7 +268,7 @@ func (tc *patchTestCase) Run(t *testing.T) {
 			if resultObj != nil {
 				t.Errorf("%s: unexpected result: %v", tc.name, resultObj)
 			}
-			return
+			continue
 		}
 
 		resultPod := resultObj.(*api.Pod)
@@ -275,18 +277,18 @@ func (tc *patchTestCase) Run(t *testing.T) {
 		expectedJS, err := runtime.Encode(codec, tc.expectedPod)
 		if err != nil {
 			t.Errorf("%s: unexpected error: %v", tc.name, err)
-			return
+			continue
 		}
 		expectedObj, err := runtime.Decode(codec, expectedJS)
 		if err != nil {
 			t.Errorf("%s: unexpected error: %v", tc.name, err)
-			return
+			continue
 		}
 		reallyExpectedPod := expectedObj.(*api.Pod)
 
 		if !reflect.DeepEqual(*reallyExpectedPod, *resultPod) {
 			t.Errorf("%s mismatch: %v\n", tc.name, diff.ObjectGoPrintDiff(reallyExpectedPod, resultPod))
-			return
+			continue
 		}
 	}
 
@@ -324,6 +326,59 @@ func TestNumberConversion(t *testing.T) {
 	}
 }
 
+func TestPatchResourceNumberConversion(t *testing.T) {
+	namespace := "bar"
+	name := "foo"
+	uid := types.UID("uid")
+	fifteen := int64(15)
+	thirty := int64(30)
+
+	tc := &patchTestCase{
+		name: "TestPatchResourceNumberConversion",
+
+		startingPod: &api.Pod{},
+		changedPod:  &api.Pod{},
+		updatePod:   &api.Pod{},
+
+		expectedPod: &api.Pod{},
+	}
+
+	tc.startingPod.Name = name
+	tc.startingPod.Namespace = namespace
+	tc.startingPod.UID = uid
+	tc.startingPod.ResourceVersion = "1"
+	tc.startingPod.APIVersion = "v1"
+	tc.startingPod.Spec.ActiveDeadlineSeconds = &fifteen
+
+	// Patch tries to change to 30.
+	tc.changedPod.Name = name
+	tc.changedPod.Namespace = namespace
+	tc.changedPod.UID = uid
+	tc.changedPod.ResourceVersion = "1"
+	tc.changedPod.APIVersion = "v1"
+	tc.changedPod.Spec.ActiveDeadlineSeconds = &thirty
+
+	// Someone else already changed it to 30.
+	// This should be fine since it's not a "meaningful conflict".
+	// Previously this was detected as a meaningful conflict because int64(30) != float64(30).
+	tc.updatePod.Name = name
+	tc.updatePod.Namespace = namespace
+	tc.updatePod.UID = uid
+	tc.updatePod.ResourceVersion = "2"
+	tc.updatePod.APIVersion = "v1"
+	tc.updatePod.Spec.ActiveDeadlineSeconds = &thirty
+	tc.updatePod.Spec.NodeName = "anywhere"
+
+	tc.expectedPod.Name = name
+	tc.expectedPod.Namespace = namespace
+	tc.expectedPod.UID = uid
+	tc.expectedPod.ResourceVersion = "2"
+	tc.expectedPod.Spec.ActiveDeadlineSeconds = &thirty
+	tc.expectedPod.Spec.NodeName = "anywhere"
+
+	tc.Run(t)
+}
+
 func TestPatchResourceWithVersionConflict(t *testing.T) {
 	namespace := "bar"
 	name := "foo"