[confmap] Store original string in confmap.Conf

.chloggen/mx-psi_string-value-for-string-fields.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: breaking
+
+# 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: When passing configuration for a string field using any provider, use the verbatim string representation as the value.
+
+# One or more tracking issues or pull requests related to the change
+issues: [10605, 10405]
+
+# (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 matches the behavior of `${ENV}` syntax prior to the promotion of the `confmap.unifyEnvVarExpansion` feature gate
+  to beta. It changes the behavior of the `${env:ENV}` syntax with escaped strings.
+
+# 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/confmap.go

@@ -108,9 +108,34 @@
 	return l.Merge(NewFromStringMap(out))
 }
 
+func (l *Conf) unsanitizedGet(key string) any {
+	return l.k.Get(key)
+}
+
+func sanitize(a any) any {
+	switch m := a.(type) {
+	case map[string]any:
+		c := maps.Copy(m)
+		for k, v := range m {
+			c[k] = sanitize(v)
+		}
+		return c
+	case []any:
+		var newSlice []any
+		for _, e := range m {
+			newSlice = append(newSlice, sanitize(e))
+		}
+		return newSlice
+	case expandedValue:
+		return m.Value
+	}
+	return a
+}
+
 // Get can retrieve any value given the key to use.
 func (l *Conf) Get(key string) any {
-	return l.k.Get(key)
+	val := l.unsanitizedGet(key)
+	return sanitize(val)
 }
 
 // IsSet checks to see if the key has been set in any of the data locations.
@@ -128,7 +153,7 @@
 // It returns an error is the sub-config is not a map[string]any (use Get()), and an empty Map if none exists.
 func (l *Conf) Sub(key string) (*Conf, error) {
 	// Code inspired by the koanf "Cut" func, but returns an error instead of empty map for unsupported sub-config type.
-	data := l.Get(key)
+	data := l.unsanitizedGet(key)
 	if data == nil {
 		return New(), nil
 	}
@@ -140,9 +165,14 @@
 	return nil, fmt.Errorf("unexpected sub-config value kind for key:%s value:%v kind:%v", key, data, reflect.TypeOf(data).Kind())
 }
 
+func (l *Conf) toStringMapWithExpand() map[string]any {
+	m := maps.Unflatten(l.k.All(), KeyDelimiter)
+	return m
+}
+
 // ToStringMap creates a map[string]any from a Parser.
 func (l *Conf) ToStringMap() map[string]any {
-	return maps.Unflatten(l.k.All(), KeyDelimiter)
+	return sanitize(l.toStringMapWithExpand()).(map[string]any)
 }
 
 // decodeConfig decodes the contents of the Conf into the result argument, using a
@@ -160,6 +190,7 @@
 		WeaklyTypedInput: !globalgates.StrictlyTypedInputGate.IsEnabled(),
 		MatchName:        caseSensitiveMatchName,
 		DecodeHook: mapstructure.ComposeDecodeHookFunc(
+			useExpandValue(),
 			expandNilStructPointersHookFunc(),
 			mapstructure.StringToSliceHookFunc(","),
 			mapKeyStringToMapKeyTextUnmarshalerHookFunc(),
@@ -177,7 +208,7 @@
 	if err != nil {
 		return err
 	}
-	if err = decoder.Decode(m.ToStringMap()); err != nil {
+	if err = decoder.Decode(m.toStringMapWithExpand()); err != nil {
 		if strings.HasPrefix(err.Error(), "error decoding ''") {
 			return errors.Unwrap(err)
 		}
@@ -206,6 +237,40 @@
 	return a == b
 }
 
+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).
+	return exp.Value, nil
+}
+
+// When a value has been loaded from an external source via a provider, we keep both the
+// parsed value and the original string value. This allows us to expand the value to its
+// original string representation when decoding into a string field, and use the original otherwise.
+func useExpandValue() mapstructure.DecodeHookFuncType {
+	return func(
+		_ reflect.Type,
+		to reflect.Type,
+		data any) (any, error) {
+		if exp, ok := data.(expandedValue); ok {
+			return castTo(exp, to.Kind() == reflect.String)
+		}
+
+		// If the target field is a map or slice, sanitize input to remove expandedValue references.
+		switch to.Kind() {
+		case reflect.Array, reflect.Slice, reflect.Map:
+			// This does not handle map[string]string and []string explicitly.
+			return sanitize(data), nil
+		}
+		return data, nil
+	}
+}
+
 // In cases where a config has a mapping of something to a struct pointers
 // we want nil values to resolve to a pointer to the zero value of the
 // underlying struct just as we want nil values of a mapping of something

confmap/confmap_test.go

@@ -845,3 +845,35 @@ func TestRecursiveUnmarshaling(t *testing.T) {
 	require.NoError(t, conf.Unmarshal(r))
 	require.Equal(t, "something", r.Foo)
 }
+
+func TestExpandedValue(t *testing.T) {
+	cm := NewFromStringMap(map[string]any{
+		"key": expandedValue{
+			Value:       0xdeadbeef,
+			HasOriginal: true,
+			Original:    "original",
+		}})
+	assert.Equal(t, 0xdeadbeef, cm.Get("key"))
+	assert.Equal(t, map[string]any{"key": 0xdeadbeef}, cm.ToStringMap())
+
+	type ConfigStr struct {
+		Key string `mapstructure:"key"`
+	}
+
+	cfgStr := ConfigStr{}
+	assert.NoError(t, cm.Unmarshal(&cfgStr))
+	assert.Equal(t, "original", cfgStr.Key)
+
+	type ConfigInt struct {
+		Key int `mapstructure:"key"`
+	}
+	cfgInt := ConfigInt{}
+	assert.NoError(t, cm.Unmarshal(&cfgInt))
+	assert.Equal(t, 0xdeadbeef, cfgInt.Key)
+
+	type ConfigBool struct {
+		Key bool `mapstructure:"key"`
+	}
+	cfgBool := ConfigBool{}
+	assert.Error(t, cm.Unmarshal(&cfgBool))
+}

confmap/expand.go

@@ -43,6 +43,40 @@
 
 func (mr *Resolver) expandValue(ctx context.Context, value any) (any, bool, error) {
 	switch v := value.(type) {
+	case expandedValue:
+		expanded, changed, err := mr.expandValue(ctx, v.Value)
+		if err != nil {
+			return nil, false, err
+		}
+
+		switch exp := expanded.(type) {
+		case expandedValue, string:
+			// Return expanded values or strings verbatim.
+			return exp, changed, nil
+		}
+
+		// 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
+		}
+
+		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,
+			}, changed || originalChanged, nil
+		}
+
+		result := expandedValue{
+			Value:       expanded,
+			Original:    v.Original,
+			HasOriginal: v.HasOriginal,
+		}
+
+		return result, changed || originalChanged, nil
 	case string:
 		if !strings.Contains(v, "${") || !strings.Contains(v, "}") {
 			// No URIs to expand.
@@ -117,6 +151,20 @@
 	return input[openIndex : closeIndex+1]
 }
 
+// expandedValue holds the YAML parsed value and original representation of a value.
+// It keeps track of the original representation to be used by the 'useExpandValue' hook
+// if the target field is a string. We need to keep both representations because we don't know
+// what the target field type is until `Unmarshal` is called.
+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
+}
+
 // findAndExpandURI attempts to find and expand the first occurrence of an expandable URI in input. If an expandable URI is found it
 // returns the input with the URI expanded, true and nil. Otherwise, it returns the unchanged input, false and the expanding error.
 // This method expects input to start with ${ and end with }
@@ -134,10 +182,17 @@
 			return input, false, err
 		}
 
-		expanded, err := ret.AsRaw()
+		expanded := expandedValue{}
+		expanded.Value, err = ret.AsRaw()
 		if err != nil {
 			return input, false, err
 		}
+
+		if asStr, err2 := ret.AsString(); err2 == nil {
+			expanded.HasOriginal = true
+			expanded.Original = asStr
+		}
+
 		return expanded, true, err
 	}
 	expanded, err := mr.expandURI(ctx, uri)

confmap/internal/e2e/fuzz_test.go

@@ -0,0 +1,88 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+package e2etest
+
+import (
+	"context"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// targetNested tests the following property:
+// > Passing a value of type T directly through an environment variable
+// > should be equivalent to passing it through a nested environment variable.
+func targetNested[T any](t *testing.T, value string) {
+	resolver := NewResolver(t, "types_expand.yaml")
+
+	// Use os.Setenv so we can check the error and return instead of failing the fuzzing.
+	os.Setenv("ENV", "${env:ENV2}") // nolint:tenv
+	defer os.Unsetenv("ENV")
+	err := os.Setenv("ENV2", value) // nolint:tenv
+	defer os.Unsetenv("ENV2")
+	if err != nil {
+		return
+	}
+	confNested, errResolveNested := resolver.Resolve(context.Background())
+
+	err = os.Setenv("ENV", value) // nolint:tenv
+	if err != nil {
+		return
+	}
+	confSimple, errResolveSimple := resolver.Resolve(context.Background())
+	require.Equal(t, errResolveNested, errResolveSimple)
+	if errResolveNested != nil {
+		return
+	}
+
+	var cfgNested TargetConfig[T]
+	errNested := confNested.Unmarshal(cfgNested)
+
+	var cfgSimple TargetConfig[T]
+	errSimple := confSimple.Unmarshal(cfgSimple)
+
+	require.Equal(t, errNested, errSimple)
+	if errNested != nil {
+		return
+	}
+	assert.Equal(t, cfgNested, cfgSimple)
+}
+
+// testStrings for fuzzing targets
+var testStrings = []string{
+	"123",
+	"opentelemetry",
+	"!!str 123",
+	"\"0123\"",
+	"\"",
+	"1111:1111:1111:1111:1111::",
+	"{field: value}",
+	"0xdeadbeef",
+	"0b101",
+	"field:",
+	"2006-01-02T15:04:05Z07:00",
+}
+
+func FuzzNestedString(f *testing.F) {
+	for _, value := range testStrings {
+		f.Add(value)
+	}
+	f.Fuzz(targetNested[string])
+}
+
+func FuzzNestedInt(f *testing.F) {
+	for _, value := range testStrings {
+		f.Add(value)
+	}
+	f.Fuzz(targetNested[int])
+}
+
+func FuzzNestedMap(f *testing.F) {
+	for _, value := range testStrings {
+		f.Add(value)
+	}
+	f.Fuzz(targetNested[map[string]any])
+}