From 16a89f136d3b56cc21a87bdf06e3652d6e078eb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Lambert?= Date: Tue, 1 Sep 2020 10:36:27 +0700 Subject: [PATCH] fix: limit the usage of chartutil.Values to avoid conversion bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the 1st PR splitted from #6876 Golang allows implicit conversion between `map[string]interface{}` and `chartutil.Values`, but not when converted to `interface{}` first. The usage of `chartutil.Values` as values of a root `chartutil.Values` leads to various bugs, only avoided by the usage of `.AsMap` and some random checks. This PR finally gets rid of those random bugs by limitating the usage of `chartutil.Values` and preventing its conversion to `iterface{}`. - All functions now return `map[string]interface{}`. Implicit conversion to `chartutil.Values` makes those changes quite transparent, and avoid accidentally turning `chartutil.Values` to `interface{}`. - All creations of a `chartutil.Values{...}` have been replaced by a `map[string]interface{}{...}`. - In general, `chartutil.Values` are only used in one of those 2 cases: - when `chartutil.Values` methods are used (often as argument of a function, implicitly converted on function call) - when `chartutil.Values` represents the root value for templates (as in `chart.Values` expressions) - `chartutil.Values.AsMap` has been removed to discorage the usage of `chartutil.Values` (implicit conversion works anyway). - Tries to convert `interface{}` to `chartutil.Values` have also been removed to avoid hiding bad usages of `chartutil.Values`. Signed-off-by: Aurélien Lambert --- cmd/helm/status.go | 2 +- pkg/chartutil/coalesce.go | 2 +- pkg/chartutil/coalesce_test.go | 2 +- pkg/chartutil/dependencies.go | 5 +++-- pkg/chartutil/values.go | 41 ++++++++++++++-------------------- pkg/chartutil/values_test.go | 5 ++++- pkg/engine/engine.go | 4 ++-- pkg/engine/engine_test.go | 34 ++++++++++++++-------------- 8 files changed, 46 insertions(+), 49 deletions(-) diff --git a/cmd/helm/status.go b/cmd/helm/status.go index abd32a9c2..4a520e60b 100644 --- a/cmd/helm/status.go +++ b/cmd/helm/status.go @@ -150,7 +150,7 @@ func (s statusPrinter) WriteTable(out io.Writer) error { } fmt.Fprintln(out, "COMPUTED VALUES:") - err = output.EncodeYAML(out, cfg.AsMap()) + err = output.EncodeYAML(out, cfg) if err != nil { return err } diff --git a/pkg/chartutil/coalesce.go b/pkg/chartutil/coalesce.go index 1d3d45e99..4ff957ea0 100644 --- a/pkg/chartutil/coalesce.go +++ b/pkg/chartutil/coalesce.go @@ -34,7 +34,7 @@ import ( // - Scalar values and arrays are replaced, maps are merged // - A chart has access to all of the variables for it, as well as all of // the values destined for its dependencies. -func CoalesceValues(chrt *chart.Chart, vals map[string]interface{}) (Values, error) { +func CoalesceValues(chrt *chart.Chart, vals map[string]interface{}) (map[string]interface{}, error) { v, err := copystructure.Copy(vals) if err != nil { return vals, err diff --git a/pkg/chartutil/coalesce_test.go b/pkg/chartutil/coalesce_test.go index 5a4656d71..13dbb4f6c 100644 --- a/pkg/chartutil/coalesce_test.go +++ b/pkg/chartutil/coalesce_test.go @@ -108,7 +108,7 @@ func TestCoalesceValues(t *testing.T) { // taking a copy of the values before passing it // to CoalesceValues as argument, so that we can // use it for asserting later - valsCopy := make(Values, len(vals)) + valsCopy := make(map[string]interface{}, len(vals)) for key, value := range vals { valsCopy[key] = value } diff --git a/pkg/chartutil/dependencies.go b/pkg/chartutil/dependencies.go index d2e7d6dc9..6fae607f5 100644 --- a/pkg/chartutil/dependencies.go +++ b/pkg/chartutil/dependencies.go @@ -222,6 +222,7 @@ func processImportValues(c *chart.Chart) error { return nil } // combine chart values and empty config to get Values + var cvals Values cvals, err := CoalesceValues(c, nil) if err != nil { return err @@ -248,7 +249,7 @@ func processImportValues(c *chart.Chart) error { continue } // create value map from child to be merged into parent - b = CoalesceTables(cvals, pathToMap(parent, vv.AsMap())) + b = CoalesceTables(cvals, pathToMap(parent, vv)) case string: child := "exports." + iv outiv = append(outiv, map[string]string{ @@ -260,7 +261,7 @@ func processImportValues(c *chart.Chart) error { log.Printf("Warning: ImportValues missing table: %v", err) continue } - b = CoalesceTables(b, vm.AsMap()) + b = CoalesceTables(b, vm) } } // set our formatted import values diff --git a/pkg/chartutil/values.go b/pkg/chartutil/values.go index e1cdf4642..4e618e4ca 100644 --- a/pkg/chartutil/values.go +++ b/pkg/chartutil/values.go @@ -32,6 +32,16 @@ import ( const GlobalKey = "global" // Values represents a collection of chart values. +// +// It should never be stored as an interface{} as conversion to +// map[string]interface{} will then fail (in particular, as value in Values +// or map[string]interface{} structures). +// To avoid this to happen, all function should return map[string]interface{}, +// since implicit conversion to Values is allowed anyway. +// +// Values should only be used in one of these cases: +// - when Values methods are used (return type must be map[string]interface{}) +// - to store the values for template rendering (as in chart.Values) type Values map[string]interface{} // YAML encodes the Values into a YAML string. @@ -52,7 +62,7 @@ func (v Values) YAML() (string, error) { // foo". // // An ErrNoTable is returned if the table does not exist. -func (v Values) Table(name string) (Values, error) { +func (v Values) Table(name string) (map[string]interface{}, error) { table := v var err error @@ -64,16 +74,6 @@ func (v Values) Table(name string) (Values, error) { return table, err } -// AsMap is a utility function for converting Values to a map[string]interface{}. -// -// It protects against nil map panics. -func (v Values) AsMap() map[string]interface{} { - if v == nil || len(v) == 0 { - return map[string]interface{}{} - } - return v -} - // Encode writes serialized Values information to the given io.Writer. func (v Values) Encode(w io.Writer) error { out, err := yaml.Marshal(v) @@ -84,7 +84,7 @@ func (v Values) Encode(w io.Writer) error { return err } -func tableLookup(v Values, simple string) (Values, error) { +func tableLookup(v map[string]interface{}, simple string) (map[string]interface{}, error) { v2, ok := v[simple] if !ok { return v, ErrNoTable{simple} @@ -93,27 +93,20 @@ func tableLookup(v Values, simple string) (Values, error) { return vv, nil } - // This catches a case where a value is of type Values, but doesn't (for some - // reason) match the map[string]interface{}. This has been observed in the - // wild, and might be a result of a nil map of type Values. - if vv, ok := v2.(Values); ok { - return vv, nil - } - - return Values{}, ErrNoTable{simple} + return map[string]interface{}{}, ErrNoTable{simple} } // ReadValues will parse YAML byte data into a Values. -func ReadValues(data []byte) (vals Values, err error) { +func ReadValues(data []byte) (vals map[string]interface{}, err error) { err = yaml.Unmarshal(data, &vals) if len(vals) == 0 { - vals = Values{} + vals = map[string]interface{}{} } return vals, err } // ReadValuesFile will parse a YAML file into a map of values. -func ReadValuesFile(filename string) (Values, error) { +func ReadValuesFile(filename string) (map[string]interface{}, error) { data, err := ioutil.ReadFile(filename) if err != nil { return map[string]interface{}{}, err @@ -134,7 +127,7 @@ type ReleaseOptions struct { // ToRenderValues composes the struct from the data coming from the Releases, Charts and Values files // // This takes both ReleaseOptions and Capabilities to merge into the render values. -func ToRenderValues(chrt *chart.Chart, chrtVals map[string]interface{}, options ReleaseOptions, caps *Capabilities) (Values, error) { +func ToRenderValues(chrt *chart.Chart, chrtVals map[string]interface{}, options ReleaseOptions, caps *Capabilities) (map[string]interface{}, error) { if caps == nil { caps = DefaultCapabilities } diff --git a/pkg/chartutil/values_test.go b/pkg/chartutil/values_test.go index c95fa503a..5338f42be 100644 --- a/pkg/chartutil/values_test.go +++ b/pkg/chartutil/values_test.go @@ -135,7 +135,7 @@ func TestToRenderValues(t *testing.T) { t.Error("Expected Capabilities to have a Kube version") } - vals := res["Values"].(Values) + vals := res["Values"].(map[string]interface{}) if vals["name"] != "Haroun" { t.Errorf("Expected 'Haroun', got %q (%v)", vals["name"], vals) } @@ -171,6 +171,7 @@ chapter: three: title: "The Spouter Inn" ` + var d Values d, err := ReadValues([]byte(doc)) if err != nil { panic(err) @@ -195,6 +196,7 @@ chapter: three: title: "The Spouter Inn" ` + var d Values d, err := ReadValues([]byte(doc)) if err != nil { t.Fatalf("Failed to parse the White Whale: %s", err) @@ -265,6 +267,7 @@ chapter: three: title: "The Spouter Inn" ` + var d Values d, err := ReadValues([]byte(doc)) if err != nil { t.Fatalf("Failed to parse the White Whale: %s", err) diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 5aa0ed8ec..a78e7eafb 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -245,7 +245,7 @@ func (e Engine) renderWithReferences(tpls, referenceTpls map[string]renderable) } // At render time, add information about the template that is being rendered. vals := tpls[filename].vals - vals["Template"] = chartutil.Values{"Name": filename, "BasePath": tpls[filename].basePath} + vals["Template"] = map[string]interface{}{"Name": filename, "BasePath": tpls[filename].basePath} var buf strings.Builder if err := t.ExecuteTemplate(&buf, filename, vals); err != nil { return map[string]string{}, cleanupExecError(filename, err) @@ -340,7 +340,7 @@ func recAllTpls(c *chart.Chart, templates map[string]renderable, vals chartutil. "Files": newFiles(c.Files), "Release": vals["Release"], "Capabilities": vals["Capabilities"], - "Values": make(chartutil.Values), + "Values": map[string]interface{}{}, } // If there is a {{.Values.ThisChart}} in the parent metadata, diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index 87e84c48b..1bb9aa4b2 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -409,7 +409,7 @@ func TestRenderNestedValues(t *testing.T) { inject := chartutil.Values{ "Values": tmp, "Chart": outer.Metadata, - "Release": chartutil.Values{ + "Release": map[string]interface{}{ "Name": "dyin", }, } @@ -464,9 +464,9 @@ func TestRenderBuiltinValues(t *testing.T) { outer.AddDependency(inner) inject := chartutil.Values{ - "Values": "", + "Values": map[string]interface{}{}, "Chart": outer.Metadata, - "Release": chartutil.Values{ + "Release": map[string]interface{}{ "Name": "Aeneid", }, } @@ -510,9 +510,9 @@ func TestAlterFuncMap_include(t *testing.T) { } v := chartutil.Values{ - "Values": "", + "Values": map[string]interface{}{}, "Chart": c.Metadata, - "Release": chartutil.Values{ + "Release": map[string]interface{}{ "Name": "Mistah Kurtz", }, } @@ -544,12 +544,12 @@ func TestAlterFuncMap_require(t *testing.T) { } v := chartutil.Values{ - "Values": chartutil.Values{ + "Values": map[string]interface{}{ "who": "us", "bases": 2, }, "Chart": c.Metadata, - "Release": chartutil.Values{ + "Release": map[string]interface{}{ "Name": "That 90s meme", }, } @@ -571,11 +571,11 @@ func TestAlterFuncMap_require(t *testing.T) { // test required without passing in needed values with lint mode on // verifies lint replaces required with an empty string (should not fail) lintValues := chartutil.Values{ - "Values": chartutil.Values{ + "Values": map[string]interface{}{ "who": "us", }, "Chart": c.Metadata, - "Release": chartutil.Values{ + "Release": map[string]interface{}{ "Name": "That 90s meme", }, } @@ -605,11 +605,11 @@ func TestAlterFuncMap_tpl(t *testing.T) { } v := chartutil.Values{ - "Values": chartutil.Values{ + "Values": map[string]interface{}{ "value": "myvalue", }, "Chart": c.Metadata, - "Release": chartutil.Values{ + "Release": map[string]interface{}{ "Name": "TestRelease", }, } @@ -634,11 +634,11 @@ func TestAlterFuncMap_tplfunc(t *testing.T) { } v := chartutil.Values{ - "Values": chartutil.Values{ + "Values": map[string]interface{}{ "value": "myvalue", }, "Chart": c.Metadata, - "Release": chartutil.Values{ + "Release": map[string]interface{}{ "Name": "TestRelease", }, } @@ -663,11 +663,11 @@ func TestAlterFuncMap_tplinclude(t *testing.T) { }, } v := chartutil.Values{ - "Values": chartutil.Values{ + "Values": map[string]interface{}{ "value": "myvalue", }, "Chart": c.Metadata, - "Release": chartutil.Values{ + "Release": map[string]interface{}{ "Name": "TestRelease", }, } @@ -694,9 +694,9 @@ func TestRenderRecursionLimit(t *testing.T) { }, } v := chartutil.Values{ - "Values": "", + "Values": map[string]interface{}{}, "Chart": c.Metadata, - "Release": chartutil.Values{ + "Release": map[string]interface{}{ "Name": "TestRelease", }, }