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 special flag node that is `null` but never
removed during merges. The added `TestApplySmPatch_Idempotency` test
verifies this behaviour. However, this approach  will may change the
value of the node in the output, e.g. if originally there was `field: ~`
it would be replaced by `field: null`. The added test case in
`kyaml/yaml/merge2/scalar_test.go` demonstrates this behaviour (this
test currently fails as I expect the desired outcome is to preserve the
null marker)

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,40 @@ spec:
 `, string(bytes))
 }
 
+func TestApplySmPatch_Idempotency(t *testing.T) {
+	patchApplyCount := 2
+	resourceYaml := `apiVersion: v1
+kind: Deployment
+metadata:
+  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

@@ -14,6 +14,9 @@ import (
 	yaml "sigs.k8s.io/yaml/goyaml.v3"
 )
 
+// KeepNullNode is used to persist a "null" node
+var KeepNullNode = NewRNode(&Node{Tag: NodeTagNull, Value: "null", Kind: yaml.ScalarNode})
+
 // Append creates an ElementAppender
 func Append(elements ...*yaml.Node) ElementAppender {
 	return ElementAppender{Elements: elements}
@@ -725,7 +728,7 @@ func (s FieldSetter) Filter(rn *RNode) (*RNode, error) {
 	}
 
 	// Clear the field if it is empty, or explicitly null
-	if s.Value == nil || s.Value.IsTaggedNull() {
+	if (s.Value == nil || s.Value.IsTaggedNull()) && s.Value != KeepNullNode {
 		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.KeepNullNode, 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,