[confmap] Fix expansion of escaped environment variables

This change fixes the issue where environment variables escaped with $$ were expanded. The collector now converts `$${ENV_VAR}` to `${ENV_VAR}` and `$$ENV_VAR` to `$ENV_VAR` without further expansion.

.chloggen/fix-env-var-double-escaping.yaml

@@ -0,0 +1,27 @@
+# Use this changelog template to create an entry for release notes.
+
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
+component: confmap
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Fix wrong expansion of environment variables escaped with `$$`, e.g. `$${ENV_VAR}` and `$$ENV_VAR`.
+
+# One or more tracking issues or pull requests related to the change
+issues: [10713]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext: |
+  This change fixes the issue where environment variables escaped with $$ were expanded. 
+  The collector now converts `$${ENV_VAR}` to `${ENV_VAR}` and `$$ENV_VAR` to `$ENV_VAR` without further expansion.
+
+# Optional: The change log or logs in which this entry should be included.
+# e.g. '[user]' or '[user, api]'
+# Include 'user' if the change is relevant to end users.
+# Include 'api' if there is a change to a library API.
+# Default: '[user]'
+change_logs: [api]

confmap/converter/expandconverter/expand.go

@@ -85,6 +85,7 @@ func (c converter) expandEnv(s string) (string, error) {
 		// - $FOO will be substituted with env var FOO
 		// - $$FOO will be replaced with $FOO
 		// - $$$FOO will be replaced with $ + substituted env var FOO
+		// TODO: Move the escaping of $$ out from the expand converter to the resolver.
 		if str == "$" {
 			return "$"
 		}

confmap/expand_test.go

@@ -577,46 +577,3 @@ func TestResolverDefaultProviderExpand(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, map[string]any{"foo": "localhost"}, cfgMap.ToStringMap())
 }
-
-func Test_EscapedEnvVars(t *testing.T) {
-	const mapValue2 = "some map value"
-
-	expectedMap := map[string]any{
-		"test_map": map[string]any{
-			"recv.1":  "$MAP_VALUE_1",
-			"recv.2":  "$$MAP_VALUE_2",
-			"recv.3":  "$$MAP_VALUE_3",
-			"recv.4":  "$" + mapValue2,
-			"recv.5":  "some${MAP_VALUE_4}text",
-			"recv.6":  "${ONE}${TWO}",
-			"recv.7":  "text$",
-			"recv.8":  "$",
-			"recv.9":  "${1}${env:2}",
-			"recv.10": "some${env:MAP_VALUE_4}text",
-			"recv.11": "${env:" + mapValue2 + "}",
-			"recv.12": "${env:${MAP_VALUE_2}}",
-			"recv.13": "env:MAP_VALUE_2}${MAP_VALUE_2}{",
-			"recv.14": "${env:MAP_VALUE_2${MAP_VALUE_2}",
-			"recv.15": "$" + mapValue2,
-		}}
-
-	fileProvider := newFakeProvider("file", func(_ context.Context, uri string, _ WatcherFunc) (*Retrieved, error) {
-		return NewRetrieved(newConfFromFile(t, uri[5:]))
-	})
-	envProvider := newFakeProvider("env", func(_ context.Context, uri string, _ WatcherFunc) (*Retrieved, error) {
-		if uri == "env:MAP_VALUE_2" {
-			return NewRetrieved(mapValue2)
-		}
-		return nil, errors.New("should not be expanding any other env vars")
-	})
-
-	resolver, err := NewResolver(ResolverSettings{URIs: []string{filepath.Join("testdata", "expand-escaped-env.yaml")}, ProviderFactories: []ProviderFactory{fileProvider, envProvider}, ConverterFactories: nil, DefaultScheme: "env"})
-	require.NoError(t, err)
-
-	// Test that expanded configs are the same with the simple config with no env vars.
-	cfgMap, err := resolver.Resolve(context.Background())
-	require.NoError(t, err)
-	m := cfgMap.ToStringMap()
-	assert.Equal(t, expectedMap, m)
-
-}

confmap/internal/e2e/expand_test.go

@@ -0,0 +1,94 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package e2etest
+
+import (
+	"context"
+	"fmt"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"go.opentelemetry.io/collector/confmap"
+	"go.opentelemetry.io/collector/confmap/converter/expandconverter"
+	"go.opentelemetry.io/collector/confmap/provider/envprovider"
+	"go.opentelemetry.io/collector/confmap/provider/fileprovider"
+	"go.opentelemetry.io/collector/confmap/provider/yamlprovider"
+)
+
+// Test_EscapedEnvVars tests that the resolver supports escaped env vars working together with expand converter.
+func Test_EscapedEnvVars(t *testing.T) {
+	tests := []struct {
+		name   string
+		scheme string
+	}{
+		{
+			name:   "no_default_scheme",
+			scheme: "",
+		},
+		{
+			name:   "env",
+			scheme: "env",
+		},
+	}
+
+	const expandedValue = "some expanded value"
+	t.Setenv("ENV_VALUE", expandedValue)
+
+	expectedFailures := map[string]string{
+		"$ENV_VALUE":   "variable substitution using $VAR has been deprecated in favor of ${VAR} and ${env:VAR}",
+		"$$$ENV_VALUE": "variable substitution using $VAR has been deprecated in favor of ${VAR} and ${env:VAR}",
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			expectedMap := map[string]any{
+				"test_map": map[string]any{
+					"key1":  "$ENV_VALUE",
+					"key2":  "$$ENV_VALUE",
+					"key3":  "$" + expandedValue,
+					"key4":  "some" + expandedValue + "text",
+					"key5":  "some${ENV_VALUE}text",
+					"key6":  "${ONE}${TWO}",
+					"key7":  "text$",
+					"key8":  "$",
+					"key9":  "${1}${env:2}",
+					"key10": "some${env:ENV_VALUE}text",
+					"key11": "${env:" + expandedValue + "}",
+					"key12": "${env:${ENV_VALUE}}",
+					"key13": "env:MAP_VALUE_2}${ENV_VALUE}{",
+					"key14": "$" + expandedValue,
+				},
+			}
+
+			resolver, err := confmap.NewResolver(confmap.ResolverSettings{
+				URIs:               []string{filepath.Join("testdata", "expand-escaped-env.yaml")},
+				ProviderFactories:  []confmap.ProviderFactory{fileprovider.NewFactory(), envprovider.NewFactory()},
+				ConverterFactories: []confmap.ConverterFactory{expandconverter.NewFactory()},
+				DefaultScheme:      tt.scheme,
+			})
+			require.NoError(t, err)
+
+			// Test that expanded configs are the same with the simple config with no env vars.
+			cfgMap, err := resolver.Resolve(context.Background())
+			require.NoError(t, err)
+			m := cfgMap.ToStringMap()
+			assert.Equal(t, expectedMap, m)
+
+			for val, expectedErr := range expectedFailures {
+				resolver, err = confmap.NewResolver(confmap.ResolverSettings{
+					URIs:               []string{fmt.Sprintf("yaml: test: %s", val)},
+					ProviderFactories:  []confmap.ProviderFactory{yamlprovider.NewFactory(), envprovider.NewFactory()},
+					ConverterFactories: []confmap.ConverterFactory{expandconverter.NewFactory()},
+					DefaultScheme:      tt.scheme,
+				})
+				require.NoError(t, err)
+				_, err := resolver.Resolve(context.Background())
+				require.ErrorContains(t, err, expectedErr)
+			}
+		})
+	}
+}

confmap/internal/e2e/go.mod

@@ -5,8 +5,10 @@ go 1.21.0
 require (
 	github.com/stretchr/testify v1.9.0
 	go.opentelemetry.io/collector/confmap v0.105.0
+	go.opentelemetry.io/collector/confmap/converter/expandconverter v0.105.0
 	go.opentelemetry.io/collector/confmap/provider/envprovider v0.105.0
 	go.opentelemetry.io/collector/confmap/provider/fileprovider v0.105.0
+	go.opentelemetry.io/collector/confmap/provider/yamlprovider v0.105.0
 	go.opentelemetry.io/collector/featuregate v1.12.0
 	go.opentelemetry.io/collector/internal/globalgates v0.105.0
 )
@@ -32,6 +34,10 @@ replace go.opentelemetry.io/collector/confmap/provider/fileprovider => ../../pro
 
 replace go.opentelemetry.io/collector/confmap/provider/envprovider => ../../provider/envprovider
 
+replace go.opentelemetry.io/collector/confmap/provider/yamlprovider => ../../provider/yamlprovider
+
 replace go.opentelemetry.io/collector/featuregate => ../../../featuregate
 
 replace go.opentelemetry.io/collector/internal/globalgates => ../../../internal/globalgates
+
+replace go.opentelemetry.io/collector/confmap/converter/expandconverter => ../../converter/expandconverter

confmap/internal/e2e/testdata/expand-escaped-env.yaml

@@ -0,0 +1,29 @@
+test_map:
+  # $$ -> escaped $
+  key1: "$$ENV_VALUE"
+  # $$$$ -> two escaped $
+  key2: "$$$$ENV_VALUE"
+  # $$ -> escaped $ + ${ENV_VALUE} expanded
+  key3: "$$${ENV_VALUE}"
+  # expanded in the middle
+  key4: "some${ENV_VALUE}text"
+  # escaped $ in the middle
+  key5: "some$${ENV_VALUE}text"
+  # two escaped $
+  key6: "$${ONE}$${TWO}"
+  # trailing escaped $
+  key7: "text$$"
+  # escaped $ alone
+  key8: "$$"
+  # escaped number and uri
+  key9: "$${1}$${env:2}"
+  # escape provider
+  key10: "some$${env:ENV_VALUE}text"
+  # can escape outer when nested
+  key11: "$${env:${ENV_VALUE}}"
+  # can escape inner and outer when nested
+  key12: "$${env:$${ENV_VALUE}}"
+  # can escape partial
+  key13: "env:MAP_VALUE_2}$${ENV_VALUE}{"
+  # $$$ -> escaped $ + expanded env var
+  key14: "$$${env:ENV_VALUE}"

confmap/resolver.go

@@ -12,8 +12,6 @@ import (
 
 	"go.uber.org/multierr"
 	"go.uber.org/zap"
-
-	"go.opentelemetry.io/collector/internal/globalgates"
 )
 
 // follows drive-letter specification:
@@ -173,13 +171,7 @@ func (mr *Resolver) Resolve(ctx context.Context) (*Conf, error) {
 		if err != nil {
 			return nil, err
 		}
-
-		if v, ok := val.(string); ok && globalgates.UseUnifiedEnvVarExpansionRules.IsEnabled() {
-			cfgMap[k] = strings.ReplaceAll(v, "$$", "$")
-		} else {
-			cfgMap[k] = val
-		}
-
+		cfgMap[k] = val
 	}
 	retMap = NewFromStringMap(cfgMap)