-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix ExpandedValue sanitization on struct collection #14413
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
3fbac1d
64dd47b
959d5a9
143c270
a74434c
97efc61
7eef7e1
8597240
d77eb64
4ce2ced
b28a536
80a019a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| // 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" | ||
| ) | ||
|
|
||
| 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 | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| 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) | ||
| } | ||
|
|
||
| // 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) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 DeferExpandedValueSanitizationOnStructCollection = featuregate.GlobalRegistry().MustRegister( | ||
| "confmap.deferExpandedValueSanitizationOnStructCollection", | ||
| featuregate.StageBeta, | ||
| featuregate.WithRegisterFromVersion("v0.144.0"), | ||
| featuregate.WithRegisterDescription("Disables early sanitization of ExpandedValue during config unmarshalling, allowing mapstructure to handle type conversion at the field level. Fixes decoding errors when environment variable values are parsed as non-string types (e.g., numbers, booleans) but need to be assigned to string fields."), | ||
| featuregate.WithRegisterReferenceURL("https://github.com/open-telemetry/opentelemetry-collector/pull/14413#issuecomment-3754949484"), | ||
|
||
| ) | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still kept the original fix inside the old behavior since it's still required to fix the problem I encountered.
I'm happy to remove it if you think it's unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I doubt the original fix would cause issues, but out of caution, I'd rather have disabling the feature gate return to the previous behavior, bug included. I figure that if someone's setup is broken by the new fix, I think there's a reasonable chance they will be broken by the original fix as well.