From eef6e7b4c54e541c9ea238d854b5f84821150a3d Mon Sep 17 00:00:00 2001 From: Aldo Culquicondor Date: Thu, 22 Aug 2024 20:39:05 +0000 Subject: [PATCH 1/7] Fix custom defaulter from deleting unknown fields --- pkg/webhook/admission/defaulter_custom.go | 20 ++++++++++++----- .../admission/defaulter_custom_test.go | 22 +++++++++++++++++++ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/pkg/webhook/admission/defaulter_custom.go b/pkg/webhook/admission/defaulter_custom.go index d15dec7a05..5687676527 100644 --- a/pkg/webhook/admission/defaulter_custom.go +++ b/pkg/webhook/admission/defaulter_custom.go @@ -71,13 +71,14 @@ func (h *defaulterForType) Handle(ctx context.Context, req Request) Response { ctx = NewContextWithRequest(ctx, req) // Get the object in the request - obj := h.object.DeepCopyObject() - if err := h.decoder.Decode(req, obj); err != nil { + original := h.object.DeepCopyObject() + if err := h.decoder.Decode(req, original); err != nil { return Errored(http.StatusBadRequest, err) } // Default the object - if err := h.defaulter.Default(ctx, obj); err != nil { + updated := original.DeepCopyObject() + if err := h.defaulter.Default(ctx, updated); err != nil { var apiStatus apierrors.APIStatus if errors.As(err, &apiStatus) { return validationResponseFromStatus(false, apiStatus.Status()) @@ -85,10 +86,17 @@ func (h *defaulterForType) Handle(ctx context.Context, req Request) Response { return Denied(err.Error()) } - // Create the patch - marshalled, err := json.Marshal(obj) + // Create the patch. + // We need to decode and marshall the original because the type registered in the + // decoder might not match the latest version of the API. + // Creating a diff from the raw object might cause new fields to be dropped. + marshalledOriginal, err := json.Marshal(original) if err != nil { return Errored(http.StatusInternalServerError, err) } - return PatchResponseFromRaw(req.Object.Raw, marshalled) + marshalledUpdated, err := json.Marshal(updated) + if err != nil { + return Errored(http.StatusInternalServerError, err) + } + return PatchResponseFromRaw(marshalledOriginal, marshalledUpdated) } diff --git a/pkg/webhook/admission/defaulter_custom_test.go b/pkg/webhook/admission/defaulter_custom_test.go index f1063ffe32..7f27bd7ec1 100644 --- a/pkg/webhook/admission/defaulter_custom_test.go +++ b/pkg/webhook/admission/defaulter_custom_test.go @@ -20,6 +20,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "gomodules.xyz/jsonpatch/v2" admissionv1 "k8s.io/api/admission/v1" "k8s.io/apimachinery/pkg/runtime" @@ -28,6 +29,27 @@ import ( var _ = Describe("Defaulter Handler", func() { + It("should should not lose unknown fields", func() { + obj := &TestDefaulter{} + handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}) + + resp := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: []byte(`{"newField":"foo"}`), + }, + }, + }) + Expect(resp.Allowed).Should(BeTrue()) + Expect(resp.Patches).To(Equal([]jsonpatch.JsonPatchOperation{{ + Operation: "add", + Path: "/replica", + Value: 2.0, + }})) + Expect(resp.Result.Code).Should(Equal(int32(http.StatusOK))) + }) + It("should return ok if received delete verb in defaulter handler", func() { obj := &TestDefaulter{} handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}) From 80dc8583dd42dfd28e635c7a400058efa580bb14 Mon Sep 17 00:00:00 2001 From: Traian Schiau Date: Fri, 11 Oct 2024 13:08:45 +0300 Subject: [PATCH 2/7] Use a zero-change patch to do find scheme caused fields removal. --- pkg/webhook/admission/defaulter_custom.go | 52 +++++++++++++++---- .../admission/defaulter_custom_test.go | 27 ++++++---- 2 files changed, 59 insertions(+), 20 deletions(-) diff --git a/pkg/webhook/admission/defaulter_custom.go b/pkg/webhook/admission/defaulter_custom.go index 5687676527..e3839176c7 100644 --- a/pkg/webhook/admission/defaulter_custom.go +++ b/pkg/webhook/admission/defaulter_custom.go @@ -21,11 +21,14 @@ import ( "encoding/json" "errors" "net/http" + "slices" + "gomodules.xyz/jsonpatch/v2" 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/util/sets" ) // CustomDefaulter defines functions for setting defaults on resources. @@ -71,14 +74,16 @@ func (h *defaulterForType) Handle(ctx context.Context, req Request) Response { ctx = NewContextWithRequest(ctx, req) // Get the object in the request - original := h.object.DeepCopyObject() - if err := h.decoder.Decode(req, original); err != nil { + obj := h.object.DeepCopyObject() + if err := h.decoder.Decode(req, obj); err != nil { return Errored(http.StatusBadRequest, err) } + // Keep a copy of the object + originalObj := obj.DeepCopyObject() + // Default the object - updated := original.DeepCopyObject() - if err := h.defaulter.Default(ctx, updated); err != nil { + if err := h.defaulter.Default(ctx, obj); err != nil { var apiStatus apierrors.APIStatus if errors.As(err, &apiStatus) { return validationResponseFromStatus(false, apiStatus.Status()) @@ -86,17 +91,42 @@ func (h *defaulterForType) Handle(ctx context.Context, req Request) Response { return Denied(err.Error()) } - // Create the patch. - // We need to decode and marshall the original because the type registered in the - // decoder might not match the latest version of the API. - // Creating a diff from the raw object might cause new fields to be dropped. - marshalledOriginal, err := json.Marshal(original) + // Create the patch + marshalled, err := json.Marshal(obj) if err != nil { return Errored(http.StatusInternalServerError, err) } - marshalledUpdated, err := json.Marshal(updated) + handlerResponse := PatchResponseFromRaw(req.Object.Raw, marshalled) + + return h.dropSchemeRemovals(handlerResponse, originalObj, req.Object.Raw) +} + +func (h *defaulterForType) dropSchemeRemovals(r Response, original runtime.Object, raw []byte) Response { + const opRemove = "remove" + if !r.Allowed || r.PatchType == nil { + return r + } + + // If we don't have removals in the patch. + if !slices.ContainsFunc(r.Patches, func(o jsonpatch.JsonPatchOperation) bool { return o.Operation == opRemove }) { + return r + } + + // Get the raw to original patch + marshalledOriginal, err := json.Marshal(original) if err != nil { return Errored(http.StatusInternalServerError, err) } - return PatchResponseFromRaw(marshalledOriginal, marshalledUpdated) + + patchOriginal := PatchResponseFromRaw(raw, marshalledOriginal).Patches + removedByScheme := sets.New(slices.DeleteFunc(patchOriginal, func(p jsonpatch.JsonPatchOperation) bool { return p.Operation != opRemove })...) + + r.Patches = slices.DeleteFunc(r.Patches, func(p jsonpatch.JsonPatchOperation) bool { + return removedByScheme.Has(p) + }) + + if len(r.Patches) == 0 { + r.PatchType = nil + } + return r } diff --git a/pkg/webhook/admission/defaulter_custom_test.go b/pkg/webhook/admission/defaulter_custom_test.go index 7f27bd7ec1..b454411968 100644 --- a/pkg/webhook/admission/defaulter_custom_test.go +++ b/pkg/webhook/admission/defaulter_custom_test.go @@ -29,7 +29,7 @@ import ( var _ = Describe("Defaulter Handler", func() { - It("should should not lose unknown fields", func() { + It("should not lose unknown fields", func() { obj := &TestDefaulter{} handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}) @@ -37,16 +37,22 @@ var _ = Describe("Defaulter Handler", func() { AdmissionRequest: admissionv1.AdmissionRequest{ Operation: admissionv1.Create, Object: runtime.RawExtension{ - Raw: []byte(`{"newField":"foo"}`), + Raw: []byte(`{"newField":"foo", "totalReplicas":5}`), }, }, }) Expect(resp.Allowed).Should(BeTrue()) - Expect(resp.Patches).To(Equal([]jsonpatch.JsonPatchOperation{{ - Operation: "add", - Path: "/replica", - Value: 2.0, - }})) + Expect(resp.Patches).To(Equal([]jsonpatch.JsonPatchOperation{ + { + Operation: "add", + Path: "/replica", + Value: 2.0, + }, + { + Operation: "remove", + Path: "/totalReplicas", + }, + })) Expect(resp.Result.Code).Should(Equal(int32(http.StatusOK))) }) @@ -70,7 +76,8 @@ var _ = Describe("Defaulter Handler", func() { var _ runtime.Object = &TestDefaulter{} type TestDefaulter struct { - Replica int `json:"replica,omitempty"` + Replica int `json:"replica,omitempty"` + TotalReplicas int `json:"totalReplicas,omitempty"` } var testDefaulterGVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: "TestDefaulter"} @@ -78,7 +85,8 @@ var testDefaulterGVK = schema.GroupVersionKind{Group: "foo.test.org", Version: " func (d *TestDefaulter) GetObjectKind() schema.ObjectKind { return d } func (d *TestDefaulter) DeepCopyObject() runtime.Object { return &TestDefaulter{ - Replica: d.Replica, + Replica: d.Replica, + TotalReplicas: d.TotalReplicas, } } @@ -103,5 +111,6 @@ func (d *TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) e if o.Replica < 2 { o.Replica = 2 } + o.TotalReplicas = 0 return nil } From 85e50cfa8e3e45758f932402dc71afed3c8ba1d9 Mon Sep 17 00:00:00 2001 From: Traian Schiau Date: Thu, 24 Oct 2024 11:17:21 +0300 Subject: [PATCH 3/7] Add `DefaulterPreserveUnknownFields` An option stops the defaulter from pruning the fields that are not recognized in the local scheme. --- pkg/webhook/admission/defaulter_custom.go | 40 +++++++++++++---- .../admission/defaulter_custom_test.go | 43 ++++++++++++++++--- 2 files changed, 69 insertions(+), 14 deletions(-) diff --git a/pkg/webhook/admission/defaulter_custom.go b/pkg/webhook/admission/defaulter_custom.go index e3839176c7..6050dd4d13 100644 --- a/pkg/webhook/admission/defaulter_custom.go +++ b/pkg/webhook/admission/defaulter_custom.go @@ -36,17 +36,33 @@ type CustomDefaulter interface { Default(ctx context.Context, obj runtime.Object) error } +type defaulterOptions struct { + preserveUnknownFields bool +} + +type defaulterOption func(*defaulterOptions) + +// DefaulterPreserveUnknownFields stops the defaulter from pruning the fields that are not recognized in the local scheme. +func DefaulterPreserveUnknownFields(o *defaulterOptions) { + o.preserveUnknownFields = true +} + // WithCustomDefaulter creates a new Webhook for a CustomDefaulter interface. -func WithCustomDefaulter(scheme *runtime.Scheme, obj runtime.Object, defaulter CustomDefaulter) *Webhook { +func WithCustomDefaulter(scheme *runtime.Scheme, obj runtime.Object, defaulter CustomDefaulter, opts ...defaulterOption) *Webhook { + options := &defaulterOptions{} + for _, o := range opts { + o(options) + } return &Webhook{ - Handler: &defaulterForType{object: obj, defaulter: defaulter, decoder: NewDecoder(scheme)}, + Handler: &defaulterForType{object: obj, defaulter: defaulter, decoder: NewDecoder(scheme), preserveUnknownFields: options.preserveUnknownFields}, } } type defaulterForType struct { - defaulter CustomDefaulter - object runtime.Object - decoder Decoder + defaulter CustomDefaulter + object runtime.Object + decoder Decoder + preserveUnknownFields bool } // Handle handles admission requests. @@ -79,8 +95,11 @@ func (h *defaulterForType) Handle(ctx context.Context, req Request) Response { return Errored(http.StatusBadRequest, err) } - // Keep a copy of the object - originalObj := obj.DeepCopyObject() + // Keep a copy of the object if needed + var originalObj runtime.Object + if h.preserveUnknownFields { + originalObj = obj.DeepCopyObject() + } // Default the object if err := h.defaulter.Default(ctx, obj); err != nil { @@ -96,9 +115,12 @@ func (h *defaulterForType) Handle(ctx context.Context, req Request) Response { if err != nil { return Errored(http.StatusInternalServerError, err) } - handlerResponse := PatchResponseFromRaw(req.Object.Raw, marshalled) - return h.dropSchemeRemovals(handlerResponse, originalObj, req.Object.Raw) + handlerResponse := PatchResponseFromRaw(req.Object.Raw, marshalled) + if h.preserveUnknownFields { + handlerResponse = h.dropSchemeRemovals(handlerResponse, originalObj, req.Object.Raw) + } + return handlerResponse } func (h *defaulterForType) dropSchemeRemovals(r Response, original runtime.Object, raw []byte) Response { diff --git a/pkg/webhook/admission/defaulter_custom_test.go b/pkg/webhook/admission/defaulter_custom_test.go index b454411968..7dda675fd7 100644 --- a/pkg/webhook/admission/defaulter_custom_test.go +++ b/pkg/webhook/admission/defaulter_custom_test.go @@ -29,7 +29,7 @@ import ( var _ = Describe("Defaulter Handler", func() { - It("should not lose unknown fields", func() { + It("should not preserve unknown fields by default", func() { obj := &TestDefaulter{} handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}) @@ -42,17 +42,50 @@ var _ = Describe("Defaulter Handler", func() { }, }) Expect(resp.Allowed).Should(BeTrue()) - Expect(resp.Patches).To(Equal([]jsonpatch.JsonPatchOperation{ - { + Expect(resp.Patches).To(HaveLen(3)) + Expect(resp.Patches).To(ContainElements( + jsonpatch.JsonPatchOperation{ Operation: "add", Path: "/replica", Value: 2.0, }, - { + jsonpatch.JsonPatchOperation{ + Operation: "remove", + Path: "/newField", + }, + jsonpatch.JsonPatchOperation{ + Operation: "remove", + Path: "/totalReplicas", + }, + )) + Expect(resp.Result.Code).Should(Equal(int32(http.StatusOK))) + }) + + It("should preserve unknown fields when DefaulterPreserveUnknownFields is passed", func() { + obj := &TestDefaulter{} + handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}, DefaulterPreserveUnknownFields) + + resp := handler.Handle(context.TODO(), Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Object: runtime.RawExtension{ + Raw: []byte(`{"newField":"foo", "totalReplicas":5}`), + }, + }, + }) + Expect(resp.Allowed).Should(BeTrue()) + Expect(resp.Patches).To(HaveLen(2)) + Expect(resp.Patches).To(ContainElements( + jsonpatch.JsonPatchOperation{ + Operation: "add", + Path: "/replica", + Value: 2.0, + }, + jsonpatch.JsonPatchOperation{ Operation: "remove", Path: "/totalReplicas", }, - })) + )) Expect(resp.Result.Code).Should(Equal(int32(http.StatusOK))) }) From 52c4c3404e4c1e8a6508913fabcbc631fefe2d6b Mon Sep 17 00:00:00 2001 From: Traian Schiau Date: Fri, 25 Oct 2024 09:23:40 +0300 Subject: [PATCH 4/7] Make it opt-out, `DefaulterRemoveUnknownFields` --- pkg/webhook/admission/defaulter_custom.go | 22 +++++++++---------- .../admission/defaulter_custom_test.go | 4 ++-- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/pkg/webhook/admission/defaulter_custom.go b/pkg/webhook/admission/defaulter_custom.go index 6050dd4d13..d10dd449e0 100644 --- a/pkg/webhook/admission/defaulter_custom.go +++ b/pkg/webhook/admission/defaulter_custom.go @@ -37,14 +37,14 @@ type CustomDefaulter interface { } type defaulterOptions struct { - preserveUnknownFields bool + removeUnknownFields bool } type defaulterOption func(*defaulterOptions) -// DefaulterPreserveUnknownFields stops the defaulter from pruning the fields that are not recognized in the local scheme. -func DefaulterPreserveUnknownFields(o *defaulterOptions) { - o.preserveUnknownFields = true +// DefaulterRemoveUnknownFields makes the defaulter prune the fields that are not recognized in the local scheme. +func DefaulterRemoveUnknownFields(o *defaulterOptions) { + o.removeUnknownFields = true } // WithCustomDefaulter creates a new Webhook for a CustomDefaulter interface. @@ -54,15 +54,15 @@ func WithCustomDefaulter(scheme *runtime.Scheme, obj runtime.Object, defaulter C o(options) } return &Webhook{ - Handler: &defaulterForType{object: obj, defaulter: defaulter, decoder: NewDecoder(scheme), preserveUnknownFields: options.preserveUnknownFields}, + Handler: &defaulterForType{object: obj, defaulter: defaulter, decoder: NewDecoder(scheme), removeUnknownFields: options.removeUnknownFields}, } } type defaulterForType struct { - defaulter CustomDefaulter - object runtime.Object - decoder Decoder - preserveUnknownFields bool + defaulter CustomDefaulter + object runtime.Object + decoder Decoder + removeUnknownFields bool } // Handle handles admission requests. @@ -97,7 +97,7 @@ func (h *defaulterForType) Handle(ctx context.Context, req Request) Response { // Keep a copy of the object if needed var originalObj runtime.Object - if h.preserveUnknownFields { + if !h.removeUnknownFields { originalObj = obj.DeepCopyObject() } @@ -117,7 +117,7 @@ func (h *defaulterForType) Handle(ctx context.Context, req Request) Response { } handlerResponse := PatchResponseFromRaw(req.Object.Raw, marshalled) - if h.preserveUnknownFields { + if !h.removeUnknownFields { handlerResponse = h.dropSchemeRemovals(handlerResponse, originalObj, req.Object.Raw) } return handlerResponse diff --git a/pkg/webhook/admission/defaulter_custom_test.go b/pkg/webhook/admission/defaulter_custom_test.go index 7dda675fd7..4d889cee8a 100644 --- a/pkg/webhook/admission/defaulter_custom_test.go +++ b/pkg/webhook/admission/defaulter_custom_test.go @@ -31,7 +31,7 @@ var _ = Describe("Defaulter Handler", func() { It("should not preserve unknown fields by default", func() { obj := &TestDefaulter{} - handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}) + handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}, DefaulterRemoveUnknownFields) resp := handler.Handle(context.TODO(), Request{ AdmissionRequest: admissionv1.AdmissionRequest{ @@ -63,7 +63,7 @@ var _ = Describe("Defaulter Handler", func() { It("should preserve unknown fields when DefaulterPreserveUnknownFields is passed", func() { obj := &TestDefaulter{} - handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}, DefaulterPreserveUnknownFields) + handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}) resp := handler.Handle(context.TODO(), Request{ AdmissionRequest: admissionv1.AdmissionRequest{ From 545a8c5495b1fdae52e28aac4f00a576c1ac3cc9 Mon Sep 17 00:00:00 2001 From: Traian Schiau Date: Fri, 25 Oct 2024 21:30:21 +0300 Subject: [PATCH 5/7] Review Remarks --- pkg/webhook/admission/defaulter_custom.go | 5 ++++- pkg/webhook/admission/defaulter_custom_test.go | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/webhook/admission/defaulter_custom.go b/pkg/webhook/admission/defaulter_custom.go index d10dd449e0..40e030823b 100644 --- a/pkg/webhook/admission/defaulter_custom.go +++ b/pkg/webhook/admission/defaulter_custom.go @@ -140,7 +140,10 @@ func (h *defaulterForType) dropSchemeRemovals(r Response, original runtime.Objec return Errored(http.StatusInternalServerError, err) } - patchOriginal := PatchResponseFromRaw(raw, marshalledOriginal).Patches + patchOriginal, err := jsonpatch.CreatePatch(raw, marshalledOriginal) + if err != nil { + return Errored(http.StatusInternalServerError, err) + } removedByScheme := sets.New(slices.DeleteFunc(patchOriginal, func(p jsonpatch.JsonPatchOperation) bool { return p.Operation != opRemove })...) r.Patches = slices.DeleteFunc(r.Patches, func(p jsonpatch.JsonPatchOperation) bool { diff --git a/pkg/webhook/admission/defaulter_custom_test.go b/pkg/webhook/admission/defaulter_custom_test.go index 4d889cee8a..22bc7d8ae6 100644 --- a/pkg/webhook/admission/defaulter_custom_test.go +++ b/pkg/webhook/admission/defaulter_custom_test.go @@ -29,7 +29,7 @@ import ( var _ = Describe("Defaulter Handler", func() { - It("should not preserve unknown fields by default", func() { + It("should remove unknown fields when DefaulterRemoveUnknownFields is passed", func() { obj := &TestDefaulter{} handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}, DefaulterRemoveUnknownFields) @@ -61,7 +61,7 @@ var _ = Describe("Defaulter Handler", func() { Expect(resp.Result.Code).Should(Equal(int32(http.StatusOK))) }) - It("should preserve unknown fields when DefaulterPreserveUnknownFields is passed", func() { + It("should preserve unknown fields by default", func() { obj := &TestDefaulter{} handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}) From c2ca3a60d06761fe448150c2f97bf697efb5c16a Mon Sep 17 00:00:00 2001 From: Traian Schiau Date: Sun, 27 Oct 2024 10:37:25 +0200 Subject: [PATCH 6/7] Review Remarks --- pkg/builder/webhook.go | 29 +++++++++++++---------- pkg/webhook/admission/defaulter_custom.go | 9 ++++--- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/pkg/builder/webhook.go b/pkg/builder/webhook.go index 543477da2e..c74742d6ea 100644 --- a/pkg/builder/webhook.go +++ b/pkg/builder/webhook.go @@ -37,16 +37,17 @@ import ( // WebhookBuilder builds a Webhook. type WebhookBuilder struct { - apiType runtime.Object - customDefaulter admission.CustomDefaulter - customValidator admission.CustomValidator - customPath string - gvk schema.GroupVersionKind - mgr manager.Manager - config *rest.Config - recoverPanic *bool - logConstructor func(base logr.Logger, req *admission.Request) logr.Logger - err error + apiType runtime.Object + customDefaulter admission.CustomDefaulter + customDefaulterOpts []admission.DefaulterOption + customValidator admission.CustomValidator + customPath string + gvk schema.GroupVersionKind + mgr manager.Manager + config *rest.Config + recoverPanic *bool + logConstructor func(base logr.Logger, req *admission.Request) logr.Logger + err error } // WebhookManagedBy returns a new webhook builder. @@ -67,9 +68,11 @@ func (blder *WebhookBuilder) For(apiType runtime.Object) *WebhookBuilder { return blder } -// WithDefaulter takes an admission.CustomDefaulter interface, a MutatingWebhook will be wired for this type. -func (blder *WebhookBuilder) WithDefaulter(defaulter admission.CustomDefaulter) *WebhookBuilder { +// WithDefaulter takes an admission.CustomDefaulter interface, a MutatingWebhook with the provided opts (admission.DefaulterOption) +// will be wired for this type. +func (blder *WebhookBuilder) WithDefaulter(defaulter admission.CustomDefaulter, opts ...admission.DefaulterOption) *WebhookBuilder { blder.customDefaulter = defaulter + blder.customDefaulterOpts = opts return blder } @@ -194,7 +197,7 @@ func (blder *WebhookBuilder) registerDefaultingWebhook() error { func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook { if defaulter := blder.customDefaulter; defaulter != nil { - w := admission.WithCustomDefaulter(blder.mgr.GetScheme(), blder.apiType, defaulter) + w := admission.WithCustomDefaulter(blder.mgr.GetScheme(), blder.apiType, defaulter, blder.customDefaulterOpts...) if blder.recoverPanic != nil { w = w.WithRecoverPanic(*blder.recoverPanic) } diff --git a/pkg/webhook/admission/defaulter_custom.go b/pkg/webhook/admission/defaulter_custom.go index 40e030823b..cae21c6830 100644 --- a/pkg/webhook/admission/defaulter_custom.go +++ b/pkg/webhook/admission/defaulter_custom.go @@ -40,15 +40,18 @@ type defaulterOptions struct { removeUnknownFields bool } -type defaulterOption func(*defaulterOptions) +// DefaulterOption defines the type of a CustomDefaulter's option +type DefaulterOption func(*defaulterOptions) -// DefaulterRemoveUnknownFields makes the defaulter prune the fields that are not recognized in the local scheme. +// DefaulterRemoveUnknownFields makes the defaulter prune fields that are in the json object retrieved by the +// webhook but not in the local go type. This happens for example when the CRD in the apiserver has fields that +// our go type doesn't know about, because it's outdated. func DefaulterRemoveUnknownFields(o *defaulterOptions) { o.removeUnknownFields = true } // WithCustomDefaulter creates a new Webhook for a CustomDefaulter interface. -func WithCustomDefaulter(scheme *runtime.Scheme, obj runtime.Object, defaulter CustomDefaulter, opts ...defaulterOption) *Webhook { +func WithCustomDefaulter(scheme *runtime.Scheme, obj runtime.Object, defaulter CustomDefaulter, opts ...DefaulterOption) *Webhook { options := &defaulterOptions{} for _, o := range opts { o(options) From fb7b6cd415ce2bc7cf004f4d9246ec2696f88465 Mon Sep 17 00:00:00 2001 From: Traian Schiau Date: Mon, 4 Nov 2024 09:42:01 +0200 Subject: [PATCH 7/7] Rename DefaulterRemoveUnknownFields to DefaulterRemoveUnknownOrOmitableFields --- pkg/webhook/admission/defaulter_custom.go | 31 +++++++++++-------- .../admission/defaulter_custom_test.go | 2 +- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/pkg/webhook/admission/defaulter_custom.go b/pkg/webhook/admission/defaulter_custom.go index cae21c6830..ac9b3ed6d3 100644 --- a/pkg/webhook/admission/defaulter_custom.go +++ b/pkg/webhook/admission/defaulter_custom.go @@ -37,17 +37,17 @@ type CustomDefaulter interface { } type defaulterOptions struct { - removeUnknownFields bool + removeUnknownOrOmitableFields bool } // DefaulterOption defines the type of a CustomDefaulter's option type DefaulterOption func(*defaulterOptions) -// DefaulterRemoveUnknownFields makes the defaulter prune fields that are in the json object retrieved by the -// webhook but not in the local go type. This happens for example when the CRD in the apiserver has fields that -// our go type doesn't know about, because it's outdated. -func DefaulterRemoveUnknownFields(o *defaulterOptions) { - o.removeUnknownFields = true +// DefaulterRemoveUnknownOrOmitableFields makes the defaulter prune fields that are in the json object retrieved by the +// webhook but not in the local go type json representation. This happens for example when the CRD in the apiserver has +// fields that our go type doesn't know about, because it's outdated, or the field has a zero value and is `omitempty`. +func DefaulterRemoveUnknownOrOmitableFields(o *defaulterOptions) { + o.removeUnknownOrOmitableFields = true } // WithCustomDefaulter creates a new Webhook for a CustomDefaulter interface. @@ -57,15 +57,20 @@ func WithCustomDefaulter(scheme *runtime.Scheme, obj runtime.Object, defaulter C o(options) } return &Webhook{ - Handler: &defaulterForType{object: obj, defaulter: defaulter, decoder: NewDecoder(scheme), removeUnknownFields: options.removeUnknownFields}, + Handler: &defaulterForType{ + object: obj, + defaulter: defaulter, + decoder: NewDecoder(scheme), + removeUnknownOrOmitableFields: options.removeUnknownOrOmitableFields, + }, } } type defaulterForType struct { - defaulter CustomDefaulter - object runtime.Object - decoder Decoder - removeUnknownFields bool + defaulter CustomDefaulter + object runtime.Object + decoder Decoder + removeUnknownOrOmitableFields bool } // Handle handles admission requests. @@ -100,7 +105,7 @@ func (h *defaulterForType) Handle(ctx context.Context, req Request) Response { // Keep a copy of the object if needed var originalObj runtime.Object - if !h.removeUnknownFields { + if !h.removeUnknownOrOmitableFields { originalObj = obj.DeepCopyObject() } @@ -120,7 +125,7 @@ func (h *defaulterForType) Handle(ctx context.Context, req Request) Response { } handlerResponse := PatchResponseFromRaw(req.Object.Raw, marshalled) - if !h.removeUnknownFields { + if !h.removeUnknownOrOmitableFields { handlerResponse = h.dropSchemeRemovals(handlerResponse, originalObj, req.Object.Raw) } return handlerResponse diff --git a/pkg/webhook/admission/defaulter_custom_test.go b/pkg/webhook/admission/defaulter_custom_test.go index 22bc7d8ae6..228636b7d6 100644 --- a/pkg/webhook/admission/defaulter_custom_test.go +++ b/pkg/webhook/admission/defaulter_custom_test.go @@ -31,7 +31,7 @@ var _ = Describe("Defaulter Handler", func() { It("should remove unknown fields when DefaulterRemoveUnknownFields is passed", func() { obj := &TestDefaulter{} - handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}, DefaulterRemoveUnknownFields) + handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}, DefaulterRemoveUnknownOrOmitableFields) resp := handler.Handle(context.TODO(), Request{ AdmissionRequest: admissionv1.AdmissionRequest{