diff --git a/.chloggen/mx-psi_remove-has-original.yaml b/.chloggen/mx-psi_remove-has-original.yaml new file mode 100644 index 00000000000..9f1417fca57 --- /dev/null +++ b/.chloggen/mx-psi_remove-has-original.yaml @@ -0,0 +1,25 @@ +# 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: Remove original string representation if invalid. + +# One or more tracking issues or pull requests related to the change +issues: [10787] + +# (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: + +# 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/confmap.go b/confmap/confmap.go index 59524527b06..4e881f9d804 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -240,9 +240,6 @@ func caseSensitiveMatchName(a, b string) bool { func castTo(exp expandedValue, useOriginal bool) (any, error) { // If the target field is a string, use `exp.Original` or fail if not available. if globalgates.StrictlyTypedInputGate.IsEnabled() && useOriginal { - if !exp.HasOriginal { - return nil, fmt.Errorf("cannot expand value to string: original value not set") - } return exp.Original, nil } // Otherwise, use the parsed value (previous behavior). diff --git a/confmap/confmap_test.go b/confmap/confmap_test.go index 713583a7115..ac48d0359e6 100644 --- a/confmap/confmap_test.go +++ b/confmap/confmap_test.go @@ -849,9 +849,8 @@ func TestRecursiveUnmarshaling(t *testing.T) { func TestExpandedValue(t *testing.T) { cm := NewFromStringMap(map[string]any{ "key": expandedValue{ - Value: 0xdeadbeef, - HasOriginal: true, - Original: "original", + Value: 0xdeadbeef, + Original: "original", }}) assert.Equal(t, 0xdeadbeef, cm.Get("key")) assert.Equal(t, map[string]any{"key": 0xdeadbeef}, cm.ToStringMap()) diff --git a/confmap/expand.go b/confmap/expand.go index 56dc512382b..09e1907eeb6 100644 --- a/confmap/expand.go +++ b/confmap/expand.go @@ -58,25 +58,19 @@ func (mr *Resolver) expandValue(ctx context.Context, value any) (any, bool, erro // At this point we don't know the target field type, so we need to expand the original representation as well. originalExpanded, originalChanged, err := mr.expandValue(ctx, v.Original) if err != nil { - return nil, false, err + // The original representation is not valid, return the expanded value. + return expanded, changed, nil } if originalExpanded, ok := originalExpanded.(string); ok { // If the original representation is a string, return the expanded value with the original representation. return expandedValue{ - Value: expanded, - Original: originalExpanded, - HasOriginal: true, + Value: expanded, + Original: originalExpanded, }, changed || originalChanged, nil } - result := expandedValue{ - Value: expanded, - Original: v.Original, - HasOriginal: v.HasOriginal, - } - - return result, changed || originalChanged, nil + return expanded, changed, nil case string: if !strings.Contains(v, "${") || !strings.Contains(v, "}") { // No URIs to expand. @@ -158,10 +152,7 @@ func (mr *Resolver) findURI(input string) string { type expandedValue struct { // Value is the expanded value. Value any - // HasOriginal is true if the original representation is set. - HasOriginal bool // Original is the original representation of the value. - // It is only valid if HasOriginal is true. Original string } @@ -182,18 +173,19 @@ func (mr *Resolver) findAndExpandURI(ctx context.Context, input string) (any, bo return input, false, err } - expanded := expandedValue{} - expanded.Value, err = ret.AsRaw() + val, err := ret.AsRaw() if err != nil { return input, false, err } if asStr, err2 := ret.AsString(); err2 == nil { - expanded.HasOriginal = true - expanded.Original = asStr + return expandedValue{ + Value: val, + Original: asStr, + }, true, nil } - return expanded, true, err + return val, true, nil } expanded, err := mr.expandURI(ctx, uri) if err != nil { diff --git a/confmap/internal/e2e/testdata/issue-10787-main.yaml b/confmap/internal/e2e/testdata/issue-10787-main.yaml new file mode 100644 index 00000000000..08bb0e61756 --- /dev/null +++ b/confmap/internal/e2e/testdata/issue-10787-main.yaml @@ -0,0 +1,22 @@ +receivers: + otlp: + protocols: + grpc: + endpoint: 0.0.0.0:4317 + http: + endpoint: 0.0.0.0:4318 +processors: + batch: + +exporters: + ${file:testdata/issue-10787-snippet.yaml} + +service: + telemetry: + metrics: + level: detailed + pipelines: + traces: + receivers: [otlp] + processors: [batch] + exporters: [logging] diff --git a/confmap/internal/e2e/testdata/issue-10787-snippet.yaml b/confmap/internal/e2e/testdata/issue-10787-snippet.yaml new file mode 100644 index 00000000000..13872dcd46a --- /dev/null +++ b/confmap/internal/e2e/testdata/issue-10787-snippet.yaml @@ -0,0 +1,3 @@ +# ${hello.world} +logging: + verbosity: detailed diff --git a/confmap/internal/e2e/types_test.go b/confmap/internal/e2e/types_test.go index cb04b5d30f4..c96847f9c8d 100644 --- a/confmap/internal/e2e/types_test.go +++ b/confmap/internal/e2e/types_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/confmap" @@ -24,6 +25,7 @@ const ( TargetFieldString TargetField = "string_field" TargetFieldBool TargetField = "bool_field" TargetFieldInlineString TargetField = "inline_string_field" + TargetFieldSlice TargetField = "slice_field" ) type Test struct { @@ -78,6 +80,9 @@ func AssertResolvesTo(t *testing.T, resolver *confmap.Resolver, tt Test) { case TargetFieldBool: var cfg TargetConfig[bool] AssertExpectedMatch(t, tt, conf, &cfg) + case TargetFieldSlice: + var cfg TargetConfig[[]any] + AssertExpectedMatch(t, tt, conf, &cfg) default: t.Fatalf("unexpected target field %q", tt.targetField) } @@ -345,6 +350,22 @@ func TestStrictTypeCasting(t *testing.T) { targetField: TargetFieldInlineString, expected: "inline field with 2006-01-02T15:04:05Z07:00 expansion", }, + // issue 10787 + { + value: "true # comment with a ${env:hello.world} reference", + targetField: TargetFieldBool, + expected: true, + }, + { + value: "true # comment with a ${env:hello.world} reference", + targetField: TargetFieldString, + unmarshalErr: `expected type 'string', got unconvertible type 'bool'`, + }, + { + value: "true # comment with a ${env:hello.world} reference", + targetField: TargetFieldInlineString, + resolveErr: `environment variable "hello.world" has invalid name`, + }, // issue 10759 { value: `["a",`, @@ -356,6 +377,12 @@ func TestStrictTypeCasting(t *testing.T) { targetField: TargetFieldInlineString, expected: `inline field with ["a", expansion`, }, + // issue 10799 + { + value: `[filelog,windowseventlog/application]`, + targetField: TargetFieldSlice, + expected: []any{"filelog", "windowseventlog/application"}, + }, } previousValue := globalgates.StrictlyTypedInputGate.IsEnabled() @@ -494,3 +521,106 @@ func TestRecursiveMaps(t *testing.T) { cfgStr.Field, ) } + +// Test that comments with invalid ${env:...} references do not prevent configuration from loading. +func TestIssue10787(t *testing.T) { + previousValue := globalgates.StrictlyTypedInputGate.IsEnabled() + err := featuregate.GlobalRegistry().Set(globalgates.StrictlyTypedInputID, true) + require.NoError(t, err) + defer func() { + seterr := featuregate.GlobalRegistry().Set(globalgates.StrictlyTypedInputID, previousValue) + require.NoError(t, seterr) + }() + + resolver := NewResolver(t, "issue-10787-main.yaml") + conf, err := resolver.Resolve(context.Background()) + require.NoError(t, err) + assert.Equal(t, conf.ToStringMap(), + map[string]any{ + "exporters": map[string]any{ + "logging": map[string]any{ + "verbosity": "detailed", + }, + }, + "processors": map[string]any{ + "batch": nil, + }, + "receivers": map[string]any{ + "otlp": map[string]any{ + "protocols": map[string]any{ + "grpc": map[string]any{ + "endpoint": "0.0.0.0:4317", + }, + "http": map[string]any{ + "endpoint": "0.0.0.0:4318", + }, + }, + }, + }, + "service": map[string]any{ + "pipelines": map[string]any{ + "traces": map[string]any{ + "exporters": []any{"logging"}, + "processors": []any{"batch"}, + "receivers": []any{"otlp"}, + }, + }, + "telemetry": map[string]any{ + "metrics": map[string]any{ + "level": "detailed", + }, + }, + }, + }, + ) +} + +func TestStructMappingIssue10787(t *testing.T) { + previousValue := globalgates.StrictlyTypedInputGate.IsEnabled() + err := featuregate.GlobalRegistry().Set(globalgates.StrictlyTypedInputID, true) + require.NoError(t, err) + defer func() { + seterr := featuregate.GlobalRegistry().Set(globalgates.StrictlyTypedInputID, previousValue) + require.NoError(t, seterr) + }() + + resolver := NewResolver(t, "types_expand.yaml") + t.Setenv("ENV", `# ${hello.world} +logging: + verbosity: detailed`) + conf, err := resolver.Resolve(context.Background()) + require.NoError(t, err) + + type Logging struct { + Verbosity string `mapstructure:"verbosity"` + } + type Exporters struct { + Logging Logging `mapstructure:"logging"` + } + type Target struct { + Field Exporters `mapstructure:"field"` + } + + var cfg Target + err = conf.Unmarshal(&cfg) + require.NoError(t, err) + require.Equal(t, + Target{Field: Exporters{ + Logging: Logging{ + Verbosity: "detailed", + }, + }}, + cfg, + ) + + confStr, err := resolver.Resolve(context.Background()) + require.NoError(t, err) + var cfgStr TargetConfig[string] + err = confStr.Unmarshal(&cfgStr) + require.NoError(t, err) + require.Equal(t, `# ${hello.world} +logging: + verbosity: detailed`, + cfgStr.Field, + ) +}