Fix ExpandedValue sanitization on struct collection

.chloggen/fix-expandedvalue-sanitization-on-struct-collection.yaml

@@ -0,0 +1,26 @@
+# 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. receiver/otlp)
+component: pkg/confmap
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: 'Fix an issue where configs could fail to decode when using interpolated values in string fields.'
+
+# One or more tracking issues or pull requests related to the change
+issues: [14413]
+
+# (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: |
+   For example, a header can be set via an environment variable to a string that is parseable as a number, e.g. `1234`
+
+# 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: []

confmap/internal/decoder.go

@@ -93,14 +93,17 @@ func useExpandValue() mapstructure.DecodeHookFuncType {
 			return v, nil
 		}
 
-		switch to.Kind() {
-		case reflect.Array, reflect.Slice, reflect.Map:
-			if isStringyStructure(to) {
-				// If the target field is a stringy structure, sanitize to use the original string value everywhere.
-				return sanitizeToStr(data), nil
+		if !NewExpandedValueSanitizer.IsEnabled() {
+			switch to.Kind() {
+			case reflect.Array, reflect.Slice, reflect.Map:
+				if isStringyStructure(to) {
+					// If the target field is a stringy structure, sanitize to use the original string value everywhere.
+					return sanitizeToStr(data), nil
+				}
+
+				// Otherwise, sanitize to use the parsed value everywhere.
+				return sanitize(data), nil
 			}
-			// Otherwise, sanitize to use the parsed value everywhere.
-			return sanitize(data), nil
 		}
 		return data, nil
 	}

confmap/internal/e2e/go.mod

@@ -8,6 +8,7 @@ require (
 	go.opentelemetry.io/collector/confmap v1.49.0
 	go.opentelemetry.io/collector/confmap/provider/envprovider v1.49.0
 	go.opentelemetry.io/collector/confmap/provider/fileprovider v1.49.0
+	go.opentelemetry.io/collector/featuregate v1.49.0
 )
 
 require (
@@ -22,7 +23,6 @@ require (
 	github.com/mitchellh/reflectwalk v1.0.2 // indirect
 	github.com/pmezard/go-difflib v1.0.0 // indirect
 	go.opentelemetry.io/collector/confmap/xconfmap v0.143.0 // indirect
-	go.opentelemetry.io/collector/featuregate v1.49.0 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
 	go.uber.org/zap v1.27.1 // indirect
 	go.yaml.in/yaml/v3 v3.0.4 // indirect

confmap/internal/e2e/maplist_expanded_test.go

@@ -0,0 +1,137 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package e2etest
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"go.opentelemetry.io/collector/config/configopaque"
+	"go.opentelemetry.io/collector/confmap"
+	"go.opentelemetry.io/collector/confmap/internal"
+	"go.opentelemetry.io/collector/featuregate"
+)
+
+type testHeadersConfig struct {
+	Headers configopaque.MapList `mapstructure:"headers"`
+}
+
+// TestMapListWithExpandedValue tests that MapList can handle ExpandedValue
+// from environment variable expansion
+func TestMapListWithExpandedValue(t *testing.T) {
+	// Simulate what happens when ${env:TOKEN} is expanded
+	// The confmap will contain an ExpandedValue instead of a plain string
+	data := map[string]any{
+		"headers": []any{
+			map[string]any{
+				"name": "Authorization",
+				"value": internal.ExpandedValue{
+					Value:    "Bearer secret-token",
+					Original: "Bearer secret-token",
+				},
+			},
+		},
+	}
+
+	conf := confmap.NewFromStringMap(data)
+	var tc testHeadersConfig
+	err := conf.Unmarshal(&tc)
+	require.NoError(t, err)
+
+	val, ok := tc.Headers.Get("Authorization")
+	require.True(t, ok)
+	require.Equal(t, configopaque.String("Bearer secret-token"), val)
+}
+
+// TestMapListWithExpandedValueIntValue tests an ExpandedValue with an integer Value
+func TestMapListWithExpandedValueIntValue(t *testing.T) {
+	// Simulate what happens when expanding a value that parses as an int
+	data := map[string]any{
+		"headers": []any{
+			map[string]any{
+				"name": "X-Port",
+				"value": internal.ExpandedValue{
+					Value:    8080,   // Value is parsed as int
+					Original: "8080", // Original is string
+				},
+			},
+		},
+	}
+
+	originalState := internal.NewExpandedValueSanitizer.IsEnabled()
+	defer func() {
+		require.NoError(t, featuregate.GlobalRegistry().Set(internal.NewExpandedValueSanitizer.ID(), originalState))
+	}()
+
+	require.NoError(t, featuregate.GlobalRegistry().Set(internal.NewExpandedValueSanitizer.ID(), true))
+
+	conf := confmap.NewFromStringMap(data)
+	var tc testHeadersConfig
+	err := conf.Unmarshal(&tc)
+	require.NoError(t, err)
+
+	val, ok := tc.Headers.Get("X-Port")
+	require.True(t, ok)
+	require.Equal(t, configopaque.String("8080"), val)
+
+	require.NoError(t, featuregate.GlobalRegistry().Set(internal.NewExpandedValueSanitizer.ID(), false))
+
+	// This will fail because when reverting to old behavior, ExpandedValues get decoded at collection time and doesn't
+	// take struct collections into account.
+	err = conf.Unmarshal(&tc)
+	require.Error(t, err)
+}
+
+// TestDirectConfigopaqueStringWithExpandedValueIntValue tests that direct unmarshaling works
+func TestDirectConfigopaqueStringWithExpandedValueIntValue(t *testing.T) {
+	type testConfig struct {
+		Value configopaque.String `mapstructure:"value"`
+	}
+
+	// Direct configopaque.String field (not in a map/slice structure)
+	data := map[string]any{
+		"value": internal.ExpandedValue{
+			Value:    8080,
+			Original: "8080",
+		},
+	}
+
+	conf := confmap.NewFromStringMap(data)
+	var tc testConfig
+	err := conf.Unmarshal(&tc)
+	// This should work because useExpandValue detects the target is a string
+	require.NoError(t, err)
+	require.Equal(t, configopaque.String("8080"), tc.Value)
+}
+
+// TestStringyStructureWithExpandedValue tests the isStringyStructure path in useExpandValue
+func TestStringyStructureWithExpandedValue(t *testing.T) {
+	type testConfig struct {
+		Tags []string `mapstructure:"tags"`
+	}
+
+	data := map[string]any{
+		"tags": []any{
+			internal.ExpandedValue{
+				Value:    8080,
+				Original: "8080",
+			},
+		},
+	}
+
+	originalState := internal.NewExpandedValueSanitizer.IsEnabled()
+	defer func() {
+		require.NoError(t, featuregate.GlobalRegistry().Set(internal.NewExpandedValueSanitizer.ID(), originalState))
+	}()
+
+	// With feature gate disabled, useExpandValue should detect []string as stringy
+	require.NoError(t, featuregate.GlobalRegistry().Set(internal.NewExpandedValueSanitizer.ID(), false))
+
+	conf := confmap.NewFromStringMap(data)
+	var tc testConfig
+	err := conf.Unmarshal(&tc)
+	require.NoError(t, err)
+	require.Equal(t, []string{"8080"}, tc.Tags)
+}

confmap/internal/featuregates.go

@@ -12,3 +12,11 @@ var EnableMergeAppendOption = featuregate.GlobalRegistry().MustRegister(
 	featuregate.WithRegisterDescription("Combines lists when resolving configs from different sources. This feature gate will not be stabilized 'as is'; the current behavior will remain the default."),
 	featuregate.WithRegisterReferenceURL("https://github.com/open-telemetry/opentelemetry-collector/issues/8754"),
 )
+
+var NewExpandedValueSanitizer = featuregate.GlobalRegistry().MustRegister(
+	"confmap.newExpandedValueSanitizer",
+	featuregate.StageBeta,
+	featuregate.WithRegisterFromVersion("v0.144.0"),
+	featuregate.WithRegisterDescription("Fixes some types of decoding errors where environment variables are parsed as non-string types but assigned to string fields."),
+	featuregate.WithRegisterReferenceURL("https://github.com/open-telemetry/opentelemetry-collector/pull/14413"),
+)