From b2e052d4e7899c79edc192d72d7c11807e66c8b0 Mon Sep 17 00:00:00 2001 From: Morten Torkildsen Date: Mon, 12 Nov 2018 03:32:21 -0800 Subject: [PATCH] fix(helm): Merge nested values correctly on upgrade (#4806) 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 81fad0bc4..ea1c88f62 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()