Make REST Decorator funcs not return error

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 {
+func (r *GenericREST) defaultOnRead(obj runtime.Object) {
-	service, ok := obj.(*api.Service)
+	switch s := obj.(type) {
-	if ok {
+	case *api.Service:
-		return r.defaultOnReadService(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])
+		r.defaultOnReadService(&serviceList.Items[i])
-		if err != nil {
-			return err
-		}
 	}
-
-	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
 }

pkg/registry/core/service/storage/storage_test.go

@@ -358,10 +358,9 @@ func TestServiceDefaultOnRead(t *testing.T) {
 	}
 
 	testCases := []struct {
-		name      string
+		name   string
-		input     runtime.Object
+		input  runtime.Object
-		expectErr bool
+		expect 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",
+		name:  "not Service or ServiceList",
-		input:     &api.Pod{},
+		input: &api.Pod{},
-		expectErr: false,
 	}}
 
 	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)
+			storage.defaultOnRead(tmp)
-			if err != nil && !tc.expectErr {
-				t.Errorf("unexpected error: %v", err)
-			}
-			if err == nil && tc.expectErr {
-				t.Errorf("unexpected success")
-			}
 
 			svc, ok := tmp.(*api.Service)
 			if !ok {

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)
+				d.decorator(recv.Object)
-				if err != nil {
-					send = makeStatusErrorEvent(err)
-					break
-				}
 				send = recv
 			case watch.Error:
 				send = recv

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)
-	}
-}

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 {
+		e.Decorator(out)
-			return nil, err
-		}
 	}
 	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 {
+		e.Decorator(out)
-			return nil, err
-		}
 	}
 	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 {
+		e.Decorator(out)
-			return nil, false, err
-		}
 	}
 	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 {
+		e.Decorator(obj)
-			return nil, err
-		}
 	}
 	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 {
+			e.Decorator(obj)
-				return nil, err
-			}
 		}
 		return obj, nil
 	}

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")