Allow setting both SPLUNK_CONFIG and --config with priority given to …

…--config (#450)

* Allow setting both SPLUNK_CONFIG and --config with cmdline priority

* testutils: add Collector.WillFail(), connect Args, and specify config dynamically

* Add endtoend doc.go for integration-test target success

cmd/otelcol/main.go

@@ -127,14 +127,17 @@ func checkRuntimeParams() {
 	args := os.Args[1:]
 	config := ""
 
-	// Check if config flag was passed
-	// If so, ensure config env var is not set
-	// Then set config properly
+	// 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 != "" {
-			log.Fatalf("Both %v and '--config' were specified, but only one is allowed", 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)
 	}
@@ -210,7 +213,7 @@ func setConfig() {
 		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 environment variable %s", v)
+				log.Fatalf("ERROR: Missing required environment variable %s with default config path %s", v, config)
 			}
 		}
 	}

tests/endtoend/doc.go

@@ -0,0 +1 @@
+package endtoend

tests/general/container_test.go

@@ -0,0 +1,117 @@
+// Copyright Splunk, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package tests
+
+import (
+	"fmt"
+	"os"
+	"path"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+	"go.uber.org/zap/zaptest/observer"
+
+	"github.com/signalfx/splunk-otel-collector/tests/testutils"
+)
+
+func TestDefaultContainerConfigRequiresEnvVars(t *testing.T) {
+	image := os.Getenv("SPLUNK_OTEL_COLLECTOR_IMAGE")
+	if strings.TrimSpace(image) == "" {
+		t.Skipf("skipping container-only test")
+	}
+
+	tests := []struct {
+		name    string
+		env     map[string]string
+		missing string
+	}{
+		{"missing realm", map[string]string{
+			"SPLUNK_REALM":        "",
+			"SPLUNK_ACCESS_TOKEN": "some_token",
+		}, "SPLUNK_REALM"},
+		{"missing token", map[string]string{
+			"SPLUNK_REALM":        "some_realm",
+			"SPLUNK_ACCESS_TOKEN": "",
+		}, "SPLUNK_ACCESS_TOKEN"},
+	}
+	for _, testcase := range tests {
+		t.Run(testcase.name, func(tt *testing.T) {
+			logCore, logs := observer.New(zap.DebugLevel)
+			logger := zap.New(logCore)
+
+			collector, err := testutils.NewCollectorContainer().WithImage(image).WithEnv(testcase.env).WithLogger(logger).WillFail(true).Build()
+			require.NoError(t, err)
+			require.NotNil(t, collector)
+			defer collector.Shutdown()
+			require.NoError(t, collector.Start())
+
+			expectedError := fmt.Sprintf("ERROR: Missing required environment variable %s with default config path /etc/otel/collector/gateway_config.yaml", testcase.missing)
+			require.Eventually(t, func() bool {
+				for _, log := range logs.All() {
+					if strings.Contains(log.Message, expectedError) {
+						return true
+					}
+				}
+				return false
+			}, 30*time.Second, time.Second)
+		})
+	}
+}
+
+func TestSpecifiedContainerConfigDefaultsToCmdLineArgIfEnvVarConflict(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)
+
+	env := map[string]string{"SPLUNK_CONFIG": "/not/a/real/path"}
+	config := path.Join(".", "testdata", "logged_hostmetrics.yaml")
+	c := testutils.NewCollectorContainer().WithImage(image).WithEnv(env).WithLogger(logger).WithConfigPath(config)
+	// specify in container path of provided config via cli.
+	collector, err := c.WithArgs("--config", "/etc/config.yaml").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,
+				`Both SPLUNK_CONFIG and '--config' were specified. Overriding "/not/a/real/path" environment `+
+					`variable value with "/etc/config.yaml" for 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)
+}

tests/general/testdata/logged_hostmetrics.yaml

@@ -0,0 +1,15 @@
+receivers:
+  hostmetrics:
+    collection_interval: 1s
+    scrapers:
+      cpu:
+
+exporters:
+  logging:
+    logLevel: debug
+
+service:
+  pipelines:
+    metrics:
+      receivers: [hostmetrics]
+      exporters: [logging]

tests/testutils/collector.go

@@ -24,6 +24,7 @@ type Collector interface {
 	WithEnv(env map[string]string) Collector
 	WithLogger(logger *zap.Logger) Collector
 	WithLogLevel(level string) Collector
+	WillFail(fail bool) Collector
 	Build() (Collector, error)
 	Start() error
 	Shutdown() error

tests/testutils/collector_container.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"strings"
 
 	"github.com/testcontainers/testcontainers-go"
 	"go.uber.org/zap"
@@ -36,6 +37,7 @@ type CollectorContainer struct {
 	Ports          []string
 	Logger         *zap.Logger
 	LogLevel       string
+	Fail           bool
 	Container      Container
 	contextArchive io.Reader
 	logConsumer    collectorLogConsumer
@@ -63,7 +65,7 @@ func (collector CollectorContainer) WithConfigPath(path string) Collector {
 	return &collector
 }
 
-// []string{} by default, but currently a noop
+// []string{} by default
 func (collector CollectorContainer) WithArgs(args ...string) Collector {
 	collector.Args = args
 	return &collector
@@ -87,6 +89,11 @@ func (collector CollectorContainer) WithLogLevel(level string) Collector {
 	return &collector
 }
 
+func (collector CollectorContainer) WillFail(fail bool) Collector {
+	collector.Fail = fail
+	return &collector
+}
+
 func (collector CollectorContainer) Build() (Collector, error) {
 	if collector.Image == "" {
 		collector.Image = "quay.io/signalfx/splunk-otel-collector:latest"
@@ -107,11 +114,19 @@ func (collector CollectorContainer) Build() (Collector, error) {
 	}
 	collector.Container = collector.Container.WithContextArchive(
 		collector.contextArchive,
-	).WithNetworkMode("host").WillWaitForLogs("Everything is ready. Begin running and processing data.")
+	).WithNetworkMode("host")
 
 	collector.Container = collector.Container.WithExposedPorts(collector.Ports...)
 
-	collector.Container = collector.Container.WithCmd("--config", "/etc/config.yaml", "--log-level", "debug")
+	if collector.Fail {
+		collector.Container = collector.Container.WillWaitForLogs("")
+	} else {
+		collector.Container = collector.Container.WillWaitForLogs("Everything is ready. Begin running and processing data.")
+	}
+
+	if len(collector.Args) > 0 {
+		collector.Container = collector.Container.WithCmd(collector.Args...)
+	}
 
 	collector.Container = *(collector.Container.Build())
 
@@ -139,7 +154,7 @@ func (collector *CollectorContainer) Shutdown() error {
 	return collector.Container.Terminate(context.Background())
 }
 
-func (collector CollectorContainer) buildContextArchive() (io.Reader, error) {
+func (collector *CollectorContainer) buildContextArchive() (io.Reader, error) {
 	var buf bytes.Buffer
 	tarWriter := tar.NewWriter(&buf)
 
@@ -163,13 +178,26 @@ func (collector CollectorContainer) buildContextArchive() (io.Reader, error) {
 			return nil, err
 		}
 
-		dockerfile += `
-COPY config.yaml /etc/config.yaml
-# ENV SPLUNK_CONFIG=/etc/config.yaml
+		dockerfile += "COPY config.yaml /etc/config.yaml\n"
 
-ENV SPLUNK_ACCESS_TOKEN=12345
-ENV SPLUNK_REALM=us0
-`
+		// We need to tell the Collector to use the provided config
+		// but only if not already done so in the test
+		var configSetByArgs bool
+		for _, c := range collector.Args {
+			if strings.Contains(c, "--config") {
+				configSetByArgs = true
+			}
+		}
+		_, configSetByEnvVar := collector.Container.Env["SPLUNK_CONFIG"]
+		if !configSetByArgs && !configSetByEnvVar {
+			// only specify w/ args if none are used in the test
+			if len(collector.Args) == 0 {
+				collector.Args = append(collector.Args, "--config", "/etc/config.yaml")
+			} else {
+				// fallback to env var
+				collector.Container.Env["SPLUNK_CONFIG"] = "/etc/config.yaml"
+			}
+		}
 	}
 
 	header := tar.Header{

tests/testutils/collector_container_test.go

@@ -43,6 +43,16 @@ func TestCollectorContainerBuilders(t *testing.T) {
 	assert.Equal(t, []string{"arg_one", "arg_two", "arg_three"}, withArgs.Args)
 	assert.Empty(t, builder.Args)
 
+	willFail, ok := builder.WillFail(true).(*CollectorContainer)
+	require.True(t, ok)
+	assert.True(t, willFail.Fail)
+	assert.False(t, builder.Fail)
+
+	wontFail, ok := willFail.WillFail(false).(*CollectorContainer)
+	require.True(t, ok)
+	assert.False(t, wontFail.Fail)
+	assert.True(t, willFail.Fail)
+
 	logger := zap.NewNop()
 	withLogger, ok := builder.WithLogger(logger).(*CollectorContainer)
 	require.True(t, ok)

tests/testutils/collector_process.go

@@ -36,6 +36,7 @@ type CollectorProcess struct {
 	Env              map[string]string
 	Logger           *zap.Logger
 	LogLevel         string
+	Fail             bool
 	Process          *subprocess.Subprocess
 	subprocessConfig *subprocess.Config
 }
@@ -81,6 +82,12 @@ func (collector CollectorProcess) WithLogLevel(level string) Collector {
 	return &collector
 }
 
+// noop at this time
+func (collector CollectorProcess) WillFail(fail bool) Collector {
+	collector.Fail = fail
+	return &collector
+}
+
 func (collector CollectorProcess) Build() (Collector, error) {
 	if collector.ConfigPath == "" && collector.Args == nil {
 		return nil, fmt.Errorf("you must specify a ConfigPath for your CollectorProcess before building")