fix kyaml issue where dropping Style created issues

dropping the node style creates a compatibility issue where quotes around "on" are dropped
because yaml.v3 interprets it as a string.

other yaml parsers interpret on as a bool value, and parse it as a bool rather than string.

fix: retain the original style so it is kept as quoted.

- fmt: don't drop the styles
- merge2: keep the style when merging elements
- setting a field: if changing the value of a scalar field, retain its style by default

cmd/config/internal/commands/cat_test.go

@@ -114,8 +114,8 @@ metadata:
     app: nginx
   annotations:
     app: nginx
-    config.kubernetes.io/package: .
-    config.kubernetes.io/path: f2.yaml
+    config.kubernetes.io/package: '.'
+    config.kubernetes.io/path: 'f2.yaml'
 spec:
   replicas: 3
 `, b.String()) {
@@ -218,8 +218,8 @@ metadata:
   name: foo
   annotations:
     config.kubernetes.io/local-config: "true"
-    config.kubernetes.io/package: .
-    config.kubernetes.io/path: f2.yaml
+    config.kubernetes.io/package: '.'
+    config.kubernetes.io/path: 'f2.yaml'
   configFn:
     container:
       image: gcr.io/example/image:version
@@ -314,8 +314,8 @@ metadata:
   name: foo
   annotations:
     config.kubernetes.io/local-config: "true"
-    config.kubernetes.io/package: .
-    config.kubernetes.io/path: f2.yaml
+    config.kubernetes.io/package: '.'
+    config.kubernetes.io/path: 'f2.yaml'
   configFn:
     container:
       image: gcr.io/example/reconciler:v1
@@ -438,8 +438,8 @@ metadata:
     app: nginx
   annotations:
     app: nginx
-    config.kubernetes.io/package: .
-    config.kubernetes.io/path: f2.yaml
+    config.kubernetes.io/package: '.'
+    config.kubernetes.io/path: 'f2.yaml'
 spec:
   replicas: 3
 `, string(actual)) {
@@ -560,8 +560,8 @@ metadata:
     app: nginx
   annotations:
     app: nginx
-    config.kubernetes.io/package: .
-    config.kubernetes.io/path: f2.yaml
+    config.kubernetes.io/package: '.'
+    config.kubernetes.io/path: 'f2.yaml'
 spec:
   replicas: 3
 `, string(actual)) {

cmd/config/internal/commands/cmdwrap_test.go

@@ -78,8 +78,8 @@ items:
       name: test
       app: nginx
     annotations:
-      config.kubernetes.io/index: "0"
-      config.kubernetes.io/path: config/test_deployment.yaml
+      config.kubernetes.io/index: '0'
+      config.kubernetes.io/path: 'config/test_deployment.yaml'
   spec:
     replicas: 11
     selector:
@@ -109,8 +109,8 @@ items:
       name: test
       app: nginx
     annotations:
-      config.kubernetes.io/index: "0"
-      config.kubernetes.io/path: config/test_service.yaml
+      config.kubernetes.io/index: '0'
+      config.kubernetes.io/path: 'config/test_service.yaml'
   spec:
     selector:
       name: test
@@ -133,8 +133,8 @@ items:
       name: test
       app: nginx
     annotations:
-      config.kubernetes.io/index: "0"
-      config.kubernetes.io/path: config/test_deployment.yaml
+      config.kubernetes.io/index: '0'
+      config.kubernetes.io/path: 'config/test_deployment.yaml'
   spec:
     replicas: 11
     selector:
@@ -161,8 +161,8 @@ items:
       name: test
       app: nginx
     annotations:
-      config.kubernetes.io/index: "0"
-      config.kubernetes.io/path: config/test_service.yaml
+      config.kubernetes.io/index: '0'
+      config.kubernetes.io/path: 'config/test_service.yaml'
   spec:
     selector:
       name: test
@@ -185,8 +185,8 @@ items:
       name: test
       app: nginx
     annotations:
-      config.kubernetes.io/index: "0"
-      config.kubernetes.io/path: config/test_deployment.yaml
+      config.kubernetes.io/index: '0'
+      config.kubernetes.io/path: 'config/test_deployment.yaml'
   spec:
     replicas: 11
     selector:
@@ -216,8 +216,8 @@ items:
       name: test
       app: nginx
     annotations:
-      config.kubernetes.io/index: "0"
-      config.kubernetes.io/path: config/test_service.yaml
+      config.kubernetes.io/index: '0'
+      config.kubernetes.io/path: 'config/test_service.yaml'
   spec:
     selector:
       name: test

kyaml/kio/filters/fmtr.go

@@ -90,7 +90,6 @@ type formatter struct {
 
 // fmtNode recursively formats the Document Contents.
 func (f *formatter) fmtNode(n *yaml.Node, path string) error {
-	n.Style = 0
 	// sort the order of mapping fields
 	if n.Kind == yaml.MappingNode {
 		sort.Sort(sortedMapContents(*n))

kyaml/kio/filters/fmtr_test.go

@@ -17,6 +17,35 @@ import (
 	"sigs.k8s.io/kustomize/kyaml/kio/filters/testyaml"
 )
 
+func TestFormatInput_Style(t *testing.T) {
+	y := `
+apiVersion: v1
+kind: Foo
+metadata:
+  name: foo
+spec:
+  notBoolean: "true"
+  notBoolean2: "on"
+  isBoolean: on
+  isBoolean2: true
+`
+
+	expected := `apiVersion: v1
+kind: Foo
+metadata:
+  name: foo
+spec:
+  isBoolean: on
+  isBoolean2: true
+  notBoolean: "true"
+  notBoolean2: "on"
+`
+
+	s, err := FormatInput(strings.NewReader(y))
+	assert.NoError(t, err)
+	assert.Equal(t, expected, s.String())
+}
+
 // TestFormatInput_configMap verifies a ConfigMap yaml is formatted correctly
 func TestFormatInput_configMap(t *testing.T) {
 	y := `
@@ -229,7 +258,7 @@ webhooks:
     - CONNECT
     - CREATE
     - UPDATE # this list is indented by 2
-    scope: Namespaced
+    scope: "Namespaced"
   timeoutSeconds: 1
 `
 	s, err := FormatInput(strings.NewReader(y))
@@ -467,7 +496,7 @@ func TestFormatFileOrDirectory_YamlExtFileWithJson(t *testing.T) {
 	// check the result is formatted as yaml
 	b, err := ioutil.ReadFile(f.Name())
 	assert.NoError(t, err)
-	assert.Equal(t, string(testyaml.FormattedYaml1), string(b))
+	assert.Equal(t, string(testyaml.FormattedJSON1), string(b))
 }
 
 // TestFormatFileOrDirectory_partialKubernetesYamlFile verifies that if a yaml file contains both

kyaml/kio/filters/testyaml/testyaml.go

@@ -55,3 +55,7 @@ status2:
   - 1
   - 2
 `)
+
+var FormattedJSON1 = []byte(`{"apiVersion": "example.com/v1beta1", "kind": "MyType", "spec": "a", "status": {"conditions": [
+      3, 1, 2]}}
+`)

kyaml/kio/pkgio_writer_test.go

@@ -68,13 +68,13 @@ func TestLocalPackageWriter_Write_keepReaderAnnotations(t *testing.T) {
 metadata:
   annotations:
     config.kubernetes.io/index: 0
-    config.kubernetes.io/path: a/b/a_test.yaml
+    config.kubernetes.io/path: "a/b/a_test.yaml"
 ---
 c: d # second
 metadata:
   annotations:
     config.kubernetes.io/index: 1
-    config.kubernetes.io/path: a/b/a_test.yaml
+    config.kubernetes.io/path: "a/b/a_test.yaml"
 `, string(b))
 
 	b, err = ioutil.ReadFile(filepath.Join(d, "a", "b", "b_test.yaml"))
@@ -89,7 +89,7 @@ g:
 metadata:
   annotations:
     config.kubernetes.io/index: 0
-    config.kubernetes.io/path: a/b/b_test.yaml
+    config.kubernetes.io/path: "a/b/b_test.yaml"
 `, string(b))
 }
 

kyaml/yaml/fns.go

@@ -446,14 +446,18 @@ type FieldSetter struct {
 	// value on the ScalarNode.
 	Name string `yaml:"name,omitempty"`
 
-	// YNode is the value to set.
+	// Value is the value to set.
 	// Optional if Kind is set.
 	Value *RNode `yaml:"value,omitempty"`
 
 	StringValue string `yaml:"stringValue,omitempty"`
+
+	// OverrideStyle can be set to override the style of the existing node
+	// when setting it.  Otherwise, if an existing node is found, the style is
+	// retained.
+	OverrideStyle bool `yaml:"overrideStyle,omitempty"`
 }
 
-// FieldSetter returns an GrepFilter that sets the named field to the given value.
 func (s FieldSetter) Filter(rn *RNode) (*RNode, error) {
 	if s.StringValue != "" && s.Value == nil {
 		s.Value = NewScalarRNode(s.StringValue)
@@ -463,6 +467,12 @@ func (s FieldSetter) Filter(rn *RNode) (*RNode, error) {
 		if err := ErrorIfInvalid(rn, yaml.ScalarNode); err != nil {
 			return rn, err
 		}
+		// only apply the style if there is not an existing style
+		// or we want to override it
+		if !s.OverrideStyle || s.Value.YNode().Style == 0 {
+			// keep the original style if it exists
+			s.Value.YNode().Style = rn.YNode().Style
+		}
 		rn.SetYNode(s.Value.YNode())
 		return rn, nil
 	}
@@ -477,6 +487,12 @@ func (s FieldSetter) Filter(rn *RNode) (*RNode, error) {
 		return nil, err
 	}
 	if field != nil {
+		// only apply the style if there is not an existing style
+		// or we want to override it
+		if !s.OverrideStyle || field.YNode().Style == 0 {
+			// keep the original style if it exists
+			s.Value.YNode().Style = field.YNode().Style
+		}
 		// need to def ref the Node since field is ephemeral
 		field.SetYNode(s.Value.YNode())
 		return field, nil

kyaml/yaml/fns_test.go

@@ -646,6 +646,25 @@ metadata:
 `, assertNoErrorString(t)(r0.String()))
 }
 
+func TestUpdateAnnotation_Fn(t *testing.T) {
+	// create metadata.annotations field
+	r0 := assertNoError(t)(Parse(`apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    a.b.c: "h.i.j"
+`))
+
+	rn := assertNoError(t)(r0.Pipe(SetAnnotation("a.b.c", "d.e.f")))
+	assert.Equal(t, "\"d.e.f\"\n", assertNoErrorString(t)(rn.String()))
+	assert.Equal(t, `apiVersion: apps/v1
+kind: Deployment
+metadata:
+  annotations:
+    a.b.c: "d.e.f"
+`, assertNoErrorString(t)(r0.String()))
+}
+
 func TestRNode_GetMeta(t *testing.T) {
 	s := `apiVersion: v1/apps
 kind: Deployment

kyaml/yaml/merge2/element_test.go

@@ -23,8 +23,7 @@ kind: Deployment
 items:
 - name: foo
   image: foo:v1
-  command:
-  - run.sh
+  command: ['run.sh']
 `,
 	},
 
@@ -47,8 +46,7 @@ kind: Deployment
 items:
 - name: foo
   image: foo:v1
-  command:
-  - run.sh
+  command: ['run.sh']
 `,
 	},
 
@@ -62,15 +60,14 @@ items:
 `,
 		`
 kind: Deployment
-items: {}
+items: []
 `,
 		`
 kind: Deployment
 items:
 - name: foo
   image: foo:v1
-  command:
-  - run.sh
+  command: ['run.sh']
 `,
 	},
 
@@ -90,8 +87,7 @@ kind: Deployment
 items:
 - name: foo
   image: foo:v1
-  command:
-  - run.sh
+  command: ['run.sh']
 `,
 	},
 
@@ -118,8 +114,7 @@ items:
   image: foo:v1
 - name: bar
   image: bar:v1
-  command:
-  - run2.sh
+  command: ['run2.sh']
 `,
 	},
 
@@ -146,8 +141,7 @@ items:
   image: foo:v1
 - name: bar
   image: bar:v1
-  command:
-  - run2.sh
+  command: ['run2.sh']
 `,
 	},
 
@@ -174,8 +168,7 @@ items:
   image: foo:v1
 - name: bar
   image: bar:v1
-  command:
-  - run2.sh
+  command: ['run2.sh']
 `,
 	},
 
@@ -205,8 +198,7 @@ items:
   image: foo:v1
 - name: bar
   image: bar:v1
-  command:
-  - run2.sh
+  command: ['run2.sh']
 `,
 	},
 
@@ -225,8 +217,7 @@ items:
   image: foo:v1
 - name: bar
   image: bar:v1
-  command:
-  - run2.sh
+  command: ['run2.sh']
 `,
 		`
 kind: Deployment
@@ -235,8 +226,7 @@ items:
   image: foo:v1
 - name: bar
   image: bar:v1
-  command:
-  - run2.sh
+  command: ['run2.sh']
 `,
 	},
 
@@ -255,8 +245,7 @@ items:
   image: foo:v1
 - name: bar
   image: bar:v1
-  command:
-  - run2.sh
+  command: ['run2.sh']
 `,
 		`
 kind: Deployment

kyaml/yaml/merge2/merge2.go

@@ -43,6 +43,9 @@ func (m Merger) VisitMap(nodes walk.Sources) (*yaml.RNode, error) {
 	if err := m.SetComments(nodes); err != nil {
 		return nil, err
 	}
+	if err := m.SetStyle(nodes); err != nil {
+		return nil, err
+	}
 	if yaml.IsEmpty(nodes.Dest()) {
 		// Add
 		return nodes.Origin(), nil
@@ -59,6 +62,9 @@ func (m Merger) VisitScalar(nodes walk.Sources) (*yaml.RNode, error) {
 	if err := m.SetComments(nodes); err != nil {
 		return nil, err
 	}
+	if err := m.SetStyle(nodes); err != nil {
+		return nil, err
+	}
 	// Override value
 	if nodes.Origin() != nil {
 		return nodes.Origin(), nil
@@ -71,6 +77,9 @@ func (m Merger) VisitList(nodes walk.Sources, kind walk.ListKind) (*yaml.RNode,
 	if err := m.SetComments(nodes); err != nil {
 		return nil, err
 	}
+	if err := m.SetStyle(nodes); err != nil {
+		return nil, err
+	}
 	if kind == walk.NonAssociateList {
 		// Override value
 		if nodes.Origin() != nil {
@@ -92,6 +101,25 @@ func (m Merger) VisitList(nodes walk.Sources, kind walk.ListKind) (*yaml.RNode,
 	return nodes.Dest(), nil
 }
 
+func (m Merger) SetStyle(sources walk.Sources) error {
+	source := sources.Origin()
+	dest := sources.Dest()
+	if dest == nil || dest.YNode() == nil || source == nil || source.YNode() == nil {
+		// avoid panic
+		return nil
+	}
+
+	// copy the style from the source.
+	// special case: if the dest was an empty map or seq, then it probably had
+	// folded style applied, but we actually want to keep the style of the origin
+	// in this case (even if it was the default).  otherwise the merged elements
+	// will get folded even though this probably isn't what is desired.
+	if dest.YNode().Kind != yaml.ScalarNode && len(dest.YNode().Content) == 0 {
+		dest.YNode().Style = source.YNode().Style
+	}
+	return nil
+}
+
 // SetComments copies the dest comments to the source comments if they are present
 // on the source.
 func (m Merger) SetComments(sources walk.Sources) error {

kyaml/yaml/merge2/merge2_old_test.go

@@ -83,10 +83,7 @@ spec:
       containers:
       - name: nginx
         image: nginx:1.7.9
-        args:
-        - c
-        - a
-        - b
+        args: ['c', 'a', 'b']
         env:
         - name: DEMO_GREETING
           value: "Hello from the environment"
@@ -139,10 +136,7 @@ spec:
       containers:
       - name: nginx
         image: nginx:1.7.9
-        args:
-        - c
-        - a
-        - b
+        args: ['c', 'a', 'b']
         env:
         - name: DEMO_GREETING
           value: "Hello from the environment"
@@ -208,10 +202,7 @@ spec:
       containers:
       - name: nginx
         image: nginx:1.7.9
-        args:
-        - c
-        - a
-        - b
+        args: ['c', 'a', 'b']
         env:
         - name: DEMO_GREETING
           value: "Hello from the environment"
@@ -276,10 +267,7 @@ spec:
       containers:
       - name: nginx
         image: nginx:1.7.9
-        args:
-        - c
-        - a
-        - b
+        args: ['c', 'a', 'b']
         env:
         - name: DEMO_GREETING
           value: "New Demo Greeting"
@@ -343,10 +331,7 @@ spec:
       containers:
       - name: nginx
         image: nginx:1.7.9
-        args:
-        - e
-        - d
-        - f
+        args: ['e', 'd', 'f']
         env:
         - name: DEMO_GREETING
           value: "Hello from the environment"

kyaml/yaml/merge3/element_test.go

@@ -174,8 +174,7 @@ kind: Deployment
 containers:
 - name: foo
   image: foo:bar
-  command:
-  - run2.sh
+  command: ['run2.sh']
 `, nil},
 
 	//
@@ -206,8 +205,7 @@ kind: Deployment
 containers:
 - name: foo
   image: foo:bar
-  command:
-  - run2.sh
+  command: ['run2.sh']
 `, nil},
 
 	//
@@ -239,8 +237,7 @@ kind: Deployment
 containers:
 - name: foo
   image: foo:bar
-  command:
-  - run2.sh
+  command: ['run2.sh']
 `, nil},
 
 	//
@@ -367,8 +364,7 @@ kind: Deployment
 containers:
 - name: foo
   image: foo:bar
-  command:
-  - run.sh
+  command: ['run.sh']
 `, nil},
 
 	//

kyaml/yaml/types.go

@@ -146,9 +146,6 @@ func NewListRNode(values ...string) *RNode {
 
 // NewRNode returns a new RNode pointer containing the provided Node.
 func NewRNode(value *yaml.Node) *RNode {
-	if value != nil {
-		value.Style = 0
-	}
 	return &RNode{value: value}
 }
 
@@ -389,7 +386,7 @@ const (
 	Flow = "Flow"
 )
 
-// String returns a string valuer for a Node, applying the supplied formatting options
+// String returns a string value for a Node, applying the supplied formatting options
 func String(node *yaml.Node, opts ...string) (string, error) {
 	if node == nil {
 		return "", nil