From ea977b906178449c18a99087df3d242365934dcb Mon Sep 17 00:00:00 2001 From: Aldo Culquicondor Date: Fri, 23 Aug 2024 15:52:37 +0000 Subject: [PATCH 1/4] Allow to wire a mutation handler --- pkg/builder/webhook.go | 25 +++++++--- pkg/builder/webhook_test.go | 82 ++++++++++++++++++++++++++++++++ pkg/webhook/admission/webhook.go | 11 +++++ 3 files changed, 111 insertions(+), 7 deletions(-) diff --git a/pkg/builder/webhook.go b/pkg/builder/webhook.go index cfb9f1a69d..fb5a32ead7 100644 --- a/pkg/builder/webhook.go +++ b/pkg/builder/webhook.go @@ -37,6 +37,7 @@ import ( // WebhookBuilder builds a Webhook. type WebhookBuilder struct { apiType runtime.Object + mutatorFactory admission.HandlerFactory customDefaulter admission.CustomDefaulter customValidator admission.CustomValidator gvk schema.GroupVersionKind @@ -65,6 +66,12 @@ func (blder *WebhookBuilder) For(apiType runtime.Object) *WebhookBuilder { return blder } +// WithMutatorFactory takes an admission.HandlerFactory, a MutatingWebhook will be wired for the handler that this factory creates. +func (blder *WebhookBuilder) WithMutatorFactory(factory admission.HandlerFactory) *WebhookBuilder { + blder.mutatorFactory = factory + return blder +} + // WithDefaulter takes an admission.CustomDefaulter interface, a MutatingWebhook will be wired for this type. func (blder *WebhookBuilder) WithDefaulter(defaulter admission.CustomDefaulter) *WebhookBuilder { blder.customDefaulter = defaulter @@ -169,14 +176,18 @@ func (blder *WebhookBuilder) registerDefaultingWebhook() { } func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook { - if defaulter := blder.customDefaulter; defaulter != nil { - w := admission.WithCustomDefaulter(blder.mgr.GetScheme(), blder.apiType, defaulter) - if blder.recoverPanic != nil { - w = w.WithRecoverPanic(*blder.recoverPanic) - } - return w + var w *admission.Webhook + if factory := blder.mutatorFactory; factory != nil { + w = admission.WithHandlerFactory(blder.mgr.GetScheme(), blder.apiType, factory) + } else if defaulter := blder.customDefaulter; defaulter != nil { + w = admission.WithCustomDefaulter(blder.mgr.GetScheme(), blder.apiType, defaulter) + } else { + return nil } - return nil + if blder.recoverPanic != nil { + w = w.WithRecoverPanic(*blder.recoverPanic) + } + return w } // registerValidatingWebhook registers a validating webhook if necessary. diff --git a/pkg/builder/webhook_test.go b/pkg/builder/webhook_test.go index 3ed422d3e9..37d917f245 100644 --- a/pkg/builder/webhook_test.go +++ b/pkg/builder/webhook_test.go @@ -217,6 +217,82 @@ func runTests(admissionReviewVersion string) { ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: fake panic test [recovered]`)) }) + It("should scaffold a mutating webhook with a mutator", func() { + By("creating a controller manager") + m, err := manager.New(cfg, manager.Options{}) + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + + By("registering the type in the Scheme") + builder := scheme.Builder{GroupVersion: testDefaulterGVK.GroupVersion()} + builder.Register(&TestDefaulter{}, &TestDefaulterList{}) + err = builder.AddToScheme(m.GetScheme()) + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + + err = WebhookManagedBy(m). + WithMutatorFactory(mutatorFactoryForTestDefaulter(m.GetScheme())). + For(&TestDefaulter{}). + WithLogConstructor(func(base logr.Logger, req *admission.Request) logr.Logger { + return admission.DefaultLogConstructor(testingLogger, req) + }). + Complete() + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + svr := m.GetWebhookServer() + ExpectWithOffset(1, svr).NotTo(BeNil()) + + reader := strings.NewReader(admissionReviewGV + admissionReviewVersion + `", + "request":{ + "uid":"07e52e8d-4513-11e9-a716-42010a800270", + "kind":{ + "group":"foo.test.org", + "version":"v1", + "kind":"TestDefaulter" + }, + "resource":{ + "group":"foo.test.org", + "version":"v1", + "resource":"testdefaulter" + }, + "namespace":"default", + "name":"foo", + "operation":"CREATE", + "object":{ + "replica":1 + }, + "oldObject":null + } +}`) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + err = svr.Start(ctx) + if err != nil && !os.IsNotExist(err) { + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + } + + By("sending a request to a mutating webhook path") + path := generateMutatePath(testDefaulterGVK) + req := httptest.NewRequest("POST", svcBaseAddr+path, reader) + req.Header.Add("Content-Type", "application/json") + w := httptest.NewRecorder() + svr.WebhookMux().ServeHTTP(w, req) + ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK)) + By("sanity checking the response contains reasonable fields") + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"patch":`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":200`)) + EventuallyWithOffset(1, logBuffer).Should(gbytes.Say(`"msg":"Defaulting object","object":{"name":"foo","namespace":"default"},"namespace":"default","name":"foo","resource":{"group":"foo.test.org","version":"v1","resource":"testdefaulter"},"user":"","requestID":"07e52e8d-4513-11e9-a716-42010a800270"`)) + + By("sending a request to a validating webhook path that doesn't exist") + path = generateValidatePath(testDefaulterGVK) + _, err = reader.Seek(0, 0) + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + req = httptest.NewRequest("POST", svcBaseAddr+path, reader) + req.Header.Add("Content-Type", "application/json") + w = httptest.NewRecorder() + svr.WebhookMux().ServeHTTP(w, req) + ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound)) + }) + It("should scaffold a custom validating webhook", func() { By("creating a controller manager") m, err := manager.New(cfg, manager.Options{}) @@ -592,6 +668,12 @@ func (*TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) err var _ admission.CustomDefaulter = &TestCustomDefaulter{} +func mutatorFactoryForTestDefaulter(scheme *runtime.Scheme) admission.HandlerFactory { + return func(obj runtime.Object, _ admission.Decoder) admission.Handler { + return admission.WithCustomDefaulter(scheme, obj, &TestCustomDefaulter{}).Handler + } +} + // TestCustomValidator. type TestCustomValidator struct{} diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index cba6da2cb0..467fcf0d0e 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -27,6 +27,7 @@ import ( "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" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/klog/v2" @@ -95,6 +96,9 @@ func (r *Response) Complete(req Request) error { return nil } +// HandlerFactory can create a Handler for the given type. +type HandlerFactory func(obj runtime.Object, decoder Decoder) Handler + // Handler can handle an AdmissionRequest. type Handler interface { // Handle yields a response to an AdmissionRequest. @@ -114,6 +118,13 @@ func (f HandlerFunc) Handle(ctx context.Context, req Request) Response { return f(ctx, req) } +// WithHandlerFactory creates a new Webhook for a handler factory. +func WithHandlerFactory(scheme *runtime.Scheme, obj runtime.Object, factory HandlerFactory) *Webhook { + return &Webhook{ + Handler: factory(obj, NewDecoder(scheme)), + } +} + // Webhook represents each individual webhook. // // It must be registered with a webhook.Server or From 50c2afb9542d80c5313e8f850aeccb174110b396 Mon Sep 17 00:00:00 2001 From: Aldo Culquicondor Date: Tue, 24 Sep 2024 19:23:00 +0000 Subject: [PATCH 2/4] Simplify design and error on multiple mutation mechanisms --- pkg/builder/webhook.go | 39 ++++++++++++++++++++------------ pkg/builder/webhook_test.go | 31 ++++++++++++++++++++----- pkg/webhook/admission/webhook.go | 11 --------- 3 files changed, 50 insertions(+), 31 deletions(-) diff --git a/pkg/builder/webhook.go b/pkg/builder/webhook.go index fb5a32ead7..825f8c8c7f 100644 --- a/pkg/builder/webhook.go +++ b/pkg/builder/webhook.go @@ -37,7 +37,7 @@ import ( // WebhookBuilder builds a Webhook. type WebhookBuilder struct { apiType runtime.Object - mutatorFactory admission.HandlerFactory + mutationHandler admission.Handler customDefaulter admission.CustomDefaulter customValidator admission.CustomValidator gvk schema.GroupVersionKind @@ -66,9 +66,9 @@ func (blder *WebhookBuilder) For(apiType runtime.Object) *WebhookBuilder { return blder } -// WithMutatorFactory takes an admission.HandlerFactory, a MutatingWebhook will be wired for the handler that this factory creates. -func (blder *WebhookBuilder) WithMutatorFactory(factory admission.HandlerFactory) *WebhookBuilder { - blder.mutatorFactory = factory +// WithMutationHandler takes an admission.Handler, a MutatingWebhook will be wired for it. +func (blder *WebhookBuilder) WithMutationHandler(h admission.Handler) *WebhookBuilder { + blder.mutationHandler = h return blder } @@ -147,7 +147,9 @@ func (blder *WebhookBuilder) registerWebhooks() error { } // Register webhook(s) for type - blder.registerDefaultingWebhook() + if err := blder.registerDefaultingWebhook(); err != nil { + return err + } blder.registerValidatingWebhook() err = blder.registerConversionWebhook() @@ -158,8 +160,11 @@ func (blder *WebhookBuilder) registerWebhooks() error { } // registerDefaultingWebhook registers a defaulting webhook if necessary. -func (blder *WebhookBuilder) registerDefaultingWebhook() { - mwh := blder.getDefaultingWebhook() +func (blder *WebhookBuilder) registerDefaultingWebhook() error { + mwh, err := blder.getDefaultingWebhook() + if err != nil { + return err + } if mwh != nil { mwh.LogConstructor = blder.logConstructor path := generateMutatePath(blder.gvk) @@ -173,21 +178,27 @@ func (blder *WebhookBuilder) registerDefaultingWebhook() { blder.mgr.GetWebhookServer().Register(path, mwh) } } + return nil } -func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook { +func (blder *WebhookBuilder) getDefaultingWebhook() (*admission.Webhook, error) { var w *admission.Webhook - if factory := blder.mutatorFactory; factory != nil { - w = admission.WithHandlerFactory(blder.mgr.GetScheme(), blder.apiType, factory) - } else if defaulter := blder.customDefaulter; defaulter != nil { + if handler := blder.mutationHandler; handler != nil { + w = &admission.Webhook{Handler: handler} + } + if defaulter := blder.customDefaulter; defaulter != nil { + if w != nil { + return nil, errors.New("a WebhookBuilder can only define a MutationHandler or a Defaulter, but not both") + } w = admission.WithCustomDefaulter(blder.mgr.GetScheme(), blder.apiType, defaulter) - } else { - return nil + } + if w == nil { + return nil, nil } if blder.recoverPanic != nil { w = w.WithRecoverPanic(*blder.recoverPanic) } - return w + return w, nil } // registerValidatingWebhook registers a validating webhook if necessary. diff --git a/pkg/builder/webhook_test.go b/pkg/builder/webhook_test.go index 37d917f245..a32f7abb45 100644 --- a/pkg/builder/webhook_test.go +++ b/pkg/builder/webhook_test.go @@ -30,9 +30,12 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/onsi/gomega/gbytes" + "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/runtime/schema" + "k8s.io/utils/ptr" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" @@ -229,8 +232,8 @@ func runTests(admissionReviewVersion string) { ExpectWithOffset(1, err).NotTo(HaveOccurred()) err = WebhookManagedBy(m). - WithMutatorFactory(mutatorFactoryForTestDefaulter(m.GetScheme())). For(&TestDefaulter{}). + WithMutationHandler(&TestMutationHandler{}). WithLogConstructor(func(base logr.Logger, req *admission.Request) logr.Logger { return admission.DefaultLogConstructor(testingLogger, req) }). @@ -280,7 +283,7 @@ func runTests(admissionReviewVersion string) { ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`)) ExpectWithOffset(1, w.Body).To(ContainSubstring(`"patch":`)) ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":200`)) - EventuallyWithOffset(1, logBuffer).Should(gbytes.Say(`"msg":"Defaulting object","object":{"name":"foo","namespace":"default"},"namespace":"default","name":"foo","resource":{"group":"foo.test.org","version":"v1","resource":"testdefaulter"},"user":"","requestID":"07e52e8d-4513-11e9-a716-42010a800270"`)) + EventuallyWithOffset(1, logBuffer).Should(gbytes.Say(`"msg":"Mutating object","object":{"name":"foo","namespace":"default"},"namespace":"default","name":"foo","resource":{"group":"foo.test.org","version":"v1","resource":"testdefaulter"},"user":"","requestID":"07e52e8d-4513-11e9-a716-42010a800270"`)) By("sending a request to a validating webhook path that doesn't exist") path = generateValidatePath(testDefaulterGVK) @@ -646,7 +649,8 @@ func (*TestDefaultValidatorList) DeepCopyObject() runtime.Object { return nil type TestCustomDefaulter struct{} func (*TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error { - logf.FromContext(ctx).Info("Defaulting object") + log := logf.FromContext(ctx) + log.Info("Defaulting object") req, err := admission.RequestFromContext(ctx) if err != nil { return fmt.Errorf("expected admission.Request in ctx: %w", err) @@ -668,12 +672,27 @@ func (*TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) err var _ admission.CustomDefaulter = &TestCustomDefaulter{} -func mutatorFactoryForTestDefaulter(scheme *runtime.Scheme) admission.HandlerFactory { - return func(obj runtime.Object, _ admission.Decoder) admission.Handler { - return admission.WithCustomDefaulter(scheme, obj, &TestCustomDefaulter{}).Handler +// TestMutationHandler +type TestMutationHandler struct{} + +func (*TestMutationHandler) Handle(ctx context.Context, req admission.Request) admission.Response { + log := logf.FromContext(ctx) + log.Info("Mutating object") + return admission.Response{ + AdmissionResponse: admissionv1.AdmissionResponse{ + Allowed: true, + PatchType: ptr.To(admissionv1.PatchTypeJSONPatch), + }, + Patches: []jsonpatch.Operation{{ + Operation: "replace", + Path: "/replica", + Value: "2", + }}, } } +var _ admission.Handler = &TestMutationHandler{} + // TestCustomValidator. type TestCustomValidator struct{} diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index 467fcf0d0e..cba6da2cb0 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -27,7 +27,6 @@ import ( "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" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/klog/v2" @@ -96,9 +95,6 @@ func (r *Response) Complete(req Request) error { return nil } -// HandlerFactory can create a Handler for the given type. -type HandlerFactory func(obj runtime.Object, decoder Decoder) Handler - // Handler can handle an AdmissionRequest. type Handler interface { // Handle yields a response to an AdmissionRequest. @@ -118,13 +114,6 @@ func (f HandlerFunc) Handle(ctx context.Context, req Request) Response { return f(ctx, req) } -// WithHandlerFactory creates a new Webhook for a handler factory. -func WithHandlerFactory(scheme *runtime.Scheme, obj runtime.Object, factory HandlerFactory) *Webhook { - return &Webhook{ - Handler: factory(obj, NewDecoder(scheme)), - } -} - // Webhook represents each individual webhook. // // It must be registered with a webhook.Server or From ed242f4eea1c419366dd2098460418348ba4e04c Mon Sep 17 00:00:00 2001 From: Aldo Culquicondor Date: Fri, 27 Sep 2024 13:26:04 +0000 Subject: [PATCH 3/4] revert formatting --- pkg/builder/webhook_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/builder/webhook_test.go b/pkg/builder/webhook_test.go index a32f7abb45..83ee6cde22 100644 --- a/pkg/builder/webhook_test.go +++ b/pkg/builder/webhook_test.go @@ -649,8 +649,7 @@ func (*TestDefaultValidatorList) DeepCopyObject() runtime.Object { return nil type TestCustomDefaulter struct{} func (*TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error { - log := logf.FromContext(ctx) - log.Info("Defaulting object") + logf.FromContext(ctx).Info("Defaulting object") req, err := admission.RequestFromContext(ctx) if err != nil { return fmt.Errorf("expected admission.Request in ctx: %w", err) @@ -676,8 +675,7 @@ var _ admission.CustomDefaulter = &TestCustomDefaulter{} type TestMutationHandler struct{} func (*TestMutationHandler) Handle(ctx context.Context, req admission.Request) admission.Response { - log := logf.FromContext(ctx) - log.Info("Mutating object") + logf.FromContext(ctx).Info("Mutating object") return admission.Response{ AdmissionResponse: admissionv1.AdmissionResponse{ Allowed: true, From cd4ea161da240f03b157b72adf9b110e8f81029c Mon Sep 17 00:00:00 2001 From: Aldo Culquicondor Date: Thu, 10 Oct 2024 17:23:19 +0000 Subject: [PATCH 4/4] Add gocomment for when to use WithMutationHandler --- pkg/builder/webhook.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/builder/webhook.go b/pkg/builder/webhook.go index 825f8c8c7f..ec22208680 100644 --- a/pkg/builder/webhook.go +++ b/pkg/builder/webhook.go @@ -67,6 +67,9 @@ func (blder *WebhookBuilder) For(apiType runtime.Object) *WebhookBuilder { } // WithMutationHandler takes an admission.Handler, a MutatingWebhook will be wired for it. +// A mutation handler is a low-level plug point for advanced use cases that need control +// to generate patches. If in doubt, prefer using WithDefaulter to register a defaulter. +// You can only use one of WithMutationHandler or WithDefaulter. func (blder *WebhookBuilder) WithMutationHandler(h admission.Handler) *WebhookBuilder { blder.mutationHandler = h return blder