From f7235937740cc101b629dd05199a29c6411475f1 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Tue, 10 Nov 2020 15:07:05 -0800 Subject: [PATCH] Make REST Decorator funcs not return error Kubernetes-commit: 625713008d8897670092a125f26d368e96b7268f --- .../generic/registry/decorated_watcher.go | 11 +- .../registry/decorated_watcher_test.go | 24 +--- pkg/registry/generic/registry/store.go | 22 +--- pkg/registry/generic/registry/store_test.go | 104 +++--------------- 4 files changed, 27 insertions(+), 134 deletions(-) diff --git a/pkg/registry/generic/registry/decorated_watcher.go b/pkg/registry/generic/registry/decorated_watcher.go index 005a376d4..034bf12c9 100644 --- a/pkg/registry/generic/registry/decorated_watcher.go +++ b/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/pkg/registry/generic/registry/decorated_watcher_test.go b/pkg/registry/generic/registry/decorated_watcher_test.go index 0afbd773f..33e47c8af 100644 --- a/pkg/registry/generic/registry/decorated_watcher_test.go +++ b/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/pkg/registry/generic/registry/store.go b/pkg/registry/generic/registry/store.go index 26c20d159..366a4472d 100644 --- a/pkg/registry/generic/registry/store.go +++ b/pkg/registry/generic/registry/store.go @@ -148,7 +148,7 @@ 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 @@ -322,9 +322,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 } @@ -425,9 +423,7 @@ func (e *Store) Create(ctx context.Context, obj runtime.Object, createValidation e.AfterCreate(out) } if e.Decorator != nil { - if err := e.Decorator(out); err != nil { - return nil, err - } + e.Decorator(out) } return out, nil } @@ -672,9 +668,7 @@ func (e *Store) Update(ctx context.Context, name string, objInfo rest.UpdatedObj } } if e.Decorator != nil { - if err := e.Decorator(out); err != nil { - return nil, false, err - } + e.Decorator(out) } return out, creating, nil } @@ -701,9 +695,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 } @@ -1163,9 +1155,7 @@ func (e *Store) finalizeDelete(ctx context.Context, obj runtime.Object, runHooks } 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/pkg/registry/generic/registry/store_test.go b/pkg/registry/generic/registry/store_test.go index 98a313731..83a59dfe8 100644 --- a/pkg/registry/generic/registry/store_test.go +++ b/pkg/registry/generic/registry/store_test.go @@ -438,7 +438,7 @@ func TestStoreCreateHooks(t *testing.T) { testCases := []struct { name string - decorator ObjectFunc + decorator func(runtime.Object) beginCreate func(context.Context, runtime.Object, *metav1.CreateOptions) (FinishFunc, error) afterCreate func(runtime.Object) // the TTLFunc is an easy hook to force a failure @@ -450,9 +450,8 @@ func TestStoreCreateHooks(t *testing.T) { name: "no hooks", }, { name: "Decorator mutation", - decorator: func(obj runtime.Object) error { + decorator: func(obj runtime.Object) { setAnn(obj, "DecoratorWasCalled") - return nil }, expectAnnotation: "DecoratorWasCalled", }, { @@ -470,9 +469,8 @@ func TestStoreCreateHooks(t *testing.T) { expectAnnotation: "BeginCreateWasCalled", }, { name: "success ordering", - decorator: func(obj runtime.Object) error { + decorator: func(obj runtime.Object) { mile("Decorator") - return nil }, afterCreate: func(obj runtime.Object) { mile("AfterCreate") @@ -486,9 +484,8 @@ func TestStoreCreateHooks(t *testing.T) { expectMilestones: []string{"BeginCreate", "FinishCreate(true)", "AfterCreate", "Decorator"}, }, { name: "fail ordering", - decorator: func(obj runtime.Object) error { + decorator: func(obj runtime.Object) { mile("Decorator") - return nil }, afterCreate: func(obj runtime.Object) { mile("AfterCreate") @@ -505,29 +502,11 @@ func TestStoreCreateHooks(t *testing.T) { }, expectMilestones: []string{"BeginCreate", "TTLError", "FinishCreate(false)"}, expectError: true, - }, { - name: "fail Decorator ordering", - expectError: true, - decorator: func(obj runtime.Object) error { - mile("Decorator") - return fmt.Errorf("decorator") - }, - afterCreate: func(obj runtime.Object) { - 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 BeginCreate ordering", expectError: true, - decorator: func(obj runtime.Object) error { + decorator: func(obj runtime.Object) { mile("Decorator") - return nil }, afterCreate: func(obj runtime.Object) { mile("AfterCreate") @@ -757,7 +736,7 @@ func TestStoreUpdateHooks(t *testing.T) { testCases := []struct { name string - decorator ObjectFunc + decorator func(runtime.Object) // create-on-update is tested elsewhere, but this proves non-use here beginCreate func(context.Context, runtime.Object, *metav1.CreateOptions) (FinishFunc, error) afterCreate func(runtime.Object) @@ -770,9 +749,8 @@ func TestStoreUpdateHooks(t *testing.T) { name: "no hooks", }, { name: "Decorator mutation", - decorator: func(obj runtime.Object) error { + decorator: func(obj runtime.Object) { setAnn(obj, "DecoratorWasCalled") - return nil }, expectAnnotation: "DecoratorWasCalled", }, { @@ -790,9 +768,8 @@ func TestStoreUpdateHooks(t *testing.T) { expectAnnotation: "BeginUpdateWasCalled", }, { name: "success ordering", - decorator: func(obj runtime.Object) error { + decorator: func(obj runtime.Object) { mile("Decorator") - return nil }, afterCreate: func(obj runtime.Object) { mile("AfterCreate") @@ -814,28 +791,10 @@ func TestStoreUpdateHooks(t *testing.T) { }, expectMilestones: []string{"BeginUpdate", "FinishUpdate(true)", "AfterUpdate", "Decorator"}, }, /* fail ordering is covered in TestStoreUpdateHooksInnerRetry */ { - name: "fail Decorator ordering", - expectError: true, - decorator: func(obj runtime.Object) error { - mile("Decorator") - return fmt.Errorf("decorator") - }, - afterUpdate: func(obj runtime.Object) { - 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"}, - }, { name: "fail BeginUpdate ordering", expectError: true, - decorator: func(obj runtime.Object) error { + decorator: func(obj runtime.Object) { mile("Decorator") - return nil }, afterUpdate: func(obj runtime.Object) { mile("AfterUpdate") @@ -907,7 +866,7 @@ func TestStoreCreateOnUpdateHooks(t *testing.T) { testCases := []struct { name string - decorator ObjectFunc + decorator func(runtime.Object) beginCreate func(context.Context, runtime.Object, *metav1.CreateOptions) (FinishFunc, error) afterCreate func(runtime.Object) beginUpdate func(context.Context, runtime.Object, runtime.Object, *metav1.UpdateOptions) (FinishFunc, error) @@ -920,9 +879,8 @@ func TestStoreCreateOnUpdateHooks(t *testing.T) { name: "no hooks", }, { name: "success ordering", - decorator: func(obj runtime.Object) error { + decorator: func(obj runtime.Object) { mile("Decorator") - return nil }, afterCreate: func(obj runtime.Object) { mile("AfterCreate") @@ -945,9 +903,8 @@ func TestStoreCreateOnUpdateHooks(t *testing.T) { expectMilestones: []string{"BeginCreate", "FinishCreate(true)", "AfterCreate", "Decorator"}, }, { name: "fail ordering", - decorator: func(obj runtime.Object) error { + decorator: func(obj runtime.Object) { mile("Decorator") - return nil }, afterCreate: func(obj runtime.Object) { mile("AfterCreate") @@ -973,38 +930,11 @@ func TestStoreCreateOnUpdateHooks(t *testing.T) { }, expectMilestones: []string{"BeginCreate", "TTLError", "FinishCreate(false)"}, expectError: true, - }, { - name: "fail Decorator ordering", - expectError: true, - decorator: func(obj runtime.Object) error { - mile("Decorator") - return fmt.Errorf("decorator") - }, - afterCreate: func(obj runtime.Object) { - 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) { - 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 BeginCreate ordering", expectError: true, - decorator: func(obj runtime.Object) error { + decorator: func(obj runtime.Object) { mile("Decorator") - return nil }, afterCreate: func(obj runtime.Object) { mile("AfterCreate") @@ -1090,7 +1020,7 @@ func TestStoreUpdateHooksInnerRetry(t *testing.T) { testCases := []struct { name string - decorator func(runtime.Object) error + decorator func(runtime.Object) beginUpdate func(context.Context, runtime.Object, runtime.Object, *metav1.UpdateOptions) (FinishFunc, error) afterUpdate func(runtime.Object) // the TTLFunc is an easy hook to force an inner-loop retry @@ -1099,9 +1029,8 @@ func TestStoreUpdateHooksInnerRetry(t *testing.T) { expectMilestones []string // to test sequence }{{ name: "inner retry success", - decorator: func(obj runtime.Object) error { + decorator: func(obj runtime.Object) { mile("Decorator") - return nil }, afterUpdate: func(obj runtime.Object) { mile("AfterUpdate") @@ -1116,9 +1045,8 @@ func TestStoreUpdateHooksInnerRetry(t *testing.T) { expectMilestones: []string{"BeginUpdate", "TTLError", "FinishUpdate(false)", "BeginUpdate", "TTL", "FinishUpdate(true)", "AfterUpdate", "Decorator"}, }, { name: "inner retry fail", - decorator: func(obj runtime.Object) error { + decorator: func(obj runtime.Object) { mile("Decorator") - return nil }, afterUpdate: func(obj runtime.Object) { mile("AfterUpdate")