fix: limit the usage of chartutil.Values to avoid conversion bugs

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 <aure@olli-ai.com>

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
 		}

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

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
 	}

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

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
 	}

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)

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,

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",
 		},
 	}