Refactored YamlToObject. (#1307)

This is to avoid splitting the YAML file "by hand" and instead use
provided libraries.
This also fixes the inconsistent treatment of whitespace-only files.

pkg/engine/renderer/enhancer.go

@@ -33,12 +33,12 @@ func (k *DefaultEnhancer) Apply(templates map[string]string, metadata Metadata)
 	for _, v := range templates {
 		parsed, err := YamlToObject(v)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("parsing YAML failed: %v", err)
 		}
 		for _, obj := range parsed {
 			unstructMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
 			if err != nil {
-				return nil, err
+				return nil, fmt.Errorf("converting to unstructured failed: %v", err)
 			}
 
 			if err = addLabels(unstructMap, metadata); err != nil {
@@ -55,13 +55,13 @@ func (k *DefaultEnhancer) Apply(templates map[string]string, metadata Metadata)
 				return nil, fmt.Errorf("setting controller reference on parsed object: %v", err)
 			}
 
-			// this is pretty important, if we don't convert it back to the original type everything will be Unstructured
-			// we depend on types later on in the processing e.g. when evaluating health
-			// additionally, as we add annotations and labels to all possible paths, this step gets rid of anything
-			// that doesn't belong to the specific object type
+			// This is pretty important, if we don't convert it back to the original type everything will be Unstructured.
+			// We depend on types later on in the processing e.g. when evaluating health.
+			// Additionally, as we add annotations and labels to all possible paths, this step gets rid of anything
+			// that doesn't belong to the specific object type.
 			err = runtime.DefaultUnstructuredConverter.FromUnstructured(objUnstructured.UnstructuredContent(), obj)
 			if err != nil {
-				return nil, err
+				return nil, fmt.Errorf("converting from unstructured failed: %v", err)
 			}
 			objs = append(objs, obj)
 		}

pkg/engine/renderer/parser.go

@@ -2,6 +2,9 @@ package renderer
 
 import (
 	"bytes"
+	"fmt"
+	"io"
+	"io/ioutil"
 	"strings"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -14,28 +17,40 @@ import (
 // If the type is not known in the scheme, it tries to parse it as Unstructured
 // TODO(av) could we use something else than a global scheme here? Should we somehow inject it?
 func YamlToObject(yaml string) (objs []runtime.Object, err error) {
-	sepYamlfiles := strings.Split(yaml, "\n---\n")
-	for _, f := range sepYamlfiles {
-		if f == "\n" || f == "" {
-			// ignore empty cases
-			continue
+	decode := scheme.Codecs.UniversalDeserializer().Decode
+	// There can be any number of chunks in a YAML file, separated with "\n---".
+	// This "decoder" returns one chunk at a time.
+	docDecoder := yamlutil.NewDocumentDecoder(ioutil.NopCloser(strings.NewReader(yaml)))
+	for {
+		// Prepare slice large enough from the start, rather than do the ErrShortBuffer dance,
+		// since we're reading from an in-memory string already anyway.
+		chunk := make([]byte, len(yaml))
+		n, err := docDecoder.Read(chunk)
+		if err != nil {
+			if err == io.EOF {
+				break
+			}
+			return nil, fmt.Errorf("reading from document decoder failed: %v", err)
 		}
+		// Truncate to what was actually read, to not confuse the consumers with NUL bytes.
+		chunk = chunk[:n]
 
-		decode := scheme.Codecs.UniversalDeserializer().Decode
-		obj, _, e := decode([]byte(f), nil, nil)
+		obj, _, e := decode(chunk, nil, nil)
 
 		if e != nil {
 			// if parsing to scheme known types fails, just try to parse into unstructured
 			unstructuredObj := &unstructured.Unstructured{}
-			fileBytes := []byte(f)
-			decoder := yamlutil.NewYAMLOrJSONDecoder(bytes.NewBuffer(fileBytes), len(fileBytes))
+			decoder := yamlutil.NewYAMLOrJSONDecoder(bytes.NewBuffer(chunk), n)
 			if err = decoder.Decode(unstructuredObj); err != nil {
-				return nil, err
+				return nil, fmt.Errorf("decoding chunk %q failed: %v", chunk, err)
+			}
+			// Skip those chunks/documents which (after rendering) consist solely of whitespace or comments.
+			if len(unstructuredObj.UnstructuredContent()) != 0 {
+				objs = append(objs, unstructuredObj)
 			}
-			objs = append(objs, unstructuredObj)
 		} else {
 			objs = append(objs, obj)
 		}
 	}
-	return
+	return objs, nil
 }

pkg/engine/renderer/parser_test.go

@@ -26,9 +26,7 @@ spec:
     matchLabels:
       spark/servicemonitor: true`)
 
-	if err != nil {
-		t.Errorf("Expecting no error but got %s", err)
-	}
+	assert.NoError(t, err)
 	assert.Equal(t, 1, len(objects))
 }
 
@@ -54,11 +52,51 @@ spec:
         - containerPort: 80
         env:
         - name: PARAM_ENV
-          value: 1`)
-
-	if err != nil {
-		t.Errorf("Expecting no error but got %s", err)
-	}
+          value: "1"`)
 
+	assert.NoError(t, err)
 	assert.Equal(t, "Deployment", obj[0].GetObjectKind().GroupVersionKind().Kind)
 }
+
+func TestParseKubernetesObjects_InvalidYAML(t *testing.T) {
+	_, err := YamlToObject(`"`)
+	assert.Error(t, err)
+}
+
+func TestParseKubernetesObjects_MoreThanOne(t *testing.T) {
+	objects, err := YamlToObject(`apiVersion: foo
+kind: Foo
+metadata:
+  name: foo1
+---
+apiVersion: foo
+kind: Foo
+metadata:
+name: foo2`)
+
+	assert.NoError(t, err)
+	assert.Equal(t, 2, len(objects))
+}
+
+func TestParseKubernetesObjects_EmptyListOfObjects(t *testing.T) {
+	tests := []struct {
+		name string
+		yaml string
+	}{
+		{"empty", ""},
+		{"comment", "#comment"},
+		{"empty line", `
+`},
+		{"empty lines", `
+
+`},
+	}
+	for _, test := range tests {
+		test := test
+		t.Run(test.name, func(t *testing.T) {
+			objects, err := YamlToObject(test.yaml)
+			assert.NoError(t, err)
+			assert.Empty(t, objects)
+		})
+	}
+}