From 9841cf11a1c511e0f08e18048ea4654525355069 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}} ``` Kubernetes-commit: 1ab6a33db486adc060e1b63eecbdc06aabdde1f6 --- pkg/endpoints/handlers/rest.go | 2 +- pkg/endpoints/handlers/rest_test.go | 85 ++++++++++++++++++++++++----- 2 files changed, 71 insertions(+), 16 deletions(-) diff --git a/pkg/endpoints/handlers/rest.go b/pkg/endpoints/handlers/rest.go index 25b9afd79..a10410e3f 100644 --- a/pkg/endpoints/handlers/rest.go +++ b/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/pkg/endpoints/handlers/rest_test.go b/pkg/endpoints/handlers/rest_test.go index b145d6efd..5c03a6180 100644 --- a/pkg/endpoints/handlers/rest_test.go +++ b/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"