Defaulting Controller options for all kind of webhooks (#2738)

* functional options

* move options to its own package

Signed-off-by: Hector Fernandez <hector@chainguard.dev>

* add controller options to the webhook options

Signed-off-by: Hector Fernandez <hector@chainguard.dev>

* create custom options funcs for each webhook type

Signed-off-by: Hector Fernandez <hector@chainguard.dev>

* address comments from reviewers

Signed-off-by: Hector Fernandez <hector@chainguard.dev>

---------

Signed-off-by: Hector Fernandez <hector@chainguard.dev>
Co-authored-by: dprotaso <dprotaso@gmail.com>

webhook/resourcesemantics/conversion/controller.go

@@ -90,16 +90,32 @@ func NewConversionController(
 	withContext func(context.Context) context.Context,
 ) *controller.Impl {
 
+	opts := []OptionFunc{
+		WithPath(path),
+		WithWrapContext(withContext),
+		WithKinds(kinds),
+	}
+
+	return newController(ctx, opts...)
+}
+
+func newController(ctx context.Context, optsFunc ...OptionFunc) *controller.Impl {
 	secretInformer := secretinformer.Get(ctx)
 	crdInformer := crdinformer.Get(ctx)
 	client := apixclient.Get(ctx)
-	options := webhook.GetOptions(ctx)
+	woptions := webhook.GetOptions(ctx)
+
+	opts := &options{}
+
+	for _, f := range optsFunc {
+		f(opts)
+	}
 
 	r := &reconciler{
 		LeaderAwareFuncs: pkgreconciler.LeaderAwareFuncs{
 			// Have this reconciler enqueue our types whenever it becomes leader.
 			PromoteFunc: func(bkt pkgreconciler.Bucket, enq func(pkgreconciler.Bucket, types.NamespacedName)) error {
-				for _, gkc := range kinds {
+				for _, gkc := range opts.kinds {
 					name := gkc.DefinitionName
 					enq(bkt, types.NamespacedName{Name: name})
 				}
@@ -107,22 +123,26 @@ func NewConversionController(
 			},
 		},
 
-		kinds:       kinds,
+		kinds:       opts.kinds,
-		path:        path,
+		path:        opts.path,
-		secretName:  options.SecretName,
+		secretName:  woptions.SecretName,
-		withContext: withContext,
+		withContext: opts.wc,
 
 		client:       client,
 		secretLister: secretInformer.Lister(),
 		crdLister:    crdInformer.Lister(),
 	}
 
-	const queueName = "ConversionWebhook"
 	logger := logging.FromContext(ctx)
-	c := controller.NewContext(ctx, r, controller.ControllerOptions{WorkQueueName: queueName, Logger: logger.Named(queueName)})
+	controllerOptions := woptions.ControllerOptions
+	if controllerOptions == nil {
+		const queueName = "ConversionWebhook"
+		controllerOptions = &controller.ControllerOptions{WorkQueueName: queueName, Logger: logger.Named(queueName)}
+	}
+	c := controller.NewContext(ctx, r, *controllerOptions)
 
 	// Reconciler when the named CRDs change.
-	for _, gkc := range kinds {
+	for _, gkc := range opts.kinds {
 		name := gkc.DefinitionName
 
 		crdInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
@@ -134,7 +154,7 @@ func NewConversionController(
 
 		// Reconcile when the cert bundle changes.
 		secretInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
-			FilterFunc: controller.FilterWithNameAndNamespace(system.Namespace(), options.SecretName),
+			FilterFunc: controller.FilterWithNameAndNamespace(system.Namespace(), woptions.SecretName),
 			Handler:    controller.HandleAll(sentinel),
 		})
 	}

webhook/resourcesemantics/conversion/options.go

@@ -0,0 +1,49 @@
+/*
+Copyright 2023 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package conversion
+
+import (
+	"context"
+
+	"k8s.io/apimachinery/pkg/runtime/schema"
+)
+
+type options struct {
+	path  string
+	wc    func(context.Context) context.Context
+	kinds map[schema.GroupKind]GroupKindConversion
+}
+
+type OptionFunc func(*options)
+
+func WithKinds(kinds map[schema.GroupKind]GroupKindConversion) OptionFunc {
+	return func(o *options) {
+		o.kinds = kinds
+	}
+}
+
+func WithPath(path string) OptionFunc {
+	return func(o *options) {
+		o.path = path
+	}
+}
+
+func WithWrapContext(f func(context.Context) context.Context) OptionFunc {
+	return func(o *options) {
+		o.wc = f
+	}
+}

webhook/resourcesemantics/conversion/options_test.go

@@ -0,0 +1,37 @@
+/*
+Copyright 2023 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+package conversion
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestOptions(t *testing.T) {
+	got := &options{}
+	WithPath("path")(got)
+
+	want := &options{
+		path: "path",
+		// we can't compare wc as functions are not
+		// comparable in golang (thus it needs to be
+		// done indirectly)
+	}
+
+	if !reflect.DeepEqual(got, want) {
+		t.Error("option was not applied")
+	}
+}

webhook/resourcesemantics/defaulting/controller.go

@@ -46,13 +46,6 @@ func NewAdmissionController(
 	callbacks ...map[schema.GroupVersionKind]Callback,
 ) *controller.Impl {
 
-	client := kubeclient.Get(ctx)
-	mwhInformer := mwhinformer.Get(ctx)
-	secretInformer := secretinformer.Get(ctx)
-	options := webhook.GetOptions(ctx)
-
-	key := types.NamespacedName{Name: name}
-
 	// This not ideal, we are using a variadic argument to effectively make callbacks optional
 	// This allows this addition to be non-breaking to consumers of /pkg
 	// TODO: once all sub-repos have adopted this, we might move this back to a traditional param.
@@ -66,6 +59,34 @@ func NewAdmissionController(
 		panic("NewAdmissionController may not be called with multiple callback maps")
 	}
 
+	opts := []OptionFunc{
+		WithPath(path),
+		WithTypes(handlers),
+		WithWrapContext(wc),
+		WithCallbacks(unwrappedCallbacks),
+	}
+
+	if disallowUnknownFields {
+		opts = append(opts, WithDisallowUnknownFields())
+	}
+
+	return newController(ctx, name, opts...)
+}
+
+func newController(ctx context.Context, name string, optsFunc ...OptionFunc) *controller.Impl {
+	client := kubeclient.Get(ctx)
+	mwhInformer := mwhinformer.Get(ctx)
+	secretInformer := secretinformer.Get(ctx)
+
+	opts := &options{}
+	wopts := webhook.GetOptions(ctx)
+
+	for _, f := range optsFunc {
+		f(opts)
+	}
+
+	key := types.NamespacedName{Name: name}
+
 	wh := &reconciler{
 		LeaderAwareFuncs: pkgreconciler.LeaderAwareFuncs{
 			// Have this reconciler enqueue our singleton whenever it becomes leader.
@@ -76,13 +97,13 @@ func NewAdmissionController(
 		},
 
 		key:       key,
-		path:      path,
+		path:      opts.path,
-		handlers:  handlers,
+		handlers:  opts.types,
-		callbacks: unwrappedCallbacks,
+		callbacks: opts.callbacks,
 
-		withContext:           wc,
+		withContext:           opts.wc,
-		disallowUnknownFields: disallowUnknownFields,
+		disallowUnknownFields: opts.disallowUnknownFields,
-		secretName:            options.SecretName,
+		secretName:            wopts.SecretName,
 
 		client:       client,
 		mwhlister:    mwhInformer.Lister(),
@@ -90,8 +111,12 @@ func NewAdmissionController(
 	}
 
 	logger := logging.FromContext(ctx)
+	controllerOptions := wopts.ControllerOptions
+	if controllerOptions == nil {
 		const queueName = "DefaultingWebhook"
-	c := controller.NewContext(ctx, wh, controller.ControllerOptions{WorkQueueName: queueName, Logger: logger.Named(queueName)})
+		controllerOptions = &controller.ControllerOptions{WorkQueueName: queueName, Logger: logger.Named(queueName)}
+	}
+	c := controller.NewContext(ctx, wh, *controllerOptions)
 
 	// Reconcile when the named MutatingWebhookConfiguration changes.
 	mwhInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{

webhook/resourcesemantics/defaulting/options.go

@@ -0,0 +1,64 @@
+/*
+Copyright 2023 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package defaulting
+
+import (
+	"context"
+
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"knative.dev/pkg/webhook/resourcesemantics"
+)
+
+type options struct {
+	path                  string
+	types                 map[schema.GroupVersionKind]resourcesemantics.GenericCRD
+	wc                    func(context.Context) context.Context
+	disallowUnknownFields bool
+	callbacks             map[schema.GroupVersionKind]Callback
+}
+
+type OptionFunc func(*options)
+
+func WithCallbacks(callbacks map[schema.GroupVersionKind]Callback) OptionFunc {
+	return func(o *options) {
+		o.callbacks = callbacks
+	}
+}
+
+func WithPath(path string) OptionFunc {
+	return func(o *options) {
+		o.path = path
+	}
+}
+
+func WithTypes(types map[schema.GroupVersionKind]resourcesemantics.GenericCRD) OptionFunc {
+	return func(o *options) {
+		o.types = types
+	}
+}
+
+func WithWrapContext(f func(context.Context) context.Context) OptionFunc {
+	return func(o *options) {
+		o.wc = f
+	}
+}
+
+func WithDisallowUnknownFields() OptionFunc {
+	return func(o *options) {
+		o.disallowUnknownFields = true
+	}
+}

webhook/resourcesemantics/defaulting/options_test.go

@@ -0,0 +1,50 @@
+/*
+Copyright 2023 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package defaulting
+
+import (
+	"reflect"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"knative.dev/pkg/webhook/resourcesemantics"
+)
+
+func TestOptions(t *testing.T) {
+	callbacks := map[schema.GroupVersionKind]Callback{}
+	types := map[schema.GroupVersionKind]resourcesemantics.GenericCRD{}
+
+	got := &options{}
+	WithCallbacks(callbacks)(got)
+	WithDisallowUnknownFields()(got)
+	WithPath("path")(got)
+	WithTypes(types)(got)
+
+	want := &options{
+		callbacks:             callbacks,
+		disallowUnknownFields: true,
+		path:                  "path",
+		types:                 types,
+		// we can't compare wc as functions are not
+		// comparable in golang (thus it needs to be
+		// done indirectly)
+	}
+
+	if !reflect.DeepEqual(got, want) {
+		t.Error("option was not applied")
+	}
+}

webhook/resourcesemantics/validation/controller.go

@@ -45,10 +45,31 @@ func NewAdmissionControllerWithConfig(
 	disallowUnknownFields bool,
 	callbacks map[schema.GroupVersionKind]Callback,
 ) *controller.Impl {
+
+	opts := []OptionFunc{
+		WithPath(path),
+		WithTypes(handlers),
+		WithWrapContext(wc),
+		WithCallbacks(callbacks),
+	}
+
+	if disallowUnknownFields {
+		opts = append(opts, WithDisallowUnknownFields())
+	}
+	return newController(ctx, name, opts...)
+}
+
+func newController(ctx context.Context, name string, optsFunc ...OptionFunc) *controller.Impl {
 	client := kubeclient.Get(ctx)
 	vwhInformer := vwhinformer.Get(ctx)
 	secretInformer := secretinformer.Get(ctx)
-	options := webhook.GetOptions(ctx)
+	woptions := webhook.GetOptions(ctx)
+
+	opts := &options{}
+
+	for _, f := range optsFunc {
+		f(opts)
+	}
 
 	wh := &reconciler{
 		LeaderAwareFuncs: pkgreconciler.LeaderAwareFuncs{
@@ -62,13 +83,13 @@ func NewAdmissionControllerWithConfig(
 		key: types.NamespacedName{
 			Name: name,
 		},
-		path:      path,
+		path:      opts.path,
-		handlers:  handlers,
+		handlers:  opts.types,
-		callbacks: callbacks,
+		callbacks: opts.callbacks,
 
-		withContext:           wc,
+		withContext:           opts.wc,
-		disallowUnknownFields: disallowUnknownFields,
+		disallowUnknownFields: opts.DisallowUnknownFields(),
-		secretName:            options.SecretName,
+		secretName:            woptions.SecretName,
 
 		client:       client,
 		vwhlister:    vwhInformer.Lister(),
@@ -76,8 +97,13 @@ func NewAdmissionControllerWithConfig(
 	}
 
 	logger := logging.FromContext(ctx)
+
+	controllerOptions := woptions.ControllerOptions
+	if woptions.ControllerOptions == nil {
 		const queueName = "ValidationWebhook"
-	c := controller.NewContext(ctx, wh, controller.ControllerOptions{WorkQueueName: queueName, Logger: logger.Named(queueName)})
+		controllerOptions = &controller.ControllerOptions{WorkQueueName: queueName, Logger: logger.Named(queueName)}
+	}
+	c := controller.NewContext(ctx, wh, *controllerOptions)
 
 	// Reconcile when the named ValidatingWebhookConfiguration changes.
 	vwhInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{

webhook/resourcesemantics/validation/options.go

@@ -0,0 +1,68 @@
+/*
+Copyright 2023 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package validation
+
+import (
+	"context"
+
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"knative.dev/pkg/webhook/resourcesemantics"
+)
+
+type options struct {
+	path                  string
+	types                 map[schema.GroupVersionKind]resourcesemantics.GenericCRD
+	wc                    func(context.Context) context.Context
+	disallowUnknownFields bool
+	callbacks             map[schema.GroupVersionKind]Callback
+}
+
+type OptionFunc func(*options)
+
+func WithCallbacks(callbacks map[schema.GroupVersionKind]Callback) OptionFunc {
+	return func(o *options) {
+		o.callbacks = callbacks
+	}
+}
+
+func WithPath(path string) OptionFunc {
+	return func(o *options) {
+		o.path = path
+	}
+}
+
+func WithTypes(types map[schema.GroupVersionKind]resourcesemantics.GenericCRD) OptionFunc {
+	return func(o *options) {
+		o.types = types
+	}
+}
+
+func WithWrapContext(f func(context.Context) context.Context) OptionFunc {
+	return func(o *options) {
+		o.wc = f
+	}
+}
+
+func WithDisallowUnknownFields() OptionFunc {
+	return func(o *options) {
+		o.disallowUnknownFields = true
+	}
+}
+
+func (o *options) DisallowUnknownFields() bool {
+	return o.disallowUnknownFields
+}

webhook/resourcesemantics/validation/options_test.go

@@ -0,0 +1,49 @@
+/*
+Copyright 2023 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+package validation
+
+import (
+	"reflect"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"knative.dev/pkg/webhook/resourcesemantics"
+)
+
+func TestOptions(t *testing.T) {
+	callbacks := map[schema.GroupVersionKind]Callback{}
+	types := map[schema.GroupVersionKind]resourcesemantics.GenericCRD{}
+
+	got := &options{}
+	WithCallbacks(callbacks)(got)
+	WithDisallowUnknownFields()(got)
+	WithPath("path")(got)
+	WithTypes(types)(got)
+
+	want := &options{
+		callbacks:             callbacks,
+		disallowUnknownFields: true,
+		path:                  "path",
+		types:                 types,
+		// we can't compare wc as functions are not
+		// comparable in golang (thus it needs to be
+		// done indirectly)
+	}
+
+	if !reflect.DeepEqual(got, want) {
+		t.Error("option was not applied")
+	}
+}

webhook/webhook.go

@@ -29,6 +29,7 @@ import (
 
 	// Injection stuff
 
+	"knative.dev/pkg/controller"
 	kubeinformerfactory "knative.dev/pkg/injection/clients/namespacedkube/informers/factory"
 	"knative.dev/pkg/network/handlers"
 
@@ -69,6 +70,10 @@ type Options struct {
 	// GracePeriod is how long to wait after failing readiness probes
 	// before shutting down.
 	GracePeriod time.Duration
+
+	// ControllerOptions encapsulates options for creating a new controller,
+	// including throttling and stats behavior.
+	ControllerOptions *controller.ControllerOptions
 }
 
 // Operation is the verb being operated on