Repository: kubernetes-sigs/kueue

Issue:
Remove our custom defaulter when controller-runtime fix is released
**What would you like to be cleaned**:

We have a custom defaulter in Kueue registered via `pkg/controller/jobframework/webhook/builder.go ` to workaround a bug in controller-runtime which was dropping unknown fields in webhooks.

For that we need to await until the fix https://github.com/kubernetes-sigs/controller-runtime/pull/2982 is released in controller-runtime, probably 0.20.

**Why is this needed**:

To cleanup Kueue code, and use a fix which allows to also remove fields in Kueue's webhooks (if we need it).

Discussion:
@mimowo: cc @tenzen-y @mbobrovskyi @k8s-triage-robot: The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:
- After 90d of inactivity, `lifecycle/stale` is applied
- After 30d of inactivity since `lifecycle/stale` was applied, `lifecycle/rotten` is applied
- After 30d of inactivity since `lifecycle/rotten` was applied, the issue is closed

You can:
- Mark this issue as fresh with `/remove-lifecycle stale`
- Close this issue with `/close`
- Offer to help out with [Issue Triage][1]

Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community).

/lifecycle stale

[1]: https://www.kubernetes.dev/docs/guide/issue-triage/@tenzen-y: /remove-lifecycle stale@mbobrovskyi: /assign

Solution patch:
diff --git a/go.mod b/go.mod
index 624e0597d..705805df9 100644
--- a/go.mod
+++ b/go.mod
@@ -20,7 +20,6 @@ require (
 	github.com/spf13/cobra v1.9.1
 	github.com/spf13/pflag v1.0.6
 	go.uber.org/zap v1.27.0
-	gomodules.xyz/jsonpatch/v2 v2.5.0
 	k8s.io/api v0.32.3
 	k8s.io/apimachinery v0.32.3
 	k8s.io/apiserver v0.32.3
@@ -132,6 +131,7 @@ require (
 	golang.org/x/text v0.23.0 // indirect
 	golang.org/x/time v0.11.0 // indirect
 	golang.org/x/tools v0.31.0 // indirect
+	gomodules.xyz/jsonpatch/v2 v2.5.0 // indirect
 	google.golang.org/genproto v0.0.0-20240528184218-531527333157 // indirect
 	google.golang.org/genproto/googleapis/api v0.0.0-20241219192143-6b3ec007d9bb // indirect
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20241219192143-6b3ec007d9bb // indirect
diff --git a/pkg/controller/jobframework/base_webhook.go b/pkg/controller/jobframework/base_webhook.go
index f3628d09e..25feb937b 100644
--- a/pkg/controller/jobframework/base_webhook.go
+++ b/pkg/controller/jobframework/base_webhook.go
@@ -53,7 +53,7 @@ func BaseWebhookFactory(job GenericJob, fromObject func(runtime.Object) GenericJ
 		}
 		return webhook.WebhookManagedBy(mgr).
 			For(job.Object()).
-			WithMutationHandler(webhook.WithLosslessDefaulter(mgr.GetScheme(), job.Object(), wh)).
+			WithMutationHandler(admission.WithCustomDefaulter(mgr.GetScheme(), job.Object(), wh)).
 			WithValidator(wh).
 			Complete()
 	}
diff --git a/pkg/controller/jobframework/webhook/defaulter.go b/pkg/controller/jobframework/webhook/defaulter.go
deleted file mode 100644
index f51019de4..000000000
--- a/pkg/controller/jobframework/webhook/defaulter.go
+++ /dev/null
@@ -1,60 +0,0 @@
-/*
-Copyright 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 webhook
-
-import (
-	"context"
-
-	jsonpatch "gomodules.xyz/jsonpatch/v2"
-	"k8s.io/apimachinery/pkg/runtime"
-	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
-)
-
-// 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,
-	}
-}
-
-type losslessDefaulter struct {
-	admission.Handler
-}
-
-// Handle handles admission requests, **dropping** remove operations from patches produced by controller-runtime.
-// The controller-runtime handler works by creating a jsondiff from the raw object and the marshalled
-// version of the object modified by the defaulter. This generates "remove" operations for fields
-// that are not present in the go types, which can occur when Kueue libraries are behind the latest
-// released CRDs.
-// Dropping the "remove" operations is safe because Kueue's job mutators never remove fields.
-func (h *losslessDefaulter) Handle(ctx context.Context, req admission.Request) admission.Response {
-	response := h.Handler.Handle(ctx, req)
-	if response.Allowed {
-		var patches []jsonpatch.Operation
-		for _, p := range response.Patches {
-			if p.Operation != "remove" {
-				patches = append(patches, p)
-			}
-		}
-		if len(patches) == 0 {
-			response.PatchType = nil
-		}
-		response.Patches = patches
-	}
-	return response
-}
diff --git a/pkg/controller/jobs/deployment/deployment_webhook.go b/pkg/controller/jobs/deployment/deployment_webhook.go
index 20e346375..c8d265fd8 100644
--- a/pkg/controller/jobs/deployment/deployment_webhook.go
+++ b/pkg/controller/jobs/deployment/deployment_webhook.go
@@ -53,7 +53,7 @@ func SetupWebhook(mgr ctrl.Manager, opts ...jobframework.Option) error {
 	obj := &appsv1.Deployment{}
 	return webhook.WebhookManagedBy(mgr).
 		For(obj).
-		WithMutationHandler(webhook.WithLosslessDefaulter(mgr.GetScheme(), obj, wh)).
+		WithMutationHandler(admission.WithCustomDefaulter(mgr.GetScheme(), obj, wh)).
 		WithValidator(wh).
 		Complete()
 }
diff --git a/pkg/controller/jobs/job/job_webhook.go b/pkg/controller/jobs/job/job_webhook.go
index 2f90f8934..49b1f52bb 100644
--- a/pkg/controller/jobs/job/job_webhook.go
+++ b/pkg/controller/jobs/job/job_webhook.go
@@ -64,7 +64,7 @@ func SetupWebhook(mgr ctrl.Manager, opts ...jobframework.Option) error {
 	obj := &batchv1.Job{}
 	return webhook.WebhookManagedBy(mgr).
 		For(obj).
-		WithMutationHandler(webhook.WithLosslessDefaulter(mgr.GetScheme(), obj, wh)).
+		WithMutationHandler(admission.WithCustomDefaulter(mgr.GetScheme(), obj, wh)).
 		WithValidator(wh).
 		Complete()
 }
diff --git a/pkg/controller/jobs/jobset/jobset_webhook.go b/pkg/controller/jobs/jobset/jobset_webhook.go
index 72a7e64c7..5966496f5 100644
--- a/pkg/controller/jobs/jobset/jobset_webhook.go
+++ b/pkg/controller/jobs/jobset/jobset_webhook.go
@@ -58,7 +58,7 @@ func SetupJobSetWebhook(mgr ctrl.Manager, opts ...jobframework.Option) error {
 	obj := &jobsetapi.JobSet{}
 	return webhook.WebhookManagedBy(mgr).
 		For(obj).
-		WithMutationHandler(webhook.WithLosslessDefaulter(mgr.GetScheme(), obj, wh)).
+		WithMutationHandler(admission.WithCustomDefaulter(mgr.GetScheme(), obj, wh)).
 		WithValidator(wh).
 		Complete()
 }
diff --git a/pkg/controller/jobs/mpijob/mpijob_webhook.go b/pkg/controller/jobs/mpijob/mpijob_webhook.go
index a41f46d24..b5686e514 100644
--- a/pkg/controller/jobs/mpijob/mpijob_webhook.go
+++ b/pkg/controller/jobs/mpijob/mpijob_webhook.go
@@ -62,7 +62,7 @@ func SetupMPIJobWebhook(mgr ctrl.Manager, opts ...jobframework.Option) error {
 	obj := &v2beta1.MPIJob{}
 	return webhook.WebhookManagedBy(mgr).
 		For(obj).
-		WithMutationHandler(webhook.WithLosslessDefaulter(mgr.GetScheme(), obj, wh)).
+		WithMutationHandler(admission.WithCustomDefaulter(mgr.GetScheme(), obj, wh)).
 		WithValidator(wh).
 		Complete()
 }
diff --git a/pkg/controller/jobs/pod/pod_webhook.go b/pkg/controller/jobs/pod/pod_webhook.go
index a3c0513a5..fd2150078 100644
--- a/pkg/controller/jobs/pod/pod_webhook.go
+++ b/pkg/controller/jobs/pod/pod_webhook.go
@@ -87,7 +87,7 @@ func SetupWebhook(mgr ctrl.Manager, opts ...jobframework.Option) error {
 	obj := &corev1.Pod{}
 	return webhook.WebhookManagedBy(mgr).
 		For(obj).
-		WithMutationHandler(webhook.WithLosslessDefaulter(mgr.GetScheme(), obj, wh)).
+		WithMutationHandler(admission.WithCustomDefaulter(mgr.GetScheme(), obj, wh)).
 		WithValidator(wh).
 		Complete()
 }
diff --git a/pkg/controller/jobs/raycluster/raycluster_webhook.go b/pkg/controller/jobs/raycluster/raycluster_webhook.go
index 7df0b0b9a..980a0f745 100644
--- a/pkg/controller/jobs/raycluster/raycluster_webhook.go
+++ b/pkg/controller/jobs/raycluster/raycluster_webhook.go
@@ -65,7 +65,7 @@ func SetupRayClusterWebhook(mgr ctrl.Manager, opts ...jobframework.Option) error
 	obj := &rayv1.RayCluster{}
 	return webhook.WebhookManagedBy(mgr).
 		For(obj).
-		WithMutationHandler(webhook.WithLosslessDefaulter(mgr.GetScheme(), obj, wh)).
+		WithMutationHandler(admission.WithCustomDefaulter(mgr.GetScheme(), obj, wh)).
 		WithValidator(wh).
 		Complete()
 }
diff --git a/pkg/controller/jobs/rayjob/rayjob_webhook.go b/pkg/controller/jobs/rayjob/rayjob_webhook.go
index eeb11107a..82fc55b48 100644
--- a/pkg/controller/jobs/rayjob/rayjob_webhook.go
+++ b/pkg/controller/jobs/rayjob/rayjob_webhook.go
@@ -63,7 +63,7 @@ func SetupRayJobWebhook(mgr ctrl.Manager, opts ...jobframework.Option) error {
 	obj := &rayv1.RayJob{}
 	return webhook.WebhookManagedBy(mgr).
 		For(obj).
-		WithMutationHandler(webhook.WithLosslessDefaulter(mgr.GetScheme(), obj, wh)).
+		WithMutationHandler(admission.WithCustomDefaulter(mgr.GetScheme(), obj, wh)).
 		WithValidator(wh).
 		Complete()
 }



Test patch:
diff --git a/pkg/controller/jobframework/webhook/defaulter_test.go b/pkg/controller/jobframework/webhook/defaulter_test.go
deleted file mode 100644
index 06db51f37..000000000
--- a/pkg/controller/jobframework/webhook/defaulter_test.go
+++ /dev/null
@@ -1,99 +0,0 @@
-/*
-Copyright 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 webhook
-
-import (
-	"context"
-	"testing"
-
-	"github.com/google/go-cmp/cmp"
-	jsonpatch "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"
-	"sigs.k8s.io/controller-runtime/pkg/scheme"
-	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
-)
-
-var (
-	testResourceKind = "TestResource"
-	testResourceGVK  = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: testResourceKind}
-)
-
-type TestResource struct {
-	Foo string `json:"foo,omitempty"`
-}
-
-func (d *TestResource) GetObjectKind() schema.ObjectKind { return d }
-func (d *TestResource) DeepCopyObject() runtime.Object {
-	return &TestResource{
-		Foo: d.Foo,
-	}
-}
-
-func (d *TestResource) GroupVersionKind() schema.GroupVersionKind {
-	return testResourceGVK
-}
-
-func (d *TestResource) SetGroupVersionKind(gvk schema.GroupVersionKind) {}
-
-type TestCustomDefaulter struct{}
-
-func (*TestCustomDefaulter) Default(ctx context.Context, obj runtime.Object) error {
-	d := obj.(*TestResource)
-	if d.Foo == "" {
-		d.Foo = "bar"
-	}
-	return nil
-}
-
-func TestLossLessDefaulter(t *testing.T) {
-	sch := runtime.NewScheme()
-	builder := scheme.Builder{GroupVersion: testResourceGVK.GroupVersion()}
-	builder.Register(&TestResource{})
-	if err := builder.AddToScheme(sch); err != nil {
-		t.Fatalf("Couldn't add types to scheme: %v", err)
-	}
-
-	handler := WithLosslessDefaulter(sch, &TestResource{}, &TestCustomDefaulter{})
-
-	req := admission.Request{
-		AdmissionRequest: admissionv1.AdmissionRequest{
-			Kind: metav1.GroupVersionKind(testResourceGVK),
-			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"}`),
-			},
-		},
-	}
-	resp := handler.Handle(t.Context(), req)
-	if !resp.Allowed {
-		t.Errorf("Response not allowed")
-	}
-	wantPatches := []jsonpatch.Operation{
-		{
-			Operation: "add",
-			Path:      "/foo",
-			Value:     "bar",
-		},
-	}
-	if diff := cmp.Diff(wantPatches, resp.Patches); diff != "" {
-		t.Errorf("Unexpected patches (-want, +got): %s", diff)
-	}
-}