⚠️ Generic Validator and Defaulter

[alvaroaleman]

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)

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman

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]

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

[alvaroaleman]

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?

[sbueringer]

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)

[alvaroaleman]

/cc @sbueringer

[k8s-ci-robot]

@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	7a149e8	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.