From 4db8fa1c18dfc3c2d11e427f9bcc2d0f50737e45 Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Tue, 9 Jun 2026 19:52:42 +0200 Subject: [PATCH] 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 --- .../clusterpromotion_controller_test.go | 4 +- controllers/handlers_utils.go | 2 +- controllers/handlers_utils_test.go | 2 +- controllers/profile_hash_test.go | 87 +++++++++++++++++++ controllers/profile_utils.go | 30 +++++-- controllers/sort.go | 66 ++++++++++---- 6 files changed, 162 insertions(+), 29 deletions(-) create mode 100644 controllers/profile_hash_test.go diff --git a/controllers/clusterpromotion_controller_test.go b/controllers/clusterpromotion_controller_test.go index 56fe4114..1bfa391a 100644 --- a/controllers/clusterpromotion_controller_test.go +++ b/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 { diff --git a/controllers/handlers_utils.go b/controllers/handlers_utils.go index 47b28226..eae1f332 100644 --- a/controllers/handlers_utils.go +++ b/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 }) diff --git a/controllers/handlers_utils_test.go b/controllers/handlers_utils_test.go index a6b50ccd..2bc301c1 100644 --- a/controllers/handlers_utils_test.go +++ b/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)) }) diff --git a/controllers/profile_hash_test.go b/controllers/profile_hash_test.go new file mode 100644 index 00000000..e938d739 --- /dev/null +++ b/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) + } + }) +}) diff --git a/controllers/profile_utils.go b/controllers/profile_utils.go index ca46b435..c57fe0e8 100644 --- a/controllers/profile_utils.go +++ b/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) } diff --git a/controllers/sort.go b/controllers/sort.go index 50143638..8bc93cd3 100644 --- a/controllers/sort.go +++ b/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 {