From 3b9164ce14118db08c1d63f780c9aba18a9e6719 Mon Sep 17 00:00:00 2001 From: Dmitrii Anoshin Date: Wed, 14 Dec 2022 16:07:02 -0800 Subject: [PATCH] Rework command line arguments parsing (#2343) This change solves two problems: 1. In the current implementation, command flags specific to the Splunk distro are dropped from the arguments list passed to the collector core service by their names. It means that all the flag values are unintentionally passed downstream. Also, flags with `=` are not removed from the list and cause failures downstream. For example, `./bin/otelcol --config-dir=/tmp/conf ...` fails with: ``` 2022/12/10 19:15:52 main.go:106: application run finished with error: unknown flag: --config-dir ``` And `removeFlag` function is called from methods that are not expected to be mutating by their definition like `ResolverURIs`. 2. Currently, we rely on the downstream collector core service to display `--help` flag output and get the following: ``` Usage: otelcol [flags] Flags: --config --config=file:/path/to/first --config=file:path/to/second Locations to the config file(s), note that only a single location can be set per flag entry e.g. --config=file:/path/to/first --config=file:path/to/second. (default []) --feature-gates Flag Comma-delimited list of feature gate identifiers. Prefix with '-' to disable the feature. '+' or no prefix will enable the feature. -h, --help help for otelcol --set func Set arbitrary component config property. The component has to be defined in the config file and the flag has a higher precedence. Array config properties are overridden and maps are joined. Example --set=processors.batch.timeout=2s -v, --version version for otelcol ``` It's not just incomplete but also provides a misleading description for `--config` flag: we don't support config schemas, so using any of the provided examples will fail. This change defines all the flags in our distro and makes the program use that definition in `--help` output instead of relying on the collector core service. Experimental flags like `--configd`, `--config-dir` and `--discovery` are hidden for now, but can be easily enabled. It provides the following output for `--help` flag: ``` Usage of otelcol: --config string Locations to the config file(s), note that only a single location can be set per flag entry e.g. --config=/path/to/first --config=path/to/second. (default "[]") --dry-run Don't run the service, just show the configuration --feature-gates string Comma-delimited list of feature gate identifiers. Prefix with '-' to disable the feature. '+' or no prefix will enable the feature. (default "[]") --no-convert-config Do not translate old configurations to the new format automatically. By default, old configurations are translated to the new format for backward compatibility. --set string Set arbitrary component config property. The component has to be defined in the config file and the flag has a higher precedence. Array config properties are overridden and maps are joined. Example --set=processors.batch.timeout=2s (default "[]") -v, --version Version of the collector. ``` Now we also explicitly pass all the flags that are handled by the collector core service instead of passing what we don't use. Reason for that is that there are no plans in the collector core to add any new flags. Most of the old flags were migrated to the configuration. And, even if something will be added, it potentially can be incompatible with our distro and introduce some broken flags to the `--help` output without us noticing. And, as an opportunistic simplification, this change replaces an unnecessary `Settings` interface with a struct. --- cmd/otelcol/main.go | 7 +- internal/settings/settings.go | 172 ++++++++++++++--------------- internal/settings/settings_test.go | 127 ++++++++------------- 3 files changed, 136 insertions(+), 170 deletions(-) diff --git a/cmd/otelcol/main.go b/cmd/otelcol/main.go index 8dafd17c2b9..2837d1c9892 100644 --- a/cmd/otelcol/main.go +++ b/cmd/otelcol/main.go @@ -22,6 +22,7 @@ import ( "log" "os" + flag "github.com/spf13/pflag" "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/confmap/provider/envprovider" @@ -44,6 +45,10 @@ func main() { collectorSettings, err := settings.New(os.Args[1:]) if err != nil { + // Exit if --help flag was supplied and usage help was displayed. + if err == flag.ErrHelp { + os.Exit(0) + } log.Fatalf(`invalid settings detected: %v. Use "--help" to show valid usage`, err) } @@ -101,7 +106,7 @@ func main() { ConfigProvider: serviceConfigProvider, } - os.Args = append(os.Args[:1], collectorSettings.ServiceArgs()...) + os.Args = append(os.Args[:1], collectorSettings.ColCoreArgs()...) if err = run(serviceSettings); err != nil { log.Fatal(err) } diff --git a/internal/settings/settings.go b/internal/settings/settings.go index d7da44944e8..335a7b70798 100644 --- a/internal/settings/settings.go +++ b/internal/settings/settings.go @@ -16,7 +16,6 @@ package settings import ( "fmt" - "io" "log" "os" "strconv" @@ -30,8 +29,6 @@ import ( ) const ( - DefaultUndeclaredFlag = -1 - APIURLEnvVar = "SPLUNK_API_URL" BallastEnvVar = "SPLUNK_BALLAST_SIZE_MIB" ConfigEnvVar = "SPLUNK_CONFIG" @@ -60,29 +57,30 @@ const ( ConfigDScheme = "splunk.configd" ) -type Settings interface { - // ResolverURIs returns the collector config provider resolver uris for the collector service - ResolverURIs() []string - // ConfMapConverters returns the collector config provider resolver confmap.Converters for the collector service - ConfMapConverters() []confmap.Converter - // ServiceArgs are the sanitized, adjusted args to be used in updating os.Args[1:] for the collector service - ServiceArgs() []string - // IsDryRun returns whether --dry-run mode was requested - IsDryRun() bool +type Settings struct { + configPaths *stringArrayFlagValue + setProperties *stringArrayFlagValue + configDir *stringPointerFlagValue + colCoreArgs []string + versionFlag bool + noConvertConfig bool + configD bool + discoveryMode bool + dryRun bool } -func New(args []string) (Settings, error) { - f, err := newFlags(args) +func New(args []string) (*Settings, error) { + s, err := parseArgs(args) if err != nil { return nil, err } // immediate exit paths, no further setup required - if f.helpFlag || f.versionFlag { - return f, nil + if s.versionFlag { + return s, nil } - if err = checkRuntimeParams(f); err != nil { + if err = checkRuntimeParams(s); err != nil { return nil, err } @@ -90,47 +88,27 @@ func New(args []string) (Settings, error) { return nil, err } - return f, nil -} - -var _ Settings = (*flags)(nil) - -type flags struct { - configPaths *stringArrayFlagValue - setProperties *stringArrayFlagValue - configDir *stringPointerFlagValue - serviceArgs []string - helpFlag bool - versionFlag bool - noConvertConfig bool - configD bool - discoveryMode bool - dryRun bool + return s, nil } -func (f *flags) ResolverURIs() []string { +// ResolverURIs returns config provider resolver URIs for the core collector service. +func (s *Settings) ResolverURIs() []string { var configPaths []string - if configPaths = f.configPaths.value; len(configPaths) == 0 { + if configPaths = s.configPaths.value; len(configPaths) == 0 { if configEnvVal := os.Getenv(ConfigEnvVar); len(configEnvVal) != 0 { configPaths = []string{"file:" + configEnvVal} } } - configDir := getConfigDir(f) - - if f.dryRun { - removeFlag(&f.serviceArgs, "--dry-run") - } + configDir := getConfigDir(s) - if f.configD { - removeFlag(&f.serviceArgs, "--configd") + if s.configD { configPaths = append(configPaths, fmt.Sprintf("%s:%s", ConfigDScheme, configDir)) } - if f.discoveryMode { - removeFlag(&f.serviceArgs, "--discovery") + if s.discoveryMode { // discovery uri must come last to successfully merge w/ other config content - configPaths = append(configPaths, fmt.Sprintf("%s:%s", DiscoveryModeScheme, f.configDir)) + configPaths = append(configPaths, fmt.Sprintf("%s:%s", DiscoveryModeScheme, s.configDir)) } configYaml := os.Getenv(ConfigYamlEnvVar) @@ -145,32 +123,27 @@ func (f *flags) ResolverURIs() []string { } } -func getConfigDir(f *flags) string { +func getConfigDir(f *Settings) string { configDir := DefaultConfigDir if envConfigDir, ok := os.LookupEnv(ConfigDirEnvVar); ok { configDir = envConfigDir } if f.configDir.value != nil { - removeFlag(&f.serviceArgs, "--config-dir") configDir = f.configDir.String() } return configDir } -func (f *flags) ConfMapConverters() []confmap.Converter { +// ConfMapConverters returns confmap.Converters for the collector core service. +func (s *Settings) ConfMapConverters() []confmap.Converter { confMapConverters := []confmap.Converter{ // nolint: staticcheck - overwritepropertiesconverter.New(f.setProperties.value), // support until there's an actual replacement + overwritepropertiesconverter.New(s.setProperties.value), // support until there's an actual replacement } - if f.noConvertConfig { - // the collector complains about this flag if we don't remove it. Unfortunately, - // this must be done manually since the flag library has no functionality to remove - // args - removeFlag(&f.serviceArgs, "--no-convert-config") - } else { + if !s.noConvertConfig { confMapConverters = append( confMapConverters, configconverter.RemoveBallastKey{}, @@ -182,64 +155,83 @@ func (f *flags) ConfMapConverters() []confmap.Converter { return confMapConverters } -func (f *flags) ServiceArgs() []string { - return f.serviceArgs +// ColCoreArgs returns list of arguments to be passed to the collector core service. +func (s *Settings) ColCoreArgs() []string { + return s.colCoreArgs } -func (f *flags) IsDryRun() bool { - return f.dryRun +// IsDryRun returns whether --dry-run mode was requested +func (s *Settings) IsDryRun() bool { + return s.dryRun } -func newFlags(args []string) (*flags, error) { - flagSet := flag.FlagSet{} - // we don't want to be responsible for tracking all supported collector service - // flags, so allow any we don't use and defer parsing to the actual service - flagSet.ParseErrorsWhitelist.UnknownFlags = true - - var cpArgs []string - cpArgs = append(cpArgs, args...) +// parseArgs returns new Settings instance from command line arguments. +func parseArgs(args []string) (*Settings, error) { + flagSet := flag.NewFlagSet("otelcol", flag.ContinueOnError) - settings := &flags{ + settings := &Settings{ configPaths: new(stringArrayFlagValue), setProperties: new(stringArrayFlagValue), - serviceArgs: cpArgs, configDir: new(stringPointerFlagValue), } - // This is an internal flag parser, it shouldn't give any output to user. - flagSet.SetOutput(io.Discard) - - flagSet.BoolVarP(&settings.helpFlag, "help", "h", false, "") - flagSet.BoolVarP(&settings.versionFlag, "version", "v", false, "") - - flagSet.BoolVar(&settings.noConvertConfig, "no-convert-config", false, "") + flagSet.Var(settings.configPaths, "config", "Locations to the config file(s), "+ + "note that only a single location can be set per flag entry e.g. --config=/path/to/first "+ + "--config=path/to/second.") + flagSet.Var(settings.setProperties, "set", "Set arbitrary component config property. "+ + "The component has to be defined in the config file and the flag has a higher precedence. "+ + "Array config properties are overridden and maps are joined. Example --set=processors.batch.timeout=2s") + flagSet.BoolVar(&settings.dryRun, "dry-run", false, "Don't run the service, just show the configuration") + flagSet.BoolVar(&settings.noConvertConfig, "no-convert-config", false, + "Do not translate old configurations to the new format automatically. "+ + "By default, old configurations are translated to the new format for backward compatibility.") + // Experimental flags + flagSet.VarPF(settings.configDir, "config-dir", "", "").Hidden = true flagSet.BoolVar(&settings.configD, "configd", false, "") - flagSet.Var(settings.configDir, "config-dir", "") + flagSet.MarkHidden("configd") flagSet.BoolVar(&settings.discoveryMode, "discovery", false, "") - flagSet.BoolVar(&settings.dryRun, "dry-run", false, "") + flagSet.MarkHidden("discovery") - flagSet.Var(settings.configPaths, "config", "") - flagSet.Var(settings.setProperties, "set", "") + // OTel Collector Core flags + colCoreFlags := []string{"version", "feature-gates"} + flagSet.BoolVarP(&settings.versionFlag, colCoreFlags[0], "v", false, "Version of the collector.") + flagSet.Var(new(stringArrayFlagValue), colCoreFlags[1], + "Comma-delimited list of feature gate identifiers. Prefix with '-' to disable the feature. "+ + "'+' or no prefix will enable the feature.") - if err := flagSet.Parse(cpArgs); err != nil { + if err := flagSet.Parse(args); err != nil { return nil, err } + // Pass flags that are handled by the collector core service as raw command line arguments. + settings.colCoreArgs = flagSetToArgs(colCoreFlags, flagSet) + return settings, nil } -func removeFlag(flags *[]string, flag string) { +// flagSetToArgs takes a list of flag names and returns a list of corresponding command line arguments +// using values from the provided flagSet. +// The flagSet must be populated (flagSet.Parse is called), otherwise the returned list of arguments will be empty. +func flagSetToArgs(flagNames []string, flagSet *flag.FlagSet) []string { var out []string - for _, s := range *flags { - if s != flag { - out = append(out, s) + for _, flagName := range flagNames { + flag := flagSet.Lookup(flagName) + if flag.Changed { + switch fv := flag.Value.(type) { + case *stringArrayFlagValue: + for _, val := range fv.value { + out = append(out, "--"+flagName, val) + } + default: + out = append(out, "--"+flagName, flag.Value.String()) + } } } - *flags = out + return out } -func checkRuntimeParams(settings *flags) error { +func checkRuntimeParams(settings *Settings) error { if err := checkConfig(settings); err != nil { return err } @@ -303,7 +295,7 @@ func setDefaultEnvVars() error { // 2. SPLUNK_CONFIG env var, // 3. SPLUNK_CONFIG_YAML env var, // 4. default gateway config path. -func checkConfig(settings *flags) error { +func checkConfig(settings *Settings) error { configPathVar := os.Getenv(ConfigEnvVar) configYaml := os.Getenv(ConfigYamlEnvVar) @@ -385,7 +377,7 @@ func setMemoryLimit(memTotalSizeMiB int) (int, error) { return memLimit, nil } -func checkInputConfigs(settings *flags) error { +func checkInputConfigs(settings *Settings) error { configPathVar := os.Getenv(ConfigEnvVar) configYaml := os.Getenv(ConfigYamlEnvVar) @@ -406,7 +398,7 @@ func checkInputConfigs(settings *flags) error { return confirmRequiredEnvVarsForDefaultConfigs(settings.configPaths.value) } -func checkConfigPathEnvVar(settings *flags) error { +func checkConfigPathEnvVar(settings *Settings) error { configPathVar := os.Getenv(ConfigEnvVar) configYaml := os.Getenv(ConfigYamlEnvVar) diff --git a/internal/settings/settings_test.go b/internal/settings/settings_test.go index ee9e0d06e8f..c0d0bbf2e3e 100644 --- a/internal/settings/settings_test.go +++ b/internal/settings/settings_test.go @@ -23,6 +23,7 @@ import ( "strings" "testing" + flag "github.com/spf13/pflag" "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/confmap/converter/overwritepropertiesconverter" @@ -37,11 +38,11 @@ var ( localOTLPLinuxConfig = filepath.Join("..", "..", "cmd/otelcol/config/collector/otlp_config_linux.yaml") ) -func TestNewSettingsWithUnknownFlagsAcceptable(t *testing.T) { +func TestNewSettingsWithUnknownFlagsNotAcceptable(t *testing.T) { t.Cleanup(setRequiredEnvVars(t)) settings, err := New([]string{"--unknown-flag", "100"}) - require.NoError(t, err) - require.NotNil(t, settings) + require.Error(t, err) + require.Nil(t, settings) } func TestNewSettingsWithVersionFlags(t *testing.T) { @@ -49,20 +50,19 @@ func TestNewSettingsWithVersionFlags(t *testing.T) { settings, err := New([]string{}) require.NoError(t, err) require.NotNil(t, settings) - f := settingsToFlags(t, settings) - require.False(t, f.versionFlag) + require.False(t, settings.versionFlag) settings, err = New([]string{"--version"}) require.NoError(t, err) require.NotNil(t, settings) - f = settingsToFlags(t, settings) - require.True(t, f.versionFlag) + require.True(t, settings.versionFlag) + require.Equal(t, []string{"--version", "true"}, settings.ColCoreArgs()) settings, err = New([]string{"-v"}) require.NoError(t, err) require.NotNil(t, settings) - f = settingsToFlags(t, settings) - require.True(t, f.versionFlag) + require.True(t, settings.versionFlag) + require.Equal(t, []string{"--version", "true"}, settings.ColCoreArgs()) } func TestNewSettingsWithHelpFlags(t *testing.T) { @@ -70,20 +70,16 @@ func TestNewSettingsWithHelpFlags(t *testing.T) { settings, err := New([]string{}) require.NoError(t, err) require.NotNil(t, settings) - f := settingsToFlags(t, settings) - require.False(t, f.helpFlag) settings, err = New([]string{"--help"}) - require.NoError(t, err) - require.NotNil(t, settings) - f = settingsToFlags(t, settings) - require.True(t, f.helpFlag) + require.Error(t, err) + require.Equal(t, flag.ErrHelp, err) + require.Nil(t, settings) settings, err = New([]string{"-h"}) - require.NoError(t, err) - require.NotNil(t, settings) - f = settingsToFlags(t, settings) - require.True(t, f.helpFlag) + require.Error(t, err) + require.Equal(t, flag.ErrHelp, err) + require.Nil(t, settings) } func TestNewSettingsNoConvertConfig(t *testing.T) { @@ -100,23 +96,17 @@ func TestNewSettingsNoConvertConfig(t *testing.T) { }) require.NoError(t, err) - f := settingsToFlags(t, settings) - require.True(t, f.noConvertConfig) + require.True(t, settings.noConvertConfig) - require.Equal(t, []string{configPath, anotherConfigPath}, f.configPaths.value) - require.Equal(t, []string{"foo", "bar", "baz"}, f.setProperties.value) + require.Equal(t, []string{configPath, anotherConfigPath}, settings.configPaths.value) + require.Equal(t, []string{"foo", "bar", "baz"}, settings.setProperties.value) require.Equal(t, []string{configPath, anotherConfigPath}, settings.ResolverURIs()) require.Equal(t, []confmap.Converter{ // nolint: staticcheck - overwritepropertiesconverter.New(f.setProperties.value), // support until there's an actual replacement + overwritepropertiesconverter.New(settings.setProperties.value), // support until there's an actual replacement }, settings.ConfMapConverters()) - require.Equal(t, []string{ - "--config", configPath, - "--config", anotherConfigPath, - "--set", "foo", "--set", "bar", "--set", "baz", - "--feature-gates", "foo", "--feature-gates", "-bar", - }, settings.ServiceArgs()) + require.Equal(t, []string{"--feature-gates", "foo", "--feature-gates", "-bar"}, settings.ColCoreArgs()) } func TestNewSettingsConvertConfig(t *testing.T) { @@ -132,29 +122,22 @@ func TestNewSettingsConvertConfig(t *testing.T) { }) require.NoError(t, err) - f := settingsToFlags(t, settings) - require.False(t, f.helpFlag) - require.False(t, f.versionFlag) - require.False(t, f.noConvertConfig) + require.False(t, settings.versionFlag) + require.False(t, settings.noConvertConfig) - require.Equal(t, []string{configPath, anotherConfigPath}, f.configPaths.value) - require.Equal(t, []string{"foo", "bar", "baz"}, f.setProperties.value) + require.Equal(t, []string{configPath, anotherConfigPath}, settings.configPaths.value) + require.Equal(t, []string{"foo", "bar", "baz"}, settings.setProperties.value) require.Equal(t, []string{configPath, anotherConfigPath}, settings.ResolverURIs()) require.Equal(t, []confmap.Converter{ // nolint: staticcheck - overwritepropertiesconverter.New(f.setProperties.value), // support until there's an actual replacement + overwritepropertiesconverter.New(settings.setProperties.value), // support until there's an actual replacement configconverter.RemoveBallastKey{}, configconverter.MoveOTLPInsecureKey{}, configconverter.MoveHecTLS{}, configconverter.RenameK8sTagger{}, }, settings.ConfMapConverters()) - require.Equal(t, []string{ - "--config", configPath, - "--config", anotherConfigPath, - "--set", "foo", "--set", "bar", "--set", "baz", - "--feature-gates", "foo", "--feature-gates", "-bar", - }, settings.ServiceArgs()) + require.Equal(t, []string{"--feature-gates", "foo", "--feature-gates", "-bar"}, settings.ColCoreArgs()) } func TestSplunkConfigYamlUtilizedInResolverURIs(t *testing.T) { @@ -293,7 +276,7 @@ func TestUseConfigPathsFromEnvVar(t *testing.T) { settings, err := New([]string{}) require.NoError(t, err) - configPaths := settingsToFlags(t, settings).configPaths.value + configPaths := settings.configPaths.value require.Equal(t, []string{localGatewayConfig}, configPaths) require.Equal(t, []string{localGatewayConfig}, settings.ResolverURIs()) } @@ -418,41 +401,36 @@ service: } } -func TestRemoveFlag(t *testing.T) { - args := []string{"--aaa", "--bbb", "--ccc"} - removeFlag(&args, "--bbb") - require.Equal(t, []string{"--aaa", "--ccc"}, args) - removeFlag(&args, "--ccc") - require.Equal(t, []string{"--aaa"}, args) - removeFlag(&args, "--aaa") - require.Empty(t, args) -} - func TestEnablingConfigD(t *testing.T) { t.Cleanup(clearEnv(t)) settings, err := New([]string{"--config", configPath}) require.NoError(t, err) - f := settingsToFlags(t, settings) - require.False(t, f.configD) - require.Nil(t, f.configDir.value) + require.False(t, settings.configD) + require.Nil(t, settings.configDir.value) settings, err = New([]string{"--configd", "--config", configPath}) require.NoError(t, err) - f = settingsToFlags(t, settings) - require.True(t, f.configD) - require.Nil(t, f.configDir.value) - require.Equal(t, "/etc/otel/collector/config.d", getConfigDir(f)) + require.True(t, settings.configD) + require.Nil(t, settings.configDir.value) + require.Equal(t, "/etc/otel/collector/config.d", getConfigDir(settings)) } func TestConfigDirFromArgs(t *testing.T) { t.Cleanup(clearEnv(t)) - settings, err := New([]string{"--config-dir", "/from/args", "--config", configPath}) - require.NoError(t, err) - f := settingsToFlags(t, settings) - require.False(t, f.configD) - require.NotNil(t, f.configDir.value) - require.Equal(t, "/from/args", f.configDir.String()) - require.Equal(t, "/from/args", getConfigDir(f)) + for _, args := range [][]string{ + {"--config-dir", "/from/args", "--config", configPath}, + {"--config-dir=/from/args", "--config=" + configPath}, + } { + t.Run(strings.Join(args, " "), func(t *testing.T) { + settings, err := New(args) + require.NoError(t, err) + require.False(t, settings.configD) + require.NotNil(t, settings.configDir.value) + require.Equal(t, "/from/args", settings.configDir.String()) + require.Equal(t, "/from/args", getConfigDir(settings)) + require.Nil(t, settings.ColCoreArgs()) + }) + } } func TestConfigDirFromEnvVar(t *testing.T) { @@ -460,9 +438,8 @@ func TestConfigDirFromEnvVar(t *testing.T) { os.Setenv("SPLUNK_CONFIG_DIR", "/from/env/var") settings, err := New([]string{"--config", configPath}) require.NoError(t, err) - f := settingsToFlags(t, settings) - require.Nil(t, f.configDir.value) - require.Equal(t, "/from/env/var", getConfigDir(f)) + require.Nil(t, settings.configDir.value) + require.Equal(t, "/from/env/var", getConfigDir(settings)) } // to satisfy Settings generation @@ -490,11 +467,3 @@ func clearEnv(t *testing.T) func() { } } } - -func settingsToFlags(t testing.TB, settings Settings) *flags { - require.NotNil(t, settings) - f, ok := settings.(*flags) - require.True(t, ok) - require.NotNil(t, f) - return f -}