From 625713008d8897670092a125f26d368e96b7268f 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 --- pkg/registry/core/service/storage/storage.go | 43 +++----- .../core/service/storage/storage_test.go | 20 +--- .../generic/registry/decorated_watcher.go | 11 +- .../registry/decorated_watcher_test.go | 24 +--- .../pkg/registry/generic/registry/store.go | 22 +--- .../registry/generic/registry/store_test.go | 104 +++--------------- 6 files changed, 49 insertions(+), 175 deletions(-) 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/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 26c20d1597e..366a4472d5d 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 @@ -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/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 98a31373122..83a59dfe8a7 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 @@ -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")