Merge pull request #3934 from yhrn/fix-json-sink

Sink: Force JSON encoding for .json files

cmd/config/internal/commands/e2e/e2e_test.go

@@ -87,8 +87,18 @@ metadata:
 			expectedFiles: func(d string) map[string]string {
 				return map[string]string{
 					"deployment.json": `
-{"apiVersion": "apps/v1", "kind": "Deployment", "metadata": {"name": "foo", annotations: {
-      a-string-value: '', a-int-value: '0', a-bool-value: 'false'}}}
+{
+  "apiVersion": "apps/v1",
+  "kind": "Deployment",
+  "metadata": {
+    "annotations": {
+      "a-bool-value": "false",
+      "a-int-value": "0",
+      "a-string-value": ""
+    },
+    "name": "foo"
+  }
+}
 `,
 				}
 			},

kyaml/kio/byteio_writer.go

@@ -6,13 +6,16 @@ package kio
 import (
 	"encoding/json"
 	"io"
-
+	"path/filepath"
 	"sigs.k8s.io/kustomize/kyaml/errors"
 	"sigs.k8s.io/kustomize/kyaml/kio/kioutil"
 	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
 
-// Writer writes ResourceNodes to bytes.
+// ByteWriter writes ResourceNodes to bytes. Generally YAML encoding will be used but in the special
+// case of writing a single, bare yaml.RNode that has a kioutil.PathAnnotation indicating that the
+// target is a JSON file JSON encoding is used. See shouldJSONEncodeSingleBareNode below for more
+// information.
 type ByteWriter struct {
 	// Writer is where ResourceNodes are encoded.
 	Writer io.Writer
@@ -55,8 +58,9 @@ func (w ByteWriter) Write(nodes []*yaml.RNode) error {
 		}
 	}
 
-	encoder := yaml.NewEncoder(w.Writer)
-	defer encoder.Close()
+	// Even though we use the this value further down we must check this before removing annotations
+	jsonEncodeSingleBareNode := w.shouldJSONEncodeSingleBareNode(nodes)
+
 	for i := range nodes {
 		// clean resources by removing annotations set by the Reader
 		if !w.KeepReaderAnnotations {
@@ -81,10 +85,18 @@ func (w ByteWriter) Write(nodes []*yaml.RNode) error {
 		}
 	}
 
+	if jsonEncodeSingleBareNode {
+		encoder := json.NewEncoder(w.Writer)
+		encoder.SetIndent("", "  ")
+		return errors.Wrap(encoder.Encode(nodes[0]))
+	}
+
+	encoder := yaml.NewEncoder(w.Writer)
+	defer encoder.Close()
 	// don't wrap the elements
 	if w.WrappingKind == "" {
 		for i := range nodes {
-			if err := w.encode(encoder, nodes[i].Document()); err != nil {
+			if err := encoder.Encode(nodes[i].Document()); err != nil {
 				return err
 			}
 		}
@@ -118,23 +130,31 @@ func (w ByteWriter) Write(nodes []*yaml.RNode) error {
 	for i := range nodes {
 		items.Content = append(items.Content, nodes[i].YNode())
 	}
-	err := w.encode(encoder, doc)
+	err := encoder.Encode(doc)
 	yaml.UndoSerializationHacksOnNodes(nodes)
 	return err
 }
 
-// encode encodes the input document node to appropriate node format
-func (w ByteWriter) encode(encoder *yaml.Encoder, doc *yaml.Node) error {
-	rNode := &yaml.RNode{}
-	rNode.SetYNode(doc)
-	str, err := rNode.String()
-	if err != nil {
-		return errors.Wrap(err)
+// shouldJSONEncodeSingleBareNode determines if nodes contain a single node that should not be
+// wrapped and has a JSON file extension, which in turn means that the node should be JSON encoded.
+// Note 1: this must be checked before any annotations to avoid losing information about the target
+//         filename extension.
+// Note 2: JSON encoding should only be used for single, unwrapped nodes because multiple unwrapped
+//         nodes cannot be represented in JSON (no multi doc support). Furthermore, the typical use
+//         cases for wrapping nodes would likely not include later writing the whole wrapper to a
+//         .json file, i.e. there is no point risking any edge case information loss e.g. comments
+//         disappearing, that could come from JSON encoding the whole wrapper just to ensure that
+//         one (or all nodes) can be read as JSON.
+func (w ByteWriter) shouldJSONEncodeSingleBareNode(nodes []*yaml.RNode) bool {
+	if w.WrappingKind == "" && len(nodes) == 1 {
+		if path, _, _ := kioutil.GetFileAnnotations(nodes[0]); path != "" {
+			filename := filepath.Base(path)
+			for _, glob := range JSONMatch {
+				if match, _ := filepath.Match(glob, filename); match {
+					return true
+				}
+			}
+		}
 	}
-	if json.Valid([]byte(str)) {
-		je := json.NewEncoder(w.Writer)
-		je.SetIndent("", "  ")
-		return errors.Wrap(je.Encode(rNode))
-	}
-	return encoder.Encode(doc)
-}
+	return false
+}

kyaml/kio/byteio_writer_test.go

@@ -26,7 +26,7 @@ func TestByteWriter(t *testing.T) {
 
 	testCases := []testCase{
 		//
-		//
+		// Test Case
 		//
 		{
 			name: "wrap_resource_list",
@@ -60,7 +60,7 @@ functionConfig:
 		},
 
 		//
-		//
+		// Test Case
 		//
 		{
 			name: "multiple_items",
@@ -187,6 +187,110 @@ c: d # second
 metadata:
   annotations:
     config.kubernetes.io/path: "a/b/a_test.yaml"
+`,
+		},
+
+		//
+		// Test Case
+		//
+		{
+			name: "encode_valid_json",
+			items: []string{
+				`{
+  "a": "a long string that would certainly see a newline introduced by the YAML marshaller abcd123",
+  metadata: {
+    annotations: {
+      config.kubernetes.io/path: test.json
+    }
+  }
+}`,
+			},
+
+			expectedOutput: `{
+  "a": "a long string that would certainly see a newline introduced by the YAML marshaller abcd123",
+  "metadata": {
+    "annotations": {
+      "config.kubernetes.io/path": "test.json"
+    }
+  }
+}`,
+		},
+
+		//
+		// Test Case
+		//
+		{
+			name: "encode_unformatted_valid_json",
+			items: []string{
+				`{ "a": "b", metadata: { annotations: { config.kubernetes.io/path: test.json } } }`,
+			},
+
+			expectedOutput: `{
+  "a": "b",
+  "metadata": {
+    "annotations": {
+      "config.kubernetes.io/path": "test.json"
+    }
+  }
+}`,
+		},
+
+		//
+		// Test Case
+		//
+		{
+			name: "encode_wrapped_json_as_yaml",
+			instance: ByteWriter{
+				Sort:               true,
+				WrappingKind:       ResourceListKind,
+				WrappingAPIVersion: ResourceListAPIVersion,
+			},
+			items: []string{
+				`{
+  "a": "b",
+  "metadata": {
+    "annotations": {
+      "config.kubernetes.io/path": "test.json"
+    }
+  }
+}`,
+			},
+
+			expectedOutput: `apiVersion: config.kubernetes.io/v1alpha1
+kind: ResourceList
+items:
+- {"a": "b", "metadata": {"annotations": {"config.kubernetes.io/path": "test.json"}}}
+`,
+		},
+
+		//
+		// Test Case
+		//
+		{
+			name: "encode_multi_doc_json_as_yaml",
+			items: []string{
+				`{
+  "a": "b",
+  "metadata": {
+    "annotations": {
+      "config.kubernetes.io/path": "test-1.json"
+    }
+  }
+}`,
+				`{
+  "c": "d",
+  "metadata": {
+    "annotations": {
+      "config.kubernetes.io/path": "test-2.json"
+    }
+  }
+}`,
+			},
+
+			expectedOutput: `
+{"a": "b", "metadata": {"annotations": {"config.kubernetes.io/path": "test-1.json"}}}
+---
+{"c": "d", "metadata": {"annotations": {"config.kubernetes.io/path": "test-2.json"}}}
 `,
 		},
 	}

kyaml/kio/filters/fmtr_test.go

@@ -743,8 +743,8 @@ func TestFormatFileOrDirectory_ymlExtFile(t *testing.T) {
 	assert.Equal(t, string(testyaml.FormattedYaml1), string(b))
 }
 
-// TestFormatFileOrDirectory_skipYamlExtFileWithJson verifies that the json content is formatted
-// as yaml
+// TestFormatFileOrDirectory_YamlExtFileWithJson verifies that the JSON compatible flow style
+// YAML content is formatted as such.
 func TestFormatFileOrDirectory_YamlExtFileWithJson(t *testing.T) {
 	// write the unformatted JSON file contents
 	f, err := ioutil.TempFile("", "yamlfmt*.yaml")
@@ -760,7 +760,27 @@ 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.FormattedJSON1), string(b))
+	assert.Equal(t, string(testyaml.FormattedFlowYAML1), string(b))
+}
+
+// TestFormatFileOrDirectory_JsonExtFileWithNotModified verifies that a file with .json extensions
+// and JSON contents won't be modified.
+func TestFormatFileOrDirectory_JsonExtFileWithNotModified(t *testing.T) {
+	// write the unformatted JSON file contents
+	f, err := ioutil.TempFile("", "yamlfmt*.json")
+	assert.NoError(t, err)
+	defer os.Remove(f.Name())
+	err = ioutil.WriteFile(f.Name(), testyaml.UnformattedJSON1, 0600)
+	assert.NoError(t, err)
+
+	// format the file
+	err = FormatFileOrDirectory(f.Name())
+	assert.NoError(t, err)
+
+	// check the result is formatted as yaml
+	b, err := ioutil.ReadFile(f.Name())
+	assert.NoError(t, err)
+	assert.Equal(t, string(testyaml.UnformattedJSON1), string(b))
 }
 
 // TestFormatFileOrDirectory_partialKubernetesYamlFile verifies that if a yaml file contains both

kyaml/kio/filters/testyaml/testyaml.go

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