Fix hash instability, file collision, and sort performance in addon-controller

Silent file loss when deploying from Flux sources with nested directories
When reading files from a Flux source path, the code was keying each file by its
bare filename rather than its path relative to the source root. If two files in
different subdirectories shared the same name, the second one silently overwrote
the first, causing part of the content to be dropped without any error. The fix
uses the relative path as the key.

Non-deterministic hash for DriftExclusion ordering
SortedDriftExclusions.Less() compared entries using only their Paths field.
Two entries with identical paths but different Target fields compared as equal,
so the sort was free to place them in any order. Because the sorted slice feeds
directly into the spec hash used by ClusterPromotionReconciler, the hash could
differ between runs for semantically identical specs, triggering spurious promotion
re-evaluations. The sort now includes Target (Group, Version, Kind, Namespace, Name)
as a stable tiebreaker.

Profile spec hash was non-deterministic for map fields and used a slow renderer

Author: gianlucam76

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 {