🐛 Fix custom defaulter: avoid deleting unknown fields

[alculquicondor]

When creating a patch for the custom defaulter, pass the original object through decoding.

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

Added a test for it. This is the patch if we don't use the coder:

     <[]jsonpatch.Operation | len:2, cap:2>: [
          {Operation: "add", Path: "/replica", Value: <float64>2},
          {Operation: "remove", Path: "/newField", Value: nil},
      ]

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alculquicondor
Once this PR has been reviewed and has the lgtm label, please assign alvaroaleman 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

[alculquicondor]

This will allow to fix kubernetes-sigs/kueue#2878 once we upgrade

[tenzen-y]

Greate improvements!
Just a nit.

[tenzen-y]

Suggested change

	It("should should not lose unknown fields", func() {
	It("should not lose unknown fields", func() {

:)

[sbueringer]

I think this can lead to invalid patches. Now we are calculating patches based on how original looks like after unmarshal + marshal. But the apiserver will have to apply the patches on the original JSON object

(We had problems like this when implementing our own patch calculation like it is suggested here on this PR)

[sbueringer]

Example: Imagine an API type that through (probably incorrect) omitempty / pointer/non-pointer fields will add additional fields to marshalledOriginal. This could be e.g.:

1. a scalar field with an empty value or
2. an object field with {}.

Now the resulting patches will:

1. if the scalar field is defaulted: generate a replace instead of an add for the field
2. if a field should be added through defaulting to this object field: it will just add the field and miss a patch to add the parent object

[alculquicondor]

I see your point, but both the existing and the proposed approaches are buggy.

Generally, the apiserver would have to go through the same unmarshal + marshal process, then the "raw" contents should not differ. A problem like the one you mention could arise when the webhook uses newer APIs than the API server has registered.

The problem I present arises when the API server has newer APIs than the webhook.

Not sure which can be more common.

[sbueringer]

Generally, the apiserver would have to go through the same unmarshal + marshal process, then the "raw" contents should not differ

For CRDs the apiserver doesn't even have the Go API types :)

Yeah it's definitely a tricky problem. I just think that if we now switch from one buggy version to another we will break a lot of people that currently have (somewhat) working implementations with controller-runtime

[alculquicondor]

Can you please look at #2932?

[k8s-ci-robot]

PR needs rebase.

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.

[alculquicondor]

/close

In favor of an approach closer to #2932

[k8s-ci-robot]

@alculquicondor: Closed this PR.

In response to this:

/close

In favor of an approach closer to #2932

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.