From 30b131ab1c6fdd9e123a8dda66ec240c215a0a13 Mon Sep 17 00:00:00 2001 From: longquan0104 Date: Thu, 20 Apr 2023 14:46:35 +0700 Subject: [PATCH] Stable sort release values by key This commit changes the way the checksum is calculated for the release values, by stable sorting the keys. By doing this, an upgrade will not be triggered when a key/value pair has just been moved, instead of containing a real change of value. To make it backwards compatible (and without triggering an upgrade due to new ordering), the checksum without ordering is continued to be calculated and compared against until removal in a future controller release. However, only the checksum of the ordered values is taken note of in the Status of the HelmRelease. Co-authored-by: Hidde Beydals Signed-off-by: longquan0104 --- api/v2beta1/helmrelease_types.go | 27 +++ go.mod | 2 +- .../controllers/helmrelease_controller.go | 10 +- internal/util/util.go | 78 +++++++++ internal/util/util_test.go | 164 ++++++++++++++++++ 5 files changed, 277 insertions(+), 4 deletions(-) diff --git a/api/v2beta1/helmrelease_types.go b/api/v2beta1/helmrelease_types.go index 24d0e1d0e..925994083 100644 --- a/api/v2beta1/helmrelease_types.go +++ b/api/v2beta1/helmrelease_types.go @@ -945,6 +945,8 @@ func HelmReleaseReady(hr HelmRelease) HelmRelease { // HelmReleaseAttempted registers an attempt of the given HelmRelease with the given state. // and returns the modified HelmRelease and a boolean indicating a state change. +// +// Deprecated: in favor of HelmReleaseChanged and HelmReleaseRecordAttempt. func HelmReleaseAttempted(hr HelmRelease, revision string, releaseRevision int, valuesChecksum string) (HelmRelease, bool) { changed := hr.Status.LastAttemptedRevision != revision || hr.Status.LastReleaseRevision != releaseRevision || @@ -956,6 +958,31 @@ func HelmReleaseAttempted(hr HelmRelease, revision string, releaseRevision int, return hr, changed } +// HelmReleaseChanged returns if the HelmRelease has changed compared to the +// provided values. +func HelmReleaseChanged(hr HelmRelease, revision string, releaseRevision int, valuesChecksums ...string) bool { + return hr.Status.LastAttemptedRevision != revision || + hr.Status.LastReleaseRevision != releaseRevision || + !inStringSlice(hr.Status.LastAttemptedValuesChecksum, valuesChecksums) +} + +// HelmReleaseRecordAttempt returns an attempt of the given HelmRelease with the +// given state in the Status of the provided object. +func HelmReleaseRecordAttempt(hr *HelmRelease, revision string, releaseRevision int, valuesChecksum string) { + hr.Status.LastAttemptedRevision = revision + hr.Status.LastReleaseRevision = releaseRevision + hr.Status.LastAttemptedValuesChecksum = valuesChecksum +} + +func inStringSlice(str string, s []string) bool { + for _, v := range s { + if str == v { + return true + } + } + return false +} + func resetFailureCounts(hr *HelmRelease) { hr.Status.Failures = 0 hr.Status.InstallFailures = 0 diff --git a/go.mod b/go.mod index 3035a66fa..b29c07f61 100644 --- a/go.mod +++ b/go.mod @@ -20,6 +20,7 @@ require ( github.com/opencontainers/go-digest v1.0.0 github.com/opencontainers/go-digest/blake3 v0.0.0-20220411205349-bde1400a84be github.com/spf13/pflag v1.0.5 + gopkg.in/yaml.v2 v2.4.0 helm.sh/helm/v3 v3.11.3 k8s.io/api v0.26.3 k8s.io/apiextensions-apiserver v0.26.3 @@ -159,7 +160,6 @@ require ( google.golang.org/grpc v1.53.0 // indirect google.golang.org/protobuf v1.28.1 // indirect gopkg.in/inf.v0 v0.9.1 // indirect - gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/apiserver v0.26.3 // indirect k8s.io/component-base v0.26.3 // indirect diff --git a/internal/controllers/helmrelease_controller.go b/internal/controllers/helmrelease_controller.go index fb3dc3ada..0ce6c1413 100644 --- a/internal/controllers/helmrelease_controller.go +++ b/internal/controllers/helmrelease_controller.go @@ -322,11 +322,15 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, return v2.HelmReleaseNotReady(hr, v2.GetLastReleaseFailedReason, "failed to get last release revision"), err } - // Register the current release attempt. + // Detect divergence between release in storage and HelmRelease spec. revision := chart.Metadata.Version releaseRevision := util.ReleaseRevision(rel) - valuesChecksum := util.ValuesChecksum(values) - hr, hasNewState := v2.HelmReleaseAttempted(hr, revision, releaseRevision, valuesChecksum) + // TODO: deprecate "unordered" checksum. + valuesChecksum := util.OrderedValuesChecksum(values) + hasNewState := v2.HelmReleaseChanged(hr, revision, releaseRevision, util.ValuesChecksum(values), valuesChecksum) + + // Register the current release attempt. + v2.HelmReleaseRecordAttempt(&hr, revision, releaseRevision, valuesChecksum) // Run diff against current cluster state. if !hasNewState && rel != nil { diff --git a/internal/util/util.go b/internal/util/util.go index 5c2247b3d..8c43d53b0 100644 --- a/internal/util/util.go +++ b/internal/util/util.go @@ -19,9 +19,12 @@ package util import ( "crypto/sha1" "fmt" + "sort" + goyaml "gopkg.in/yaml.v2" "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/release" + "sigs.k8s.io/yaml" ) // ValuesChecksum calculates and returns the SHA1 checksum for the @@ -34,6 +37,81 @@ func ValuesChecksum(values chartutil.Values) string { return fmt.Sprintf("%x", sha1.Sum([]byte(s))) } +// OrderedValuesChecksum sort the chartutil.Values then calculates +// and returns the SHA1 checksum for the sorted values. +func OrderedValuesChecksum(values chartutil.Values) string { + var s []byte + if len(values) != 0 { + msValues := yaml.JSONObjectToYAMLObject(copyValues(values)) + SortMapSlice(msValues) + s, _ = goyaml.Marshal(msValues) + } + return fmt.Sprintf("%x", sha1.Sum(s)) +} + +// SortMapSlice recursively sorts the given goyaml.MapSlice by key. +// This is used to ensure that the values checksum is the same regardless +// of the order of the keys in the values file. +func SortMapSlice(ms goyaml.MapSlice) { + sort.Slice(ms, func(i, j int) bool { + return fmt.Sprint(ms[i].Key) < fmt.Sprint(ms[j].Key) + }) + for _, item := range ms { + if nestedMS, ok := item.Value.(goyaml.MapSlice); ok { + SortMapSlice(nestedMS) + } else if _, ok := item.Value.([]interface{}); ok { + for _, vItem := range item.Value.([]interface{}) { + if itemMS, ok := vItem.(goyaml.MapSlice); ok { + SortMapSlice(itemMS) + } + } + } + } +} + +// cleanUpMapValue changes all instances of +// map[interface{}]interface{} to map[string]interface{}. +// This is for handling the mismatch when unmarshaling +// reference to the issue: https://github.com/go-yaml/yaml/issues/139 +func cleanUpMapValue(v interface{}) interface{} { + switch v := v.(type) { + case []interface{}: + return cleanUpInterfaceArray(v) + case map[interface{}]interface{}: + return cleanUpInterfaceMap(v) + default: + return v + } +} + +func cleanUpInterfaceMap(in map[interface{}]interface{}) map[string]interface{} { + result := make(map[string]interface{}) + for k, v := range in { + result[fmt.Sprintf("%v", k)] = cleanUpMapValue(v) + } + return result +} + +func cleanUpInterfaceArray(in []interface{}) []interface{} { + result := make([]interface{}, len(in)) + for i, v := range in { + result[i] = cleanUpMapValue(v) + } + return result +} + +func copyValues(in map[string]interface{}) map[string]interface{} { + copiedValues, _ := goyaml.Marshal(in) + newValues := make(map[string]interface{}) + + _ = goyaml.Unmarshal(copiedValues, newValues) + for i, value := range newValues { + newValues[i] = cleanUpMapValue(value) + } + + return newValues +} + // ReleaseRevision returns the revision of the given release.Release. func ReleaseRevision(rel *release.Release) int { if rel == nil { diff --git a/internal/util/util_test.go b/internal/util/util_test.go index 1dabf4fe4..04826f642 100644 --- a/internal/util/util_test.go +++ b/internal/util/util_test.go @@ -17,8 +17,10 @@ limitations under the License. package util import ( + "reflect" "testing" + goyaml "gopkg.in/yaml.v2" "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/release" ) @@ -54,6 +56,38 @@ func TestValuesChecksum(t *testing.T) { } } +func TestOrderedValuesChecksum(t *testing.T) { + tests := []struct { + name string + values chartutil.Values + want string + }{ + { + name: "empty", + values: chartutil.Values{}, + want: "da39a3ee5e6b4b0d3255bfef95601890afd80709", + }, + { + name: "value map", + values: chartutil.Values{ + "foo": "bar", + "baz": map[string]string{ + "fruit": "apple", + "cool": "stuff", + }, + }, + want: "dfd6589332e4d2da5df7bcbf5885f406f08b58ee", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := OrderedValuesChecksum(tt.values); got != tt.want { + t.Errorf("OrderedValuesChecksum() = %v, want %v", got, tt.want) + } + }) + } +} + func TestReleaseRevision(t *testing.T) { var rel *release.Release if rev := ReleaseRevision(rel); rev != 0 { @@ -64,3 +98,133 @@ func TestReleaseRevision(t *testing.T) { t.Fatalf("ReleaseRevision() = %v, want %v", rev, 1) } } + +func TestSortMapSlice(t *testing.T) { + tests := []struct { + name string + input goyaml.MapSlice + expected goyaml.MapSlice + }{ + { + name: "Simple case", + input: goyaml.MapSlice{ + {Key: "b", Value: 2}, + {Key: "a", Value: 1}, + }, + expected: goyaml.MapSlice{ + {Key: "a", Value: 1}, + {Key: "b", Value: 2}, + }, + }, + { + name: "Nested MapSlice", + input: goyaml.MapSlice{ + {Key: "b", Value: 2}, + {Key: "a", Value: 1}, + {Key: "c", Value: goyaml.MapSlice{ + {Key: "d", Value: 4}, + {Key: "e", Value: 5}, + }}, + }, + expected: goyaml.MapSlice{ + {Key: "a", Value: 1}, + {Key: "b", Value: 2}, + {Key: "c", Value: goyaml.MapSlice{ + {Key: "d", Value: 4}, + {Key: "e", Value: 5}, + }}, + }, + }, + { + name: "Empty MapSlice", + input: goyaml.MapSlice{}, + expected: goyaml.MapSlice{}, + }, + { + name: "Single element", + input: goyaml.MapSlice{ + {Key: "a", Value: 1}, + }, + expected: goyaml.MapSlice{ + {Key: "a", Value: 1}, + }, + }, + { + name: "Already sorted", + input: goyaml.MapSlice{ + {Key: "a", Value: 1}, + {Key: "b", Value: 2}, + {Key: "c", Value: 3}, + }, + expected: goyaml.MapSlice{ + {Key: "a", Value: 1}, + {Key: "b", Value: 2}, + {Key: "c", Value: 3}, + }, + }, + + { + name: "Complex Case", + input: goyaml.MapSlice{ + {Key: "b", Value: 2}, + {Key: "a", Value: map[interface{}]interface{}{ + "d": []interface{}{4, 5}, + "c": 3, + }}, + {Key: "c", Value: goyaml.MapSlice{ + {Key: "f", Value: 6}, + {Key: "e", Value: goyaml.MapSlice{ + {Key: "h", Value: 8}, + {Key: "g", Value: 7}, + }}, + }}, + }, + expected: goyaml.MapSlice{ + {Key: "a", Value: map[interface{}]interface{}{ + "c": 3, + "d": []interface{}{4, 5}, + }}, + {Key: "b", Value: 2}, + {Key: "c", Value: goyaml.MapSlice{ + {Key: "e", Value: goyaml.MapSlice{ + {Key: "g", Value: 7}, + {Key: "h", Value: 8}, + }}, + {Key: "f", Value: 6}, + }}, + }, + }, + { + name: "Map slice in slice", + input: goyaml.MapSlice{ + {Key: "b", Value: 2}, + {Key: "a", Value: []interface{}{ + map[interface{}]interface{}{ + "d": 4, + "c": 3, + }, + 1, + }}, + }, + expected: goyaml.MapSlice{ + {Key: "a", Value: []interface{}{ + map[interface{}]interface{}{ + "c": 3, + "d": 4, + }, + 1, + }}, + {Key: "b", Value: 2}, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + SortMapSlice(test.input) + if !reflect.DeepEqual(test.input, test.expected) { + t.Errorf("Expected %v, got %v", test.expected, test.input) + } + }) + } +}