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 <mortent@google.com>

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 {

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)
+		}
+	}
+}

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" {
+			currentConfig, err = chartutil.ReadValues([]byte(current.Config.Raw))
-				req.Values.Raw = current.Config.Raw + "\n" + req.Values.Raw
+			if err != nil {
-			} else {
+				return err
-				req.Values.Raw = current.Config.Raw + "\n"
 			}
 		}
-		req.Chart.Values = &chart.Config{Raw: nv}
 
-		// yaml unmarshal and marshal to remove duplicate keys
+		currentConfig.MergeInto(reqValues)
-		y := map[string]interface{}{}
+		data, err := currentConfig.YAML()
-		if err := yaml.Unmarshal([]byte(req.Values.Raw), &y); err != nil {
-			return err
-		}
-		data, err := yaml.Marshal(y)
 		if err != nil {
 			return err
 		}
 
-		req.Values.Raw = string(data)
+		req.Values.Raw = data
 		return nil
 	}
 

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()