✨ Allow webhooks to register custom validators/defaulter types #1676

vincepri · 2021-09-27T19:01:21Z

This changeset allows our webhook builder to take in a handler any other
struct other than a runtime.Object.

Today having an object as the primary source of truth for both
Defaulting and Validators makes API types carry a lot of information and
business logic alongside their definitions.

Moreover, lots of folks in the past have asked for ways to have an
external type to handle these operations and use a controller runtime
client for validations.

This change brings a new way to register webhooks, which admission.For
handler any type (struct) can be a defaulting or validating handler for
a runtime Object.

Signed-off-by: Vince Prignano [email protected]

vincepri · 2021-09-27T19:01:27Z

/hold

k8s-ci-robot · 2021-09-27T19:01:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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~~ [vincepri]

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

fabriziopandini

I see two aspect for this PR

Allowing registering webhook implemented outside of the API types
Improving validator and default interface with context and always returning error.

For the first part, I have the impression that what we need is just somethings that implements the two methods defined in runtime.Object, but I still need to investigate this a little bit.
For the second part, might be worth also considering a breaking change, given that those seems best practices to me

pkg/webhook/admission/defaulter_handler.go

vincepri · 2021-09-27T19:36:57Z

For the first part, I have the impression that what we need is just somethings that implements the two methods defined in runtime.Object, but I still need to investigate this a little bit.

Would that be a preferred solution to this problem? It seems like that overwriting the deep copy methods is a hacky way to force an external type into the current webhooks builder logic.

vincepri · 2021-09-27T19:40:15Z

For the second part, might be worth also considering a breaking change, given that those seems best practices to me

This is something we can consider breaking in a future release, once we can streamline the interface as well

fabriziopandini · 2021-09-27T19:42:36Z

For the first part, I have the impression that what we need is just somethings that implements the two methods defined in runtime.Object, but I still need to investigate this a little bit.

Would that be a preferred solution to this problem? It seems like that overwriting the deep copy methods is a hacky way to force an external type into the current webhooks builder logic.

That's right, I'm trying to understand if we can be explicit using HandlerFor in the builder but avoiding to duplicate the Handle code in validation/defaulter handler

coderanger · 2021-09-27T19:47:54Z

pkg/builder/webhook.go

+//
+// If the given object implements the admission.DefaulterFor interface, a MutatingWebhook will be wired for this type.
+// If the given object implements the admission.ValidatorFor interface, a ValidatingWebhook will be wired for this type.
+func (blder *WebhookBuilder) HandlerFor(forType admission.For) *WebhookBuilder {


Would something like WebhookFor be better? I worry that Handler is such a generic term.

Not sure if that's clear either, wouldn't the WebhookBuilder be in charge of returning the webhook?

We don't document or advertise that approach, but its definitely possible to do this external to the api package with the existing interface by embedding the api type into the webhook implementation

@alvaroaleman I tried to do that, but I couldn't get it to work with the DeepCopy and subsequent Decode. (because the webhook struct is not registered as type for the decoding). Do you have a link to an existing implementation?

ah ok, I didn't test it I just assumed that it should work.

Okay, I just thought maybe there is a way and I just couldn't find it :)

alvaroaleman · 2021-09-27T21:27:29Z

pkg/builder/webhook.go

+//
+// If the given object implements the admission.DefaulterFor interface, a MutatingWebhook will be wired for this type.
+// If the given object implements the admission.ValidatorFor interface, a ValidatingWebhook will be wired for this type.
+func (blder *WebhookBuilder) HandlerFor(forType admission.For) *WebhookBuilder {


We don't document or advertise that approach, but its definitely possible to do this external to the api package with the existing interface by embedding the api type into the webhook implementation

pkg/builder/webhook.go

pkg/webhook/admission/validator_handler.go

pkg/builder/webhook.go

pkg/webhook/admission/defaulter_handler.go

pkg/webhook/admission/validator_handler.go

alvaroaleman · 2021-09-28T19:53:19Z

pkg/webhook/admission/validator_handler.go

+
+// WithValidator defines functions for validating an operation.
+type WithValidator interface {
+	ValidateCreate(ctx context.Context, obj runtime.Object) error


Can we use client.Object for these?

Not really, unless we make it a breaking change? I was trying to change any interface until 0.11, so we can backport this change

These are new interfaces, how would using client.Object here be breaking?

I was thinking that this would make usage slightly better, i.E. you wouldn't be able to pass a List object in here

While the interfaces are new, the object passed in comes from the webhook builder, we'd have to cast it before passing creating the webhook. Moreover, when we use DeepCopyObject() that returns a runtime.Object, which we'd have to cast again if we want to use client.Object everywhere

pkg/webhook/admission/defaulter_handler.go

alvaroaleman · 2021-09-28T20:00:53Z

pkg/webhook/admission/validator_handler.go

+
+	// Get the object in the request
+	obj := h.object.DeepCopyObject()
+	if req.Operation == v1.Create {


Nit: We could switch on the operation to be clear that these are mutually excluse (and maybe have a erorring default clause). Also, the error final error handling can be moved out of the conditions and deduplicated

pkg/builder/webhook.go

sbueringer · 2021-09-29T07:40:29Z

pkg/webhook/admission/validator_custom.go

+		}
+
+		err = h.validator.ValidateDelete(ctx, obj)
+	}


Do we want to return an error in the default case?

pkg/webhook/alias.go

pkg/webhook/admission/defaulter_custom.go

This changeset allows our webhook builder to take in a handler any other struct other than a runtime.Object. Today having an object as the primary source of truth for both Defaulting and Validators makes API types carry a lot of information and business logic alongside their definitions. Moreover, lots of folks in the past have asked for ways to have an external type to handle these operations and use a controller runtime client for validations. This change brings a new way to register webhooks, which admission.For handler any type (struct) can be a defaulting or validating handler for a runtime Object. Signed-off-by: Vince Prignano <[email protected]>

vincepri · 2021-09-29T16:27:21Z

/hold cancel

sbueringer · 2021-09-29T16:51:42Z

No more nits from my side :)

Thank you very much for implementing this!!

sbueringer · 2021-09-29T16:51:47Z

/lgtm

sbueringer · 2021-09-29T16:52:35Z

/hold
@vincepri Not sure if you want to wait for feedback from others. (tide would have merged otherwise)

vincepri · 2021-09-29T18:58:27Z

/hold for @alvaroaleman

alvaroaleman · 2021-09-29T19:14:35Z

/hold cancel
Thanks for your work!

vincepri · 2021-09-29T19:50:19Z

/cherrypick release-0.10

k8s-infra-cherrypick-robot · 2021-09-29T19:51:01Z

@vincepri: new pull request created: #1679

In response to this:

/cherrypick release-0.10

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/test-infra repository.

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 27, 2021

k8s-ci-robot requested review from gerred and mengqiy September 27, 2021 19:01

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2021

vincepri force-pushed the admission-for branch 2 times, most recently from bc58c4b to 9fd7870 Compare September 27, 2021 19:27

fabriziopandini reviewed Sep 27, 2021

View reviewed changes

pkg/webhook/admission/defaulter_handler.go Outdated Show resolved Hide resolved

vincepri force-pushed the admission-for branch from 9fd7870 to 1f1fd9c Compare September 27, 2021 19:36

coderanger reviewed Sep 27, 2021

View reviewed changes

alvaroaleman reviewed Sep 27, 2021

View reviewed changes

sbueringer reviewed Sep 28, 2021

View reviewed changes

pkg/webhook/admission/validator_handler.go Outdated Show resolved Hide resolved

pkg/builder/webhook.go Outdated Show resolved Hide resolved

pkg/builder/webhook.go Outdated Show resolved Hide resolved

vincepri force-pushed the admission-for branch 4 times, most recently from bbaed32 to cd7d01a Compare September 28, 2021 14:47

sbueringer reviewed Sep 28, 2021

View reviewed changes

vincepri force-pushed the admission-for branch from cd7d01a to 08aca1c Compare September 28, 2021 15:46

alvaroaleman reviewed Sep 28, 2021

View reviewed changes

pkg/webhook/admission/defaulter_handler.go Outdated Show resolved Hide resolved

alvaroaleman reviewed Sep 28, 2021

View reviewed changes

vincepri force-pushed the admission-for branch 3 times, most recently from 9844bd4 to 6b01655 Compare September 28, 2021 20:36

vincepri force-pushed the admission-for branch 4 times, most recently from 6052973 to f99b738 Compare September 28, 2021 21:37

sbueringer reviewed Sep 29, 2021

View reviewed changes

vincepri force-pushed the admission-for branch from f99b738 to e855958 Compare September 29, 2021 16:25

vincepri force-pushed the admission-for branch from e855958 to e133413 Compare September 29, 2021 16:26

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2021

k8s-ci-robot assigned sbueringer Sep 29, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2021

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 29, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2021

k8s-ci-robot merged commit dba3220 into kubernetes-sigs:master Sep 29, 2021

k8s-ci-robot added this to the v0.10.x milestone Sep 29, 2021

k8s-infra-cherrypick-robot mentioned this pull request Sep 29, 2021

✨ Allow webhooks to register custom validators/defaulter types #1679

Merged

timebertt mentioned this pull request Oct 12, 2021

Upgrade k8s.io/* to v0.22, sigs.k8s.io/controller-runtime to v0.10 gardener/gardener#4772

Closed

6 tasks

acumino mentioned this pull request Feb 1, 2022

Upgrade k8s.io/* to v0.23, sigs.k8s.io/controller-runtime to v0.11 gardener/gardener#5282

Closed

1 task

muvaf mentioned this pull request Mar 1, 2022

design: webhooks in crossplane crossplane/crossplane#2719

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Allow webhooks to register custom validators/defaulter types #1676

✨ Allow webhooks to register custom validators/defaulter types #1676

vincepri commented Sep 27, 2021

vincepri commented Sep 27, 2021

k8s-ci-robot commented Sep 27, 2021

fabriziopandini left a comment

vincepri commented Sep 27, 2021

vincepri commented Sep 27, 2021 •

edited

Loading

fabriziopandini commented Sep 27, 2021

coderanger Sep 27, 2021

vincepri Sep 27, 2021

alvaroaleman Sep 27, 2021

sbueringer Sep 28, 2021 •

edited

Loading

alvaroaleman Sep 28, 2021

sbueringer Sep 28, 2021

alvaroaleman Sep 27, 2021

alvaroaleman Sep 28, 2021

vincepri Sep 28, 2021

alvaroaleman Sep 29, 2021

vincepri Sep 29, 2021

alvaroaleman Sep 28, 2021

vincepri Sep 28, 2021

sbueringer Sep 29, 2021

vincepri commented Sep 29, 2021

sbueringer commented Sep 29, 2021

sbueringer commented Sep 29, 2021

sbueringer commented Sep 29, 2021 •

edited

Loading

vincepri commented Sep 29, 2021 •

edited

Loading

alvaroaleman commented Sep 29, 2021

vincepri commented Sep 29, 2021

k8s-infra-cherrypick-robot commented Sep 29, 2021

✨ Allow webhooks to register custom validators/defaulter types #1676

✨ Allow webhooks to register custom validators/defaulter types #1676

Conversation

vincepri commented Sep 27, 2021

vincepri commented Sep 27, 2021

k8s-ci-robot commented Sep 27, 2021

fabriziopandini left a comment

Choose a reason for hiding this comment

vincepri commented Sep 27, 2021

vincepri commented Sep 27, 2021 • edited Loading

fabriziopandini commented Sep 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincepri commented Sep 29, 2021

sbueringer commented Sep 29, 2021

sbueringer commented Sep 29, 2021

sbueringer commented Sep 29, 2021 • edited Loading

vincepri commented Sep 29, 2021 • edited Loading

alvaroaleman commented Sep 29, 2021

vincepri commented Sep 29, 2021

k8s-infra-cherrypick-robot commented Sep 29, 2021

vincepri commented Sep 27, 2021 •

edited

Loading

sbueringer Sep 28, 2021 •

edited

Loading

sbueringer commented Sep 29, 2021 •

edited

Loading

vincepri commented Sep 29, 2021 •

edited

Loading