From 000a6fa66067be3d87f10ef7ece3ef9480a06fdd Mon Sep 17 00:00:00 2001 From: zebreay <34366742+ban11111@users.noreply.github.com> Date: Sat, 3 Oct 2026 15:31:04 +0800 Subject: [PATCH] fix: support escaped commas in --state-values-set (#2813) * feat: support comma escape for values overwrites Signed-off-by: zhengbayi * 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 --------- Signed-off-by: zhengbayi Signed-off-by: yxxhero Co-authored-by: zhengbayi Co-authored-by: yxxhero --- cmd/root.go | 4 +- docs/cli.md | 24 ++++ pkg/config/config.go | 155 +++++++++++++++++++----- pkg/config/config_test.go | 242 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 394 insertions(+), 31 deletions(-) create mode 100644 pkg/config/config_test.go diff --git a/cmd/root.go b/cmd/root.go index 99f85745..a2a0c895 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -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"`) diff --git a/docs/cli.md b/docs/cli.md index 427afcc6..9293fb9a 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -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. diff --git a/pkg/config/config.go b/pkg/config/config.go index f49ec705..f9b87b1b 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -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 =", 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() +} diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go new file mode 100644 index 00000000..ba177045 --- /dev/null +++ b/pkg/config/config_test.go @@ -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 =`, + }, + { + name: "trailing comma is rejected", + set: []string{"message=hello,"}, + wantErr: `invalid assignment "": expected =`, + }, + { + name: "empty key is rejected", + set: []string{"=hello"}, + wantErr: `invalid assignment "=hello": expected =`, + }, + } + + 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)) + }) + } +}