fix(helm): sort hooks by kind for equal weight

Use the same install order for hooks as for normal resources (non-hooks) for hooks with equal weight.
This makes resource handling more consistent and helps, when there are hook consisting of several resources like e.g. a service account and a job using this service account.

The sort functions are changed from an in place search to an out of place sort to avoid inout parameters.

Closes #7416.

Signed-off-by: Daniel Strobusch <1847260+dastrobu@users.noreply.github.com>

pkg/action/hooks.go

@@ -38,7 +38,8 @@ func (cfg *Configuration) execHook(rl *release.Release, hook release.HookEvent,
 		}
 	}
 
-	sort.Sort(hookByWeight(executingHooks))
+	// hooke are pre-ordered by kind, so keep order stable
+	sort.Stable(hookByWeight(executingHooks))
 
 	for _, h := range executingHooks {
 		// Set default delete policy to before-hook-creation

pkg/releaseutil/kind_sorter.go

@@ -16,7 +16,11 @@ limitations under the License.
 
 package releaseutil
 
-import "sort"
+import (
+	"sort"
+
+	"helm.sh/helm/v3/pkg/release"
+)
 
 // KindSortOrder is an ordering of Kinds.
 type KindSortOrder []string
@@ -99,46 +103,84 @@ var UninstallOrder KindSortOrder = []string{
 	"Namespace",
 }
 
-// sortByKind does an in-place sort of manifests by Kind.
+// sort manifests by kind (out of place sort)
 //
 // Results are sorted by 'ordering', keeping order of items with equal kind/priority
-func sortByKind(manifests []Manifest, ordering KindSortOrder) []Manifest {
+func manifestsSortedByKind(manifests []Manifest, ordering KindSortOrder) []Manifest {
-	ks := newKindSorter(manifests, ordering)
+	k := make([]string, len(manifests))
+	for i, h := range manifests {
+		k[i] = h.Head.Kind
+	}
+	ks := newKindSorter(k, ordering)
 	sort.Stable(ks)
-	return ks.manifests
+
+	// apply permutation
+	sorted := make([]Manifest, len(manifests))
+	for i, p := range ks.permutation {
+		sorted[i] = manifests[p]
+	}
+	return sorted
+}
+
+// sort hooks by kind (out of place sort)
+//
+// Results are sorted by 'ordering', keeping order of items with equal kind/priority
+func hooksSortedByKind(hooks []*release.Hook, ordering KindSortOrder) []*release.Hook {
+	k := make([]string, len(hooks))
+	for i, h := range hooks {
+		k[i] = h.Kind
+	}
+
+	ks := newKindSorter(k, ordering)
+	sort.Stable(ks)
+
+	// apply permutation
+	sorted := make([]*release.Hook, len(hooks))
+	for i, p := range ks.permutation {
+		sorted[i] = hooks[p]
+	}
+	return sorted
 }
 
 type kindSorter struct {
+	permutation []int
 	ordering    map[string]int
-	manifests []Manifest
+	kinds       []string
 }
 
-func newKindSorter(m []Manifest, s KindSortOrder) *kindSorter {
+func newKindSorter(kinds []string, s KindSortOrder) *kindSorter {
 	o := make(map[string]int, len(s))
 	for v, k := range s {
 		o[k] = v
 	}
 
+	p := make([]int, len(kinds))
+	for i := range p {
+		p[i] = i
+	}
 	return &kindSorter{
-		manifests: m,
+		permutation: p,
+		kinds:       kinds,
 		ordering:    o,
 	}
 }
 
-func (k *kindSorter) Len() int { return len(k.manifests) }
+func (k *kindSorter) Len() int { return len(k.kinds) }
 
-func (k *kindSorter) Swap(i, j int) { k.manifests[i], k.manifests[j] = k.manifests[j], k.manifests[i] }
+func (k *kindSorter) Swap(i, j int) {
+	k.permutation[i], k.permutation[j] = k.permutation[j], k.permutation[i]
+}
 
 func (k *kindSorter) Less(i, j int) bool {
-	a := k.manifests[i]
+	a := k.kinds[k.permutation[i]]
-	b := k.manifests[j]
+	b := k.kinds[k.permutation[j]]
-	first, aok := k.ordering[a.Head.Kind]
+	first, aok := k.ordering[a]
-	second, bok := k.ordering[b.Head.Kind]
+	second, bok := k.ordering[b]
 
 	if !aok && !bok {
 		// if both are unknown then sort alphabetically by kind, keep original order if same kind
-		if a.Head.Kind != b.Head.Kind {
+		if a != b {
-			return a.Head.Kind < b.Head.Kind
+			return a < b
 		}
 		return first < second
 	}

pkg/releaseutil/kind_sorter_test.go

@@ -19,6 +19,8 @@ package releaseutil
 import (
 	"bytes"
 	"testing"
+
+	"helm.sh/helm/v3/pkg/release"
 )
 
 func TestKindSorter(t *testing.T) {
@@ -175,7 +177,7 @@ func TestKindSorter(t *testing.T) {
 				t.Fatalf("Expected %d names in order, got %d", want, got)
 			}
 			defer buf.Reset()
-			for _, r := range sortByKind(manifests, test.order) {
+			for _, r := range manifestsSortedByKind(manifests, test.order) {
 				buf.WriteString(r.Name)
 			}
 			if got := buf.String(); got != test.expected {
@@ -236,7 +238,7 @@ func TestKindSorterKeepOriginalOrder(t *testing.T) {
 		var buf bytes.Buffer
 		t.Run(test.description, func(t *testing.T) {
 			defer buf.Reset()
-			for _, r := range sortByKind(manifests, test.order) {
+			for _, r := range manifestsSortedByKind(manifests, test.order) {
 				buf.WriteString(r.Name)
 			}
 			if got := buf.String(); got != test.expected {
@@ -257,7 +259,7 @@ func TestKindSorterNamespaceAgainstUnknown(t *testing.T) {
 	}
 
 	manifests := []Manifest{unknown, namespace}
-	sortByKind(manifests, InstallOrder)
+	manifests = manifestsSortedByKind(manifests, InstallOrder)
 
 	expectedOrder := []Manifest{namespace, unknown}
 	for i, manifest := range manifests {
@@ -266,3 +268,48 @@ func TestKindSorterNamespaceAgainstUnknown(t *testing.T) {
 		}
 	}
 }
+
+// test hook sorting with a small subset of kinds, since it uses the same algorithm as manifestsSortedByKind
+func TestKindSorterForHooks(t *testing.T) {
+	hooks := []*release.Hook{
+		{
+			Name: "i",
+			Kind: "ClusterRole",
+		},
+		{
+			Name: "j",
+			Kind: "ClusterRoleBinding",
+		},
+		{
+			Name: "c",
+			Kind: "LimitRange",
+		},
+		{
+			Name: "a",
+			Kind: "Namespace",
+		},
+	}
+
+	for _, test := range []struct {
+		description string
+		order       KindSortOrder
+		expected    string
+	}{
+		{"install", InstallOrder, "acij"},
+		{"uninstall", UninstallOrder, "jica"},
+	} {
+		var buf bytes.Buffer
+		t.Run(test.description, func(t *testing.T) {
+			if got, want := len(test.expected), len(hooks); got != want {
+				t.Fatalf("Expected %d names in order, got %d", want, got)
+			}
+			defer buf.Reset()
+			for _, r := range hooksSortedByKind(hooks, test.order) {
+				buf.WriteString(r.Name)
+			}
+			if got := buf.String(); got != test.expected {
+				t.Errorf("Expected %q, got %q", test.expected, got)
+			}
+		})
+	}
+}

pkg/releaseutil/manifest_sorter.go

@@ -108,7 +108,7 @@ func SortManifests(files map[string]string, apis chartutil.VersionSet, ordering
 		}
 	}
 
-	return result.hooks, sortByKind(result.generic, ordering), nil
+	return hooksSortedByKind(result.hooks, ordering), manifestsSortedByKind(result.generic, ordering), nil
 }
 
 // sort takes a manifestFile object which may contain multiple resource definition

pkg/releaseutil/manifest_sorter_test.go

@@ -219,7 +219,7 @@ metadata:
 		}
 	}
 
-	sorted = sortByKind(sorted, InstallOrder)
+	sorted = manifestsSortedByKind(sorted, InstallOrder)
 	for i, m := range generic {
 		if m.Content != sorted[i].Content {
 			t.Errorf("Expected %q, got %q", m.Content, sorted[i].Content)