⚠️ Generic Validator and Defaulter (#3360)

* Type defaulter and validator

* Builder

* Introduce WebhookFor

* Update existing WebhookManagedBy

* Linting

* Preserve alias for NewWebhookManagedBy

* Use WithDefaulter/WithValidator going forward

* Test defaulter

* TestValidatorBuilder

* Mixed

* Custom and Typed validator/defaulter are mutually exclusive

* Type new in validator

* Simplify builder

* Re-add aliases

* feedback

Author: alvaroaleman

alias.go

@@ -18,6 +18,7 @@ package controllerruntime
 
 import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
@@ -104,9 +105,6 @@ var (
 	// NewControllerManagedBy returns a new controller builder that will be started by the provided Manager.
 	NewControllerManagedBy = builder.ControllerManagedBy
 
-	// NewWebhookManagedBy returns a new webhook builder that will be started by the provided Manager.
-	NewWebhookManagedBy = builder.WebhookManagedBy
-
 	// NewManager returns a new Manager for creating Controllers.
 	// Note that if ContentType in the given config is not set, "application/vnd.kubernetes.protobuf"
 	// will be used for all built-in resources of Kubernetes, and "application/json" is for other types
@@ -155,3 +153,8 @@ var (
 	// SetLogger sets a concrete logging implementation for all deferred Loggers.
 	SetLogger = log.SetLogger
 )
+
+// NewWebhookManagedBy returns a new webhook builder for the provided type T.
+func NewWebhookManagedBy[T runtime.Object](mgr manager.Manager, obj T) *builder.WebhookBuilder[T] {
+	return builder.WebhookManagedBy(mgr, obj)
+}

examples/builtins/main.go

@@ -60,8 +60,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	if err := ctrl.NewWebhookManagedBy(mgr).
-		For(&corev1.Pod{}).
+	if err := ctrl.NewWebhookManagedBy(mgr, &corev1.Pod{}).
 		WithDefaulter(&podAnnotator{}).
 		WithValidator(&podValidator{}).
 		Complete(); err != nil {

examples/builtins/mutatingwebhook.go

@@ -18,10 +18,8 @@ package main
 
 import (
 	"context"
-	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )
@@ -31,12 +29,8 @@ import (
 // podAnnotator annotates Pods
 type podAnnotator struct{}
 
-func (a *podAnnotator) Default(ctx context.Context, obj runtime.Object) error {
+func (a *podAnnotator) Default(ctx context.Context, pod *corev1.Pod) error {
 	log := logf.FromContext(ctx)
-	pod, ok := obj.(*corev1.Pod)
-	if !ok {
-		return fmt.Errorf("expected a Pod but got a %T", obj)
-	}
 
 	if pod.Annotations == nil {
 		pod.Annotations = map[string]string{}

examples/builtins/validatingwebhook.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -33,12 +32,8 @@ import (
 type podValidator struct{}
 
 // validate admits a pod if a specific annotation exists.
-func (v *podValidator) validate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+func (v *podValidator) validate(ctx context.Context, pod *corev1.Pod) (admission.Warnings, error) {
 	log := logf.FromContext(ctx)
-	pod, ok := obj.(*corev1.Pod)
-	if !ok {
-		return nil, fmt.Errorf("expected a Pod but got a %T", obj)
-	}
 
 	log.Info("Validating Pod")
 	key := "example-mutating-admission-webhook"
@@ -53,14 +48,14 @@ func (v *podValidator) validate(ctx context.Context, obj runtime.Object) (admiss
 	return nil, nil
 }
 
-func (v *podValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+func (v *podValidator) ValidateCreate(ctx context.Context, obj *corev1.Pod) (admission.Warnings, error) {
 	return v.validate(ctx, obj)
 }
 
-func (v *podValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
+func (v *podValidator) ValidateUpdate(ctx context.Context, oldObj, newObj *corev1.Pod) (admission.Warnings, error) {
 	return v.validate(ctx, newObj)
 }
 
-func (v *podValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+func (v *podValidator) ValidateDelete(ctx context.Context, obj *corev1.Pod) (admission.Warnings, error) {
 	return v.validate(ctx, obj)
 }

examples/crd/main.go

@@ -129,8 +129,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	err = ctrl.NewWebhookManagedBy(mgr).
-		For(&api.ChaosPod{}).
+	err = ctrl.NewWebhookManagedBy(mgr, &api.ChaosPod{}).
 		Complete()
 	if err != nil {
 		setupLog.Error(err, "unable to create webhook")

pkg/builder/example_webhook_test.go

@@ -40,8 +40,7 @@ func ExampleWebhookBuilder() {
 	}
 
 	err = builder.
-		WebhookManagedBy(mgr).         // Create the WebhookManagedBy
-		For(&examplegroup.ChaosPod{}). // ChaosPod is a CRD.
+		WebhookManagedBy(mgr, &examplegroup.ChaosPod{}). // Create the WebhookManagedBy
 		Complete()
 	if err != nil {
 		log.Error(err, "could not create webhook")

pkg/builder/webhook.go

@@ -37,11 +37,13 @@ import (
 )
 
 // WebhookBuilder builds a Webhook.
-type WebhookBuilder struct {
+type WebhookBuilder[T runtime.Object] struct {
 	apiType                   runtime.Object
 	customDefaulter           admission.CustomDefaulter
+	defaulter                 admission.Defaulter[T]
 	customDefaulterOpts       []admission.DefaulterOption
 	customValidator           admission.CustomValidator
+	validator                 admission.Validator[T]
 	customPath                string
 	customValidatorCustomPath string
 	customDefaulterCustomPath string
@@ -56,59 +58,61 @@ type WebhookBuilder struct {
 }
 
 // WebhookManagedBy returns a new webhook builder.
-func WebhookManagedBy(m manager.Manager) *WebhookBuilder {
-	return &WebhookBuilder{mgr: m}
+func WebhookManagedBy[T runtime.Object](m manager.Manager, object T) *WebhookBuilder[T] {
+	return &WebhookBuilder[T]{mgr: m, apiType: object}
 }
 
-// TODO(droot): update the GoDoc for conversion.
-
-// For takes a runtime.Object which should be a CR.
-// If the given object implements the admission.Defaulter interface, a MutatingWebhook will be wired for this type.
-// If the given object implements the admission.Validator interface, a ValidatingWebhook will be wired for this type.
-func (blder *WebhookBuilder) For(apiType runtime.Object) *WebhookBuilder {
-	if blder.apiType != nil {
-		blder.err = errors.New("For(...) should only be called once, could not assign multiple objects for webhook registration")
-	}
-	blder.apiType = apiType
+// WithCustomDefaulter takes an admission.CustomDefaulter interface, a MutatingWebhook with the provided opts (admission.DefaulterOption)
+// will be wired for this type.
+// Deprecated: Use WithDefaulter instead.
+func (blder *WebhookBuilder[T]) WithCustomDefaulter(defaulter admission.CustomDefaulter, opts ...admission.DefaulterOption) *WebhookBuilder[T] {
+	blder.customDefaulter = defaulter
+	blder.customDefaulterOpts = opts
 	return blder
 }
 
-// WithDefaulter takes an admission.CustomDefaulter interface, a MutatingWebhook with the provided opts (admission.DefaulterOption)
-// will be wired for this type.
-func (blder *WebhookBuilder) WithDefaulter(defaulter admission.CustomDefaulter, opts ...admission.DefaulterOption) *WebhookBuilder {
-	blder.customDefaulter = defaulter
+// WithDefaulter sets up the provided admission.Defaulter in a defaulting webhook.
+func (blder *WebhookBuilder[T]) WithDefaulter(defaulter admission.Defaulter[T], opts ...admission.DefaulterOption) *WebhookBuilder[T] {
+	blder.defaulter = defaulter
 	blder.customDefaulterOpts = opts
 	return blder
 }
 
-// WithValidator takes a admission.CustomValidator interface, a ValidatingWebhook will be wired for this type.
-func (blder *WebhookBuilder) WithValidator(validator admission.CustomValidator) *WebhookBuilder {
+// WithCustomValidator takes a admission.CustomValidator interface, a ValidatingWebhook will be wired for this type.
+// Deprecated: Use WithValidator instead.
+func (blder *WebhookBuilder[T]) WithCustomValidator(validator admission.CustomValidator) *WebhookBuilder[T] {
 	blder.customValidator = validator
 	return blder
 }
 
+// WithValidator sets up the provided admission.Validator in a validating webhook.
+func (blder *WebhookBuilder[T]) WithValidator(validator admission.Validator[T]) *WebhookBuilder[T] {
+	blder.validator = validator
+	return blder
+}
+
 // WithConverter takes a func that constructs a converter.Converter.
-// The Converter will then be used by the conversion endpoint for the type passed into For().
-func (blder *WebhookBuilder) WithConverter(converterConstructor func(*runtime.Scheme) (conversion.Converter, error)) *WebhookBuilder {
+// The Converter will then be used by the conversion endpoint for the type passed into NewWebhookManagedBy()
+func (blder *WebhookBuilder[T]) WithConverter(converterConstructor func(*runtime.Scheme) (conversion.Converter, error)) *WebhookBuilder[T] {
 	blder.converterConstructor = converterConstructor
 	return blder
 }
 
 // WithLogConstructor overrides the webhook's LogConstructor.
-func (blder *WebhookBuilder) WithLogConstructor(logConstructor func(base logr.Logger, req *admission.Request) logr.Logger) *WebhookBuilder {
+func (blder *WebhookBuilder[T]) WithLogConstructor(logConstructor func(base logr.Logger, req *admission.Request) logr.Logger) *WebhookBuilder[T] {
 	blder.logConstructor = logConstructor
 	return blder
 }
 
 // WithContextFunc overrides the webhook's WithContextFunc.
-func (blder *WebhookBuilder) WithContextFunc(contextFunc func(context.Context, *http.Request) context.Context) *WebhookBuilder {
+func (blder *WebhookBuilder[T]) WithContextFunc(contextFunc func(context.Context, *http.Request) context.Context) *WebhookBuilder[T] {
 	blder.contextFunc = contextFunc
 	return blder
 }
 
 // RecoverPanic indicates whether panics caused by the webhook should be recovered.
 // Defaults to true.
-func (blder *WebhookBuilder) RecoverPanic(recoverPanic bool) *WebhookBuilder {
+func (blder *WebhookBuilder[T]) RecoverPanic(recoverPanic bool) *WebhookBuilder[T] {
 	blder.recoverPanic = &recoverPanic
 	return blder
 }
@@ -117,25 +121,25 @@ func (blder *WebhookBuilder) RecoverPanic(recoverPanic bool) *WebhookBuilder {
 //
 // Deprecated: WithCustomPath should not be used anymore.
 // Please use WithValidatorCustomPath or WithDefaulterCustomPath instead.
-func (blder *WebhookBuilder) WithCustomPath(customPath string) *WebhookBuilder {
+func (blder *WebhookBuilder[T]) WithCustomPath(customPath string) *WebhookBuilder[T] {
 	blder.customPath = customPath
 	return blder
 }
 
 // WithValidatorCustomPath overrides the path of the Validator.
-func (blder *WebhookBuilder) WithValidatorCustomPath(customPath string) *WebhookBuilder {
+func (blder *WebhookBuilder[T]) WithValidatorCustomPath(customPath string) *WebhookBuilder[T] {
 	blder.customValidatorCustomPath = customPath
 	return blder
 }
 
 // WithDefaulterCustomPath overrides the path of the Defaulter.
-func (blder *WebhookBuilder) WithDefaulterCustomPath(customPath string) *WebhookBuilder {
+func (blder *WebhookBuilder[T]) WithDefaulterCustomPath(customPath string) *WebhookBuilder[T] {
 	blder.customDefaulterCustomPath = customPath
 	return blder
 }
 
 // Complete builds the webhook.
-func (blder *WebhookBuilder) Complete() error {
+func (blder *WebhookBuilder[T]) Complete() error {
 	// Set the Config
 	blder.loadRestConfig()
 
@@ -146,13 +150,13 @@ func (blder *WebhookBuilder) Complete() error {
 	return blder.registerWebhooks()
 }
 
-func (blder *WebhookBuilder) loadRestConfig() {
+func (blder *WebhookBuilder[T]) loadRestConfig() {
 	if blder.config == nil {
 		blder.config = blder.mgr.GetConfig()
 	}
 }
 
-func (blder *WebhookBuilder) setLogConstructor() {
+func (blder *WebhookBuilder[T]) setLogConstructor() {
 	if blder.logConstructor == nil {
 		blder.logConstructor = func(base logr.Logger, req *admission.Request) logr.Logger {
 			log := base.WithValues(
@@ -172,11 +176,11 @@ func (blder *WebhookBuilder) setLogConstructor() {
 	}
 }
 
-func (blder *WebhookBuilder) isThereCustomPathConflict() bool {
+func (blder *WebhookBuilder[T]) isThereCustomPathConflict() bool {
 	return (blder.customPath != "" && blder.customDefaulter != nil && blder.customValidator != nil) || (blder.customPath != "" && blder.customDefaulterCustomPath != "") || (blder.customPath != "" && blder.customValidatorCustomPath != "")
 }
 
-func (blder *WebhookBuilder) registerWebhooks() error {
+func (blder *WebhookBuilder[T]) registerWebhooks() error {
 	typ, err := blder.getType()
 	if err != nil {
 		return err
@@ -217,8 +221,11 @@ func (blder *WebhookBuilder) registerWebhooks() error {
 }
 
 // registerDefaultingWebhook registers a defaulting webhook if necessary.
-func (blder *WebhookBuilder) registerDefaultingWebhook() error {
-	mwh := blder.getDefaultingWebhook()
+func (blder *WebhookBuilder[T]) registerDefaultingWebhook() error {
+	mwh, err := blder.getDefaultingWebhook()
+	if err != nil {
+		return err
+	}
 	if mwh != nil {
 		mwh.LogConstructor = blder.logConstructor
 		mwh.WithContextFunc = blder.contextFunc
@@ -244,20 +251,28 @@ func (blder *WebhookBuilder) registerDefaultingWebhook() error {
 	return nil
 }
 
-func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook {
-	if defaulter := blder.customDefaulter; defaulter != nil {
-		w := admission.WithCustomDefaulter(blder.mgr.GetScheme(), blder.apiType, defaulter, blder.customDefaulterOpts...)
-		if blder.recoverPanic != nil {
-			w = w.WithRecoverPanic(*blder.recoverPanic)
+func (blder *WebhookBuilder[T]) getDefaultingWebhook() (*admission.Webhook, error) {
+	var w *admission.Webhook
+	if blder.defaulter != nil {
+		if blder.customDefaulter != nil {
+			return nil, errors.New("only one of Defaulter or CustomDefaulter can be set")
 		}
-		return w
+		w = admission.WithDefaulter(blder.mgr.GetScheme(), blder.defaulter, blder.customDefaulterOpts...)
+	} else if blder.customDefaulter != nil {
+		w = admission.WithCustomDefaulter(blder.mgr.GetScheme(), blder.apiType, blder.customDefaulter, blder.customDefaulterOpts...)
 	}
-	return nil
+	if w != nil && blder.recoverPanic != nil {
+		w = w.WithRecoverPanic(*blder.recoverPanic)
+	}
+	return w, nil
 }
 
 // registerValidatingWebhook registers a validating webhook if necessary.
-func (blder *WebhookBuilder) registerValidatingWebhook() error {
-	vwh := blder.getValidatingWebhook()
+func (blder *WebhookBuilder[T]) registerValidatingWebhook() error {
+	vwh, err := blder.getValidatingWebhook()
+	if err != nil {
+		return err
+	}
 	if vwh != nil {
 		vwh.LogConstructor = blder.logConstructor
 		vwh.WithContextFunc = blder.contextFunc
@@ -283,18 +298,23 @@ func (blder *WebhookBuilder) registerValidatingWebhook() error {
 	return nil
 }
 
-func (blder *WebhookBuilder) getValidatingWebhook() *admission.Webhook {
-	if validator := blder.customValidator; validator != nil {
-		w := admission.WithCustomValidator(blder.mgr.GetScheme(), blder.apiType, validator)
-		if blder.recoverPanic != nil {
-			w = w.WithRecoverPanic(*blder.recoverPanic)
+func (blder *WebhookBuilder[T]) getValidatingWebhook() (*admission.Webhook, error) {
+	var w *admission.Webhook
+	if blder.validator != nil {
+		if blder.customValidator != nil {
+			return nil, errors.New("only one of Validator or CustomValidator can be set")
 		}
-		return w
+		w = admission.WithValidator(blder.mgr.GetScheme(), blder.validator)
+	} else if blder.customValidator != nil {
+		w = admission.WithCustomValidator(blder.mgr.GetScheme(), blder.apiType, blder.customValidator)
 	}
-	return nil
+	if w != nil && blder.recoverPanic != nil {
+		w = w.WithRecoverPanic(*blder.recoverPanic)
+	}
+	return w, nil
 }
 
-func (blder *WebhookBuilder) registerConversionWebhook() error {
+func (blder *WebhookBuilder[T]) registerConversionWebhook() error {
 	if blder.converterConstructor != nil {
 		converter, err := blder.converterConstructor(blder.mgr.GetScheme())
 		if err != nil {
@@ -323,14 +343,14 @@ func (blder *WebhookBuilder) registerConversionWebhook() error {
 	return nil
 }
 
-func (blder *WebhookBuilder) getType() (runtime.Object, error) {
+func (blder *WebhookBuilder[T]) getType() (runtime.Object, error) {
 	if blder.apiType != nil {
 		return blder.apiType, nil
 	}
-	return nil, errors.New("For() must be called with a valid object")
+	return nil, errors.New("NewWebhookManagedBy() must be called with a valid object")
 }
 
-func (blder *WebhookBuilder) isAlreadyHandled(path string) bool {
+func (blder *WebhookBuilder[T]) isAlreadyHandled(path string) bool {
 	if blder.mgr.GetWebhookServer().WebhookMux() == nil {
 		return false
 	}