From 9ceeda241961bbf1dfb88cfc06e067cf67966902 Mon Sep 17 00:00:00 2001 From: Dmitry Anoshin Date: Tue, 23 Jul 2024 22:47:18 -0700 Subject: [PATCH] [confmap] Fix expansion of escaped environment variables This change fixes the issue where environment variables escaped with $$ were expanded. The collector now converts `$${ENV_VAR}` to `${ENV_VAR}` and `$$ENV_VAR` to `$ENV_VAR` without further expansion. --- .chloggen/fix-env-var-double-escaping.yaml | 27 ++++++ confmap/converter/expandconverter/expand.go | 1 + confmap/expand_test.go | 43 --------- confmap/internal/e2e/expand_test.go | 94 +++++++++++++++++++ confmap/internal/e2e/go.mod | 6 ++ .../e2e/testdata/expand-escaped-env.yaml | 29 ++++++ confmap/resolver.go | 10 +- confmap/testdata/expand-escaped-env.yaml | 31 ------ 8 files changed, 158 insertions(+), 83 deletions(-) create mode 100644 .chloggen/fix-env-var-double-escaping.yaml create mode 100644 confmap/internal/e2e/expand_test.go create mode 100644 confmap/internal/e2e/testdata/expand-escaped-env.yaml delete mode 100644 confmap/testdata/expand-escaped-env.yaml diff --git a/.chloggen/fix-env-var-double-escaping.yaml b/.chloggen/fix-env-var-double-escaping.yaml new file mode 100644 index 00000000000..5ea730a1965 --- /dev/null +++ b/.chloggen/fix-env-var-double-escaping.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: 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: Fix wrong expansion of environment variables escaped with `$$`, e.g. `$${ENV_VAR}` and `$$ENV_VAR`. + +# One or more tracking issues or pull requests related to the change +issues: [10713] + +# (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 change fixes the issue where environment variables escaped with $$ were expanded. + The collector now converts `$${ENV_VAR}` to `${ENV_VAR}` and `$$ENV_VAR` to `$ENV_VAR` without further expansion. + +# 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: [api] diff --git a/confmap/converter/expandconverter/expand.go b/confmap/converter/expandconverter/expand.go index e603ca9ce04..2c4af613499 100644 --- a/confmap/converter/expandconverter/expand.go +++ b/confmap/converter/expandconverter/expand.go @@ -85,6 +85,7 @@ func (c converter) expandEnv(s string) (string, error) { // - $FOO will be substituted with env var FOO // - $$FOO will be replaced with $FOO // - $$$FOO will be replaced with $ + substituted env var FOO + // TODO: Move the escaping of $$ out from the expand converter to the resolver. if str == "$" { return "$" } diff --git a/confmap/expand_test.go b/confmap/expand_test.go index dd406922948..53b244374b1 100644 --- a/confmap/expand_test.go +++ b/confmap/expand_test.go @@ -577,46 +577,3 @@ 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) - -} diff --git a/confmap/internal/e2e/expand_test.go b/confmap/internal/e2e/expand_test.go new file mode 100644 index 00000000000..018725d30d5 --- /dev/null +++ b/confmap/internal/e2e/expand_test.go @@ -0,0 +1,94 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +package e2etest + +import ( + "context" + "fmt" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "go.opentelemetry.io/collector/confmap" + "go.opentelemetry.io/collector/confmap/converter/expandconverter" + "go.opentelemetry.io/collector/confmap/provider/envprovider" + "go.opentelemetry.io/collector/confmap/provider/fileprovider" + "go.opentelemetry.io/collector/confmap/provider/yamlprovider" +) + +// Test_EscapedEnvVars tests that the resolver supports escaped env vars working together with expand converter. +func Test_EscapedEnvVars(t *testing.T) { + tests := []struct { + name string + scheme string + }{ + { + name: "no_default_scheme", + scheme: "", + }, + { + name: "env", + scheme: "env", + }, + } + + const expandedValue = "some expanded value" + t.Setenv("ENV_VALUE", expandedValue) + + expectedFailures := map[string]string{ + "$ENV_VALUE": "variable substitution using $VAR has been deprecated in favor of ${VAR} and ${env:VAR}", + "$$$ENV_VALUE": "variable substitution using $VAR has been deprecated in favor of ${VAR} and ${env:VAR}", + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + expectedMap := map[string]any{ + "test_map": map[string]any{ + "key1": "$ENV_VALUE", + "key2": "$$ENV_VALUE", + "key3": "$" + expandedValue, + "key4": "some" + expandedValue + "text", + "key5": "some${ENV_VALUE}text", + "key6": "${ONE}${TWO}", + "key7": "text$", + "key8": "$", + "key9": "${1}${env:2}", + "key10": "some${env:ENV_VALUE}text", + "key11": "${env:" + expandedValue + "}", + "key12": "${env:${ENV_VALUE}}", + "key13": "env:MAP_VALUE_2}${ENV_VALUE}{", + "key14": "$" + expandedValue, + }, + } + + resolver, err := confmap.NewResolver(confmap.ResolverSettings{ + URIs: []string{filepath.Join("testdata", "expand-escaped-env.yaml")}, + ProviderFactories: []confmap.ProviderFactory{fileprovider.NewFactory(), envprovider.NewFactory()}, + ConverterFactories: []confmap.ConverterFactory{expandconverter.NewFactory()}, + DefaultScheme: tt.scheme, + }) + 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) + + for val, expectedErr := range expectedFailures { + resolver, err = confmap.NewResolver(confmap.ResolverSettings{ + URIs: []string{fmt.Sprintf("yaml: test: %s", val)}, + ProviderFactories: []confmap.ProviderFactory{yamlprovider.NewFactory(), envprovider.NewFactory()}, + ConverterFactories: []confmap.ConverterFactory{expandconverter.NewFactory()}, + DefaultScheme: tt.scheme, + }) + require.NoError(t, err) + _, err := resolver.Resolve(context.Background()) + require.ErrorContains(t, err, expectedErr) + } + }) + } +} diff --git a/confmap/internal/e2e/go.mod b/confmap/internal/e2e/go.mod index 705f96795f8..4e27927b911 100644 --- a/confmap/internal/e2e/go.mod +++ b/confmap/internal/e2e/go.mod @@ -5,8 +5,10 @@ go 1.21.0 require ( github.com/stretchr/testify v1.9.0 go.opentelemetry.io/collector/confmap v0.105.0 + go.opentelemetry.io/collector/confmap/converter/expandconverter v0.105.0 go.opentelemetry.io/collector/confmap/provider/envprovider v0.105.0 go.opentelemetry.io/collector/confmap/provider/fileprovider v0.105.0 + go.opentelemetry.io/collector/confmap/provider/yamlprovider v0.105.0 go.opentelemetry.io/collector/featuregate v1.12.0 go.opentelemetry.io/collector/internal/globalgates v0.105.0 ) @@ -32,6 +34,10 @@ replace go.opentelemetry.io/collector/confmap/provider/fileprovider => ../../pro replace go.opentelemetry.io/collector/confmap/provider/envprovider => ../../provider/envprovider +replace go.opentelemetry.io/collector/confmap/provider/yamlprovider => ../../provider/yamlprovider + replace go.opentelemetry.io/collector/featuregate => ../../../featuregate replace go.opentelemetry.io/collector/internal/globalgates => ../../../internal/globalgates + +replace go.opentelemetry.io/collector/confmap/converter/expandconverter => ../../converter/expandconverter diff --git a/confmap/internal/e2e/testdata/expand-escaped-env.yaml b/confmap/internal/e2e/testdata/expand-escaped-env.yaml new file mode 100644 index 00000000000..ae8cb280ede --- /dev/null +++ b/confmap/internal/e2e/testdata/expand-escaped-env.yaml @@ -0,0 +1,29 @@ +test_map: + # $$ -> escaped $ + key1: "$$ENV_VALUE" + # $$$$ -> two escaped $ + key2: "$$$$ENV_VALUE" + # $$ -> escaped $ + ${ENV_VALUE} expanded + key3: "$$${ENV_VALUE}" + # expanded in the middle + key4: "some${ENV_VALUE}text" + # escaped $ in the middle + key5: "some$${ENV_VALUE}text" + # two escaped $ + key6: "$${ONE}$${TWO}" + # trailing escaped $ + key7: "text$$" + # escaped $ alone + key8: "$$" + # escaped number and uri + key9: "$${1}$${env:2}" + # escape provider + key10: "some$${env:ENV_VALUE}text" + # can escape outer when nested + key11: "$${env:${ENV_VALUE}}" + # can escape inner and outer when nested + key12: "$${env:$${ENV_VALUE}}" + # can escape partial + key13: "env:MAP_VALUE_2}$${ENV_VALUE}{" + # $$$ -> escaped $ + expanded env var + key14: "$$${env:ENV_VALUE}" diff --git a/confmap/resolver.go b/confmap/resolver.go index ca197076327..7c9ed303c73 100644 --- a/confmap/resolver.go +++ b/confmap/resolver.go @@ -12,8 +12,6 @@ import ( "go.uber.org/multierr" "go.uber.org/zap" - - "go.opentelemetry.io/collector/internal/globalgates" ) // follows drive-letter specification: @@ -173,13 +171,7 @@ func (mr *Resolver) Resolve(ctx context.Context) (*Conf, error) { if err != nil { return nil, err } - - if v, ok := val.(string); ok && globalgates.UseUnifiedEnvVarExpansionRules.IsEnabled() { - cfgMap[k] = strings.ReplaceAll(v, "$$", "$") - } else { - cfgMap[k] = val - } - + cfgMap[k] = val } retMap = NewFromStringMap(cfgMap) diff --git a/confmap/testdata/expand-escaped-env.yaml b/confmap/testdata/expand-escaped-env.yaml deleted file mode 100644 index 6b2cd162831..00000000000 --- a/confmap/testdata/expand-escaped-env.yaml +++ /dev/null @@ -1,31 +0,0 @@ -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}"