⚠️ Simplify webhook wiring with Defaulter and Validator interfaces #328

mengqiy · 2019-02-14T01:21:22Z

If a CRD type implements the Defaulter and Validator interface, we will generate the admission webhook for the CRD type.
For non-CRD type, we provide methods to pass in path and handlers to create the mutating|validting webhook.

godoc and tests are on the way.

droot

This is looking good.

droot · 2019-02-20T00:05:05Z

pkg/webhook/server.go

+		if wh == nil {
+			continue
+		}
+		s.Register("path", wh)


We can generate the path using object's GVK and name. And, the default path generation should probably be shared with controller-tools so that generator is in sync with this code ?

droot · 2019-02-20T00:06:45Z

pkg/webhook/server.go

-	once sync.Once
+// For registers defaulting and validation webhooks for the provided types that implement either
+// webhooktuil.Validator or webhookutil.Defaulter.
+func (s *Server) For(objects ...runtime.Object) error {


For name itself doesn't convey the intent. Wondering if we should give it a more friendly name like EnableValidationFor or EnableDefaultsFor ?

mengqiy · 2019-02-22T02:56:13Z

Addressed comments.
PTAL
If looks OK, I will push more tests and godoc.

droot

The change looks good, have a few comments.

crdexample/pkg/resource.go

droot · 2019-02-22T18:03:37Z

crdexample/pkg/zz_generated.deepcopy.go

+	out := new(FirstMateStatus)
+	in.DeepCopyInto(out)
+	return out
+}


I have seen these example projects add too much noise to the repo and introduce additional maintenance burden. Is there a way to avoid it ?

pkg/builder/build.go

droot · 2019-02-22T18:09:01Z

pkg/builder/build.go

+		if err != nil {
+			return nil, err
+		}
+		err = svr.EnableValidationFor(blder.mgr.GetScheme(), blder.apiType)


Won't svr have access to the scheme ? If yes, then we can get rid of scheme parameter from EnableDefaultsFor and EnableValidationFor.

EnableValidationFor happens at setup time. But the setup time is earlier than the inject time.

But I just realized that there is another way to do it.

pkg/manager/internal.go

droot · 2019-02-22T18:30:38Z

pkg/webhook/server.go

+			return err
+		}
+		if len(gvks) != 1 {
+			return fmt.Errorf("expected only GVK returned by scheme.ObjectKinds")


we should some webhook related context to the error message and also include all the GVKs to help in debugging.

pkg/webhook/server.go

droot · 2019-02-22T18:33:35Z

pkg/webhook/server.go

+		s.Register("/validate-"+generatePath(gvks[0]), wh)
+	}
+	return nil
+}


I am sort of wondering if we should enable validation/defaulting by default when webhook server starts ? (This will save people one step if they want to enable validation/defaulting). And I am thinking of doing the same thing for conversion.

pkg/webhook/server.go

mengqiy · 2019-02-25T20:52:58Z

Addressed comments. PTAL at the interfaces.
2 things are still open for discussion:

should we do ⚠️ Simplify webhook wiring with Defaulter and Validator interfaces #328 (comment)
should we have an example for CRD like the one in commit b51eb44

@DirectXMan12 WDYT

droot

Looking good to me. Will defer it to @DirectXMan12 for the final LGTM because two issues that you pointed out still need to be resolved.

droot · 2019-02-26T18:37:35Z

pkg/builder/build.go

+	// If we get a NotImplementDefaulterValidatorInterfacesError, we discord the Server instance.
+	// If other type of error, surface it.
+	if err == nil {
+		e := blder.mgr.Add(svr)


quick question: shouldn't this be done before calling svr.EnableWebhooksFor anyways to ensure mgr injects dependencies for webhooks (mainly scheme) ?

droot · 2019-02-26T18:38:28Z

pkg/webhook/defaulter.go

+func (h *mutatingHandler) InjectScheme(s *runtime.Scheme) error {
+	h.scheme = s
+	var err error
+	h.decoder, err = admission.NewDecoder(h.scheme)


+1 on setting decoder this way.

crdexample/controller.go

crdexample/logutil/log.go

DirectXMan12 · 2019-02-26T19:27:02Z

crdexample/controller.go

+var fmLog = logutil.Log.WithName("firstmate-reconciler")
+
+// FirstMateController reconciles ReplicaSets
+type FirstMateController struct {


name this something useful -- while "firstmatecontroller" is fun, it's not particularly descriptive

DirectXMan12 · 2019-02-26T20:26:47Z

crdexample/main.go

+
+	appsv1 "k8s.io/api/apps/v1"
+	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
+	"sigs.k8s.io/controller-runtime/crdexample/logutil"


lets switch these over to using the alias package (ctrl "sigs.k8s.io/controller-runtime") as opposed to a bajillion separate packages.

crdexample/pkg/resource.go

DirectXMan12 · 2019-02-26T20:31:13Z

pkg/webhook/admission/decode.go

@@ -35,7 +35,7 @@ func NewDecoder(scheme *runtime.Scheme) (*Decoder, error) {
 }

 // Decode decodes the inlined object in the AdmissionRequest into the passed-in runtime.Object.
-func (d *Decoder) Decode(req Request, into runtime.Object) error {
+func (d *Decoder) Decode(rawObj runtime.RawExtension, into runtime.Object) error {


add an additional method for this -- the plain old decode latest from admission request behavior is really nice.

DirectXMan12 · 2019-02-26T21:09:35Z

pkg/webhook/server.go

+// neither the Defaulter nor the Validator interfaces.
+// It returns a NotImplementDefaulterValidatorInterfacesError error if a provided object implements
+// neither the Defaulter interface nor the Validator interface.
+func (s *Server) EnableWebhooksFor(objects ...runtime.Object) error {


I'm not convinced this needs to be a method (as opposed to a helper or helpers). The helper makes it more composable, easier to test individually, and lets us actually have a type signature that requires an object that implements the interfaces (removing the error path in favor of compile-time checking).

DirectXMan12 · 2019-02-26T21:10:17Z

pkg/webhook/server.go

+
+func generatePath(gvk schema.GroupVersionKind) string {
+	var pathItems []string
+	splittedGroup := strings.Split(gvk.Group, ".")


why aren't we just strings.Replace(gvk.Group, ".", "-") or something?

Curret behavior doesn't include domain name in the path:

Group: apps.example.com Kind: Foo

I will get /mutate-apps-foo instead of /mutate-apps-example-com-foo. IMO it's more concise, but there may be a small risk of collision. e.g. with

Group: apps.bar.com Kind: Foo

I don't have a strong opinion about if we should have domain name.
Thoughts?

I don't think conciseness here is valuable (only ever have to write this once, and it's usually going to be autogenerated), and I'd rather avoid collisions

pkg/webhook/validator.go

DirectXMan12 · 2019-02-26T21:17:37Z

pkg/webhook/server.go

+			return nil
+		}
+
+		mwh := newDefaultingWebhookFor(object)


we should opportunistically merge validating and defaulting hooks into a single hook when possible, to avoid the extra round-trip

we should opportunistically merge validating and defaulting hooks into a single hook when possible, to avoid the extra round-trip

This means you want to put defaulting and validating hooks in one mutatingWebhook. But doing it this way is not safe, since there may be another mutating webhook changes the object after your validation logic. The safe way to do it is making validation logic in a validating webhook, which is always run after all mutating webhooks.

good point 👍

mengqiy · 2019-02-28T19:48:31Z

pkg dir is ready for another round of reviewing interfaces.
I haven't make a lot of changes in examples/crd. I'd like to discuss with @DirectXMan12 more before spending more than time on it.

DirectXMan12 · 2019-02-28T21:52:00Z

pkg/builder/build.go

@@ -218,3 +240,60 @@ func (blder *Builder) doController(r reconcile.Reconciler) error {
 	blder.ctrl, err = newController(name, blder.mgr, controller.Options{Reconciler: r})
 	return err
 }
+
+func (blder *Builder) doWebhook() error {
+	if blder.mgr.GetScheme() == nil {


we default the scheme, no? This should never be nil

Was being more defensive here. But if you think it's not necessary at all, I will drop it.

pkg/builder/build.go

pkg/webhook/admission/decode.go

pkg/webhook/admission/validator.go

pkg/webhook/server.go

pkg/webhook/admission/defaulter.go

DirectXMan12 · 2019-02-28T22:02:16Z

examples/crd/main.go

+
+	appsv1 "k8s.io/api/apps/v1"
+	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
+	"sigs.k8s.io/controller-runtime/examples/crd/pkg"


let's use ctrl "sigs.k8s.io/controller-runtime" instead of most of these in the examples

DirectXMan12 · 2019-02-28T22:03:07Z

examples/crd/Dockerfile

@@ -0,0 +1,17 @@
+# Build the manager binary


don't think we should have the deployment stuff here -- this should be only code. Deployment examples can live in KB

Ack. Will polish the CRD example and remote deployment bits in the example.

DirectXMan12 · 2019-02-28T22:03:34Z

examples/crd/pkg/resource.go

+var _ admission.Validator = &FirstMate{}
+
+// ValidateCreate implements webhookutil.validator so a webhook will be registered for the type
+func (f *FirstMate) ValidateCreate() error {


should have webhook generation annotations?

mengqiy · 2019-03-04T18:37:53Z

PTAL

mengqiy · 2019-03-08T18:25:31Z

Rebased on HEAD to resolved the conflicts and squashed some commits.
Ready for the final round of review for API changes e.g. pkg/manager/manager.go

DirectXMan12

minor nits inline

DirectXMan12 · 2019-03-09T01:45:39Z

pkg/builder/build.go

+		gvk.Version + "-" + strings.ToLower(gvk.Kind)
+
+	defaulter, isDefaulter := blder.apiType.(admission.Defaulter)
+	if isDefaulter {


nit: collapse this into a compound statement.

DirectXMan12 · 2019-03-09T01:45:52Z

pkg/builder/build.go

+		}
+	}
+
+	validator, isValidator := blder.apiType.(admission.Validator)


nit: collapse this into a compound statement

DirectXMan12 · 2019-03-09T01:47:54Z

pkg/builder/build.go

+		vwh := admission.ValidatingWebhookFor(validator)
+		if vwh != nil {
+			path := "/validate-" + partialPath
+			log.Info("registering a validating webhook", "path", path)


log the GVK?

pkg/builder/build.go

DirectXMan12 · 2019-03-09T02:43:03Z

/lgtm
/approve

Can you follow up with a PR with slightly more comprehensive tests?

k8s-ci-robot · 2019-03-09T02:43:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, mengqiy

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

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

mengqiy · 2019-03-09T02:44:55Z

Can you follow up with a PR with slightly more comprehensive tests?

Sure!

If user defines a CRD type and make it implement the Defaulter and (or) the Validator interface, it will automatically wire a webhook server for the admission webhooks.

k8s-ci-robot · 2019-03-11T17:18:57Z

New changes are detected. LGTM label has been removed.

mengqiy · 2019-03-11T17:20:50Z

Forgot to remove the do-not-merge label last Friday, and this PR ended up with a small merge conflict with #352 in the comments.
I rebased and resolved conflicts.

Re-applying the label per #328 (comment), since there is no actual code changes.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 14, 2019

k8s-ci-robot requested review from DirectXMan12 and droot February 14, 2019 01:21

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 14, 2019

mengqiy force-pushed the simplifywhif branch from cbd3775 to 81c3025 Compare February 14, 2019 01:37

mengqiy mentioned this pull request Feb 14, 2019

✨ use builder in controller scaffolding kubernetes-sigs/kubebuilder#603

Closed

droot reviewed Feb 20, 2019

View reviewed changes

mengqiy force-pushed the simplifywhif branch from 81c3025 to 5264408 Compare February 22, 2019 02:53

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 22, 2019

droot reviewed Feb 22, 2019

View reviewed changes

mengqiy force-pushed the simplifywhif branch from 5264408 to 8c836d7 Compare February 25, 2019 20:49

droot reviewed Feb 26, 2019

View reviewed changes

DirectXMan12 suggested changes Feb 26, 2019

View reviewed changes

DirectXMan12 changed the title ~~Simplify the webhook interface~~ ⚠️ Simplify the webhook interface Feb 27, 2019

DirectXMan12 added this to the 0.2.0 milestone Feb 27, 2019

mengqiy force-pushed the simplifywhif branch from 6c379c1 to 49dca2b Compare February 28, 2019 19:44

DirectXMan12 suggested changes Feb 28, 2019

View reviewed changes

mengqiy force-pushed the simplifywhif branch from 5cbd99c to 81d53b3 Compare March 1, 2019 19:47

mengqiy added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 4, 2019

DirectXMan12 mentioned this pull request Mar 6, 2019

Webhook: Need to extend the interface of type.Decoder #308

Closed

mengqiy force-pushed the simplifywhif branch from 81d53b3 to c1e3a9e Compare March 8, 2019 18:21

mengqiy removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2019

mengqiy added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2019

mengqiy force-pushed the simplifywhif branch from c1e3a9e to b6115b2 Compare March 8, 2019 19:24

DirectXMan12 suggested changes Mar 9, 2019

View reviewed changes

mengqiy force-pushed the simplifywhif branch from b6115b2 to d075be5 Compare March 9, 2019 02:21

k8s-ci-robot assigned DirectXMan12 Mar 9, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2019

mengqiy removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2019

Mengqi Yu added 5 commits March 11, 2019 10:16

✨ support decoding the old object in an AdmissionRequest

41ba537

⚠️ wire wehbooks if implements the interfaces

0208f43

If user defines a CRD type and make it implement the Defaulter and (or) the Validator interface, it will automatically wire a webhook server for the admission webhooks.

🏃 not use deprecated method in builder example

69e51f7

🏃 move example for builtin type

5e3a6da

🏃 add an example for CRD

ae02690

mengqiy force-pushed the simplifywhif branch from d075be5 to ae02690 Compare March 11, 2019 17:18

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2019

mengqiy added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2019

k8s-ci-robot merged commit 8810470 into kubernetes-sigs:master Mar 11, 2019

mengqiy mentioned this pull request Mar 12, 2019

🏃 add tests for simplified webhook interface #358

Merged

mengqiy deleted the simplifywhif branch March 12, 2019 04:08

mengqiy changed the title ~~⚠️ Simplify the webhook interface~~ ⚠️ Simplify webhook wiring with Defaulter and Validator interfaces Sep 5, 2019

⚠️ Simplify webhook wiring with Defaulter and Validator interfaces #328

⚠️ Simplify webhook wiring with Defaulter and Validator interfaces #328

Conversation

mengqiy commented Feb 14, 2019 • edited Loading

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy commented Feb 22, 2019

droot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy Feb 22, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy commented Feb 25, 2019

droot left a 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

mengqiy Feb 28, 2019 • 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

mengqiy commented Feb 28, 2019

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

mengqiy Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy commented Mar 4, 2019

mengqiy commented Mar 8, 2019 • edited Loading

DirectXMan12 left a 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

DirectXMan12 commented Mar 9, 2019

k8s-ci-robot commented Mar 9, 2019

mengqiy commented Mar 9, 2019

k8s-ci-robot commented Mar 11, 2019

mengqiy commented Mar 11, 2019 • edited Loading

mengqiy commented Feb 14, 2019 •

edited

Loading

mengqiy Feb 22, 2019 •

edited

Loading

mengqiy Feb 28, 2019 •

edited

Loading

mengqiy Mar 1, 2019 •

edited

Loading

mengqiy commented Mar 8, 2019 •

edited

Loading

mengqiy commented Mar 11, 2019 •

edited

Loading