From e31f794b0d698e45c70a3c7038911f0bd67c357f Mon Sep 17 00:00:00 2001 From: Morten Torkildsen Date: Thu, 18 Oct 2018 08:59:22 -0700 Subject: [PATCH] fix(helm): Merge nested values correctly on upgrade Upgrading a release and override existing values doesn't work as expected for nested values. Maps should be merged recursively, but currently maps are treated just like values and replaced at the top level. If the existing values are: ```yaml resources: requests: cpu: 400m something: else ``` and an update is done with ```--set=resources.requests.cpu=500m```, it currently ends up as ```yaml resources: requests: cpu: 500m ``` but it should have been ```yaml resources: requests: cpu: 500m something: else ``` This PR updates the way override values are merged into the existing set of values to merge rather than replace maps. Closes: #4792 Signed-off-by: Morten Torkildsen --- pkg/chartutil/values.go | 16 ++++++ pkg/chartutil/values_test.go | 82 +++++++++++++++++++++++++++++++ pkg/tiller/release_server.go | 27 +++++----- pkg/tiller/release_update_test.go | 67 +++++++++++++++++++++++++ 4 files changed, 178 insertions(+), 14 deletions(-) diff --git a/pkg/chartutil/values.go b/pkg/chartutil/values.go index 1ea7edd8c..a47073b67 100644 --- a/pkg/chartutil/values.go +++ b/pkg/chartutil/values.go @@ -94,6 +94,22 @@ func (v Values) Encode(w io.Writer) error { return err } +// MergeInto takes the properties in src and merges them into Values. Maps +// are merged while values and arrays are replaced. +func (v Values) MergeInto(src Values) { + for key, srcVal := range src { + destVal, found := v[key] + + if found && istable(srcVal) && istable(destVal) { + destMap := destVal.(map[string]interface{}) + srcMap := srcVal.(map[string]interface{}) + Values(destMap).MergeInto(Values(srcMap)) + } else { + v[key] = srcVal + } + } +} + func tableLookup(v Values, simple string) (Values, error) { v2, ok := v[simple] if !ok { diff --git a/pkg/chartutil/values_test.go b/pkg/chartutil/values_test.go index f54b25827..f38afaf95 100644 --- a/pkg/chartutil/values_test.go +++ b/pkg/chartutil/values_test.go @@ -20,6 +20,7 @@ import ( "bytes" "encoding/json" "fmt" + "reflect" "testing" "text/template" @@ -457,3 +458,84 @@ chapter: } } } + +func TestValuesMergeInto(t *testing.T) { + testCases := map[string]struct { + destination string + source string + result string + }{ + "maps are merged": { + ` +resources: + requests: + cpu: 400m + something: else +`, + ` +resources: + requests: + cpu: 500m +`, + ` +resources: + requests: + cpu: 500m + something: else +`, + }, + "values are replaced": { + ` +firstKey: firstValue +secondKey: secondValue +thirdKey: thirdValue +`, + ` +firstKey: newFirstValue +thirdKey: newThirdValue +`, + ` +firstKey: newFirstValue +secondKey: secondValue +thirdKey: newThirdValue +`, + }, + "new values are added": { + ` +existingKey: existingValue +`, + ` +newKey: newValue +anotherNewKey: + nestedNewKey: nestedNewValue +`, + ` +existingKey: existingValue +newKey: newValue +anotherNewKey: + nestedNewKey: nestedNewValue +`, + }, + } + + for name, tc := range testCases { + d, err := ReadValues([]byte(tc.destination)) + if err != nil { + t.Error(err) + } + s, err := ReadValues([]byte(tc.source)) + if err != nil { + t.Error(err) + } + expectedRes, err := ReadValues([]byte(tc.result)) + if err != nil { + t.Error(err) + } + + d.MergeInto(s) + + if !reflect.DeepEqual(expectedRes, d) { + t.Errorf("%s: Expected %v, but got %v", name, expectedRes, d) + } + } +} diff --git a/pkg/tiller/release_server.go b/pkg/tiller/release_server.go index f5e96ed00..cf7748f44 100644 --- a/pkg/tiller/release_server.go +++ b/pkg/tiller/release_server.go @@ -25,7 +25,6 @@ import ( "strings" "github.com/technosophos/moniker" - "gopkg.in/yaml.v2" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/discovery" "k8s.io/client-go/kubernetes" @@ -136,28 +135,28 @@ func (s *ReleaseServer) reuseValues(req *services.UpdateReleaseRequest, current if err != nil { return err } + req.Chart.Values = &chart.Config{Raw: nv} + + reqValues, err := chartutil.ReadValues([]byte(req.Values.Raw)) + if err != nil { + return err + } - // merge new values with current + currentConfig := chartutil.Values{} if current.Config != nil && current.Config.Raw != "" && current.Config.Raw != "{}\n" { - if req.Values.Raw != "{}\n" { - req.Values.Raw = current.Config.Raw + "\n" + req.Values.Raw - } else { - req.Values.Raw = current.Config.Raw + "\n" + currentConfig, err = chartutil.ReadValues([]byte(current.Config.Raw)) + if err != nil { + return err } } - req.Chart.Values = &chart.Config{Raw: nv} - // yaml unmarshal and marshal to remove duplicate keys - y := map[string]interface{}{} - if err := yaml.Unmarshal([]byte(req.Values.Raw), &y); err != nil { - return err - } - data, err := yaml.Marshal(y) + currentConfig.MergeInto(reqValues) + data, err := currentConfig.YAML() if err != nil { return err } - req.Values.Raw = string(data) + req.Values.Raw = data return nil } diff --git a/pkg/tiller/release_update_test.go b/pkg/tiller/release_update_test.go index 1f189a8b7..fe9ef12eb 100644 --- a/pkg/tiller/release_update_test.go +++ b/pkg/tiller/release_update_test.go @@ -23,10 +23,12 @@ import ( "github.com/golang/protobuf/proto" + "k8s.io/helm/pkg/chartutil" "k8s.io/helm/pkg/helm" "k8s.io/helm/pkg/proto/hapi/chart" "k8s.io/helm/pkg/proto/hapi/release" "k8s.io/helm/pkg/proto/hapi/services" + "reflect" ) func TestUpdateRelease(t *testing.T) { @@ -168,6 +170,71 @@ func TestUpdateRelease_ReuseValuesWithNoValues(t *testing.T) { } } +func TestUpdateRelease_NestedReuseValues(t *testing.T) { + c := helm.NewContext() + rs := rsFixture() + + installReq := &services.InstallReleaseRequest{ + Namespace: "spaced", + Chart: &chart.Chart{ + Metadata: &chart.Metadata{Name: "hello"}, + Templates: []*chart.Template{ + {Name: "templates/hello", Data: []byte("hello: world")}, + }, + Values: &chart.Config{Raw: "defaultFoo: defaultBar"}, + }, + Values: &chart.Config{Raw: ` +foo: bar +root: + nested: nestedValue + anotherNested: anotherNestedValue +`}, + } + + installResp, err := rs.InstallRelease(c, installReq) + if err != nil { + t.Fatal(err) + } + + rel := installResp.Release + req := &services.UpdateReleaseRequest{ + Name: rel.Name, + Chart: &chart.Chart{ + Metadata: &chart.Metadata{Name: "hello"}, + Templates: []*chart.Template{ + {Name: "templates/hello", Data: []byte("hello: world")}, + }, + Values: &chart.Config{Raw: "defaultFoo: defaultBar"}, + }, + Values: &chart.Config{Raw: ` +root: + nested: newNestedValue +`}, + ReuseValues: true, + } + + res, err := rs.UpdateRelease(c, req) + if err != nil { + t.Fatalf("Failed updated: %s", err) + } + + expect, _ := chartutil.ReadValues([]byte(` +foo: bar +root: + nested: newNestedValue + anotherNested: anotherNestedValue +`)) + + requestConfig, err := chartutil.ReadValues([]byte(res.Release.Config.Raw)) + if err != nil { + t.Errorf("Request config could not be parsed: %v", err) + } + + if !reflect.DeepEqual(expect, requestConfig) { + t.Errorf("Expected request config to be %v, got %v", expect, requestConfig) + } +} + // This is a regression test for bug found in issue #3655 func TestUpdateRelease_ComplexReuseValues(t *testing.T) { c := helm.NewContext()