fix install storing computed values in release

this was partially fixed in  but the fix only
worked for values without nesting. this PR fixes it.
this is done by doing a deep copy of values rather
than a top level keys copy. deep copy ensures
values are not mutated during coalesce()
execution which leads to bugs like 

the deep copy code has been copied from:

https://gist.github.com/soroushjp/0ec92102641ddfc3ad5515ca76405f4d

which is in turn inspired by this stackoverflow answer:
http://stackoverflow.com/a/28579297/1366283

Signed-off-by: Karuppiah Natarajan <karuppiah7890@gmail.com>
pull/6661/head
Karuppiah Natarajan 6 years ago
parent e7413bd61c
commit dfed8ab5e3
No known key found for this signature in database
GPG Key ID: C674A28337662A96

@ -0,0 +1,39 @@
package deepcopy
// DeepCopyMap a function for deep copying map[string]interface{}
// values. Inspired by:
// https://gist.github.com/soroushjp/0ec92102641ddfc3ad5515ca76405f4d by Soroush Pour
// The gist code is MIT licensed
// which in turn has been inspired by the StackOverflow answer at:
// http://stackoverflow.com/a/28579297/1366283
//
// Uses the golang.org/pkg/encoding/gob package to do this and therefore has the
// same caveats.
// See: https://blog.golang.org/gobs-of-data
// See: https://golang.org/pkg/encoding/gob/
import (
"bytes"
"encoding/gob"
)
func init() {
gob.Register(map[string]interface{}{})
}
// Map performs a deep copy of the given map m.
func Map(m map[string]interface{}) (map[string]interface{}, error) {
var buf bytes.Buffer
enc := gob.NewEncoder(&buf)
dec := gob.NewDecoder(&buf)
err := enc.Encode(m)
if err != nil {
return nil, err
}
var mapCopy map[string]interface{}
err = dec.Decode(&mapCopy)
if err != nil {
return nil, err
}
return mapCopy, nil
}

@ -0,0 +1,130 @@
package deepcopy_test
import (
"helm.sh/helm/v3/internal/third_party/deepcopy"
"testing"
"github.com/stretchr/testify/assert"
)
func TestCopyMap(t *testing.T) {
testCases := []struct {
// original and expectedOriginal are the same value in each test case. We do
// this to avoid unintentionally asserting against a mutated
// expectedOriginal and having the test pass erroneously. We also do not
// want to rely on the deep copy function we are testing to ensure this does
// not happen.
original map[string]interface{}
transformer func(m map[string]interface{}) map[string]interface{}
expectedCopy map[string]interface{}
expectedOriginal map[string]interface{}
}{
// reassignment of entire map, should be okay even without deep copy.
{
original: nil,
transformer: func(m map[string]interface{}) map[string]interface{} {
return map[string]interface{}{}
},
expectedCopy: map[string]interface{}{},
expectedOriginal: nil,
},
{
original: map[string]interface{}{},
transformer: func(m map[string]interface{}) map[string]interface{} {
return nil
},
expectedCopy: nil,
expectedOriginal: map[string]interface{}{},
},
// mutation of map
{
original: map[string]interface{}{},
transformer: func(m map[string]interface{}) map[string]interface{} {
m["foo"] = "bar"
return m
},
expectedCopy: map[string]interface{}{
"foo": "bar",
},
expectedOriginal: map[string]interface{}{},
},
{
original: map[string]interface{}{
"foo": "bar",
},
transformer: func(m map[string]interface{}) map[string]interface{} {
m["foo"] = "car"
return m
},
expectedCopy: map[string]interface{}{
"foo": "car",
},
expectedOriginal: map[string]interface{}{
"foo": "bar",
},
},
// mutation of nested maps
{
original: map[string]interface{}{},
transformer: func(m map[string]interface{}) map[string]interface{} {
m["foo"] = map[string]interface{}{
"biz": "baz",
}
return m
},
expectedCopy: map[string]interface{}{
"foo": map[string]interface{}{
"biz": "baz",
},
},
expectedOriginal: map[string]interface{}{},
},
{
original: map[string]interface{}{
"foo": map[string]interface{}{
"biz": "booz",
"gaz": "gooz",
},
},
transformer: func(m map[string]interface{}) map[string]interface{} {
m["foo"] = map[string]interface{}{
"biz": "baz",
}
return m
},
expectedCopy: map[string]interface{}{
"foo": map[string]interface{}{
"biz": "baz",
},
},
expectedOriginal: map[string]interface{}{
"foo": map[string]interface{}{
"biz": "booz",
"gaz": "gooz",
},
},
},
// mutation of slice values
{
original: map[string]interface{}{
"foo": []string{"biz", "baz"},
},
transformer: func(m map[string]interface{}) map[string]interface{} {
m["foo"].([]string)[0] = "hiz"
return m
},
expectedCopy: map[string]interface{}{
"foo": []string{"hiz", "baz"},
},
expectedOriginal: map[string]interface{}{
"foo": []string{"biz", "baz"},
},
},
}
for i, tc := range testCases {
mapCopy, err := deepcopy.Map(tc.original)
assert.NoError(t, err)
assert.Exactly(t, tc.expectedCopy, tc.transformer(mapCopy), "copy was not mutated. test case: %d", i)
assert.Exactly(t, tc.expectedOriginal, tc.original, "original was mutated. test case: %d", i)
}
}

@ -173,7 +173,17 @@ func withName(name string) chartOption {
}
func withSampleValues() chartOption {
values := map[string]interface{}{"someKey": "someValue"}
values := map[string]interface{}{
"someKey": "someValue",
"nestedKey": map[string]interface{}{
"simpleKey": "simpleValue",
"anotherNestedKey": map[string]interface{}{
"yetAnotherNestedKey": map[string]interface{}{
"youReadyForAnotherNestedKey": "No",
},
},
},
}
return func(opts *chartOptions) {
opts.Values = values
}

@ -78,8 +78,16 @@ func TestInstallRelease(t *testing.T) {
func TestInstallReleaseWithValues(t *testing.T) {
is := assert.New(t)
instAction := installAction(t)
userVals := map[string]interface{}{}
expectedUserValues := map[string]interface{}{}
userVals := map[string]interface{}{
"nestedKey": map[string]interface{}{
"simpleKey": "simpleValue",
},
}
expectedUserValues := map[string]interface{}{
"nestedKey": map[string]interface{}{
"simpleKey": "simpleValue",
},
}
res, err := instAction.Run(buildChart(withSampleValues()), userVals)
if err != nil {
t.Fatalf("Failed install: %s", err)

@ -223,7 +223,6 @@ func TestUpgradeRelease_ReuseValues(t *testing.T) {
expectedValues := map[string]interface{}{
"subchart": map[string]interface{}{
"enabled": false,
"global": map[string]interface{}{},
},
}
is.Equal(expectedValues, updatedRes.Config)

@ -19,6 +19,8 @@ package chartutil
import (
"log"
"helm.sh/helm/v3/internal/third_party/deepcopy"
"github.com/pkg/errors"
"helm.sh/helm/v3/pkg/chart"
@ -36,7 +38,10 @@ import (
func CoalesceValues(chrt *chart.Chart, vals map[string]interface{}) (Values, error) {
// create a copy of vals and then pass it to coalesce
// and coalesceDeps, as both will mutate the passed values
valsCopy := copyMap(vals)
valsCopy, err := deepcopy.Map(vals)
if err != nil {
return vals, err
}
if _, err := coalesce(chrt, valsCopy); err != nil {
return valsCopy, err
}

Loading…
Cancel
Save