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 -}