diff --git a/pkg/registry/core/service/storage/storage.go b/pkg/registry/core/service/storage/storage.go index ba7d86980aa..6aabaec0f2b 100644 --- a/pkg/registry/core/service/storage/storage.go +++ b/pkg/registry/core/service/storage/storage.go @@ -131,42 +131,33 @@ func (r *StatusREST) Update(ctx context.Context, name string, objInfo rest.Updat // applies on Get, Create, and Update, but we need to distinguish between them. // // This will be called on both Service and ServiceList types. -func (r *GenericREST) defaultOnRead(obj runtime.Object) error { - service, ok := obj.(*api.Service) - if ok { - return r.defaultOnReadService(service) +func (r *GenericREST) defaultOnRead(obj runtime.Object) { + switch s := obj.(type) { + case *api.Service: + r.defaultOnReadService(s) + case *api.ServiceList: + r.defaultOnReadServiceList(s) + default: + // This was not an object we can default. This is not an error, as the + // caching layer can pass through here, too. } - - serviceList, ok := obj.(*api.ServiceList) - if ok { - return r.defaultOnReadServiceList(serviceList) - } - - // This was not an object we can default. This is not an error, as the - // caching layer can pass through here, too. - return nil } // defaultOnReadServiceList defaults a ServiceList. -func (r *GenericREST) defaultOnReadServiceList(serviceList *api.ServiceList) error { +func (r *GenericREST) defaultOnReadServiceList(serviceList *api.ServiceList) { if serviceList == nil { - return nil + return } for i := range serviceList.Items { - err := r.defaultOnReadService(&serviceList.Items[i]) - if err != nil { - return err - } + r.defaultOnReadService(&serviceList.Items[i]) } - - return nil } // defaultOnReadService defaults a single Service. -func (r *GenericREST) defaultOnReadService(service *api.Service) error { +func (r *GenericREST) defaultOnReadService(service *api.Service) { if service == nil { - return nil + return } // We might find Services that were written before ClusterIP became plural. @@ -176,11 +167,11 @@ func (r *GenericREST) defaultOnReadService(service *api.Service) error { // The rest of this does not apply unless dual-stack is enabled. if !utilfeature.DefaultFeatureGate.Enabled(features.IPv6DualStack) { - return nil + return } if len(service.Spec.IPFamilies) > 0 { - return nil // already defaulted + return // already defaulted } // set clusterIPs based on ClusterIP @@ -241,6 +232,4 @@ func (r *GenericREST) defaultOnReadService(service *api.Service) error { } } } - - return nil } diff --git a/pkg/registry/core/service/storage/storage_test.go b/pkg/registry/core/service/storage/storage_test.go index 271e47c27ac..3c5fc8040bf 100644 --- a/pkg/registry/core/service/storage/storage_test.go +++ b/pkg/registry/core/service/storage/storage_test.go @@ -358,10 +358,9 @@ func TestServiceDefaultOnRead(t *testing.T) { } testCases := []struct { - name string - input runtime.Object - expectErr bool - expect runtime.Object + name string + input runtime.Object + expect runtime.Object }{{ name: "no change v4", input: makeService(nil), @@ -403,9 +402,8 @@ func TestServiceDefaultOnRead(t *testing.T) { }), expect: makeService(nil), }, { - name: "not Service or ServiceList", - input: &api.Pod{}, - expectErr: false, + name: "not Service or ServiceList", + input: &api.Pod{}, }} for _, tc := range testCases { @@ -435,13 +433,7 @@ func TestServiceDefaultOnRead(t *testing.T) { defer storage.Store.DestroyFunc() tmp := tc.input.DeepCopyObject() - err := storage.defaultOnRead(tmp) - if err != nil && !tc.expectErr { - t.Errorf("unexpected error: %v", err) - } - if err == nil && tc.expectErr { - t.Errorf("unexpected success") - } + storage.defaultOnRead(tmp) svc, ok := tmp.(*api.Service) if !ok { diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/BUILD b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/BUILD index d948a3c0726..2483440c3ad 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/BUILD @@ -46,6 +46,7 @@ go_test( "//staging/src/k8s.io/apiserver/pkg/storage/storagebackend/factory:go_default_library", "//staging/src/k8s.io/apiserver/pkg/storage/testing:go_default_library", "//staging/src/k8s.io/client-go/tools/cache:go_default_library", + "//vendor/github.com/google/gofuzz:go_default_library", ], ) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/decorated_watcher.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/decorated_watcher.go index 005a376d404..034bf12c94c 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/decorated_watcher.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/decorated_watcher.go @@ -21,17 +21,18 @@ import ( "net/http" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/watch" ) type decoratedWatcher struct { w watch.Interface - decorator ObjectFunc + decorator func(runtime.Object) cancel context.CancelFunc resultCh chan watch.Event } -func newDecoratedWatcher(w watch.Interface, decorator ObjectFunc) *decoratedWatcher { +func newDecoratedWatcher(w watch.Interface, decorator func(runtime.Object)) *decoratedWatcher { ctx, cancel := context.WithCancel(context.Background()) d := &decoratedWatcher{ w: w, @@ -56,11 +57,7 @@ func (d *decoratedWatcher) run(ctx context.Context) { } switch recv.Type { case watch.Added, watch.Modified, watch.Deleted, watch.Bookmark: - err := d.decorator(recv.Object) - if err != nil { - send = makeStatusErrorEvent(err) - break - } + d.decorator(recv.Object) send = recv case watch.Error: send = recv diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/decorated_watcher_test.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/decorated_watcher_test.go index 0afbd773f07..33e47c8af1b 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/decorated_watcher_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/decorated_watcher_test.go @@ -17,7 +17,6 @@ limitations under the License. package registry import ( - "fmt" "testing" "time" @@ -30,10 +29,9 @@ import ( func TestDecoratedWatcher(t *testing.T) { w := watch.NewFake() - decorator := func(obj runtime.Object) error { + decorator := func(obj runtime.Object) { pod := obj.(*example.Pod) pod.Annotations = map[string]string{"decorated": "true"} - return nil } dw := newDecoratedWatcher(w, decorator) defer dw.Stop() @@ -53,23 +51,3 @@ func TestDecoratedWatcher(t *testing.T) { t.Errorf("timeout after %v", wait.ForeverTestTimeout) } } - -func TestDecoratedWatcherError(t *testing.T) { - w := watch.NewFake() - expErr := fmt.Errorf("expected error") - decorator := func(obj runtime.Object) error { - return expErr - } - dw := newDecoratedWatcher(w, decorator) - defer dw.Stop() - - go w.Add(&example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "foo"}}) - select { - case e := <-dw.ResultChan(): - if e.Type != watch.Error { - t.Errorf("event type want=%v, get=%v", watch.Error, e.Type) - } - case <-time.After(wait.ForeverTestTimeout): - t.Errorf("timeout after %v", wait.ForeverTestTimeout) - } -} diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go index bbc167d3996..4a80a43948d 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go @@ -50,10 +50,23 @@ import ( "k8s.io/klog/v2" ) -// ObjectFunc is a function to act on a given object. An error may be returned -// if the hook cannot be completed. An ObjectFunc may transform the provided -// object. -type ObjectFunc func(obj runtime.Object) error +// FinishFunc is a function returned by Begin hooks to complete an operation. +type FinishFunc func(ctx context.Context, success bool) + +// AfterDeleteFunc is the type used for the Store.AfterDelete hook. +type AfterDeleteFunc func(obj runtime.Object, options *metav1.DeleteOptions) + +// BeginCreateFunc is the type used for the Store.BeginCreate hook. +type BeginCreateFunc func(ctx context.Context, obj runtime.Object, options *metav1.CreateOptions) (FinishFunc, error) + +// AfterCreateFunc is the type used for the Store.AfterCreate hook. +type AfterCreateFunc func(obj runtime.Object, options *metav1.CreateOptions) + +// BeginUpdateFunc is the type used for the Store.BeginUpdate hook. +type BeginUpdateFunc func(ctx context.Context, obj, old runtime.Object, options *metav1.UpdateOptions) (FinishFunc, error) + +// AfterUpdateFunc is the type used for the Store.AfterUpdate hook. +type AfterUpdateFunc func(obj runtime.Object, options *metav1.UpdateOptions) // GenericStore interface can be used for type assertions when we need to access the underlying strategies. type GenericStore interface { @@ -63,10 +76,11 @@ type GenericStore interface { GetExportStrategy() rest.RESTExportStrategy } -// Store implements pkg/api/rest.StandardStorage. It's intended to be -// embeddable and allows the consumer to implement any non-generic functions -// that are required. This object is intended to be copyable so that it can be -// used in different ways but share the same underlying behavior. +// Store implements k8s.io/apiserver/pkg/registry/rest.StandardStorage. It's +// intended to be embeddable and allows the consumer to implement any +// non-generic functions that are required. This object is intended to be +// copyable so that it can be used in different ways but share the same +// underlying behavior. // // All fields are required unless specified. // @@ -145,24 +159,37 @@ type Store struct { // integrations that are above storage and should only be used for // specific cases where storage of the value is not appropriate, since // they cannot be watched. - Decorator ObjectFunc + Decorator func(runtime.Object) + // CreateStrategy implements resource-specific behavior during creation. CreateStrategy rest.RESTCreateStrategy + // BeginCreate is an optional hook that returns a "transaction-like" + // commit/revert function which will be called at the end of the operation, + // but before AfterCreate and Decorator, indicating via the argument + // whether the operation succeeded. If this returns an error, the function + // is not called. Almost nobody should use this hook. + BeginCreate BeginCreateFunc // AfterCreate implements a further operation to run after a resource is // created and before it is decorated, optional. - AfterCreate ObjectFunc + AfterCreate AfterCreateFunc // UpdateStrategy implements resource-specific behavior during updates. UpdateStrategy rest.RESTUpdateStrategy + // BeginUpdate is an optional hook that returns a "transaction-like" + // commit/revert function which will be called at the end of the operation, + // but before AfterUpdate and Decorator, indicating via the argument + // whether the operation succeeded. If this returns an error, the function + // is not called. Almost nobody should use this hook. + BeginUpdate BeginUpdateFunc // AfterUpdate implements a further operation to run after a resource is // updated and before it is decorated, optional. - AfterUpdate ObjectFunc + AfterUpdate AfterUpdateFunc // DeleteStrategy implements resource-specific behavior during deletion. DeleteStrategy rest.RESTDeleteStrategy // AfterDelete implements a further operation to run after a resource is // deleted and before it is decorated, optional. - AfterDelete ObjectFunc + AfterDelete AfterDeleteFunc // ReturnDeletedObject determines whether the Store returns the object // that was deleted. Otherwise, return a generic success status response. ReturnDeletedObject bool @@ -171,9 +198,11 @@ type Store struct { // If specified, this is checked in addition to standard finalizer, // deletionTimestamp, and deletionGracePeriodSeconds checks. ShouldDeleteDuringUpdate func(ctx context.Context, key string, obj, existing runtime.Object) bool + // ExportStrategy implements resource-specific behavior during export, // optional. Exported objects are not decorated. ExportStrategy rest.RESTExportStrategy + // TableConvertor is an optional interface for transforming items or lists // of items into tabular output. If unset, the default will be used. TableConvertor rest.TableConvertor @@ -304,9 +333,7 @@ func (e *Store) List(ctx context.Context, options *metainternalversion.ListOptio return nil, err } if e.Decorator != nil { - if err := e.Decorator(out); err != nil { - return nil, err - } + e.Decorator(out) } return out, nil } @@ -335,8 +362,24 @@ func (e *Store) ListPredicate(ctx context.Context, p storage.SelectionPredicate, return list, storeerr.InterpretListError(err, qualifiedResource) } +// finishNothing is a do-nothing FinishFunc. +func finishNothing(context.Context, bool) {} + // Create inserts a new item according to the unique key from the object. func (e *Store) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { + var finishCreate FinishFunc = finishNothing + + if e.BeginCreate != nil { + fn, err := e.BeginCreate(ctx, obj, options) + if err != nil { + return nil, err + } + finishCreate = fn + defer func() { + finishCreate(ctx, false) + }() + } + if err := rest.BeforeCreate(e.CreateStrategy, ctx, obj); err != nil { return nil, err } @@ -381,15 +424,17 @@ func (e *Store) Create(ctx context.Context, obj runtime.Object, createValidation } return nil, err } + // The operation has succeeded. Call the finish function if there is one, + // and then make sure the defer doesn't call it again. + fn := finishCreate + finishCreate = finishNothing + fn(ctx, true) + if e.AfterCreate != nil { - if err := e.AfterCreate(out); err != nil { - return nil, err - } + e.AfterCreate(out, options) } if e.Decorator != nil { - if err := e.Decorator(out); err != nil { - return nil, err - } + e.Decorator(out) } return out, nil } @@ -424,16 +469,16 @@ func ShouldDeleteDuringUpdate(ctx context.Context, key string, obj, existing run // deleteWithoutFinalizers handles deleting an object ignoring its finalizer list. // Used for objects that are either been finalized or have never initialized. -func (e *Store) deleteWithoutFinalizers(ctx context.Context, name, key string, obj runtime.Object, preconditions *storage.Preconditions, dryRun bool) (runtime.Object, bool, error) { +func (e *Store) deleteWithoutFinalizers(ctx context.Context, name, key string, obj runtime.Object, preconditions *storage.Preconditions, options *metav1.DeleteOptions) (runtime.Object, bool, error) { out := e.NewFunc() klog.V(6).Infof("going to delete %s from registry, triggered by update", name) // Using the rest.ValidateAllObjectFunc because the request is an UPDATE request and has already passed the admission for the UPDATE verb. - if err := e.Storage.Delete(ctx, key, out, preconditions, rest.ValidateAllObjectFunc, dryRun, nil); err != nil { + if err := e.Storage.Delete(ctx, key, out, preconditions, rest.ValidateAllObjectFunc, dryrun.IsDryRun(options.DryRun), nil); err != nil { // Deletion is racy, i.e., there could be multiple update // requests to remove all finalizers from the object, so we // ignore the NotFound error. if storage.IsNotFound(err) { - _, err := e.finalizeDelete(ctx, obj, true) + _, err := e.finalizeDelete(ctx, obj, true, options) // clients are expecting an updated object if a PUT succeeded, // but finalizeDelete returns a metav1.Status, so return // the object in the request instead. @@ -441,7 +486,7 @@ func (e *Store) deleteWithoutFinalizers(ctx context.Context, name, key string, o } return nil, false, storeerr.InterpretDeleteError(err, e.qualifiedResourceFromContext(ctx), name) } - _, err := e.finalizeDelete(ctx, out, true) + _, err := e.finalizeDelete(ctx, out, true, options) // clients are expecting an updated object if a PUT succeeded, but // finalizeDelete returns a metav1.Status, so return the object in // the request instead. @@ -500,6 +545,19 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj doUnconditionalUpdate := newResourceVersion == 0 && e.UpdateStrategy.AllowUnconditionalUpdate() if existingResourceVersion == 0 { + var finishCreate FinishFunc = finishNothing + + if e.BeginCreate != nil { + fn, err := e.BeginCreate(ctx, obj, newCreateOptionsFromUpdateOptions(options)) + if err != nil { + return nil, nil, err + } + finishCreate = fn + defer func() { + finishCreate(ctx, false) + }() + } + creating = true creatingObj = obj if err := rest.BeforeCreate(e.CreateStrategy, ctx, obj); err != nil { @@ -517,6 +575,12 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj return nil, nil, err } + // The operation has succeeded. Call the finish function if there is one, + // and then make sure the defer doesn't call it again. + fn := finishCreate + finishCreate = finishNothing + fn(ctx, true) + return obj, &ttl, nil } @@ -544,6 +608,20 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj return nil, nil, apierrors.NewConflict(qualifiedResource, name, fmt.Errorf(OptimisticLockErrorMsg)) } } + + var finishUpdate FinishFunc = finishNothing + + if e.BeginUpdate != nil { + fn, err := e.BeginUpdate(ctx, obj, existing, options) + if err != nil { + return nil, nil, err + } + finishUpdate = fn + defer func() { + finishUpdate(ctx, false) + }() + } + if err := rest.BeforeUpdate(e.UpdateStrategy, ctx, obj, existing); err != nil { return nil, nil, err } @@ -564,6 +642,13 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj if err != nil { return nil, nil, err } + + // The operation has succeeded. Call the finish function if there is one, + // and then make sure the defer doesn't call it again. + fn := finishUpdate + finishUpdate = finishNothing + fn(ctx, true) + if int64(ttl) != res.TTL { return obj, &ttl, nil } @@ -573,7 +658,7 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj if err != nil { // delete the object if err == errEmptiedFinalizers { - return e.deleteWithoutFinalizers(ctx, name, key, deleteObj, storagePreconditions, dryrun.IsDryRun(options.DryRun)) + return e.deleteWithoutFinalizers(ctx, name, key, deleteObj, storagePreconditions, newDeleteOptionsFromUpdateOptions(options)) } if creating { err = storeerr.InterpretCreateError(err, qualifiedResource, name) @@ -586,25 +671,40 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj if creating { if e.AfterCreate != nil { - if err := e.AfterCreate(out); err != nil { - return nil, false, err - } + e.AfterCreate(out, newCreateOptionsFromUpdateOptions(options)) } } else { if e.AfterUpdate != nil { - if err := e.AfterUpdate(out); err != nil { - return nil, false, err - } + e.AfterUpdate(out, options) } } if e.Decorator != nil { - if err := e.Decorator(out); err != nil { - return nil, false, err - } + e.Decorator(out) } return out, creating, nil } +// This is a helper to convert UpdateOptions to CreateOptions for the +// create-on-update path. +func newCreateOptionsFromUpdateOptions(in *metav1.UpdateOptions) *metav1.CreateOptions { + co := &metav1.CreateOptions{ + DryRun: in.DryRun, + FieldManager: in.FieldManager, + } + co.TypeMeta.SetGroupVersionKind(metav1.SchemeGroupVersion.WithKind("CreateOptions")) + return co +} + +// This is a helper to convert UpdateOptions to DeleteOptions for the +// delete-on-update path. +func newDeleteOptionsFromUpdateOptions(in *metav1.UpdateOptions) *metav1.DeleteOptions { + do := &metav1.DeleteOptions{ + DryRun: in.DryRun, + } + do.TypeMeta.SetGroupVersionKind(metav1.SchemeGroupVersion.WithKind("DeleteOptions")) + return do +} + // Get retrieves the item from storage. func (e *Store) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) { obj := e.NewFunc() @@ -616,9 +716,7 @@ func (e *Store) Get(ctx context.Context, name string, options *metav1.GetOptions return nil, storeerr.InterpretGetError(err, e.qualifiedResourceFromContext(ctx), name) } if e.Decorator != nil { - if err := e.Decorator(obj); err != nil { - return nil, err - } + e.Decorator(obj) } return obj, nil } @@ -879,7 +977,7 @@ func (e *Store) updateForGracefulDeletionAndFinalizers(ctx context.Context, name // we should fall through and truly delete the object. return nil, false, true, out, lastExisting case errAlreadyDeleting: - out, err = e.finalizeDelete(ctx, in, true) + out, err = e.finalizeDelete(ctx, in, true, options) return err, false, false, out, lastExisting default: return storeerr.InterpretUpdateError(err, e.qualifiedResourceFromContext(ctx), name), false, false, out, lastExisting @@ -913,7 +1011,7 @@ func (e *Store) Delete(ctx context.Context, name string, deleteValidation rest.V } // this means finalizers cannot be updated via DeleteOptions if a deletion is already pending if pendingGraceful { - out, err := e.finalizeDelete(ctx, obj, false) + out, err := e.finalizeDelete(ctx, obj, false, options) return out, false, err } // check if obj has pending finalizers @@ -969,12 +1067,12 @@ func (e *Store) Delete(ctx context.Context, name string, deleteValidation rest.V if storage.IsNotFound(err) && ignoreNotFound && lastExisting != nil { // The lastExisting object may not be the last state of the object // before its deletion, but it's the best approximation. - out, err := e.finalizeDelete(ctx, lastExisting, true) + out, err := e.finalizeDelete(ctx, lastExisting, true, options) return out, true, err } return nil, false, storeerr.InterpretDeleteError(err, qualifiedResource, name) } - out, err = e.finalizeDelete(ctx, out, true) + out, err = e.finalizeDelete(ctx, out, true, options) return out, true, err } @@ -1072,17 +1170,13 @@ func (e *Store) DeleteCollection(ctx context.Context, deleteValidation rest.Vali // finalizeDelete runs the Store's AfterDelete hook if runHooks is set and // returns the decorated deleted object if appropriate. -func (e *Store) finalizeDelete(ctx context.Context, obj runtime.Object, runHooks bool) (runtime.Object, error) { +func (e *Store) finalizeDelete(ctx context.Context, obj runtime.Object, runHooks bool, options *metav1.DeleteOptions) (runtime.Object, error) { if runHooks && e.AfterDelete != nil { - if err := e.AfterDelete(obj); err != nil { - return nil, err - } + e.AfterDelete(obj, options) } if e.ReturnDeletedObject { if e.Decorator != nil { - if err := e.Decorator(obj); err != nil { - return nil, err - } + e.Decorator(obj) } return obj, nil } diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go index cf679d066a8..3f89a3fac82 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store_test.go @@ -18,6 +18,7 @@ package registry import ( "context" + "encoding/json" "fmt" "path" "reflect" @@ -27,6 +28,7 @@ import ( "testing" "time" + fuzz "github.com/google/gofuzz" apitesting "k8s.io/apimachinery/pkg/api/apitesting" apiequality "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/errors" @@ -310,11 +312,6 @@ func TestStoreCreate(t *testing.T) { // re-define delete strategy to have graceful delete capability defaultDeleteStrategy := testRESTStrategy{scheme, names.SimpleNameGenerator, true, false, true} registry.DeleteStrategy = testGracefulStrategy{defaultDeleteStrategy} - registry.Decorator = func(obj runtime.Object) error { - pod := obj.(*example.Pod) - pod.Status.Phase = example.PodPhase("Testing") - return nil - } // create the object with denying admission _, err := registry.Create(testContext, podA, denyCreateValidation, &metav1.CreateOptions{}) @@ -328,11 +325,6 @@ func TestStoreCreate(t *testing.T) { t.Errorf("Unexpected error: %v", err) } - // verify the decorator was called - if objA.(*example.Pod).Status.Phase != example.PodPhase("Testing") { - t.Errorf("Decorator was not called: %#v", objA) - } - // get the object checkobj, err := registry.Get(testContext, podA.Name, &metav1.GetOptions{}) if err != nil { @@ -376,6 +368,261 @@ func TestStoreCreate(t *testing.T) { } } +func TestNewCreateOptionsFromUpdateOptions(t *testing.T) { + f := fuzz.New().NilChance(0.0).NumElements(1, 1) + + // The goal here is to trigger when any changes are made to either + // CreateOptions or UpdateOptions types, so we can update the converter. + for i := 0; i < 20; i++ { + in := &metav1.UpdateOptions{} + f.Fuzz(in) + in.TypeMeta.SetGroupVersionKind(metav1.SchemeGroupVersion.WithKind("CreateOptions")) + + out := newCreateOptionsFromUpdateOptions(in) + + // This sequence is intending to elide type information, but produce an + // intermediate structure (map) that can be manually patched up to make + // the comparison work as needed. + + // Convert both structs to maps of primitives. + inBytes, err := json.Marshal(in) + if err != nil { + t.Fatalf("failed to json.Marshal(in): %v", err) + } + outBytes, err := json.Marshal(out) + if err != nil { + t.Fatalf("failed to json.Marshal(out): %v", err) + } + inMap := map[string]interface{}{} + if err := json.Unmarshal(inBytes, &inMap); err != nil { + t.Fatalf("failed to json.Unmarshal(in): %v", err) + } + outMap := map[string]interface{}{} + if err := json.Unmarshal(outBytes, &outMap); err != nil { + t.Fatalf("failed to json.Unmarshal(out): %v", err) + } + + // Patch the maps to handle any expected differences before we compare + // - none for now. + + // Compare the results. + inBytes, err = json.Marshal(inMap) + if err != nil { + t.Fatalf("failed to json.Marshal(in): %v", err) + } + outBytes, err = json.Marshal(outMap) + if err != nil { + t.Fatalf("failed to json.Marshal(out): %v", err) + } + if i, o := string(inBytes), string(outBytes); i != o { + t.Fatalf("output != input:\n want: %s\n got: %s", i, o) + } + } +} + +func TestNewDeleteOptionsFromUpdateOptions(t *testing.T) { + f := fuzz.New().NilChance(0.0).NumElements(1, 1) + + // The goal here is to trigger when any changes are made to either + // DeleteOptions or UpdateOptions types, so we can update the converter. + for i := 0; i < 20; i++ { + in := &metav1.UpdateOptions{} + f.Fuzz(in) + in.TypeMeta.SetGroupVersionKind(metav1.SchemeGroupVersion.WithKind("DeleteOptions")) + + out := newDeleteOptionsFromUpdateOptions(in) + + // This sequence is intending to elide type information, but produce an + // intermediate structure (map) that can be manually patched up to make + // the comparison work as needed. + + // Convert both structs to maps of primitives. + inBytes, err := json.Marshal(in) + if err != nil { + t.Fatalf("failed to json.Marshal(in): %v", err) + } + outBytes, err := json.Marshal(out) + if err != nil { + t.Fatalf("failed to json.Marshal(out): %v", err) + } + inMap := map[string]interface{}{} + if err := json.Unmarshal(inBytes, &inMap); err != nil { + t.Fatalf("failed to json.Unmarshal(in): %v", err) + } + outMap := map[string]interface{}{} + if err := json.Unmarshal(outBytes, &outMap); err != nil { + t.Fatalf("failed to json.Unmarshal(out): %v", err) + } + + // Patch the maps to handle any expected differences before we compare. + + // DeleteOptions does not have these fields. + delete(inMap, "fieldManager") + + // UpdateOptions does not have these fields. + delete(outMap, "gracePeriodSeconds") + delete(outMap, "preconditions") + delete(outMap, "orphanDependents") + delete(outMap, "propagationPolicy") + + // Compare the results. + inBytes, err = json.Marshal(inMap) + if err != nil { + t.Fatalf("failed to json.Marshal(in): %v", err) + } + outBytes, err = json.Marshal(outMap) + if err != nil { + t.Fatalf("failed to json.Marshal(out): %v", err) + } + if i, o := string(inBytes), string(outBytes); i != o { + t.Fatalf("output != input:\n want: %s\n got: %s", i, o) + } + } +} + +func TestStoreCreateHooks(t *testing.T) { + // To track which hooks were called in what order. Not all hooks can + // mutate the object. + var milestones []string + + setAnn := func(obj runtime.Object, key string) { + pod := obj.(*example.Pod) + if pod.Annotations == nil { + pod.Annotations = make(map[string]string) + } + pod.Annotations[key] = "true" + } + mile := func(s string) { + milestones = append(milestones, s) + } + + testCases := []struct { + name string + decorator func(runtime.Object) + beginCreate BeginCreateFunc + afterCreate AfterCreateFunc + // the TTLFunc is an easy hook to force a failure + ttl func(obj runtime.Object, existing uint64, update bool) (uint64, error) + expectError bool + expectAnnotation string // to test object mutations + expectMilestones []string // to test sequence + }{{ + name: "no hooks", + }, { + name: "Decorator mutation", + decorator: func(obj runtime.Object) { + setAnn(obj, "DecoratorWasCalled") + }, + expectAnnotation: "DecoratorWasCalled", + }, { + name: "AfterCreate mutation", + afterCreate: func(obj runtime.Object, opts *metav1.CreateOptions) { + setAnn(obj, "AfterCreateWasCalled") + }, + expectAnnotation: "AfterCreateWasCalled", + }, { + name: "BeginCreate mutation", + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + setAnn(obj, "BeginCreateWasCalled") + return func(context.Context, bool) {}, nil + }, + expectAnnotation: "BeginCreateWasCalled", + }, { + name: "success ordering", + decorator: func(obj runtime.Object) { + mile("Decorator") + }, + afterCreate: func(obj runtime.Object, opts *metav1.CreateOptions) { + mile("AfterCreate") + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginCreate", "FinishCreate(true)", "AfterCreate", "Decorator"}, + }, { + name: "fail ordering", + decorator: func(obj runtime.Object) { + mile("Decorator") + }, + afterCreate: func(obj runtime.Object, opts *metav1.CreateOptions) { + mile("AfterCreate") + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + ttl: func(_ runtime.Object, existing uint64, _ bool) (uint64, error) { + mile("TTLError") + return existing, fmt.Errorf("TTL fail") + }, + expectMilestones: []string{"BeginCreate", "TTLError", "FinishCreate(false)"}, + expectError: true, + }, { + name: "fail BeginCreate ordering", + expectError: true, + decorator: func(obj runtime.Object) { + mile("Decorator") + }, + afterCreate: func(obj runtime.Object, opts *metav1.CreateOptions) { + mile("AfterCreate") + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, fmt.Errorf("begin") + }, + expectMilestones: []string{"BeginCreate"}, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + pod := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test"}, + Spec: example.PodSpec{NodeName: "machine"}, + } + + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() + registry.Decorator = tc.decorator + registry.BeginCreate = tc.beginCreate + registry.AfterCreate = tc.afterCreate + registry.TTLFunc = tc.ttl + + // create the object + milestones = nil + obj, err := registry.Create(testContext, pod, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if err != nil && !tc.expectError { + t.Fatalf("Unexpected error: %v", err) + } + if err == nil && tc.expectError { + t.Fatalf("Unexpected success") + } + + // verify the results + if tc.expectAnnotation != "" { + out := obj.(*example.Pod) + if v, found := out.Annotations[tc.expectAnnotation]; !found { + t.Errorf("Expected annotation %q not found", tc.expectAnnotation) + } else if v != "true" { + t.Errorf("Expected annotation %q has wrong value: %q", tc.expectAnnotation, v) + } + } + if tc.expectMilestones != nil { + if !reflect.DeepEqual(milestones, tc.expectMilestones) { + t.Errorf("Unexpected milestones: wanted %v, got %v", tc.expectMilestones, milestones) + } + } + }) + } +} + func isQualifiedResource(err error, kind, group string) bool { if err.(errors.APIStatus).Status().Details.Kind != kind || err.(errors.APIStatus).Status().Details.Group != group { return false @@ -531,6 +778,390 @@ func TestNoOpUpdates(t *testing.T) { } } +func TestStoreUpdateHooks(t *testing.T) { + // To track which hooks were called in what order. Not all hooks can + // mutate the object. + var milestones []string + + setAnn := func(obj runtime.Object, key string) { + pod := obj.(*example.Pod) + if pod.Annotations == nil { + pod.Annotations = make(map[string]string) + } + pod.Annotations[key] = "true" + } + mile := func(s string) { + milestones = append(milestones, s) + } + + testCases := []struct { + name string + decorator func(runtime.Object) + // create-on-update is tested elsewhere, but this proves non-use here + beginCreate BeginCreateFunc + afterCreate AfterCreateFunc + beginUpdate BeginUpdateFunc + afterUpdate AfterUpdateFunc + expectError bool + expectAnnotation string // to test object mutations + expectMilestones []string // to test sequence + }{{ + name: "no hooks", + }, { + name: "Decorator mutation", + decorator: func(obj runtime.Object) { + setAnn(obj, "DecoratorWasCalled") + }, + expectAnnotation: "DecoratorWasCalled", + }, { + name: "AfterUpdate mutation", + afterUpdate: func(obj runtime.Object, opts *metav1.UpdateOptions) { + setAnn(obj, "AfterUpdateWasCalled") + }, + expectAnnotation: "AfterUpdateWasCalled", + }, { + name: "BeginUpdate mutation", + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + setAnn(obj, "BeginUpdateWasCalled") + return func(context.Context, bool) {}, nil + }, + expectAnnotation: "BeginUpdateWasCalled", + }, { + name: "success ordering", + decorator: func(obj runtime.Object) { + mile("Decorator") + }, + afterCreate: func(obj runtime.Object, opts *metav1.CreateOptions) { + mile("AfterCreate") + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + afterUpdate: func(obj runtime.Object, opts *metav1.UpdateOptions) { + mile("AfterUpdate") + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginUpdate", "FinishUpdate(true)", "AfterUpdate", "Decorator"}, + }, /* fail ordering is covered in TestStoreUpdateHooksInnerRetry */ { + name: "fail BeginUpdate ordering", + expectError: true, + decorator: func(obj runtime.Object) { + mile("Decorator") + }, + afterUpdate: func(obj runtime.Object, opts *metav1.UpdateOptions) { + mile("AfterUpdate") + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, fmt.Errorf("begin") + }, + expectMilestones: []string{"BeginUpdate"}, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + pod := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test"}, + Spec: example.PodSpec{NodeName: "machine"}, + } + + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() + registry.BeginUpdate = tc.beginUpdate + registry.AfterUpdate = tc.afterUpdate + registry.BeginCreate = tc.beginCreate + registry.AfterCreate = tc.afterCreate + + _, err := registry.Create(testContext, pod, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + milestones = nil + registry.Decorator = tc.decorator + obj, _, err := registry.Update(testContext, pod.Name, rest.DefaultUpdatedObjectInfo(pod), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) + if err != nil && !tc.expectError { + t.Fatalf("Unexpected error: %v", err) + } + if err == nil && tc.expectError { + t.Fatalf("Unexpected success") + } + + // verify the results + if tc.expectAnnotation != "" { + out := obj.(*example.Pod) + if v, found := out.Annotations[tc.expectAnnotation]; !found { + t.Errorf("Expected annotation %q not found", tc.expectAnnotation) + } else if v != "true" { + t.Errorf("Expected annotation %q has wrong value: %q", tc.expectAnnotation, v) + } + } + if tc.expectMilestones != nil { + if !reflect.DeepEqual(milestones, tc.expectMilestones) { + t.Errorf("Unexpected milestones: wanted %v, got %v", tc.expectMilestones, milestones) + } + } + }) + } +} + +func TestStoreCreateOnUpdateHooks(t *testing.T) { + // To track which hooks were called in what order. Not all hooks can + // mutate the object. + var milestones []string + + mile := func(s string) { + milestones = append(milestones, s) + } + + testCases := []struct { + name string + decorator func(runtime.Object) + beginCreate BeginCreateFunc + afterCreate AfterCreateFunc + beginUpdate BeginUpdateFunc + afterUpdate AfterUpdateFunc + // the TTLFunc is an easy hook to force a failure + ttl func(obj runtime.Object, existing uint64, update bool) (uint64, error) + expectError bool + expectMilestones []string // to test sequence + }{{ + name: "no hooks", + }, { + name: "success ordering", + decorator: func(obj runtime.Object) { + mile("Decorator") + }, + afterCreate: func(obj runtime.Object, opts *metav1.CreateOptions) { + mile("AfterCreate") + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + afterUpdate: func(obj runtime.Object, opts *metav1.UpdateOptions) { + mile("AfterUpdate") + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginCreate", "FinishCreate(true)", "AfterCreate", "Decorator"}, + }, { + name: "fail ordering", + decorator: func(obj runtime.Object) { + mile("Decorator") + }, + afterCreate: func(obj runtime.Object, opts *metav1.CreateOptions) { + mile("AfterCreate") + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, nil + }, + afterUpdate: func(obj runtime.Object, opts *metav1.UpdateOptions) { + mile("AfterUpdate") + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + ttl: func(_ runtime.Object, existing uint64, _ bool) (uint64, error) { + mile("TTLError") + return existing, fmt.Errorf("TTL fail") + }, + expectMilestones: []string{"BeginCreate", "TTLError", "FinishCreate(false)"}, + expectError: true, + }, { + name: "fail BeginCreate ordering", + expectError: true, + decorator: func(obj runtime.Object) { + mile("Decorator") + }, + afterCreate: func(obj runtime.Object, opts *metav1.CreateOptions) { + mile("AfterCreate") + }, + beginCreate: func(_ context.Context, obj runtime.Object, _ *metav1.CreateOptions) (FinishFunc, error) { + mile("BeginCreate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishCreate(%v)", success)) + }, fmt.Errorf("begin") + }, + afterUpdate: func(obj runtime.Object, opts *metav1.UpdateOptions) { + mile("AfterUpdate") + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + expectMilestones: []string{"BeginCreate"}, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + pod := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test"}, + Spec: example.PodSpec{NodeName: "machine"}, + } + + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() + registry.Decorator = tc.decorator + registry.UpdateStrategy.(*testRESTStrategy).allowCreateOnUpdate = true + registry.BeginUpdate = tc.beginUpdate + registry.AfterUpdate = tc.afterUpdate + registry.BeginCreate = tc.beginCreate + registry.AfterCreate = tc.afterCreate + registry.TTLFunc = tc.ttl + + // NB: did not create it first. + milestones = nil + _, _, err := registry.Update(testContext, pod.Name, rest.DefaultUpdatedObjectInfo(pod), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) + if err != nil && !tc.expectError { + t.Fatalf("Unexpected error: %v", err) + } + if err == nil && tc.expectError { + t.Fatalf("Unexpected success") + } + + // verify the results + if tc.expectMilestones != nil { + if !reflect.DeepEqual(milestones, tc.expectMilestones) { + t.Errorf("Unexpected milestones: wanted %v, got %v", tc.expectMilestones, milestones) + } + } + }) + } +} + +func TestStoreUpdateHooksInnerRetry(t *testing.T) { + // To track which hooks were called in what order. Not all hooks can + // mutate the object. + var milestones []string + + mile := func(s string) { + milestones = append(milestones, s) + } + ttlFailDone := false + ttlFailOnce := func(_ runtime.Object, existing uint64, _ bool) (uint64, error) { + if ttlFailDone { + mile("TTL") + return existing, nil + } + ttlFailDone = true + mile("TTLError") + return existing, fmt.Errorf("TTL fail") + } + ttlFailAlways := func(_ runtime.Object, existing uint64, _ bool) (uint64, error) { + mile("TTLError") + return existing, fmt.Errorf("TTL fail") + } + + testCases := []struct { + name string + decorator func(runtime.Object) + beginUpdate func(context.Context, runtime.Object, runtime.Object, *metav1.UpdateOptions) (FinishFunc, error) + afterUpdate AfterUpdateFunc + // the TTLFunc is an easy hook to force an inner-loop retry + ttl func(obj runtime.Object, existing uint64, update bool) (uint64, error) + expectError bool + expectMilestones []string // to test sequence + }{{ + name: "inner retry success", + decorator: func(obj runtime.Object) { + mile("Decorator") + }, + afterUpdate: func(obj runtime.Object, opts *metav1.UpdateOptions) { + mile("AfterUpdate") + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + ttl: ttlFailOnce, + expectMilestones: []string{"BeginUpdate", "TTLError", "FinishUpdate(false)", "BeginUpdate", "TTL", "FinishUpdate(true)", "AfterUpdate", "Decorator"}, + }, { + name: "inner retry fail", + decorator: func(obj runtime.Object) { + mile("Decorator") + }, + afterUpdate: func(obj runtime.Object, opts *metav1.UpdateOptions) { + mile("AfterUpdate") + }, + beginUpdate: func(_ context.Context, obj, _ runtime.Object, _ *metav1.UpdateOptions) (FinishFunc, error) { + mile("BeginUpdate") + return func(_ context.Context, success bool) { + mile(fmt.Sprintf("FinishUpdate(%v)", success)) + }, nil + }, + ttl: ttlFailAlways, + expectError: true, + expectMilestones: []string{"BeginUpdate", "TTLError", "FinishUpdate(false)", "BeginUpdate", "TTLError", "FinishUpdate(false)"}, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + pod := &example.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test"}, + Spec: example.PodSpec{NodeName: "machine"}, + } + + testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") + destroyFunc, registry := NewTestGenericStoreRegistry(t) + defer destroyFunc() + registry.BeginUpdate = tc.beginUpdate + registry.AfterUpdate = tc.afterUpdate + + created, err := registry.Create(testContext, pod, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + milestones = nil + registry.Decorator = tc.decorator + ttlFailDone = false + registry.TTLFunc = tc.ttl + registry.Storage.Storage = &staleGuaranteedUpdateStorage{Interface: registry.Storage.Storage, cachedObj: created} + _, _, err = registry.Update(testContext, pod.Name, rest.DefaultUpdatedObjectInfo(pod), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}) + if err != nil && !tc.expectError { + t.Fatalf("Unexpected error: %v", err) + } + if err == nil && tc.expectError { + t.Fatalf("Unexpected success") + } + + // verify the results + if tc.expectMilestones != nil { + if !reflect.DeepEqual(milestones, tc.expectMilestones) { + t.Errorf("Unexpected milestones: wanted %v, got %v", tc.expectMilestones, milestones) + } + } + }) + } +} + // TODO: Add a test to check no-op update if we have object with ResourceVersion // already stored in etcd. Currently there is no easy way to store object with // ResourceVersion in etcd. @@ -660,11 +1291,19 @@ func TestStoreDelete(t *testing.T) { destroyFunc, registry := NewTestGenericStoreRegistry(t) defer destroyFunc() + afterWasCalled := false + registry.AfterDelete = func(obj runtime.Object, options *metav1.DeleteOptions) { + afterWasCalled = true + } + // test failure condition _, _, err := registry.Delete(testContext, podA.Name, rest.ValidateAllObjectFunc, nil) if !errors.IsNotFound(err) { t.Errorf("Unexpected error: %v", err) } + if afterWasCalled { + t.Errorf("Unexpected call to AfterDelete") + } // create pod _, err = registry.Create(testContext, podA, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) @@ -680,6 +1319,9 @@ func TestStoreDelete(t *testing.T) { if !wasDeleted { t.Errorf("unexpected, pod %s should have been deleted immediately", podA.Name) } + if !afterWasCalled { + t.Errorf("Expected call to AfterDelete, but got none") + } // try to get a item which should be deleted _, err = registry.Get(testContext, podA.Name, &metav1.GetOptions{}) @@ -795,11 +1437,18 @@ func TestGracefulStoreHandleFinalizers(t *testing.T) { registry.DeleteStrategy = testGracefulStrategy{defaultDeleteStrategy} defer destroyFunc() + afterWasCalled := false + registry.AfterDelete = func(obj runtime.Object, options *metav1.DeleteOptions) { + afterWasCalled = true + } + gcStates := []bool{true, false} for _, gcEnabled := range gcStates { t.Logf("garbage collection enabled: %t", gcEnabled) registry.EnableGarbageCollection = gcEnabled + afterWasCalled = false // reset + // create pod _, err := registry.Create(testContext, podWithFinalizer, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) if err != nil { @@ -814,6 +1463,9 @@ func TestGracefulStoreHandleFinalizers(t *testing.T) { if wasDeleted { t.Errorf("unexpected, pod %s should not have been deleted immediately", podWithFinalizer.Name) } + if afterWasCalled { + t.Errorf("unexpected, AfterDelete() was called") + } _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) if err != nil { t.Fatalf("Unexpected error: %v", err) @@ -827,6 +1479,9 @@ func TestGracefulStoreHandleFinalizers(t *testing.T) { if err != nil { t.Fatalf("Unexpected error: %v", err) } + if afterWasCalled { + t.Errorf("unexpected, AfterDelete() was called") + } // the object should still exist, because it still has a finalizer _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) @@ -842,6 +1497,9 @@ func TestGracefulStoreHandleFinalizers(t *testing.T) { if err != nil { t.Fatalf("Unexpected error: %v", err) } + if !afterWasCalled { + t.Errorf("unexpected, AfterDelete() was not called") + } // the pod should be removed, because its finalizer is removed _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) if !errors.IsNotFound(err) { @@ -859,13 +1517,20 @@ func TestNonGracefulStoreHandleFinalizers(t *testing.T) { testContext := genericapirequest.WithNamespace(genericapirequest.NewContext(), "test") destroyFunc, registry := NewTestGenericStoreRegistry(t) - defer destroyFunc() + + afterWasCalled := false + registry.AfterDelete = func(obj runtime.Object, options *metav1.DeleteOptions) { + afterWasCalled = true + } + gcStates := []bool{true, false} for _, gcEnabled := range gcStates { t.Logf("garbage collection enabled: %t", gcEnabled) registry.EnableGarbageCollection = gcEnabled + afterWasCalled = false // reset + // create pod _, err := registry.Create(testContext, podWithFinalizer, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) if err != nil { @@ -880,6 +1545,9 @@ func TestNonGracefulStoreHandleFinalizers(t *testing.T) { if wasDeleted { t.Errorf("unexpected, pod %s should not have been deleted immediately", podWithFinalizer.Name) } + if afterWasCalled { + t.Errorf("unexpected, AfterDelete() was called") + } // the object should still exist obj, err := registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) @@ -908,6 +1576,9 @@ func TestNonGracefulStoreHandleFinalizers(t *testing.T) { if err != nil { t.Errorf("Unexpected error: %v", err) } + if afterWasCalled { + t.Errorf("unexpected, AfterDelete() was called") + } // the object should still exist, because it still has a finalizer obj, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) @@ -927,6 +1598,9 @@ func TestNonGracefulStoreHandleFinalizers(t *testing.T) { if err != nil { t.Errorf("Unexpected error: %v", err) } + if !afterWasCalled { + t.Errorf("unexpected, AfterDelete() was not called") + } // the pod should be removed, because its finalizer is removed _, err = registry.Get(testContext, podWithFinalizer.Name, &metav1.GetOptions{}) if !errors.IsNotFound(err) { @@ -1677,7 +2351,7 @@ func TestFinalizeDelete(t *testing.T) { obj := &example.Pod{ ObjectMeta: metav1.ObjectMeta{Name: "foo", UID: "random-uid"}, } - result, err := s.finalizeDelete(genericapirequest.NewContext(), obj, false) + result, err := s.finalizeDelete(genericapirequest.NewContext(), obj, false, &metav1.DeleteOptions{}) if err != nil { t.Fatalf("unexpected err: %s", err) } diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go index 8f9c981dd85..aca8da8b4a0 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/rest.go @@ -214,7 +214,7 @@ type UpdatedObjectInfo interface { } // ValidateObjectFunc is a function to act on a given object. An error may be returned -// if the hook cannot be completed. An ObjectFunc may NOT transform the provided +// if the hook cannot be completed. A ValidateObjectFunc may NOT transform the provided // object. type ValidateObjectFunc func(ctx context.Context, obj runtime.Object) error