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.

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

Fix bug checksum chart values will not consider about order of key-value #676

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion go.mod
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
6 changes: 6 additions & 0 deletions internal/controllers/helmrelease_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,12 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context,
valuesChecksum := util.ValuesChecksum(values)
hr, hasNewState := v2.HelmReleaseAttempted(hr, revision, releaseRevision, valuesChecksum)

if hasNewState {
// If there is a change, check on ordered checksum.
valuesChecksum = util.OrderedValuesChecksum(values)
hr, hasNewState = v2.HelmReleaseAttempted(hr, revision, releaseRevision, valuesChecksum)
}

// Run diff against current cluster state.
if !hasNewState && rel != nil {
if ok, _ := features.Enabled(features.DetectDrift); ok {
Expand Down
77 changes: 77 additions & 0 deletions internal/util/util.go
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"
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for not using v3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the v2 which is being used in "sigs.k8s.io/yaml" and in the v2, they support MapSlice structure which is easy to handle to me.

"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,80 @@ 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 {
// Check sum on the formatted values
newValues := copyValues(values)
msValues := yaml.JSONObjectToYAMLObject(newValues)
// Sort
SortMapSlice(msValues)
// Marshal
s, _ = goyaml.Marshal(msValues)
}
// Get hash
return fmt.Sprintf("%x", sha1.Sum(s))
}

func SortMapSlice(ms goyaml.MapSlice) {
sort.Slice(ms, func(i, j int) bool {
return fmt.Sprintf("%T.%v", ms[i].Key, ms[i].Key) < fmt.Sprintf("%T.%v", ms[j].Key, 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)
}
}
}
}
}

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{} {
// Marshal
coppiedValues, _ := goyaml.Marshal(in)
// Unmarshal
newValues := make(map[string]interface{})
goyaml.Unmarshal(coppiedValues, newValues)
// cleanUpInterfaceMap
for i, value := range newValues {
newValues[fmt.Sprint(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
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)
}
})
}
}