Fix bug requiring $$ when using config source variables

Previously, if a user wanted to use a config source variable (which have the
form ${env:MY_ENV_VAR}) they would have to use two dollar signs, like
$${env:MY_ENV_VAR}. This is because the expandMapProvider, which lives in
core and expands ${MY_ENV_VAR} -style variables, would run before the config
source providers, and it would replace any ${env:MY_ENV_VAR}s with "" but
convert any $${env:MY_ENV_VAR} to ${env:MY_ENV_VAR}. The fix proposed here
is to reverse the order in which these providers run: now the config source
providers run first, then expandMapProvider runs. In addition, this change
fixes up any $${env:MY_ENV_VAR} workarounds users may have put in place.

cmd/otelcol/main.go

@@ -18,7 +18,6 @@
 package main
 
 import (
-	"bytes"
 	"flag"
 	"fmt"
 	"io"
@@ -30,12 +29,9 @@ import (
 	"go.opentelemetry.io/collector/component"
 	"go.opentelemetry.io/collector/config/configmapprovider"
 	"go.opentelemetry.io/collector/service"
-	"go.uber.org/zap"
 
+	"github.com/signalfx/splunk-otel-collector/internal/collectorconfig"
 	"github.com/signalfx/splunk-otel-collector/internal/components"
-	"github.com/signalfx/splunk-otel-collector/internal/configconverter"
-	"github.com/signalfx/splunk-otel-collector/internal/configprovider"
-	"github.com/signalfx/splunk-otel-collector/internal/configsources"
 	"github.com/signalfx/splunk-otel-collector/internal/version"
 )
 
@@ -82,36 +78,43 @@ func main() {
 		Version: version.Version,
 	}
 
-	parserProvider := configprovider.NewConfigSourceParserProvider(
-		newBaseParserProvider(),
-		zap.NewNop(), // The service logger is not available yet, setting it to NoP.
+	serviceParams := service.CollectorSettings{
+		BuildInfo:         info,
+		Factories:         factories,
+		ConfigMapProvider: newConfigMapProvider(info),
+	}
+
+	if err := run(serviceParams); err != nil {
+		log.Fatal(err)
+	}
+}
+
+func newConfigMapProvider(info component.BuildInfo) configmapprovider.Provider {
+	return collectorconfig.NewConfigMapProvider(
 		info,
-		configsources.Get()...,
+		hasNoConvertFlag(),
+		getConfigPath(),
+		os.Getenv(configYamlEnvVarName),
+		getSetProperties(),
 	)
+}
 
+func hasNoConvertFlag() bool {
 	const noConvertConfigFlag = "--no-convert-config"
 	if hasFlag(noConvertConfigFlag) {
 		// the collector complains about this flag if we don't remove it
 		removeFlag(&os.Args, noConvertConfigFlag)
-	} else {
-		parserProvider = configconverter.ParserProvider(
-			parserProvider,
-			configconverter.RemoveBallastKey,
-			configconverter.MoveOTLPInsecureKey,
-			configconverter.MoveHecTLS,
-			configconverter.RenameK8sTagger,
-		)
-	}
-
-	serviceParams := service.CollectorSettings{
-		BuildInfo:         info,
-		Factories:         factories,
-		ConfigMapProvider: parserProvider,
+		return true
 	}
+	return false
+}
 
-	if err := run(serviceParams); err != nil {
-		log.Fatal(err)
+func getConfigPath() string {
+	ok, configPath := getKeyValue(os.Args[1:], "--config")
+	if !ok {
+		return os.Getenv(configEnvVarName)
 	}
+	return configPath
 }
 
 // required to support --set functionality no longer directly parsed by the core config loader.
@@ -370,22 +373,6 @@ func setDefaultEnvVars() {
 	}
 }
 
-// Returns a ParserProvider that reads configuration YAML from an environment variable when applicable.
-func newBaseParserProvider() configmapprovider.Provider {
-	var configPath string
-	var ok bool
-	if ok, configPath = getKeyValue(os.Args[1:], "--config"); !ok {
-		configPath = os.Getenv(configEnvVarName)
-	}
-	configYaml := os.Getenv(configYamlEnvVarName)
-
-	if configPath == "" && configYaml != "" {
-		return configmapprovider.NewExpand(configmapprovider.NewInMemory(bytes.NewBufferString(configYaml)))
-	}
-
-	return configmapprovider.NewDefault(configPath, getSetProperties())
-}
-
 func runInteractive(settings service.CollectorSettings) error {
 	cmd := service.NewCommand(settings)
 	if err := cmd.Execute(); err != nil {

internal/collectorconfig/config_provider.go

@@ -0,0 +1,81 @@
+// Copyright The OpenTelemetry Authors
+//
+// 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 collectorconfig
+
+import (
+	"bytes"
+
+	"go.opentelemetry.io/collector/component"
+	"go.opentelemetry.io/collector/config/configmapprovider"
+	"go.uber.org/zap"
+
+	"github.com/signalfx/splunk-otel-collector/internal/configconverter"
+	"github.com/signalfx/splunk-otel-collector/internal/configprovider"
+	"github.com/signalfx/splunk-otel-collector/internal/configsources"
+)
+
+func NewConfigMapProvider(
+	info component.BuildInfo,
+	hasNoConvertConfigFlag bool,
+	configFlagPath string,
+	configYamlFromEnv string,
+	properties []string,
+) configmapprovider.Provider {
+	provider := newExpandProvider(info, configFlagPath, configYamlFromEnv, properties, hasNoConvertConfigFlag)
+	if !hasNoConvertConfigFlag {
+		provider = configconverter.Provider(
+			provider,
+			configconverter.RemoveBallastKey,
+			configconverter.MoveOTLPInsecureKey,
+			configconverter.MoveHecTLS,
+			configconverter.RenameK8sTagger,
+		)
+	}
+	return provider
+}
+
+func newExpandProvider(
+	info component.BuildInfo,
+	configPath string,
+	configYamlFromEnv string,
+	properties []string,
+	hasNoConvertConfigFlag bool,
+) configmapprovider.Provider {
+	provider := newBaseProvider(configPath, configYamlFromEnv, properties)
+	if !hasNoConvertConfigFlag {
+		// we have to convert any $${foo:bar} *before* the expand provider runs,
+		// so we do it here rather than in NewConfigMapProvider
+		provider = configconverter.Provider(
+			provider,
+			configconverter.ReplaceDollarDollar,
+		)
+	}
+	return configmapprovider.NewExpand(configprovider.NewConfigSourceParserProvider(
+		provider,
+		zap.NewNop(), // The service logger is not available yet, setting it to NoP.
+		info,
+		configsources.Get()...,
+	))
+}
+
+func newBaseProvider(configPath string, configYamlFromEnv string, properties []string) configmapprovider.Provider {
+	if configPath == "" && configYamlFromEnv != "" {
+		return configmapprovider.NewInMemory(bytes.NewBufferString(configYamlFromEnv))
+	}
+	return configmapprovider.NewMerge(
+		configmapprovider.NewFile(configPath),
+		configmapprovider.NewProperties(properties),
+	)
+}

internal/collectorconfig/config_provider_test.go

@@ -0,0 +1,52 @@
+// Copyright The OpenTelemetry Authors
+//
+// 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 collectorconfig
+
+import (
+	"context"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.opentelemetry.io/collector/component"
+	"go.opentelemetry.io/collector/config/configmapprovider"
+)
+
+func TestParseConfigSource_ConfigFile(t *testing.T) {
+	_ = os.Setenv("CFG_SRC", "my_cfg_src_val")
+	_ = os.Setenv("LEGACY", "my_legacy_val")
+	pp := NewConfigMapProvider(component.BuildInfo{}, false, "testdata/config.yaml", "", nil)
+	assertProviderOK(t, pp)
+}
+
+func TestParseConfigSource_InMemory(t *testing.T) {
+	cfgYaml, err := os.ReadFile("testdata/config.yaml")
+	require.NoError(t, err)
+	pp := NewConfigMapProvider(component.BuildInfo{}, false, "", string(cfgYaml), nil)
+	assertProviderOK(t, pp)
+}
+
+func assertProviderOK(t *testing.T, provider configmapprovider.Provider) {
+	ctx := context.Background()
+	retrieved, err := provider.Retrieve(ctx, nil)
+	require.NoError(t, err)
+	cfgMap, err := retrieved.Get(ctx)
+	require.NoError(t, err)
+	v := cfgMap.Get("config_source_env_key")
+	assert.Equal(t, "my_cfg_src_val", v)
+	v = cfgMap.Get("legacy_env_key")
+	assert.Equal(t, "my_legacy_val", v)
+}

internal/collectorconfig/testdata/config.yaml

@@ -0,0 +1,4 @@
+config_sources:
+  env:
+config_source_env_key: ${env:CFG_SRC}
+legacy_env_key: $LEGACY

internal/configconverter/dollar_dollar.go

@@ -0,0 +1,48 @@
+// Copyright The OpenTelemetry Authors
+//
+// 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 configconverter
+
+import (
+	"log"
+	"regexp"
+
+	"go.opentelemetry.io/collector/config"
+)
+
+// ReplaceDollarDollar replaces any $${foo:MY_VAR} config source variables with
+// ${foo:MY_VAR}. These might exist because of customers working around a bug
+// in how the Collector expanded these variables.
+func ReplaceDollarDollar(m *config.Map) *config.Map {
+	re := dollarDollarRegex()
+	for _, k := range m.AllKeys() {
+		v := m.Get(k)
+		if s, ok := v.(string); ok {
+			replaced := replaceDollarDollar(re, s)
+			if replaced != s {
+				log.Printf("[WARNING] the notation %q is no longer recommended. Please replace with %q.\n", v, replaced)
+				m.Set(k, replaced)
+			}
+		}
+	}
+	return m
+}
+
+func dollarDollarRegex() *regexp.Regexp {
+	return regexp.MustCompile(`\$\${(.+?:.+?)}`)
+}
+
+func replaceDollarDollar(re *regexp.Regexp, s string) string {
+	return re.ReplaceAllString(s, "${$1}")
+}

internal/configconverter/dollar_dollar_test.go

@@ -0,0 +1,52 @@
+// Copyright The OpenTelemetry Authors
+//
+// 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 configconverter
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.opentelemetry.io/collector/config/configmapprovider"
+)
+
+func TestReplaceDollarDollar(t *testing.T) {
+	pp := &converterProvider{
+		wrapped:     configmapprovider.NewFile("testdata/dollar-dollar.yaml"),
+		cfgMapFuncs: []CfgMapFunc{ReplaceDollarDollar},
+	}
+	r, err := pp.Retrieve(context.Background(), nil)
+	require.NoError(t, err)
+	v, err := r.Get(context.Background())
+	require.NoError(t, err)
+	endpt := v.Get("exporters::otlp::endpoint")
+	assert.Equal(t, "localhost:${env:OTLP_PORT}", endpt)
+	insecure := v.Get("exporters::otlp::insecure")
+	assert.Equal(t, "$OTLP_INSECURE", insecure)
+	pwd := v.Get("receivers::redis::password")
+	assert.Equal(t, "$$ecret", pwd)
+	host := v.Get("receivers::redis::host")
+	assert.Equal(t, "ho$$tname:${env:PORT}", host)
+}
+
+func TestRegexReplace(t *testing.T) {
+	assert.Equal(t, "${foo/bar:PORT}", testReplace("$${foo/bar:PORT}"))
+	assert.Equal(t, "$${PORT}", testReplace("$${PORT}"))
+}
+
+func testReplace(s string) string {
+	return replaceDollarDollar(dollarDollarRegex(), s)
+}

internal/configconverter/parser_provider.go

@@ -35,7 +35,7 @@ type CfgMapFunc func(*config.Map) *config.Map
 
 var _ configmapprovider.Provider = (*converterProvider)(nil)
 
-func ParserProvider(wrapped configmapprovider.Provider, funcs ...CfgMapFunc) configmapprovider.Provider {
+func Provider(wrapped configmapprovider.Provider, funcs ...CfgMapFunc) configmapprovider.Provider {
 	return &converterProvider{wrapped: wrapped, cfgMapFuncs: funcs}
 }
 

internal/configconverter/testdata/dollar-dollar.yaml

@@ -0,0 +1,19 @@
+receivers:
+  hostmetrics:
+    collection_interval: 1s
+    scrapers:
+      cpu:
+  redis:
+    password: $$ecret
+    host: "ho$$tname:$${env:PORT}"
+exporters:
+  otlp:
+    endpoint: localhost:$${env:OTLP_PORT}
+    insecure: $OTLP_INSECURE
+service:
+  pipelines:
+    metrics:
+      receivers:
+        - hostmetrics
+      exporters:
+        - otlp