⚠️ Generic Validator and Defaulter #3360

alvaroaleman · 2025-10-20T23:25:07Z

This change adds the Validator and Defaulter interfaces which are generic versions of CustomValidator and CustomDefaulter. The existing CusotmDefaulter and CustomValidator are being deprecated.

Fixes #2675
/hold

This is missing tests, I will add them once we agree on the direction in #3360 (comment)

alvaroaleman · 2025-10-20T23:36:48Z

pkg/builder/webhook.go

-	}
-	blder.apiType = apiType
-	return blder
+func WebhookManagedBy[T runtime.Object](m manager.Manager, object T) *WebhookBuilder[T] {


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

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

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

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?

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

Go with your current names, keep them like that going forward eventually drop WithValidator/Defaulter

1 + but eventually do another round of deprecation to go from WithAdmissionValidator/Defaulter to WithValidator/Defaulter

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)

Okay, if we say that we want to end up with WithValidator/Defaulter then option 3 makes most sense, otherwise its just a back and forth. I guess we can add a codesnippet before/after to the changelog to make that at least easy

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)

That is just the English language, one has an er suffix and the other or

Okay, if we say that we want to end up with WithValidator/Defaulter then option 3 makes most sense, otherwise its just a back and forth. I guess we can add a codesnippet before/after to the changelog to make that at least easy

Agree, sounds good

alvaroaleman · 2025-10-20T23:37:57Z

/cc @sbueringer

sbueringer

Just a few minor findings

pkg/webhook/admission/validator_custom.go

pkg/webhook/alias.go

pkg/builder/webhook.go

pkg/builder/webhook_test.go

sbueringer · 2025-10-30T13:12:38Z

pkg/builder/webhook_test.go


 var _ admission.CustomValidator = &TestCustomValidator{}
+
+type testValidatorDefaulter struct{}


Do we intentionally have two structs here that both are using TestCustomDefaultValidator internally?

Yeah, because one of them is a Validator and the other a CustomValidator but I wanted to avoid implementing this twice, hence one is just a shim over the other.

The tests here also define their own runtime.Object which I guess is from back when these where methods on that. There isn't a good reason to do that at this point vs just using any built-in type, but the diff of this PR is already quite large so I will probably clean this up in a follow-up.

Sorry I was a bit unclear.

I meant do we need testValidatorDefaulter and testDefaultValidatorValidator
(First one implements only Default, second one Validate*)

The name of the second one is also a bit strange

In any case, fine to clean up in a follow-up

k8s-ci-robot · 2025-10-30T23:20:16Z

@alvaroaleman: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-controller-runtime-apidiff	`31c6c99`	link	false	`/test pull-controller-runtime-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

sbueringer · 2025-10-31T13:08:58Z

Thank you very much!!!

/lgtm
/approve
/hold cancel

k8s-ci-robot · 2025-10-31T13:09:05Z

LGTM label has been added.

Git tree hash: a30fb81d8a79301f79c37ce8474d6d78ccede2ba

k8s-ci-robot · 2025-10-31T13:09:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alvaroaleman,sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alvaroaleman added 4 commits October 20, 2025 19:20

Type defaulter and validator

955d723

Builder

8196239

Introduce WebhookFor

fc15d4a

Update existing WebhookManagedBy

48a1c08

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 20, 2025

k8s-ci-robot requested review from FillZpp and inteon October 20, 2025 23:25

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 20, 2025

alvaroaleman commented Oct 20, 2025

View reviewed changes

alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 20, 2025

k8s-ci-robot requested a review from sbueringer October 20, 2025 23:37

Linting

3cc1910

alvaroaleman assigned sbueringer Oct 20, 2025

alvaroaleman force-pushed the typed branch from 05ed02f to f7f2b3b Compare October 21, 2025 03:16

Preserve alias for NewWebhookManagedBy

7a149e8

alvaroaleman force-pushed the typed branch from f7f2b3b to 7a149e8 Compare October 21, 2025 03:29

alvaroaleman added 5 commits October 29, 2025 20:41

Use WithDefaulter/WithValidator going forward

2b33f30

Test defaulter

6fc8cb1

TestValidatorBuilder

cad0736

Mixed

4344a72

Custom and Typed validator/defaulter are mutually exclusive

3c11c98

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 30, 2025

sbueringer reviewed Oct 30, 2025

View reviewed changes

alvaroaleman added 3 commits October 30, 2025 18:56

Type new in validator

480d040

Simplify builder

2e246e9

Re-add aliases

8f65834

feedback

31c6c99

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 31, 2025

k8s-ci-robot merged commit 0f049b8 into kubernetes-sigs:main Oct 31, 2025
8 of 9 checks passed


		var _ admission.CustomValidator = &TestCustomValidator{}

		type testValidatorDefaulter struct{}

⚠️ Generic Validator and Defaulter #3360

⚠️ Generic Validator and Defaulter #3360

Conversation

alvaroaleman commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alvaroaleman Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvaroaleman Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

alvaroaleman commented Oct 20, 2025

Uh oh!

sbueringer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sbueringer Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

alvaroaleman Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

sbueringer Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 30, 2025

Uh oh!

sbueringer commented Oct 31, 2025

Uh oh!

k8s-ci-robot commented Oct 31, 2025

Uh oh!

k8s-ci-robot commented Oct 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alvaroaleman commented Oct 20, 2025 •

edited

Loading

alvaroaleman Oct 20, 2025 •

edited

Loading

sbueringer Oct 21, 2025 •

edited

Loading

sbueringer Oct 31, 2025 •

edited

Loading