Repository: grafana/tanka
Pull request #827 (2023-03-23 16:26:11)

Improved manifest extraction errors
Closes https://github.com/grafana/tanka/issues/817

This is something that comes up quite often in our internal environments 
When a data structure is not a Kubernetes manifest, the error makes it hard to find out why

This adds a reason, and context for why the object we found isn't a Kubernetes object 

In the issue linked above, the error was:
```
>> tk show environments/default
Error: got an error while extracting env `environments/default`: recursion did not resolve in a valid Kubernetes object.  In path `.grafana.deployment` found key `apiVersion` of type `string` instead.
```

It is now:
```
>> tk show environments/default
Error: got an error while extracting env `environments/default`: found invalid Kubernetes object (at .grafana.deployment): attribute "kind" is not a string, it is a float64

apiVersion: apps/v1
kind: 3000
metadata:
    name: grafana
spec:
    selector:
        matchLabels:
            name: grafana
    template:
        metadata:
            labels:
                name: grafana
        spec:
            containers:
                - image: grafana/grafana
                  name: grafana
                  ports:
                    - containerPort: 3000
                      name: ui
```

More verbose, but it actually helps the user find the error

github-actions[bot]: [PR Preview Action](https://github.com/rossjrw/pr-preview-action) v1.3.0
:---:
:rocket: Deployed preview to https://grafana.github.io/tanka/pr-preview/pr-827/
on branch [`gh-pages`](https://github.com/grafana/tanka/tree/gh-pages) at 2023-03-23 16:29 UTC
<!-- Sticky Pull Request Commentpr-preview -->

Patch:
diff --git a/pkg/process/extract.go b/pkg/process/extract.go
index 56da7f5b..a80b1e91 100644
--- a/pkg/process/extract.go
+++ b/pkg/process/extract.go
@@ -3,10 +3,13 @@ package process
 import (
 	"errors"
 	"fmt"
+	"reflect"
 	"sort"
 	"strings"
 
+	"github.com/rs/zerolog/log"
 	"github.com/stretchr/objx"
+	"gopkg.in/yaml.v3"
 
 	"github.com/grafana/tanka/pkg/kubernetes/manifest"
 )
@@ -38,6 +41,7 @@ func walkJSON(ptr interface{}, extracted map[string]manifest.Manifest, path trac
 		return walkList(v, extracted, path)
 	}
 
+	log.Debug().Msgf("recursion ended on key %q of type %T which does not belong to a valid Kubernetes object", path.Name(), ptr)
 	return ErrorPrimitiveReached{
 		path:      path.Base(),
 		key:       path.Name(),
@@ -59,7 +63,8 @@ func walkObj(obj objx.Map, extracted map[string]manifest.Manifest, path trace) e
 	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) {
+	ok, manifestErr := isKubernetesManifest(obj)
+	if ok {
 		m, err := manifest.NewFromObj(obj)
 		var e *manifest.SchemaError
 		if errors.As(err, &e) {
@@ -84,7 +89,7 @@ func walkObj(obj objx.Map, extracted map[string]manifest.Manifest, path trace) e
 		}
 		err := walkJSON(obj[key], extracted, path)
 		if err != nil {
-			return err
+			return err.(ErrorPrimitiveReached).WithContainingObj(obj, manifestErr)
 		}
 	}
 
@@ -114,25 +119,58 @@ func (t trace) Name() string {
 
 // ErrorPrimitiveReached occurs when walkJSON reaches the end of nested dicts without finding a valid Kubernetes manifest
 type ErrorPrimitiveReached struct {
-	path, key string
-	primitive interface{}
+	path, key        string
+	primitive        interface{}
+	containingObj    objx.Map
+	containingObjErr error
+}
+
+func (e ErrorPrimitiveReached) WithContainingObj(obj objx.Map, err error) ErrorPrimitiveReached {
+	if e.containingObj == nil {
+		e.containingObj = obj
+		e.containingObjErr = err
+	}
+	return e
 }
 
 func (e ErrorPrimitiveReached) Error() string {
-	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)
+	errMessage := fmt.Sprintf(`found invalid Kubernetes object (at %s): %s`, e.path, e.containingObjErr)
+
+	container, err := yaml.Marshal(e.containingObj)
+	if err != nil {
+		log.Error().Msgf("failed to marshal invalid Kubernetes object: %s", err)
+	} else {
+		errMessage += "\n\n" + string(container)
+	}
+
+	return errMessage
 }
 
 // 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.
-func isKubernetesManifest(obj objx.Map) bool {
-	if apiVersion := obj.Get("apiVersion"); !apiVersion.IsStr() || apiVersion.Str() == "" {
-		return false
+// The error return value indicates the reason why the object is not a valid
+func isKubernetesManifest(obj objx.Map) (ok bool, err error) {
+	checkAttribute := func(key string) error {
+		v := obj.Get(key)
+		if v.IsNil() {
+			return fmt.Errorf("missing attribute %q", key)
+		}
+		if !v.IsStr() {
+			return fmt.Errorf("attribute %q is not a string, it is a %s", key, reflect.TypeOf(v.Data()))
+		}
+		if v.Str() == "" {
+			return fmt.Errorf("attribute %q is empty", key)
+		}
+		return nil
 	}
-	if kind := obj.Get("kind"); !kind.IsStr() || kind.Str() == "" {
-		return false
+
+	if err := checkAttribute("apiVersion"); err != nil {
+		return false, err
+	}
+	if err := checkAttribute("kind"); err != nil {
+		return false, err
 	}
-	return true
+
+	return true, nil
 }


Test patch:
diff --git a/pkg/process/data_test.go b/pkg/process/data_test.go
index fee3e8a4..ea226570 100644
--- a/pkg/process/data_test.go
+++ b/pkg/process/data_test.go
@@ -52,6 +52,16 @@ func testDataPrimitive() testData {
 	return loadFixture("tdInvalidPrimitive.jsonnet")
 }
 
+// testBadKindType is an invalid manifest, because it has an invalid `kind` value
+func testBadKindType() testData {
+	return loadFixture("tdBadKindType.jsonnet")
+}
+
+// testMissingAttribute is an invalid manifest, because it is missing the `kind`
+func testMissingAttribute() testData {
+	return loadFixture("tdMissingAttribute.jsonnet")
+}
+
 // testDataDeep is super deeply nested on multiple levels
 func testDataDeep() testData {
 	return loadFixture("tdDeep.jsonnet")
diff --git a/pkg/process/extract_test.go b/pkg/process/extract_test.go
index ed70679e..a72aec87 100644
--- a/pkg/process/extract_test.go
+++ b/pkg/process/extract_test.go
@@ -8,9 +8,9 @@ import (
 )
 
 var extractTestCases = []struct {
-	name string
-	data testData
-	err  error
+	name       string
+	data       testData
+	errMessage string
 }{
 	{
 		name: "regular",
@@ -23,7 +23,45 @@ var extractTestCases = []struct {
 	{
 		name: "primitive",
 		data: testDataPrimitive(),
-		err:  ErrorPrimitiveReached{path: ".service", key: "note", primitive: "invalid because apiVersion and kind are missing"},
+		errMessage: `found invalid Kubernetes object (at .service): missing attribute "apiVersion"
+
+note: invalid because apiVersion and kind are missing
+`,
+	},
+	{
+		name: "missing kind",
+		data: testMissingAttribute(),
+		errMessage: `found invalid Kubernetes object (at .service): missing attribute "kind"
+
+apiVersion: v1
+spec:
+    ports:
+        - port: 80
+          protocol: TCP
+          targetPort: 8080
+    selector:
+        app: deep
+`,
+	},
+	{
+		name: "bad kind",
+		data: testBadKindType(),
+		errMessage: `found invalid Kubernetes object (at .deployment): attribute "kind" is not a string, it is a float64
+
+apiVersion: apps/v1
+kind: 3000
+metadata:
+    name: grafana
+spec:
+    replicas: 1
+    template:
+        containers:
+            - image: grafana/grafana
+              name: grafana
+        metadata:
+            labels:
+                app: grafana
+`,
 	},
 	{
 		name: "deep",
@@ -40,7 +78,7 @@ var extractTestCases = []struct {
 			d.Deep.(map[string]interface{})["disabledObject"] = nil
 			return d
 		}(),
-		err: nil, // we expect no error, just the result of testDataRegular
+		errMessage: "", // we expect no error, just the result of testDataRegular
 	},
 }
 
@@ -49,7 +87,13 @@ func TestExtract(t *testing.T) {
 		t.Run(c.name, func(t *testing.T) {
 			extracted, err := Extract(c.data.Deep)
 
-			require.Equal(t, c.err, err)
+			if c.errMessage != "" {
+				require.Error(t, err)
+				assert.Equal(t, c.errMessage, err.Error())
+				return
+			}
+
+			require.NoError(t, err)
 			assert.EqualValues(t, c.data.Flat, extracted)
 		})
 	}
@@ -57,7 +101,7 @@ func TestExtract(t *testing.T) {
 
 func BenchmarkExtract(b *testing.B) {
 	for _, c := range extractTestCases {
-		if c.err != nil {
+		if c.errMessage != "" {
 			continue
 		}
 		b.Run(c.name, func(b *testing.B) {
diff --git a/pkg/process/testdata/tdBadKindType.jsonnet b/pkg/process/testdata/tdBadKindType.jsonnet
new file mode 100644
index 00000000..9d9c7966
--- /dev/null
+++ b/pkg/process/testdata/tdBadKindType.jsonnet
@@ -0,0 +1,12 @@
+local deployment = (import './resources.jsonnet').deployment;
+
+{
+  deep: {
+    deployment: deployment {
+      kind: 3000,
+    },
+  },
+  flat: {
+    '.deployment': deployment,
+  },
+}
diff --git a/pkg/process/testdata/tdMissingAttribute.jsonnet b/pkg/process/testdata/tdMissingAttribute.jsonnet
new file mode 100644
index 00000000..1390380d
--- /dev/null
+++ b/pkg/process/testdata/tdMissingAttribute.jsonnet
@@ -0,0 +1,21 @@
+{
+  deep: {
+    deploy: (import './resources.jsonnet').deployment,
+    service: {
+      // Missing kind
+      apiVersion: 'v1',
+      spec: {
+        selector: {
+          app: 'deep',
+        },
+        ports: [
+          {
+            protocol: 'TCP',
+            port: 80,
+            targetPort: 8080,
+          },
+        ],
+      },
+    },
+  },
+}