Skip to content

⚠️ Generic Validator and Defaulter #3360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+153 −159
Open

⚠️ Generic Validator and Defaulter #3360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
955d723
Type defaulter and validator
alvaroaleman Oct 18, 2025
8196239
Builder
alvaroaleman Oct 18, 2025
fc15d4a
Introduce WebhookFor
alvaroaleman Oct 19, 2025
48a1c08
Update existing WebhookManagedBy
alvaroaleman Oct 20, 2025
3cc1910
Linting
alvaroaleman Oct 20, 2025
7a149e8
Preserve alias for NewWebhookManagedBy
alvaroaleman Oct 20, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions alias.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
7 changes: 3 additions & 4 deletions examples/builtins/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,9 @@ func main() {
os.Exit(1)
}

if err := ctrl.NewWebhookManagedBy(mgr).
For(&corev1.Pod{}).
WithDefaulter(&podAnnotator{}).
WithValidator(&podValidator{}).
if err := ctrl.NewWebhookManagedBy(mgr, &corev1.Pod{}).
WithAdmissionDefaulter(&podAnnotator{}).
WithAdmissionValidator(&podValidator{}).
Complete(); err != nil {
entryLog.Error(err, "unable to create webhook", "webhook", "Pod")
os.Exit(1)
Expand Down
8 changes: 1 addition & 7 deletions examples/builtins/mutatingwebhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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{}
Expand Down
13 changes: 4 additions & 9 deletions examples/builtins/validatingwebhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand All @@ -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)
}
3 changes: 1 addition & 2 deletions examples/crd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
3 changes: 1 addition & 2 deletions pkg/builder/example_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
110 changes: 59 additions & 51 deletions pkg/builder/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -56,59 +58,61 @@ type WebhookBuilder struct {
}

// WebhookManagedBy returns a new webhook builder.
func WebhookManagedBy(m manager.Manager) *WebhookBuilder {
return &WebhookBuilder{mgr: m}
}

// 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
return blder
func WebhookManagedBy[T runtime.Object](m manager.Manager, object T) *WebhookBuilder[T] {
Copy link
Member Author

@alvaroaleman alvaroaleman Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I've spent a fair bit of time on pondering about how to best deal with this in the webhook builder:

  1. Initially, I didn't have the object T func arg and used the existing WithValidator and WithDefaulter methods. That works but requires everyone to explicitly type WebhookManagedBy as the typing must be know during initial construction and can not be inferred during subsequent method calls. This has two drawbacks IMHO:
    • I anticipate that to cause confusion, because my impression has been that many go engineers are not very used to explicitly typing generics as opposed to relying on type inference
    • Anyone who has an existing CustomValidator/Defaulter would have to type this to runtime.Object which would likely cause further confusion
  2. So then I added a WebhookFor that has the same signature as the current WebhookMangagedBy and made the WebhookManagedBy non-generic and return a *WebhookBuilder[runtime.Object]. This is great for existing code as it will all keep working, but once we remove this, it will be confusing to have different names for the controller and webhook builder IMHO
  3. Then I finally ended up with this, make WebhookManagedBy generic, add an explicit type argument so type inference works and add successors to WithValidator/Defaulter in the form of WithAdmissionValidator/Defaulter. This means a breaking change for everyone that should be pretty easy to understand and fix and avoid requiring to type this to runtime.Object for existing validators/defaulters. The main drawback of that is that the new names aren't as nice (happy to hear suggestions for that).

All in all, the last option seemed the by far least bad one. What do you think?
Copy link
Member

@sbueringer sbueringer Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with all your points.

The main drawback of that is that the new names aren't as nice (happy to hear suggestions for that).

I see the following options

  1. Go with your current names, keep them like that going forward eventually drop WithValidator/Defaulter
  2. 1 + but eventually do another round of deprecation to go from WithAdmissionValidator/Defaulter to WithValidator/Defaulter
  3. Directly go to WithValidator/Defaulter, temporarily introduce deprecated WithCustomValidator/Defaulter

No absolutely strong opinions from my side, but if possible I would like to get to WithValidator/Defaulter long-term. I have a slight tendency for option 3. We already have to do a breaking change in this PR, maybe it's better to just get it over with and do slightly more breaking changes now then dragging this out over a few years. It will also give a clear hint to folks that they should just migrate to the typed versions which is super straightforward then (just start using types in Validator/Defaulter, it's not even necessary to use different methods on the builder for Validator/Defaulter). So slightly less effort to do the right migration (use types), slightly more effort to delay the migration and keep using CustomValidator/Defaulter.

Somewhat related. Do you know why Defaulter is spelled with er and Validator with or? (probably don't want to change that though because it's not worth the additional confusion)
return &WebhookBuilder[T]{mgr: m, apiType: object}
}

// 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 {
// Deprecated: Use WithAdmissionDefaulter instead.
func (blder *WebhookBuilder[T]) WithDefaulter(defaulter admission.CustomDefaulter, opts ...admission.DefaulterOption) *WebhookBuilder[T] {
blder.customDefaulter = defaulter
blder.customDefaulterOpts = opts
return blder
}

// WithAdmissionDefaulter sets up the provided admission.Defaulter in a defaulting webhook.
func (blder *WebhookBuilder[T]) WithAdmissionDefaulter(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 {
// Deprecated: Use WithAdmissionValidator instead.
func (blder *WebhookBuilder[T]) WithValidator(validator admission.CustomValidator) *WebhookBuilder[T] {
blder.customValidator = validator
return blder
}

// WithAdmissionValidator sets up the provided admission.Validator in a validating webhook.
func (blder *WebhookBuilder[T]) WithAdmissionValidator(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 {
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
}
Expand All @@ -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()

Expand All @@ -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(
Expand All @@ -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
Expand Down Expand Up @@ -217,7 +221,7 @@ func (blder *WebhookBuilder) registerWebhooks() error {
}

// registerDefaultingWebhook registers a defaulting webhook if necessary.
func (blder *WebhookBuilder) registerDefaultingWebhook() error {
func (blder *WebhookBuilder[T]) registerDefaultingWebhook() error {
mwh := blder.getDefaultingWebhook()
if mwh != nil {
mwh.LogConstructor = blder.logConstructor
Expand All @@ -244,19 +248,21 @@ 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)
}
return w
func (blder *WebhookBuilder[T]) getDefaultingWebhook() *admission.Webhook {
var w *admission.Webhook
if defaulter := blder.defaulter; defaulter != nil {
w = admission.WithDefaulter(blder.mgr.GetScheme(), blder.defaulter, blder.customDefaulterOpts...)
} else if customDefaulter := blder.customDefaulter; customDefaulter != nil {
w = admission.WithCustomDefaulter(blder.mgr.GetScheme(), blder.apiType, customDefaulter, blder.customDefaulterOpts...)
}
return nil
if w != nil && blder.recoverPanic != nil {
w = w.WithRecoverPanic(*blder.recoverPanic)
}
return w
}

// registerValidatingWebhook registers a validating webhook if necessary.
func (blder *WebhookBuilder) registerValidatingWebhook() error {
func (blder *WebhookBuilder[T]) registerValidatingWebhook() error {
vwh := blder.getValidatingWebhook()
if vwh != nil {
vwh.LogConstructor = blder.logConstructor
Expand All @@ -283,18 +289,20 @@ 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)
}
return w
func (blder *WebhookBuilder[T]) getValidatingWebhook() *admission.Webhook {
var w *admission.Webhook
if validator := blder.validator; validator != nil {
w = admission.WithValidator(blder.mgr.GetScheme(), validator)
} else if customValidator := blder.customValidator; customValidator != nil {
w = admission.WithCustomValidator(blder.mgr.GetScheme(), blder.apiType, customValidator)
}
return nil
if w != nil && blder.recoverPanic != nil {
w = w.WithRecoverPanic(*blder.recoverPanic)
}
return w
}

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 {
Expand Down Expand Up @@ -323,14 +331,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")
}

func (blder *WebhookBuilder) isAlreadyHandled(path string) bool {
func (blder *WebhookBuilder[T]) isAlreadyHandled(path string) bool {
if blder.mgr.GetWebhookServer().WebhookMux() == nil {
return false
}
Expand Down
Loading
Loading