From 5b14ce094e8fcfbbcd1301c5ef9b6a5893fb5b77 Mon Sep 17 00:00:00 2001 From: Michelle Noorali Date: Wed, 4 Apr 2018 15:44:28 -0400 Subject: [PATCH 1/2] fix(pkg/tiller): reuseValues combines all prev val Resolves #3655 We were seeing that when running helm upgrade with the reuse-values flag enabled that you could end up in the position where overrides a.k.a computed values from previous revisions were not being saved on the updated revision. This left us in a weird position where some computed values would disappear mysteriously in the abyss. That happened because computed values from previous revisions weren't merged with the new computed values every time the reuse-values flag was used. This PR merges computed values from the previous revisions so you don't end up in that kind of conundrum. --- pkg/tiller/release_server.go | 16 +++++ pkg/tiller/release_update_test.go | 106 +++++++++++++++++++++++++++++- 2 files changed, 120 insertions(+), 2 deletions(-) diff --git a/pkg/tiller/release_server.go b/pkg/tiller/release_server.go index a96c64938..c72107026 100644 --- a/pkg/tiller/release_server.go +++ b/pkg/tiller/release_server.go @@ -25,6 +25,7 @@ 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/kubernetes/pkg/client/clientset_generated/internalclientset" @@ -135,7 +136,22 @@ func (s *ReleaseServer) reuseValues(req *services.UpdateReleaseRequest, current if err != nil { return err } + + // merge new values with current + req.Values.Raw = current.Config.Raw + "\n" + req.Values.Raw 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) + if err != nil { + return err + } + + req.Values.Raw = string(data) return nil } diff --git a/pkg/tiller/release_update_test.go b/pkg/tiller/release_update_test.go index 642952f19..0189201d1 100644 --- a/pkg/tiller/release_update_test.go +++ b/pkg/tiller/release_update_test.go @@ -17,6 +17,7 @@ limitations under the License. package tiller import ( + "fmt" "strings" "testing" @@ -128,6 +129,107 @@ func TestUpdateRelease_ResetValues(t *testing.T) { } } +// This is a regression test for bug found in issue #3655 +func TestUpdateRelease_ComplexReuseValues(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")}, + {Name: "templates/hooks", Data: []byte(manifestWithHook)}, + }, + Values: &chart.Config{Raw: "defaultFoo: defaultBar"}, + }, + Values: &chart.Config{Raw: "foo: bar"}, + } + + fmt.Println("Running Install release with foo: bar override") + 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")}, + {Name: "templates/hooks", Data: []byte(manifestWithUpgradeHooks)}, + }, + Values: &chart.Config{Raw: "defaultFoo: defaultBar"}, + }, + } + + fmt.Println("Running Update release with no overrides and no reuse-values flag") + res, err := rs.UpdateRelease(c, req) + if err != nil { + t.Fatalf("Failed updated: %s", err) + } + + expect := "foo: bar" + if res.Release.Config != nil && res.Release.Config.Raw != expect { + t.Errorf("Expected chart values to be %q, got %q", expect, res.Release.Config.Raw) + } + + rel = res.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")}, + {Name: "templates/hooks", Data: []byte(manifestWithUpgradeHooks)}, + }, + Values: &chart.Config{Raw: "defaultFoo: defaultBar"}, + }, + Values: &chart.Config{Raw: "foo2: bar2"}, + ReuseValues: true, + } + + fmt.Println("Running Update release with foo2: bar2 override and reuse-values") + res, err = rs.UpdateRelease(c, req) + if err != nil { + t.Fatalf("Failed updated: %s", err) + } + + // This should have the newly-passed overrides. + expect = "foo: bar\nfoo2: bar2\n" + if res.Release.Config != nil && res.Release.Config.Raw != expect { + t.Errorf("Expected request config to be %q, got %q", expect, res.Release.Config.Raw) + } + + rel = res.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")}, + {Name: "templates/hooks", Data: []byte(manifestWithUpgradeHooks)}, + }, + Values: &chart.Config{Raw: "defaultFoo: defaultBar"}, + }, + Values: &chart.Config{Raw: "foo: baz"}, + ReuseValues: true, + } + + fmt.Println("Running Update release with foo=baz override with reuse-values flag") + res, err = rs.UpdateRelease(c, req) + if err != nil { + t.Fatalf("Failed updated: %s", err) + } + expect = "foo: baz\nfoo2: bar2\n" + if res.Release.Config != nil && res.Release.Config.Raw != expect { + t.Errorf("Expected chart values to be %q, got %q", expect, res.Release.Config.Raw) + } +} + func TestUpdateRelease_ReuseValues(t *testing.T) { c := helm.NewContext() rs := rsFixture() @@ -157,8 +259,8 @@ func TestUpdateRelease_ReuseValues(t *testing.T) { if res.Release.Chart.Values != nil && res.Release.Chart.Values.Raw != expect { t.Errorf("Expected chart values to be %q, got %q", expect, res.Release.Chart.Values.Raw) } - // This should have the newly-passed overrides. - expect = "name2: val2" + // This should have the newly-passed overrides and any other computed values. `name: value` comes from release Config via releaseStub() + expect = "name: value\nname2: val2" if res.Release.Config != nil && res.Release.Config.Raw != expect { t.Errorf("Expected request config to be %q, got %q", expect, res.Release.Config.Raw) } From 8455b12869ccc483f2b165184a1b765045031562 Mon Sep 17 00:00:00 2001 From: Michelle Noorali Date: Wed, 4 Apr 2018 15:54:12 -0400 Subject: [PATCH 2/2] ref(pkg/tiller): clarify reuseValues comment --- pkg/tiller/release_update.go | 2 +- pkg/tiller/release_update_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/tiller/release_update.go b/pkg/tiller/release_update.go index cb8b57792..6f5d37331 100644 --- a/pkg/tiller/release_update.go +++ b/pkg/tiller/release_update.go @@ -80,7 +80,7 @@ func (s *ReleaseServer) prepareUpdate(req *services.UpdateReleaseRequest) (*rele return nil, nil, err } - // If new values were not supplied in the upgrade, re-use the existing values. + // determine if values will be reused if err := s.reuseValues(req, currentRelease); err != nil { return nil, nil, err } diff --git a/pkg/tiller/release_update_test.go b/pkg/tiller/release_update_test.go index 0189201d1..a1b9a4bff 100644 --- a/pkg/tiller/release_update_test.go +++ b/pkg/tiller/release_update_test.go @@ -260,7 +260,7 @@ func TestUpdateRelease_ReuseValues(t *testing.T) { t.Errorf("Expected chart values to be %q, got %q", expect, res.Release.Chart.Values.Raw) } // This should have the newly-passed overrides and any other computed values. `name: value` comes from release Config via releaseStub() - expect = "name: value\nname2: val2" + expect = "name: value\nname2: val2\n" if res.Release.Config != nil && res.Release.Config.Raw != expect { t.Errorf("Expected request config to be %q, got %q", expect, res.Release.Config.Raw) }