From f60a6185577bea19460a62ad4041dd6169c80901 Mon Sep 17 00:00:00 2001 From: Bremer Jonathan Date: Wed, 14 Jul 2021 10:56:58 -0400 Subject: [PATCH] Add env var SPLUNK_CONFIG_YAML for storing configuration YAML (#462) --- cmd/otelcol/main.go | 168 +++++++++++------- cmd/otelcol/main_test.go | 137 ++++++++++++-- docs/getting-started/linux-manual.md | 54 +++++- .../configprovider/config_source_provider.go | 7 +- .../config_source_provider_test.go | 1 + tests/general/container_test.go | 62 ++++++- 6 files changed, 341 insertions(+), 88 deletions(-) diff --git a/cmd/otelcol/main.go b/cmd/otelcol/main.go index 930dfac0916..aafabdb649b 100644 --- a/cmd/otelcol/main.go +++ b/cmd/otelcol/main.go @@ -18,6 +18,7 @@ package main import ( + "bytes" "fmt" "log" "os" @@ -26,6 +27,7 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/service" + "go.opentelemetry.io/collector/service/parserprovider" "go.uber.org/zap" "github.com/signalfx/splunk-otel-collector/internal/components" @@ -37,6 +39,7 @@ import ( const ( ballastEnvVarName = "SPLUNK_BALLAST_SIZE_MIB" configEnvVarName = "SPLUNK_CONFIG" + configYamlEnvVarName = "SPLUNK_CONFIG_YAML" configServerEnabledEnvVar = "SPLUNK_DEBUG_CONFIG_SERVER" memLimitMiBEnvVarName = "SPLUNK_MEMORY_LIMIT_MIB" memTotalEnvVarName = "SPLUNK_MEMORY_TOTAL_MIB" @@ -57,8 +60,7 @@ func main() { // TODO: Use same format as the collector log.SetFlags(log.Ldate | log.Ltime | log.Lshortfile) - args := os.Args[1:] - if !contains(args, "-h") && !contains(args, "--help") { + if !contains(os.Args[1:], "-h") && !contains(os.Args[1:], "--help") { checkRuntimeParams() } @@ -77,6 +79,7 @@ func main() { } parserProvider := configprovider.NewConfigSourceParserProvider( + newBaseParserProvider(), zap.NewNop(), // The service logger is not available yet, setting it to NoP. info, configsources.Get()..., @@ -109,19 +112,21 @@ func contains(arr []string, str string) bool { // Get the value of a key in an array // Support key/value with and with an equal sign -func getKeyValue(args []string, argName string) string { - val := "" - for i, arg := range args { +func getKeyValue(args []string, arg string) (exists bool, value string) { + argEq := arg + "=" + for i := range args { switch { - case strings.HasPrefix(arg, argName+"="): - s := strings.Split(arg, "=") - val = s[1] - case arg == argName: - i++ - val = args[i] + case strings.HasPrefix(args[i], argEq): + return true, strings.SplitN(args[i], "=", 2)[1] + case args[i] == arg: + exists = true + if i < (len(args) - 1) { + value = args[i+1] + } + return } } - return val + return } // Check runtime parameters @@ -129,24 +134,7 @@ func getKeyValue(args []string, argName string) string { // Config and ballast flags are checked // Config and all memory env vars are checked func checkRuntimeParams() { - args := os.Args[1:] - config := "" - - // Check if config flag was passed and its value differs from config env var. - // If so, log that it will be used instead of env var value and set env var with that value. - // This allows users to set `--config` and have it take priority when running from most contexts. - cliConfig := getKeyValue(args, "--config") - if cliConfig != "" { - config = os.Getenv(configEnvVarName) - if config != "" && config != cliConfig { - log.Printf( - "Both %v and '--config' were specified. Overriding %q environment variable value with %q for this session", - configEnvVarName, config, cliConfig, - ) - } - os.Setenv(configEnvVarName, cliConfig) - } - setConfig() + checkConfig() // Set default total memory memTotalSizeMiB := defaultMemoryTotalMiB @@ -169,11 +157,10 @@ func checkRuntimeParams() { // Check if memory ballast flag was passed // If so, ensure memory ballast env var is not set // Then set memory ballast and limit properly - ballastSize := getKeyValue(args, "--mem-ballast-size-mib") + _, ballastSize := getKeyValue(os.Args[1:], "--mem-ballast-size-mib") if ballastSize != "" { - config = os.Getenv(ballastEnvVarName) - if config != "" { - log.Fatalf("Both %v and '--config' were specified, but only one is allowed", ballastEnvVarName) + if os.Getenv(ballastEnvVarName) != "" { + log.Fatalf("Both %v and '--mem-ballast-size-mib' were specified, but only one is allowed", ballastEnvVarName) } os.Setenv(ballastEnvVarName, ballastSize) } @@ -181,54 +168,92 @@ func checkRuntimeParams() { setMemoryLimit(memTotalSizeMiB) } -// Validate and set the configuration -func setConfig() { - // Check if the config is specified via the env var. - config := os.Getenv(configEnvVarName) - // If not attempt to use a default config; supports Docker and local - if config == "" { - _, err := os.Stat(defaultDockerSAPMConfig) - if err == nil { - config = defaultDockerSAPMConfig +// Sets flag '--config' to specified env var SPLUNK_CONFIG, if the flag not specified. +// Logs a message and returns if env var SPLUNK_CONFIG_YAML specified, and '--config' and SPLUNK_CONFIG no specified. +// Sets '--config' to default config file path if '--config', SPLUNK_CONFIG and SPLUNK_CONFIG_YAML not specified. +func checkConfig() { + configPathFlagExists, configPathFlag := getKeyValue(os.Args[1:], "--config") + configPathVar := os.Getenv(configEnvVarName) + configYaml := os.Getenv(configYamlEnvVarName) + + if configPathFlagExists && configPathFlag == "" { + log.Fatal("Command line flag --config specified but empty") + } + + if configPathFlag != "" { + if _, err := os.Stat(configPathFlag); err != nil { + log.Fatalf("Unable to find the configuration file (%s) ensure flag '--config' is set properly", configPathFlag) } - _, err = os.Stat(defaultLocalSAPMConfig) - if err == nil { - config = defaultLocalSAPMConfig + + if configPathVar != "" && configPathVar != configPathFlag { + log.Printf("Both environment variable %v and flag '--config' were specified. Using the flag value %s and ignoring the environment variable value %s in this session", configEnvVarName, configPathFlag, configPathVar) } - if config == "" { - log.Fatalf("Unable to find the default configuration file, ensure %s environment variable is set properly", configEnvVarName) + + if configYaml != "" { + log.Printf("Both environment variable %s and flag '--config' were specified. Using the flag value %s and ignoring the environment variable in this session", configYamlEnvVarName, configPathFlag) } - } else { - // Check if file exists. - _, err := os.Stat(config) - if err != nil { - log.Fatalf("Unable to find the configuration file (%s) ensure %s environment variable is set properly", config, configEnvVarName) + + checkRequiredEnvVars(configPathFlag) + + log.Printf("Set config to %v", configPathFlag) + return + } + + if configPathVar != "" { + if _, err := os.Stat(configPathVar); err != nil { + log.Fatalf("Unable to find the configuration file (%s) ensure %s environment variable is set properly", configPathVar, configEnvVarName) + } + + os.Args = append(os.Args, "--config="+configPathVar) + + if configYaml != "" { + log.Printf("Both %s and %s were specified. Using %s environment variable value %s for this session", configEnvVarName, configYamlEnvVarName, configEnvVarName, configPathVar) } + + checkRequiredEnvVars(configPathVar) + + log.Printf("Set config to %v", configPathVar) + return + } + + if configYaml != "" { + log.Printf("Using environment variable %s for configuration", configYamlEnvVarName) + return + } + + defaultConfigPath := getExistingDefaultConfigPath() + checkRequiredEnvVars(defaultConfigPath) + os.Args = append(os.Args, "--config="+defaultConfigPath) + log.Printf("Set config to %v", defaultConfigPath) +} + +func getExistingDefaultConfigPath() (path string) { + if _, err := os.Stat(defaultLocalSAPMConfig); err == nil { + return defaultLocalSAPMConfig + } + if _, err := os.Stat(defaultDockerSAPMConfig); err == nil { + return defaultDockerSAPMConfig } + log.Fatalf("Unable to find the default configuration file (%s) or (%s)", defaultLocalSAPMConfig, defaultDockerSAPMConfig) + return +} - switch config { +func checkRequiredEnvVars(path string) { + // Check environment variables required by default configuration. + switch path { case defaultDockerSAPMConfig, defaultDockerOTLPConfig, defaultLocalSAPMConfig, defaultLocalOTLPConfig: - // The following environment variables are required. - // If any are missing stop here. requiredEnvVars := []string{realmEnvVarName, tokenEnvVarName} for _, v := range requiredEnvVars { if len(os.Getenv(v)) == 0 { log.Printf("Usage: %s=12345 %s=us0 %s", tokenEnvVarName, realmEnvVarName, os.Args[0]) - log.Fatalf("ERROR: Missing required environment variable %s with default config path %s", v, config) + log.Fatalf("ERROR: Missing required environment variable %s with default config path %s", v, path) } } } - - args := os.Args[1:] - if !contains(args, "--config") { - // Inject the command line flag that controls the configuration. - os.Args = append(os.Args, "--config="+config) - } - log.Printf("Set config to %v", config) } // Validate and set the memory ballast @@ -277,7 +302,7 @@ func setMemoryLimit(memTotalSizeMiB int) { // Validate memoryLimit is sane args := os.Args[1:] - b := getKeyValue(args, "--mem-ballast-size-mib") + _, b := getKeyValue(args, "--mem-ballast-size-mib") ballastSize, _ := strconv.Atoi(b) if (ballastSize * 2) > memLimit { log.Fatalf("Memory limit (%v) is less than 2x ballast (%v). Increase memory limit or decrease ballast size.", memLimit, ballastSize) @@ -287,6 +312,19 @@ func setMemoryLimit(memTotalSizeMiB int) { log.Printf("Set memory limit to %d MiB", memLimit) } +// Returns a ParserProvider that reads configuration YAML from an environment variable when applicable. +func newBaseParserProvider() parserprovider.ParserProvider { + _, configPathFlag := getKeyValue(os.Args[1:], "--config") + configPathVar := os.Getenv(configEnvVarName) + configYaml := os.Getenv(configYamlEnvVarName) + + if configPathFlag == "" && configPathVar == "" && configYaml != "" { + return parserprovider.NewInMemory(bytes.NewBufferString(configYaml)) + } + + return parserprovider.Default() +} + func runInteractive(params service.CollectorSettings) error { app, err := service.New(params) if err != nil { diff --git a/cmd/otelcol/main_test.go b/cmd/otelcol/main_test.go index ee64f0e3c16..2b4564d2de5 100644 --- a/cmd/otelcol/main_test.go +++ b/cmd/otelcol/main_test.go @@ -16,8 +16,14 @@ package main import ( + "bytes" + "fmt" + "log" "os" + "path" "testing" + + "github.com/stretchr/testify/assert" ) func TestContains(t *testing.T) { @@ -50,7 +56,7 @@ func TestGetKeyValue(t *testing.T) { {"foo", "--test", "foo"}, } for _, v := range testArgs { - result := getKeyValue(v, "--test") + _, result := getKeyValue(v, "--test") if result != v[0] { t.Errorf("Expected %v got %v", v[0], v) } @@ -59,9 +65,8 @@ func TestGetKeyValue(t *testing.T) { func TestCheckRuntimeParams(*testing.T) { oldArgs := os.Args - os.Setenv(configEnvVarName, "../../"+defaultLocalSAPMConfig) - setConfig() - os.Unsetenv(configEnvVarName) + os.Setenv(configEnvVarName, path.Join("../../", defaultLocalSAPMConfig)) + checkConfig() checkRuntimeParams() os.Args = oldArgs @@ -80,7 +85,7 @@ func TestCheckRuntimeParams(*testing.T) { func HelperTestSetMemoryBallast(val string, t *testing.T) { args := os.Args[1:] - c := getKeyValue(args, "--mem-ballast-size-mib") + _, c := getKeyValue(args, "--mem-ballast-size-mib") if c != val { t.Errorf("Expected memory ballast CLI param %v got %v", val, c) } @@ -98,18 +103,127 @@ func HelperTestSetMemoryLimit(val string, t *testing.T) { } func TestUseConfigFromEnvVar(t *testing.T) { - os.Setenv(tokenEnvVarName, "12345") - os.Setenv(realmEnvVarName, "us0") - os.Setenv(configEnvVarName, "../../"+defaultLocalSAPMConfig) - setConfig() + oldArgs := os.Args + defer func() { + os.Args = oldArgs + }() + + configPath := path.Join("../../", defaultLocalSAPMConfig) + os.Setenv(configEnvVarName, configPath) + defer os.Unsetenv(configEnvVarName) + checkConfig() args := os.Args[1:] - c := getKeyValue(args, "--config") - if c != "../../"+defaultLocalSAPMConfig { + _, c := getKeyValue(args, "--config") + if c != path.Join("../../", defaultLocalSAPMConfig) { t.Error("Config CLI param not set as expected") } } +func TestConfigPrecedence(t *testing.T) { + validPath1 := path.Join("../../", defaultLocalSAPMConfig) + validPath2 := path.Join("../../", defaultLocalOTLPConfig) + validConfig := `receivers: + hostmetrics: + collection_interval: 1s + scrapers: + cpu: +exporters: + logging: + logLevel: debug +service: + pipelines: + metrics: + receivers: [hostmetrics] + exporters: [logging]` + + tests := []struct { + name string + configFlagVal string // Flag --config value + splunkConfigVal string // Environment variable SPLUNK_CONFIG value + splunkConfigYamlVal string // Environment variable SPLUNK_CONFIG_YAML value + expectedLogs []string + unexpectedLogs []string + }{ + { + name: "Flag --config precedences env SPLUNK_CONFIG and SPLUNK_CONFIG_YAML", + configFlagVal: validPath1, + splunkConfigVal: validPath2, + splunkConfigYamlVal: validConfig, + expectedLogs: []string{ + fmt.Sprintf("Both environment variable SPLUNK_CONFIG and flag '--config' were specified. Using the flag value %s and ignoring the environment variable value %s in this session", validPath1, validPath2), + fmt.Sprintf("Both environment variable SPLUNK_CONFIG_YAML and flag '--config' were specified. Using the flag value %s and ignoring the environment variable in this session", validPath1), + fmt.Sprintf("Set config to %v", validPath1), + }, + unexpectedLogs: []string{ + fmt.Sprintf("Set config to %v", validPath2), + fmt.Sprintf("Using environment variable %s for configuration", configYamlEnvVarName), + }, + }, + { + name: "env SPLUNK_CONFIG precedences SPLUNK_CONFIG_YAML", + configFlagVal: "", + splunkConfigVal: validPath2, + splunkConfigYamlVal: validConfig, + expectedLogs: []string{ + fmt.Sprintf("Both %s and %s were specified. Using %s environment variable value %s for this session", configEnvVarName, configYamlEnvVarName, configEnvVarName, validPath2), + fmt.Sprintf("Set config to %v", validPath2), + }, + unexpectedLogs: []string{ + fmt.Sprintf("Set config to %v", validPath1), + fmt.Sprintf("Using environment variable %s for configuration", configYamlEnvVarName), + }, + }, + { + name: "env SPLUNK_CONFIG_YAML used when flag --config and env SPLUNK_CONFIG not specified", + configFlagVal: "", + splunkConfigVal: "", + splunkConfigYamlVal: validConfig, + expectedLogs: []string{ + fmt.Sprintf("Using environment variable %s for configuration", configYamlEnvVarName), + }, + unexpectedLogs: []string{ + fmt.Sprintf("Set config to %v", validPath1), + fmt.Sprintf("Set config to %v", validPath2), + }, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + func() { + oldArgs := os.Args + oldWriter := log.Default().Writer() + + defer func() { + os.Args = oldArgs + os.Unsetenv(configEnvVarName) + os.Unsetenv(configYamlEnvVarName) + log.Default().SetOutput(oldWriter) + }() + + actualLogsBuf := new(bytes.Buffer) + log.Default().SetOutput(actualLogsBuf) + if test.configFlagVal != "" { + os.Args = append(os.Args, "--config="+test.configFlagVal) + } + os.Setenv(configEnvVarName, test.splunkConfigVal) + os.Setenv(configYamlEnvVarName, test.splunkConfigYamlVal) + + checkConfig() + + actualLogs := actualLogsBuf.String() + + for _, expectedLog := range test.expectedLogs { + assert.Contains(t, actualLogs, expectedLog) + } + for _, unexpectedLog := range test.unexpectedLogs { + assert.NotContains(t, actualLogs, unexpectedLog) + } + }() + }) + } +} + func TestSetMemoryBallast(t *testing.T) { oldArgs := os.Args setMemoryBallast(100) @@ -118,6 +232,7 @@ func TestSetMemoryBallast(t *testing.T) { os.Args = oldArgs os.Setenv(ballastEnvVarName, "50") + defer os.Unsetenv(ballastEnvVarName) setMemoryBallast(100) HelperTestSetMemoryBallast("50", t) diff --git a/docs/getting-started/linux-manual.md b/docs/getting-started/linux-manual.md index 9be16e5adec..cfe791e1fc2 100644 --- a/docs/getting-started/linux-manual.md +++ b/docs/getting-started/linux-manual.md @@ -139,13 +139,21 @@ $ docker run --rm -e SPLUNK_ACCESS_TOKEN=12345 -e SPLUNK_REALM=us0 \ ### Custom Configuration -When changes to the default configuration file is needed, a custom -configuration file can specified and used instead. Use the `SPLUNK_CONFIG` -environment variable or the `--config` command line argument to provide a -custom configuration. +When changes to the default configuration YAML file are needed, create a +custom configuration file. Use environment variable `SPLUNK_CONFIG` or +command line argument `--config` to provide the path to this file. + +Also, you can use environment variable `SPLUNK_CONFIG_YAML` to specify +your custom configuration YAML at the command line. This is useful in +environments where access to the underlying file system is not readily +available. For example, in AWS Fargate you can store your custom configuration +YAML in a parameter in AWS Systems Manager Parameter Store, then in +your container definition specify `SPLUNK_CONFIG_YAML` to get the +configuration from the parameter. > Command line arguments take precedence over environment variables. This -> applies to `--config` and `--mem-ballast-size-mib`. +> applies to `--config` and `--mem-ballast-size-mib`. `SPLUNK_CONFIG` +> takes precedence over `SPLUNK_CONFIG_YAML` For example in Docker: @@ -160,11 +168,41 @@ $ docker run --rm -e SPLUNK_ACCESS_TOKEN=12345 -e SPLUNK_REALM=us0 \ > Use of a SemVer tag over `latest` is highly recommended. -In the case of Docker, a volume mount may be required to load custom -configuration as shown above. +In the case of Docker, a volume mount may be required to load the custom +configuration file, as shown above. -If the custom configuration includes a `memory_limiter` processor then the +If the custom configuration includes a `memory_limiter` processor, then the `ballast_size_mib` parameter should be the same as the `SPLUNK_BALLAST_SIZE_MIB` environment variable. See [gateway_config.yaml](../../cmd/otelcol/config/collector/gateway_config.yaml) as an example. + +The following example shows the `SPLUNK_CONFIG_YAML` environment variable: +```bash +$ CONFIG_YAML=$(cat <<-END +receivers: + hostmetrics: + collection_interval: 1s + scrapers: + cpu: +exporters: + logging: + logLevel: debug +service: + pipelines: + metrics: + receivers: [hostmetrics] + exporters: [logging] +END +) + +$ docker run --rm \ + -e SPLUNK_CONFIG_YAML=${CONFIG_YAML} \ + --name otelcol quay.io/signalfx/splunk-otel-collector:latest +``` +The configuration YAML above is for collecting and logging cpu +metrics. The YAML is assigned to parameter `CONFIG_YAML` for +convenience in the first command. In the subsequent `docker run` +command, parameter `CONFIG_YAML` is expanded and assigned to +environment variable `SPLUNK_CONFIG_YAML`. Note that YAML +requires whitespace indentation to be maintained. diff --git a/internal/configprovider/config_source_provider.go b/internal/configprovider/config_source_provider.go index 6ff55e46ff6..75d940483cb 100644 --- a/internal/configprovider/config_source_provider.go +++ b/internal/configprovider/config_source_provider.go @@ -37,9 +37,12 @@ type configSourceParserProvider struct { } // NewConfigSourceParserProvider creates a ParserProvider that uses config sources. -func NewConfigSourceParserProvider(logger *zap.Logger, buildInfo component.BuildInfo, factories ...Factory) parserprovider.ParserProvider { +func NewConfigSourceParserProvider(pp parserprovider.ParserProvider, logger *zap.Logger, buildInfo component.BuildInfo, factories ...Factory) parserprovider.ParserProvider { + if pp == nil { + pp = parserprovider.Default() + } return &configSourceParserProvider{ - pp: parserprovider.Default(), + pp: pp, logger: logger, factories: factories, buildInfo: buildInfo, diff --git a/internal/configprovider/config_source_provider_test.go b/internal/configprovider/config_source_provider_test.go index d5e6c763c2f..1615eab9640 100644 --- a/internal/configprovider/config_source_provider_test.go +++ b/internal/configprovider/config_source_provider_test.go @@ -88,6 +88,7 @@ func TestConfigSourceParserProvider(t *testing.T) { } pp := NewConfigSourceParserProvider( + parserprovider.Default(), zap.NewNop(), component.DefaultBuildInfo(), factories..., diff --git a/tests/general/container_test.go b/tests/general/container_test.go index 5fc083d24aa..c107b4fd75c 100644 --- a/tests/general/container_test.go +++ b/tests/general/container_test.go @@ -96,8 +96,66 @@ func TestSpecifiedContainerConfigDefaultsToCmdLineArgIfEnvVarConflict(t *testing for _, log := range logs.All() { if strings.Contains( log.Message, - `Both SPLUNK_CONFIG and '--config' were specified. Overriding "/not/a/real/path" environment `+ - `variable value with "/etc/config.yaml" for this session`, + `Both environment variable SPLUNK_CONFIG and flag '--config' were specified. `+ + `Using the flag value /etc/config.yaml and ignoring the environment variable value `+ + `/not/a/real/path in this session`, + ) { + return true + } + } + return false + }, 20*time.Second, time.Second) + + require.Eventually(t, func() bool { + for _, log := range logs.All() { + // logged host metric to confirm basic functionality + if strings.Contains(log.Message, "Value: ") { + return true + } + } + return false + }, 5*time.Second, time.Second) +} + +func TestConfigYamlEnvVarUsingLogs(t *testing.T) { + image := os.Getenv("SPLUNK_OTEL_COLLECTOR_IMAGE") + if strings.TrimSpace(image) == "" { + t.Skipf("skipping container-only test") + } + + logCore, logs := observer.New(zap.DebugLevel) + logger := zap.New(logCore) + + configYamlEnv := map[string]string{"SPLUNK_CONFIG_YAML": `receivers: + hostmetrics: + collection_interval: 1s + scrapers: + cpu: +exporters: + logging: + logLevel: debug +service: + pipelines: + metrics: + receivers: [hostmetrics] + exporters: [logging]`} + + collector, err := testutils.NewCollectorContainer(). + WithImage(image). + WithEnv(configYamlEnv). + WithLogger(logger). + Build() + + require.NoError(t, err) + require.NotNil(t, collector) + require.NoError(t, collector.Start()) + defer func() { require.NoError(t, collector.Shutdown()) }() + + require.Eventually(t, func() bool { + for _, log := range logs.All() { + if strings.Contains( + log.Message, + `Using environment variable SPLUNK_CONFIG_YAML for configuration`, ) { return true }