Fix null YAML values being replaced by "null"

Related issues:

* kubernetes-sigs#5031
* kubernetes-sigs#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 `RNode`s that is checked before
clearing any node we should keep. The added
`TestApplySmPatch_Idempotency` test verifies this behaviour.

See also kubernetes-sigs#5365 for an
alternative approach

api/resource/resource_test.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"sigs.k8s.io/kustomize/api/internal/utils"
 	"sigs.k8s.io/kustomize/api/provider"
 	. "sigs.k8s.io/kustomize/api/resource"
@@ -281,6 +282,43 @@ spec:
 `, string(bytes))
 }
 
+// regression test for https://github.com/kubernetes-sigs/kustomize/issues/5031
+func TestApplySmPatch_Idempotency(t *testing.T) {
+	// an arbitrary number of times to apply the patch
+	patchApplyCount := 4
+	resourceYaml := `apiVersion: v1
+kind: Deployment
+metadata:
+  creationTimestamp: null
+  labels: null
+  name: my-deployment
+`
+	resource, err := factory.FromBytes([]byte(resourceYaml))
+	require.NoError(t, err)
+
+	noOpPatch, err := factory.FromBytes([]byte(`
+apiVersion: v1
+kind: Deployment
+metadata:
+  name: my-deployment
+`))
+	require.NoError(t, err)
+
+	for i := 0; i < patchApplyCount; i++ {
+		require.NoError(t, resource.ApplySmPatch(noOpPatch))
+
+		bytes, err := resource.AsYAML()
+		require.NoError(t, err)
+
+		require.Equal(
+			t,
+			resourceYaml,
+			string(bytes),
+			"resource should be unchanged after re-application of patch",
+		)
+	}
+}
+
 func TestApplySmPatchShouldOutputListItemsInCorrectOrder(t *testing.T) {
 	cases := []struct {
 		name           string

kyaml/yaml/fns.go

@@ -724,8 +724,14 @@ func (s FieldSetter) Filter(rn *RNode) (*RNode, error) {
 		return rn, nil
 	}
 
-	// Clear the field if it is empty, or explicitly null
-	if s.Value == nil || s.Value.IsTaggedNull() {
+	// Clearing nil fields:
+	//   1. Clear any fields with no value
+	//   2. Clear any "null" YAML fields unless we explicitly want to keep them
+	// This is to balance
+	//   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.ShouldKeep) {
 		return rn.Pipe(Clear(s.Name))
 	}
 

kyaml/yaml/merge2/merge2.go

@@ -66,8 +66,7 @@ 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 a new node so that it won't have a "!!null" tag and therefore won't be cleared.
-			return yaml.NewScalarRNode(nodes.Dest().YNode().Value), nil
+			return yaml.MakePersistentNullNode(nodes.Dest().YNode().Value), nil
 		}
 
 		return nodes.Origin(), nil

kyaml/yaml/merge2/scalar_test.go

@@ -197,6 +197,22 @@ field: null
 		expected: `
 kind: Deployment
 field: null
+`,
+		mergeOptions: yaml.MergeOptions{
+			ListIncreaseDirection: yaml.MergeOptionsListAppend,
+		},
+	},
+	{description: `keep scalar -- missing in src, null in dest, preserves null marker`,
+		source: `
+kind: Deployment
+`,
+		dest: `
+kind: Deployment
+field: ~
+`,
+		expected: `
+kind: Deployment
+field: ~
 `,
 		mergeOptions: yaml.MergeOptions{
 			ListIncreaseDirection: yaml.MergeOptionsListAppend,

kyaml/yaml/rnode.go

@@ -24,6 +24,20 @@ func MakeNullNode() *RNode {
 	return NewRNode(&Node{Tag: NodeTagNull})
 }
 
+// MakePersistentNullNode returns an RNode that should be persisted,
+// even when merging
+func MakePersistentNullNode(value string) *RNode {
+	n := NewRNode(
+		&Node{
+			Tag:   NodeTagNull,
+			Value: value,
+			Kind:  yaml.ScalarNode,
+		},
+	)
+	n.ShouldKeep = true
+	return n
+}
+
 // IsMissingOrNull is true if the RNode is nil or explicitly tagged null.
 // TODO: make this a method on RNode.
 func IsMissingOrNull(node *RNode) bool {
@@ -214,6 +228,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
 }