🐛 Fix custom defaulter: avoid deleting unknown fields (zero change patch)

[trasc]

When creating a patch for the custom defaulter, don't return the removals generated by object decoding with the local scheme.

Otherwise, the patch will include a remove instruction for fields that the go type doesn't contain.

This could happen when building a webhook for a resource that belongs to another project (including k8s types).

This PR stops the defaulter from pruning the fields that are not recognized in the local scheme.
When doing this we are creating two patches one before and one after the custom defaulter call and only send back the remove operations that are not present in both patches.

The user can opt-out from this new behavior by specifying the DefaulterRemoveUnknownFields option when calling WithCustomDefaulter.

(This is a rework of #2931)

[trasc]

/cc @alculquicondor
/cc @sbueringer
/cc @alvaroaleman

[alculquicondor]

For context to c-r maintainers, we have been discussing what alternatives we have for a generic defaulter that doesn't drop unknown fields. Here is the thread for additional context kubernetes-sigs/kueue#3194 (comment)

Taking @mimowo's words, we would be happy to first merge a solution in kueue and let it mature there before moving it here. But we would appreciate your input to the approaches.

[alvaroaleman]

@alculquicondor @trasc @mimowo We (Stefan, Vince and I) had some discussion around #2932 vs this and we landed at this here being better but we aren't 100% sure if its compatible in all cases and currently don't have time to spend on that question, which is why we would like an opt-out for what is suggested here (but on by default).

Does that sound reasonable to you all?

[trasc]

@alculquicondor @trasc @mimowo We (Stefan, Vince and I) had some discussion around #2932 vs this and we landed at this here being better but we aren't 100% sure if its compatible in all cases and currently don't have time to spend on that question, which is why we would like an opt-out for what is suggested here (but on by default).

Does that sound reasonable to you all?

Sure, I'll ping you when I have something in that direction.

[alculquicondor]

Thanks for considering the options!

Do you have any particular guideline on how the opt-out mechanism could look like? Should we have a different method in the factory? Or should the method WithDefaulter accept some options? Any other suggestion?

Other than that, I hope the overhead of the approach is not significant. Fortunately, a new encoding is coming to k/k CRDs https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/4222-cbor-serializer#motivation

[trasc]

I think

... should the method WithDefaulter accept some options ...

it leave room to maybe add an additional option that removes the add zero values.

[trasc]

@alvaroaleman please have a look at my DefaulterPreserveUnknownFields proposal when you have some time.

Also please let me know if we can do something about the pull-controller-runtime-apidiff check,

changed from 
func(*k8s.io/apimachinery/pkg/runtime.Scheme, k8s.io/apimachinery/pkg/runtime.Object, CustomDefaulter) *Webhook to 
func(*k8s.io/apimachinery/pkg/runtime.Scheme, k8s.io/apimachinery/pkg/runtime.Object, CustomDefaulter, ...defaulterOption) *Webhook

is a flase positive in my opinion, as you don't need to actually do something to old code in order to use the new version.

[alculquicondor]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: dc8f633f86d5467de1824324a9004cf5c10d3d61

[alvaroaleman]

lgtm, thanks for this
/hold
For @sbueringer and @vincepri

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: ad6c906a29abd3648f1ceeefac007955a4d3b86a

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, trasc

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman]

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

[sbueringer]

I'll take a look soon, just low bandwidth at the moment

[sbueringer]

In general okay. One comment about the option name and documentation

[k8s-ci-robot]

@trasc: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-controller-runtime-apidiff	b5419bf	link	false	/test pull-controller-runtime-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[sbueringer]

Thank you!!

/lgtm

@alvaroaleman Keeping the hold in case you want to take a final look after the last change

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: f8f793a6960067cffea8b08e87822f9d14246b85