Skip to content

Commit

Permalink
Stable sort release values by key
Browse files Browse the repository at this point in the history 
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 <hidde@hhh.computer>
Signed-off-by: longquan0104 <longquan0104@gmail.com>
  • Loading branch information
@longquan0104 @hiddeco
longquan0104 and hiddeco committed May 11, 2023
1 parent 2d1dbc1 commit 30b131a
Show file tree
Hide file tree
Showing 5 changed files with 277 additions and 4 deletions.
27 changes: 27 additions & 0 deletions api/v2beta1/helmrelease_types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ||
Expand All @@ -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
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
10 changes: 7 additions & 3 deletions internal/controllers/helmrelease_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
78 changes: 78 additions & 0 deletions internal/util/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand Down
164 changes: 164 additions & 0 deletions internal/util/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
}
})
}
}

0 comments on commit 30b131a

Please sign in to comment.