From 10074505ca32e1b1d41f1fbf15b2f63eb4d898c8 Mon Sep 17 00:00:00 2001 From: ajatprabha Date: Thu, 14 Feb 2019 04:09:12 +0530 Subject: [PATCH 1/4] add ResourceVersion to DeleteOptions.Preconditions Kubernetes-commit: 8f48d762717dfe1f479cfabaabc4fb9261a1bc8f --- pkg/registry/generic/registry/store.go | 2 ++ pkg/registry/rest/delete.go | 9 +++++-- pkg/registry/rest/resttest/resttest.go | 37 ++++++++++++++++++++++++++ pkg/storage/interfaces.go | 3 +++ 4 files changed, 49 insertions(+), 2 deletions(-) diff --git a/pkg/registry/generic/registry/store.go b/pkg/registry/generic/registry/store.go index c84a3276c..be15e2bb3 100644 --- a/pkg/registry/generic/registry/store.go +++ b/pkg/registry/generic/registry/store.go @@ -448,6 +448,7 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj storagePreconditions := &storage.Preconditions{} if preconditions := objInfo.Preconditions(); preconditions != nil { storagePreconditions.UID = preconditions.UID + storagePreconditions.ResourceVersion = preconditions.ResourceVersion } out := e.NewFunc() @@ -879,6 +880,7 @@ func (e *Store) Delete(ctx context.Context, name string, options *metav1.DeleteO var preconditions storage.Preconditions if options.Preconditions != nil { preconditions.UID = options.Preconditions.UID + preconditions.ResourceVersion = options.Preconditions.ResourceVersion } graceful, pendingGraceful, err := rest.BeforeDelete(e.DeleteStrategy, ctx, obj, options) if err != nil { diff --git a/pkg/registry/rest/delete.go b/pkg/registry/rest/delete.go index 7c39f6be1..0713464e0 100644 --- a/pkg/registry/rest/delete.go +++ b/pkg/registry/rest/delete.go @@ -77,8 +77,13 @@ func BeforeDelete(strategy RESTDeleteStrategy, ctx context.Context, obj runtime. return false, false, errors.NewInvalid(schema.GroupKind{Group: metav1.GroupName, Kind: "DeleteOptions"}, "", errs) } // Checking the Preconditions here to fail early. They'll be enforced later on when we actually do the deletion, too. - if options.Preconditions != nil && options.Preconditions.UID != nil && *options.Preconditions.UID != objectMeta.GetUID() { - return false, false, errors.NewConflict(schema.GroupResource{Group: gvk.Group, Resource: gvk.Kind}, objectMeta.GetName(), fmt.Errorf("the UID in the precondition (%s) does not match the UID in record (%s). The object might have been deleted and then recreated", *options.Preconditions.UID, objectMeta.GetUID())) + if options.Preconditions != nil { + if options.Preconditions.UID != nil && *options.Preconditions.UID != objectMeta.GetUID() { + return false, false, errors.NewConflict(schema.GroupResource{Group: gvk.Group, Resource: gvk.Kind}, objectMeta.GetName(), fmt.Errorf("the UID in the precondition (%s) does not match the UID in record (%s). The object might have been deleted and then recreated", *options.Preconditions.UID, objectMeta.GetUID())) + } + if options.Preconditions.ResourceVersion != nil && *options.Preconditions.ResourceVersion != objectMeta.GetResourceVersion() { + return false, false, errors.NewConflict(schema.GroupResource{Group: gvk.Group, Resource: gvk.Kind}, objectMeta.GetName(), fmt.Errorf("the ResourceVersion in the precondition (%s) does not match the ResourceVersion in record (%s). The object might have been deleted and then recreated", *options.Preconditions.ResourceVersion, objectMeta.GetResourceVersion())) + } } gracefulStrategy, ok := strategy.(RESTGracefulDeleteStrategy) if !ok { diff --git a/pkg/registry/rest/resttest/resttest.go b/pkg/registry/rest/resttest/resttest.go index 8e6b55542..4316592f4 100644 --- a/pkg/registry/rest/resttest/resttest.go +++ b/pkg/registry/rest/resttest/resttest.go @@ -909,6 +909,43 @@ func (t *Tester) testDeleteWithUID(obj runtime.Object, createFn CreateFunc, getF } } +// This test the fast-fail path. We test that the precondition gets verified +// again before deleting the object in tests of pkg/storage/etcd. +func (t *Tester) testDeleteWithResourceVersion(obj runtime.Object, createFn CreateFunc, getFn GetFunc, isNotFoundFn IsErrorFunc, opts metav1.DeleteOptions) { + ctx := t.TestContext() + + foo := obj.DeepCopyObject() + t.setObjectMeta(foo, t.namer(1)) + objectMeta := t.getObjectMetaOrFail(foo) + objectMeta.SetResourceVersion("RV0000") + if err := createFn(ctx, foo); err != nil { + t.Errorf("unexpected error: %v", err) + } + opts.Preconditions = metav1.NewPreconditionDeleteOptionsWithResourceVersion("RV1111").Preconditions + obj, _, err := t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), &opts) + if err == nil || !errors.IsConflict(err) { + t.Errorf("unexpected error: %v", err) + } + + obj, _, err = t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), metav1.NewPreconditionDeleteOptionsWithResourceVersion("RV0000")) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + if !t.returnDeletedObject { + if status, ok := obj.(*metav1.Status); !ok { + t.Errorf("expected status of delete, got %v", status) + } else if status.Status != metav1.StatusSuccess { + t.Errorf("expected success, got: %v", status.Status) + } + } + + _, err = getFn(ctx, foo) + if err == nil || !isNotFoundFn(err) { + t.Errorf("unexpected error: %v", err) + } +} + // ============================================================================= // Graceful Deletion tests. diff --git a/pkg/storage/interfaces.go b/pkg/storage/interfaces.go index 56dd6dbdc..93b3f5deb 100644 --- a/pkg/storage/interfaces.go +++ b/pkg/storage/interfaces.go @@ -98,6 +98,9 @@ type Preconditions struct { // Specifies the target UID. // +optional UID *types.UID `json:"uid,omitempty"` + // Specifies the target ResourceVersion + // +optional + ResourceVersion *string `json:"resourceVersion,omitempty"` } // NewUIDPreconditions returns a Preconditions with UID set. From e6793285a239ae6da820e8b8d0ede26bab25c192 Mon Sep 17 00:00:00 2001 From: ajatprabha Date: Fri, 15 Feb 2019 04:37:18 +0530 Subject: [PATCH 2/4] check for ResourceVersion conflict in separate if block Kubernetes-commit: 42f0a36f44bac33f4230fdea9f3dcc4bfb645a0a --- pkg/registry/rest/delete.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/registry/rest/delete.go b/pkg/registry/rest/delete.go index 0713464e0..16c9e1b40 100644 --- a/pkg/registry/rest/delete.go +++ b/pkg/registry/rest/delete.go @@ -82,7 +82,7 @@ func BeforeDelete(strategy RESTDeleteStrategy, ctx context.Context, obj runtime. return false, false, errors.NewConflict(schema.GroupResource{Group: gvk.Group, Resource: gvk.Kind}, objectMeta.GetName(), fmt.Errorf("the UID in the precondition (%s) does not match the UID in record (%s). The object might have been deleted and then recreated", *options.Preconditions.UID, objectMeta.GetUID())) } if options.Preconditions.ResourceVersion != nil && *options.Preconditions.ResourceVersion != objectMeta.GetResourceVersion() { - return false, false, errors.NewConflict(schema.GroupResource{Group: gvk.Group, Resource: gvk.Kind}, objectMeta.GetName(), fmt.Errorf("the ResourceVersion in the precondition (%s) does not match the ResourceVersion in record (%s). The object might have been deleted and then recreated", *options.Preconditions.ResourceVersion, objectMeta.GetResourceVersion())) + return false, false, errors.NewConflict(schema.GroupResource{Group: gvk.Group, Resource: gvk.Kind}, objectMeta.GetName(), fmt.Errorf("the ResourceVersion in the precondition (%s) does not match the ResourceVersion in record (%s). The object might have been modified", *options.Preconditions.ResourceVersion, objectMeta.GetResourceVersion())) } } gracefulStrategy, ok := strategy.(RESTGracefulDeleteStrategy) From ad205f8388e8cddc174bb5aaa28efd2c5cfc2202 Mon Sep 17 00:00:00 2001 From: ajatprabha Date: Fri, 15 Feb 2019 12:30:34 +0530 Subject: [PATCH 3/4] add ResourceVersion check to Preconditions#Check Kubernetes-commit: 3135cea2cce02bc8c6796e97f4579d538f357f74 --- pkg/storage/interfaces.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/storage/interfaces.go b/pkg/storage/interfaces.go index 93b3f5deb..cff425f3a 100644 --- a/pkg/storage/interfaces.go +++ b/pkg/storage/interfaces.go @@ -128,8 +128,14 @@ func (p *Preconditions) Check(key string, obj runtime.Object) error { objMeta.GetUID()) return NewInvalidObjError(key, err) } + if p.ResourceVersion != nil && *p.ResourceVersion != objMeta.GetResourceVersion() { + err := fmt.Sprintf( + "Precondition failed: ResourceVersion in precondition: %v, ResourceVersion in object meta: %v", + *p.ResourceVersion, + objMeta.GetResourceVersion()) + return NewInvalidObjError(key, err) + } return nil - } // Interface offers a common interface for object marshaling/unmarshaling operations and From 9f2ef664a0002ec68a71d1775c6fb0cbb16b0d3e Mon Sep 17 00:00:00 2001 From: ajatprabha Date: Mon, 25 Feb 2019 22:43:28 +0530 Subject: [PATCH 4/4] update testDeleteWithResourceVersion Kubernetes-commit: fe3b9f486fe988cf6b6cd83c54c2be77892fa123 --- pkg/registry/rest/resttest/resttest.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg/registry/rest/resttest/resttest.go b/pkg/registry/rest/resttest/resttest.go index 4316592f4..f29f4325b 100644 --- a/pkg/registry/rest/resttest/resttest.go +++ b/pkg/registry/rest/resttest/resttest.go @@ -921,13 +921,15 @@ func (t *Tester) testDeleteWithResourceVersion(obj runtime.Object, createFn Crea if err := createFn(ctx, foo); err != nil { t.Errorf("unexpected error: %v", err) } - opts.Preconditions = metav1.NewPreconditionDeleteOptionsWithResourceVersion("RV1111").Preconditions - obj, _, err := t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), &opts) + opts.Preconditions = metav1.NewRVDeletionPrecondition("RV1111").Preconditions + obj, wasDeleted, err := t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), &opts) if err == nil || !errors.IsConflict(err) { t.Errorf("unexpected error: %v", err) } - - obj, _, err = t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), metav1.NewPreconditionDeleteOptionsWithResourceVersion("RV0000")) + if wasDeleted { + t.Errorf("unexpected, object %s should not have been deleted immediately", objectMeta.GetName()) + } + obj, _, err = t.storage.(rest.GracefulDeleter).Delete(ctx, objectMeta.GetName(), metav1.NewRVDeletionPrecondition("RV0000")) if err != nil { t.Errorf("unexpected error: %v", err) }