From a248d1117b910c40cb9dffb16d99c9e69d372ef6 Mon Sep 17 00:00:00 2001 From: Chao Xu Date: Wed, 6 Jan 2016 10:34:08 -0800 Subject: [PATCH 1/2] remove ResourceVersion validation in client --- pkg/api/rest/resttest/resttest.go | 13 +++++--- pkg/client/unversioned/endpoints.go | 5 --- pkg/client/unversioned/events.go | 3 -- pkg/client/unversioned/limit_ranges.go | 6 ---- pkg/client/unversioned/limit_ranges_test.go | 33 ------------------- pkg/client/unversioned/namespaces.go | 4 --- pkg/client/unversioned/nodes.go | 10 ------ .../unversioned/persistentvolumeclaim.go | 6 ---- pkg/client/unversioned/persistentvolumes.go | 6 ---- pkg/registry/generic/etcd/etcd.go | 18 +++++----- 10 files changed, 18 insertions(+), 86 deletions(-) diff --git a/pkg/api/rest/resttest/resttest.go b/pkg/api/rest/resttest/resttest.go index d565eb0abd8..17e0a584220 100644 --- a/pkg/api/rest/resttest/resttest.go +++ b/pkg/api/rest/resttest/resttest.go @@ -148,7 +148,7 @@ func (t *Tester) TestCreate(valid runtime.Object, setFn SetFunc, getFn GetFunc, // Test updating an object. func (t *Tester) TestUpdate(valid runtime.Object, setFn SetFunc, getFn GetFunc, updateFn UpdateFunc, invalidUpdateFn ...UpdateFunc) { t.testUpdateEquals(copyOrDie(valid), setFn, getFn, updateFn) - t.testUpdateFailsOnVersionTooOld(copyOrDie(valid), setFn) + t.testUpdateFailsOnVersionTooOld(copyOrDie(valid), setFn, getFn) t.testUpdateOnNotFound(copyOrDie(valid)) if !t.clusterScope { t.testUpdateRejectsMismatchedNamespace(copyOrDie(valid), setFn) @@ -426,7 +426,7 @@ func (t *Tester) testUpdateEquals(obj runtime.Object, setFn SetFunc, getFn GetFu } } -func (t *Tester) testUpdateFailsOnVersionTooOld(obj runtime.Object, setFn SetFunc) { +func (t *Tester) testUpdateFailsOnVersionTooOld(obj runtime.Object, setFn SetFunc, getFn GetFunc) { ctx := t.TestContext() foo := copyOrDie(obj) @@ -436,11 +436,16 @@ func (t *Tester) testUpdateFailsOnVersionTooOld(obj runtime.Object, setFn SetFun t.Errorf("unexpected error: %v", err) } - older := copyOrDie(foo) + storedFoo, err := getFn(ctx, foo) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + older := copyOrDie(storedFoo) olderMeta := t.getObjectMetaOrFail(older) olderMeta.ResourceVersion = "1" - _, _, err := t.storage.(rest.Updater).Update(t.TestContext(), older) + _, _, err = t.storage.(rest.Updater).Update(t.TestContext(), older) if err == nil { t.Errorf("Expected an error, but we didn't get one") } else if !errors.IsConflict(err) { diff --git a/pkg/client/unversioned/endpoints.go b/pkg/client/unversioned/endpoints.go index 24993efda23..8c525e72e96 100644 --- a/pkg/client/unversioned/endpoints.go +++ b/pkg/client/unversioned/endpoints.go @@ -17,8 +17,6 @@ limitations under the License. package unversioned import ( - "fmt" - "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/watch" ) @@ -92,9 +90,6 @@ func (c *endpoints) Watch(opts api.ListOptions) (watch.Interface, error) { func (c *endpoints) Update(endpoints *api.Endpoints) (*api.Endpoints, error) { result := &api.Endpoints{} - if len(endpoints.ResourceVersion) == 0 { - return nil, fmt.Errorf("invalid update object, missing resource version: %v", endpoints) - } err := c.r.Put(). Namespace(c.ns). Resource("endpoints"). diff --git a/pkg/client/unversioned/events.go b/pkg/client/unversioned/events.go index 2e909319295..ab21dd01fc7 100644 --- a/pkg/client/unversioned/events.go +++ b/pkg/client/unversioned/events.go @@ -86,9 +86,6 @@ func (e *events) Create(event *api.Event) (*api.Event, error) { // created with the "" namespace. Update also requires the ResourceVersion to be set in the event // object. func (e *events) Update(event *api.Event) (*api.Event, error) { - if len(event.ResourceVersion) == 0 { - return nil, fmt.Errorf("invalid event update object, missing resource version: %#v", event) - } result := &api.Event{} err := e.client.Put(). Namespace(event.Namespace). diff --git a/pkg/client/unversioned/limit_ranges.go b/pkg/client/unversioned/limit_ranges.go index 6820e2c22f5..88048f59967 100644 --- a/pkg/client/unversioned/limit_ranges.go +++ b/pkg/client/unversioned/limit_ranges.go @@ -17,8 +17,6 @@ limitations under the License. package unversioned import ( - "fmt" - "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/watch" ) @@ -81,10 +79,6 @@ func (c *limitRanges) Create(limitRange *api.LimitRange) (result *api.LimitRange // Update takes the representation of a limitRange to update. Returns the server's representation of the limitRange, and an error, if it occurs. func (c *limitRanges) Update(limitRange *api.LimitRange) (result *api.LimitRange, err error) { result = &api.LimitRange{} - if len(limitRange.ResourceVersion) == 0 { - err = fmt.Errorf("invalid update object, missing resource version: %v", limitRange) - return - } err = c.r.Put().Namespace(c.ns).Resource("limitRanges").Name(limitRange.Name).Body(limitRange).Do().Into(result) return } diff --git a/pkg/client/unversioned/limit_ranges_test.go b/pkg/client/unversioned/limit_ranges_test.go index dec5448f6a0..f6936570ccd 100644 --- a/pkg/client/unversioned/limit_ranges_test.go +++ b/pkg/client/unversioned/limit_ranges_test.go @@ -164,39 +164,6 @@ func TestLimitRangeUpdate(t *testing.T) { c.Validate(t, response, err) } -func TestInvalidLimitRangeUpdate(t *testing.T) { - ns := api.NamespaceDefault - limitRange := &api.LimitRange{ - ObjectMeta: api.ObjectMeta{ - Name: "abc", - }, - Spec: api.LimitRangeSpec{ - Limits: []api.LimitRangeItem{ - { - Type: api.LimitTypePod, - Max: api.ResourceList{ - api.ResourceCPU: resource.MustParse("100"), - api.ResourceMemory: resource.MustParse("10000"), - }, - Min: api.ResourceList{ - api.ResourceCPU: resource.MustParse("0"), - api.ResourceMemory: resource.MustParse("100"), - }, - }, - }, - }, - } - c := &simple.Client{ - Request: simple.Request{Method: "PUT", Path: testapi.Default.ResourcePath(getLimitRangesResourceName(), ns, "abc"), Query: simple.BuildQueryValues(nil)}, - Response: simple.Response{StatusCode: 200, Body: limitRange}, - } - _, err := c.Setup(t).LimitRanges(ns).Update(limitRange) - defer c.Close() - if err == nil { - t.Errorf("Expected an error due to missing ResourceVersion") - } -} - func TestLimitRangeDelete(t *testing.T) { ns := api.NamespaceDefault c := &simple.Client{ diff --git a/pkg/client/unversioned/namespaces.go b/pkg/client/unversioned/namespaces.go index 9b669f7cd34..2bba0cfc52d 100644 --- a/pkg/client/unversioned/namespaces.go +++ b/pkg/client/unversioned/namespaces.go @@ -68,10 +68,6 @@ func (c *namespaces) List(opts api.ListOptions) (*api.NamespaceList, error) { // Update takes the representation of a namespace to update. Returns the server's representation of the namespace, and an error, if it occurs. func (c *namespaces) Update(namespace *api.Namespace) (result *api.Namespace, err error) { result = &api.Namespace{} - if len(namespace.ResourceVersion) == 0 { - err = fmt.Errorf("invalid update object, missing resource version: %v", namespace) - return - } err = c.r.Put().Resource("namespaces").Name(namespace.Name).Body(namespace).Do().Into(result) return } diff --git a/pkg/client/unversioned/nodes.go b/pkg/client/unversioned/nodes.go index 12570425de4..210f3495e59 100644 --- a/pkg/client/unversioned/nodes.go +++ b/pkg/client/unversioned/nodes.go @@ -17,8 +17,6 @@ limitations under the License. package unversioned import ( - "fmt" - "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/watch" ) @@ -81,20 +79,12 @@ func (c *nodes) Delete(name string) error { // Update updates an existing node. func (c *nodes) Update(node *api.Node) (*api.Node, error) { result := &api.Node{} - if len(node.ResourceVersion) == 0 { - err := fmt.Errorf("invalid update object, missing resource version: %v", node) - return nil, err - } err := c.r.Put().Resource(c.resourceName()).Name(node.Name).Body(node).Do().Into(result) return result, err } func (c *nodes) UpdateStatus(node *api.Node) (*api.Node, error) { result := &api.Node{} - if len(node.ResourceVersion) == 0 { - err := fmt.Errorf("invalid update object, missing resource version: %v", node) - return nil, err - } err := c.r.Put().Resource(c.resourceName()).Name(node.Name).SubResource("status").Body(node).Do().Into(result) return result, err } diff --git a/pkg/client/unversioned/persistentvolumeclaim.go b/pkg/client/unversioned/persistentvolumeclaim.go index 4764e42b052..60a36a18345 100644 --- a/pkg/client/unversioned/persistentvolumeclaim.go +++ b/pkg/client/unversioned/persistentvolumeclaim.go @@ -17,8 +17,6 @@ limitations under the License. package unversioned import ( - "fmt" - "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/watch" ) @@ -77,10 +75,6 @@ func (c *persistentVolumeClaims) Create(claim *api.PersistentVolumeClaim) (resul func (c *persistentVolumeClaims) Update(claim *api.PersistentVolumeClaim) (result *api.PersistentVolumeClaim, err error) { result = &api.PersistentVolumeClaim{} - if len(claim.ResourceVersion) == 0 { - err = fmt.Errorf("invalid update object, missing resource version: %v", claim) - return - } err = c.client.Put().Namespace(c.namespace).Resource("persistentVolumeClaims").Name(claim.Name).Body(claim).Do().Into(result) return } diff --git a/pkg/client/unversioned/persistentvolumes.go b/pkg/client/unversioned/persistentvolumes.go index c9f735c8b31..568effdb5a1 100644 --- a/pkg/client/unversioned/persistentvolumes.go +++ b/pkg/client/unversioned/persistentvolumes.go @@ -17,8 +17,6 @@ limitations under the License. package unversioned import ( - "fmt" - "k8s.io/kubernetes/pkg/api" "k8s.io/kubernetes/pkg/watch" ) @@ -72,10 +70,6 @@ func (c *persistentVolumes) Create(volume *api.PersistentVolume) (result *api.Pe func (c *persistentVolumes) Update(volume *api.PersistentVolume) (result *api.PersistentVolume, err error) { result = &api.PersistentVolume{} - if len(volume.ResourceVersion) == 0 { - err = fmt.Errorf("invalid update object, missing resource version: %v", volume) - return - } err = c.client.Put().Resource("persistentVolumes").Name(volume.Name).Body(volume).Do().Into(result) return } diff --git a/pkg/registry/generic/etcd/etcd.go b/pkg/registry/generic/etcd/etcd.go index 317ffea7972..267b7fde4a9 100644 --- a/pkg/registry/generic/etcd/etcd.go +++ b/pkg/registry/generic/etcd/etcd.go @@ -279,19 +279,19 @@ func (e *Etcd) Update(ctx api.Context, obj runtime.Object) (runtime.Object, bool if err != nil { return nil, nil, err } - } else { - // Check if the object's resource version matches the latest resource version. - newVersion, err := e.Storage.Versioner().ObjectResourceVersion(obj) - if err != nil { - return nil, nil, err - } - if newVersion != version { - return nil, nil, kubeerr.NewConflict(e.QualifiedResource, name, fmt.Errorf("the object has been modified; please apply your changes to the latest version and try again")) - } } if err := rest.BeforeUpdate(e.UpdateStrategy, ctx, obj, existing); err != nil { return nil, nil, err } + // Check if the object's resource version matches the latest resource version. + newVersion, err := e.Storage.Versioner().ObjectResourceVersion(obj) + if err != nil { + return nil, nil, err + } + if newVersion != version { + return nil, nil, kubeerr.NewConflict(e.QualifiedResource, name, fmt.Errorf("the object has been modified; please apply your changes to the latest version and try again")) + } + ttl, err := e.calculateTTL(obj, res.TTL, true) if err != nil { return nil, nil, err From db9b5c97fb6e59f590cc2616155cd932f65e650c Mon Sep 17 00:00:00 2001 From: Chao Xu Date: Wed, 20 Jan 2016 11:16:55 -0800 Subject: [PATCH 2/2] fix e2e --- pkg/registry/generic/etcd/etcd.go | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/pkg/registry/generic/etcd/etcd.go b/pkg/registry/generic/etcd/etcd.go index 267b7fde4a9..a6532d9d42f 100644 --- a/pkg/registry/generic/etcd/etcd.go +++ b/pkg/registry/generic/etcd/etcd.go @@ -32,6 +32,7 @@ import ( "k8s.io/kubernetes/pkg/registry/generic" "k8s.io/kubernetes/pkg/runtime" "k8s.io/kubernetes/pkg/storage" + "k8s.io/kubernetes/pkg/util/validation/field" "k8s.io/kubernetes/pkg/watch" "github.com/golang/glog" @@ -279,19 +280,27 @@ func (e *Etcd) Update(ctx api.Context, obj runtime.Object) (runtime.Object, bool if err != nil { return nil, nil, err } + } else { + // Check if the object's resource version matches the latest resource version. + newVersion, err := e.Storage.Versioner().ObjectResourceVersion(obj) + if err != nil { + return nil, nil, err + } + if newVersion == 0 { + // TODO: The Invalid error should has a field for Resource. + // After that field is added, we should fill the Resource and + // leave the Kind field empty. See the discussion in #18526. + qualifiedKind := unversioned.GroupKind{e.QualifiedResource.Group, e.QualifiedResource.Resource} + fieldErrList := field.ErrorList{field.Invalid(field.NewPath("metadata").Child("resourceVersion"), newVersion, "must be specified for an update")} + return nil, nil, kubeerr.NewInvalid(qualifiedKind, name, fieldErrList) + } + if newVersion != version { + return nil, nil, kubeerr.NewConflict(e.QualifiedResource, name, fmt.Errorf("the object has been modified; please apply your changes to the latest version and try again")) + } } if err := rest.BeforeUpdate(e.UpdateStrategy, ctx, obj, existing); err != nil { return nil, nil, err } - // Check if the object's resource version matches the latest resource version. - newVersion, err := e.Storage.Versioner().ObjectResourceVersion(obj) - if err != nil { - return nil, nil, err - } - if newVersion != version { - return nil, nil, kubeerr.NewConflict(e.QualifiedResource, name, fmt.Errorf("the object has been modified; please apply your changes to the latest version and try again")) - } - ttl, err := e.calculateTTL(obj, res.TTL, true) if err != nil { return nil, nil, err