Merge pull request #6842 from AndiDog/object-order

fix(template): process manifests in file path order, then in order found in each file (before sorting manifests)

cmd/helm/helm_test.go

@@ -47,24 +47,26 @@ func init() {
 func runTestCmd(t *testing.T, tests []cmdTestCase) {
 	t.Helper()
 	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			defer resetEnv()()
-
-			storage := storageFixture()
-			for _, rel := range tt.rels {
-				if err := storage.Create(rel); err != nil {
-					t.Fatal(err)
+		for i := 0; i <= tt.repeat; i++ {
+			t.Run(tt.name, func(t *testing.T) {
+				defer resetEnv()()
+
+				storage := storageFixture()
+				for _, rel := range tt.rels {
+					if err := storage.Create(rel); err != nil {
+						t.Fatal(err)
+					}
 				}
-			}
-			t.Log("running cmd: ", tt.cmd)
-			_, out, err := executeActionCommandC(storage, tt.cmd)
-			if (err != nil) != tt.wantError {
-				t.Errorf("expected error, got '%v'", err)
-			}
-			if tt.golden != "" {
-				test.AssertGoldenString(t, out, tt.golden)
-			}
-		})
+				t.Logf("running cmd (attempt %d): %s", i+1, tt.cmd)
+				_, out, err := executeActionCommandC(storage, tt.cmd)
+				if (err != nil) != tt.wantError {
+					t.Errorf("expected error, got '%v'", err)
+				}
+				if tt.golden != "" {
+					test.AssertGoldenString(t, out, tt.golden)
+				}
+			})
+		}
 	}
 }
 
@@ -124,6 +126,9 @@ type cmdTestCase struct {
 	wantError bool
 	// Rels are the available releases at the start of the test.
 	rels []*release.Release
+	// Number of repeats (in case a feature was previously flaky and the test checks
+	// it's now stably producing identical results). 0 means test is run exactly once.
+	repeat int
 }
 
 func executeActionCommand(cmd string) (*cobra.Command, string, error) {

cmd/helm/template_test.go

@@ -84,6 +84,14 @@ func TestTemplateCmd(t *testing.T) {
 			cmd:    fmt.Sprintf("template '%s' --include-crds", chartPath),
 			golden: "output/template-with-crds.txt",
 		},
+		{
+			name:   "sorted output of manifests (order of filenames, then order of objects within each YAML file)",
+			cmd:    fmt.Sprintf("template '%s'", "testdata/testcharts/object-order"),
+			golden: "output/object-order.txt",
+			// Helm previously used random file order. Repeat the test so we
+			// don't accidentally get the expected result.
+			repeat: 10,
+		},
 	}
 	runTestCmd(t, tests)
 }

cmd/helm/testdata/output/object-order.txt

@@ -0,0 +1,191 @@
+---
+# Source: object-order/templates/01-a.yml
+# 1
+kind: NetworkPolicy
+apiVersion: networking.k8s.io/v1
+metadata:
+  name: first
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+---
+# Source: object-order/templates/01-a.yml
+# 2
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: second
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+---
+# Source: object-order/templates/01-a.yml
+# 3
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: third
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+---
+# Source: object-order/templates/02-b.yml
+# 5
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: fifth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+---
+# Source: object-order/templates/02-b.yml
+# 7
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: seventh
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+---
+# Source: object-order/templates/02-b.yml
+# 8
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: eighth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+---
+# Source: object-order/templates/02-b.yml
+# 9
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: ninth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+---
+# Source: object-order/templates/02-b.yml
+# 10
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: tenth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+---
+# Source: object-order/templates/02-b.yml
+# 11
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: eleventh
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+---
+# Source: object-order/templates/02-b.yml
+# 12
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: twelfth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+---
+# Source: object-order/templates/02-b.yml
+# 13
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: thirteenth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+---
+# Source: object-order/templates/02-b.yml
+# 14
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: fourteenth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+---
+# Source: object-order/templates/02-b.yml
+# 15 (11th object within 02-b.yml, in order to test `SplitManifests` which assigns `manifest-10`
+# to this object which should then come *after* `manifest-9`)
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: fifteenth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+---
+# Source: object-order/templates/01-a.yml
+# 4 (Deployment should come after all NetworkPolicy manifests, since 'helm template' outputs in install order)
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: fourth
+spec:
+  selector:
+    matchLabels:
+      pod: fourth
+  replicas: 1
+  template:
+    metadata:
+      labels:
+        pod: fourth
+    spec:
+      containers:
+        - name: hello-world
+          image: gcr.io/google-samples/node-hello:1.0
+---
+# Source: object-order/templates/02-b.yml
+# 6 (implementation detail: currently, 'helm template' outputs hook manifests last; and yes, NetworkPolicy won't make a reasonable hook, this is just a dummy unit test manifest)
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  annotations:
+    "helm.sh/hook": pre-install
+  name: sixth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress

cmd/helm/testdata/testcharts/object-order/Chart.yaml

@@ -0,0 +1,5 @@
+apiVersion: v2
+name: object-order
+description: Test ordering of manifests in output
+type: application
+version: 0.1.0

cmd/helm/testdata/testcharts/object-order/templates/01-a.yml

@@ -0,0 +1,57 @@
+# 1
+kind: NetworkPolicy
+apiVersion: networking.k8s.io/v1
+metadata:
+  name: first
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+
+---
+
+# 2
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: second
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+
+---
+
+# 3
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: third
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+
+---
+
+# 4 (Deployment should come after all NetworkPolicy manifests, since 'helm template' outputs in install order)
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: fourth
+spec:
+  selector:
+    matchLabels:
+      pod: fourth
+  replicas: 1
+  template:
+    metadata:
+      labels:
+        pod: fourth
+    spec:
+      containers:
+        - name: hello-world
+          image: gcr.io/google-samples/node-hello:1.0

cmd/helm/testdata/testcharts/object-order/templates/02-b.yml

@@ -0,0 +1,143 @@
+# 5
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: fifth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+
+---
+
+# 6 (implementation detail: currently, 'helm template' outputs hook manifests last; and yes, NetworkPolicy won't make a reasonable hook, this is just a dummy unit test manifest)
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  annotations:
+    "helm.sh/hook": pre-install
+  name: sixth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+
+---
+
+# 7
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: seventh
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+
+---
+
+# 8
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: eighth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+
+---
+
+# 9
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: ninth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+
+---
+
+# 10
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: tenth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+
+---
+
+# 11
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: eleventh
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+
+---
+
+# 12
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: twelfth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+
+---
+
+# 13
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: thirteenth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+
+---
+
+# 14
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: fourteenth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress
+
+---
+
+# 15 (11th object within 02-b.yml, in order to test `SplitManifests` which assigns `manifest-10`
+# to this object which should then come *after* `manifest-9`)
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: fifteenth
+spec:
+  podSelector: {}
+  policyTypes:
+    - Egress
+    - Ingress

pkg/releaseutil/kind_sorter.go

@@ -101,10 +101,10 @@ var UninstallOrder KindSortOrder = []string{
 
 // sortByKind does an in-place sort of manifests by Kind.
 //
-// Results are sorted by 'ordering'
+// Results are sorted by 'ordering', keeping order of items with equal kind/priority
 func sortByKind(manifests []Manifest, ordering KindSortOrder) []Manifest {
 	ks := newKindSorter(manifests, ordering)
-	sort.Sort(ks)
+	sort.Stable(ks)
 	return ks.manifests
 }
 
@@ -134,13 +134,11 @@ func (k *kindSorter) Less(i, j int) bool {
 	b := k.manifests[j]
 	first, aok := k.ordering[a.Head.Kind]
 	second, bok := k.ordering[b.Head.Kind]
-	// if same kind (including unknown) sub sort alphanumeric
 	if first == second {
 		// if both are unknown and of different kind sort by kind alphabetically
 		if !aok && !bok && a.Head.Kind != b.Head.Kind {
 			return a.Head.Kind < b.Head.Kind
 		}
-		return a.Name < b.Name
 	}
 	// unknown kind is last
 	if !aok {
@@ -149,6 +147,6 @@ func (k *kindSorter) Less(i, j int) bool {
 	if !bok {
 		return true
 	}
-	// sort different kinds
+	// sort different kinds, keep original order if same priority
 	return first < second
 }

pkg/releaseutil/kind_sorter_test.go

@@ -185,8 +185,8 @@ func TestKindSorter(t *testing.T) {
 	}
 }
 
-// TestKindSorterSubSort verifies manifests of same kind are also sorted alphanumeric
-func TestKindSorterSubSort(t *testing.T) {
+// TestKindSorterKeepOriginalOrder verifies manifests of same kind are kept in original order
+func TestKindSorterKeepOriginalOrder(t *testing.T) {
 	manifests := []Manifest{
 		{
 			Name: "a",
@@ -230,8 +230,8 @@ func TestKindSorterSubSort(t *testing.T) {
 		order       KindSortOrder
 		expected    string
 	}{
-		// expectation is sorted by kind (unknown is last) and then sub sorted alphabetically within each group
-		{"cm,clusterRole,clusterRoleBinding,Unknown,Unknown2", InstallOrder, "01Aa!zu1u2t3"},
+		// expectation is sorted by kind (unknown is last) and within each group of same kind, the order is kept
+		{"cm,clusterRole,clusterRoleBinding,Unknown,Unknown2", InstallOrder, "01aAz!u2u1t3"},
 	} {
 		var buf bytes.Buffer
 		t.Run(test.description, func(t *testing.T) {

pkg/releaseutil/manifest.go

@@ -19,6 +19,7 @@ package releaseutil
 import (
 	"fmt"
 	"regexp"
+	"strconv"
 	"strings"
 )
 
@@ -37,8 +38,9 @@ var sep = regexp.MustCompile("(?:^|\\s*\n)---\\s*")
 // SplitManifests takes a string of manifest and returns a map contains individual manifests
 func SplitManifests(bigFile string) map[string]string {
 	// Basically, we're quickly splitting a stream of YAML documents into an
-	// array of YAML docs. In the current implementation, the file name is just
-	// a place holder, and doesn't have any further meaning.
+	// array of YAML docs. The file name is just a place holder, but should be
+	// integer-sortable so that manifests get output in the same order as the
+	// input (see `BySplitManifestsOrder`).
 	tpl := "manifest-%d"
 	res := map[string]string{}
 	// Making sure that any extra whitespace in YAML stream doesn't interfere in splitting documents correctly.
@@ -56,3 +58,15 @@ func SplitManifests(bigFile string) map[string]string {
 	}
 	return res
 }
+
+// BySplitManifestsOrder sorts by in-file manifest order, as provided in function `SplitManifests`
+type BySplitManifestsOrder []string
+
+func (a BySplitManifestsOrder) Len() int { return len(a) }
+func (a BySplitManifestsOrder) Less(i, j int) bool {
+	// Split `manifest-%d`
+	anum, _ := strconv.ParseInt(a[i][len("manifest-"):], 10, 0)
+	bnum, _ := strconv.ParseInt(a[j][len("manifest-"):], 10, 0)
+	return anum < bnum
+}
+func (a BySplitManifestsOrder) Swap(i, j int) { a[i], a[j] = a[j], a[i] }

pkg/releaseutil/manifest_sorter.go

@@ -19,6 +19,7 @@ package releaseutil
 import (
 	"log"
 	"path"
+	"sort"
 	"strconv"
 	"strings"
 
@@ -74,10 +75,17 @@ var events = map[string]release.HookEvent{
 //
 // Files that do not parse into the expected format are simply placed into a map and
 // returned.
-func SortManifests(files map[string]string, apis chartutil.VersionSet, sort KindSortOrder) ([]*release.Hook, []Manifest, error) {
+func SortManifests(files map[string]string, apis chartutil.VersionSet, ordering KindSortOrder) ([]*release.Hook, []Manifest, error) {
 	result := &result{}
 
-	for filePath, c := range files {
+	var sortedFilePaths []string
+	for filePath := range files {
+		sortedFilePaths = append(sortedFilePaths, filePath)
+	}
+	sort.Strings(sortedFilePaths)
+
+	for _, filePath := range sortedFilePaths {
+		content := files[filePath]
 
 		// Skip partials. We could return these as a separate map, but there doesn't
 		// seem to be any need for that at this time.
@@ -85,12 +93,12 @@ func SortManifests(files map[string]string, apis chartutil.VersionSet, sort Kind
 			continue
 		}
 		// Skip empty files and log this.
-		if strings.TrimSpace(c) == "" {
+		if strings.TrimSpace(content) == "" {
 			continue
 		}
 
 		manifestFile := &manifestFile{
-			entries: SplitManifests(c),
+			entries: SplitManifests(content),
 			path:    filePath,
 			apis:    apis,
 		}
@@ -100,7 +108,7 @@ func SortManifests(files map[string]string, apis chartutil.VersionSet, sort Kind
 		}
 	}
 
-	return result.hooks, sortByKind(result.generic, sort), nil
+	return result.hooks, sortByKind(result.generic, ordering), nil
 }
 
 // sort takes a manifestFile object which may contain multiple resource definition
@@ -123,7 +131,16 @@ func SortManifests(files map[string]string, apis chartutil.VersionSet, sort Kind
 // 		annotations:
 // 			helm.sh/hook-delete-policy: hook-succeeded
 func (file *manifestFile) sort(result *result) error {
-	for _, m := range file.entries {
+	// Go through manifests in order found in file (function `SplitManifests` creates integer-sortable keys)
+	var sortedEntryKeys []string
+	for entryKey := range file.entries {
+		sortedEntryKeys = append(sortedEntryKeys, entryKey)
+	}
+	sort.Sort(BySplitManifestsOrder(sortedEntryKeys))
+
+	for _, entryKey := range sortedEntryKeys {
+		m := file.entries[entryKey]
+
 		var entry SimpleHead
 		if err := yaml.Unmarshal([]byte(m), &entry); err != nil {
 			return errors.Wrapf(err, "YAML parse error on %s", file.path)