hive: Unify parsing of string slices

[joamaki]

Hive uses pflag and viper to parse configuration flags from multiple sources. If a flag is set via command-line then the pflag parser is invoked to get to the destination type as defined in the FlagSet ("flags.StringSlice" 1), however if the flag comes from enviroment or config-map, then the parsing was done by a mapstructure hook 2.

This is all well and good as long as these two ways of parsing into say []string are aligned with each other. And they were. Unfortunately these were not aligned with the pre-Hive way of parsing which used viper.GetStringSlice and similar methods. Specifically viper.GetStringSlice is implemented (3) via cast.ToStringSlice, which uses strings.Fields that splits by whitespace instead of by commas.

So to summarize the different ways a StringSlice can be parsed:

• 1: flags.StringSlice: parses with csv.Reader (split by comma)
• 2: stringToSliceHookFunc: splits by comma
• 3: viper.GetStringSlice: splits by whitespaces

So while arguably the first two are more consistent, we can't just flip from splitting by spaces to splitting by commas as that creates a huge foot-gun when fields are moved from option.Config to individual hive.Config structs.

To solve this we allow splitting both ways by using two mapstructure hooks that process the values before they're pushed to the config struct:

• stringToSliceHookFunc now splits first by commas and if it doesn't produce multiple values it will split by commas. This handles the values going to []string and coming from "flags.String", config file, config map and the environment.

• fixupStringSliceHookFunc takes []string coming from flags.StringSlice that was split by comma and resplits it if splitting by comma resulted in a single value.

With this, we have unified the parsing of []string across all the config input methods:

• "foo,bar,baz" => []string{"foo", "bar", "baz"}
• "foo bar baz" => []string{"foo", "bar", "baz"}
• "foo bar,baz" => []string{"foo bar", "baz"}

Fixes: #29210
Fixes: b407ffc ("hive: Reimplement on top of dig")

Unify parsing of StringSlice flags and allow splitting by commas (preferably) or by spaces. This fixes parsing of 'prometheus.metrics'.

[joamaki]

Marking with needs-backport/1.14 as pkg/metrics was modularized in v1.14 and thus split '--metrics' by comma resulting in prometheus.metrics helm value to be incorrectly set. (e.g. via Helm we'd get config-map file metrics with content +metric1 +metric2 which would get parsed to []string{"+metric1 +metric2"} and not []string{"+metric1", "+metric2"}).

Looking at go run ./daemon hive for v1.14 the other []string config fields are --enable-cilium-api-server-access and --enable-cilium-health-api-server-access, but these are not configured via helm templates and thus not an issue.

[giorio94]

/lgtm, thanks!

I've just left a couple of minor comments inline.

[joamaki]

/test

[ti-mo]

It's not clear to me why we need both stringToSliceHookFunc and fixupStringSliceHookFunc. Doesn't mapstructure.StringToSliceHookFunc(","), already split by comma? There's some overlap between both functions introduced here.

Also, isn't is possible to wrap mapstructure.StringToSliceHookFunc() and make one take precedence over the other?

A quick overview of all the kinds of expected inputs would be nice to have as a comment. 👍

[joamaki]

It's not clear to me why we need both stringToSliceHookFunc and fixupStringSliceHookFunc. Doesn't mapstructure.StringToSliceHookFunc(","), already split by comma? There's some overlap between both functions introduced here.

stringToSliceHookFunc is getting the input as string (e.g. from environment or configmap), fixupStringSliceHookFunc is getting the input from pflag's StringSlice where it's already of type []string and split by comma and we're post-processing it to split the same way as we do when it's coming from environment variables or configmap.

Also, isn't is possible to wrap mapstructure.StringToSliceHookFunc() and make one take precedence over the other?

Ah now that you said it, yes I think we can just have StringToSliceHookFunc split by commas and then have fixupStringSliceHookFunc do the rest. I'll try it out.

EDIT: Worked nicely!

A quick overview of all the kinds of expected inputs would be nice to have as a comment. 👍

Doesn't the commit description list them? ("So to summarize the different ways a StringSlice can be parsed:").

EDIT: Ah as comment in code? Adding!

[joestringer]

Thanks for the fix and the great writeup on the origins of how we got into this confusing situation. I think that the approach used here is a reasonable way to fix the backwards compatibility while defining some consistency going forward.

[joamaki]

/test