🐛 Fix custom defaulter: avoid deleting unknown fields (zero change patch) (#2982)

* Fix custom defaulter from deleting unknown fields

* Use a zero-change patch to do find scheme caused fields removal.

* Add `DefaulterPreserveUnknownFields`

An option stops the defaulter from pruning the fields
that are not recognized in the local scheme.

* Make it opt-out, `DefaulterRemoveUnknownFields`

* Review Remarks

* Review Remarks

* Rename DefaulterRemoveUnknownFields to DefaulterRemoveUnknownOrOmitableFields

---------

Co-authored-by: Aldo Culquicondor <acondor@google.com>

Author: trasc

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)
 		}

pkg/webhook/admission/defaulter_custom.go

@@ -21,29 +21,56 @@ 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.
 type CustomDefaulter interface {
 	Default(ctx context.Context, obj runtime.Object) error
 }
 
+type defaulterOptions struct {
+	removeUnknownOrOmitableFields bool
+}
+
+// DefaulterOption defines the type of a CustomDefaulter's option
+type DefaulterOption func(*defaulterOptions)
+
+// 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.
-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),
+			removeUnknownOrOmitableFields: options.removeUnknownOrOmitableFields,
+		},
 	}
 }
 
 type defaulterForType struct {
-	defaulter CustomDefaulter
-	object    runtime.Object
-	decoder   Decoder
+	defaulter                     CustomDefaulter
+	object                        runtime.Object
+	decoder                       Decoder
+	removeUnknownOrOmitableFields bool
 }
 
 // Handle handles admission requests.
@@ -76,6 +103,12 @@ func (h *defaulterForType) Handle(ctx context.Context, req Request) Response {
 		return Errored(http.StatusBadRequest, err)
 	}
 
+	// Keep a copy of the object if needed
+	var originalObj runtime.Object
+	if !h.removeUnknownOrOmitableFields {
+		originalObj = obj.DeepCopyObject()
+	}
+
 	// Default the object
 	if err := h.defaulter.Default(ctx, obj); err != nil {
 		var apiStatus apierrors.APIStatus
@@ -90,5 +123,43 @@ func (h *defaulterForType) Handle(ctx context.Context, req Request) Response {
 	if err != nil {
 		return Errored(http.StatusInternalServerError, err)
 	}
-	return PatchResponseFromRaw(req.Object.Raw, marshalled)
+
+	handlerResponse := PatchResponseFromRaw(req.Object.Raw, marshalled)
+	if !h.removeUnknownOrOmitableFields {
+		handlerResponse = h.dropSchemeRemovals(handlerResponse, originalObj, req.Object.Raw)
+	}
+	return handlerResponse
+}
+
+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)
+	}
+
+	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 {
+		return removedByScheme.Has(p)
+	})
+
+	if len(r.Patches) == 0 {
+		r.PatchType = nil
+	}
+	return r
 }

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,66 @@ import (
 
 var _ = Describe("Defaulter Handler", func() {
 
+	It("should remove unknown fields when DefaulterRemoveUnknownFields is passed", func() {
+		obj := &TestDefaulter{}
+		handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{}, DefaulterRemoveUnknownOrOmitableFields)
+
+		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(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 by default", 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", "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)))
+	})
+
 	It("should return ok if received delete verb in defaulter handler", func() {
 		obj := &TestDefaulter{}
 		handler := WithCustomDefaulter(admissionScheme, obj, &TestCustomDefaulter{})
@@ -48,15 +109,17 @@ 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"}
 
 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,
 	}
 }
 
@@ -81,5 +144,6 @@ func (d *TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) e
 	if o.Replica < 2 {
 		o.Replica = 2
 	}
+	o.TotalReplicas = 0
 	return nil
 }