From 83846f53a11cda25e9ccf32e78e8891a2d929944 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Fri, 12 Mar 2021 23:49:10 +0000 Subject: [PATCH 1/6] Simple helper for unmanaged webhook server --- pkg/webhook/server.go | 63 +++++++++++++ pkg/webhook/server_test.go | 28 ++++++ pkg/webhook/webhook_integration_test.go | 113 ++++++++++++++++++++++++ pkg/webhook/webhook_suite_test.go | 68 ++++++++++++++ 4 files changed, 272 insertions(+) create mode 100644 pkg/webhook/webhook_integration_test.go diff --git a/pkg/webhook/server.go b/pkg/webhook/server.go index 9fefc9a697..514412ffb1 100644 --- a/pkg/webhook/server.go +++ b/pkg/webhook/server.go @@ -31,6 +31,8 @@ import ( "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" @@ -105,6 +107,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) { + server := &Server{ + Host: options.Host, + Port: options.Port, + CertDir: options.CertDir, + CertName: options.CertName, + KeyName: options.KeyName, + WebhookMux: options.WebhookMux, + } + server.setDefaults() + // 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 { diff --git a/pkg/webhook/server_test.go b/pkg/webhook/server_test.go index eccc438ef4..75801b0a25 100644 --- a/pkg/webhook/server_test.go +++ b/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() { + 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 { diff --git a/pkg/webhook/webhook_integration_test.go b/pkg/webhook/webhook_integration_test.go new file mode 100644 index 0000000000..0aa5207eb1 --- /dev/null +++ b/pkg/webhook/webhook_integration_test.go @@ -0,0 +1,113 @@ +package webhook_test + +import ( + "context" + "fmt" + "time" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" +) + +var _ = Describe("Webhook", func() { + var c client.Client + var obj *appsv1.Deployment + BeforeEach(func() { + Expect(cfg).NotTo(BeNil()) + var err error + c, err = client.New(cfg, client.Options{}) + Expect(err).NotTo(HaveOccurred()) + + obj = &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apps/v1", + Kind: "Deployment", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deployment", + Namespace: "default", + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"foo": "bar"}, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "nginx", + Image: "nginx", + }, + }, + }, + }, + }, + } + }) + Context("when running a webhook server with a manager", func() { + It("should reject create request for webhook that rejects all requests", func(done Done) { + m, err := manager.New(cfg, manager.Options{ + Port: testenv.WebhookInstallOptions.LocalServingPort, + Host: testenv.WebhookInstallOptions.LocalServingHost, + CertDir: testenv.WebhookInstallOptions.LocalServingCertDir, + }) // we need manager here just to leverage manager.SetFields + Expect(err).NotTo(HaveOccurred()) + server := m.GetWebhookServer() + server.Register("/failing", &webhook.Admission{Handler: &rejectingValidator{}}) + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + _ = server.Start(ctx) + }() + + Eventually(func() bool { + err = c.Create(context.TODO(), obj) + return errors.ReasonForError(err) == metav1.StatusReason("Always denied") + }, 1*time.Second).Should(BeTrue()) + + cancel() + close(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) { + opts := webhook.Options{ + Port: testenv.WebhookInstallOptions.LocalServingPort, + Host: testenv.WebhookInstallOptions.LocalServingHost, + CertDir: testenv.WebhookInstallOptions.LocalServingCertDir, + } + server, err := webhook.NewUnmanaged(opts) + Expect(err).NotTo(HaveOccurred()) + server.Register("/failing", &webhook.Admission{Handler: &rejectingValidator{}}) + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + _ = server.Start(ctx) + }() + + Eventually(func() bool { + err = c.Create(context.TODO(), obj) + return errors.ReasonForError(err) == metav1.StatusReason("Always denied") + }, 1*time.Second).Should(BeTrue()) + + cancel() + close(done) + }) + }) +}) + +type rejectingValidator struct { +} + +func (v *rejectingValidator) Handle(ctx context.Context, req admission.Request) admission.Response { + return admission.Denied(fmt.Sprint("Always denied")) +} diff --git a/pkg/webhook/webhook_suite_test.go b/pkg/webhook/webhook_suite_test.go index d921493f0e..fb2b02f195 100644 --- a/pkg/webhook/webhook_suite_test.go +++ b/pkg/webhook/webhook_suite_test.go @@ -17,11 +17,17 @@ limitations under the License. package webhook_test import ( + "fmt" "testing" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + admissionv1 "k8s.io/api/admissionregistration/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" "sigs.k8s.io/controller-runtime/pkg/envtest/printer" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -33,8 +39,70 @@ func TestSource(t *testing.T) { RunSpecsWithDefaultAndCustomReporters(t, suiteName, []Reporter{printer.NewlineReporter{}, printer.NewProwReporter(suiteName)}) } +var testenv *envtest.Environment +var cfg *rest.Config + var _ = BeforeSuite(func(done Done) { logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + testenv = &envtest.Environment{} + // we're initializing webhook here and not in webhook.go to also test the envtest install code via WebhookOptions + initializeWebhookInEnvironment() + var err error + cfg, err = testenv.Start() + Expect(err).NotTo(HaveOccurred()) close(done) }, 60) + +var _ = AfterSuite(func() { + fmt.Println("stopping?") + Expect(testenv.Stop()).To(Succeed()) +}, 60) + +func initializeWebhookInEnvironment() { + namespacedScopeV1 := admissionv1.NamespacedScope + failedTypeV1 := admissionv1.Fail + equivalentTypeV1 := admissionv1.Equivalent + noSideEffectsV1 := admissionv1.SideEffectClassNone + webhookPathV1 := "/failing" + + testenv.WebhookInstallOptions = envtest.WebhookInstallOptions{ + ValidatingWebhooks: []client.Object{ + &admissionv1.ValidatingWebhookConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "deployment-validation-webhook-config", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ValidatingWebhookConfiguration", + APIVersion: "admissionregistration.k8s.io/v1beta1", + }, + Webhooks: []admissionv1.ValidatingWebhook{ + { + Name: "deployment-validation.kubebuilder.io", + Rules: []admissionv1.RuleWithOperations{ + { + Operations: []admissionv1.OperationType{"CREATE", "UPDATE"}, + Rule: admissionv1.Rule{ + APIGroups: []string{"apps"}, + APIVersions: []string{"v1"}, + Resources: []string{"deployments"}, + Scope: &namespacedScopeV1, + }, + }, + }, + FailurePolicy: &failedTypeV1, + MatchPolicy: &equivalentTypeV1, + SideEffects: &noSideEffectsV1, + ClientConfig: admissionv1.WebhookClientConfig{ + Service: &admissionv1.ServiceReference{ + Name: "deployment-validation-service", + Namespace: "default", + Path: &webhookPathV1, + }, + }, + }, + }, + }, + }, + } +} From 2c244428ef632d2f521e419d4ef2981374e587fd Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Sat, 20 Mar 2021 01:00:26 +0000 Subject: [PATCH 2/6] StandaloneWebhook can run on any arbitrary mux --- pkg/webhook/admission/webhook.go | 64 +++++++++++++++++++++++++ pkg/webhook/server.go | 27 +---------- pkg/webhook/webhook_integration_test.go | 57 +++++++++++++++++++++- 3 files changed, 122 insertions(+), 26 deletions(-) diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index 4430c3132c..41de16b9a7 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -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 ( @@ -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 { + 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. +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) + 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 +} diff --git a/pkg/webhook/server.go b/pkg/webhook/server.go index 514412ffb1..987a3c3c1b 100644 --- a/pkg/webhook/server.go +++ b/pkg/webhook/server.go @@ -29,13 +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. @@ -187,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") @@ -212,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 { diff --git a/pkg/webhook/webhook_integration_test.go b/pkg/webhook/webhook_integration_test.go index 0aa5207eb1..424d263514 100644 --- a/pkg/webhook/webhook_integration_test.go +++ b/pkg/webhook/webhook_integration_test.go @@ -2,7 +2,12 @@ package webhook_test import ( "context" + "crypto/tls" "fmt" + "net" + "net/http" + "path/filepath" + "strconv" "time" . "github.com/onsi/ginkgo" @@ -11,6 +16,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/certwatcher" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -78,7 +84,7 @@ var _ = Describe("Webhook", func() { close(done) }) }) - Context("when running a webhook server without a manager ", func() { + Context("when running a webhook server without a manager", func() { It("should reject create request for webhook that rejects all requests", func(done Done) { opts := webhook.Options{ Port: testenv.WebhookInstallOptions.LocalServingPort, @@ -99,6 +105,55 @@ var _ = Describe("Webhook", func() { return errors.ReasonForError(err) == metav1.StatusReason("Always denied") }, 1*time.Second).Should(BeTrue()) + cancel() + close(done) + }) + }) + Context("when running a standalone webhook", func() { + It("should reject create request for webhook that rejects all requests", func(done Done) { + ctx, cancel := context.WithCancel(context.Background()) + // generate tls cfg + certPath := filepath.Join(testenv.WebhookInstallOptions.LocalServingCertDir, "tls.crt") + keyPath := filepath.Join(testenv.WebhookInstallOptions.LocalServingCertDir, "tls.key") + + certWatcher, err := certwatcher.New(certPath, keyPath) + Expect(err).NotTo(HaveOccurred()) + go func() { + Expect(certWatcher.Start(ctx)).NotTo(HaveOccurred()) + }() + + cfg := &tls.Config{ + NextProtos: []string{"h2"}, + GetCertificate: certWatcher.GetCertificate, + } + + // generate listener + listener, err := tls.Listen("tcp", net.JoinHostPort(testenv.WebhookInstallOptions.LocalServingHost, strconv.Itoa(int(testenv.WebhookInstallOptions.LocalServingPort))), cfg) + Expect(err).NotTo(HaveOccurred()) + + // create and register the standalone webhook + hook, err := admission.StandaloneWebhook(&webhook.Admission{Handler: &rejectingValidator{}}, admission.StandaloneOptions{}) + Expect(err).NotTo(HaveOccurred()) + http.Handle("/failing", hook) + + // run the http server + srv := &http.Server{} + go func() { + idleConnsClosed := make(chan struct{}) + go func() { + <-ctx.Done() + Expect(srv.Shutdown(context.Background())).NotTo(HaveOccurred()) + close(idleConnsClosed) + }() + srv.Serve(listener) + <-idleConnsClosed + }() + + Eventually(func() bool { + err = c.Create(context.TODO(), obj) + return errors.ReasonForError(err) == metav1.StatusReason("Always denied") + }, 1*time.Second).Should(BeTrue()) + cancel() close(done) }) From 12c19f76346c562d791de98786efd076c57463ff Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Thu, 8 Apr 2021 00:27:38 +0000 Subject: [PATCH 3/6] move InstrumentedHook to metrics package --- pkg/webhook/admission/webhook.go | 25 +------------------------ pkg/webhook/internal/metrics/metrics.go | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index 41de16b9a7..c832e5fadc 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -22,8 +22,6 @@ 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" @@ -209,27 +207,6 @@ 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 { - 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 @@ -265,5 +242,5 @@ func StandaloneWebhook(hook *Webhook, opts StandaloneOptions) (http.Handler, err if opts.Path == "" { return hook, nil } - return InstrumentedHook(opts.Path, hook), nil + return metrics.InstrumentedHook(opts.Path, hook), nil } diff --git a/pkg/webhook/internal/metrics/metrics.go b/pkg/webhook/internal/metrics/metrics.go index a29643b244..557004908b 100644 --- a/pkg/webhook/internal/metrics/metrics.go +++ b/pkg/webhook/internal/metrics/metrics.go @@ -17,7 +17,10 @@ limitations under the License. package metrics import ( + "net/http" + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promhttp" "sigs.k8s.io/controller-runtime/pkg/metrics" ) @@ -59,3 +62,24 @@ var ( func init() { metrics.Registry.MustRegister(RequestLatency, RequestTotal, RequestInFlight) } + +// 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 := RequestLatency.MustCurryWith(lbl) + cnt := RequestTotal.MustCurryWith(lbl) + gge := 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), + ), + ) +} From 9a73ff46d6813eef3258304c803290ea581a8143 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Thu, 8 Apr 2021 14:44:33 +0000 Subject: [PATCH 4/6] Update tests and docs for StandaloneServer --- pkg/webhook/admission/webhook.go | 10 ++- pkg/webhook/server.go | 87 +++++++------------------ pkg/webhook/server_test.go | 51 ++++++++------- pkg/webhook/webhook_integration_test.go | 9 ++- 4 files changed, 61 insertions(+), 96 deletions(-) diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index c832e5fadc..01c7c142a3 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -113,6 +113,9 @@ func (f HandlerFunc) Handle(ctx context.Context, req Request) Response { } // Webhook represents each individual webhook. +// +// It must be registered with a webhook.Server or +// populated by StandaloneWebhook to be ran on an arbitrary HTTP server. type Webhook struct { // Handler actually processes an admission request returning whether it was allowed or denied, // and potentially patches to apply to the handler. @@ -221,8 +224,11 @@ type StandaloneOptions struct { 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. +// 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. func StandaloneWebhook(hook *Webhook, opts StandaloneOptions) (http.Handler, error) { if opts.Scheme == nil { opts.Scheme = scheme.Scheme diff --git a/pkg/webhook/server.go b/pkg/webhook/server.go index 987a3c3c1b..94a8fda54f 100644 --- a/pkg/webhook/server.go +++ b/pkg/webhook/server.go @@ -30,10 +30,10 @@ import ( "sync" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/kubernetes/scheme" + kscheme "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/admission" + "sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics" ) // DefaultPort is the default port that the webhook server serves. @@ -105,67 +105,6 @@ 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) { - server := &Server{ - Host: options.Host, - Port: options.Port, - CertDir: options.CertDir, - CertName: options.CertName, - KeyName: options.KeyName, - WebhookMux: options.WebhookMux, - } - server.setDefaults() - // 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 { @@ -185,7 +124,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, admission.InstrumentedHook(path, hook)) + s.WebhookMux.Handle(path, metrics.InstrumentedHook(path, hook)) regLog := log.WithValues("path", path) regLog.Info("registering webhook") @@ -210,6 +149,26 @@ func (s *Server) Register(path string, hook http.Handler) { } } +// StartStandalone runs a webhook server without +// a controller manager. +func (s *Server) StartStandalone(ctx context.Context, scheme *runtime.Scheme) error { + // Use the Kubernetes client-go scheme if none is specified + if scheme == nil { + scheme = kscheme.Scheme + } + + if err := s.InjectFunc(func(i interface{}) error { + if _, err := inject.SchemeInto(scheme, i); err != nil { + return err + } + return nil + }); err != nil { + return err + } + + return s.Start(ctx) +} + // Start runs the server. // It will install the webhook related resources depend on the server configuration. func (s *Server) Start(ctx context.Context) error { diff --git a/pkg/webhook/server_test.go b/pkg/webhook/server_test.go index 75801b0a25..1426b4dec9 100644 --- a/pkg/webhook/server_test.go +++ b/pkg/webhook/server_test.go @@ -25,6 +25,7 @@ import ( . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/envtest" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -68,12 +69,12 @@ var _ = Describe("Webhook Server", func() { Expect(servingOpts.Cleanup()).To(Succeed()) }) - startServer := func() (done <-chan struct{}) { + genericStartServer := func(f func(ctx context.Context)) (done <-chan struct{}) { doneCh := make(chan struct{}) go func() { defer GinkgoRecover() defer close(doneCh) - Expect(server.Start(ctx)).To(Succeed()) + f(ctx) }() // wait till we can ping the server to start the test Eventually(func() error { @@ -93,6 +94,12 @@ var _ = Describe("Webhook Server", func() { return doneCh } + startServer := func() (done <-chan struct{}) { + return genericStartServer(func(ctx context.Context) { + Expect(server.Start(ctx)).To(Succeed()) + }) + } + // TODO(directxman12): figure out a good way to test all the serving setup // with httptest.Server to get all the niceness from that. @@ -175,32 +182,26 @@ var _ = Describe("Webhook Server", func() { }) }) - Context("when using an unmanaged webhook server", func() { - 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) + It("should serve be able to serve in unmanaged mode", func() { + server = &webhook.Server{ + Host: servingOpts.LocalServingHost, + Port: servingOpts.LocalServingPort, + CertDir: servingOpts.LocalServingCertDir, + } + server.Register("/somepath", &testHandler{}) + doneCh := genericStartServer(func(ctx context.Context) { + Expect(server.StartStandalone(ctx, scheme.Scheme)) + }) + Eventually(func() ([]byte, error) { + resp, err := client.Get(fmt.Sprintf("https://%s/somepath", testHostPort)) 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()) - }) + defer resp.Body.Close() + return ioutil.ReadAll(resp.Body) + }).Should(Equal([]byte("gadzooks!"))) + ctxCancel() + Eventually(doneCh, "4s").Should(BeClosed()) }) }) diff --git a/pkg/webhook/webhook_integration_test.go b/pkg/webhook/webhook_integration_test.go index 424d263514..e2a1d2b3b3 100644 --- a/pkg/webhook/webhook_integration_test.go +++ b/pkg/webhook/webhook_integration_test.go @@ -16,6 +16,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/certwatcher" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/manager" @@ -86,22 +87,20 @@ var _ = Describe("Webhook", func() { }) Context("when running a webhook server without a manager", func() { It("should reject create request for webhook that rejects all requests", func(done Done) { - opts := webhook.Options{ + server := webhook.Server{ Port: testenv.WebhookInstallOptions.LocalServingPort, Host: testenv.WebhookInstallOptions.LocalServingHost, CertDir: testenv.WebhookInstallOptions.LocalServingCertDir, } - server, err := webhook.NewUnmanaged(opts) - Expect(err).NotTo(HaveOccurred()) server.Register("/failing", &webhook.Admission{Handler: &rejectingValidator{}}) ctx, cancel := context.WithCancel(context.Background()) go func() { - _ = server.Start(ctx) + _ = server.StartStandalone(ctx, scheme.Scheme) }() Eventually(func() bool { - err = c.Create(context.TODO(), obj) + err := c.Create(context.TODO(), obj) return errors.ReasonForError(err) == metav1.StatusReason("Always denied") }, 1*time.Second).Should(BeTrue()) From 4dc10f984de1d23ac97cdd2434bbd5cf44fd1395 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Thu, 8 Apr 2021 16:01:07 +0000 Subject: [PATCH 5/6] Add examples --- pkg/webhook/admission/webhook.go | 11 +++-- pkg/webhook/example_test.go | 85 ++++++++++++++++++++++++++++++-- 2 files changed, 87 insertions(+), 9 deletions(-) diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index 01c7c142a3..f0db50be81 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -219,9 +219,10 @@ type StandaloneOptions struct { // 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 + // MetricsPath is used for labelling prometheus metrics + // by the path is served on. + // If none is set, prometheus metrics will not be generated. + MetricsPath string } // StandaloneWebhook prepares a webhook for use without a webhook.Server, @@ -245,8 +246,8 @@ func StandaloneWebhook(hook *Webhook, opts StandaloneOptions) (http.Handler, err } hook.log = opts.Logger - if opts.Path == "" { + if opts.MetricsPath == "" { return hook, nil } - return metrics.InstrumentedHook(opts.Path, hook), nil + return metrics.InstrumentedHook(opts.MetricsPath, hook), nil } diff --git a/pkg/webhook/example_test.go b/pkg/webhook/example_test.go index b225fea89b..0deb382324 100644 --- a/pkg/webhook/example_test.go +++ b/pkg/webhook/example_test.go @@ -18,18 +18,24 @@ package webhook_test import ( "context" + "net/http" + "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" + logf "sigs.k8s.io/controller-runtime/pkg/internal/log" + "sigs.k8s.io/controller-runtime/pkg/manager/signals" . "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) -func Example() { - // Build webhooks +var ( + // Build webhooks used for the various server + // configuration options + // // These handlers could be also be implementations // of the AdmissionHandler interface for more complex // implementations. - mutatingHook := &Admission{ + mutatingHook = &Admission{ Handler: admission.HandlerFunc(func(ctx context.Context, req AdmissionRequest) AdmissionResponse { return Patched("some changes", JSONPatchOp{Operation: "add", Path: "/metadata/annotations/access", Value: "granted"}, @@ -38,12 +44,16 @@ func Example() { }), } - validatingHook := &Admission{ + validatingHook = &Admission{ Handler: admission.HandlerFunc(func(ctx context.Context, req AdmissionRequest) AdmissionResponse { return Denied("none shall pass!") }), } +) +// This example registers a webhooks to a webhook server +// that gets ran by a controller manager. +func Example() { // Create a manager // Note: GetConfigOrDie will os.Exit(1) w/o any message if no kube-config can be found mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{}) @@ -70,3 +80,70 @@ func Example() { panic(err) } } + +// This example creates a webhook server that can be +// ran without a controller manager. +func ExampleStandaloneServer() { + // Create a webhook server + hookServer := &Server{ + Port: 8443, + } + + // Register the webhooks in the server. + hookServer.Register("/mutating", mutatingHook) + hookServer.Register("/validating", validatingHook) + + // Start the server without a manger + err := hookServer.StartStandalone(signals.SetupSignalHandler(), scheme.Scheme) + if err != nil { + // handle error + panic(err) + } +} + +// This example creates a standalone webhook handler +// and runs it on a vanilla go HTTP server to demonstrate +// how you could run a webhook on an existing server +// without a controller manager. +func ExampleArbitraryHTTPServer() { + // Assume you have an existing HTTP server at your disposal + // configured as desired (e.g. with TLS). + // For this example just create a basic http.ServeMux + mux := http.NewServeMux() + port := ":8000" + + // Create the standalone HTTP handlers from our webhooks + mutatingHookHandler, err := admission.StandaloneWebhook(mutatingHook, admission.StandaloneOptions{ + Scheme: scheme.Scheme, + // Logger let's you optionally pass + // a custom logger (defaults to log.Log global Logger) + Logger: logf.RuntimeLog.WithName("mutating-webhook"), + // MetricsPath let's you optionally + // provide the path it will be served on + // to be used for labelling prometheus metrics + // If none is set, prometheus metrics will not be generated. + MetricsPath: "/mutating", + }) + if err != nil { + // handle error + panic(err) + } + + validatingHookHandler, err := admission.StandaloneWebhook(validatingHook, admission.StandaloneOptions{ + Scheme: scheme.Scheme, + Logger: logf.RuntimeLog.WithName("validating-webhook"), + MetricsPath: "/validating", + }) + if err != nil { + // handle error + panic(err) + } + + // Register the webhook handlers to your server + mux.Handle("/mutating", mutatingHookHandler) + mux.Handle("/validating", validatingHookHandler) + + // Run your handler + http.ListenAndServe(port, mux) + +} From 5a5106db332f63abd3b7daa1b87ad83b4ed97937 Mon Sep 17 00:00:00 2001 From: Kevin Delgado Date: Thu, 8 Apr 2021 17:03:59 +0000 Subject: [PATCH 6/6] fix CI --- pkg/webhook/example_test.go | 8 +++++--- pkg/webhook/webhook_integration_test.go | 6 ++++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/webhook/example_test.go b/pkg/webhook/example_test.go index 0deb382324..1c15a4455a 100644 --- a/pkg/webhook/example_test.go +++ b/pkg/webhook/example_test.go @@ -83,7 +83,7 @@ func Example() { // This example creates a webhook server that can be // ran without a controller manager. -func ExampleStandaloneServer() { +func ExampleServer_StartStandalone() { // Create a webhook server hookServer := &Server{ Port: 8443, @@ -105,7 +105,7 @@ func ExampleStandaloneServer() { // and runs it on a vanilla go HTTP server to demonstrate // how you could run a webhook on an existing server // without a controller manager. -func ExampleArbitraryHTTPServer() { +func ExampleStandaloneWebhook() { // Assume you have an existing HTTP server at your disposal // configured as desired (e.g. with TLS). // For this example just create a basic http.ServeMux @@ -144,6 +144,8 @@ func ExampleArbitraryHTTPServer() { mux.Handle("/validating", validatingHookHandler) // Run your handler - http.ListenAndServe(port, mux) + if err := http.ListenAndServe(port, mux); err != nil { + panic(err) + } } diff --git a/pkg/webhook/webhook_integration_test.go b/pkg/webhook/webhook_integration_test.go index e2a1d2b3b3..2ab7b4e24f 100644 --- a/pkg/webhook/webhook_integration_test.go +++ b/pkg/webhook/webhook_integration_test.go @@ -127,7 +127,9 @@ var _ = Describe("Webhook", func() { } // generate listener - listener, err := tls.Listen("tcp", net.JoinHostPort(testenv.WebhookInstallOptions.LocalServingHost, strconv.Itoa(int(testenv.WebhookInstallOptions.LocalServingPort))), cfg) + listener, err := tls.Listen("tcp", + net.JoinHostPort(testenv.WebhookInstallOptions.LocalServingHost, + strconv.Itoa(int(testenv.WebhookInstallOptions.LocalServingPort))), cfg) Expect(err).NotTo(HaveOccurred()) // create and register the standalone webhook @@ -144,7 +146,7 @@ var _ = Describe("Webhook", func() { Expect(srv.Shutdown(context.Background())).NotTo(HaveOccurred()) close(idleConnsClosed) }() - srv.Serve(listener) + _ = srv.Serve(listener) <-idleConnsClosed }()