New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ Helper for unmanaged webhook server #1429
✨ Helper for unmanaged webhook server #1429
Conversation
4f05a36
to
1c41197
Compare
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. |
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 Once the patch is verified, the new status will be reflected by the 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. |
/assign @DirectXMan12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some questions inline PTAL @DirectXMan12
pkg/webhook/server.go
Outdated
|
||
// NewUnmanaged provides a webhook server that can be ran without | ||
// a controller manager. | ||
func NewUnmanaged(cluster cluster.Cluster, options Options) (*Server, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
}) | ||
}) | ||
Context("when running a webhook server without a manager ", func() { | ||
It("should reject create request for webhook that rejects all requests", func(done Done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either JustBeforeEach
and BeforeEach
, or use a helper method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
1c41197
to
5e23ce1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added StandaloneWebhook
helper for attach c-r webhooks to an existing arbitrary http.ServeMux (part two of #1255 (comment)). PTAL @DirectXMan12
pkg/webhook/admission/webhook.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this applies to all webhooks, not just admission ones, so it belongs in pkg/webhook
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or you could put it in a subpackage to avoid import loops, if necessary, like pkg/webhook/internal/metrics
or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved
}) | ||
}) | ||
Context("when running a standalone webhook", func() { | ||
It("should reject create request for webhook that rejects all requests", func(done Done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Can you also add some docs on who to use this? It's hard to discover these without these being in the docs. |
4b6013a
to
2c24442
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pkg/webhook/admission/webhook.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this applies to all webhooks, not just admission ones, so it belongs in pkg/webhook
.
pkg/webhook/admission/webhook.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or you could put it in a subpackage to avoid import loops, if necessary, like pkg/webhook/internal/metrics
or something
pkg/webhook/admission/webhook.go
Outdated
} | ||
|
||
// StandaloneWebhook transforms a Webhook that needs to be registered | ||
// on a webhook.Server into one that can be ran on any arbitrary mux. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This documentation is technically correct but slightly unclear. I'd
- Update the documentation on
Webhook
to mention that it needs to be registered with awebhook.Server
or populated byStandaloneWebhook
- 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
pkg/webhook/admission/webhook.go
Outdated
if opts.Path == "" { | ||
return hook, nil | ||
} | ||
return InstrumentedHook(opts.Path, hook), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
pkg/webhook/server.go
Outdated
|
||
// NewUnmanaged provides a webhook server that can be ran without | ||
// a controller manager. | ||
func NewUnmanaged(options Options) (*Server, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: rename to NewStandaloneServer
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 {
// ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or even
func (s *Server) Standalone() error { /* ... */ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 callingStart
. My concern here is that this would override the scheme that is set by the manager. (we could do something like only inject it inStart
ifServer.Scheme
is not nil, but that seems like a footgun). - Instead of
Standalone()
modifying the server and requiring the caller to then callStart
on it, we just make the call beStartStandalone(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
pkg/webhook/server.go
Outdated
KeyName: options.KeyName, | ||
WebhookMux: options.WebhookMux, | ||
} | ||
server.setDefaults() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this must be called under the defaultingOnce
. That aside, why are you calling this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm dumb and forgot that it gets called on Start()
. Removed
pkg/webhook/server_test.go
Outdated
@@ -174,6 +174,34 @@ var _ = Describe("Webhook Server", func() { | |||
Expect(handler.injectedField).To(BeTrue()) | |||
}) | |||
}) | |||
|
|||
Context("when using an unmanaged webhook server", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'd just write this as it should be able to serve in unmanaged mode
(no need for the context, IMO)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
}) | ||
}) | ||
Context("when running a webhook server without a manager ", func() { | ||
It("should reject create request for webhook that rejects all requests", func(done Done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either JustBeforeEach
and BeforeEach
, or use a helper method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
pkg/webhook/admission/webhook.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved
pkg/webhook/admission/webhook.go
Outdated
if opts.Path == "" { | ||
return hook, nil | ||
} | ||
return InstrumentedHook(opts.Path, hook), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
pkg/webhook/server.go
Outdated
|
||
// NewUnmanaged provides a webhook server that can be ran without | ||
// a controller manager. | ||
func NewUnmanaged(options Options) (*Server, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 callingStart
. My concern here is that this would override the scheme that is set by the manager. (we could do something like only inject it inStart
ifServer.Scheme
is not nil, but that seems like a footgun). - Instead of
Standalone()
modifying the server and requiring the caller to then callStart
on it, we just make the call beStartStandalone(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
pkg/webhook/server.go
Outdated
KeyName: options.KeyName, | ||
WebhookMux: options.WebhookMux, | ||
} | ||
server.setDefaults() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm dumb and forgot that it gets called on Start()
. Removed
pkg/webhook/server_test.go
Outdated
@@ -174,6 +174,34 @@ var _ = Describe("Webhook Server", func() { | |||
Expect(handler.injectedField).To(BeTrue()) | |||
}) | |||
}) | |||
|
|||
Context("when using an unmanaged webhook server", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
}) | ||
}) | ||
Context("when running a webhook server without a manager ", func() { | ||
It("should reject create request for webhook that rejects all requests", func(done Done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
pkg/webhook/admission/webhook.go
Outdated
} | ||
|
||
// StandaloneWebhook transforms a Webhook that needs to be registered | ||
// on a webhook.Server into one that can be ran on any arbitrary mux. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good
2eb0aad
to
39309dd
Compare
39309dd
to
5a5106d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
Expect(err).NotTo(HaveOccurred()) | ||
http.Handle("/failing", hook) | ||
|
||
// run the http server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the future, use By("running the http server")
instead of just comments like this. No need to fix this now though
[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:
Approvers can indicate their approval by writing |
} | ||
|
||
var err error | ||
hook.decoder, err = NewDecoder(opts.Scheme) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kevindelgado what actually injects this into the hook.Handler
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤦 It doesn't... because I'm dumb
See #1490
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi, fixed now @stevekuznetsov
hookServer.Register("/validating", validatingHook) | ||
|
||
// Start the server without a manger | ||
err := hookServer.StartStandalone(signals.SetupSignalHandler(), scheme.Scheme) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds helper functions to setup webhooks server without a manager. It adds
webhook.Unmanaged
that creates awebhook.Server
without a manager / cluster connection informationadmission.StandalonWebhook
that creates anhttp.Handler
that can be run on any arbitrary Go HTTP server (http.SeverMux
) rather than awebhook.Server
)Fixes #1255