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]

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

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

[mbobrovskyi]

/cc @alculquicondor

[mimowo]

I think this extension is quite complex and we need much better test coverage, optimally 100%, including cases like removing from a slice (for example to drop a condition).

It is essential piece of code for correctness of webhooks so as a bailout mechanism, in case of an issue, I would like to consider a construction parameter to control this selective behavior. Such as "AllowRemovingSchemaFields".

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

[trasc]

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

[trasc]

nit: maybe add a comment explaining why < 2

[trasc]

I think we should also check for maps

[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 #3186 (comment) and take a decision to keep it on the same package.

[mbobrovskyi]

/close

Due to found better solution #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 #3194 (comment).

[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.