Make LossLessDefaulter selective to only prevent dropping fields outs…

…ide of schema.

go.mod

@@ -33,6 +33,7 @@ require (
 	k8s.io/utils v0.0.0-20240711033017-18e509b52bc8
 	sigs.k8s.io/controller-runtime v0.19.0
 	sigs.k8s.io/jobset v0.6.0
+	sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd
 	sigs.k8s.io/structured-merge-diff/v4 v4.4.1
 	sigs.k8s.io/yaml v1.4.0
 )
@@ -144,7 +145,6 @@ require (
 	k8s.io/gengo/v2 v2.0.0-20240812201722-3b05ca7b6e59 // indirect
 	k8s.io/kms v0.31.1 // indirect
 	sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.30.3 // indirect
-	sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
 	sigs.k8s.io/kustomize/api v0.17.3 // indirect
 	sigs.k8s.io/kustomize/kyaml v0.17.2 // indirect
 )

pkg/controller/jobframework/webhook/defaulter.go

@@ -18,22 +18,28 @@ package webhook
 
 import (
 	"context"
+	"fmt"
+	"net/http"
+	"strings"
 
-	jsonpatch "gomodules.xyz/jsonpatch/v2"
+	"gomodules.xyz/jsonpatch/v2"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+	"sigs.k8s.io/json"
 )
 
 // WithLosslessDefaulter creates a new Handler for a CustomDefaulter interface that **drops** remove operations,
 // which are typically the result of new API fields not present in Kueue libraries.
 func WithLosslessDefaulter(scheme *runtime.Scheme, obj runtime.Object, defaulter admission.CustomDefaulter) admission.Handler {
 	return &losslessDefaulter{
 		Handler: admission.WithCustomDefaulter(scheme, obj, defaulter).Handler,
+		object:  obj,
 	}
 }
 
 type losslessDefaulter struct {
 	admission.Handler
+	object runtime.Object
 }
 
 // Handle handles admission requests, **dropping** remove operations from patches produced by controller-runtime.
@@ -45,9 +51,15 @@ type losslessDefaulter struct {
 func (h *losslessDefaulter) Handle(ctx context.Context, req admission.Request) admission.Response {
 	response := h.Handler.Handle(ctx, req)
 	if response.Allowed {
+		fieldErrors, err := h.fieldErrors(req.Object.Raw)
+		if err != nil {
+			return admission.Errored(http.StatusInternalServerError, err)
+		}
+
 		var patches []jsonpatch.Operation
 		for _, p := range response.Patches {
-			if p.Operation != "remove" {
+			fmt.Println(p.Json())
+			if p.Operation != "remove" || !checkIsUnknownField(p, fieldErrors) {
 				patches = append(patches, p)
 			}
 		}
@@ -58,3 +70,27 @@ func (h *losslessDefaulter) Handle(ctx context.Context, req admission.Request) a
 	}
 	return response
 }
+
+func (h *losslessDefaulter) fieldErrors(raw []byte) ([]json.FieldError, error) {
+	obj := h.object.DeepCopyObject()
+	strictErrors, err := json.UnmarshalStrict(raw, obj, json.DisallowUnknownFields)
+	if err != nil {
+		return nil, err
+	}
+	fieldErrors := make([]json.FieldError, len(strictErrors))
+	for i, err := range strictErrors {
+		fieldErrors[i] = err.(json.FieldError)
+	}
+	return fieldErrors, nil
+}
+
+func checkIsUnknownField(patchOperation jsonpatch.JsonPatchOperation, fieldErrors []json.FieldError) bool {
+	for _, fieldError := range fieldErrors {
+		fieldPath := fieldError.FieldPath()
+		fieldPath = strings.ReplaceAll(fieldPath, ".", "/")
+		if patchOperation.Path == fmt.Sprintf("/%s", fieldPath) {
+			return true
+		}
+	}
+	return false
+}

pkg/controller/jobframework/webhook/defaulter_test.go

@@ -21,11 +21,13 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
-	jsonpatch "gomodules.xyz/jsonpatch/v2"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"gomodules.xyz/jsonpatch/v2"
 	admissionv1 "k8s.io/api/admission/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/scheme"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
@@ -36,7 +38,18 @@ var (
 )
 
 type TestResource struct {
-	Foo string `json:"foo,omitempty"`
+	Foo  string   `json:"foo,omitempty"`
+	Bar  string   `json:"bar,omitempty"`
+	Qux  []string `json:"qux,omitempty"`
+	Quux []string `json:"quux,omitempty"`
+
+	SubResource TestSubResource    `json:"subresource"`
+	Conditions  []metav1.Condition `json:"conditions,omitempty"`
+}
+
+type TestSubResource struct {
+	Foo string  `json:"foo"`
+	Bar *string `json:"bar"`
 }
 
 func (d *TestResource) GetObjectKind() schema.ObjectKind { return d }
@@ -57,7 +70,38 @@ type TestCustomDefaulter struct{}
 func (*TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error {
 	d := obj.(*TestResource)
 	if d.Foo == "" {
-		d.Foo = "bar"
+		d.Foo = "foo"
+	}
+	if d.Bar == "bar" {
+		d.Bar = ""
+	}
+	if d.SubResource.Foo == "foo" {
+		d.SubResource.Foo = ""
+	}
+	if ptr.Deref(d.SubResource.Bar, "") == "bar" {
+		d.SubResource.Bar = nil
+	}
+	if len(d.Qux) > 0 {
+		d.Qux = nil
+	}
+	if len(d.Quux) > 0 {
+		quux := make([]string, 0, len(d.Quux))
+		for _, val := range d.Quux {
+			if val != "foo" {
+				quux = append(quux, val)
+			}
+		}
+		d.Quux = quux
+	}
+	if len(d.Conditions) > 0 {
+		conditions := make([]metav1.Condition, 0, len(d.Conditions))
+		for _, cond := range d.Conditions {
+			if cond.Type == "foo" {
+				cond.ObservedGeneration = 0
+				conditions = append(conditions, cond)
+			}
+		}
+		d.Conditions = conditions
 	}
 	return nil
 }
@@ -78,7 +122,18 @@ func TestLossLessDefaulter(t *testing.T) {
 			Object: runtime.RawExtension{
 				// This raw object has a field not defined in the go type.
 				// controller-runtime CustomDefaulter would have added a remove operation for it.
-				Raw: []byte(`{"baz": "qux"}`),
+				Raw: []byte(`{
+	"hoge": "hoge",
+	"fuga": ["hoge"],
+	"bar": "bar", 
+	"qux": ["foo"],
+	"quux": ["foo","bar"],
+	"subresource": {"hoge": "hoge", "fuga": ["hoge"], "foo": "foo", "bar": "bar"},
+	"conditions": [
+		{"type": "foo", "message": "foo", "reason": "", "status": "", "lastTransitionTime": null, "observedGeneration": 1}, 
+		{"type": "bar", "message": "bar", "reason": "", "status": "", "lastTransitionTime": null, "observedGeneration": 1}
+	]
+}`),
 			},
 		},
 	}
@@ -87,13 +142,19 @@ func TestLossLessDefaulter(t *testing.T) {
 		t.Errorf("Response not allowed")
 	}
 	wantPatches := []jsonpatch.Operation{
-		{
-			Operation: "add",
-			Path:      "/foo",
-			Value:     "bar",
-		},
+		{Operation: "add", Path: "/foo", Value: "foo"},
+		{Operation: "remove", Path: "/bar"},
+		{Operation: "remove", Path: "/qux"},
+		{Operation: "replace", Path: "/quux/0", Value: "bar"},
+		{Operation: "remove", Path: "/quux/1"},
+		{Operation: "replace", Path: "/subresource/foo", Value: ""},
+		{Operation: "replace", Path: "/subresource/bar"},
+		{Operation: "remove", Path: "/conditions/0/observedGeneration"},
+		{Operation: "remove", Path: "/conditions/1"},
 	}
-	if diff := cmp.Diff(wantPatches, resp.Patches); diff != "" {
+	if diff := cmp.Diff(wantPatches, resp.Patches, cmpopts.SortSlices(func(a, b jsonpatch.Operation) bool {
+		return a.Path < b.Path
+	})); diff != "" {
 		t.Errorf("Unexpected patches (-want, +got): %s", diff)
 	}
 }