From 1ab6a33db486adc060e1b63eecbdc06aabdde1f6 Mon Sep 17 00:00:00 2001 From: Anthony Yeh Date: Fri, 21 Apr 2017 17:24:07 -0700 Subject: [PATCH] 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}} ``` --- .../apiserver/pkg/endpoints/handlers/rest.go | 2 +- .../pkg/endpoints/handlers/rest_test.go | 85 +++++++++++++++---- 2 files changed, 71 insertions(+), 16 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go index 25b9afd798..a10410e3f9 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go +++ b/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" diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go index b145d6efd3..5c03a61804 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest_test.go +++ b/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"