fix(kubernetes): deterministic sorting (#259)

* Implement sorting by NS and test object sorting.

- the original sort code in Reconcile() didn't take into account
  the case where two objects of the same kind and name might reside in
  different namespace, allowing the sort being non-deterministic at
  times.

- testing of the sort code is now implemented.

There might be some refactor pending, since `kubernetes_test.go` also
contains code testing `Reconcile()`, but this is outside the scope of
this PR.

Signed-off-by: Milan Plzik <[email protected]>

* Implement review comments.

- Refactored `mkobj` to look nicer.
- Added a testcase that tests prioritizing Kind -> Namespace -> Name, in
  that order.

Signed-off-by: Milan Plzik <[email protected]>

Author: mplzik

pkg/kubernetes/reconcile.go

@@ -113,6 +113,11 @@ func Reconcile(raw map[string]interface{}, spec v1alpha1.Spec, targets []*regexp
 			return out[i].Kind() < out[j].Kind()
 		}
 
+		// If namespaces differ, sort by the namespace.
+		if out[i].Metadata().Namespace() != out[j].Metadata().Namespace() {
+			return out[i].Metadata().Namespace() < out[j].Metadata().Namespace()
+		}
+
 		// Otherwise, order the objects by name.
 		return out[i].Metadata().Name() < out[j].Metadata().Name()
 	})

pkg/kubernetes/reconcile_test.go

@@ -1,10 +1,14 @@
 package kubernetes
 
 import (
+	"regexp"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/tanka/pkg/kubernetes/manifest"
+	"github.com/grafana/tanka/pkg/spec/v1alpha1"
 )
 
 func TestExtract(t *testing.T) {
@@ -54,3 +58,130 @@ func TestExtract(t *testing.T) {
 		})
 	}
 }
+
+func mkobj(kind string, name string, ns string) map[string]interface{} {
+	ret := map[string]interface{}{
+		"kind":       kind,
+		"apiVersion": "apiversion",
+		"metadata": map[string]interface{}{
+			"name": name,
+		},
+	}
+	if ns != "" {
+		ret["metadata"].(map[string]interface{})["namespace"] = ns
+	}
+
+	return ret
+}
+
+func TestReconcileSorting(t *testing.T) {
+	tests := []struct {
+		raw     map[string]interface{}
+		targets []*regexp.Regexp
+		state   manifest.List
+		err     error
+	}{
+		{
+			// sorting by kinds in the `kindOrder` list
+			raw: map[string]interface{}{
+				"a": mkobj("Service", "service", "default"),
+				"b": mkobj("Deployment", "deployment", "default"),
+				"c": mkobj("CustomResourceDefinition", "crd", ""),
+			},
+			state: manifest.List{
+				mkobj("CustomResourceDefinition", "crd", ""),
+				mkobj("Service", "service", "default"),
+				mkobj("Deployment", "deployment", "default"),
+			},
+		},
+		{
+			// alphabtical sorting by kinds outside `kindOrder` list
+			raw: map[string]interface{}{
+				"a": mkobj("B", "b", "default"),
+				"b": mkobj("C", "c", "default"),
+				"c": mkobj("A", "a", "default"),
+			},
+			state: manifest.List{
+				mkobj("A", "a", "default"),
+				mkobj("B", "b", "default"),
+				mkobj("C", "c", "default"),
+			},
+		},
+		{
+			// sorting by the namespace if kinds match
+			raw: map[string]interface{}{
+				"a": mkobj("Service", "service", "default2"),
+				"b": mkobj("Service", "service", "default"),
+				"c": mkobj("Service", "service", "default1"),
+			},
+			state: manifest.List{
+				mkobj("Service", "service", "default"),
+				mkobj("Service", "service", "default1"),
+				mkobj("Service", "service", "default2"),
+			},
+		},
+		{
+			// sorting by the names if both kinds and namespaces match
+			raw: map[string]interface{}{
+				"a": mkobj("Service", "service2", "default"),
+				"b": mkobj("Service", "service", "default"),
+				"c": mkobj("Service", "service1", "default"),
+			},
+			state: manifest.List{
+				mkobj("Service", "service", "default"),
+				mkobj("Service", "service1", "default"),
+				mkobj("Service", "service2", "default"),
+			},
+		},
+		{
+			// sorting by the names if both kinds match and there are no namespaces
+			raw: map[string]interface{}{
+				"a": mkobj("CustomResourceDefinition", "crd2", ""),
+				"b": mkobj("CustomResourceDefinition", "crd", ""),
+				"c": mkobj("CustomResourceDefinition", "crd1", ""),
+			},
+			state: manifest.List{
+				mkobj("CustomResourceDefinition", "crd", ""),
+				mkobj("CustomResourceDefinition", "crd1", ""),
+				mkobj("CustomResourceDefinition", "crd2", ""),
+			},
+		},
+		{
+			raw: map[string]interface{}{
+				"a": mkobj("Deployment", "b", "a"),
+				"b": mkobj("ConfigMap", "a", "a"),
+				"c": mkobj("Issuer", "a", "a"),
+				"d": mkobj("Service", "b", "a"),
+				"e": mkobj("Service", "a", "a"),
+				"f": mkobj("Deployment", "a", "a"),
+				"g": mkobj("ConfigMap", "a", "b"),
+				"h": mkobj("Issuer", "b", "a"),
+				"i": mkobj("Service", "b", "b"),
+				"j": mkobj("Deployment", "a", "b"),
+				"k": mkobj("ConfigMap", "b", "a"),
+				"l": mkobj("Issuer", "a", "b"),
+			},
+			state: manifest.List{
+				mkobj("ConfigMap", "a", "a"),
+				mkobj("ConfigMap", "b", "a"),
+				mkobj("ConfigMap", "a", "b"),
+				mkobj("Service", "a", "a"),
+				mkobj("Service", "b", "a"),
+				mkobj("Service", "b", "b"),
+				mkobj("Deployment", "a", "a"),
+				mkobj("Deployment", "b", "a"),
+				mkobj("Deployment", "a", "b"),
+				mkobj("Issuer", "a", "a"),
+				mkobj("Issuer", "b", "a"),
+				mkobj("Issuer", "a", "b"),
+			},
+		},
+	}
+
+	for _, test := range tests {
+		res, err := Reconcile(test.raw, v1alpha1.New().Spec, test.targets)
+
+		require.NoError(t, err)
+		require.Equal(t, test.state, res)
+	}
+}