Repository: grafana/tanka
Pull request #166 (2020-01-14 15:02:10)

Support nested lists when discovering manifests
Resolves feedback in https://github.com/grafana/tanka/pull/122

This commit modifies the generation of kubernetes manifests so that we
can support lists of resources, at arbitrary levels of nesting. As an
example, where before the following jsonnet would fail:

```jsonnet
{
  resources: [
    {
      apiVersion: "v1",
      kind: "ConfigMap",
      metadata: {
        name: "config",
      },
    }
  ],
}
```

We will now return a valid configmap resource.

sh0rez: Thanks @lawrencejones @dyson @jackatbancast for your efforts! This already looks really good, found some nits (tests, etc). After these are addressed nothing prevents this anymore to be merged :D (finally)
jackatbancast: @sh0rez I've added support for an indeterminate depth of arrays as suggested, as well as creating a duplicate of ``testDataArray`` with an additional level of depth at the beginning.

As mentioned in my commit messages, I've brought in the ``reflect`` module, to supplement the type switch. Unfortunately it seems that this is needed to support the indeterminate depth, but I've tried to make the implementation as clean as possible.

We should still get around to refactoring some of the code a la @dyson's changes, but for now I think this may be the smallest set of changes I could think of to add the functionality we wanted, with a suitable test.

Let me know if there are any more changes, happy to work on getting this in ASAP.
sh0rez: Thanks so much, this looks very good! I'd suggest a slight refactoring of `tryCoerceList` and `walkJSON` to keep their concerns separate:

```patch
From 045999cb169af3e3e4a690efa04b039e5fad44fc Mon Sep 17 00:00:00 2001
From: sh0rez <me@shorez.de>
Date: Sun, 19 Jan 2020 12:18:13 +0100
Subject: [PATCH] refactor(kubernetes): separate concerns of walkJSON and
 tryCoerceSlice

---
 pkg/kubernetes/reconcile.go | 51 ++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 24 deletions(-)

diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go
index 729fca9..fee55bc 100644
--- a/pkg/kubernetes/reconcile.go
+++ b/pkg/kubernetes/reconcile.go
@@ -71,21 +71,13 @@ func extract(deep interface{}) (map[string]manifest.Manifest, error) {
 	return extracted, nil
 }
 
-// tryCoerceSlice will test that an input is an array or a slice,
-// and then construct a slice of empty interface.
-//
-// This is useful to handle recursion in walkJSON.
-// Types not specified exactly in a type switch (such as highly nested arrays),
-// will not be matched.
-func tryCoerceSlice(input interface{}, path trace) ([]interface{}, error) {
+// tryCoerceSlice attempts to construct a []interface{} from any other kind of
+// array/slice.
+// If the argument is not a list at all, ok will be false
+func tryCoerceSlice(input interface{}, path trace) (list []interface{}, ok bool) {
 	v := reflect.ValueOf(input)
-	v.Kind()
 	if v.Kind() != reflect.Slice && v.Kind() != reflect.Array {
-		return nil, ErrorPrimitiveReached{
-			path:      path.Base(),
-			key:       path.Name(),
-			primitive: input,
-		}
+		return nil, false
 	}
 
 	l := v.Len()
@@ -95,7 +87,7 @@ func tryCoerceSlice(input interface{}, path trace) ([]interface{}, error) {
 		s[i] = v.Index(i).Interface()
 	}
 
-	return s, nil
+	return s, true
 }
 
 // walkJSON recurses into either a map or list, returning a list of all objects that look
@@ -106,24 +98,35 @@ func tryCoerceSlice(input interface{}, path trace) ([]interface{}, error) {
 // walkJSON, and then walkObj/walkList to handle the two different types of collection we
 // support.
 func walkJSON(ptr interface{}, extracted map[string]manifest.Manifest, path trace) error {
+	// check for known types
 	switch v := ptr.(type) {
 	case map[string]interface{}:
 		return walkObj(v, extracted, path)
-	case []interface{}: // Duplicated to handle multiple list types
-		for idx, value := range v {
-			err := walkJSON(value, extracted, append(path, fmt.Sprintf("[%d]", idx)))
-			if err != nil {
-				return err
-			}
+	case []interface{}:
+		return walkList(v, extracted, path)
+	}
+
+	// Lists other than []interface{} need to be handled separately
+	s, ok := tryCoerceSlice(ptr, path)
+	if !ok {
+		// object and list tried, so not a collection.
+		return ErrorPrimitiveReached{
+			path:      path.Base(),
+			key:       path.Name(),
+			primitive: ptr,
 		}
-		return nil
-	default:
-		s, err := tryCoerceSlice(ptr, path)
+	}
+	return walkJSON(s, extracted, path)
+}
+
+func walkList(list []interface{}, extracted map[string]manifest.Manifest, path trace) error {
+	for idx, value := range list {
+		err := walkJSON(value, extracted, append(path, fmt.Sprintf("[%d]", idx)))
 		if err != nil {
 			return err
 		}
-		return walkJSON(s, extracted, path)
 	}
+	return nil
 }
 
 func walkObj(obj objx.Map, extracted map[string]manifest.Manifest, path trace) error {
-- 
2.24.1
```
sh0rez: Also, could `tryCoerceList` be moved below `walkJSON`? That way it resembles the order of use in the file, so it can be read from top to bottom

Patch:
diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go
index b8ecf551..71e7ac01 100644
--- a/pkg/kubernetes/reconcile.go
+++ b/pkg/kubernetes/reconcile.go
@@ -2,6 +2,7 @@ package kubernetes
 
 import (
 	"fmt"
+	"reflect"
 	"regexp"
 	"sort"
 	"strings"
@@ -14,11 +15,13 @@ import (
 	"github.com/grafana/tanka/pkg/spec/v1alpha1"
 )
 
-// Reconcile extracts all valid Kubernetes objects from the raw output of the
-// Jsonnet compiler. A valid object is identified by the presence of `kind` and
-// `apiVersion`.
-// TODO: Check on `metadata.name` as well and assert that they are
-// not only set but also strings.
+// Reconcile extracts kubernetes Manifests from raw evaluated jsonnet <kind>/<name>,
+// provided the manifests match the given regular expressions. It finds each manifest by
+// recursively walking the jsonnet structure.
+//
+// In addition, we sort the manifests to ensure the order is consistent in each
+// show/diff/apply cycle. This isn't necessary, but it does help users by producing
+// consistent diffs.
 func Reconcile(raw map[string]interface{}, spec v1alpha1.Spec, targets []*regexp.Regexp) (state manifest.List, err error) {
 	extracted, err := extract(raw)
 	if err != nil {
@@ -34,7 +37,8 @@ func Reconcile(raw map[string]interface{}, spec v1alpha1.Spec, targets []*regexp
 		out = append(out, m)
 	}
 
-	// optionally filter the working set of objects
+	// If we have any kind-name matchers, we should filter all the manifests by matching
+	// against their <kind>/<name> identifier.
 	if len(targets) > 0 {
 		tmp := funk.Filter(out, func(i interface{}) bool {
 			p := objectspec(i.(manifest.Manifest))
@@ -67,67 +71,93 @@ func extract(deep interface{}) (map[string]manifest.Manifest, error) {
 	return extracted, nil
 }
 
-// walkJSON traverses deeply nested kubernetes manifest and extracts them into a flat []dict.
-func walkJSON(deep interface{}, extracted map[string]manifest.Manifest, path trace) error {
-	// array: walkJSON for each
-	if d, ok := deep.([]map[string]interface{}); ok {
-		for i, j := range d {
-			path := append(path, fmt.Sprintf("[%v]", i))
-			if err := walkJSON(j, extracted, path); err != nil {
-				return err
-			}
-		}
-		return nil
+// walkJSON recurses into either a map or list, returning a list of all objects that look
+// like kubernetes resources. We support resources at an arbitrary level of nesting, and
+// return an error if a node is not walkable.
+//
+// Handling the different types is quite gross, so we split this method into a generic
+// walkJSON, and then walkObj/walkList to handle the two different types of collection we
+// support.
+func walkJSON(ptr interface{}, extracted map[string]manifest.Manifest, path trace) error {
+	// check for known types
+	switch v := ptr.(type) {
+	case map[string]interface{}:
+		return walkObj(v, extracted, path)
+	case []interface{}:
+		return walkList(v, extracted, path)
 	}
 
-	// assert for map[string]interface{} (also aliased objx.Map)
-	if m, ok := deep.(objx.Map); ok {
-		deep = map[string]interface{}(m)
-	}
-	deep, ok := deep.(map[string]interface{})
+	// Lists other than []interface{} need to be handled separately
+	s, ok := tryCoerceSlice(ptr)
 	if !ok {
-		return fmt.Errorf("deep has unexpected type %T @ %s", deep, path)
+		// object and list tried, so not a collection.
+		return ErrorPrimitiveReached{
+			path:      path.Base(),
+			key:       path.Name(),
+			primitive: ptr,
+		}
 	}
+	return walkJSON(s, extracted, path)
+}
 
-	// already flat?
-	r := objx.New(deep)
-
-	if r.Has("apiVersion") && r.Has("kind") {
-		extracted[path.Full()] = deep.(map[string]interface{})
-		return nil
+func walkList(list []interface{}, extracted map[string]manifest.Manifest, path trace) error {
+	for idx, value := range list {
+		err := walkJSON(value, extracted, append(path, fmt.Sprintf("[%d]", idx)))
+		if err != nil {
+			return err
+		}
 	}
+	return nil
+}
 
-	// walk it
-	for key, d := range deep.(map[string]interface{}) {
-		switch {
-		case key == "__ksonnet": // exclude ksonnet object
-			continue
-		case d == nil: // result from false if condition in Jsonnet
-			continue
+func walkObj(obj objx.Map, extracted map[string]manifest.Manifest, path trace) error {
+	obj = obj.Exclude([]string{"__ksonnet"}) // remove our private ksonnet field
+
+	// This looks like a kubernetes manifest, so make one and return it
+	if isKubernetesManifest(obj) {
+		m, err := manifest.NewFromObj(obj)
+		if err != nil {
+			return err.(*manifest.SchemaError).WithName(path.Full())
 		}
 
+		extracted[path.Full()] = m
+		return nil
+	}
+
+	for key, value := range obj {
 		path := append(path, key)
 
-		if _, ok := d.(map[string]interface{}); !ok {
-			return ErrorPrimitiveReached{path.Base(), key, d}
+		if value == nil { // result from false if condition in Jsonnet
+			continue
 		}
-
-		m := objx.New(d)
-		if m.Has("apiVersion") && m.Has("kind") {
-			mf, err := manifest.NewFromObj(m)
-			if err != nil {
-				return err.(*manifest.SchemaError).WithName(path.Full())
-			}
-			extracted[path.Full()] = mf
-		} else {
-			if err := walkJSON(m, extracted, path); err != nil {
-				return err
-			}
+		err := walkJSON(value, extracted, path)
+		if err != nil {
+			return err
 		}
 	}
+
 	return nil
 }
 
+// tryCoerceSlice attempts to construct a []interface{} from any other kind of
+// array/slice.
+// If the argument is not a list at all, ok will be false
+func tryCoerceSlice(input interface{}) ([]interface{}, bool) {
+	v := reflect.ValueOf(input)
+	if v.Kind() != reflect.Slice && v.Kind() != reflect.Array {
+		return nil, false
+	}
+
+	l := v.Len()
+	s := make([]interface{}, l)
+
+	for i := 0; i < l; i++ {
+		s[i] = v.Index(i).Interface()
+	}
+
+	return s, true
+}
+
 type trace []string
 
 func (t trace) Full() string {
@@ -141,6 +171,14 @@ func (t trace) Base() string {
 	return "." + strings.Join(t, ".")
 }
 
+func (t trace) Name() string {
+	if len(t) > 0 {
+		return t[len(t)-1]
+	}
+
+	return ""
+}
+
 // ErrorPrimitiveReached occurs when walkJSON reaches the end of nested dicts without finding a valid Kubernetes manifest
 type ErrorPrimitiveReached struct {
 	path, key string
@@ -148,8 +186,20 @@ type ErrorPrimitiveReached struct {
 }
 
 func (e ErrorPrimitiveReached) Error() string {
-	return fmt.Sprintf("recursion did not resolve in a valid Kubernetes object, "+
-		"because one of `kind` or `apiVersion` is missing in path `%s`."+
-		" Found non-dict value `%s` of type `%T` instead.",
+	return fmt.Sprintf("recursion did not resolve in a valid Kubernetes object. "+
+		" In path `%s` found key `%s` of type `%T` instead.",
 		e.path, e.key, e.primitive)
 }
+
+// isKubernetesManifest attempts to infer whether the given object is a valid kubernetes
+// resource by verifying the presence of apiVersion and kind. These two
+// fields are required for kubernetes to accept any resource.
+//
+// In future, it might be a good idea to allow users to opt their object out of being
+// interpreted as a kubernetes resource, perhaps with a field like `exclude: true`. For
+// now, any object within the jsonnet output that quacks like a kubernetes resource will
+// be provided to the kubernetes API.
+func isKubernetesManifest(obj objx.Map) bool {
+	return obj.Get("apiVersion").IsStr() && obj.Get("apiVersion").Str() != "" &&
+		obj.Get("kind").IsStr() && obj.Get("kind").Str() != ""
+}


Test patch:
diff --git a/pkg/kubernetes/data_test.go b/pkg/kubernetes/data_test.go
index 9026606f..0ab62ea8 100644
--- a/pkg/kubernetes/data_test.go
+++ b/pkg/kubernetes/data_test.go
@@ -220,10 +220,10 @@ func testDataDeep() testData {
 // flattened
 func testDataArray() testData {
 	return testData{
-		deep: append(make([]map[string]interface{}, 0),
+		deep: []map[string]interface{}{
 			testDataDeep().deep.(map[string]interface{}),
 			testDataFlat().deep.(map[string]interface{}),
-		),
+		},
 
 		flat: map[string]manifest.Manifest{
 			".[0].app.web.backend.server.nginx.deployment":    testDataDeep().flat[".app.web.backend.server.nginx.deployment"],
@@ -234,3 +234,24 @@ func testDataArray() testData {
 		},
 	}
 }
+
+// testDataDeepArray is an array of (deeply nested) dicts that should be fully
+// flattened
+func testDataDeepArray() testData {
+	return testData{
+		deep: [][]map[string]interface{}{
+			{
+				testDataDeep().deep.(map[string]interface{}),
+				testDataFlat().deep.(map[string]interface{}),
+			},
+		},
+
+		flat: map[string]manifest.Manifest{
+			".[0].[0].app.web.backend.server.nginx.deployment":    testDataDeep().flat[".app.web.backend.server.nginx.deployment"],
+			".[0].[0].app.web.frontend.nodejs.express.service":    testDataDeep().flat[".app.web.frontend.nodejs.express.service"],
+			".[0].[0].app.web.frontend.nodejs.express.deployment": testDataDeep().flat[".app.web.frontend.nodejs.express.deployment"],
+			".[0].[0].app.namespace":                              testDataDeep().flat[".app.namespace"],
+			".[0].[1]":                                            testDataFlat().flat["."],
+		},
+	}
+}
diff --git a/pkg/kubernetes/reconcile_test.go b/pkg/kubernetes/reconcile_test.go
index d16c13d3..d88c7974 100644
--- a/pkg/kubernetes/reconcile_test.go
+++ b/pkg/kubernetes/reconcile_test.go
@@ -34,6 +34,10 @@ func TestExtract(t *testing.T) {
 			name: "array",
 			data: testDataArray(),
 		},
+		{
+			name: "deep array",
+			data: testDataDeepArray(),
+		},
 		{
 			name: "nil",
 			data: func() testData {