fix: support escaped commas in --state-values-set (#2813)

* feat: support comma escape for values overwrites Signed-off-by: zhengbayi <zhengbaiyi@sensetime.com> * fix: harden state-values-set parsing and support \, in state-values-set-string - Return a descriptive error instead of panicking when a --state-values-set assignment is missing '=', has an empty key, or ends with a trailing comma - Support escaped commas in --state-values-set-string and preserve other backslashes (including trailing ones), aligning it with --state-values-set and Helm's --set/--set-string; quoted values keep working - Allow empty values in --state-values-set-string for parity with --state-values-set - Restructure parsing into guard-clause helpers with single-level conditionals; parse assignments via strings.Cut - Extend table-driven tests: error cases, --state-values-set-string coverage, combined flags, and unit tests for the split/unescape helpers Signed-off-by: yxxhero <aiopsclub@163.com> --------- Signed-off-by: zhengbayi <zhengbaiyi@sensetime.com> Signed-off-by: yxxhero <aiopsclub@163.com> Co-authored-by: zhengbayi <zhengbaiyi@sensetime.com> Co-authored-by: yxxhero <aiopsclub@163.com>
2026-10-04 23:08:51 +02:00 · 2026-10-03 15:31:04 +08:00
parent 226c16b88e
commit 000a6fa660
4 changed files with 394 additions and 31 deletions
@@ -154,8 +154,8 @@ func setGlobalOptionsForRootCmd(fs *pflag.FlagSet, globalOptions *config.GlobalO
 	fs.StringVarP(&globalOptions.KustomizeBinary, "kustomize-binary", "k", "", fmt.Sprintf(`Path to the kustomize binary. Overrides "HELMFILE_KUSTOMIZE_BINARY" OS environment variable when specified (default %q)`, app.DefaultKustomizeBinary))
 	fs.StringVarP(&globalOptions.File, "file", "f", "", "load config from file or directory. defaults to \"`helmfile.yaml`\" or \"helmfile.yaml.gotmpl\" or \"helmfile.d\" (means \"helmfile.d/*.yaml\" or \"helmfile.d/*.yaml.gotmpl\") in this preference. Specify - to load the config from the standard input.")
 	fs.StringVarP(&globalOptions.Environment, "environment", "e", "", `specify the environment name. Overrides "HELMFILE_ENVIRONMENT" OS environment variable when specified. defaults to "default"`)
-	fs.StringArrayVar(&globalOptions.StateValuesSet, "state-values-set", nil, "set state values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2). Used to override .Values within the helmfile template (not values template).")
-	fs.StringArrayVar(&globalOptions.StateValuesSetString, "state-values-set-string", nil, "set state STRING values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2). Used to override .Values within the helmfile template (not values template).")
+	fs.StringArrayVar(&globalOptions.StateValuesSet, "state-values-set", nil, "set state values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2; escape a comma in a value with \\,). Used to override .Values within the helmfile template (not values template).")
+	fs.StringArrayVar(&globalOptions.StateValuesSetString, "state-values-set-string", nil, "set state STRING values on the command line (can specify multiple or separate values with commas: key1=val1,key2=val2; escape a comma in a value with \\,). Used to override .Values within the helmfile template (not values template).")
 	fs.StringArrayVar(&globalOptions.StateValuesFile, "state-values-file", nil, "specify state values in a YAML file. Used to override .Values within the helmfile template (not values template).")
 	fs.BoolVar(&globalOptions.SkipDeps, "skip-deps", false, `skip running "helm repo update" and "helm dependency build"`)
 	fs.BoolVar(&globalOptions.SkipRefresh, "skip-refresh", false, `skip running "helm repo update"`)
@@ -76,6 +76,30 @@ Use "helmfile [command] --help" for more information about a command.

 **Note:** Each command has its own specific flags. Use `helmfile [command] --help` to see command-specific options. For example, `helmfile sync --help` shows operational flags like `--timeout`, `--wait`, and `--wait-for-jobs`.

+### State value overrides
+
+Use `--state-values-set` to override `.Values` within the Helmfile template. Separate
+assignments with commas, and escape a comma inside a value with `\,`:
+
+```bash
+helmfile --state-values-set 'message=hello\,world,replicas=2' build
+```
+
+This sets `message` to the string `hello,world` and `replicas` to the number `2`.
+Single quotes preserve the backslash when passing the argument through the shell.
+Backslashes before other characters and at the end of a value are preserved.
+
+`--state-values-set-string` accepts the same syntax but keeps every value as a
+string without type conversion. Its values may additionally be wrapped in
+single or double quotes, which allows commas without escaping:
+
+```bash
+helmfile --state-values-set-string 'zone="zone1,zone2",imageTag=1.23.3' build
+```
+
+Malformed assignments (missing `=`, empty key, or a trailing comma) are reported
+as errors instead of being partially applied.
+
 ### init

 The `helmfile init` sub-command checks the dependencies required for helmfile operation, such as `helm`, `helm diff plugin`, `helm secrets plugin`, `helm helm-git plugin`, `helm s3 plugin`. When it does not exist or the version is too low, it can be installed automatically.
@@ -1,44 +1,141 @@
 package config

 import (
+	"fmt"
 	"regexp"
 	"strings"

 	"github.com/helmfile/helmfile/pkg/maputil"
 )

+// stateValuesSetStringRE matches a single key=value assignment within one
+// --state-values-set-string flag value. The value is either a quoted string,
+// which may contain commas, or a run of plain or backslash-escaped characters
+// ending at the next unescaped comma.
+var stateValuesSetStringRE = regexp.MustCompile(`(?s)(?:,|^)([^\s=]+)=(['"][^'"]*['"]|(?:\\.|[^,\\])*\\?)`)
+
+// NewCLIConfigImpl parses the raw --state-values-set-string and
+// --state-values-set flag values into state value overrides on g.
 func NewCLIConfigImpl(g *GlobalImpl) error {
-	re := regexp.MustCompile(`(?:,|^)([^\s=]+)=(['"][^'"]*['"]|[^,]+)`)
+	if err := parseStateValuesSetString(g); err != nil {
+		return err
+	}
+	return parseStateValuesSet(g)
+}
+
+// parseStateValuesSetString applies the --state-values-set-string flag values
+// to g. Unlike --state-values-set, values are kept as strings without type
+// conversion.
+func parseStateValuesSetString(g *GlobalImpl) error {
 	optsSet := g.RawStateValuesSetString()
-	if len(optsSet) > 0 {
-		set := map[string]any{}
-		for i := range optsSet {
-			ops := re.FindAllStringSubmatch(optsSet[i], -1)
-			for j := range ops {
-				op := ops[j]
-				k := maputil.ParseKey(op[1])
-				v := op[2]
-
-				maputil.Set(set, k, v, true)
-			}
-		}
-		g.SetSet(set)
-	}
-	optsSet = g.RawStateValuesSet()
-	if len(optsSet) > 0 {
-		set := map[string]any{}
-		for i := range optsSet {
-			ops := strings.Split(optsSet[i], ",")
-			for j := range ops {
-				op := strings.SplitN(ops[j], "=", 2)
-				k := maputil.ParseKey(op[0])
-				v := op[1]
-
-				maputil.Set(set, k, v, false)
-			}
-		}
-		g.SetSet(set)
+	if len(optsSet) == 0 {
+		return nil
 	}

+	set := map[string]any{}
+	for _, raw := range optsSet {
+		for _, match := range stateValuesSetStringRE.FindAllStringSubmatch(raw, -1) {
+			key, value := match[1], match[2]
+			maputil.Set(set, maputil.ParseKey(key), unescapeCommas(value), true)
+		}
+	}
+	g.SetSet(set)
 	return nil
 }
+
+// parseStateValuesSet applies the --state-values-set flag values to g. Values
+// are type-converted like Helm's --set.
+func parseStateValuesSet(g *GlobalImpl) error {
+	optsSet := g.RawStateValuesSet()
+	if len(optsSet) == 0 {
+		return nil
+	}
+
+	set := map[string]any{}
+	for _, raw := range optsSet {
+		for _, assignment := range splitOnUnescapedCommas(raw) {
+			if err := setAssignment(set, assignment); err != nil {
+				return err
+			}
+		}
+	}
+	g.SetSet(set)
+	return nil
+}
+
+// setAssignment parses a single key=value assignment into set, returning an
+// error instead of panicking when the assignment is malformed.
+func setAssignment(set map[string]any, assignment string) error {
+	key, value, found := strings.Cut(unescapeCommas(assignment), "=")
+	if !found || key == "" {
+		return fmt.Errorf("--state-values-set: invalid assignment %q: expected <key>=<value>", assignment)
+	}
+
+	maputil.Set(set, maputil.ParseKey(key), value, false)
+	return nil
+}
+
+// splitOnUnescapedCommas splits the input on commas that are not escaped by a
+// preceding backslash, keeping every backslash intact so that escapes remain
+// available to key and value parsing.
+func splitOnUnescapedCommas(input string) []string {
+	segments := make([]string, 0, 1+strings.Count(input, ","))
+	var current strings.Builder
+	escaped := false
+
+	for i := range len(input) {
+		char := input[i]
+
+		switch {
+		case escaped:
+			current.WriteByte('\\')
+			current.WriteByte(char)
+			escaped = false
+		case char == '\\':
+			escaped = true
+		case char == ',':
+			segments = append(segments, current.String())
+			current.Reset()
+		default:
+			current.WriteByte(char)
+		}
+	}
+	if escaped {
+		current.WriteByte('\\')
+	}
+	return append(segments, current.String())
+}
+
+// unescapeCommas turns every backslash-escaped comma into a literal comma,
+// preserving all other backslashes, including a trailing one.
+func unescapeCommas(input string) string {
+	if !strings.Contains(input, `\,`) {
+		return input
+	}
+
+	var out strings.Builder
+	out.Grow(len(input))
+	escaped := false
+
+	for i := range len(input) {
+		char := input[i]
+
+		switch {
+		case escaped && char == ',':
+			out.WriteByte(',')
+			escaped = false
+		case escaped:
+			out.WriteByte('\\')
+			out.WriteByte(char)
+			escaped = false
+		case char == '\\':
+			escaped = true
+		default:
+			out.WriteByte(char)
+		}
+	}
+	if escaped {
+		out.WriteByte('\\')
+	}
+	return out.String()
+}
@@ -0,0 +1,242 @@
+package config
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestNewCLIConfigImplStateValuesSet(t *testing.T) {
+	tests := []struct {
+		name    string
+		set     []string
+		want    map[string]any
+		wantErr string
+	}{
+		{
+			name: "comma separated assignments retain their types",
+			set:  []string{"count=2,enabled=true,disabled=false,empty=null,zero=0,code=001"},
+			want: map[string]any{
+				"count": int64(2), "enabled": true, "disabled": false,
+				"empty": nil, "zero": int64(0), "code": "001",
+			},
+		},
+		{
+			name: "escaped commas within multiple values",
+			set:  []string{`a=1,b=2\,3,c=9\,10\,11`},
+			want: map[string]any{"a": int64(1), "b": "2,3", "c": "9,10,11"},
+		},
+		{
+			name: "escaped commas at the start and end of a value",
+			set:  []string{`message=\,hello\,`},
+			want: map[string]any{"message": ",hello,"},
+		},
+		{
+			name: "repeated flags overwrite earlier values",
+			set:  []string{`message=first\,second`, `message=third\,fourth,count=2`},
+			want: map[string]any{"message": "third,fourth", "count": int64(2)},
+		},
+		{
+			name: "equals signs remain within values",
+			set:  []string{`message=first\,second=value,count=2`},
+			want: map[string]any{"message": "first,second=value", "count": int64(2)},
+		},
+		{
+			name: "unicode values",
+			set:  []string{`message=你好\,世界`},
+			want: map[string]any{"message": "你好,世界"},
+		},
+		{
+			name: "empty value",
+			set:  []string{"message=,count=2"},
+			want: map[string]any{"message": "", "count": int64(2)},
+		},
+		{
+			name: "backslashes before other characters are preserved",
+			set:  []string{`path=C:\tmp\config`},
+			want: map[string]any{"path": `C:\tmp\config`},
+		},
+		{
+			name: "trailing backslash is preserved",
+			set:  []string{`path=C:\tmp\`},
+			want: map[string]any{"path": `C:\tmp\`},
+		},
+		{
+			name: "escaped dots in keys are preserved for key parsing",
+			set:  []string{`annotations.example\.com/key=first\,second`},
+			want: map[string]any{
+				"annotations": map[string]any{"example.com/key": "first,second"},
+			},
+		},
+		{
+			name: "nested array keys",
+			set:  []string{`servers[0].host=east\,west,enabled=true`},
+			want: map[string]any{
+				"servers": []any{map[string]any{"host": "east,west"}},
+				"enabled": true,
+			},
+		},
+		{
+			name: "even backslashes leave comma as an assignment separator",
+			set:  []string{`path=C:\\,count=2`},
+			want: map[string]any{"path": `C:\\`, "count": int64(2)},
+		},
+		{
+			name: "odd backslashes escape the comma",
+			set:  []string{`message=first\\\,second`},
+			want: map[string]any{"message": `first\\,second`},
+		},
+		{
+			name:    "assignment without equals sign is rejected",
+			set:     []string{"message"},
+			wantErr: `invalid assignment "message": expected <key>=<value>`,
+		},
+		{
+			name:    "trailing comma is rejected",
+			set:     []string{"message=hello,"},
+			wantErr: `invalid assignment "": expected <key>=<value>`,
+		},
+		{
+			name:    "empty key is rejected",
+			set:     []string{"=hello"},
+			wantErr: `invalid assignment "=hello": expected <key>=<value>`,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewGlobalImpl(&GlobalOptions{StateValuesSet: tt.set})
+			err := NewCLIConfigImpl(g)
+			if tt.wantErr != "" {
+				require.ErrorContains(t, err, tt.wantErr)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tt.want, g.StateValuesSet())
+		})
+	}
+}
+
+func TestNewCLIConfigImplStateValuesSetString(t *testing.T) {
+	tests := []struct {
+		name    string
+		set     []string
+		want    map[string]any
+		wantErr string
+	}{
+		{
+			name: "values are kept as strings without type conversion",
+			set:  []string{"count=2,enabled=true"},
+			want: map[string]any{"count": "2", "enabled": "true"},
+		},
+		{
+			name: "quoted values may contain commas",
+			set:  []string{`zone="zone1,zone2",imageTag=1.23.3`},
+			want: map[string]any{"zone": `"zone1,zone2"`, "imageTag": "1.23.3"},
+		},
+		{
+			name: "escaped commas within values",
+			set:  []string{`zone=zone1\,zone2,imageTag=1.23.3`},
+			want: map[string]any{"zone": "zone1,zone2", "imageTag": "1.23.3"},
+		},
+		{
+			name: "backslashes and trailing backslash are preserved",
+			set:  []string{`path=C:\tmp\config,trailing=C:\tmp\`},
+			want: map[string]any{"path": `C:\tmp\config`, "trailing": `C:\tmp\`},
+		},
+		{
+			name: "even backslashes leave comma as an assignment separator",
+			set:  []string{`path=C:\\,count=2`},
+			want: map[string]any{"path": `C:\\`, "count": "2"},
+		},
+		{
+			name: "empty value",
+			set:  []string{"message=,count=2"},
+			want: map[string]any{"message": "", "count": "2"},
+		},
+		{
+			name: "repeated flags overwrite earlier values",
+			set:  []string{`zone=a\,b`, `zone=c\,d`},
+			want: map[string]any{"zone": "c,d"},
+		},
+		{
+			name: "escaped dots in keys are preserved for key parsing",
+			set:  []string{`annotations.example\.com/key=first\,second`},
+			want: map[string]any{
+				"annotations": map[string]any{"example.com/key": "first,second"},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewGlobalImpl(&GlobalOptions{StateValuesSetString: tt.set})
+			err := NewCLIConfigImpl(g)
+			if tt.wantErr != "" {
+				require.ErrorContains(t, err, tt.wantErr)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tt.want, g.StateValuesSet())
+		})
+	}
+}
+
+func TestNewCLIConfigImplBothSetFlags(t *testing.T) {
+	g := NewGlobalImpl(&GlobalOptions{
+		StateValuesSet:       []string{"count=2"},
+		StateValuesSetString: []string{"imageTag=1.23.3"},
+	})
+	require.NoError(t, NewCLIConfigImpl(g))
+
+	want := map[string]any{
+		"count":    int64(2),
+		"imageTag": "1.23.3",
+	}
+	assert.Equal(t, want, g.StateValuesSet())
+}
+
+func TestSplitOnUnescapedCommas(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+		want  []string
+	}{
+		{name: "no commas", input: "a=1", want: []string{"a=1"}},
+		{name: "plain separators", input: "a=1,b=2", want: []string{"a=1", "b=2"}},
+		{name: "escaped separator", input: `a=1\,2,b=3`, want: []string{`a=1\,2`, "b=3"}},
+		{name: "escaped separator only", input: `a=1\,2`, want: []string{`a=1\,2`}},
+		{name: "even backslashes unescape the separator", input: `a=1\\,b=2`, want: []string{`a=1\\`, "b=2"}},
+		{name: "trailing separator", input: "a=1,", want: []string{"a=1", ""}},
+		{name: "trailing backslash", input: `a=1\`, want: []string{`a=1\`}},
+		{name: "unicode", input: `a=你好\,世界,b=2`, want: []string{`a=你好\,世界`, "b=2"}},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, splitOnUnescapedCommas(tt.input))
+		})
+	}
+}
+
+func TestUnescapeCommas(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+		want  string
+	}{
+		{name: "no escapes", input: "a=1,b=2", want: "a=1,b=2"},
+		{name: "escaped comma", input: `a=1\,2`, want: "a=1,2"},
+		{name: "escaped dot preserved", input: `a\.b=1`, want: `a\.b=1`},
+		{name: "even backslashes preserved", input: `a=1\\`, want: `a=1\\`},
+		{name: "trailing backslash preserved", input: `a=1\`, want: `a=1\`},
+		{name: "unicode preserved", input: `a=你好\,世界`, want: "a=你好,世界"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, unescapeCommas(tt.input))
+		})
+	}
+}