Skip to content

Commit 637b1f4

Browse files
authored
[confmap] Fix bug where expand didn't honor escaping (#10560)
#### Description When we promoted `confmap.unifyEnvVarExpansion` to beta, we found that the new expansion logic in `confmap` wasn't handling escaping of `$$` like it is supposed to. This PR fixes that bug, but adding escaping logic for `$$`. @azunna1 this fixes the bug you mentioned in #10435 around the metricstransformprocessor: ```yaml metricstransform: transforms: - include: '^k8s\.(.*)\.(.*)$$' match_type: regexp action: update new_name: 'kubernetes.$${1}.$${2}' - include: '^container_([0-9A-Za-z]+)_([0-9A-Za-z]+)_.*' match_type: regexp action: update new_name: 'container.$${1}.$${2}' ``` #### Testing Added new unit tests explicitly for escaping logic
1 parent 182c610 commit 637b1f4

File tree

5 files changed

+124
-1
lines changed

5 files changed

+124
-1
lines changed

.chloggen/confmap-fix-escape-bug.yaml

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
# Use this changelog template to create an entry for release notes.
2+
3+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
4+
change_type: bug_fix
5+
6+
# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver)
7+
component: confmap
8+
9+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
10+
note: Fixes issue where confmap could not escape `$$` when `confmap.unifyEnvVarExpansion` is enabled.
11+
12+
# One or more tracking issues or pull requests related to the change
13+
issues: [10560]
14+
15+
# (Optional) One or more lines of additional information to render under the primary note.
16+
# These lines will be padded with 2 spaces and then inserted directly into the document.
17+
# Use pipe (|) for multiline entries.
18+
subtext:
19+
20+
# Optional: The change log or logs in which this entry should be included.
21+
# e.g. '[user]' or '[user, api]'
22+
# Include 'user' if the change is relevant to end users.
23+
# Include 'api' if there is a change to a library API.
24+
# Default: '[user]'
25+
change_logs: []

confmap/expand.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ func (mr *Resolver) expandValue(ctx context.Context, value any) (any, bool, erro
8181
// findURI attempts to find the first potentially expandable URI in input. It returns a potentially expandable
8282
// URI, or an empty string if none are found.
8383
// Note: findURI is only called when input contains a closing bracket.
84+
// We do not support escaping nested URIs (such as ${env:$${FOO}}, since that would result in an invalid outer URI (${env:${FOO}}).
8485
func (mr *Resolver) findURI(input string) string {
8586
closeIndex := strings.Index(input, "}")
8687
remaining := input[closeIndex+1:]
@@ -98,6 +99,21 @@ func (mr *Resolver) findURI(input string) string {
9899
return mr.findURI(remaining)
99100
}
100101

102+
index := openIndex - 1
103+
currentRune := '$'
104+
count := 0
105+
for index >= 0 && currentRune == '$' {
106+
currentRune = rune(input[index])
107+
if currentRune == '$' {
108+
count++
109+
}
110+
index--
111+
}
112+
// if we found an odd number of immediately $ preceding ${, then the expansion is escaped
113+
if count%2 == 1 {
114+
return ""
115+
}
116+
101117
return input[openIndex : closeIndex+1]
102118
}
103119

confmap/expand_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -577,3 +577,46 @@ func TestResolverDefaultProviderExpand(t *testing.T) {
577577
require.NoError(t, err)
578578
assert.Equal(t, map[string]any{"foo": "localhost"}, cfgMap.ToStringMap())
579579
}
580+
581+
func Test_EscapedEnvVars(t *testing.T) {
582+
const mapValue2 = "some map value"
583+
584+
expectedMap := map[string]any{
585+
"test_map": map[string]any{
586+
"recv.1": "$MAP_VALUE_1",
587+
"recv.2": "$$MAP_VALUE_2",
588+
"recv.3": "$$MAP_VALUE_3",
589+
"recv.4": "$" + mapValue2,
590+
"recv.5": "some${MAP_VALUE_4}text",
591+
"recv.6": "${ONE}${TWO}",
592+
"recv.7": "text$",
593+
"recv.8": "$",
594+
"recv.9": "${1}${env:2}",
595+
"recv.10": "some${env:MAP_VALUE_4}text",
596+
"recv.11": "${env:" + mapValue2 + "}",
597+
"recv.12": "${env:${MAP_VALUE_2}}",
598+
"recv.13": "env:MAP_VALUE_2}${MAP_VALUE_2}{",
599+
"recv.14": "${env:MAP_VALUE_2${MAP_VALUE_2}",
600+
"recv.15": "$" + mapValue2,
601+
}}
602+
603+
fileProvider := newFakeProvider("file", func(_ context.Context, uri string, _ WatcherFunc) (*Retrieved, error) {
604+
return NewRetrieved(newConfFromFile(t, uri[5:]))
605+
})
606+
envProvider := newFakeProvider("env", func(_ context.Context, uri string, _ WatcherFunc) (*Retrieved, error) {
607+
if uri == "env:MAP_VALUE_2" {
608+
return NewRetrieved(mapValue2)
609+
}
610+
return nil, errors.New("should not be expanding any other env vars")
611+
})
612+
613+
resolver, err := NewResolver(ResolverSettings{URIs: []string{filepath.Join("testdata", "expand-escaped-env.yaml")}, ProviderFactories: []ProviderFactory{fileProvider, envProvider}, ConverterFactories: nil, DefaultScheme: "env"})
614+
require.NoError(t, err)
615+
616+
// Test that expanded configs are the same with the simple config with no env vars.
617+
cfgMap, err := resolver.Resolve(context.Background())
618+
require.NoError(t, err)
619+
m := cfgMap.ToStringMap()
620+
assert.Equal(t, expectedMap, m)
621+
622+
}

confmap/resolver.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ import (
1212

1313
"go.uber.org/multierr"
1414
"go.uber.org/zap"
15+
16+
"go.opentelemetry.io/collector/internal/featuregates"
1517
)
1618

1719
// follows drive-letter specification:
@@ -171,7 +173,13 @@ func (mr *Resolver) Resolve(ctx context.Context) (*Conf, error) {
171173
if err != nil {
172174
return nil, err
173175
}
174-
cfgMap[k] = val
176+
177+
if v, ok := val.(string); ok && featuregates.UseUnifiedEnvVarExpansionRules.IsEnabled() {
178+
cfgMap[k] = strings.ReplaceAll(v, "$$", "$")
179+
} else {
180+
cfgMap[k] = val
181+
}
182+
175183
}
176184
retMap = NewFromStringMap(cfgMap)
177185

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
test_map:
2+
# $$ -> escaped $
3+
recv.1: "$$MAP_VALUE_1"
4+
# $$$ -> escaped $ + $MAP_VALUE_2
5+
recv.2: "$$$MAP_VALUE_2"
6+
# $$$$ -> two escaped $
7+
recv.3: "$$$$MAP_VALUE_3"
8+
# $$$ -> escaped $ + substituted env var
9+
recv.4: "$$${MAP_VALUE_2}"
10+
# escaped $ in the middle
11+
recv.5: "some$${MAP_VALUE_4}text"
12+
# two escaped $
13+
recv.6: "$${ONE}$${TWO}"
14+
# trailing escaped $
15+
recv.7: "text$$"
16+
# escaped $ alone
17+
recv.8: "$$"
18+
# Escape numbers
19+
recv.9: "$${1}$${env:2}"
20+
# can escape provider
21+
recv.10: "some$${env:MAP_VALUE_4}text"
22+
# can escape outer when nested
23+
recv.11: "$${env:${MAP_VALUE_2}}"
24+
# can escape inner and outer when nested
25+
recv.12: "$${env:$${MAP_VALUE_2}}"
26+
# can escape partial
27+
recv.13: "env:MAP_VALUE_2}$${MAP_VALUE_2}{"
28+
# can escape partial
29+
recv.14: "${env:MAP_VALUE_2$${MAP_VALUE_2}"
30+
# $$$ -> escaped $ + substituted env var
31+
recv.15: "$$${env:MAP_VALUE_2}"

0 commit comments

Comments
 (0)