From 5cb9b0b07b637b3b67b7e3325eed8071716d0141 Mon Sep 17 00:00:00 2001 From: Marcin Owsiany Date: Thu, 6 Feb 2020 09:44:44 +0100 Subject: [PATCH] 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 | 14 ++++---- pkg/engine/renderer/parser.go | 39 ++++++++++++++------- pkg/engine/renderer/parser_test.go | 54 +++++++++++++++++++++++++----- 3 files changed, 80 insertions(+), 27 deletions(-) diff --git a/pkg/engine/renderer/enhancer.go b/pkg/engine/renderer/enhancer.go index e9cc72115..d43e325dd 100644 --- a/pkg/engine/renderer/enhancer.go +++ b/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) } diff --git a/pkg/engine/renderer/parser.go b/pkg/engine/renderer/parser.go index 2d912751b..91502ceac 100644 --- a/pkg/engine/renderer/parser.go +++ b/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 } diff --git a/pkg/engine/renderer/parser_test.go b/pkg/engine/renderer/parser_test.go index 993c6f5e8..698c09117 100644 --- a/pkg/engine/renderer/parser_test.go +++ b/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) + }) + } +}