diff --git a/api/resource/resource_test.go b/api/resource/resource_test.go index 8e2b692ed..3bc4d89c5 100644 --- a/api/resource/resource_test.go +++ b/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 diff --git a/kyaml/yaml/fns.go b/kyaml/yaml/fns.go index e80eb48bf..e0802a897 100644 --- a/kyaml/yaml/fns.go +++ b/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)) } diff --git a/kyaml/yaml/merge2/merge2.go b/kyaml/yaml/merge2/merge2.go index 6c09e1fc2..3b23019e1 100644 --- a/kyaml/yaml/merge2/merge2.go +++ b/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 diff --git a/kyaml/yaml/merge2/scalar_test.go b/kyaml/yaml/merge2/scalar_test.go index 0b5f16e19..95546d122 100644 --- a/kyaml/yaml/merge2/scalar_test.go +++ b/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, diff --git a/kyaml/yaml/rnode.go b/kyaml/yaml/rnode.go index bf3b5347f..07c782d73 100644 --- a/kyaml/yaml/rnode.go +++ b/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 }