Make LossLessDefaulter selective to only prevent dropping fields outside of schema.

[mbobrovskyi]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Make LossLessDefaulter selective to only prevent dropping fields outside of schema.

Which issue(s) this PR fixes:

Fixes #3174

Special notes for your reviewer:

For more information see:

• https://jsonpatch.com/
• https://datatracker.ietf.org/doc/html/rfc6902/
• https://datatracker.ietf.org/doc/html/rfc6901

Does this PR introduce a user-facing change?

NONE

[netlify[bot]]

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	ac7821f3c4fc8e573540b1b4ea7b20b54019bc70
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/67082c12c48c370008faf46c

[mbobrovskyi]

/cc @alculquicondor

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mbobrovskyi Once this PR has been reviewed and has the lgtm label, please assign kerthcet for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• pkg/controller/OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[trasc]

Maybe I'm missing something but an alternative (slightly hacky) can be to overwrite the Raw object before the actual defaulter is called.

Something like:

import (
        "context"
+       "encoding/json"
+       "net/http"
-       jsonpatch "gomodules.xyz/jsonpatch/v2"
        "k8s.io/apimachinery/pkg/runtime"
        "sigs.k8s.io/controller-runtime/pkg/webhook/admission"
)
@@ -29,11 +30,15 @@ import (
func WithLosslessDefaulter(scheme *runtime.Scheme, obj runtime.Object, defaulter admission.CustomDefaulter) admission.Handler {
        return &losslessDefaulter{
                Handler: admission.WithCustomDefaulter(scheme, obj, defaulter).Handler,
+               obj:     obj.DeepCopyObject(),
+               decoder: admission.NewDecoder(scheme),
        }
}
type losslessDefaulter struct {
        admission.Handler
+       obj     runtime.Object
+       decoder admission.Decoder
}
// Handle handles admission requests, **dropping** remove operations from patches produced by controller-runtime.
@@ -43,18 +48,14 @@ type losslessDefaulter struct {
// 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
+       o := h.obj.DeepCopyObject()
+       if err := h.decoder.Decode(req, o); err != nil {
+               return admission.Errored(http.StatusBadRequest, err)
        }
-       return response
+       marshalled, err := json.Marshal(o)
+       if err != nil {
+               return admission.Errored(http.StatusInternalServerError, err)
+       }
+       req.Object.Raw = marshalled
+       return h.Handler.Handle(ctx, req)
}

[mimowo]

Maybe I'm missing something but an alternative (slightly hacky) can be to overwrite the Raw object before the actual defaulter is called.

I'm not sure about that, maybe @alculquicondor can you think of some pros & cons of the alternative?

[trasc]

Maybe I'm missing something but an alternative (slightly hacky) can be to overwrite the Raw object before the actual defaulter is called.

I'm not sure about that, maybe @alculquicondor can you think of some pros & cons of the alternative?

I did find some discussions about that an the probability to generate invalid patches.

[mbobrovskyi]

Maybe fieldExistsByJSONPath(object interface{}, path string) and friends can be moved in pkg/util/api.

We already discussed it here https://github.com/kubernetes-sigs/kueue/pull/3186#discussion_r1787524717 and take a decision to keep it on the same package.

[mbobrovskyi]

/close

Due to found better solution https://github.com/kubernetes-sigs/kueue/pull/3194.

[k8s-ci-robot]

@mbobrovskyi: Closed this PR.

In response to this:

/close

Due to found better solution.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[mbobrovskyi]

/reopen

Due to https://github.com/kubernetes-sigs/kueue/pull/3194#discussion_r1795708267.

[k8s-ci-robot]

@mbobrovskyi: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[mbobrovskyi]

/close

Due to very hard to handle inline object and Unmarshal methods.

[k8s-ci-robot]

@mbobrovskyi: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.