✨ Helper for unmanaged webhook server #1429

kevindelgado · 2021-03-16T01:18:58Z

This adds helper functions to setup webhooks server without a manager. It adds

webhook.Unmanaged that creates a webhook.Server without a manager / cluster connection information
admission.StandalonWebhook that creates an http.Handler that can be run on any arbitrary Go HTTP server (http.SeverMux) rather than a webhook.Server)

kevindelgado · 2021-03-16T01:21:00Z

It’s worth noting that dependency injection is in the process of being deprecated.

I plan to follow this up with an issue/attempt at doing this without dependency injection. This is more involved though (and probably touches a lot of how the webhook code is organized) and it doesn’t seem like we should prevent publishing a simple helper for unmanaged webhook servers until dependency injection is sorted out.

k8s-ci-robot · 2021-03-16T01:24:09Z

Hi @kevindelgado. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

kevindelgado · 2021-03-16T01:26:42Z

/assign @DirectXMan12

kevindelgado

Left some questions inline PTAL @DirectXMan12

kevindelgado · 2021-03-16T01:23:21Z

pkg/webhook/server.go

+
+// NewUnmanaged provides a webhook server that can be ran without
+// a controller manager.
+func NewUnmanaged(cluster cluster.Cluster, options Options) (*Server, error) {


Should we just pass a server directly instead of creating this new Options type? It’s weird because Server is publicly exposed whereas in the other types it’s usually an internal type.

I think eventually we probably want to more closely align with how other packages handle options (and default the options instead of the server itself), right? Not sure what's best for right now though.

kevindelgado · 2021-03-16T01:24:02Z

pkg/webhook/webhook_integration_test.go

+		})
+	})
+	Context("when running a webhook server without a manager ", func() {
+		It("should reject create request for webhook that rejects all requests", func(done Done) {


Is there a more canonical way of running the same assertions across different contexts (instead of duplicating everything after the server creation in the It() statements)?

either JustBeforeEach and BeforeEach, or use a helper method.

Eh, now that I've changed added StartStandalone the tests feel just different enough to not make it worth using a helper or forcing deduplication. The tests just seem more readable as is, but let me know if you think there's anything you'd want to factor out.

kevindelgado

Added StandaloneWebhook helper for attach c-r webhooks to an existing arbitrary http.ServeMux (part two of #1255 (comment)). PTAL @DirectXMan12

kevindelgado · 2021-03-22T15:44:08Z

pkg/webhook/admission/webhook.go

@@ -203,3 +208,62 @@ func (w *Webhook) InjectFunc(f inject.Func) error {

 	return setFields(w.Handler)
 }
+
+// InstrumentedHook adds some instrumentation on top of the given webhook.
+func InstrumentedHook(path string, hookRaw http.Handler) http.Handler {


I moved InstrumentedHook from server.go here into admission/webhook.go because I'm using it in StandaloneWebhook but also because it seemed more appropriate because its wrapping instrumentation around the handler rather than the server stuff. Let me know if you think somewhere else is more appropriate.

this applies to all webhooks, not just admission ones, so it belongs in pkg/webhook.

or you could put it in a subpackage to avoid import loops, if necessary, like pkg/webhook/internal/metrics or something

kevindelgado · 2021-03-22T15:46:24Z

pkg/webhook/webhook_integration_test.go

+		})
+	})
+	Context("when running a standalone webhook", func() {
+		It("should reject create request for webhook that rejects all requests", func(done Done) {


This is the only test I have for StandaloneWebhook. I tried to pull the minimal setup I could out of server.go#Start to mimic what an arbitrary Go HTTP server could look like. Let me know if you had something else in mind for testing it.

rohitagarwal003 · 2021-03-22T19:19:42Z

Can you also add some docs on who to use this? It's hard to discover these without these being in the docs.

DirectXMan12

we should also probably have a couple godoc examples (see example_test.go files) and a bit more verbose docs on when you might want to use these.

DirectXMan12 · 2021-03-22T22:22:46Z

pkg/webhook/admission/webhook.go

@@ -203,3 +208,62 @@ func (w *Webhook) InjectFunc(f inject.Func) error {

 	return setFields(w.Handler)
 }
+
+// InstrumentedHook adds some instrumentation on top of the given webhook.
+func InstrumentedHook(path string, hookRaw http.Handler) http.Handler {


this applies to all webhooks, not just admission ones, so it belongs in pkg/webhook.

DirectXMan12 · 2021-03-22T22:23:44Z

pkg/webhook/admission/webhook.go

@@ -203,3 +208,62 @@ func (w *Webhook) InjectFunc(f inject.Func) error {

 	return setFields(w.Handler)
 }
+
+// InstrumentedHook adds some instrumentation on top of the given webhook.
+func InstrumentedHook(path string, hookRaw http.Handler) http.Handler {


or you could put it in a subpackage to avoid import loops, if necessary, like pkg/webhook/internal/metrics or something

DirectXMan12 · 2021-03-22T22:27:45Z

pkg/webhook/admission/webhook.go

+}
+
+// StandaloneWebhook transforms a Webhook that needs to be registered
+// on a webhook.Server into one that can be ran on any arbitrary mux.


This documentation is technically correct but slightly unclear. I'd

Update the documentation on Webhook to mention that it needs to be registered with a webhook.Server or populated by StandaloneWebhook

maybe reword to something like

// StandaloneWebhook prepares a webhook for use without a webhook.Server, passing in the information normally // populated by webhook.Server and instrumenting the webhook with metrics. // // Use this to attach your webhook to an arbitrary HTTP server or mux.

sounds good

DirectXMan12 · 2021-03-22T22:28:39Z

pkg/webhook/admission/webhook.go

+	if opts.Path == "" {
+		return hook, nil
+	}
+	return InstrumentedHook(opts.Path, hook), nil


Should this instrument since we don't actually know the path? I almost feel like it should just be up to the caller to do this, or the caller's webhook server, but I'm open to opinions.

My thinking was that there is no reason why the caller can't leave out the path and call InstrumentedHook themselves, but if they do supply a path in the options, it would be nice to do it for them?

DirectXMan12 · 2021-03-22T22:29:07Z

pkg/webhook/server.go

+
+// NewUnmanaged provides a webhook server that can be ran without
+// a controller manager.
+func NewUnmanaged(options Options) (*Server, error) {


nit: rename to NewStandaloneServer or something?

actually, since server is designed to be initialized from a struct, I think we could even switch this around and have a method called just Standalone that took an existing server instance. That way we don't need an extra options struct, etc.

So something like

func Standalone(server *Server) error { // ... }

or even

func (s *Server) Standalone() error { /* ... */ }

hmm, thinking about this more, Standalone() does not really do anything other than inject the scheme.

It still needs the scheme though so I don't think Standalone() with no arguments will work unless we add a Scheme field to the Server struct.

Thus I could propose something like this:

func (s *Server) Standalone(scheme *runtime.Scheme) error { // Use the Kubernetes client-go scheme if none is specified if scheme == nil { scheme = kscheme.Scheme } return nil return s.InjectFunc(func(i interface{}) error { if _, err := inject.SchemeInto(scheme, i); err != nil { return err } return nil }) }

but if all it is doing is injecting the scheme, why don't we just name it InjectScheme since that's what it's doing? Is it because it's not obvious that to get a standalone server all you need to do is initialize a server struct and call InjectScheme on it?

If that is the case, then I can think of two other options:

Add scheme field to the Server struct and inject it upon calling Start. My concern here is that this would override the scheme that is set by the manager. (we could do something like only inject it in Start if Server.Scheme is not nil, but that seems like a footgun).

Instead of Standalone() modifying the server and requiring the caller to then call Start on it, we just make the call be StartStandalone(ctx, scheme) which injects the scheme and calls start for when you have a Server, but want to run it without a manager. I like this the best, but let me know what you think

Alternatively, we could just get rid of the whole standalone concept, because it's clear to me that the StandaloneWebhook is the more interesting use case and so we just tell people, if you want to run a webhook without a manager, you can either bring your own server/mux or if you really want use our server, just inject the

DirectXMan12 · 2021-03-22T22:32:01Z

pkg/webhook/server.go

+		KeyName:    options.KeyName,
+		WebhookMux: options.WebhookMux,
+	}
+	server.setDefaults()


this must be called under the defaultingOnce. That aside, why are you calling this here?

I'm dumb and forgot that it gets called on Start(). Removed

DirectXMan12 · 2021-03-22T22:34:23Z

pkg/webhook/server_test.go

@@ -174,6 +174,34 @@ var _ = Describe("Webhook Server", func() {
 			Expect(handler.injectedField).To(BeTrue())
 		})
 	})
+
+	Context("when using an unmanaged webhook server", func() {


nit: I'd just write this as it should be able to serve in unmanaged mode (no need for the context, IMO)

DirectXMan12 · 2021-03-22T22:35:22Z

pkg/webhook/webhook_integration_test.go

+		})
+	})
+	Context("when running a webhook server without a manager ", func() {
+		It("should reject create request for webhook that rejects all requests", func(done Done) {


either JustBeforeEach and BeforeEach, or use a helper method.

kevindelgado

Addressed all feedback and added examples to example_test.go. Let me know if you think there's anywhere else I should document how/why to use this.

PTAL @DirectXMan12

kevindelgado · 2021-04-07T23:05:31Z

pkg/webhook/admission/webhook.go

@@ -203,3 +208,62 @@ func (w *Webhook) InjectFunc(f inject.Func) error {

 	return setFields(w.Handler)
 }
+
+// InstrumentedHook adds some instrumentation on top of the given webhook.
+func InstrumentedHook(path string, hookRaw http.Handler) http.Handler {


kevindelgado · 2021-04-07T23:07:19Z

pkg/webhook/admission/webhook.go

+	if opts.Path == "" {
+		return hook, nil
+	}
+	return InstrumentedHook(opts.Path, hook), nil


My thinking was that there is no reason why the caller can't leave out the path and call InstrumentedHook themselves, but if they do supply a path in the options, it would be nice to do it for them?

kevindelgado · 2021-04-08T00:09:23Z

pkg/webhook/server.go

+
+// NewUnmanaged provides a webhook server that can be ran without
+// a controller manager.
+func NewUnmanaged(options Options) (*Server, error) {


hmm, thinking about this more, Standalone() does not really do anything other than inject the scheme.

It still needs the scheme though so I don't think Standalone() with no arguments will work unless we add a Scheme field to the Server struct.

Thus I could propose something like this:

func (s *Server) Standalone(scheme *runtime.Scheme) error { // Use the Kubernetes client-go scheme if none is specified if scheme == nil { scheme = kscheme.Scheme } return nil return s.InjectFunc(func(i interface{}) error { if _, err := inject.SchemeInto(scheme, i); err != nil { return err } return nil }) }

but if all it is doing is injecting the scheme, why don't we just name it InjectScheme since that's what it's doing? Is it because it's not obvious that to get a standalone server all you need to do is initialize a server struct and call InjectScheme on it?

If that is the case, then I can think of two other options:

Add scheme field to the Server struct and inject it upon calling Start. My concern here is that this would override the scheme that is set by the manager. (we could do something like only inject it in Start if Server.Scheme is not nil, but that seems like a footgun).

Instead of Standalone() modifying the server and requiring the caller to then call Start on it, we just make the call be StartStandalone(ctx, scheme) which injects the scheme and calls start for when you have a Server, but want to run it without a manager. I like this the best, but let me know what you think

Alternatively, we could just get rid of the whole standalone concept, because it's clear to me that the StandaloneWebhook is the more interesting use case and so we just tell people, if you want to run a webhook without a manager, you can either bring your own server/mux or if you really want use our server, just inject the

kevindelgado · 2021-04-08T01:01:25Z

pkg/webhook/server.go

+		KeyName:    options.KeyName,
+		WebhookMux: options.WebhookMux,
+	}
+	server.setDefaults()


I'm dumb and forgot that it gets called on Start(). Removed

kevindelgado · 2021-04-08T01:02:04Z

pkg/webhook/server_test.go

@@ -174,6 +174,34 @@ var _ = Describe("Webhook Server", func() {
 			Expect(handler.injectedField).To(BeTrue())
 		})
 	})
+
+	Context("when using an unmanaged webhook server", func() {


kevindelgado · 2021-04-08T14:39:07Z

pkg/webhook/webhook_integration_test.go

+		})
+	})
+	Context("when running a webhook server without a manager ", func() {
+		It("should reject create request for webhook that rejects all requests", func(done Done) {


Eh, now that I've changed added StartStandalone the tests feel just different enough to not make it worth using a helper or forcing deduplication. The tests just seem more readable as is, but let me know if you think there's anything you'd want to factor out.

kevindelgado · 2021-04-08T14:41:12Z

pkg/webhook/admission/webhook.go

+}
+
+// StandaloneWebhook transforms a Webhook that needs to be registered
+// on a webhook.Server into one that can be ran on any arbitrary mux.


sounds good

stevekuznetsov · 2021-04-21T21:53:43Z

@kevindelgado 👋

DirectXMan12

/lgtm
/approve

DirectXMan12 · 2021-04-21T23:22:41Z

pkg/webhook/webhook_integration_test.go

+			Expect(err).NotTo(HaveOccurred())
+			http.Handle("/failing", hook)
+
+			// run the http server


in the future, use By("running the http server") instead of just comments like this. No need to fix this now though

k8s-ci-robot · 2021-04-21T23:25:55Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

stevekuznetsov · 2021-04-22T17:13:59Z

pkg/webhook/admission/webhook.go

+	}
+
+	var err error
+	hook.decoder, err = NewDecoder(opts.Scheme)


@kevindelgado what actually injects this into the hook.Handler?

🤦 It doesn't... because I'm dumb

See #1490

fyi, fixed now @stevekuznetsov

stevekuznetsov · 2021-04-24T22:47:12Z

pkg/webhook/example_test.go

+	hookServer.Register("/validating", validatingHook)
+
+	// Start the server without a manger
+	err := hookServer.StartStandalone(signals.SetupSignalHandler(), scheme.Scheme)


@kevindelgado one more thing - maybe we could write an e2e for examples to help catch these, but it looks like the cert and key are not optional on this server? When using code similar to this, I get:

time="2021-04-23T20:34:54Z" level=error msg="Failed to serve admission webhooks." error="open /tmp/k8s-webhook-server/serving-certs/tls.crt: no such file or directory"

that's intentional -- same with the non-standalone server. More broadly, the main point of the webhook server is to help with managing TLS setup properly. If you don't want TLS, use StandaloneWebhook.

We could certainly document that better, though.

kevindelgado force-pushed the standalone-webhooks branch from 4f05a36 to 1c41197 Compare March 16, 2021 01:19

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 16, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 16, 2021

k8s-ci-robot requested review from alvaroaleman and shawn-hurley March 16, 2021 01:24

k8s-ci-robot assigned DirectXMan12 Mar 16, 2021

kevindelgado commented Mar 16, 2021

View reviewed changes

kevindelgado force-pushed the standalone-webhooks branch from 1c41197 to 5e23ce1 Compare March 17, 2021 18:17

kevindelgado commented Mar 22, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2021

kevindelgado added 2 commits March 22, 2021 21:38

Simple helper for unmanaged webhook server

83846f5

StandaloneWebhook can run on any arbitrary mux

2c24442

kevindelgado force-pushed the standalone-webhooks branch from 4b6013a to 2c24442 Compare March 22, 2021 21:41

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 22, 2021

DirectXMan12 suggested changes Mar 22, 2021

View reviewed changes

kevindelgado added 3 commits April 8, 2021 00:27

move InstrumentedHook to metrics package

12c19f7

Update tests and docs for StandaloneServer

9a73ff4

Add examples

4dc10f9

kevindelgado commented Apr 8, 2021

View reviewed changes

kevindelgado requested a review from DirectXMan12 April 8, 2021 16:07

alvaroaleman removed their request for review April 8, 2021 17:06

kevindelgado force-pushed the standalone-webhooks branch from 2eb0aad to 39309dd Compare April 8, 2021 17:07

fix CI

5a5106d

kevindelgado force-pushed the standalone-webhooks branch from 39309dd to 5a5106d Compare April 8, 2021 17:19

stevekuznetsov mentioned this pull request Apr 21, 2021

pod-scaler: add labels to Build Pods via admission openshift/ci-tools#1895

Merged

DirectXMan12 approved these changes Apr 21, 2021

View reviewed changes

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

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

k8s-ci-robot merged commit d5d2551 into kubernetes-sigs:master Apr 21, 2021

k8s-ci-robot added this to the v0.9.x milestone Apr 21, 2021

stevekuznetsov reviewed Apr 22, 2021

View reviewed changes

kevindelgado mentioned this pull request Apr 23, 2021

🐛 Actually inject the scheme in StandaloneWebhook #1490

Merged

stevekuznetsov reviewed Apr 24, 2021

View reviewed changes

kevindelgado mentioned this pull request Apr 29, 2021

📖 Better document TLS requirements for webhook servers #1501

Merged

kevindelgado changed the title ~~✨ Simple helper for unmanaged webhook server~~ ✨ Helper for unmanaged webhook server Aug 16, 2021

✨ Helper for unmanaged webhook server #1429

✨ Helper for unmanaged webhook server #1429

Conversation

kevindelgado commented Mar 16, 2021 • edited

kevindelgado commented Mar 16, 2021

k8s-ci-robot commented Mar 16, 2021

kevindelgado commented Mar 16, 2021

kevindelgado 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

kevindelgado 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

rohitagarwal003 commented Mar 22, 2021

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

DirectXMan12 Mar 22, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindelgado 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

stevekuznetsov commented Apr 21, 2021

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 21, 2021

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 Apr 29, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindelgado commented Mar 16, 2021 •

edited

DirectXMan12 Mar 22, 2021 •

edited

DirectXMan12 Apr 29, 2021 •

edited