From e5a4a04b1e6d08263b8a47102078ff348c7152bf Mon Sep 17 00:00:00 2001 From: Troy Connor Date: Mon, 8 Jul 2024 12:04:23 -0400 Subject: [PATCH 1/4] remove deprecated Defaulter and Validator interfaces Signed-off-by: Troy Connor --- pkg/builder/webhook.go | 18 +- pkg/builder/webhook_test.go | 435 ------------------- pkg/webhook/admission/defaulter.go | 84 ---- pkg/webhook/admission/defaulter_test.go | 68 --- pkg/webhook/admission/validator.go | 127 ------ pkg/webhook/admission/validator_custom.go | 3 + pkg/webhook/admission/validator_test.go | 494 ---------------------- pkg/webhook/alias.go | 8 - 8 files changed, 5 insertions(+), 1232 deletions(-) delete mode 100644 pkg/webhook/admission/defaulter.go delete mode 100644 pkg/webhook/admission/defaulter_test.go delete mode 100644 pkg/webhook/admission/validator.go delete mode 100644 pkg/webhook/admission/validator_test.go diff --git a/pkg/builder/webhook.go b/pkg/builder/webhook.go index 81d8f74056..350cdfa719 100644 --- a/pkg/builder/webhook.go +++ b/pkg/builder/webhook.go @@ -176,15 +176,8 @@ func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook { } return w } - if defaulter, ok := blder.apiType.(admission.Defaulter); ok { - w := admission.DefaultingWebhookFor(blder.mgr.GetScheme(), defaulter) - if blder.recoverPanic != nil { - w = w.WithRecoverPanic(*blder.recoverPanic) - } - return w - } log.Info( - "skip registering a mutating webhook, object does not implement admission.Defaulter or WithDefaulter wasn't called", + "skip registering a mutating webhook, object does not implement admission.CustomDefaulter or WithCustomDefaulter wasn't called", "GVK", blder.gvk) return nil } @@ -215,15 +208,8 @@ func (blder *WebhookBuilder) getValidatingWebhook() *admission.Webhook { } return w } - if validator, ok := blder.apiType.(admission.Validator); ok { - w := admission.ValidatingWebhookFor(blder.mgr.GetScheme(), validator) - if blder.recoverPanic != nil { - w = w.WithRecoverPanic(*blder.recoverPanic) - } - return w - } log.Info( - "skip registering a validating webhook, object does not implement admission.Validator or WithValidator wasn't called", + "skip registering a validating webhook, object does not implement admission.CustomValidator or WithCustomValidator wasn't called", "GVK", blder.gvk) return nil } diff --git a/pkg/builder/webhook_test.go b/pkg/builder/webhook_test.go index 4574d5cc77..147c176101 100644 --- a/pkg/builder/webhook_test.go +++ b/pkg/builder/webhook_test.go @@ -77,138 +77,6 @@ func runTests(admissionReviewVersion string) { close(stop) }) - It("should scaffold a defaulting webhook if the type implements the Defaulter interface", 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). - For(&TestDefaulter{}). - 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":"", - "version":"v1", - "kind":"TestDefaulter" - }, - "resource":{ - "group":"", - "version":"v1", - "resource":"testdefaulter" - }, - "namespace":"default", - "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`)) - - 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 defaulting webhook which recovers from panics", 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). - For(&TestDefaulter{Panic: true}). - // RecoverPanic defaults to true. - 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":"", - "version":"v1", - "kind":"TestDefaulter" - }, - "resource":{ - "group":"", - "version":"v1", - "resource":"testdefaulter" - }, - "namespace":"default", - "operation":"CREATE", - "object":{ - "replica":1, - "panic":true - }, - "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":false`)) - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":500`)) - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: fake panic test [recovered]`)) - }) - It("should scaffold a defaulting webhook with a custom defaulter", func() { By("creating a controller manager") m, err := manager.New(cfg, manager.Options{}) @@ -285,140 +153,6 @@ func runTests(admissionReviewVersion string) { ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound)) }) - It("should scaffold a validating webhook if the type implements the Validator interface", 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: testValidatorGVK.GroupVersion()} - builder.Register(&TestValidator{}, &TestValidatorList{}) - err = builder.AddToScheme(m.GetScheme()) - ExpectWithOffset(1, err).NotTo(HaveOccurred()) - - err = WebhookManagedBy(m). - For(&TestValidator{}). - 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":"", - "version":"v1", - "kind":"TestValidator" - }, - "resource":{ - "group":"", - "version":"v1", - "resource":"testvalidator" - }, - "namespace":"default", - "operation":"UPDATE", - "object":{ - "replica":1 - }, - "oldObject":{ - "replica":2 - } - } -}`) - - 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 that doesn't exist") - path := generateMutatePath(testValidatorGVK) - 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)) - - By("sending a request to a validating webhook path") - path = generateValidatePath(testValidatorGVK) - _, 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.StatusOK)) - By("sanity checking the response contains reasonable field") - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`)) - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":403`)) - }) - - It("should scaffold a validating webhook which recovers from panics", 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: testValidatorGVK.GroupVersion()} - builder.Register(&TestValidator{}, &TestValidatorList{}) - err = builder.AddToScheme(m.GetScheme()) - ExpectWithOffset(1, err).NotTo(HaveOccurred()) - - err = WebhookManagedBy(m). - For(&TestValidator{Panic: true}). - RecoverPanic(true). - 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":"", - "version":"v1", - "kind":"TestValidator" - }, - "resource":{ - "group":"", - "version":"v1", - "resource":"testvalidator" - }, - "namespace":"default", - "operation":"CREATE", - "object":{ - "replica":2, - "panic":true - } - } -}`) - - 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 validating webhook path") - path := generateValidatePath(testValidatorGVK) - _, 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.StatusOK)) - By("sanity checking the response contains reasonable field") - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`)) - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":500`)) - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: fake panic test [recovered]`)) - }) - It("should scaffold a validating webhook with a custom validator", func() { By("creating a controller manager") m, err := manager.New(cfg, manager.Options{}) @@ -496,171 +230,6 @@ func runTests(admissionReviewVersion string) { EventuallyWithOffset(1, logBuffer).Should(gbytes.Say(`"msg":"Validating object","object":{"name":"foo","namespace":"default"},"namespace":"default","name":"foo","resource":{"group":"foo.test.org","version":"v1","resource":"testvalidator"},"user":"","requestID":"07e52e8d-4513-11e9-a716-42010a800270"`)) }) - It("should scaffold defaulting and validating webhooks if the type implements both Defaulter and Validator interfaces", 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: testDefaultValidatorGVK.GroupVersion()} - builder.Register(&TestDefaultValidator{}, &TestDefaultValidatorList{}) - err = builder.AddToScheme(m.GetScheme()) - ExpectWithOffset(1, err).NotTo(HaveOccurred()) - - err = WebhookManagedBy(m). - For(&TestDefaultValidator{}). - 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":"", - "version":"v1", - "kind":"TestDefaultValidator" - }, - "resource":{ - "group":"", - "version":"v1", - "resource":"testdefaultvalidator" - }, - "namespace":"default", - "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(testDefaultValidatorGVK) - 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 field") - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`)) - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"patch":`)) - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":200`)) - - By("sending a request to a validating webhook path") - path = generateValidatePath(testDefaultValidatorGVK) - _, 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.StatusOK)) - By("sanity checking the response contains reasonable field") - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`)) - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":200`)) - }) - - It("should scaffold a validating webhook if the type implements the Validator interface to validate deletes", func() { - By("creating a controller manager") - ctx, cancel := context.WithCancel(context.Background()) - - m, err := manager.New(cfg, manager.Options{}) - ExpectWithOffset(1, err).NotTo(HaveOccurred()) - - By("registering the type in the Scheme") - builder := scheme.Builder{GroupVersion: testValidatorGVK.GroupVersion()} - builder.Register(&TestValidator{}, &TestValidatorList{}) - err = builder.AddToScheme(m.GetScheme()) - ExpectWithOffset(1, err).NotTo(HaveOccurred()) - - err = WebhookManagedBy(m). - For(&TestValidator{}). - 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":"", - "version":"v1", - "kind":"TestValidator" - }, - "resource":{ - "group":"", - "version":"v1", - "resource":"testvalidator" - }, - "namespace":"default", - "operation":"DELETE", - "object":null, - "oldObject":{ - "replica":1 - } - } -}`) - - cancel() - err = svr.Start(ctx) - if err != nil && !os.IsNotExist(err) { - ExpectWithOffset(1, err).NotTo(HaveOccurred()) - } - - By("sending a request to a validating webhook path to check for failed delete") - path := generateValidatePath(testValidatorGVK) - 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 field") - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`)) - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":403`)) - - reader = strings.NewReader(admissionReviewGV + admissionReviewVersion + `", - "request":{ - "uid":"07e52e8d-4513-11e9-a716-42010a800270", - "kind":{ - "group":"", - "version":"v1", - "kind":"TestValidator" - }, - "resource":{ - "group":"", - "version":"v1", - "resource":"testvalidator" - }, - "namespace":"default", - "operation":"DELETE", - "object":null, - "oldObject":{ - "replica":0 - } - } -}`) - By("sending a request to a validating webhook path with correct request") - path = generateValidatePath(testValidatorGVK) - 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 field") - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`)) - ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":200`)) - }) - It("should send an error when trying to register a webhook with more than one For", func() { By("creating a controller manager") m, err := manager.New(cfg, manager.Options{}) @@ -753,8 +322,6 @@ type TestValidatorList struct{} func (*TestValidatorList) GetObjectKind() schema.ObjectKind { return nil } func (*TestValidatorList) DeepCopyObject() runtime.Object { return nil } -var _ admission.Validator = &TestValidator{} - func (v *TestValidator) ValidateCreate() (admission.Warnings, error) { if v.Panic { panic("fake panic test") @@ -828,8 +395,6 @@ func (dv *TestDefaultValidator) Default() { } } -var _ admission.Validator = &TestDefaultValidator{} - func (dv *TestDefaultValidator) ValidateCreate() (admission.Warnings, error) { if dv.Replica < 0 { return nil, errors.New("number of replica should be greater than or equal to 0") diff --git a/pkg/webhook/admission/defaulter.go b/pkg/webhook/admission/defaulter.go deleted file mode 100644 index efbbf60282..0000000000 --- a/pkg/webhook/admission/defaulter.go +++ /dev/null @@ -1,84 +0,0 @@ -/* -Copyright 2018 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package admission - -import ( - "context" - "encoding/json" - "net/http" - - admissionv1 "k8s.io/api/admission/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" -) - -// Defaulter defines functions for setting defaults on resources. -// Deprecated: Ue CustomDefaulter instead. -type Defaulter interface { - runtime.Object - Default() -} - -// DefaultingWebhookFor creates a new Webhook for Defaulting the provided type. -// Deprecated: Use WithCustomDefaulter instead. -func DefaultingWebhookFor(scheme *runtime.Scheme, defaulter Defaulter) *Webhook { - return &Webhook{ - Handler: &mutatingHandler{defaulter: defaulter, decoder: NewDecoder(scheme)}, - } -} - -type mutatingHandler struct { - defaulter Defaulter - decoder Decoder -} - -// Handle handles admission requests. -func (h *mutatingHandler) Handle(ctx context.Context, req Request) Response { - if h.decoder == nil { - panic("decoder should never be nil") - } - if h.defaulter == nil { - panic("defaulter should never be nil") - } - - // always skip when a DELETE operation received in mutation handler - // describe in https://github.com/kubernetes-sigs/controller-runtime/issues/1762 - if req.Operation == admissionv1.Delete { - return Response{AdmissionResponse: admissionv1.AdmissionResponse{ - Allowed: true, - Result: &metav1.Status{ - Code: http.StatusOK, - }, - }} - } - - // Get the object in the request - obj := h.defaulter.DeepCopyObject().(Defaulter) - if err := h.decoder.Decode(req, obj); err != nil { - return Errored(http.StatusBadRequest, err) - } - - // Default the object - obj.Default() - marshalled, err := json.Marshal(obj) - if err != nil { - return Errored(http.StatusInternalServerError, err) - } - - // Create the patch - return PatchResponseFromRaw(req.Object.Raw, marshalled) -} diff --git a/pkg/webhook/admission/defaulter_test.go b/pkg/webhook/admission/defaulter_test.go deleted file mode 100644 index cf7571663c..0000000000 --- a/pkg/webhook/admission/defaulter_test.go +++ /dev/null @@ -1,68 +0,0 @@ -package admission - -import ( - "context" - "net/http" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - - admissionv1 "k8s.io/api/admission/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" -) - -var _ = Describe("Defaulter Handler", func() { - - It("should return ok if received delete verb in defaulter handler", func() { - obj := &TestDefaulter{} - handler := DefaultingWebhookFor(admissionScheme, obj) - - resp := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Delete, - OldObject: runtime.RawExtension{ - Raw: []byte("{}"), - }, - }, - }) - Expect(resp.Allowed).Should(BeTrue()) - Expect(resp.Result.Code).Should(Equal(int32(http.StatusOK))) - }) - -}) - -// TestDefaulter. -var _ runtime.Object = &TestDefaulter{} - -type TestDefaulter struct { - Replica int `json:"replica,omitempty"` -} - -var testDefaulterGVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: "TestDefaulter"} - -func (d *TestDefaulter) GetObjectKind() schema.ObjectKind { return d } -func (d *TestDefaulter) DeepCopyObject() runtime.Object { - return &TestDefaulter{ - Replica: d.Replica, - } -} - -func (d *TestDefaulter) GroupVersionKind() schema.GroupVersionKind { - return testDefaulterGVK -} - -func (d *TestDefaulter) SetGroupVersionKind(gvk schema.GroupVersionKind) {} - -var _ runtime.Object = &TestDefaulterList{} - -type TestDefaulterList struct{} - -func (*TestDefaulterList) GetObjectKind() schema.ObjectKind { return nil } -func (*TestDefaulterList) DeepCopyObject() runtime.Object { return nil } - -func (d *TestDefaulter) Default() { - if d.Replica < 2 { - d.Replica = 2 - } -} diff --git a/pkg/webhook/admission/validator.go b/pkg/webhook/admission/validator.go deleted file mode 100644 index b28a56eef8..0000000000 --- a/pkg/webhook/admission/validator.go +++ /dev/null @@ -1,127 +0,0 @@ -/* -Copyright 2018 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package admission - -import ( - "context" - "errors" - "fmt" - "net/http" - - v1 "k8s.io/api/admission/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime" -) - -// Warnings represents warning messages. -type Warnings []string - -// Validator defines functions for validating an operation. -// The custom resource kind which implements this interface can validate itself. -// To validate the custom resource with another specific struct, use CustomValidator instead. -// Deprecated: Use CustomValidator instead. -type Validator interface { - runtime.Object - - // ValidateCreate validates the object on creation. - // The optional warnings will be added to the response as warning messages. - // Return an error if the object is invalid. - ValidateCreate() (warnings Warnings, err error) - - // ValidateUpdate validates the object on update. The oldObj is the object before the update. - // The optional warnings will be added to the response as warning messages. - // Return an error if the object is invalid. - ValidateUpdate(old runtime.Object) (warnings Warnings, err error) - - // ValidateDelete validates the object on deletion. - // The optional warnings will be added to the response as warning messages. - // Return an error if the object is invalid. - ValidateDelete() (warnings Warnings, err error) -} - -// ValidatingWebhookFor creates a new Webhook for validating the provided type. -// Deprecated: Use WithCustomValidator instead. -func ValidatingWebhookFor(scheme *runtime.Scheme, validator Validator) *Webhook { - return &Webhook{ - Handler: &validatingHandler{validator: validator, decoder: NewDecoder(scheme)}, - } -} - -type validatingHandler struct { - validator Validator - decoder Decoder -} - -// Handle handles admission requests. -func (h *validatingHandler) Handle(ctx context.Context, req Request) Response { - if h.decoder == nil { - panic("decoder should never be nil") - } - if h.validator == nil { - panic("validator should never be nil") - } - // Get the object in the request - obj := h.validator.DeepCopyObject().(Validator) - - var err error - var warnings []string - - switch req.Operation { - case v1.Connect: - // No validation for connect requests. - // TODO(vincepri): Should we validate CONNECT requests? In what cases? - case v1.Create: - if err = h.decoder.Decode(req, obj); err != nil { - return Errored(http.StatusBadRequest, err) - } - - warnings, err = obj.ValidateCreate() - case v1.Update: - oldObj := obj.DeepCopyObject() - - err = h.decoder.DecodeRaw(req.Object, obj) - if err != nil { - return Errored(http.StatusBadRequest, err) - } - err = h.decoder.DecodeRaw(req.OldObject, oldObj) - if err != nil { - return Errored(http.StatusBadRequest, err) - } - - warnings, err = obj.ValidateUpdate(oldObj) - case v1.Delete: - // In reference to PR: https://github.com/kubernetes/kubernetes/pull/76346 - // OldObject contains the object being deleted - err = h.decoder.DecodeRaw(req.OldObject, obj) - if err != nil { - return Errored(http.StatusBadRequest, err) - } - - warnings, err = obj.ValidateDelete() - default: - return Errored(http.StatusBadRequest, fmt.Errorf("unknown operation %q", req.Operation)) - } - - if err != nil { - var apiStatus apierrors.APIStatus - if errors.As(err, &apiStatus) { - return validationResponseFromStatus(false, apiStatus.Status()).WithWarnings(warnings...) - } - return Denied(err.Error()).WithWarnings(warnings...) - } - return Allowed("").WithWarnings(warnings...) -} diff --git a/pkg/webhook/admission/validator_custom.go b/pkg/webhook/admission/validator_custom.go index b8f194401e..ef1be52a8f 100644 --- a/pkg/webhook/admission/validator_custom.go +++ b/pkg/webhook/admission/validator_custom.go @@ -27,6 +27,9 @@ import ( "k8s.io/apimachinery/pkg/runtime" ) +// Warnings represents warning messages. +type Warnings []string + // CustomValidator defines functions for validating an operation. // The object to be validated is passed into methods as a parameter. type CustomValidator interface { diff --git a/pkg/webhook/admission/validator_test.go b/pkg/webhook/admission/validator_test.go deleted file mode 100644 index 404fad9016..0000000000 --- a/pkg/webhook/admission/validator_test.go +++ /dev/null @@ -1,494 +0,0 @@ -/* -Copyright 2021 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package admission - -import ( - "context" - "errors" - "net/http" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - admissionv1 "k8s.io/api/admission/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/client-go/kubernetes/scheme" -) - -var fakeValidatorVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: "fakeValidator"} - -var _ = Describe("validatingHandler", func() { - - decoder := NewDecoder(scheme.Scheme) - - Context("when dealing with successful results without warning", func() { - f := &fakeValidator{ErrorToReturn: nil, GVKToReturn: fakeValidatorVK, WarningsToReturn: nil} - handler := validatingHandler{validator: f, decoder: decoder} - - It("should return 200 in response when create succeeds", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - - Expect(response.Allowed).Should(BeTrue()) - Expect(response.Result.Code).Should(Equal(int32(http.StatusOK))) - }) - - It("should return 200 in response when update succeeds", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Update, - Object: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - OldObject: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - Expect(response.Allowed).Should(BeTrue()) - Expect(response.Result.Code).Should(Equal(int32(http.StatusOK))) - }) - - It("should return 200 in response when delete succeeds", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Delete, - OldObject: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - Expect(response.Allowed).Should(BeTrue()) - Expect(response.Result.Code).Should(Equal(int32(http.StatusOK))) - }) - }) - - const warningMessage = "warning message" - const anotherWarningMessage = "another warning message" - Context("when dealing with successful results with warning", func() { - f := &fakeValidator{ErrorToReturn: nil, GVKToReturn: fakeValidatorVK, WarningsToReturn: []string{ - warningMessage, - anotherWarningMessage, - }} - handler := validatingHandler{validator: f, decoder: decoder} - - It("should return 200 in response when create succeeds, with warning messages", func() { - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - - Expect(response.Allowed).Should(BeTrue()) - Expect(response.Result.Code).Should(Equal(int32(http.StatusOK))) - Expect(response.AdmissionResponse.Warnings).Should(ContainElement(warningMessage)) - Expect(response.AdmissionResponse.Warnings).Should(ContainElement(anotherWarningMessage)) - }) - - It("should return 200 in response when update succeeds, with warning messages", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Update, - Object: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - OldObject: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - Expect(response.Allowed).Should(BeTrue()) - Expect(response.Result.Code).Should(Equal(int32(http.StatusOK))) - Expect(response.AdmissionResponse.Warnings).Should(ContainElement(warningMessage)) - Expect(response.AdmissionResponse.Warnings).Should(ContainElement(anotherWarningMessage)) - }) - - It("should return 200 in response when delete succeeds, with warning messages", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Delete, - OldObject: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - Expect(response.Allowed).Should(BeTrue()) - Expect(response.Result.Code).Should(Equal(int32(http.StatusOK))) - Expect(response.AdmissionResponse.Warnings).Should(ContainElement(warningMessage)) - Expect(response.AdmissionResponse.Warnings).Should(ContainElement(anotherWarningMessage)) - }) - }) - - Context("when dealing with Status errors, with warning messages", func() { - // Status error would overwrite the warning messages, so no warning messages should be observed. - expectedError := &apierrors.StatusError{ - ErrStatus: metav1.Status{ - Message: "some message", - Code: http.StatusUnprocessableEntity, - }, - } - f := &fakeValidator{ErrorToReturn: expectedError, GVKToReturn: fakeValidatorVK, WarningsToReturn: []string{warningMessage, anotherWarningMessage}} - handler := validatingHandler{validator: f, decoder: decoder} - - It("should propagate the Status from ValidateCreate's return value to the HTTP response", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - - Expect(response.Allowed).Should(BeFalse()) - Expect(response.Result.Code).Should(Equal(expectedError.Status().Code)) - Expect(*response.Result).Should(Equal(expectedError.Status())) - Expect(response.AdmissionResponse.Warnings).Should(ContainElements(warningMessage)) - Expect(response.AdmissionResponse.Warnings).Should(ContainElements(anotherWarningMessage)) - - }) - - It("should propagate the Status from ValidateUpdate's return value to the HTTP response", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Update, - Object: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - OldObject: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - - Expect(response.Allowed).Should(BeFalse()) - Expect(response.Result.Code).Should(Equal(expectedError.Status().Code)) - Expect(*response.Result).Should(Equal(expectedError.Status())) - Expect(response.AdmissionResponse.Warnings).Should(ContainElements(warningMessage)) - Expect(response.AdmissionResponse.Warnings).Should(ContainElements(anotherWarningMessage)) - - }) - - It("should propagate the Status from ValidateDelete's return value to the HTTP response", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Delete, - OldObject: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - - Expect(response.Allowed).Should(BeFalse()) - Expect(response.Result.Code).Should(Equal(expectedError.Status().Code)) - Expect(*response.Result).Should(Equal(expectedError.Status())) - Expect(response.AdmissionResponse.Warnings).Should(ContainElements(warningMessage)) - Expect(response.AdmissionResponse.Warnings).Should(ContainElements(anotherWarningMessage)) - - }) - - }) - - Context("when dealing with Status errors, without warning messages", func() { - - expectedError := &apierrors.StatusError{ - ErrStatus: metav1.Status{ - Message: "some message", - Code: http.StatusUnprocessableEntity, - }, - } - f := &fakeValidator{ErrorToReturn: expectedError, GVKToReturn: fakeValidatorVK, WarningsToReturn: nil} - handler := validatingHandler{validator: f, decoder: decoder} - - It("should propagate the Status from ValidateCreate's return value to the HTTP response", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - - Expect(response.Allowed).Should(BeFalse()) - Expect(response.Result.Code).Should(Equal(expectedError.Status().Code)) - Expect(*response.Result).Should(Equal(expectedError.Status())) - - }) - - It("should propagate the Status from ValidateUpdate's return value to the HTTP response", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Update, - Object: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - OldObject: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - - Expect(response.Allowed).Should(BeFalse()) - Expect(response.Result.Code).Should(Equal(expectedError.Status().Code)) - Expect(*response.Result).Should(Equal(expectedError.Status())) - - }) - - It("should propagate the Status from ValidateDelete's return value to the HTTP response", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Delete, - OldObject: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - - Expect(response.Allowed).Should(BeFalse()) - Expect(response.Result.Code).Should(Equal(expectedError.Status().Code)) - Expect(*response.Result).Should(Equal(expectedError.Status())) - - }) - - }) - - Context("when dealing with non-status errors, without warning messages", func() { - - expectedError := errors.New("some error") - f := &fakeValidator{ErrorToReturn: expectedError, GVKToReturn: fakeValidatorVK} - handler := validatingHandler{validator: f, decoder: decoder} - - It("should return 403 response when ValidateCreate with error message embedded", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - Expect(response.Allowed).Should(BeFalse()) - Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) - Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) - Expect(response.Result.Message).Should(Equal(expectedError.Error())) - - }) - - It("should return 403 response when ValidateUpdate returns non-APIStatus error", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Update, - Object: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - OldObject: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - Expect(response.Allowed).Should(BeFalse()) - Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) - Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) - Expect(response.Result.Message).Should(Equal(expectedError.Error())) - - }) - - It("should return 403 response when ValidateDelete returns non-APIStatus error", func() { - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Delete, - OldObject: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - Expect(response.Allowed).Should(BeFalse()) - Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) - Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) - Expect(response.Result.Message).Should(Equal(expectedError.Error())) - }) - }) - - Context("when dealing with non-status errors, with warning messages", func() { - - expectedError := errors.New("some error") - f := &fakeValidator{ErrorToReturn: expectedError, GVKToReturn: fakeValidatorVK, WarningsToReturn: []string{warningMessage, anotherWarningMessage}} - handler := validatingHandler{validator: f, decoder: decoder} - - It("should return 403 response when ValidateCreate with error message embedded", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Create, - Object: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - Expect(response.Allowed).Should(BeFalse()) - Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) - Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) - Expect(response.Result.Message).Should(Equal(expectedError.Error())) - Expect(response.AdmissionResponse.Warnings).Should(ContainElement(warningMessage)) - Expect(response.AdmissionResponse.Warnings).Should(ContainElement(anotherWarningMessage)) - }) - - It("should return 403 response when ValidateUpdate returns non-APIStatus error", func() { - - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Update, - Object: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - OldObject: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - Expect(response.Allowed).Should(BeFalse()) - Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) - Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) - Expect(response.Result.Message).Should(Equal(expectedError.Error())) - Expect(response.AdmissionResponse.Warnings).Should(ContainElement(warningMessage)) - Expect(response.AdmissionResponse.Warnings).Should(ContainElement(anotherWarningMessage)) - - }) - - It("should return 403 response when ValidateDelete returns non-APIStatus error", func() { - response := handler.Handle(context.TODO(), Request{ - AdmissionRequest: admissionv1.AdmissionRequest{ - Operation: admissionv1.Delete, - OldObject: runtime.RawExtension{ - Raw: []byte("{}"), - Object: handler.validator, - }, - }, - }) - Expect(response.Allowed).Should(BeFalse()) - Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) - Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) - Expect(response.Result.Message).Should(Equal(expectedError.Error())) - Expect(response.AdmissionResponse.Warnings).Should(ContainElement(warningMessage)) - Expect(response.AdmissionResponse.Warnings).Should(ContainElement(anotherWarningMessage)) - - }) - }) - - PIt("should return 400 in response when create fails on decode", func() {}) - - PIt("should return 400 in response when update fails on decoding new object", func() {}) - - PIt("should return 400 in response when update fails on decoding old object", func() {}) - - PIt("should return 400 in response when delete fails on decode", func() {}) - -}) - -// fakeValidator provides fake validating webhook functionality for testing -// It implements the admission.Validator interface and -// rejects all requests with the same configured error -// or passes if ErrorToReturn is nil. -// And it would always return configured warning messages WarningsToReturn. -type fakeValidator struct { - // ErrorToReturn is the error for which the fakeValidator rejects all requests - ErrorToReturn error `json:"errorToReturn,omitempty"` - // GVKToReturn is the GroupVersionKind that the webhook operates on - GVKToReturn schema.GroupVersionKind - // WarningsToReturn is the warnings for fakeValidator returns to all requests - WarningsToReturn []string -} - -func (v *fakeValidator) ValidateCreate() (warnings Warnings, err error) { - return v.WarningsToReturn, v.ErrorToReturn -} - -func (v *fakeValidator) ValidateUpdate(old runtime.Object) (warnings Warnings, err error) { - return v.WarningsToReturn, v.ErrorToReturn -} - -func (v *fakeValidator) ValidateDelete() (warnings Warnings, err error) { - return v.WarningsToReturn, v.ErrorToReturn -} - -func (v *fakeValidator) SetGroupVersionKind(gvk schema.GroupVersionKind) { - v.GVKToReturn = gvk -} - -func (v *fakeValidator) GroupVersionKind() schema.GroupVersionKind { - return v.GVKToReturn -} - -func (v *fakeValidator) GetObjectKind() schema.ObjectKind { - return v -} - -func (v *fakeValidator) DeepCopyObject() runtime.Object { - return &fakeValidator{ - ErrorToReturn: v.ErrorToReturn, - GVKToReturn: v.GVKToReturn, - WarningsToReturn: v.WarningsToReturn, - } -} diff --git a/pkg/webhook/alias.go b/pkg/webhook/alias.go index e8439e2ea2..2882e7bab3 100644 --- a/pkg/webhook/alias.go +++ b/pkg/webhook/alias.go @@ -23,14 +23,6 @@ import ( // define some aliases for common bits of the webhook functionality -// Defaulter defines functions for setting defaults on resources. -// Deprecated: Use CustomDefaulter instead. -type Defaulter = admission.Defaulter - -// Validator defines functions for validating an operation. -// Deprecated: Use CustomValidator instead. -type Validator = admission.Validator - // CustomDefaulter defines functions for setting defaults on resources. type CustomDefaulter = admission.CustomDefaulter From d77038f13e0b4fd3953aa2f550abb509f8662bfb Mon Sep 17 00:00:00 2001 From: Troy Connor Date: Wed, 21 Aug 2024 11:52:10 -0400 Subject: [PATCH 2/4] remove log, add test for customValidator/customDefaulter Signed-off-by: Troy Connor --- pkg/builder/webhook.go | 6 - pkg/builder/webhook_test.go | 234 ++++++++++++++++++++++++++++++++++++ 2 files changed, 234 insertions(+), 6 deletions(-) diff --git a/pkg/builder/webhook.go b/pkg/builder/webhook.go index 350cdfa719..cfb9f1a69d 100644 --- a/pkg/builder/webhook.go +++ b/pkg/builder/webhook.go @@ -176,9 +176,6 @@ func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook { } return w } - log.Info( - "skip registering a mutating webhook, object does not implement admission.CustomDefaulter or WithCustomDefaulter wasn't called", - "GVK", blder.gvk) return nil } @@ -208,9 +205,6 @@ func (blder *WebhookBuilder) getValidatingWebhook() *admission.Webhook { } return w } - log.Info( - "skip registering a validating webhook, object does not implement admission.CustomValidator or WithCustomValidator wasn't called", - "GVK", blder.gvk) return nil } diff --git a/pkg/builder/webhook_test.go b/pkg/builder/webhook_test.go index 147c176101..8581c74e70 100644 --- a/pkg/builder/webhook_test.go +++ b/pkg/builder/webhook_test.go @@ -77,6 +77,69 @@ func runTests(admissionReviewVersion string) { close(stop) }) + It("should scaffold a custom defaulter webhook which recovers from panics", 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). + For(&TestDefaulter{}). + WithDefaulter(&TestCustomDefaulter{}). + RecoverPanic(true). + // RecoverPanic defaults to true. + 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":"", + "version":"v1", + "kind":"TestDefaulter" + }, + "resource":{ + "group":"", + "version":"v1", + "resource":"testdefaulter" + }, + "namespace":"default", + "operation":"CREATE", + "object":{ + "panic":true + }, + "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":false`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":500`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: fake panic test [recovered]`)) + }) + It("should scaffold a defaulting webhook with a custom defaulter", func() { By("creating a controller manager") m, err := manager.New(cfg, manager.Options{}) @@ -230,6 +293,74 @@ func runTests(admissionReviewVersion string) { EventuallyWithOffset(1, logBuffer).Should(gbytes.Say(`"msg":"Validating object","object":{"name":"foo","namespace":"default"},"namespace":"default","name":"foo","resource":{"group":"foo.test.org","version":"v1","resource":"testvalidator"},"user":"","requestID":"07e52e8d-4513-11e9-a716-42010a800270"`)) }) + It("should scaffold a customValidator webhook which recovers from panics", 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: testValidatorGVK.GroupVersion()} + builder.Register(&TestValidator{}, &TestValidatorList{}) + err = builder.AddToScheme(m.GetScheme()) + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + + err = WebhookManagedBy(m). + For(&TestValidator{Panic: true}). + WithValidator(&TestCustomValidator{}). + RecoverPanic(true). + 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":"", + "version":"v1", + "kind":"TestValidator" + }, + + "resource":{ + "group":"", + + "version":"v1", + "resource":"testvalidator" + }, + "namespace":"default", + "operation":"CREATE", + + "object":{ + "replica":2, + "panic":true + } + } +}`) + + 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 validating webhook path") + path := generateValidatePath(testValidatorGVK) + _, 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.StatusOK)) + By("sanity checking the response contains reasonable field") + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":500`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: fake panic test [recovered]`)) + }) + It("should send an error when trying to register a webhook with more than one For", func() { By("creating a controller manager") m, err := manager.New(cfg, manager.Options{}) @@ -247,6 +378,102 @@ func runTests(admissionReviewVersion string) { Complete() Expect(err).To(HaveOccurred()) }) + + It("should scaffold a custom validator webhook if the type implements the CustomValidator interface to validate deletes", func() { + By("creating a controller manager") + ctx, cancel := context.WithCancel(context.Background()) + + m, err := manager.New(cfg, manager.Options{}) + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + + By("registering the type in the Scheme") + builder := scheme.Builder{GroupVersion: testValidatorGVK.GroupVersion()} + builder.Register(&TestValidator{}, &TestValidatorList{}) + err = builder.AddToScheme(m.GetScheme()) + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + + err = WebhookManagedBy(m). + For(&TestValidator{}). + WithValidator(&TestCustomValidator{}). + 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":"", + "version":"v1", + "kind":"TestValidator" + + }, + "resource":{ + "group":"", + "version":"v1", + "resource":"testvalidator" + }, + "namespace":"default", + "operation":"DELETE", + "object":null, + "oldObject":{ + "replica":1 + } + } +}`) + + cancel() + err = svr.Start(ctx) + if err != nil && !os.IsNotExist(err) { + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + } + + By("sending a request to a validating webhook path to check for failed delete") + path := generateValidatePath(testValidatorGVK) + 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 field") + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":403`)) + + reader = strings.NewReader(admissionReviewGV + admissionReviewVersion + `", + "request":{ + "uid":"07e52e8d-4513-11e9-a716-42010a800270", + + "kind":{ + "group":"", + "version":"v1", + "kind":"TestValidator" + }, + "resource":{ + + "group":"", + "version":"v1", + "resource":"testvalidator" + }, + "namespace":"default", + "operation":"DELETE", + "object":null, + "oldObject":{ + "replica":0 + } + } +}`) + By("sending a request to a validating webhook path with correct request") + path = generateValidatePath(testValidatorGVK) + 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 field") + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":200`)) + }) } // TestDefaulter. @@ -431,9 +658,13 @@ func (*TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) err } d := obj.(*TestDefaulter) //nolint:ifshort + if d.Replica < 2 { d.Replica = 2 } + if d.Panic { + panic("fake panic test") + } return nil } @@ -454,6 +685,9 @@ func (*TestCustomValidator) ValidateCreate(ctx context.Context, obj runtime.Obje } v := obj.(*TestValidator) //nolint:ifshort + if v.Panic { + panic("fake panic test") + } if v.Replica < 0 { return nil, errors.New("number of replica should be greater than or equal to 0") } From d1ef9cd2d1313da2adc8d9d5927611733fbf4c9a Mon Sep 17 00:00:00 2001 From: Troy Connor Date: Thu, 22 Aug 2024 12:47:36 -0400 Subject: [PATCH 3/4] remove deprecated funcs, add test for customDefaulter Signed-off-by: Troy Connor --- pkg/builder/webhook_test.go | 72 --- .../admission/defaulter_custom_test.go | 85 +++ .../admission/validator_custom_test.go | 508 ++++++++++++++++++ 3 files changed, 593 insertions(+), 72 deletions(-) create mode 100644 pkg/webhook/admission/defaulter_custom_test.go create mode 100644 pkg/webhook/admission/validator_custom_test.go diff --git a/pkg/builder/webhook_test.go b/pkg/builder/webhook_test.go index 8581c74e70..3f529cd43e 100644 --- a/pkg/builder/webhook_test.go +++ b/pkg/builder/webhook_test.go @@ -508,15 +508,6 @@ type TestDefaulterList struct{} func (*TestDefaulterList) GetObjectKind() schema.ObjectKind { return nil } func (*TestDefaulterList) DeepCopyObject() runtime.Object { return nil } -func (d *TestDefaulter) Default() { - if d.Panic { - panic("fake panic test") - } - if d.Replica < 2 { - d.Replica = 2 - } -} - // TestValidator. var _ runtime.Object = &TestValidator{} @@ -549,41 +540,6 @@ type TestValidatorList struct{} func (*TestValidatorList) GetObjectKind() schema.ObjectKind { return nil } func (*TestValidatorList) DeepCopyObject() runtime.Object { return nil } -func (v *TestValidator) ValidateCreate() (admission.Warnings, error) { - if v.Panic { - panic("fake panic test") - } - if v.Replica < 0 { - return nil, errors.New("number of replica should be greater than or equal to 0") - } - return nil, nil -} - -func (v *TestValidator) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - if v.Panic { - panic("fake panic test") - } - if v.Replica < 0 { - return nil, errors.New("number of replica should be greater than or equal to 0") - } - if oldObj, ok := old.(*TestValidator); !ok { - return nil, fmt.Errorf("the old object is expected to be %T", oldObj) - } else if v.Replica < oldObj.Replica { - return nil, fmt.Errorf("new replica %v should not be fewer than old replica %v", v.Replica, oldObj.Replica) - } - return nil, nil -} - -func (v *TestValidator) ValidateDelete() (admission.Warnings, error) { - if v.Panic { - panic("fake panic test") - } - if v.Replica > 0 { - return nil, errors.New("number of replica should be less than or equal to 0 to delete") - } - return nil, nil -} - // TestDefaultValidator. var _ runtime.Object = &TestDefaultValidator{} @@ -616,35 +572,7 @@ type TestDefaultValidatorList struct{} func (*TestDefaultValidatorList) GetObjectKind() schema.ObjectKind { return nil } func (*TestDefaultValidatorList) DeepCopyObject() runtime.Object { return nil } -func (dv *TestDefaultValidator) Default() { - if dv.Replica < 2 { - dv.Replica = 2 - } -} - -func (dv *TestDefaultValidator) ValidateCreate() (admission.Warnings, error) { - if dv.Replica < 0 { - return nil, errors.New("number of replica should be greater than or equal to 0") - } - return nil, nil -} - -func (dv *TestDefaultValidator) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { - if dv.Replica < 0 { - return nil, errors.New("number of replica should be greater than or equal to 0") - } - return nil, nil -} - -func (dv *TestDefaultValidator) ValidateDelete() (admission.Warnings, error) { - if dv.Replica > 0 { - return nil, errors.New("number of replica should be less than or equal to 0 to delete") - } - return nil, nil -} - // TestCustomDefaulter. - type TestCustomDefaulter struct{} func (*TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error { diff --git a/pkg/webhook/admission/defaulter_custom_test.go b/pkg/webhook/admission/defaulter_custom_test.go new file mode 100644 index 0000000000..f1063ffe32 --- /dev/null +++ b/pkg/webhook/admission/defaulter_custom_test.go @@ -0,0 +1,85 @@ +/* +Copyright 2021 The Kubernetes Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package admission + +import ( + "context" + "net/http" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + admissionv1 "k8s.io/api/admission/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +var _ = Describe("Defaulter Handler", func() { + + It("should return ok if received delete verb in defaulter handler", func() { + obj := &TestDefaulter{} + handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}) + resp := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Delete, + OldObject: runtime.RawExtension{ + Raw: []byte("{}"), + }, + }, + }) + Expect(resp.Allowed).Should(BeTrue()) + Expect(resp.Result.Code).Should(Equal(int32(http.StatusOK))) + }) +}) + +// TestDefaulter. +var _ runtime.Object = &TestDefaulter{} + +type TestDefaulter struct { + Replica int `json:"replica,omitempty"` +} + +var testDefaulterGVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: "TestDefaulter"} + +func (d *TestDefaulter) GetObjectKind() schema.ObjectKind { return d } +func (d *TestDefaulter) DeepCopyObject() runtime.Object { + return &TestDefaulter{ + Replica: d.Replica, + } +} + +func (d *TestDefaulter) GroupVersionKind() schema.GroupVersionKind { + return testDefaulterGVK +} + +func (d *TestDefaulter) SetGroupVersionKind(gvk schema.GroupVersionKind) {} + +var _ runtime.Object = &TestDefaulterList{} + +type TestDefaulterList struct{} + +func (*TestDefaulterList) GetObjectKind() schema.ObjectKind { return nil } +func (*TestDefaulterList) DeepCopyObject() runtime.Object { return nil } + +// TestCustomDefaulter +type TestCustomDefaulter struct{} + +func (d *TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error { + o := obj.(*TestDefaulter) + if o.Replica < 2 { + o.Replica = 2 + } + return nil +} diff --git a/pkg/webhook/admission/validator_custom_test.go b/pkg/webhook/admission/validator_custom_test.go new file mode 100644 index 0000000000..0e783560a1 --- /dev/null +++ b/pkg/webhook/admission/validator_custom_test.go @@ -0,0 +1,508 @@ +/* +Copyright 2021 The Kubernetes Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package admission + +import ( + "context" + "errors" + "net/http" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + admissionv1 "k8s.io/api/admission/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +var fakeValidatorVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: "fakeValidator"} + +var _ = Describe("customValidatingHandler", func() { + + Context("when dealing with successful results without warning", func() { + val := &fakeCustomValidator{ErrorToReturn: nil, GVKToReturn: fakeValidatorVK, WarningsToReturn: nil} + f := &fakeValidator{} + handler := WithCustomValidator(admissionScheme, f, val) + + It("should return 200 in response when create succeeds", func() { + + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + + Expect(response.Allowed).Should(BeTrue()) + Expect(response.Result.Code).Should(Equal(int32(http.StatusOK))) + }) + + It("should return 200 in response when update succeeds", func() { + + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Object: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + OldObject: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + Expect(response.Allowed).Should(BeTrue()) + Expect(response.Result.Code).Should(Equal(int32(http.StatusOK))) + }) + + It("should return 200 in response when delete succeeds", func() { + + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Delete, + OldObject: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + Expect(response.Allowed).Should(BeTrue()) + Expect(response.Result.Code).Should(Equal(int32(http.StatusOK))) + }) + }) + + const warningMessage = "warning message" + const anotherWarningMessage = "another warning message" + Context("when dealing with successful results with warning", func() { + f := &fakeValidator{} + val := &fakeCustomValidator{ErrorToReturn: nil, GVKToReturn: fakeValidatorVK, WarningsToReturn: []string{ + warningMessage, + anotherWarningMessage, + }} + handler := WithCustomValidator(admissionScheme, f, val) + It("should return 200 in response when create succeeds, with warning messages", func() { + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + + Expect(response.Allowed).Should(BeTrue()) + Expect(response.Result.Code).Should(Equal(int32(http.StatusOK))) + Expect(response.AdmissionResponse.Warnings).Should(ContainElement(warningMessage)) + Expect(response.AdmissionResponse.Warnings).Should(ContainElement(anotherWarningMessage)) + }) + + It("should return 200 in response when update succeeds, with warning messages", func() { + + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Object: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + OldObject: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + Expect(response.Allowed).Should(BeTrue()) + Expect(response.Result.Code).Should(Equal(int32(http.StatusOK))) + Expect(response.AdmissionResponse.Warnings).Should(ContainElement(warningMessage)) + Expect(response.AdmissionResponse.Warnings).Should(ContainElement(anotherWarningMessage)) + }) + + It("should return 200 in response when delete succeeds, with warning messages", func() { + + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Delete, + OldObject: runtime.RawExtension{ + Raw: []byte("{}"), + + Object: f, + }, + }, + }) + Expect(response.Allowed).Should(BeTrue()) + Expect(response.Result.Code).Should(Equal(int32(http.StatusOK))) + Expect(response.AdmissionResponse.Warnings).Should(ContainElement(warningMessage)) + Expect(response.AdmissionResponse.Warnings).Should(ContainElement(anotherWarningMessage)) + }) + }) + + Context("when dealing with Status errors, with warning messages", func() { + // Status error would overwrite the warning messages, so no warning messages should be observed. + expectedError := &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Message: "some message", + Code: http.StatusUnprocessableEntity, + }, + } + f := &fakeValidator{} + val := &fakeCustomValidator{ErrorToReturn: expectedError, GVKToReturn: fakeValidatorVK, WarningsToReturn: []string{warningMessage, anotherWarningMessage}} + handler := WithCustomValidator(admissionScheme, f, val) + + It("should propagate the Status from ValidateCreate's return value to the HTTP response", func() { + + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + + Expect(response.Allowed).Should(BeFalse()) + Expect(response.Result.Code).Should(Equal(expectedError.Status().Code)) + Expect(*response.Result).Should(Equal(expectedError.Status())) + Expect(response.AdmissionResponse.Warnings).Should(ContainElements(warningMessage)) + Expect(response.AdmissionResponse.Warnings).Should(ContainElements(anotherWarningMessage)) + + }) + + It("should propagate the Status from ValidateUpdate's return value to the HTTP response", func() { + + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Object: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + OldObject: runtime.RawExtension{ + Raw: []byte("{}"), + + Object: f, + }, + }, + }) + + Expect(response.Allowed).Should(BeFalse()) + Expect(response.Result.Code).Should(Equal(expectedError.Status().Code)) + Expect(*response.Result).Should(Equal(expectedError.Status())) + Expect(response.AdmissionResponse.Warnings).Should(ContainElements(warningMessage)) + Expect(response.AdmissionResponse.Warnings).Should(ContainElements(anotherWarningMessage)) + + }) + + It("should propagate the Status from ValidateDelete's return value to the HTTP response", func() { + + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Delete, + + OldObject: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + + Expect(response.Allowed).Should(BeFalse()) + Expect(response.Result.Code).Should(Equal(expectedError.Status().Code)) + Expect(*response.Result).Should(Equal(expectedError.Status())) + Expect(response.AdmissionResponse.Warnings).Should(ContainElements(warningMessage)) + Expect(response.AdmissionResponse.Warnings).Should(ContainElements(anotherWarningMessage)) + + }) + + }) + + Context("when dealing with Status errors, without warning messages", func() { + + expectedError := &apierrors.StatusError{ + ErrStatus: metav1.Status{ + Message: "some message", + Code: http.StatusUnprocessableEntity, + }, + } + f := &fakeValidator{} + val := &fakeCustomValidator{ErrorToReturn: expectedError, GVKToReturn: fakeValidatorVK, WarningsToReturn: nil} + handler := WithCustomValidator(admissionScheme, f, val) + + It("should propagate the Status from ValidateCreate's return value to the HTTP response", func() { + + response := handler.Handle(context.TODO(), Request{ + + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + + Expect(response.Allowed).Should(BeFalse()) + Expect(response.Result.Code).Should(Equal(expectedError.Status().Code)) + Expect(*response.Result).Should(Equal(expectedError.Status())) + + }) + + It("should propagate the Status from ValidateUpdate's return value to the HTTP response", func() { + + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Object: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + OldObject: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + + Expect(response.Allowed).Should(BeFalse()) + Expect(response.Result.Code).Should(Equal(expectedError.Status().Code)) + Expect(*response.Result).Should(Equal(expectedError.Status())) + + }) + + It("should propagate the Status from ValidateDelete's return value to the HTTP response", func() { + + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Delete, + OldObject: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + + Expect(response.Allowed).Should(BeFalse()) + Expect(response.Result.Code).Should(Equal(expectedError.Status().Code)) + + Expect(*response.Result).Should(Equal(expectedError.Status())) + + }) + + }) + + Context("when dealing with non-status errors, without warning messages", func() { + + expectedError := errors.New("some error") + f := &fakeValidator{} + val := &fakeCustomValidator{ErrorToReturn: expectedError, GVKToReturn: fakeValidatorVK} + handler := WithCustomValidator(admissionScheme, f, val) + + It("should return 403 response when ValidateCreate with error message embedded", func() { + + response := handler.Handle(context.TODO(), Request{ + + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + Expect(response.Allowed).Should(BeFalse()) + Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) + Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) + Expect(response.Result.Message).Should(Equal(expectedError.Error())) + + }) + + It("should return 403 response when ValidateUpdate returns non-APIStatus error", func() { + + response := handler.Handle(context.TODO(), Request{ + + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Object: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + OldObject: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + Expect(response.Allowed).Should(BeFalse()) + Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) + Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) + Expect(response.Result.Message).Should(Equal(expectedError.Error())) + + }) + + It("should return 403 response when ValidateDelete returns non-APIStatus error", func() { + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Delete, + OldObject: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + Expect(response.Allowed).Should(BeFalse()) + Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) + Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) + Expect(response.Result.Message).Should(Equal(expectedError.Error())) + }) + }) + + Context("when dealing with non-status errors, with warning messages", func() { + + expectedError := errors.New("some error") + f := &fakeValidator{} + val := &fakeCustomValidator{ErrorToReturn: expectedError, GVKToReturn: fakeValidatorVK, WarningsToReturn: []string{warningMessage, anotherWarningMessage}} + handler := WithCustomValidator(admissionScheme, f, val) + + It("should return 403 response when ValidateCreate with error message embedded", func() { + + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + Expect(response.Allowed).Should(BeFalse()) + + Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) + Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) + Expect(response.Result.Message).Should(Equal(expectedError.Error())) + Expect(response.AdmissionResponse.Warnings).Should(ContainElement(warningMessage)) + Expect(response.AdmissionResponse.Warnings).Should(ContainElement(anotherWarningMessage)) + }) + + It("should return 403 response when ValidateUpdate returns non-APIStatus error", func() { + + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Object: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + OldObject: runtime.RawExtension{ + + Raw: []byte("{}"), + Object: f, + }, + }, + }) + Expect(response.Allowed).Should(BeFalse()) + Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) + Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) + Expect(response.Result.Message).Should(Equal(expectedError.Error())) + + Expect(response.AdmissionResponse.Warnings).Should(ContainElement(warningMessage)) + Expect(response.AdmissionResponse.Warnings).Should(ContainElement(anotherWarningMessage)) + + }) + + It("should return 403 response when ValidateDelete returns non-APIStatus error", func() { + response := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Delete, + OldObject: runtime.RawExtension{ + Raw: []byte("{}"), + Object: f, + }, + }, + }) + Expect(response.Allowed).Should(BeFalse()) + Expect(response.Result.Code).Should(Equal(int32(http.StatusForbidden))) + Expect(response.Result.Reason).Should(Equal(metav1.StatusReasonForbidden)) + Expect(response.Result.Message).Should(Equal(expectedError.Error())) + Expect(response.AdmissionResponse.Warnings).Should(ContainElement(warningMessage)) + Expect(response.AdmissionResponse.Warnings).Should(ContainElement(anotherWarningMessage)) + + }) + }) + + PIt("should return 400 in response when create fails on decode", func() {}) + + PIt("should return 400 in response when update fails on decoding new object", func() {}) + + PIt("should return 400 in response when update fails on decoding old object", func() {}) + + PIt("should return 400 in response when delete fails on decode", func() {}) + +}) + +// fakeCustomValidator provides fake validating webhook functionality for testing +// It implements the admission.CustomValidator interface and +// rejects all requests with the same configured error +// or passes if ErrorToReturn is nil. +// And it would always return configured warning messages WarningsToReturn. +type fakeCustomValidator struct { + // ErrorToReturn is the error for which the fakeValidator rejects all requests + ErrorToReturn error `json:"errorToReturn,omitempty"` + // GVKToReturn is the GroupVersionKind that the webhook operates on + GVKToReturn schema.GroupVersionKind + // WarningsToReturn is the warnings for fakeValidator returns to all requests + WarningsToReturn []string +} + +func (v *fakeCustomValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (warnings Warnings, err error) { + return v.WarningsToReturn, v.ErrorToReturn +} + +func (v *fakeCustomValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (warnings Warnings, err error) { + return v.WarningsToReturn, v.ErrorToReturn +} + +func (v *fakeCustomValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (warnings Warnings, err error) { + return v.WarningsToReturn, v.ErrorToReturn +} + +type fakeValidator struct { + // GVKToReturn is the GroupVersionKind that the webhook operates on + GVKToReturn schema.GroupVersionKind +} + +func (v *fakeValidator) SetGroupVersionKind(gvk schema.GroupVersionKind) { + v.GVKToReturn = gvk +} + +func (v *fakeValidator) GroupVersionKind() schema.GroupVersionKind { + return v.GVKToReturn +} + +func (v *fakeValidator) GetObjectKind() schema.ObjectKind { + return v +} + +func (v *fakeValidator) DeepCopyObject() runtime.Object { + return &fakeValidator{ + GVKToReturn: v.GVKToReturn, + } +} From b265d8d009a4bd52192b4d91523f865187475c37 Mon Sep 17 00:00:00 2001 From: Troy Connor Date: Mon, 26 Aug 2024 11:09:17 -0400 Subject: [PATCH 4/4] minimize the diff Signed-off-by: Troy Connor --- pkg/builder/webhook_test.go | 195 ++++++++++++++++++++++++++++++------ 1 file changed, 166 insertions(+), 29 deletions(-) diff --git a/pkg/builder/webhook_test.go b/pkg/builder/webhook_test.go index 3f529cd43e..106825b2d1 100644 --- a/pkg/builder/webhook_test.go +++ b/pkg/builder/webhook_test.go @@ -77,7 +77,78 @@ func runTests(admissionReviewVersion string) { close(stop) }) - It("should scaffold a custom defaulter webhook which recovers from panics", func() { + It("should scaffold a custom defaulting webhook if the type implements the CustomDefaulter interface", 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). + For(&TestDefaulter{}). + WithDefaulter(&TestCustomDefaulter{}). + 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":"", + "version":"v1", + "kind":"TestDefaulter" + }, + "resource":{ + "group":"", + "version":"v1", + "resource":"testdefaulter" + }, + "namespace":"default", + "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`)) + + 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 defaulting webhook which recovers from panics", func() { By("creating a controller manager") m, err := manager.New(cfg, manager.Options{}) ExpectWithOffset(1, err).NotTo(HaveOccurred()) @@ -114,6 +185,7 @@ func runTests(admissionReviewVersion string) { "namespace":"default", "operation":"CREATE", "object":{ + "replica":1, "panic":true }, "oldObject":null @@ -216,7 +288,7 @@ func runTests(admissionReviewVersion string) { ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound)) }) - It("should scaffold a validating webhook with a custom validator", func() { + It("should scaffold a custom validating webhook if the type implements the CustomValidator interface", func() { By("creating a controller manager") m, err := manager.New(cfg, manager.Options{}) ExpectWithOffset(1, err).NotTo(HaveOccurred()) @@ -228,11 +300,8 @@ func runTests(admissionReviewVersion string) { ExpectWithOffset(1, err).NotTo(HaveOccurred()) err = WebhookManagedBy(m). - WithValidator(&TestCustomValidator{}). For(&TestValidator{}). - WithLogConstructor(func(base logr.Logger, req *admission.Request) logr.Logger { - return admission.DefaultLogConstructor(testingLogger, req) - }). + WithValidator(&TestCustomValidator{}). Complete() ExpectWithOffset(1, err).NotTo(HaveOccurred()) svr := m.GetWebhookServer() @@ -242,17 +311,16 @@ func runTests(admissionReviewVersion string) { "request":{ "uid":"07e52e8d-4513-11e9-a716-42010a800270", "kind":{ - "group":"foo.test.org", + "group":"", "version":"v1", "kind":"TestValidator" }, "resource":{ - "group":"foo.test.org", + "group":"", "version":"v1", "resource":"testvalidator" }, "namespace":"default", - "name":"foo", "operation":"UPDATE", "object":{ "replica":1 @@ -290,13 +358,11 @@ func runTests(admissionReviewVersion string) { By("sanity checking the response contains reasonable field") ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`)) ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":403`)) - EventuallyWithOffset(1, logBuffer).Should(gbytes.Say(`"msg":"Validating object","object":{"name":"foo","namespace":"default"},"namespace":"default","name":"foo","resource":{"group":"foo.test.org","version":"v1","resource":"testvalidator"},"user":"","requestID":"07e52e8d-4513-11e9-a716-42010a800270"`)) }) - It("should scaffold a customValidator webhook which recovers from panics", func() { + It("should scaffold a custom validating webhook which recovers from panics", 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") @@ -306,7 +372,7 @@ func runTests(admissionReviewVersion string) { ExpectWithOffset(1, err).NotTo(HaveOccurred()) err = WebhookManagedBy(m). - For(&TestValidator{Panic: true}). + For(&TestValidator{}). WithValidator(&TestCustomValidator{}). RecoverPanic(true). Complete() @@ -322,16 +388,13 @@ func runTests(admissionReviewVersion string) { "version":"v1", "kind":"TestValidator" }, - "resource":{ "group":"", - "version":"v1", "resource":"testvalidator" }, "namespace":"default", "operation":"CREATE", - "object":{ "replica":2, "panic":true @@ -361,25 +424,84 @@ func runTests(admissionReviewVersion string) { ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: fake panic test [recovered]`)) }) - It("should send an error when trying to register a webhook with more than one For", func() { + It("should scaffold a validating webhook with a custom validator", 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{}) + builder := scheme.Builder{GroupVersion: testValidatorGVK.GroupVersion()} + builder.Register(&TestValidator{}, &TestValidatorList{}) err = builder.AddToScheme(m.GetScheme()) ExpectWithOffset(1, err).NotTo(HaveOccurred()) err = WebhookManagedBy(m). - For(&TestDefaulter{}). - For(&TestDefaulter{}). + WithValidator(&TestCustomValidator{}). + For(&TestValidator{}). + WithLogConstructor(func(base logr.Logger, req *admission.Request) logr.Logger { + return admission.DefaultLogConstructor(testingLogger, req) + }). Complete() - Expect(err).To(HaveOccurred()) + 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":"TestValidator" + }, + "resource":{ + "group":"foo.test.org", + "version":"v1", + "resource":"testvalidator" + }, + "namespace":"default", + "name":"foo", + "operation":"UPDATE", + "object":{ + "replica":1 + }, + "oldObject":{ + "replica":2 + } + } +}`) + + 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 that doesn't exist") + path := generateMutatePath(testValidatorGVK) + 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)) + + By("sending a request to a validating webhook path") + path = generateValidatePath(testValidatorGVK) + _, 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.StatusOK)) + By("sanity checking the response contains reasonable field") + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":403`)) + EventuallyWithOffset(1, logBuffer).Should(gbytes.Say(`"msg":"Validating object","object":{"name":"foo","namespace":"default"},"namespace":"default","name":"foo","resource":{"group":"foo.test.org","version":"v1","resource":"testvalidator"},"user":"","requestID":"07e52e8d-4513-11e9-a716-42010a800270"`)) }) - It("should scaffold a custom validator webhook if the type implements the CustomValidator interface to validate deletes", func() { + It("should scaffold a custom validating webhook if the type implements the CustomValidator interface to validate deletes", func() { By("creating a controller manager") ctx, cancel := context.WithCancel(context.Background()) @@ -407,7 +529,6 @@ func runTests(admissionReviewVersion string) { "group":"", "version":"v1", "kind":"TestValidator" - }, "resource":{ "group":"", @@ -443,14 +564,12 @@ func runTests(admissionReviewVersion string) { reader = strings.NewReader(admissionReviewGV + admissionReviewVersion + `", "request":{ "uid":"07e52e8d-4513-11e9-a716-42010a800270", - "kind":{ "group":"", "version":"v1", "kind":"TestValidator" }, "resource":{ - "group":"", "version":"v1", "resource":"testvalidator" @@ -474,6 +593,24 @@ func runTests(admissionReviewVersion string) { ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`)) ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":200`)) }) + + It("should send an error when trying to register a webhook with more than one For", 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). + For(&TestDefaulter{}). + For(&TestDefaulter{}). + Complete() + Expect(err).To(HaveOccurred()) + }) } // TestDefaulter. @@ -586,13 +723,13 @@ func (*TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) err } d := obj.(*TestDefaulter) //nolint:ifshort + if d.Panic { + panic("fake panic test") + } if d.Replica < 2 { d.Replica = 2 } - if d.Panic { - panic("fake panic test") - } return nil }