Skip to content

Commit

Permalink
[confmap] Fix bug where expand didn't honor escaping (#10560)
Browse files Browse the repository at this point in the history
#### 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
  • Loading branch information
TylerHelmuth authored Jul 10, 2024
1 parent 182c610 commit 637b1f4
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 1 deletion.
25 changes: 25 additions & 0 deletions .chloggen/confmap-fix-escape-bug.yaml
Original file line number Diff line number Diff line change
@@ -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: Fixes issue where confmap could not escape `$$` when `confmap.unifyEnvVarExpansion` is enabled.

# One or more tracking issues or pull requests related to the change
issues: [10560]

# (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: []
16 changes: 16 additions & 0 deletions confmap/expand.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ func (mr *Resolver) expandValue(ctx context.Context, value any) (any, bool, erro
// findURI attempts to find the first potentially expandable URI in input. It returns a potentially expandable
// URI, or an empty string if none are found.
// Note: findURI is only called when input contains a closing bracket.
// We do not support escaping nested URIs (such as ${env:$${FOO}}, since that would result in an invalid outer URI (${env:${FOO}}).
func (mr *Resolver) findURI(input string) string {
closeIndex := strings.Index(input, "}")
remaining := input[closeIndex+1:]
Expand All @@ -98,6 +99,21 @@ func (mr *Resolver) findURI(input string) string {
return mr.findURI(remaining)
}

index := openIndex - 1
currentRune := '$'
count := 0
for index >= 0 && currentRune == '$' {
currentRune = rune(input[index])
if currentRune == '$' {
count++
}
index--
}
// if we found an odd number of immediately $ preceding ${, then the expansion is escaped
if count%2 == 1 {
return ""
}

return input[openIndex : closeIndex+1]
}

Expand Down
43 changes: 43 additions & 0 deletions confmap/expand_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -577,3 +577,46 @@ func TestResolverDefaultProviderExpand(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, map[string]any{"foo": "localhost"}, cfgMap.ToStringMap())
}

func Test_EscapedEnvVars(t *testing.T) {
const mapValue2 = "some map value"

expectedMap := map[string]any{
"test_map": map[string]any{
"recv.1": "$MAP_VALUE_1",
"recv.2": "$$MAP_VALUE_2",
"recv.3": "$$MAP_VALUE_3",
"recv.4": "$" + mapValue2,
"recv.5": "some${MAP_VALUE_4}text",
"recv.6": "${ONE}${TWO}",
"recv.7": "text$",
"recv.8": "$",
"recv.9": "${1}${env:2}",
"recv.10": "some${env:MAP_VALUE_4}text",
"recv.11": "${env:" + mapValue2 + "}",
"recv.12": "${env:${MAP_VALUE_2}}",
"recv.13": "env:MAP_VALUE_2}${MAP_VALUE_2}{",
"recv.14": "${env:MAP_VALUE_2${MAP_VALUE_2}",
"recv.15": "$" + mapValue2,
}}

fileProvider := newFakeProvider("file", func(_ context.Context, uri string, _ WatcherFunc) (*Retrieved, error) {
return NewRetrieved(newConfFromFile(t, uri[5:]))
})
envProvider := newFakeProvider("env", func(_ context.Context, uri string, _ WatcherFunc) (*Retrieved, error) {
if uri == "env:MAP_VALUE_2" {
return NewRetrieved(mapValue2)
}
return nil, errors.New("should not be expanding any other env vars")
})

resolver, err := NewResolver(ResolverSettings{URIs: []string{filepath.Join("testdata", "expand-escaped-env.yaml")}, ProviderFactories: []ProviderFactory{fileProvider, envProvider}, ConverterFactories: nil, DefaultScheme: "env"})
require.NoError(t, err)

// Test that expanded configs are the same with the simple config with no env vars.
cfgMap, err := resolver.Resolve(context.Background())
require.NoError(t, err)
m := cfgMap.ToStringMap()
assert.Equal(t, expectedMap, m)

}
10 changes: 9 additions & 1 deletion confmap/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import (

"go.uber.org/multierr"
"go.uber.org/zap"

"go.opentelemetry.io/collector/internal/featuregates"
)

// follows drive-letter specification:
Expand Down Expand Up @@ -171,7 +173,13 @@ func (mr *Resolver) Resolve(ctx context.Context) (*Conf, error) {
if err != nil {
return nil, err
}
cfgMap[k] = val

if v, ok := val.(string); ok && featuregates.UseUnifiedEnvVarExpansionRules.IsEnabled() {
cfgMap[k] = strings.ReplaceAll(v, "$$", "$")
} else {
cfgMap[k] = val
}

}
retMap = NewFromStringMap(cfgMap)

Expand Down
31 changes: 31 additions & 0 deletions confmap/testdata/expand-escaped-env.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
test_map:
# $$ -> escaped $
recv.1: "$$MAP_VALUE_1"
# $$$ -> escaped $ + $MAP_VALUE_2
recv.2: "$$$MAP_VALUE_2"
# $$$$ -> two escaped $
recv.3: "$$$$MAP_VALUE_3"
# $$$ -> escaped $ + substituted env var
recv.4: "$$${MAP_VALUE_2}"
# escaped $ in the middle
recv.5: "some$${MAP_VALUE_4}text"
# two escaped $
recv.6: "$${ONE}$${TWO}"
# trailing escaped $
recv.7: "text$$"
# escaped $ alone
recv.8: "$$"
# Escape numbers
recv.9: "$${1}$${env:2}"
# can escape provider
recv.10: "some$${env:MAP_VALUE_4}text"
# can escape outer when nested
recv.11: "$${env:${MAP_VALUE_2}}"
# can escape inner and outer when nested
recv.12: "$${env:$${MAP_VALUE_2}}"
# can escape partial
recv.13: "env:MAP_VALUE_2}$${MAP_VALUE_2}{"
# can escape partial
recv.14: "${env:MAP_VALUE_2$${MAP_VALUE_2}"
# $$$ -> escaped $ + substituted env var
recv.15: "$$${env:MAP_VALUE_2}"

0 comments on commit 637b1f4

Please sign in to comment.