Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stable sort release values by key #684

Merged
merged 1 commit into from
May 11, 2023
Merged

Stable sort release values by key #684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions api/v2beta1/helmrelease_types.go
View file
Open in desktop
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 {
Comment on lines +961 to +963
Copy link
Contributor

@darkowlzz darkowlzz May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be ignored, but would it be helpful for a future reader to provide some context in the function comment about why we allow for multiple valuesChecksums when a HelmRelease can have only one of all the these parameters? We can add some hint about creating room for the possibility of the checksum algorithm change in the future. It's clear now considering the issue we have, but I'm afraid it may be confusing to see multiple values checksums being allowed here. One may be able to check the history and figure it out.
Another way to provide some context here would be to have some tests with multiple values checksums, old and new.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code shouldn't continue to exist for too long, as we will be moving away from any of the utility methods in the API package soon anyway. Which is why I simply restructured this to be done with it, while taking into account existing contracts due to SemVer compatibility and "being nice".

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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)
}
})
}
}