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}}
```
pull/6/head
Anthony Yeh 2017-04-21 17:24:07 -07:00
parent 808982c702
commit 1ab6a33db4
No known key found for this signature in database
GPG Key ID: 339F46A383E6ED08
2 changed files with 71 additions and 16 deletions

View File

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

View File

@ -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.NewDefaultContext()
ctx = request.WithNamespace(ctx, namespace) ctx = request.WithNamespace(ctx, namespace)
@ -211,6 +206,13 @@ func (tc *patchTestCase) Run(t *testing.T) {
versionedObj := &v1.Pod{} versionedObj := &v1.Pod{}
for _, patchType := range []types.PatchType{types.JSONPatchType, types.MergePatchType, types.StrategicMergePatchType} { 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! // TODO SUPPORT THIS!
if patchType == types.JSONPatchType { if patchType == types.JSONPatchType {
continue continue
@ -220,12 +222,12 @@ func (tc *patchTestCase) Run(t *testing.T) {
originalObjJS, err := runtime.Encode(codec, tc.startingPod) originalObjJS, err := runtime.Encode(codec, tc.startingPod)
if err != nil { if err != nil {
t.Errorf("%s: unexpected error: %v", tc.name, err) t.Errorf("%s: unexpected error: %v", tc.name, err)
return continue
} }
changedJS, err := runtime.Encode(codec, tc.changedPod) changedJS, err := runtime.Encode(codec, tc.changedPod)
if err != nil { if err != nil {
t.Errorf("%s: unexpected error: %v", tc.name, err) t.Errorf("%s: unexpected error: %v", tc.name, err)
return continue
} }
patch := []byte{} patch := []byte{}
@ -237,14 +239,14 @@ func (tc *patchTestCase) Run(t *testing.T) {
patch, err = strategicpatch.CreateTwoWayMergePatch(originalObjJS, changedJS, versionedObj) patch, err = strategicpatch.CreateTwoWayMergePatch(originalObjJS, changedJS, versionedObj)
if err != nil { if err != nil {
t.Errorf("%s: unexpected error: %v", tc.name, err) t.Errorf("%s: unexpected error: %v", tc.name, err)
return continue
} }
case types.MergePatchType: case types.MergePatchType:
patch, err = jsonpatch.CreateMergePatch(originalObjJS, changedJS) patch, err = jsonpatch.CreateMergePatch(originalObjJS, changedJS)
if err != nil { if err != nil {
t.Errorf("%s: unexpected error: %v", tc.name, err) 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 len(tc.expectedError) != 0 {
if err == nil || err.Error() != tc.expectedError { if err == nil || err.Error() != tc.expectedError {
t.Errorf("%s: expected error %v, but got %v", tc.name, tc.expectedError, err) t.Errorf("%s: expected error %v, but got %v", tc.name, tc.expectedError, err)
return continue
} }
} else { } else {
if err != nil { if err != nil {
t.Errorf("%s: unexpected error: %v", tc.name, err) 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 { if resultObj != nil {
t.Errorf("%s: unexpected result: %v", tc.name, resultObj) t.Errorf("%s: unexpected result: %v", tc.name, resultObj)
} }
return continue
} }
resultPod := resultObj.(*api.Pod) resultPod := resultObj.(*api.Pod)
@ -275,18 +277,18 @@ func (tc *patchTestCase) Run(t *testing.T) {
expectedJS, err := runtime.Encode(codec, tc.expectedPod) expectedJS, err := runtime.Encode(codec, tc.expectedPod)
if err != nil { if err != nil {
t.Errorf("%s: unexpected error: %v", tc.name, err) t.Errorf("%s: unexpected error: %v", tc.name, err)
return continue
} }
expectedObj, err := runtime.Decode(codec, expectedJS) expectedObj, err := runtime.Decode(codec, expectedJS)
if err != nil { if err != nil {
t.Errorf("%s: unexpected error: %v", tc.name, err) t.Errorf("%s: unexpected error: %v", tc.name, err)
return continue
} }
reallyExpectedPod := expectedObj.(*api.Pod) reallyExpectedPod := expectedObj.(*api.Pod)
if !reflect.DeepEqual(*reallyExpectedPod, *resultPod) { if !reflect.DeepEqual(*reallyExpectedPod, *resultPod) {
t.Errorf("%s mismatch: %v\n", tc.name, diff.ObjectGoPrintDiff(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) { func TestPatchResourceWithVersionConflict(t *testing.T) {
namespace := "bar" namespace := "bar"
name := "foo" name := "foo"