diff --git a/.chloggen/fix-expandedvalue-sanitization-on-struct-collection.yaml b/.chloggen/fix-expandedvalue-sanitization-on-struct-collection.yaml new file mode 100644 index 00000000000..73604d838e1 --- /dev/null +++ b/.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: [] diff --git a/confmap/internal/decoder.go b/confmap/internal/decoder.go index 7009c9f410b..f3844379f74 100644 --- a/confmap/internal/decoder.go +++ b/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 } diff --git a/confmap/internal/e2e/go.mod b/confmap/internal/e2e/go.mod index 4063faad71f..fea3ce9fd8a 100644 --- a/confmap/internal/e2e/go.mod +++ b/confmap/internal/e2e/go.mod @@ -8,6 +8,7 @@ require ( go.opentelemetry.io/collector/confmap v1.50.0 go.opentelemetry.io/collector/confmap/provider/envprovider v1.50.0 go.opentelemetry.io/collector/confmap/provider/fileprovider v1.50.0 + go.opentelemetry.io/collector/featuregate v1.50.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.144.0 // indirect - go.opentelemetry.io/collector/featuregate v1.50.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 diff --git a/confmap/internal/e2e/maplist_expanded_test.go b/confmap/internal/e2e/maplist_expanded_test.go new file mode 100644 index 00000000000..c1d12f7f850 --- /dev/null +++ b/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) +} diff --git a/confmap/internal/featuregates.go b/confmap/internal/featuregates.go index 47f9348dc42..2a186f55d12 100644 --- a/confmap/internal/featuregates.go +++ b/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"), +)