Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

retain quotes in namespace transformer filter #4421

Merged
merged 15 commits into from Mar 23, 2022
Merged
3 changes: 0 additions & 3 deletions api/filters/filtersutil/setters.go
Expand Up @@ -21,9 +21,6 @@ func SetEntry(key, value, tag string) SetFn {
Value: value,
Tag: tag,
}
if tag == yaml.NodeTagString && yaml.IsYaml1_1NonString(n) {
n.Style = yaml.DoubleQuotedStyle
}
return func(node *yaml.RNode) error {
return node.PipeE(yaml.FieldSetter{
Name: key,
Expand Down
6 changes: 3 additions & 3 deletions api/filters/namespace/namespace.go
Expand Up @@ -52,7 +52,7 @@ func (ns Filter) run(node *yaml.RNode) (*yaml.RNode, error) {
// transformations based on data -- :)
err := node.PipeE(fsslice.Filter{
FsSlice: ns.FsSlice,
SetValue: ns.trackableSetter.SetScalar(ns.Namespace),
SetValue: ns.trackableSetter.SetEntry("", ns.Namespace, yaml.NodeTagString),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed changes to SetScalar and am using SetEntry("", ns.Namespace, <tag>) where a tag is needed, so as not to make any changes to public functions.
I only added tag to namespace and replicacount filters to minimize changes but could add to other filters (suffix, prefix, etc.) if requested so they don't also remove quotes.

I'm resolving the outdated comments with @natasha41575 but would like to know if using SetEntry is fine in these cases, since that allows moving the addition of the double quotes style to the FieldSetter func in the kyaml package, which should work whenever the scalar type tag is set on the rnode passed to it.

The other problems mentioned in the issue I opened (configmap "true|false" values, a separate env variable issue #4472) are not resolved by this PR.

CreateKind: yaml.ScalarNode, // Namespace is a ScalarNode
CreateTag: yaml.NodeTagString,
})
Expand Down Expand Up @@ -85,7 +85,7 @@ func (ns Filter) metaNamespaceHack(obj *yaml.RNode, gvk resid.Gvk) error {
FsSlice: []types.FieldSpec{
{Path: types.MetadataNamespacePath, CreateIfNotPresent: true},
},
SetValue: ns.trackableSetter.SetScalar(ns.Namespace),
SetValue: ns.trackableSetter.SetEntry("", ns.Namespace, yaml.NodeTagString),
CreateKind: yaml.ScalarNode, // Namespace is a ScalarNode
}
_, err := f.Filter(obj)
Expand Down Expand Up @@ -138,7 +138,7 @@ func (ns Filter) roleBindingHack(obj *yaml.RNode, gvk resid.Gvk) error {
return err
}

return ns.trackableSetter.SetScalar(ns.Namespace)(node)
return ns.trackableSetter.SetEntry("", ns.Namespace, yaml.NodeTagString)(node)

})

Expand Down
34 changes: 34 additions & 0 deletions api/filters/namespace/namespace_test.go
Expand Up @@ -332,26 +332,60 @@ a:
expectedSetValueArgs: []filtertest_test.SetValueArg{
{
Value: "bar",
Tag: "!!str",
NodePath: []string{"metadata", "namespace"},
},
{
Value: "bar",
Tag: "!!str",
NodePath: []string{"a", "b", "c"},
},
{
Value: "bar",
Tag: "!!str",
NodePath: []string{"metadata", "namespace"},
},
{
Value: "bar",
Tag: "!!str",
NodePath: []string{"namespace"},
},
{
Value: "bar",
Tag: "!!str",
NodePath: []string{"a", "b", "c"},
},
},
},

{
name: "numeric",
input: `
apiVersion: example.com/v1
kind: Foo
metadata:
name: instance
---
apiVersion: example.com/v1
kind: Bar
metadata:
name: instance
`,
expected: `
apiVersion: example.com/v1
kind: Foo
metadata:
name: instance
namespace: "01234"
---
apiVersion: example.com/v1
kind: Bar
metadata:
name: instance
namespace: "01234"
`,
filter: namespace.Filter{Namespace: "01234"},
},
}

type TestCase struct {
Expand Down
2 changes: 1 addition & 1 deletion api/filters/replicacount/replicacount.go
Expand Up @@ -41,5 +41,5 @@ func (rc Filter) run(node *yaml.RNode) (*yaml.RNode, error) {
}

func (rc Filter) set(node *yaml.RNode) error {
return rc.trackableSetter.SetScalar(strconv.FormatInt(rc.Replica.Count, 10))(node)
return rc.trackableSetter.SetEntry("", strconv.FormatInt(rc.Replica.Count, 10), yaml.NodeTagInt)(node)
}
1 change: 1 addition & 0 deletions api/filters/replicacount/replicacount_test.go
Expand Up @@ -192,6 +192,7 @@ spec:
expectedSetValueArgs: []filtertest_test.SetValueArg{
{
Value: "42",
Tag: "!!int",
NodePath: []string{"spec", "replicas"},
},
},
Expand Down
4 changes: 2 additions & 2 deletions api/internal/generators/configmap_test.go
Expand Up @@ -140,7 +140,7 @@ metadata:
foo: 'bar'
data:
a: x
b: y
b: "y"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does "y" get quoted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is because LoadMapIntoConfigMapData uses yaml.FieldSetter, so it will have the DoubleQuotedStyle added by the check in to FieldSetter.Filter since it's a yaml1.1 non-string and has the string tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured it's okay to change this test since this is an invalid config (I shared output in my comment in the linked issue #4386 showing issues with invalid yaml 1.1 values in config map data).

Any data that should be quoted and isn't passed through FieldSetter.Filter won't be corrected.

This won't fix #4472, idk if the env var gets passed through FieldSetter.Filter at all but the env var losing quotes in the example is not an invalid yaml 1.1 string and that config can be applied.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. I had to do some research because I'm not a yaml expert, but TIL that "y" is the equivalent of "true" in yaml 1.1.

c: Hello World
d: "true"
`,
Expand Down Expand Up @@ -181,7 +181,7 @@ metadata:
river: 'Missouri'
data:
a: x
b: y
b: "y"
c: Hello World
d: "true"
immutable: true
Expand Down
8 changes: 7 additions & 1 deletion api/internal/generators/utils.go
Expand Up @@ -83,9 +83,15 @@ func setImmutable(
return nil
}
if opts.Immutable {
if _, err := rn.Pipe(yaml.SetField("immutable", yaml.NewScalarRNode("true"))); err != nil {
n := &yaml.Node{
Kind: yaml.ScalarNode,
Value: "true",
Tag: yaml.NodeTagBool,
}
if _, err := rn.Pipe(yaml.FieldSetter{Name: "immutable", Value: yaml.NewRNode(n)}); err != nil {
natasha41575 marked this conversation as resolved.
Show resolved Hide resolved
return err
}
}

return nil
}
@@ -1,7 +1,7 @@
# Copyright 2019 The Kubernetes Authors.
# SPDX-License-Identifier: Apache-2.0

FROM golang:1.13-stretch
natasha41575 marked this conversation as resolved.
Show resolved Hide resolved
FROM golang:1.16-alpine
ENV CGO_ENABLED=0
WORKDIR /go/src/
COPY . .
Expand Down
2 changes: 1 addition & 1 deletion cmd/config/internal/commands/e2e/e2econtainerconfig/go.mod
Expand Up @@ -2,4 +2,4 @@ module sigs.k8s.io/kustomize/cmd/config/internal/commands/e2e/e2econtainerconfig

go 1.16

require sigs.k8s.io/kustomize/kyaml v0.1.10
require sigs.k8s.io/kustomize/kyaml v0.13.2
664 changes: 556 additions & 108 deletions cmd/config/internal/commands/e2e/e2econtainerconfig/go.sum

Large diffs are not rendered by default.

26 changes: 15 additions & 11 deletions cmd/config/internal/commands/e2e/e2econtainerconfig/main.go
Expand Up @@ -8,6 +8,8 @@ import (
"os"

"sigs.k8s.io/kustomize/kyaml/fn/framework"
"sigs.k8s.io/kustomize/kyaml/fn/framework/command"
"sigs.k8s.io/kustomize/kyaml/kio"
"sigs.k8s.io/kustomize/kyaml/yaml"
)

Expand All @@ -32,27 +34,29 @@ type Example struct {

func main() {
functionConfig := &Example{}
resourceList := &framework.ResourceList{FunctionConfig: functionConfig}

cmd := framework.Command(resourceList, func() error {
for i := range resourceList.Items {
if err := resourceList.Items[i].PipeE(yaml.SetAnnotation("a-string-value",
fn := func(items []*yaml.RNode) ([]*yaml.RNode, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't impact tests passing or not but I think having this updated helps rule out whether this is the source of a bug in the run fs command unit tests. I can remove if a reviewer doesn't think this should be included.

for i := range items {
// set the annotation on each resource item
if err := items[i].PipeE(yaml.SetAnnotation("a-string-value",
functionConfig.Data.StringValue)); err != nil {
return err
return nil, err
}

if err := resourceList.Items[i].PipeE(yaml.SetAnnotation("a-int-value",
if err := items[i].PipeE(yaml.SetAnnotation("a-int-value",
fmt.Sprintf("%v", functionConfig.Data.IntValue))); err != nil {
return err
return nil, err
}

if err := resourceList.Items[i].PipeE(yaml.SetAnnotation("a-bool-value",
if err := items[i].PipeE(yaml.SetAnnotation("a-bool-value",
fmt.Sprintf("%v", functionConfig.Data.BoolValue))); err != nil {
return err
return nil, err
}
}
return nil
})
return items, nil
}
p := framework.SimpleProcessor{Filter: kio.FilterFunc(fn), Config: functionConfig}
cmd := command.Build(p, command.StandaloneDisabled, false)
natasha41575 marked this conversation as resolved.
Show resolved Hide resolved

if err := cmd.Execute(); err != nil {
os.Exit(1)
Expand Down
11 changes: 9 additions & 2 deletions cmd/config/internal/commands/run-fns.go
Expand Up @@ -135,9 +135,14 @@ func (r *RunFnRunner) getContainerFunctions(c *cobra.Command, dataItems []string
return nil, err
}
if r.Network {
n := &yaml.Node{
Kind: yaml.ScalarNode,
Value: "true",
Tag: yaml.NodeTagBool,
}
err = fn.PipeE(
yaml.Lookup("container"),
yaml.SetField("network", yaml.NewScalarRNode("true")))
yaml.SetField("network", yaml.NewRNode(n)))
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -231,7 +236,9 @@ data: {}
if len(kv) != 2 {
return nil, fmt.Errorf("args must have keys and values separated by =")
}
err := dataField.PipeE(yaml.SetField(kv[0], yaml.NewScalarRNode(kv[1])))
// do not set style since function should determine tag and style
err := dataField.PipeE(
yaml.FieldSetter{Name: kv[0], Value: yaml.NewScalarRNode(kv[1]), OverrideStyle: true})
if err != nil {
return nil, err
}
Expand Down
17 changes: 15 additions & 2 deletions kyaml/fn/framework/command/command_test.go
Expand Up @@ -5,6 +5,7 @@ package command_test

import (
"bytes"
"fmt"
"io/ioutil"
"os"
"path/filepath"
Expand Down Expand Up @@ -66,6 +67,7 @@ ENTRYPOINT ["function"]
func TestCommand_standalone(t *testing.T) {
var config struct {
A string `json:"a" yaml:"a"`
B int `json:"b" yaml:"b"`
}

fn := func(items []*yaml.RNode) ([]*yaml.RNode, error) {
Expand All @@ -83,6 +85,10 @@ metadata:
if err != nil {
return nil, err
}
err = items[i].PipeE(yaml.SetAnnotation("b", fmt.Sprintf("%v", config.B)))
if err != nil {
return nil, err
}
}

return items, nil
Expand All @@ -99,6 +105,7 @@ metadata:
func TestCommand_standalone_stdin(t *testing.T) {
var config struct {
A string `json:"a" yaml:"a"`
B int `json:"b" yaml:"b"`
shanalily marked this conversation as resolved.
Show resolved Hide resolved
}

p := &framework.SimpleProcessor{
Expand All @@ -119,6 +126,10 @@ metadata:
if err != nil {
return nil, err
}
err = items[i].PipeE(yaml.SetAnnotation("b", fmt.Sprintf("%v", config.B)))
if err != nil {
return nil, err
}
}

return items, nil
Expand Down Expand Up @@ -150,7 +161,8 @@ metadata:
namespace: default
annotations:
foo: bar1
a: 'b'
a: 'c'
b: '1'
spec:
replicas: 1
---
Expand All @@ -161,6 +173,7 @@ metadata:
namespace: default
annotations:
foo: bar2
a: 'b'
a: 'c'
b: '1'
`), strings.TrimSpace(out.String()))
}
3 changes: 2 additions & 1 deletion kyaml/fn/framework/command/testdata/standalone/config.yaml
Expand Up @@ -3,4 +3,5 @@

apiVersion: example.com/v1alpha1
kind: Foo
a: b
a: c
b: 1
6 changes: 4 additions & 2 deletions kyaml/fn/framework/command/testdata/standalone/expected.yaml
Expand Up @@ -8,7 +8,8 @@ metadata:
namespace: default
annotations:
foo: bar2
a: 'b'
a: 'c'
b: '1'
---
apiVersion: apps/v1
kind: Deployment
Expand All @@ -17,4 +18,5 @@ metadata:
namespace: default
annotations:
foo: bar1
a: 'b'
a: 'c'
b: '1'
7 changes: 6 additions & 1 deletion kyaml/setters2/set.go
Expand Up @@ -479,7 +479,12 @@ func (s SetOpenAPI) Filter(object *yaml.RNode) (*yaml.RNode, error) {
}

if s.IsSet {
if err := def.PipeE(&yaml.FieldSetter{Name: "isSet", StringValue: "true"}); err != nil {
n := &yaml.Node{
Kind: yaml.ScalarNode,
Value: "true",
Tag: yaml.NodeTagBool,
}
if err := def.PipeE(&yaml.FieldSetter{Name: "isSet", Value: yaml.NewRNode(n)}); err != nil {
return nil, err
}
}
Expand Down
6 changes: 6 additions & 0 deletions kyaml/yaml/fns.go
Expand Up @@ -641,6 +641,12 @@ func (s FieldSetter) Filter(rn *RNode) (*RNode, error) {
s.Value = NewScalarRNode(s.StringValue)
}

// need to set style for strings not recognized by yaml 1.1 to quoted if not previously set
// TODO: fix in upstream yaml library so this can be handled with yaml SetString
if IsStringValue(s.Value) && !s.OverrideStyle && s.Value.YNode().Style == 0 && IsYaml1_1NonString(s.Value.YNode()) {
shanalily marked this conversation as resolved.
Show resolved Hide resolved
s.Value.YNode().Style = yaml.DoubleQuotedStyle
}

if s.Name == "" {
if err := ErrorIfInvalid(rn, yaml.ScalarNode); err != nil {
return rn, err
Expand Down
6 changes: 6 additions & 0 deletions kyaml/yaml/rnode.go
Expand Up @@ -35,6 +35,12 @@ func IsEmptyMap(node *RNode) bool {
return IsMissingOrNull(node) || IsYNodeEmptyMap(node.YNode())
}

// IsStringValue is true if the RNode is not nil and is scalar string node
// TODO: make this a method on RNode.
natasha41575 marked this conversation as resolved.
Show resolved Hide resolved
func IsStringValue(node *RNode) bool {
return !node.IsNil() && IsYNodeString(node.YNode())
}

// GetValue returns underlying yaml.Node Value field
func GetValue(node *RNode) string {
if IsMissingOrNull(node) {
Expand Down