Fix hash instability, file collision, and sort performance

controllers/clusterpromotion_controller_test.go

@@ -66,8 +66,8 @@ var _ = Describe("ClusterPromotionController", func() {
 			sourceField := profileSpecType.Field(i)
 
 			// Those fields are only present in ClusterProfile.Spec
-			if sourceField.Name == ClusterSelectorField || sourceField.Name == "ClusterRefs" ||
-				sourceField.Name == "SetRefs" {
+			if sourceField.Name == ClusterSelectorField || sourceField.Name == clusterRefsField ||
+				sourceField.Name == setRefsField {
 
 				_, found := clusterPromotionProfileSpecField.FieldByName(sourceField.Name)
 				if found {

controllers/handlers_utils.go

@@ -207,7 +207,7 @@ func readFiles(dir string) (map[string]string, error) {
 				return err
 			}
 
-			files[d.Name()] = string(content)
+			files[rel] = string(content)
 		}
 		return nil
 	})

controllers/handlers_utils_test.go

@@ -1102,7 +1102,7 @@ metadata:
 		Expect(ok).To(BeTrue())
 		Expect(v).To(Equal(serviceTemplate))
 
-		v, ok = result["file2.txt"]
+		v, ok = result[filepath.Join(subdir, "file2.txt")]
 		Expect(ok).To(BeTrue())
 		Expect(v).To(Equal(deplTemplate))
 	})

controllers/profile_hash_test.go

@@ -0,0 +1,87 @@
+/*
+Copyright 2022. projectsveltos.io. All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controllers_test
+
+import (
+	"reflect"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1"
+)
+
+const (
+	// clusterRefsField and setRefsField name the two Spec slice fields that
+	// are explicitly excluded from hash normalisation (their order is
+	// author-determined). The constants are also used by
+	// clusterpromotion_controller_test.go.
+	clusterRefsField = "ClusterRefs"
+	setRefsField     = "SetRefs"
+)
+
+var _ = Describe("getProfileSpecHash slice-field coverage", func() {
+	// This test guards against getProfileSpecHash silently ignoring newly
+	// added slice fields in configv1beta1.Spec. json.Marshal includes every
+	// field automatically, but slice fields must also be *sorted* before
+	// marshaling so the hash is stable regardless of element order.
+	//
+	// When a new slice field is added to configv1beta1.Spec:
+	//   • Add a getSorted* helper (or reuse an existing one) in sort.go
+	//   • Call it inside getProfileSpecHash in profile_utils.go
+	//   • Add the field name to sortedFields below
+	//
+	// If its element order is always deterministic (e.g. explicit single-item
+	// refs typed by the user with no meaningful reordering), add it to
+	// orderDeterministic instead.
+	It("accounts for every slice field in configv1beta1.Spec", func() {
+		// Fields that getProfileSpecHash explicitly sorts before hashing.
+		sortedFields := map[string]bool{
+			"HelmCharts":           true,
+			"PolicyRefs":           true,
+			"KustomizationRefs":    true,
+			"TemplateResourceRefs": true,
+			"ValidateHealths":      true,
+			"PreDeployChecks":      true,
+			"PreDeleteChecks":      true,
+			"PostDeleteChecks":     true,
+			"Patches":              true,
+			"PatchesFrom":          true,
+			"DriftExclusions":      true,
+			"DependsOn":            true,
+		}
+		// Slice fields whose element order is author-determined and therefore
+		// does not need normalisation to produce a stable hash.
+		orderDeterministic := map[string]bool{
+			clusterRefsField: true,
+			setRefsField:     true,
+		}
+
+		specType := reflect.TypeOf(configv1beta1.Spec{})
+		for i := 0; i < specType.NumField(); i++ {
+			f := specType.Field(i)
+			if f.Type.Kind() != reflect.Slice {
+				continue
+			}
+			Expect(sortedFields[f.Name] || orderDeterministic[f.Name]).To(BeTrue(),
+				"Spec.%s is a slice field not yet handled by getProfileSpecHash.\n"+
+					"Add sorting for it in getProfileSpecHash (profile_utils.go) and\n"+
+					"list it in 'sortedFields' in this test, or add it to\n"+
+					"'orderDeterministic' if its element order is always stable.", f.Name)
+		}
+	})
+})

controllers/profile_utils.go

@@ -19,13 +19,14 @@ package controllers
 import (
 	"context"
 	"crypto/sha256"
+	"encoding/json"
 	"fmt"
 	"math"
 	"reflect"
+	"sort"
 	"strings"
 
 	"github.com/dariubs/percent"
-	"github.com/gdexlab/go-render/render"
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
@@ -1066,14 +1067,31 @@ func cleanClusterReports(ctx context.Context, c client.Client, profileScope *sco
 	return nil
 }
 
-// getProfileSpecHash returns hash of current clusterProfile/Profile Spec
+// getProfileSpecHash returns hash of current clusterProfile/Profile Spec.
+// All slice fields are sorted before marshaling so the hash is stable
+// regardless of the order in which items appear in the original spec.
 func getProfileSpecHash(profileScope *scope.ProfileScope) []byte {
 	h := sha256.New()
-	var config string
 
-	config += render.AsCode(profileScope.GetSpec())
-
-	h.Write([]byte(config))
+	specCopy := *profileScope.GetSpec()
+	specCopy.HelmCharts = getSortedHelmCharts(specCopy.HelmCharts)
+	specCopy.PolicyRefs = getSortedPolicyRefs(specCopy.PolicyRefs)
+	specCopy.KustomizationRefs = getSortedKustomizationRefs(specCopy.KustomizationRefs)
+	specCopy.TemplateResourceRefs = getSortedTemplateResourceRefs(specCopy.TemplateResourceRefs)
+	specCopy.ValidateHealths = getSortedValidateHealths(specCopy.ValidateHealths)
+	specCopy.PreDeployChecks = getSortedValidateHealths(specCopy.PreDeployChecks)
+	specCopy.PreDeleteChecks = getSortedValidateHealths(specCopy.PreDeleteChecks)
+	specCopy.PostDeleteChecks = getSortedValidateHealths(specCopy.PostDeleteChecks)
+	specCopy.Patches = getSortedPatches(specCopy.Patches)
+	specCopy.PatchesFrom = getSortedValueFroms(specCopy.PatchesFrom)
+	specCopy.DriftExclusions = getSortedDriftExclusions(specCopy.DriftExclusions)
+	specCopy.DependsOn = make([]string, len(specCopy.DependsOn))
+	copy(specCopy.DependsOn, profileScope.GetSpec().DependsOn)
+	sort.Strings(specCopy.DependsOn)
+
+	if data, err := json.Marshal(specCopy); err == nil {
+		h.Write(data)
+	}
 	return h.Sum(nil)
 }
 

controllers/sort.go

@@ -138,31 +138,59 @@ func getSortedPatches(patches []libsveltosv1beta1.Patch) []libsveltosv1beta1.Pat
 	return sortedPatches
 }
 
-type SortedDriftExclusions []libsveltosv1beta1.DriftExclusion
-
-func (a SortedDriftExclusions) Len() int      { return len(a) }
-func (a SortedDriftExclusions) Swap(i, j int) { a[i], a[j] = a[j], a[i] }
-func (a SortedDriftExclusions) Less(i, j int) bool {
-	pathsI := make([]string, len(a[i].Paths))
-	copy(pathsI, a[i].Paths)
-	sort.Strings(pathsI)
+// driftExclusionEntry pairs a DriftExclusion with its pre-computed sort key
+// so the sort comparator can reference the slice being sorted.
+type driftExclusionEntry struct {
+	item libsveltosv1beta1.DriftExclusion
+	key  string
+}
 
-	pathsJ := make([]string, len(a[j].Paths))
-	copy(pathsJ, a[j].Paths)
-	sort.Strings(pathsJ)
+func getSortedDriftExclusions(driftExclusions []libsveltosv1beta1.DriftExclusion) []libsveltosv1beta1.DriftExclusion {
+	// Pre-compute a stable sort key for each element once. The key encodes
+	// sorted Paths (so path order doesn't matter) plus Target fields as a
+	// tiebreaker (so entries with identical paths sort deterministically).
+	entries := make([]driftExclusionEntry, len(driftExclusions))
+	for i := range driftExclusions {
+		p := make([]string, len(driftExclusions[i].Paths))
+		copy(p, driftExclusions[i].Paths)
+		sort.Strings(p)
+		key := strings.Join(p, "\x00")
+		if t := driftExclusions[i].Target; t != nil {
+			key += "\x01" + t.Group + "\x00" + t.Version + "\x00" + t.Kind + "\x00" + t.Namespace + "\x00" + t.Name
+		}
+		entries[i] = driftExclusionEntry{item: driftExclusions[i], key: key}
+	}
 
-	strI := strings.Join(pathsI, "|")
-	strJ := strings.Join(pathsJ, "|")
+	sort.SliceStable(entries, func(i, j int) bool {
+		return entries[i].key < entries[j].key
+	})
 
-	return strI < strJ
+	sorted := make([]libsveltosv1beta1.DriftExclusion, len(entries))
+	for i := range entries {
+		sorted[i] = entries[i].item
+	}
+	return sorted
 }
 
-func getSortedDriftExclusions(driftExclusions []libsveltosv1beta1.DriftExclusion) []libsveltosv1beta1.DriftExclusion {
-	sortedDriftExclusions := make([]libsveltosv1beta1.DriftExclusion, len(driftExclusions))
-	copy(sortedDriftExclusions, driftExclusions)
+type SortedValueFroms []configv1beta1.ValueFrom
+
+func (a SortedValueFroms) Len() int      { return len(a) }
+func (a SortedValueFroms) Swap(i, j int) { a[i], a[j] = a[j], a[i] }
+func (a SortedValueFroms) Less(i, j int) bool {
+	if a[i].Kind != a[j].Kind {
+		return a[i].Kind < a[j].Kind
+	}
+	if a[i].Namespace != a[j].Namespace {
+		return a[i].Namespace < a[j].Namespace
+	}
+	return a[i].Name < a[j].Name
+}
 
-	sort.Sort(SortedDriftExclusions(sortedDriftExclusions))
-	return sortedDriftExclusions
+func getSortedValueFroms(valueFroms []configv1beta1.ValueFrom) []configv1beta1.ValueFrom {
+	sorted := make([]configv1beta1.ValueFrom, len(valueFroms))
+	copy(sorted, valueFroms)
+	sort.Sort(SortedValueFroms(sorted))
+	return sorted
 }
 
 func getSortedKeys(m interface{}) []string {