Specifying {} to remove nested fields should yield {}, not nil

[alexzielenski]

Consider old object:

{
  requiredField: {
      optionalField1: 1
      optionalFIeld2: 2
   }
}

And patch, all with a single owner

{
    requiredField: {}
}

I expect this to yield:

{
    requiredField: {}
}

But today it yields:

{
    requiredField: nil
}

Which results in an error on k8s since nil is not an acceptable value for this requiredField

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

[alexzielenski]

/cc @apelisse

[k8s-ci-robot]

@alexzielenski: 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-structured-merge-diff-test	d7c26a3	link	true	/test pull-structured-merge-diff-test

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.

[apelisse]

Can we take the opportunity to summarize the various use-cases here that we've found so far? I'm getting a bit lost by the different possibilities and the differences they have. I suspect we should create a table for what happens when nil/empty is sent in various cases because there's a lot of confusion here.

[apelisse]

Should we treat separately empty and nil here? Is that part of the problem?

[alexzielenski]

This change wasn't directly part of the problem I was trying to solve but I think something like this will be needed to have a consistent solution. The behavior of this right now always results in a nil list even if you specify []

[apelisse]

Aren't the other tests usually importing everything from fixture?

[apelisse]

Yeah, so I guess now I'm confused about:

1. What happens if we apply null instead.
2. What happens if you have further level of nesting (i.e. you're apply {} when you had {'nested': {'numeric':1}} (I'm guessing the same but would love to see)
3. How do you entirely remove the field? If null keeps null and {} keep it to empty, then how do you remove it altogether?

[alexzielenski]

Will add cases for 1 & 2

Re: 3. If you wanted to make {nestedMap: { myField: value }} into {} you would just apply {}. If you applied {} then nestedMap field is included in the toRemove set. If you had applied {nestedMap: {}} then only nestedMap.myField is in the toRemove set, so nestedMap would be retained yielding {nestedMap: {}}

[alexzielenski]

For the null case naively I would expect it to keep null if you specified null.

[apelisse]

One last thing, I suspect lists and maps should behave the same. Struct, I'm not entirely sure. maybe they should?

[alexzielenski]

One last thing, I suspect lists and maps should behave the same. Struct, I'm not entirely sure. maybe they should?

This bug also affects structs. There are valid CRDs that can be constructed with defined object fields where {} is valid value users should be able to set. Today they cannot

[ybettan]

Hello.

Is this fix planned to be merged soon? We currently have to bump our API cause we need to add the +nullable kubebuilder annotation to a bunch of API object and adding a new mutating webhook is just breaking any pointers/slices in the API objects.

Ref: kubernetes-sigs/kernel-module-management#826