Skip to content
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.

Sign up for GitHub

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

Jump to bottom

✨ Helper for unmanaged webhook server #1429

Merged
merged 6 commits into from Apr 21, 2021
Merged

✨ Helper for unmanaged webhook server #1429

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
83846f5
Simple helper for unmanaged webhook server
kevindelgado Mar 12, 2021
2c24442
StandaloneWebhook can run on any arbitrary mux
kevindelgado Mar 20, 2021
12c19f7
move InstrumentedHook to metrics package
kevindelgado Apr 8, 2021
9a73ff4
Update tests and docs for StandaloneServer
kevindelgado Apr 8, 2021
4dc10f9
Add examples
kevindelgado Apr 8, 2021
5a5106d
fix CI
kevindelgado Apr 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
64 changes: 64 additions & 0 deletions pkg/webhook/admission/webhook.go
View file
Open in desktop
Expand Up @@ -22,13 +22,18 @@ import (
"net/http"

"github.com/go-logr/logr"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promhttp"
jsonpatch "gomodules.xyz/jsonpatch/v2"
admissionv1 "k8s.io/api/admission/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/json"
"k8s.io/client-go/kubernetes/scheme"

logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
"sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics"
)

var (
Expand Down Expand Up @@ -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 {
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved

lbl := prometheus.Labels{"webhook": path}

lat := metrics.RequestLatency.MustCurryWith(lbl)
cnt := metrics.RequestTotal.MustCurryWith(lbl)
gge := metrics.RequestInFlight.With(lbl)

// Initialize the most likely HTTP status codes.
cnt.WithLabelValues("200")
cnt.WithLabelValues("500")

return promhttp.InstrumentHandlerDuration(
lat,
promhttp.InstrumentHandlerCounter(
cnt,
promhttp.InstrumentHandlerInFlight(gge, hookRaw),
),
)
}

// StandaloneOptions let you configure a StandaloneWebhook.
type StandaloneOptions struct {
// Scheme is the scheme used to resolve runtime.Objects to GroupVersionKinds / Resources
// Defaults to the kubernetes/client-go scheme.Scheme, but it's almost always better
// idea to pass your own scheme in. See the documentation in pkg/scheme for more information.
Scheme *runtime.Scheme
// Logger to be used by the webhook.
// If none is set, it defaults to log.Log global logger.
Logger logr.Logger
// Path the webhook will be served at.
// Used for labelling prometheus metrics.
Path string
}

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

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

  1. Update the documentation on Webhook to mention that it needs to be registered with a webhook.Server or populated by StandaloneWebhook
  2. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

func StandaloneWebhook(hook *Webhook, opts StandaloneOptions) (http.Handler, error) {
if opts.Scheme == nil {
opts.Scheme = scheme.Scheme
}

var err error
hook.decoder, err = NewDecoder(opts.Scheme)
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi, fixed now @stevekuznetsov

if err != nil {
return nil, err
}

if opts.Logger == nil {
opts.Logger = logf.RuntimeLog.WithName("webhook")
}
hook.log = opts.Logger

if opts.Path == "" {
return hook, nil
}
return InstrumentedHook(opts.Path, hook), nil
Copy link
Contributor

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.

Copy link
Contributor Author

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?

}
90 changes: 65 additions & 25 deletions pkg/webhook/server.go
View file
Open in desktop
Expand Up @@ -29,11 +29,11 @@ import (
"strconv"
"sync"

"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promhttp"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes/scheme"
"sigs.k8s.io/controller-runtime/pkg/certwatcher"
"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
"sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics"
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
)

// DefaultPort is the default port that the webhook server serves.
Expand Down Expand Up @@ -105,6 +105,67 @@ func (s *Server) setDefaults() {
}
}

// Options are the subset of fields on the controller that can be
// configured when running an unmanaged webhook server (i.e. webhook.NewUnmanaged())
type Options struct {
// Host is the address that the server will listen on.
// Defaults to "" - all addresses.
Host string

// Port is the port number that the server will serve.
// It will be defaulted to 9443 if unspecified.
Port int

// CertDir is the directory that contains the server key and certificate. The
// server key and certificate.
CertDir string

// CertName is the server certificate name. Defaults to tls.crt.
CertName string

// KeyName is the server key name. Defaults to tls.key.
KeyName string

// ClientCAName is the CA certificate name which server used to verify remote(client)'s certificate.
// Defaults to "", which means server does not verify client's certificate.
ClientCAName string

// WebhookMux is the multiplexer that handles different webhooks.
WebhookMux *http.ServeMux

// Scheme is the scheme used to resolve runtime.Objects to GroupVersionKinds / Resources
// Defaults to the kubernetes/client-go scheme.Scheme, but it's almost always better
// idea to pass your own scheme in. See the documentation in pkg/scheme for more information.
Scheme *runtime.Scheme
}

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

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?

Copy link
Contributor

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 {
  // ...
}

Copy link
Contributor

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 { /* ... */ }

Copy link
Contributor Author

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:

  1. 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).
  2. 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

server := &Server{
Host: options.Host,
Port: options.Port,
CertDir: options.CertDir,
CertName: options.CertName,
KeyName: options.KeyName,
WebhookMux: options.WebhookMux,
}
server.setDefaults()
Copy link
Contributor

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?

Copy link
Contributor Author

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

// Use the Kubernetes client-go scheme if none is specified
if options.Scheme == nil {
options.Scheme = scheme.Scheme
}

// TODO: can we do this without dep injection?
server.InjectFunc(func(i interface{}) error {
if _, err := inject.SchemeInto(options.Scheme, i); err != nil {
return err
}
return nil
})
return server, nil
}

// NeedLeaderElection implements the LeaderElectionRunnable interface, which indicates
// the webhook server doesn't need leader election.
func (*Server) NeedLeaderElection() bool {
Expand All @@ -124,7 +185,7 @@ func (s *Server) Register(path string, hook http.Handler) {
}
// TODO(directxman12): call setfields if we've already started the server
s.webhooks[path] = hook
s.WebhookMux.Handle(path, instrumentedHook(path, hook))
s.WebhookMux.Handle(path, admission.InstrumentedHook(path, hook))

regLog := log.WithValues("path", path)
regLog.Info("registering webhook")
Expand All @@ -149,27 +210,6 @@ func (s *Server) Register(path string, hook http.Handler) {
}
}

// instrumentedHook adds some instrumentation on top of the given webhook.
func instrumentedHook(path string, hookRaw http.Handler) http.Handler {
lbl := prometheus.Labels{"webhook": path}

lat := metrics.RequestLatency.MustCurryWith(lbl)
cnt := metrics.RequestTotal.MustCurryWith(lbl)
gge := metrics.RequestInFlight.With(lbl)

// Initialize the most likely HTTP status codes.
cnt.WithLabelValues("200")
cnt.WithLabelValues("500")

return promhttp.InstrumentHandlerDuration(
lat,
promhttp.InstrumentHandlerCounter(
cnt,
promhttp.InstrumentHandlerInFlight(gge, hookRaw),
),
)
}

// Start runs the server.
// It will install the webhook related resources depend on the server configuration.
func (s *Server) Start(ctx context.Context) error {
Expand Down
28 changes: 28 additions & 0 deletions pkg/webhook/server_test.go
View file
Open in desktop
Expand Up @@ -174,6 +174,34 @@ var _ = Describe("Webhook Server", func() {
Expect(handler.injectedField).To(BeTrue())
})
})

Context("when using an unmanaged webhook server", func() {
Copy link
Contributor

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

It("should serve a webhook on the requested path", func() {
opts := webhook.Options{
Host: servingOpts.LocalServingHost,
Port: servingOpts.LocalServingPort,
CertDir: servingOpts.LocalServingCertDir,
}
var err error
// overwrite the server so that startServer() starts it
server, err = webhook.NewUnmanaged(opts)

Expect(err).NotTo(HaveOccurred())
server.Register("/somepath", &testHandler{})
doneCh := startServer()

Eventually(func() ([]byte, error) {
resp, err := client.Get(fmt.Sprintf("https://%s/somepath", testHostPort))
Expect(err).NotTo(HaveOccurred())
defer resp.Body.Close()
return ioutil.ReadAll(resp.Body)
}).Should(Equal([]byte("gadzooks!")))

ctxCancel()
Eventually(doneCh, "4s").Should(BeClosed())
})

})
})

type testHandler struct {
Expand Down