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 {