Repository: helm/helm (go)

Issue:
Passing json objects/arrays as values through --set
I would like to set  json objects/arrays as values through --set.
According to https://helm.sh/docs/intro/using_helm/#the-format-and-limitations-of---set
currently that is not possible (with the exception of arrays of scalar types) 

As example, let's  take the following excerpt from my values.yaml file:

```
spec:
  ....
  sNssaiUpfInfoList:
  - sNssai:
      sd: 0002f0
      sst: 1
    dnnUpfInfoList:
    - dnn: intranet
      ipv4AddressRanges:
      - end: 9.9.9.94
        start: 9.9.9.65
  ....
```

In the template there is all the "range" based logic for slices and maps, to correctly render sNssaiUpfInfoList from .Values.spec.sNssaiUpfInfoList, so further elements could be added to each of the above arrays and they would appear in the final manifest generated by helm.

In order to set sNssaiUpfInfoList above, currently I have to set the individual leaf attributes of the data structure, by providing the following with --set  on command line:

`'spec.sNssaiUpfInfoList[0].sNssai.sd=0002f0,spec.sNssaiUpfInfoList[0].sNssai.sst=1,spec.sNssaiUpfInfoList[0].dnnUpfInfoList[0].dnn=intranet, spec.sNssaiUpfInfoList[0].dnnUpfInfoList[0].ipv4AddressRanges[0].end=9.9.9.94, spec.sNssaiUpfInfoList[0].dnnUpfInfoList[0].ipv4AddressRanges[0].start=9.9.9.65'
`

This is not very flexible because extra logic is required to translate the data structure into individual key=val for each of its leaf attributes. Moreover, the arrays are dynamic, and quite a long list of key=val could be generated.

I think it would be much simpler if we had an option to set a complete json array/object on the command line.  
For example, to set sNssaiUpfInfoList above, we could use something like:

`--set-json  'spec.sNssaiUpfInfoList=[{"sNssai":{"sd":"0002f0","sst":1},"dnnUpfInfoList":[{"dnn":"intranet","ipv4AddressRanges":[{"end":"9.9.9.94","start":"9.9.9.65"}]}]}]'
`

Is it possible to add this option to helm?



Pull request:
feat: add --set-json flag to set json values.
When used with helm install, helm template, helm upgrade, it enables
to set json values (scalars/objects/arrays) from the command line.

Closes #10428

Signed-off-by: Luca Di Rocco <lucadirocco@gmail.com>

**What this PR does / why we need it**:
This PR allows to set any key of the exposed values (scalars, objects, arrays) of a chart, by using a json value on the command line.
It become easier and more concise to update entire portions of the exposed values, as json data structures on command line, rather than setting the value of each individual leaf attribute of the data structure (#10428 provides an example) 

**Special notes for your reviewer**:
The implementation has tried to reuse as much as possible the existing code and the standard libraries, and is quite straightforward.
There are some obvious changes in cmd/helm/flags.go and pkg/cli/values/options.go in order to include the new option flag (--set-json)
The parser logic from pkg/strvals/parser.go is reused, with an extra field in the parser struct (isjsonval bool, true only for --set-json) and some extra code in the following two functions, to parse a json value when a "=" char is met:
func (t *parser) key(data map[string]interface{}) (reterr error)
func (t *parser) listItem(list []interface{}, i int) ([]interface{}, error) 

**If applicable**:
- [X] this PR contains documentation  
        Little updates in the description constants of cmd/helm/install.go and cmd/helm/upgrade.go to.
        Together with the info from cmd/helm/flags.go, this should update the documentation when "helm docs --type markdown --generate-headers" is executed.
- [X] this PR contains unit tests
        Some extra unit tests with function TestParseJSON in pkg/strvals/parser_test.go
- [X] this PR has been tested for backwards compatibility
        All unit test run, I don't know if something else is required. Being this a new feature, there are no changes in existing functionalities.
        Please note that I had also to remove the ending new line '\n' from an errors.Errorf statement in the  
func compare(actual []byte, filename string) error, in file ./internal/test/test.go, because go lint was failing and also tests were not running.


Patch:
diff --git a/cmd/helm/flags.go b/cmd/helm/flags.go
index 0cc0564e2..76d6e0476 100644
--- a/cmd/helm/flags.go
+++ b/cmd/helm/flags.go
@@ -47,6 +47,7 @@ func addValueOptionsFlags(f *pflag.FlagSet, v *values.Options) {
 	f.StringArrayVar(&v.Values, "set", []string{}, "set values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)")
 	f.StringArrayVar(&v.StringValues, "set-string", []string{}, "set STRING values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2)")
 	f.StringArrayVar(&v.FileValues, "set-file", []string{}, "set values from respective files specified via the command line (can specify multiple or separate values with commas: key1=path1,key2=path2)")
+	f.StringArrayVar(&v.JSONValues, "set-json", []string{}, "set JSON values on the command line (can specify multiple or separate values with commas: key1=jsonval1,key2=jsonval2)")
 }
 
 func addChartPathOptionsFlags(f *pflag.FlagSet, c *action.ChartPathOptions) {
diff --git a/cmd/helm/install.go b/cmd/helm/install.go
index 0e63ab3a5..9ccffed4b 100644
--- a/cmd/helm/install.go
+++ b/cmd/helm/install.go
@@ -51,7 +51,8 @@ To override values in a chart, use either the '--values' flag and pass in a file
 or use the '--set' flag and pass configuration from the command line, to force
 a string value use '--set-string'. You can use '--set-file' to set individual
 values from a file when the value itself is too long for the command line
-or is dynamically generated.
+or is dynamically generated. You can also use '--set-json' to set json values
+(scalars/objects/arrays) from the command line.
 
     $ helm install -f myvalues.yaml myredis ./redis
 
@@ -67,6 +68,11 @@ or
 
     $ helm install --set-file my_script=dothings.sh myredis ./redis
 
+or
+
+    $ helm install --set-json 'master.sidecars=[{"name":"sidecar","image":"myImage","imagePullPolicy":"Always","ports":[{"name":"portname","containerPort":1234}]}]' myredis ./redis
+
+
 You can specify the '--values'/'-f' flag multiple times. The priority will be given to the
 last (right-most) file specified. For example, if both myvalues.yaml and override.yaml
 contained a key called 'Test', the value set in override.yaml would take precedence:
@@ -79,6 +85,13 @@ set for a key called 'foo', the 'newbar' value would take precedence:
 
     $ helm install --set foo=bar --set foo=newbar  myredis ./redis
 
+Similarly, in the following example 'foo' is set to '["four"]': 
+
+    $ helm install --set-json='foo=["one", "two", "three"]' --set-json='foo=["four"]' myredis ./redis
+
+And in the following example, 'foo' is set to '{"key1":"value1","key2":"bar"}':
+
+    $ helm install --set-json='foo={"key1":"value1","key2":"value2"}' --set-json='foo.key2="bar"' myredis ./redis
 
 To check the generated manifests of a release without installing the chart,
 the '--debug' and '--dry-run' flags can be combined.
diff --git a/cmd/helm/upgrade.go b/cmd/helm/upgrade.go
index 7ada8e3b1..7aed385bb 100644
--- a/cmd/helm/upgrade.go
+++ b/cmd/helm/upgrade.go
@@ -51,7 +51,8 @@ To override values in a chart, use either the '--values' flag and pass in a file
 or use the '--set' flag and pass configuration from the command line, to force string
 values, use '--set-string'. You can use '--set-file' to set individual
 values from a file when the value itself is too long for the command line
-or is dynamically generated.
+or is dynamically generated. You can also use '--set-json' to set json values
+(scalars/objects/arrays) from the command line.
 
 You can specify the '--values'/'-f' flag multiple times. The priority will be given to the
 last (right-most) file specified. For example, if both myvalues.yaml and override.yaml
diff --git a/internal/resolver/resolver.go b/internal/resolver/resolver.go
index 5e8921f96..c9472dfb4 100644
--- a/internal/resolver/resolver.go
+++ b/internal/resolver/resolver.go
@@ -135,36 +135,22 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string
 			found = false
 		} else {
 			version = d.Version
+			// Retrieve list of tags for repository
+			ref := fmt.Sprintf("%s/%s", strings.TrimPrefix(d.Repository, fmt.Sprintf("%s://", registry.OCIScheme)), d.Name)
+			tags, err := r.registryClient.Tags(ref)
+			if err != nil {
+				return nil, errors.Wrapf(err, "could not retrieve list of tags for repository %s", d.Repository)
+			}
 
-			// Check to see if an explicit version has been provided
-			_, err := semver.NewVersion(version)
-
-			// Use an explicit version, otherwise search for tags
-			if err == nil {
-				vs = []*repo.ChartVersion{{
+			vs = make(repo.ChartVersions, len(tags))
+			for ti, t := range tags {
+				// Mock chart version objects
+				version := &repo.ChartVersion{
 					Metadata: &chart.Metadata{
-						Version: version,
+						Version: t,
 					},
-				}}
-
-			} else {
-				// Retrieve list of tags for repository
-				ref := fmt.Sprintf("%s/%s", strings.TrimPrefix(d.Repository, fmt.Sprintf("%s://", registry.OCIScheme)), d.Name)
-				tags, err := r.registryClient.Tags(ref)
-				if err != nil {
-					return nil, errors.Wrapf(err, "could not retrieve list of tags for repository %s", d.Repository)
-				}
-
-				vs = make(repo.ChartVersions, len(tags))
-				for ti, t := range tags {
-					// Mock chart version objects
-					version := &repo.ChartVersion{
-						Metadata: &chart.Metadata{
-							Version: t,
-						},
-					}
-					vs[ti] = version
 				}
+				vs[ti] = version
 			}
 		}
 
diff --git a/pkg/cli/values/options.go b/pkg/cli/values/options.go
index e6ad71767..fea25185b 100644
--- a/pkg/cli/values/options.go
+++ b/pkg/cli/values/options.go
@@ -34,6 +34,7 @@ type Options struct {
 	StringValues []string
 	Values       []string
 	FileValues   []string
+	JSONValues   []string
 }
 
 // MergeValues merges values from files specified via -f/--values and directly
@@ -57,6 +58,13 @@ func (opts *Options) MergeValues(p getter.Providers) (map[string]interface{}, er
 		base = mergeMaps(base, currentMap)
 	}
 
+	// User specified a value via --set-json
+	for _, value := range opts.JSONValues {
+		if err := strvals.ParseJSON(value, base); err != nil {
+			return nil, errors.Errorf("failed parsing --set-json data %s", value)
+		}
+	}
+
 	// User specified a value via --set
 	for _, value := range opts.Values {
 		if err := strvals.ParseInto(value, base); err != nil {
diff --git a/pkg/strvals/parser.go b/pkg/strvals/parser.go
index 457b99f94..fb5df33e0 100644
--- a/pkg/strvals/parser.go
+++ b/pkg/strvals/parser.go
@@ -17,10 +17,13 @@ package strvals
 
 import (
 	"bytes"
+	"encoding/json"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"strconv"
 	"strings"
+	"unicode"
 
 	"github.com/pkg/errors"
 	"sigs.k8s.io/yaml"
@@ -94,6 +97,18 @@ func ParseIntoString(s string, dest map[string]interface{}) error {
 	return t.parse()
 }
 
+// ParseJSON parses a string with format key1=val1, key2=val2, ...
+// where values are json strings (null, or scalars, or arrays, or objects).
+// An empty val is treated as null.
+//
+// If a key exists in dest, the new value overwrites the dest version.
+//
+func ParseJSON(s string, dest map[string]interface{}) error {
+	scanner := bytes.NewBufferString(s)
+	t := newJSONParser(scanner, dest)
+	return t.parse()
+}
+
 // ParseIntoFile parses a filevals line and merges the result into dest.
 //
 // This method always returns a string as the value.
@@ -113,9 +128,10 @@ type RunesValueReader func([]rune) (interface{}, error)
 // where sc is the source of the original data being parsed
 // where data is the final parsed data from the parses with correct types
 type parser struct {
-	sc     *bytes.Buffer
-	data   map[string]interface{}
-	reader RunesValueReader
+	sc        *bytes.Buffer
+	data      map[string]interface{}
+	reader    RunesValueReader
+	isjsonval bool
 }
 
 func newParser(sc *bytes.Buffer, data map[string]interface{}, stringBool bool) *parser {
@@ -125,6 +141,10 @@ func newParser(sc *bytes.Buffer, data map[string]interface{}, stringBool bool) *
 	return &parser{sc: sc, data: data, reader: stringConverter}
 }
 
+func newJSONParser(sc *bytes.Buffer, data map[string]interface{}) *parser {
+	return &parser{sc: sc, data: data, reader: nil, isjsonval: true}
+}
+
 func newFileParser(sc *bytes.Buffer, data map[string]interface{}, reader RunesValueReader) *parser {
 	return &parser{sc: sc, data: data, reader: reader}
 }
@@ -184,6 +204,33 @@ func (t *parser) key(data map[string]interface{}) (reterr error) {
 			set(data, kk, list)
 			return err
 		case last == '=':
+			if t.isjsonval {
+				empval, err := t.emptyVal()
+				if err != nil {
+					return err
+				}
+				if empval {
+					set(data, string(k), nil)
+					return nil
+				}
+				// parse jsonvals by using Go’s JSON standard library
+				// Decode is preferred to Unmarshal in order to parse just the json parts of the list key1=jsonval1,key2=jsonval2,...
+				// Since Decode has its own buffer that consumes more characters (from underlying t.sc) than the ones actually decoded,
+				// we invoke Decode on a separate reader built with a copy of what is left in t.sc. After Decode is executed, we
+				// discard in t.sc the chars of the decoded json value (the number of those characters is returned by InputOffset).
+				var jsonval interface{}
+				dec := json.NewDecoder(strings.NewReader(t.sc.String()))
+				if err = dec.Decode(&jsonval); err != nil {
+					return err
+				}
+				set(data, string(k), jsonval)
+				if _, err = io.CopyN(ioutil.Discard, t.sc, dec.InputOffset()); err != nil {
+					return err
+				}
+				// skip possible blanks and comma
+				_, err = t.emptyVal()
+				return err
+			}
 			//End of key. Consume =, Get value.
 			// FIXME: Get value list first
 			vl, e := t.valList()
@@ -205,7 +252,6 @@ func (t *parser) key(data map[string]interface{}) (reterr error) {
 			default:
 				return e
 			}
-
 		case last == ',':
 			// No value given. Set the value to empty string. Return error.
 			set(data, string(k), "")
@@ -280,6 +326,34 @@ func (t *parser) listItem(list []interface{}, i int) ([]interface{}, error) {
 	case err != nil:
 		return list, err
 	case last == '=':
+		if t.isjsonval {
+			empval, err := t.emptyVal()
+			if err != nil {
+				return list, err
+			}
+			if empval {
+				return setIndex(list, i, nil)
+			}
+			// parse jsonvals by using Go’s JSON standard library
+			// Decode is preferred to Unmarshal in order to parse just the json parts of the list key1=jsonval1,key2=jsonval2,...
+			// Since Decode has its own buffer that consumes more characters (from underlying t.sc) than the ones actually decoded,
+			// we invoke Decode on a separate reader built with a copy of what is left in t.sc. After Decode is executed, we
+			// discard in t.sc the chars of the decoded json value (the number of those characters is returned by InputOffset).
+			var jsonval interface{}
+			dec := json.NewDecoder(strings.NewReader(t.sc.String()))
+			if err = dec.Decode(&jsonval); err != nil {
+				return list, err
+			}
+			if list, err = setIndex(list, i, jsonval); err != nil {
+				return list, err
+			}
+			if _, err = io.CopyN(ioutil.Discard, t.sc, dec.InputOffset()); err != nil {
+				return list, err
+			}
+			// skip possible blanks and comma
+			_, err = t.emptyVal()
+			return list, err
+		}
 		vl, e := t.valList()
 		switch e {
 		case nil:
@@ -343,6 +417,28 @@ func (t *parser) listItem(list []interface{}, i int) ([]interface{}, error) {
 	}
 }
 
+// check for an empty value
+// read and consume optional spaces until comma or EOF (empty val) or any other char (not empty val)
+// comma and spaces are consumed, while any other char is not cosumed
+func (t *parser) emptyVal() (bool, error) {
+	for {
+		r, _, e := t.sc.ReadRune()
+		if e == io.EOF {
+			return true, nil
+		}
+		if e != nil {
+			return false, e
+		}
+		if r == ',' {
+			return true, nil
+		}
+		if !unicode.IsSpace(r) {
+			t.sc.UnreadRune()
+			return false, nil
+		}
+	}
+}
+
 func (t *parser) val() ([]rune, error) {
 	stop := runeSet([]rune{','})
 	v, _, err := runesUntil(t.sc, stop)


Test patch:
diff --git a/internal/test/test.go b/internal/test/test.go
index 646037606..e4f7f639a 100644
--- a/internal/test/test.go
+++ b/internal/test/test.go
@@ -88,7 +88,7 @@ func compare(actual []byte, filename string) error {
 	}
 	expected = normalize(expected)
 	if !bytes.Equal(expected, actual) {
-		return errors.Errorf("does not match golden file %s\n\nWANT:\n'%s'\n\nGOT:\n'%s'\n", filename, expected, actual)
+		return errors.Errorf("does not match golden file %s\n\nWANT:\n'%s'\n\nGOT:\n'%s'", filename, expected, actual)
 	}
 	return nil
 }
diff --git a/pkg/strvals/parser_test.go b/pkg/strvals/parser_test.go
index cef98ba0a..57623eae5 100644
--- a/pkg/strvals/parser_test.go
+++ b/pkg/strvals/parser_test.go
@@ -567,6 +567,107 @@ func TestParseIntoString(t *testing.T) {
 	}
 }
 
+func TestParseJSON(t *testing.T) {
+	tests := []struct {
+		input  string
+		got    map[string]interface{}
+		expect map[string]interface{}
+		err    bool
+	}{
+		{ // set json scalars values, and replace one existing key
+			input: "outer.inner1=\"1\",outer.inner3=3,outer.inner4=true,outer.inner5=\"true\"",
+			got: map[string]interface{}{
+				"outer": map[string]interface{}{
+					"inner1": "overwrite",
+					"inner2": "value2",
+				},
+			},
+			expect: map[string]interface{}{
+				"outer": map[string]interface{}{
+					"inner1": "1",
+					"inner2": "value2",
+					"inner3": 3,
+					"inner4": true,
+					"inner5": "true",
+				},
+			},
+			err: false,
+		},
+		{ // set json objects and arrays, and replace one existing key
+			input: "outer.inner1={\"a\":\"1\",\"b\":2,\"c\":[1,2,3]},outer.inner3=[\"new value 1\",\"new value 2\"],outer.inner4={\"aa\":\"1\",\"bb\":2,\"cc\":[1,2,3]},outer.inner5=[{\"A\":\"1\",\"B\":2,\"C\":[1,2,3]}]",
+			got: map[string]interface{}{
+				"outer": map[string]interface{}{
+					"inner1": map[string]interface{}{
+						"x": "overwrite",
+					},
+					"inner2": "value2",
+					"inner3": []interface{}{
+						"overwrite",
+					},
+				},
+			},
+			expect: map[string]interface{}{
+				"outer": map[string]interface{}{
+					"inner1": map[string]interface{}{"a": "1", "b": 2, "c": []interface{}{1, 2, 3}},
+					"inner2": "value2",
+					"inner3": []interface{}{"new value 1", "new value 2"},
+					"inner4": map[string]interface{}{"aa": "1", "bb": 2, "cc": []interface{}{1, 2, 3}},
+					"inner5": []interface{}{map[string]interface{}{"A": "1", "B": 2, "C": []interface{}{1, 2, 3}}},
+				},
+			},
+			err: false,
+		},
+		{ // null assigment, and no value assigned (equivalent to null)
+			input: "outer.inner1=,outer.inner3={\"aa\":\"1\",\"bb\":2,\"cc\":[1,2,3]},outer.inner3.cc[1]=null",
+			got: map[string]interface{}{
+				"outer": map[string]interface{}{
+					"inner1": map[string]interface{}{
+						"x": "overwrite",
+					},
+					"inner2": "value2",
+				},
+			},
+			expect: map[string]interface{}{
+				"outer": map[string]interface{}{
+					"inner1": nil,
+					"inner2": "value2",
+					"inner3": map[string]interface{}{"aa": "1", "bb": 2, "cc": []interface{}{1, nil, 3}},
+				},
+			},
+			err: false,
+		},
+		{ // syntax error
+			input:  "outer.inner1={\"a\":\"1\",\"b\":2,\"c\":[1,2,3]},outer.inner3=[\"new value 1\",\"new value 2\"],outer.inner4={\"aa\":\"1\",\"bb\":2,\"cc\":[1,2,3]},outer.inner5={\"A\":\"1\",\"B\":2,\"C\":[1,2,3]}]",
+			got:    nil,
+			expect: nil,
+			err:    true,
+		},
+	}
+	for _, tt := range tests {
+		if err := ParseJSON(tt.input, tt.got); err != nil {
+			if tt.err {
+				continue
+			}
+			t.Fatalf("%s: %s", tt.input, err)
+		}
+		if tt.err {
+			t.Fatalf("%s: Expected error. Got nil", tt.input)
+		}
+		y1, err := yaml.Marshal(tt.expect)
+		if err != nil {
+			t.Fatalf("Error serializing expected value: %s", err)
+		}
+		y2, err := yaml.Marshal(tt.got)
+		if err != nil {
+			t.Fatalf("Error serializing parsed value: %s", err)
+		}
+
+		if string(y1) != string(y2) {
+			t.Errorf("%s: Expected:\n%s\nGot:\n%s", tt.input, y1, y2)
+		}
+	}
+}
+
 func TestParseFile(t *testing.T) {
 	input := "name1=path1"
 	expect := map[string]interface{}{