search

kubernetes-sigs / kustomize

Customization of kubernetes YAML configurations
Apache License 2.0
10.76k stars 2.22k forks source link

yaml Walker removes null values from map #4740

Closed tal-sapan closed 2 days ago

tal-sapan commented 1 year ago

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 commented 1 year ago

@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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
KnVerey commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

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

You can:

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

/lifecycle stale

KnVerey commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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:

You can:

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

/lifecycle rotten

tal-sapan commented 1 year ago

/remove-lifecycle rotten

k8s-triage-robot commented 1 year ago

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

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

You can:

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

/lifecycle stale

tal-sapan commented 1 year ago

/remove-lifecycle stale

k8s-triage-robot commented 6 months ago

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

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

You can:

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

/lifecycle stale

tal-sapan commented 6 months ago

/remove-lifecycle stale

k8s-triage-robot commented 3 months ago

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

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

You can:

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

/lifecycle stale

k8s-triage-robot commented 2 months ago

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:

You can:

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

/lifecycle rotten

k8s-triage-robot commented 1 month ago

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

This bot triages issues according to the following rules:

You can:

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

/close not-planned

k8s-ci-robot commented 1 month ago

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to [this](https://github.com/kubernetes-sigs/kustomize/issues/4740#issuecomment-2184070745): >The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. > >This bot triages 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: >- Reopen this issue with `/reopen` >- Mark this issue as fresh with `/remove-lifecycle rotten` >- Offer to help out with [Issue Triage][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close not-planned > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
stormqueen1990 commented 1 month ago

/reopen

k8s-ci-robot commented 1 month ago

@stormqueen1990: Reopened this issue.

In response to [this](https://github.com/kubernetes-sigs/kustomize/issues/4740#issuecomment-2184071280): >/reopen > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
k8s-triage-robot commented 2 days ago

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

This bot triages issues according to the following rules:

You can:

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

/close not-planned

k8s-ci-robot commented 2 days ago

@k8s-triage-robot: Closing this issue, marking it as "Not Planned".

In response to [this](https://github.com/kubernetes-sigs/kustomize/issues/4740#issuecomment-2243259950): >The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. > >This bot triages 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: >- Reopen this issue with `/reopen` >- Mark this issue as fresh with `/remove-lifecycle rotten` >- Offer to help out with [Issue Triage][1] > >Please send feedback to sig-contributor-experience at [kubernetes/community](https://github.com/kubernetes/community). > >/close not-planned > >[1]: https://www.kubernetes.dev/docs/guide/issue-triage/ Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.