Make LossLessDefaulter selective to only prevent dropping fields outside of schema (unmarshal strict solution).

[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 (unmarshal strict solution).

Which issue(s) this PR fixes:

Fixes #3174

Special notes for your reviewer:

The reflection-based solution for prevent dropping fields outside of schema is quite complex. This is an alternative solution for #3186 using json.UnmarshalStrict(). Thanks to @trasc for the idea.

Does this PR introduce a user-facing change?

NONE

[netlify]

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	06c36b7
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/6705516c24850d0008ff9db1

[mbobrovskyi]

/cc @trasc

[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 tenzen-y 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:

• OWNERS

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

[mimowo]

Please add a test case for removing label by key example.com/foo.

[mbobrovskyi]

Yeah, it's a good case. Thanks!

[mbobrovskyi]

I also thinking about [][]string case. I will add this case as well.

[mimowo]

/hold
Thanks for bringing this up, let's hold for fixing this case

[mbobrovskyi]

I also thinking about [][]string case. I will add this case as well.

OK. On this case we can have two cases /qux/0/1 => qux[0][1] and /quux/0/0/unknown => quux[0][0].unknown.

The problem here is that we don't know exactly is it fieldName 0 or index of array 😕.

[alculquicondor]

Change Path to Stack and generate Path on demand in the function.

Since the struct is private, it sounds like the change would be backwards-compatible.
I guess there could potentially be performance regressions if a user calls FieldPath() repeatedly on the same object.

[mbobrovskyi]

At least escape dots in the field name. However, I doubt that it will meet all our requirements.

So... We only have one way?

[alculquicondor]

No, I think we can go the way of Change Path to Stack and generate Path on demand in the function, if @liggitt agrees to it.

[liggitt]

to do that we have to clone / alloc the stack for every error we return - that's what I wasn't in favor of in #3194 (comment)

[alculquicondor]

Oh, I see what you mean. Ok, then let's proceed the way of #3186

[mimowo]

LGTM, but we need more eyes on this.
cc @alculquicondor @tenzen-y @trasc

[alculquicondor]

Uhm... I wonder if there are other escape characters we should be aware of. Can you check the jsonpath documentation?

Or is there a different library or setting for strict json verification that we can use?

[mimowo]

I'm still hesitant (no strong opinion at this point) that this path-mapping approach is simpler than #3186.

It is certainly simpler on the surface, but the path mapping code makes me feel uneasy about productising it. First, I have a feeling we are building on top of API which is meant to surface errors for human readers, not to build functionality around it. Second, the mapping is done with regexes which are intrinsically meant for unstructured data. Finally, there are many nuances about syntax and escaping. I suppose the code to support all cases properly, like [][]string will be even more complex. And I'm not even sure how to methodically approach verifying correctness.

I consider the previous structured-traversal approach straighforward conceptually. Yes, there are many cases to take care about corresponding to different field types (slice, Map, Pointer, Struct, primitives), but there is only a finite number of those, and we can get help from code analysis tools to see what are the uncovered types.

Let me know your thoughts.

[alculquicondor]

I consider the previous structured-traversal approach straighforward conceptually. Yes, there are many cases to take care about corresponding to different field types (slice, Map, Pointer, Struct, primitives),

I would add the inline structs.

But yes, it seems like a more limited set. It overall sounds more reliable than the parsing via regex. Right now, the code in #3186 doesn't actually look that long.

[trasc]

In my opinion, in a longer run we can try to modify the json error to be able to return a reliable jsonpatch path and we'll be able to get rid of the string manipulation we are doing now.

In a even longer run maybe we can get controller-runtime/pkg/webhook/admission/defaulter_custom.go to use a strict decoder and do the patch sanitation in there.

[trasc]

Yet another possible approach: main...epam:kubernetes-kueue:dont-drop-on-muatation

In short, we crate a patch that shows what is removed just by an Unmarshal/Marshal cycle with the local scheme, and we drop the paths in question from the final patch.

[mbobrovskyi]

Yet another possible approach: main...epam:kubernetes-kueue:dont-drop-on-muatation

In short, we crate a patch that shows what is removed just by an Unmarshal/Marshal cycle with the local scheme, and we drop the paths in question from the final patch.

I like this idea. Could you please create a separate PR to continue the discussion?

[mbobrovskyi]

Yet another possible approach: main...epam:kubernetes-kueue:dont-drop-on-muatation
In short, we crate a patch that shows what is removed just by an Unmarshal/Marshal cycle with the local scheme, and we drop the paths in question from the final patch.

I like this idea. Could you please create a separate PR to continue the discussion?

It would also be great to add more test cases from the current PR and check how it works with them. But I think it should work fine.

[alculquicondor]

Let me try to compare the options:

@trasc's

• Con: It adds three passes to the structure (to unmarshal, then marshal, then diff)
• Pro: It delegates any nuances of json marshaling to well tested libraries.
• Pro: It handles structs that implement MarshalJSON, which is not uncommon. We have resource.Quantity upstream.

@mbobrovskyi's

• Pro: It adds one pass to the structure
• Con: More maintenance on our side
• Con: It can't handle MarshalJSON

So maybe @trasc's is the only viable option? Maybe we can optimize a little if we can reuse the unmarshalled object from CustomDefaulter, once the change goes to controller-runtime.

[mimowo]

I think the extra in-memory pass is only a minimal con. The whole object is anyway serialized / deserialized multiple times during webhook admission and transferred over the wire between kube-apiserver and webhooks.

[mimowo]

Con: More maintenance on our side

That is some con, but ideally we can transfer the solution to controller-runtime in the long run. In order to make the transfer possible I think we need to make sure it is correct in all cases.

So, for now I'm mostly focused on correctness, and running all tests we have against main...epam:kubernetes-kueue:dont-drop-on-muatation would be great to build confidence in this approach.

Regarding MarshalJSON - I'm not familiar with this in the context of webhooks and wondering how this is working. IIUC kube-apiserver needs to serialize the object to send it over the wire to webhooks. I would suppose it doesn't use custom MarshalJSON because it would risk running arbitrary code on kube-apiserver. So, if the webhook uses it to deserialize it, we can create a mismatch in the serialize / deserialize functions. Maybe it warrants an experiment how/if this is supported, WDYT @alculquicondor ?

[alculquicondor]

This is how a Quantity file is described in the CRD:

kueue/config/components/crd/bases/kueue.x-k8s.io_clusterqueues.yaml

Lines 382 to 397 in 391a000

	borrowingLimit:
	anyOf:
	- type: integer
	- type: string
	description: |-
	borrowingLimit is the maximum amount of quota for the [flavor, resource]
	combination that this ClusterQueue is allowed to borrow from the unused
	quota of other ClusterQueues in the same cohort.
	In total, at a given time, Workloads in a ClusterQueue can consume a
	quantity of quota equal to nominalQuota+borrowingLimit, assuming the other
	ClusterQueues in the cohort have enough unused quota.
	If null, it means that there is no borrowing limit.
	If not null, it must be non-negative.
	borrowingLimit must be null if spec.cohort is empty.
	pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
	x-kubernetes-int-or-string: true

I think, overall, we can say that the values that use MarshalJSON will be pretty much stored as strings in etcd.
To clarify, apiserver doesn't run marshal/unmarshal code. It is the client (like kueue itself) that runs that code, taken from the imported types.

Maybe LossLessDefaulter will need to just treat the structs that define MarshalJSON as leaves in the structure? Not sure if that's generic enough.

[trasc]

I think that a field with custom Marshal/Unmarshal can also declare itself as object with // +kubebuilder:validation:Type=object.

[mbobrovskyi]

/close

It's very hard to compare UnmarshalStrict and json patch paths.

Alternative solution #3315.

[k8s-ci-robot]

@mbobrovskyi: Closed this PR.

In response to this:

/close

It's very hard to compare UnmarshalStrict and json patch paths.

Alternative solution #3315.

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.