yaml Walker removes null values from map

[tal-sapan]

Describe the bug

When implementing a custom transformer using the Walker type (from "sigs.k8s.io/kustomize/kyaml/yaml/walk") to visit yaml nodes, in the nodes returned by the Walker, any keys that had null values in mapping nodes are removed. This is true even if the visitor function passed to the Walker doesn't do anything. This is inconsistent with the case when we don't use the transformer, even though the output should be the same if the transformer didn't change anything.

Files that can reproduce the issue

The custom transformer implementation is below, and after it a failing test that reproduces the issue. The test doesn't run Kustomize directly, just the transformer, since this is where the issue is. Please let me know if you need an example that runs Kustomize.

main.go

package main

import (
	"fmt"
	"os"
	"sigs.k8s.io/kustomize/kyaml/kio"
	"sigs.k8s.io/kustomize/kyaml/openapi"
	"sigs.k8s.io/kustomize/kyaml/yaml"
	"sigs.k8s.io/kustomize/kyaml/yaml/walk"
)

func main() {
	err := executePipeline(&kio.ByteReader{Reader: os.Stdin}, &kio.ByteWriter{Writer: os.Stdout})
	if err != nil {
		fmt.Printf("Error executing plugin: %v", err)
		os.Exit(1)
	}
}

func executePipeline(reader *kio.ByteReader, writer *kio.ByteWriter) error {
	return kio.Pipeline{
		Inputs: []kio.Reader{reader},
		Filters: []kio.Filter{
			WalkerFilter{},
		},
		Outputs: []kio.Writer{writer},
	}.Execute()
}

type WalkerFilter struct {
}

func (n WalkerFilter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
	for _, target := range nodes {
		noopWalker := walk.Walker{
			Sources: []*yaml.RNode{target},
			Visitor: NoopVisitor{},
		}
		_, err := noopWalker.Walk()
		if err != nil {
			return nodes, err
		}
	}
	return nodes, nil
}

type NoopVisitor struct {
}

func (n NoopVisitor) VisitMap(sources walk.Sources, schema *openapi.ResourceSchema) (*yaml.RNode, error) {
	return sources.Dest(), nil
}

func (n NoopVisitor) VisitScalar(sources walk.Sources, schema *openapi.ResourceSchema) (*yaml.RNode, error) {
	return sources.Dest(), nil
}

func (n NoopVisitor) VisitList(sources walk.Sources, schema *openapi.ResourceSchema, kind walk.ListKind) (*yaml.RNode, error) {
	return sources.Dest(), nil
}

main_test.go

package main

import (
	"bytes"
	"strings"
	"testing"

	"sigs.k8s.io/kustomize/kyaml/kio"
)

var input = `
apiVersion: example.com/v1alpha1
kind: Deployment
metadata:
  name: resource-name
spec:
  testNull: null
`

var expected = `
apiVersion: example.com/v1alpha1
kind: Deployment
metadata:
  name: resource-name
spec:
  testNull: null
`

func Test_executePipeline(t *testing.T) {
	type args struct {
		reader *kio.ByteReader
	}
	tests := []struct {
		name     string
		args     args
		expected string
	}{
		{
			name: "TestNull",
			args: args{
				reader: &kio.ByteReader{Reader: bytes.NewReader([]byte(input))},
			},
			expected: expected,
		},
	}
	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			actual := &bytes.Buffer{}
			if err := executePipeline(tt.args.reader, &kio.ByteWriter{Writer: actual}); err != nil {
				t.Errorf("executePipeline() error = %v", err)
			}

			result := actual.String()
			if strings.TrimSpace(tt.expected) != strings.TrimSpace(result) {
				t.Errorf("Expected:\n%s\nGot:\n%s", tt.expected, result)
			}
		})
	}
}

Expected output

The test should pass.

Actual output

=== RUN   Test_executePipeline
--- FAIL: Test_executePipeline (0.71s)
=== RUN   Test_executePipeline/TestNull
    main_test.go:55: Expected:
        
        apiVersion: example.com/v1alpha1
        kind: Deployment
        metadata:
          name: resource-name
        spec:
          testNull: null
        
        Got:
        apiVersion: example.com/v1alpha1
        kind: Deployment
        metadata:
          name: resource-name
        spec: {}
    --- FAIL: Test_executePipeline/TestNull (0.71s)

Kustomize version

Tested using sigs.k8s.io/kustomize/kyaml v0.13.8

Platform

Windows

Additional context

The removal of null values happens in the FieldSetter.Filter method: https://github.com/kubernetes-sigs/kustomize/blob/e57b5d283f52509262d5ad120cfe6e717c006696/kyaml/yaml/fns.go#L713-L715 This is used from the Walker here: https://github.com/kubernetes-sigs/kustomize/blob/e57b5d283f52509262d5ad120cfe6e717c006696/kyaml/yaml/walk/map.go#L93 I think at least the custom transformer owner should be able to specify if the Walker should remove null values or not, if this is required in certain cases.

[k8s-ci-robot]

@tal-sapan: This issue is currently awaiting triage.

SIG CLI takes a lead on issue triage for this repo, but any Kubernetes member can accept issues by applying the triage/accepted label.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[KnVerey]

May I ask why you're using the Walker to implement custom transformers rather than the higher level tools in the kyaml/fn/framework package? For Kustomize, Walker is used in the implementation of Strategic Merge Patch functionality, so I'm guessing that is why null values are treated as deletion directives; that's what a null value in an SMP indicates. That said, I suspect the LOC that is causing a problem for you is likely causing this for Kustomize as well: https://github.com/kubernetes-sigs/kustomize/issues/4628. Since it is a null in the source we want Kustomize to ignore, it seems plausible the solution could address your issue as well.

/unassign

[tal-sapan]

Hi,

We are using the Walker to replace the values of environment variables or other configurable parameters used in string values in the yaml (e.g. if a string value in the yaml contains the substring ${MY_ENV_VAR} it will be replaced with the value of the environment variable MY_ENV_VAR or a similarly named configuration parameter of the kustomize plugin). Do you think there is a better solution for this functionality?

Thanks, Tal

[KnVerey]

If I'm understanding correctly that the input can contain variables in arbitrary positions within any field of any type, then I think you're right that the higher level tools (I was thinking kyaml/fn/framework processors or perhaps kyaml/yaml/fns setters) don't have anything that will help much, since they're all resource-oriented.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[KnVerey]

We merged a fix to #4628 in the latest release. Can you please confirm whether that fixed your issue with the Walker as well?

[tal-sapan]

Hi @KnVerey ,

I upgraded sigs.k8s.io/kustomize/kyaml to the newest version (v0.14.0) but the issue was not fixed, it's still the same behavior (which can be reproduced with the files I added when I opened the issue).

Thanks, Tal

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle rotten
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[tal-sapan]

/remove-lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[tal-sapan]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[tal-sapan]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle rotten
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten