Fix: Warning messages from helm template break NS injection (#233)

- Non map type entries in the output of `helm template` are now munged from final generated output; a warning is outputted of what the breaking entry was.

docs/component.md

@@ -75,7 +75,8 @@ component with the following schema:
 
 ### Prometheus Grafana
 
-This [component specification](https://github.com/timfpark/fabrikate-prometheus-grafana)
+This
+[component specification](https://github.com/timfpark/fabrikate-prometheus-grafana)
 generates static manifests for the `grafana` and `prometheus` namespaces and
 then remotely sources two helm charts for prometheus and grafana respectively.
 
@@ -115,9 +116,8 @@ generator: helm
 path: "./tmp/istio-1.1.2/install/kubernetes/helm/istio"
 hooks:
   before-install:
-    - curl -Lv
-      https://github.com/istio/istio/releases/download/1.1.2/istio-1.1.2-linux.tar.gz
-      -o istio.tar.gz
+    - |
+      curl -Lv https://github.com/istio/istio/releases/download/1.1.2/istio-1.1.2-linux.tar.gz -o istio.tar.gz
     - mkdir -p tmp
     - tar xvf istio.tar.gz -C tmp
   after-install:

generators/helm.go

@@ -7,6 +7,7 @@ import (
 	"os/exec"
 	"path"
 	"path/filepath"
+	"reflect"
 	"strings"
 	"sync"
 
@@ -22,36 +23,91 @@ import (
 type HelmGenerator struct {
 }
 
-func addNamespaceToManifests(manifests, namespace string) (namespacedManifests string, err error) {
+type namespaceInjectionResponse struct {
+	namespacedManifest *[]byte
+	err                error
+	warn               *string
+}
+
+// func addNamespaceToManifests(manifests, namespace string) (namespacedManifests string, err error) {
+func addNamespaceToManifests(manifests, namespace string) chan namespaceInjectionResponse {
+	respChan := make(chan namespaceInjectionResponse)
+	syncGroup := sync.WaitGroup{}
+	splitManifest := strings.Split(manifests, "\n---")
+
+	// Wait for all manifests to be iterated over then close the channel
+	syncGroup.Add(len(splitManifest))
+	go func() {
+		syncGroup.Wait()
+		close(respChan)
+	}()
+
+	// Iterate over all manifests, decrementing the wait group for every channel put
+	for _, manifest := range splitManifest {
+		go func(manifest string) {
+			parsedManifest := make(map[interface{}]interface{})
+
+			// Push a warning if unable to unmarshal
+			if err := yaml.Unmarshal([]byte(manifest), &parsedManifest); err != nil {
+				warning := emoji.Sprintf(":question: Unable to unmarshal manifest into type '%s', this is most likely a warning message outputted from `helm template`. Skipping namespace injection of '%s' into manifest: '%s'", reflect.TypeOf(parsedManifest), namespace, manifest)
+				respChan <- namespaceInjectionResponse{warn: &warning}
+				syncGroup.Done()
+				return
+			}
+
+			// strip any empty entries
+			if len(parsedManifest) == 0 {
+				syncGroup.Done()
+				return
+			}
+
+			// Inject the namespace
+			if parsedManifest["metadata"] != nil {
+				metadataMap := parsedManifest["metadata"].(map[interface{}]interface{})
+				if metadataMap["namespace"] == nil {
+					metadataMap["namespace"] = namespace
+				}
+			}
+
+			// Marshal updated manifest and put the response on channel
+			updatedManifest, err := yaml.Marshal(&parsedManifest)
+			if err != nil {
+				respChan <- namespaceInjectionResponse{err: err}
+				syncGroup.Done()
+				return
+			}
+			respChan <- namespaceInjectionResponse{namespacedManifest: &updatedManifest}
+			syncGroup.Done()
+		}(manifest)
+	}
+
+	return respChan
+}
+
+// cleanK8sManifest attempts to remove any invalid entries in k8s yaml.
+// If any entries after being split by "---" are not a map or are empty, they are removed
+func cleanK8sManifest(manifests string) (cleanedManifests string, err error) {
 	splitManifest := strings.Split(manifests, "\n---")
 
 	for _, manifest := range splitManifest {
 		parsedManifest := make(map[interface{}]interface{})
+
+		// Log a warning if unable to unmarshal; skip the entry
 		if err := yaml.Unmarshal([]byte(manifest), &parsedManifest); err != nil {
-			return "", err
+			warning := emoji.Sprintf(":question: Unable to unmarshal manifest into type '%s', this is most likely a warning message outputted from `helm template`.\nRemoving manifest entry: '%s'\nUnmarshal error encountered: '%s'", reflect.TypeOf(parsedManifest), manifest, err)
+			log.Warn(warning)
+			continue
 		}
 
-		// strip any empty entries
+		// Remove empty entries
 		if len(parsedManifest) == 0 {
 			continue
 		}
 
-		if parsedManifest["metadata"] != nil {
-			metadataMap := parsedManifest["metadata"].(map[interface{}]interface{})
-			if metadataMap["namespace"] == nil {
-				metadataMap["namespace"] = namespace
-			}
-		}
-
-		updatedManifest, err := yaml.Marshal(&parsedManifest)
-		if err != nil {
-			return "", err
-		}
-
-		namespacedManifests += fmt.Sprintf("---\n%s\n", updatedManifest)
+		cleanedManifests += fmt.Sprintf("---\n%s\n", manifest)
 	}
 
-	return namespacedManifests, nil
+	return cleanedManifests, err
 }
 
 // makeHelmRepoPath returns the path where the components helm charts are
@@ -121,20 +177,43 @@ func (hg *HelmGenerator) Generate(component *core.Component) (manifest string, e
 	if err != nil {
 		return "", err
 	}
-	log.Infof("Runing `helm template` on template '%s'\n", chartPath)
+	log.Info(emoji.Sprintf(":memo: Running `helm template` on template '%s'", chartPath))
 	output, err := exec.Command("helm", "template", chartPath, "--values", absOverriddenPath, "--name", component.Name, "--namespace", namespace).CombinedOutput()
 	if err != nil {
 		log.Errorf("helm template failed with:\n%s: %s", err, output)
 		return "", err
 	}
-	stringManifests := string(output)
+	// Remove any empty/non-map entries in manifests
+	stringManifests, err := cleanK8sManifest(string(output))
+	if err != nil {
+		return "", err
+	}
 
 	// helm template does not inject namespace unless chart directly provides support for it: https://github.com/helm/helm/issues/3553
 	// some helm templates expect Tiller to inject namespace, so enable Fabrikate component designer to
 	// opt into injecting these namespaces manually.  We should reassess if this is necessary after Helm 3 is released and client side
 	// templating really becomes a first class function in Helm.
 	if component.Config.InjectNamespace && component.Config.Namespace != "" {
-		stringManifests, err = addNamespaceToManifests(stringManifests, component.Config.Namespace)
+		log.Info(emoji.Sprintf(":syringe: Injecting namespace '%s' into manifests for component '%s'", component.Config.Namespace, component.Name))
+		namespacedManifests := ""
+		for resp := range addNamespaceToManifests(stringManifests, component.Config.Namespace) {
+			// If error; return the error immediately
+			if resp.err != nil {
+				log.Error(emoji.Sprintf(":exclamation: Encountered error while injecting namespace '%s' into manifests for component '%s':\n%s", component.Config.Namespace, component.Name, resp.err))
+				return stringManifests, resp.err
+			}
+
+			// If warning; just log the warning
+			if resp.warn != nil {
+				log.Warn(emoji.Sprintf(":question: Encountered warning while injecting namespace '%s' into manifests for component '%s':\n%s", component.Config.Namespace, component.Name, *resp.warn))
+			}
+
+			// Add the manifest if one was returned
+			if resp.namespacedManifest != nil {
+				namespacedManifests += fmt.Sprintf("---\n%s\n", *resp.namespacedManifest)
+			}
+		}
+		stringManifests = namespacedManifests
 	}
 
 	return stringManifests, err

generators/helm_test.go

@@ -0,0 +1,33 @@
+package generators
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestCleanK8sManifest(t *testing.T) {
+	manifest := `
+---
+this should be removed
+---
+this: is a valid map and should stay
+another:
+  entry: in the map
+---
+this should be removed as well
+---
+# This should be removed
+---
+---
+this is another: valid map
+should: not be removed
+---
+# Another to be removed
+`
+	cleaned, err := cleanK8sManifest(manifest)
+	assert.Nil(t, err)
+	entries := strings.Split(cleaned, "\n---")
+	assert.Equal(t, 2, len(entries))
+}