Fix null YAML values being replaced by "null"

[matthewhughes934]

Related issues:

• Creation timestamp set to invalid value "null" when using kustomize v5.0.0 #5031
• Empty Statefulset nodeAffinity gets build with string value "null" in complex scenario  #5171

After noting this behaviour was not present in
d89b448 a git bisect pointed to the
change 1b7db20. The issue with that
change is that upon seeing a null node it would replace it with a node
whose value was equivalent but without a !!null tag. This meant that
one application of a patch would have the desired approach: the result
would be null in the output, but on a second application of a similar
patch the field would be rendered as "null".

To avoid this, define a new attribute on RNodes that is checked before
clearing any node we should keep. The added
TestApplySmPatch_Idempotency test verifies this behaviour.

See also #5365 for an
alternative approach

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: matthewhughes934 / name: Matthew Hughes (8aafbac)

[k8s-ci-robot]

Welcome @matthewhughes934!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @matthewhughes934. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[k8s-ci-robot]

This PR has multiple commits, and the default merge method is: merge.
You can request commits to be squashed using the label: tide/merge-method-squash

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/test-infra repository.

[justinsb]

I was wondering why this change was necessarily. I tried it out and it was because otherwise the nil we so carefully preserved would be cleared. Can we update the comment though, maybe adding "...unless it is our KeepNull marker" (though admittedly this makes the IsTaggedNull / "explicitly null" sort of confusing)

[matthewhughes934]

I was wondering why this change was necessarily. I tried it out and it was because otherwise the nil we so carefully preserved would be cleared. Can we update the comment though, maybe adding "...unless it is our KeepNull marker" (though admittedly this makes the IsTaggedNull / "explicitly null" sort of confusing)

Good call, I tried to express the my understanding with c18fcbc

[justinsb]

I applied this patch locally and verified that it fixed the null -> "null" problem for me. I also verified that if we don't have the && s.Value != KeepNullNode that the nulls are removed entirely, so that line is needed (I didn't understand why at first, personally)

I don't think I have approval rights, but I may be able to mark this as...

/ok-to-test

[stormqueen1990]

/cc

[matthewhughes934]

An alternative (I think possibly nicer) approach that preserves the value of the nil field is to add a new attribute on RNodes like:

diff --git a/kyaml/yaml/fns.go b/kyaml/yaml/fns.go
index c0bfbe609..2a2f5f2b3 100644
--- a/kyaml/yaml/fns.go
+++ b/kyaml/yaml/fns.go
@@ -734,7 +734,8 @@ func (s FieldSetter) Filter(rn *RNode) (*RNode, error) {
        //   1. Persisting 'null' values passed by the user (see issue #4628)
        //   2. Avoiding producing noisy documents that add any field defaulting to
        //   'nil' even if they weren't present in the source document
-       if (s.Value == nil || s.Value.IsTaggedNull()) && s.Value != KeepNullNode {
+       // Clear the field if it is empty, or explicitly null
+       if s.Value == nil || (s.Value.IsTaggedNull() && !s.Value.ShouldKeep) {
                return rn.Pipe(Clear(s.Name))
        }
 
diff --git a/kyaml/yaml/merge2/merge2.go b/kyaml/yaml/merge2/merge2.go
index 4942092ad..585a48096 100644
--- a/kyaml/yaml/merge2/merge2.go
+++ b/kyaml/yaml/merge2/merge2.go
@@ -66,7 +66,9 @@ func (m Merger) VisitMap(nodes walk.Sources, s *openapi.ResourceSchema) (*yaml.R
 
                // If Origin is missing, preserve explicitly set null in Dest ("null", "~", etc)
                if nodes.Origin().IsNil() && !nodes.Dest().IsNil() && len(nodes.Dest().YNode().Value) > 0 {
-                       return yaml.KeepNullNode, nil
+                       n := yaml.NewScalarRNode(nodes.Dest().YNode().Value)
+                       n.ShouldKeep = true
+                       return n, nil
                }
 
                return nodes.Origin(), nil
diff --git a/kyaml/yaml/rnode.go b/kyaml/yaml/rnode.go
index bf3b5347f..8bdb9e8bd 100644
--- a/kyaml/yaml/rnode.go
+++ b/kyaml/yaml/rnode.go
@@ -214,6 +214,9 @@ type RNode struct {
        // object root: object root
        value *yaml.Node
 
+       // Whether we should keep this node, even if otherwise we would clear it
+       ShouldKeep bool
+
        Match []string
 }

[stormqueen1990]

Hi there, @matthewhughes934! 👋🏻

Thank you very much for your contribution!

This is looking good.
However, could you please also add a test case that covers the current scenario (a null value being replaced by the word "null"), so we have some coverage to avoid regressions?

[stormqueen1990]

/label tide/merge-method-squash

[matthewhughes934]

This is looking good. However, could you please also add a test case that covers the current scenario (a null value being replaced by the word "null"), so we have some coverage to avoid regressions?

Thanks for the review. The added test TestApplySmPatch_Idempotency functions as a regression test for this change.

just noting: I've updated things to use the approach I mentioned in a comment just above (because I found this was clearer), and squashed all the commits down

[stormqueen1990]

This is looking good. However, could you please also add a test case that covers the current scenario (a null value being replaced by the word "null"), so we have some coverage to avoid regressions?

Thanks for the review. The added test TestApplySmPatch_Idempotency functions as a regression test for this change.

just noting: I've updated things to use the approach I mentioned in a comment just above (because I found this was clearer), and squashed all the commits down

Hi there, @matthewhughes934! 👋🏻

I was looking for an explicit test that fails when the resulting YAML has the string "null", but that's fair.

Thanks for squashing your commits!

[stormqueen1990]

/remove-label tide/merge-method-squash

[stormqueen1990]

/lgtm

[matthewhughes934]

I was looking for an explicit test that fails when the resulting YAML has the string "null", but that's fair.

👍 since I think it wasn't clear, I've added a comment noting that it is the regression test for that issue (just squashed it into the latest change which I've also rebased on master)

[stormqueen1990]

/lgtm

Thanks for adding the comment!

/cc @varshaprasad96
for maintainer review

[varshaprasad96]

Thanks for the fix @matthewhughes934! The changes look good to me!

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matthewhughes934, varshaprasad96

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 [varshaprasad96]

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