From c9e78c0547f0c15e515edd1cf986a15aae80feb6 Mon Sep 17 00:00:00 2001 From: Lawrence Jones Date: Thu, 28 Nov 2019 12:12:16 +0000 Subject: [PATCH 01/12] Support nested lists when discovering manifests 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: ```json { resources: [ { apiVersion: "v1", kind: "ConfigMap", metadata: { name: "config", }, } ], } ``` We will now return a valid configmap resource. --- pkg/kubernetes/data_test.go | 4 +- pkg/kubernetes/kubernetes.go | 7 - pkg/kubernetes/manifest/manifest.go | 6 + pkg/kubernetes/reconcile.go | 190 +++++++++++++--------------- pkg/kubernetes/reconcile_test.go | 12 +- 5 files changed, 107 insertions(+), 112 deletions(-) diff --git a/pkg/kubernetes/data_test.go b/pkg/kubernetes/data_test.go index 9026606f2..b15bd9c88 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: []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"], diff --git a/pkg/kubernetes/kubernetes.go b/pkg/kubernetes/kubernetes.go index 19d2a64b9..40c5695be 100644 --- a/pkg/kubernetes/kubernetes.go +++ b/pkg/kubernetes/kubernetes.go @@ -128,10 +128,3 @@ func (k *Kubernetes) Diff(state manifest.List, opts DiffOpts) (*string, error) { func (k *Kubernetes) Info() client.Info { return k.info } - -func objectspec(m manifest.Manifest) string { - return fmt.Sprintf("%s/%s", - m.Kind(), - m.Metadata().Name(), - ) -} diff --git a/pkg/kubernetes/manifest/manifest.go b/pkg/kubernetes/manifest/manifest.go index 641944020..736395c4d 100644 --- a/pkg/kubernetes/manifest/manifest.go +++ b/pkg/kubernetes/manifest/manifest.go @@ -3,6 +3,7 @@ package manifest import ( "bytes" "encoding/json" + "fmt" "strings" "github.com/pkg/errors" @@ -72,6 +73,11 @@ func (m Manifest) Kind() string { return m["kind"].(string) } +// KindName returns a / string +func (m Manifest) KindName() string { + return fmt.Sprintf("%s/%s", m.Kind(), m.Metadata().Name()) +} + // APIVersion returns the version of the API this object uses func (m Manifest) APIVersion() string { return m["apiVersion"].(string) diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go index b8ecf5512..04c4ccc88 100644 --- a/pkg/kubernetes/reconcile.go +++ b/pkg/kubernetes/reconcile.go @@ -14,142 +14,132 @@ 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. -func Reconcile(raw map[string]interface{}, spec v1alpha1.Spec, targets []*regexp.Regexp) (state manifest.List, err error) { - extracted, err := extract(raw) +// Reconcile extracts kubernetes Manifests from raw evaluated jsonnet /, +// 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, kindNameMatchers []*regexp.Regexp) (state manifest.List, err error) { + manifests, err := walkJSON(raw) if err != nil { return nil, errors.Wrap(err, "flattening manifests") } - out := make(manifest.List, 0, len(extracted)) - for _, m := range extracted { - if spec.Namespace != "" && !m.Metadata().HasNamespace() { - m.Metadata()["namespace"] = spec.Namespace + // If we don't have a namespace, we want to set it to the default that is configured in + // our kubernetes specification + for _, manifest := range manifests { + if spec.Namespace != "" && !manifest.Metadata().HasNamespace() { + manifest.Metadata()["namespace"] = spec.Namespace } - - out = append(out, m) } - // optionally filter the working set of objects - if len(targets) > 0 { - tmp := funk.Filter(out, func(i interface{}) bool { - p := objectspec(i.(manifest.Manifest)) - for _, t := range targets { - if t.MatchString(p) { + // If we have any kind-name matchers, we should filter all the manifests by matching + // against their / identifier. + if len(kindNameMatchers) > 0 { + manifests = funk.Filter(manifests, func(elem interface{}) bool { + manifest := elem.(manifest.Manifest) + for _, matcher := range kindNameMatchers { + if matcher.MatchString(manifest.KindName()) { return true } } + return false }).([]manifest.Manifest) - out = manifest.List(tmp) } - // Stable output order - sort.SliceStable(out, func(i int, j int) bool { - if out[i].Kind() != out[j].Kind() { - return out[i].Kind() < out[j].Kind() - } - return out[i].Metadata().Name() < out[j].Metadata().Name() + sort.SliceStable(manifests, func(i int, j int) bool { + return manifests[i].KindName() < manifests[j].KindName() }) - return out, nil + return manifests, nil } -func extract(deep interface{}) (map[string]manifest.Manifest, error) { - extracted := make(map[string]manifest.Manifest) - if err := walkJSON(deep, extracted, nil); err != nil { - return nil, err - } - return extracted, nil +type ErrorPrimitiveReached struct { + path, key string + primitive interface{} } -// 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 - } +func (e ErrorPrimitiveReached) Error() string { + return fmt.Sprintf("recursion did not resolve in a valid Kubernetes object. "+ + " In path path `%s` found key `%s` of type `%T` instead.", + e.path, e.key, e.primitive) +} - // 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{}) - if !ok { - return fmt.Errorf("deep has unexpected type %T @ %s", deep, path) +// 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 any leaf nodes f +// +// 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{}, paths ...string) (manifest.List, error) { + switch v := ptr.(type) { + case map[string]interface{}: + return walkObj(v, paths...) + case []interface{}: + return walkList(v, paths...) } - // already flat? - r := objx.New(deep) + key := paths[len(paths)-1] + path := "." + strings.Join(paths[:len(paths)-1], ".") + + return nil, ErrorPrimitiveReached{path: path, key: key, primitive: ptr} +} + +// case d == nil: // result from false if condition in Jsonnet +func walkObj(obj objx.Map, paths ...string) (manifest.List, error) { + obj = obj.Exclude([]string{"__ksonnet"}) // remove our private ksonnet field - if r.Has("apiVersion") && r.Has("kind") { - extracted[path.Full()] = deep.(map[string]interface{}) - return nil + // This looks like a kubernetes manifest, so make one and return it + if isKubernetesManifest(obj) { + m := manifest.Manifest(obj.Value().MSI()) + + return manifest.List{m}, 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 + manifests := manifest.List{} + for key, value := range obj { + if value == nil { // result from false if condition in Jsonnet continue } - - path := append(path, key) - - if _, ok := d.(map[string]interface{}); !ok { - return ErrorPrimitiveReached{path.Base(), key, d} + children, err := walkJSON(value, append(paths, key)...) + if err != nil { + return nil, err } - 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 - } - } + manifests = append(manifests, children...) } - return nil -} - -type trace []string -func (t trace) Full() string { - return "." + strings.Join(t, ".") + return manifests, nil } -func (t trace) Base() string { - if len(t) > 0 { - t = t[:len(t)-1] +func walkList(list []interface{}, paths ...string) (manifest.List, error) { + manifests := manifest.List{} + for idx, value := range list { + children, err := walkJSON(value, append(paths, fmt.Sprintf("%d", idx))...) + if err != nil { + return nil, err + } + + manifests = append(manifests, children...) } - return "." + strings.Join(t, ".") -} -// ErrorPrimitiveReached occurs when walkJSON reaches the end of nested dicts without finding a valid Kubernetes manifest -type ErrorPrimitiveReached struct { - path, key string - primitive interface{} + return manifests, nil } -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.", - 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, kind and metadata.name. These three +// 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() != "" && + obj.Get("metadata.name").IsStr() && obj.Get("metadata.name").Str() != "" } diff --git a/pkg/kubernetes/reconcile_test.go b/pkg/kubernetes/reconcile_test.go index d16c13d34..ad89547a6 100644 --- a/pkg/kubernetes/reconcile_test.go +++ b/pkg/kubernetes/reconcile_test.go @@ -3,11 +3,12 @@ package kubernetes import ( "testing" + "github.com/grafana/tanka/pkg/kubernetes/manifest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestExtract(t *testing.T) { +func TestWalkJSON(t *testing.T) { tests := []struct { name string data testData @@ -47,10 +48,15 @@ func TestExtract(t *testing.T) { for _, c := range tests { t.Run(c.name, func(t *testing.T) { - extracted, err := extract(c.data.deep) + manifests, err := walkJSON(c.data.deep) + + expectedManifests := manifest.List{} + for _, manifest := range c.data.flat { + expectedManifests = append(expectedManifests, manifest) + } require.Equal(t, c.err, err) - assert.EqualValues(t, c.data.flat, extracted) + assert.ElementsMatch(t, expectedManifests, manifests) }) } } From 2ff5d249c87f39efdc8df705401e52440d5ba6b5 Mon Sep 17 00:00:00 2001 From: Jack Date: Fri, 17 Jan 2020 17:16:19 +0000 Subject: [PATCH 02/12] Revert non-functional Changes The branch so far contains many good refactorings, however they conflate with the purpose of the branch. Most of the changes will be included in a further changeset to clean up the implementation. --- pkg/kubernetes/data_test.go | 4 +- pkg/kubernetes/kubernetes.go | 7 ++ pkg/kubernetes/manifest/manifest.go | 6 -- pkg/kubernetes/reconcile.go | 135 +++++++++++++++++----------- pkg/kubernetes/reconcile_test.go | 12 +-- 5 files changed, 97 insertions(+), 67 deletions(-) diff --git a/pkg/kubernetes/data_test.go b/pkg/kubernetes/data_test.go index b15bd9c88..9026606f2 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: []interface{}{ + deep: append(make([]map[string]interface{}, 0), 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"], diff --git a/pkg/kubernetes/kubernetes.go b/pkg/kubernetes/kubernetes.go index 40c5695be..19d2a64b9 100644 --- a/pkg/kubernetes/kubernetes.go +++ b/pkg/kubernetes/kubernetes.go @@ -128,3 +128,10 @@ func (k *Kubernetes) Diff(state manifest.List, opts DiffOpts) (*string, error) { func (k *Kubernetes) Info() client.Info { return k.info } + +func objectspec(m manifest.Manifest) string { + return fmt.Sprintf("%s/%s", + m.Kind(), + m.Metadata().Name(), + ) +} diff --git a/pkg/kubernetes/manifest/manifest.go b/pkg/kubernetes/manifest/manifest.go index 736395c4d..641944020 100644 --- a/pkg/kubernetes/manifest/manifest.go +++ b/pkg/kubernetes/manifest/manifest.go @@ -3,7 +3,6 @@ package manifest import ( "bytes" "encoding/json" - "fmt" "strings" "github.com/pkg/errors" @@ -73,11 +72,6 @@ func (m Manifest) Kind() string { return m["kind"].(string) } -// KindName returns a / string -func (m Manifest) KindName() string { - return fmt.Sprintf("%s/%s", m.Kind(), m.Metadata().Name()) -} - // APIVersion returns the version of the API this object uses func (m Manifest) APIVersion() string { return m["apiVersion"].(string) diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go index 04c4ccc88..f79d89ae0 100644 --- a/pkg/kubernetes/reconcile.go +++ b/pkg/kubernetes/reconcile.go @@ -21,52 +21,55 @@ import ( // 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, kindNameMatchers []*regexp.Regexp) (state manifest.List, err error) { - manifests, err := walkJSON(raw) +func Reconcile(raw map[string]interface{}, spec v1alpha1.Spec, targets []*regexp.Regexp) (state manifest.List, err error) { + extracted, err := extract(raw) if err != nil { return nil, errors.Wrap(err, "flattening manifests") } - // If we don't have a namespace, we want to set it to the default that is configured in - // our kubernetes specification - for _, manifest := range manifests { - if spec.Namespace != "" && !manifest.Metadata().HasNamespace() { - manifest.Metadata()["namespace"] = spec.Namespace + out := make(manifest.List, 0, len(extracted)) + for _, m := range extracted { + if spec.Namespace != "" && !m.Metadata().HasNamespace() { + m.Metadata()["namespace"] = spec.Namespace } + + out = append(out, m) } // If we have any kind-name matchers, we should filter all the manifests by matching // against their / identifier. - if len(kindNameMatchers) > 0 { - manifests = funk.Filter(manifests, func(elem interface{}) bool { - manifest := elem.(manifest.Manifest) - for _, matcher := range kindNameMatchers { - if matcher.MatchString(manifest.KindName()) { + if len(targets) > 0 { + tmp := funk.Filter(out, func(i interface{}) bool { + p := objectspec(i.(manifest.Manifest)) + for _, t := range targets { + if t.MatchString(p) { return true } } - return false }).([]manifest.Manifest) + out = manifest.List(tmp) } - sort.SliceStable(manifests, func(i int, j int) bool { - return manifests[i].KindName() < manifests[j].KindName() + // Stable output order + sort.SliceStable(out, func(i int, j int) bool { + if out[i].Kind() != out[j].Kind() { + return out[i].Kind() < out[j].Kind() + } + return out[i].Metadata().Name() < out[j].Metadata().Name() }) - return manifests, nil + return out, nil } -type ErrorPrimitiveReached struct { - path, key string - primitive interface{} +func extract(deep interface{}) (map[string]manifest.Manifest, error) { + extracted := make(map[string]manifest.Manifest) + if err := walkJSON(deep, extracted, nil); err != nil { + return nil, err + } + return extracted, nil } -func (e ErrorPrimitiveReached) Error() string { - return fmt.Sprintf("recursion did not resolve in a valid Kubernetes object. "+ - " In path path `%s` found key `%s` of type `%T` instead.", - e.path, e.key, e.primitive) -} // 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 @@ -75,59 +78,92 @@ func (e ErrorPrimitiveReached) Error() string { // 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{}, paths ...string) (manifest.List, error) { +func walkJSON(ptr interface{}, extracted map[string]manifest.Manifest, path trace) error { switch v := ptr.(type) { case map[string]interface{}: - return walkObj(v, paths...) - case []interface{}: - return walkList(v, paths...) + return walkObj(v, extracted, path) + case []map[string]interface{}: + return walkList(v, extracted, path) } - key := paths[len(paths)-1] - path := "." + strings.Join(paths[:len(paths)-1], ".") - - return nil, ErrorPrimitiveReached{path: path, key: key, primitive: ptr} + return ErrorPrimitiveReached{ + path: path.Base(), + key: path.Name(), + primitive: ptr, + } } -// case d == nil: // result from false if condition in Jsonnet -func walkObj(obj objx.Map, paths ...string) (manifest.List, error) { +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 := manifest.Manifest(obj.Value().MSI()) + m, err := manifest.NewFromObj(obj) + if err != nil { + return err.(*manifest.SchemaError).WithName(path.Full()) + } - return manifest.List{m}, nil + extracted[path.Full()] = m + return nil } - manifests := manifest.List{} for key, value := range obj { + path := append(path, key) + if value == nil { // result from false if condition in Jsonnet continue } - children, err := walkJSON(value, append(paths, key)...) + err := walkJSON(value, extracted, path) if err != nil { - return nil, err + return err } - - manifests = append(manifests, children...) } - return manifests, nil + return nil } -func walkList(list []interface{}, paths ...string) (manifest.List, error) { - manifests := manifest.List{} +func walkList(list []map[string]interface{}, extracted map[string]manifest.Manifest, path trace) error { for idx, value := range list { - children, err := walkJSON(value, append(paths, fmt.Sprintf("%d", idx))...) + err := walkJSON(value, extracted, append(path, fmt.Sprintf("[%d]", idx))) if err != nil { - return nil, err + return err } + } - manifests = append(manifests, children...) + return nil +} + +type trace []string + +func (t trace) Full() string { + return "." + strings.Join(t, ".") +} + +func (t trace) Base() string { + if len(t) > 0 { + t = t[:len(t)-1] } + return "." + strings.Join(t, ".") +} + +func (t trace) Name() string { + if len(t) > 0 { + return t[len(t)-1] + } + + return "" +} - return manifests, nil +// ErrorPrimitiveReached occurs when walkJSON reaches the end of nested dicts without finding a valid Kubernetes manifest +type ErrorPrimitiveReached struct { + path, key string + primitive interface{} +} + +func (e ErrorPrimitiveReached) Error() string { + return fmt.Sprintf("recursion did not resolve in a valid Kubernetes object. "+ + " In path 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 @@ -140,6 +176,5 @@ func walkList(list []interface{}, paths ...string) (manifest.List, error) { // 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() != "" && - obj.Get("metadata.name").IsStr() && obj.Get("metadata.name").Str() != "" + obj.Get("kind").IsStr() && obj.Get("kind").Str() != "" } diff --git a/pkg/kubernetes/reconcile_test.go b/pkg/kubernetes/reconcile_test.go index ad89547a6..d16c13d34 100644 --- a/pkg/kubernetes/reconcile_test.go +++ b/pkg/kubernetes/reconcile_test.go @@ -3,12 +3,11 @@ package kubernetes import ( "testing" - "github.com/grafana/tanka/pkg/kubernetes/manifest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestWalkJSON(t *testing.T) { +func TestExtract(t *testing.T) { tests := []struct { name string data testData @@ -48,15 +47,10 @@ func TestWalkJSON(t *testing.T) { for _, c := range tests { t.Run(c.name, func(t *testing.T) { - manifests, err := walkJSON(c.data.deep) - - expectedManifests := manifest.List{} - for _, manifest := range c.data.flat { - expectedManifests = append(expectedManifests, manifest) - } + extracted, err := extract(c.data.deep) require.Equal(t, c.err, err) - assert.ElementsMatch(t, expectedManifests, manifests) + assert.EqualValues(t, c.data.flat, extracted) }) } } From e67dc3922c83302a1025e2a368dfb554d95901f4 Mon Sep 17 00:00:00 2001 From: Jack Date: Fri, 17 Jan 2020 17:22:34 +0000 Subject: [PATCH 03/12] Update walkJSON comment The comment for ``walkJSON`` was incomplete. A more complete comment has been added --- pkg/kubernetes/reconcile.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go index f79d89ae0..978abb224 100644 --- a/pkg/kubernetes/reconcile.go +++ b/pkg/kubernetes/reconcile.go @@ -73,7 +73,7 @@ func extract(deep interface{}) (map[string]manifest.Manifest, error) { // 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 any leaf nodes f +// 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 From e7510d02f45d66f4953cd3022cd3ab1a67781c1c Mon Sep 17 00:00:00 2001 From: Jack Date: Fri, 17 Jan 2020 17:29:57 +0000 Subject: [PATCH 04/12] Run gofmt --- pkg/kubernetes/reconcile.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go index 978abb224..3bbbbbffe 100644 --- a/pkg/kubernetes/reconcile.go +++ b/pkg/kubernetes/reconcile.go @@ -70,7 +70,6 @@ func extract(deep interface{}) (map[string]manifest.Manifest, error) { return extracted, 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. @@ -87,8 +86,8 @@ func walkJSON(ptr interface{}, extracted map[string]manifest.Manifest, path trac } return ErrorPrimitiveReached{ - path: path.Base(), - key: path.Name(), + path: path.Base(), + key: path.Name(), primitive: ptr, } } From d41016fa3b7981690bf0a02fc253f12bfe1efe14 Mon Sep 17 00:00:00 2001 From: Jack Date: Fri, 17 Jan 2020 18:10:30 +0000 Subject: [PATCH 05/12] Simplify and fix list branch logic The previous logic resulted in an inability to pass objects as they were typed as ``[]interface{}``, although they were ``[]map[string]interface{}``-able. This fixes that, and takes in all values as ``[]interface{}``, then asserts the type, throwing ``ErrorPrimitiveReached`` if the type is not a ``[]map[string]interface{}``. --- pkg/kubernetes/reconcile.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go index 3bbbbbffe..c92d61c7a 100644 --- a/pkg/kubernetes/reconcile.go +++ b/pkg/kubernetes/reconcile.go @@ -81,7 +81,7 @@ func walkJSON(ptr interface{}, extracted map[string]manifest.Manifest, path trac switch v := ptr.(type) { case map[string]interface{}: return walkObj(v, extracted, path) - case []map[string]interface{}: + case []interface{}: return walkList(v, extracted, path) } @@ -121,9 +121,17 @@ func walkObj(obj objx.Map, extracted map[string]manifest.Manifest, path trace) e return nil } -func walkList(list []map[string]interface{}, extracted map[string]manifest.Manifest, path trace) error { +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))) + v, ok := value.(map[string]interface{}) + if !ok { + return ErrorPrimitiveReached{ + path: path.Base(), + key: path.Name(), + primitive: value, + } + } + err := walkJSON(v, extracted, append(path, fmt.Sprintf("[%d]", idx))) if err != nil { return err } @@ -161,7 +169,7 @@ type ErrorPrimitiveReached struct { func (e ErrorPrimitiveReached) Error() string { return fmt.Sprintf("recursion did not resolve in a valid Kubernetes object. "+ - " In path path `%s` found key `%s` of type `%T` instead.", + " In path `%s` found key `%s` of type `%T` instead.", e.path, e.key, e.primitive) } From ab3a92c6ea0f8697e76ba5ebb77742a15f929311 Mon Sep 17 00:00:00 2001 From: Jack Date: Fri, 17 Jan 2020 18:49:05 +0000 Subject: [PATCH 06/12] Fix tests It's required to have branches for each possible type here. Without both ``[]interface{}`` and ``[]map[string]interface{}`` it fails to render YAML and pass tests. --- pkg/kubernetes/reconcile.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go index c92d61c7a..a02652234 100644 --- a/pkg/kubernetes/reconcile.go +++ b/pkg/kubernetes/reconcile.go @@ -81,8 +81,16 @@ func walkJSON(ptr interface{}, extracted map[string]manifest.Manifest, path trac switch v := ptr.(type) { case map[string]interface{}: return walkObj(v, extracted, path) - case []interface{}: + case []map[string]interface{}: return walkList(v, extracted, path) + case []interface{}: + l := make([]map[string]interface{}, 0, len(v)) + for _, o := range v { + if dict, ok := o.(map[string]interface{}); ok { + l = append(l, dict) + } + } + return walkList(l, extracted, path) } return ErrorPrimitiveReached{ @@ -121,17 +129,9 @@ func walkObj(obj objx.Map, extracted map[string]manifest.Manifest, path trace) e return nil } -func walkList(list []interface{}, extracted map[string]manifest.Manifest, path trace) error { +func walkList(list []map[string]interface{}, extracted map[string]manifest.Manifest, path trace) error { for idx, value := range list { - v, ok := value.(map[string]interface{}) - if !ok { - return ErrorPrimitiveReached{ - path: path.Base(), - key: path.Name(), - primitive: value, - } - } - err := walkJSON(v, extracted, append(path, fmt.Sprintf("[%d]", idx))) + err := walkJSON(value, extracted, append(path, fmt.Sprintf("[%d]", idx))) if err != nil { return err } From c5f0b560f3a6398826c30359e68570000e0b8b55 Mon Sep 17 00:00:00 2001 From: Jack Stephenson Date: Sat, 18 Jan 2020 14:22:07 +0000 Subject: [PATCH 07/12] Update pkg/kubernetes/reconcile.go Co-Authored-By: sh0rez --- pkg/kubernetes/reconcile.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go index a02652234..046c63460 100644 --- a/pkg/kubernetes/reconcile.go +++ b/pkg/kubernetes/reconcile.go @@ -174,7 +174,7 @@ func (e ErrorPrimitiveReached) Error() string { } // isKubernetesManifest attempts to infer whether the given object is a valid kubernetes -// resource by verifying the presence of apiVersion, kind and metadata.name. These three +// 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 From d5ec1521b83628b3bfd1038777bb76415e20086b Mon Sep 17 00:00:00 2001 From: Jack Date: Sun, 19 Jan 2020 01:42:42 +0000 Subject: [PATCH 08/12] Handle recursive lists By removing ``walkList``, and handling the recursion in-place in ``walkJSON``, we now handle multiple list types, rather than just checking for ``[]map[string]interface{}``. --- pkg/kubernetes/reconcile.go | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go index 046c63460..9f93644b0 100644 --- a/pkg/kubernetes/reconcile.go +++ b/pkg/kubernetes/reconcile.go @@ -81,16 +81,22 @@ func walkJSON(ptr interface{}, extracted map[string]manifest.Manifest, path trac 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 + } + } + return nil case []map[string]interface{}: - return walkList(v, extracted, path) - case []interface{}: - l := make([]map[string]interface{}, 0, len(v)) - for _, o := range v { - if dict, ok := o.(map[string]interface{}); ok { - l = append(l, dict) + for idx, value := range v { + err := walkJSON(value, extracted, append(path, fmt.Sprintf("[%d]", idx))) + if err != nil { + return err } } - return walkList(l, extracted, path) + return nil } return ErrorPrimitiveReached{ @@ -129,17 +135,6 @@ func walkObj(obj objx.Map, extracted map[string]manifest.Manifest, path trace) e return nil } -func walkList(list []map[string]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 -} - type trace []string func (t trace) Full() string { From 6003b7e335532d2d4200c123e72d6ebf64103812 Mon Sep 17 00:00:00 2001 From: Jack Date: Sun, 19 Jan 2020 02:27:47 +0000 Subject: [PATCH 09/12] Fix handling of indeterminate depth of arrays This change brings in the ``reflect`` module, which appears to be needed to supplement the lack of generics, or advanced handling in Go's type switches. This is unfortunate as the functionality inside of the ``reflect`` modules often slows down the runtime considerably, however a similar technique is used inside of ``kubectl``, among other CLIs. It should probably be avoided if attempting to serve lots of clients in an available web API, however the slowdown should be relatively minor in typical usage of tanka. Great effort has been taken to keep the changes in the code to as low a level as possible, whilst retaining as familiar structure as possible. --- pkg/kubernetes/data_test.go | 26 ++++++++++++++++-- pkg/kubernetes/reconcile.go | 46 +++++++++++++++++++++++--------- pkg/kubernetes/reconcile_test.go | 4 +++ 3 files changed, 61 insertions(+), 15 deletions(-) diff --git a/pkg/kubernetes/data_test.go b/pkg/kubernetes/data_test.go index 9026606f2..bda6520b6 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,25 @@ 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.go b/pkg/kubernetes/reconcile.go index 9f93644b0..cd5e7c18a 100644 --- a/pkg/kubernetes/reconcile.go +++ b/pkg/kubernetes/reconcile.go @@ -2,6 +2,7 @@ package kubernetes import ( "fmt" + "reflect" "regexp" "sort" "strings" @@ -70,6 +71,33 @@ 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) { + 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, + } + } + + l := v.Len() + s := make([]interface{}, l) + + for i := 0; i Date: Sun, 19 Jan 2020 02:34:44 +0000 Subject: [PATCH 10/12] gofmt --- pkg/kubernetes/data_test.go | 1 - pkg/kubernetes/reconcile.go | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/kubernetes/data_test.go b/pkg/kubernetes/data_test.go index bda6520b6..0ab62ea8d 100644 --- a/pkg/kubernetes/data_test.go +++ b/pkg/kubernetes/data_test.go @@ -235,7 +235,6 @@ func testDataArray() testData { } } - // testDataDeepArray is an array of (deeply nested) dicts that should be fully // flattened func testDataDeepArray() testData { diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go index cd5e7c18a..729fca9ed 100644 --- a/pkg/kubernetes/reconcile.go +++ b/pkg/kubernetes/reconcile.go @@ -71,7 +71,6 @@ 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. // @@ -79,7 +78,8 @@ func extract(deep interface{}) (map[string]manifest.Manifest, error) { // 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) { - v := reflect.ValueOf(input); v.Kind() + v := reflect.ValueOf(input) + v.Kind() if v.Kind() != reflect.Slice && v.Kind() != reflect.Array { return nil, ErrorPrimitiveReached{ path: path.Base(), @@ -91,7 +91,7 @@ func tryCoerceSlice(input interface{}, path trace) ([]interface{}, error) { l := v.Len() s := make([]interface{}, l) - for i := 0; i Date: Sun, 19 Jan 2020 11:18:06 +0000 Subject: [PATCH 11/12] Remove duplicate v.Kind() --- pkg/kubernetes/reconcile.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go index 729fca9ed..822fff1e6 100644 --- a/pkg/kubernetes/reconcile.go +++ b/pkg/kubernetes/reconcile.go @@ -79,7 +79,6 @@ func extract(deep interface{}) (map[string]manifest.Manifest, error) { // will not be matched. func tryCoerceSlice(input interface{}, path trace) ([]interface{}, error) { v := reflect.ValueOf(input) - v.Kind() if v.Kind() != reflect.Slice && v.Kind() != reflect.Array { return nil, ErrorPrimitiveReached{ path: path.Base(), From 1ed14775f77569e62702be1de747bd9cd79bd17f Mon Sep 17 00:00:00 2001 From: Jack Date: Sun, 19 Jan 2020 12:05:22 +0000 Subject: [PATCH 12/12] Apply patch from @sh0rez This returns the error to ``walkJSON``, avoiding conflation with ``tryCoerceSlice``. ``tryCoerceSlice`` is now also below ``walkObj`` and ``walkList``, so that the functions are defined in the order in which they are used. Thanks for the patch @sh0rez --- pkg/kubernetes/reconcile.go | 76 +++++++++++++++++++------------------ 1 file changed, 40 insertions(+), 36 deletions(-) diff --git a/pkg/kubernetes/reconcile.go b/pkg/kubernetes/reconcile.go index 822fff1e6..71e7ac012 100644 --- a/pkg/kubernetes/reconcile.go +++ b/pkg/kubernetes/reconcile.go @@ -71,32 +71,6 @@ 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) { - v := reflect.ValueOf(input) - if v.Kind() != reflect.Slice && v.Kind() != reflect.Array { - return nil, ErrorPrimitiveReached{ - path: path.Base(), - key: path.Name(), - primitive: input, - } - } - - l := v.Len() - s := make([]interface{}, l) - - for i := 0; i < l; i++ { - s[i] = v.Index(i).Interface() - } - - return s, 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. @@ -105,24 +79,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) + 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 { @@ -154,6 +139,25 @@ func walkObj(obj objx.Map, extracted map[string]manifest.Manifest, path trace) e 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 {