From 37f7fbac7a6e818e2c243b1622be77143d4b0c82 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 2 Jul 2024 09:50:21 -0600 Subject: [PATCH 01/16] Set confmap.unifyEnvVarExpansion feature gate to stable --- internal/featuregates/featuregates.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/featuregates/featuregates.go b/internal/featuregates/featuregates.go index e74c75fe481..09b17a83fe3 100644 --- a/internal/featuregates/featuregates.go +++ b/internal/featuregates/featuregates.go @@ -6,6 +6,7 @@ package featuregates // import "go.opentelemetry.io/collector/internal/featurega import "go.opentelemetry.io/collector/featuregate" var UseUnifiedEnvVarExpansionRules = featuregate.GlobalRegistry().MustRegister("confmap.unifyEnvVarExpansion", - featuregate.StageBeta, + featuregate.StageStable, featuregate.WithRegisterFromVersion("v0.103.0"), + featuregate.WithRegisterToVersion("v0.106.0"), featuregate.WithRegisterDescription("`${FOO}` will now be expanded as if it was `${env:FOO}` and no longer expands $ENV syntax. See https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/rfcs/env-vars.md for more details. When this feature gate is stable, expandconverter will be removed.")) From c57a49ff2285082c2d060e59ed416d0b38064ee0 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 2 Jul 2024 10:12:57 -0600 Subject: [PATCH 02/16] changelog --- .chloggen/unify-env-var-gate-stable.yaml | 25 ++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .chloggen/unify-env-var-gate-stable.yaml diff --git a/.chloggen/unify-env-var-gate-stable.yaml b/.chloggen/unify-env-var-gate-stable.yaml new file mode 100644 index 00000000000..1a6a10fb5d6 --- /dev/null +++ b/.chloggen/unify-env-var-gate-stable.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: 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: Set the `confmap.unifyEnvVarExpansion` feature gate to Stable. Expansion of `$FOO` env vars is no longer supporter. Use `${FOO}` or `${env:FOO}` instead. + +# One or more tracking issues or pull requests related to the change +issues: [10508] + +# (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: [] From 62616c6229cb1cae31136743e5517715acb98c72 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 2 Jul 2024 11:54:12 -0600 Subject: [PATCH 03/16] fix some tests --- .../converter/expandconverter/expand_test.go | 367 ------------------ otelcol/command_test.go | 5 - 2 files changed, 372 deletions(-) delete mode 100644 confmap/converter/expandconverter/expand_test.go diff --git a/confmap/converter/expandconverter/expand_test.go b/confmap/converter/expandconverter/expand_test.go deleted file mode 100644 index ab2fa49c549..00000000000 --- a/confmap/converter/expandconverter/expand_test.go +++ /dev/null @@ -1,367 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -package expandconverter - -import ( - "context" - "fmt" - "path/filepath" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "go.uber.org/zap" - "go.uber.org/zap/zapcore" - "go.uber.org/zap/zaptest/observer" - - "go.opentelemetry.io/collector/confmap" - "go.opentelemetry.io/collector/confmap/confmaptest" - "go.opentelemetry.io/collector/confmap/internal/envvar" - "go.opentelemetry.io/collector/featuregate" - "go.opentelemetry.io/collector/internal/featuregates" -) - -func TestNewExpandConverter(t *testing.T) { - var testCases = []struct { - name string // test case name (also file name containing config yaml) - }{ - {name: "expand-with-no-env.yaml"}, - {name: "expand-with-partial-env.yaml"}, - {name: "expand-with-all-env.yaml"}, - } - - const valueExtra = "some string" - const valueExtraMapValue = "some map value" - const valueExtraListMapValue = "some list map value" - const valueExtraListElement = "some list value" - t.Setenv("EXTRA", valueExtra) - t.Setenv("EXTRA_MAP_VALUE_1", valueExtraMapValue+"_1") - t.Setenv("EXTRA_MAP_VALUE_2", valueExtraMapValue+"_2") - t.Setenv("EXTRA_LIST_MAP_VALUE_1", valueExtraListMapValue+"_1") - t.Setenv("EXTRA_LIST_MAP_VALUE_2", valueExtraListMapValue+"_2") - t.Setenv("EXTRA_LIST_VALUE_1", valueExtraListElement+"_1") - t.Setenv("EXTRA_LIST_VALUE_2", valueExtraListElement+"_2") - - expectedCfgMap, errExpected := confmaptest.LoadConf(filepath.Join("testdata", "expand-with-no-env.yaml")) - require.NoError(t, errExpected, "Unable to get expected config") - - for _, test := range testCases { - t.Run(test.name, func(t *testing.T) { - require.NoError(t, featuregate.GlobalRegistry().Set(featuregates.UseUnifiedEnvVarExpansionRules.ID(), false)) - t.Cleanup(func() { - require.NoError(t, featuregate.GlobalRegistry().Set(featuregates.UseUnifiedEnvVarExpansionRules.ID(), true)) - }) - - conf, err := confmaptest.LoadConf(filepath.Join("testdata", test.name)) - require.NoError(t, err, "Unable to get config") - - // Test that expanded configs are the same with the simple config with no env vars. - require.NoError(t, createConverter().Convert(context.Background(), conf)) - assert.Equal(t, expectedCfgMap.ToStringMap(), conf.ToStringMap()) - }) - } -} - -func TestNewExpandConverter_UseUnifiedEnvVarExpansionRules(t *testing.T) { - require.NoError(t, featuregate.GlobalRegistry().Set(featuregates.UseUnifiedEnvVarExpansionRules.ID(), true)) - t.Cleanup(func() { - require.NoError(t, featuregate.GlobalRegistry().Set(featuregates.UseUnifiedEnvVarExpansionRules.ID(), false)) - }) - - const valueExtra = "some string" - const valueExtraMapValue = "some map value" - const valueExtraListMapValue = "some list map value" - const valueExtraListElement = "some list value" - t.Setenv("EXTRA", valueExtra) - t.Setenv("EXTRA_MAP_VALUE_1", valueExtraMapValue+"_1") - t.Setenv("EXTRA_MAP_VALUE_2", valueExtraMapValue+"_2") - t.Setenv("EXTRA_LIST_MAP_VALUE_1", valueExtraListMapValue+"_1") - t.Setenv("EXTRA_LIST_MAP_VALUE_2", valueExtraListMapValue+"_2") - t.Setenv("EXTRA_LIST_VALUE_1", valueExtraListElement+"_1") - t.Setenv("EXTRA_LIST_VALUE_2", valueExtraListElement+"_2") - - conf, err := confmaptest.LoadConf(filepath.Join("testdata", "expand-with-all-env.yaml")) - require.NoError(t, err, "Unable to get config") - - // Test that expanded configs are the same with the simple config with no env vars. - require.ErrorContains(t, createConverter().Convert(context.Background(), conf), "variable substitution using $VAR has been deprecated in favor of ${VAR} and ${env:VAR}") -} - -func TestNewExpandConverter_EscapedMaps(t *testing.T) { - const receiverExtraMapValue = "some map value" - t.Setenv("MAP_VALUE", receiverExtraMapValue) - - conf := confmap.NewFromStringMap( - map[string]any{ - "test_string_map": map[string]any{ - "recv": "$MAP_VALUE", - }, - "test_interface_map": map[any]any{ - "recv": "$MAP_VALUE", - }}, - ) - require.NoError(t, createConverter().Convert(context.Background(), conf)) - - expectedMap := map[string]any{ - "test_string_map": map[string]any{ - "recv": receiverExtraMapValue, - }, - "test_interface_map": map[string]any{ - "recv": receiverExtraMapValue, - }} - assert.Equal(t, expectedMap, conf.ToStringMap()) -} - -func TestNewExpandConverter_EscapedEnvVars(t *testing.T) { - const receiverExtraMapValue = "some map value" - t.Setenv("MAP_VALUE_2", receiverExtraMapValue) - - // Retrieve the config - conf, err := confmaptest.LoadConf(filepath.Join("testdata", "expand-escaped-env.yaml")) - require.NoError(t, err, "Unable to get config") - - expectedMap := map[string]any{ - "test_map": map[string]any{ - // $$ -> escaped $ - "recv.1": "$MAP_VALUE_1", - // $$$ -> escaped $ + substituted env var - "recv.2": "$" + receiverExtraMapValue, - // $$$$ -> two escaped $ - "recv.3": "$$MAP_VALUE_3", - // escaped $ in the middle - "recv.4": "some${MAP_VALUE_4}text", - // $$$$ -> two escaped $ - "recv.5": "${ONE}${TWO}", - // trailing escaped $ - "recv.6": "text$", - // escaped $ alone - "recv.7": "$", - }} - require.NoError(t, createConverter().Convert(context.Background(), conf)) - assert.Equal(t, expectedMap, conf.ToStringMap()) -} - -func TestNewExpandConverterHostPort(t *testing.T) { - t.Setenv("HOST", "127.0.0.1") - t.Setenv("PORT", "4317") - - var testCases = []struct { - name string - input map[string]any - expected map[string]any - }{ - { - name: "brackets", - input: map[string]any{ - "test": "${HOST}:${PORT}", - }, - expected: map[string]any{ - "test": "127.0.0.1:4317", - }, - }, - { - name: "no brackets", - input: map[string]any{ - "test": "$HOST:$PORT", - }, - expected: map[string]any{ - "test": "127.0.0.1:4317", - }, - }, - { - name: "mix", - input: map[string]any{ - "test": "${HOST}:$PORT", - }, - expected: map[string]any{ - "test": "127.0.0.1:4317", - }, - }, - { - name: "reverse mix", - input: map[string]any{ - "test": "$HOST:${PORT}", - }, - expected: map[string]any{ - "test": "127.0.0.1:4317", - }, - }, - } - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { - conf := confmap.NewFromStringMap(tt.input) - require.NoError(t, createConverter().Convert(context.Background(), conf)) - assert.Equal(t, tt.expected, conf.ToStringMap()) - }) - } -} - -func NewTestConverter() (confmap.Converter, *observer.ObservedLogs) { - core, logs := observer.New(zapcore.InfoLevel) - conv := converter{loggedDeprecations: make(map[string]struct{}), logger: zap.New(core)} - return conv, logs -} - -func TestDeprecatedWarning(t *testing.T) { - msgTemplate := `Variable substitution using $VAR will be deprecated in favor of ${VAR} and ${env:VAR}, please update $%s` - t.Setenv("HOST", "127.0.0.1") - t.Setenv("PORT", "4317") - - t.Setenv("HOST_NAME", "127.0.0.2") - t.Setenv("HOSTNAME", "127.0.0.3") - - t.Setenv("BAD!HOST", "127.0.0.2") - - var testCases = []struct { - name string - input map[string]any - expectedOutput map[string]any - expectedWarnings []string - expectedError error - }{ - { - name: "no warning", - input: map[string]any{ - "test": "${HOST}:${PORT}", - }, - expectedOutput: map[string]any{ - "test": "127.0.0.1:4317", - }, - expectedWarnings: []string{}, - expectedError: nil, - }, - { - name: "one deprecated var", - input: map[string]any{ - "test": "${HOST}:$PORT", - }, - expectedOutput: map[string]any{ - "test": "127.0.0.1:4317", - }, - expectedWarnings: []string{"PORT"}, - expectedError: nil, - }, - { - name: "two deprecated vars", - input: map[string]any{ - "test": "$HOST:$PORT", - }, - expectedOutput: map[string]any{ - "test": "127.0.0.1:4317", - }, - expectedWarnings: []string{"HOST", "PORT"}, - expectedError: nil, - }, - { - name: "one depracated serveral times", - input: map[string]any{ - "test": "$HOST,$HOST", - "test2": "$HOST", - }, - expectedOutput: map[string]any{ - "test": "127.0.0.1,127.0.0.1", - "test2": "127.0.0.1", - }, - expectedWarnings: []string{"HOST"}, - expectedError: nil, - }, - { - name: "one warning", - input: map[string]any{ - "test": "$HOST_NAME,${HOSTNAME}", - }, - expectedOutput: map[string]any{ - "test": "127.0.0.2,127.0.0.3", - }, - expectedWarnings: []string{"HOST_NAME"}, - expectedError: nil, - }, - { - name: "malformed environment variable", - input: map[string]any{ - "test": "${BAD!HOST}", - }, - expectedOutput: map[string]any{ - "test": "blah", - }, - expectedWarnings: []string{}, - expectedError: fmt.Errorf("environment variable \"BAD!HOST\" has invalid name: must match regex %s", envvar.ValidationRegexp), - }, - { - name: "malformed environment variable number", - input: map[string]any{ - "test": "${2BADHOST}", - }, - expectedOutput: map[string]any{ - "test": "blah", - }, - expectedWarnings: []string{}, - expectedError: fmt.Errorf("environment variable \"2BADHOST\" has invalid name: must match regex %s", envvar.ValidationRegexp), - }, - { - name: "malformed environment variable unicode", - input: map[string]any{ - "test": "${😊BADHOST}", - }, - expectedOutput: map[string]any{ - "test": "blah", - }, - expectedWarnings: []string{}, - expectedError: fmt.Errorf("environment variable \"😊BADHOST\" has invalid name: must match regex %s", envvar.ValidationRegexp), - }, - } - - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { - conf := confmap.NewFromStringMap(tt.input) - conv, logs := NewTestConverter() - err := conv.Convert(context.Background(), conf) - assert.Equal(t, tt.expectedError, err) - if tt.expectedError == nil { - assert.Equal(t, tt.expectedOutput, conf.ToStringMap()) - } - assert.Equal(t, len(tt.expectedWarnings), len(logs.All())) - for i, variable := range tt.expectedWarnings { - errorMsg := fmt.Sprintf(msgTemplate, variable) - assert.Equal(t, errorMsg, logs.All()[i].Message) - } - }) - } -} - -func TestNewExpandConverterWithErrors(t *testing.T) { - var testCases = []struct { - name string // test case name (also file name containing config yaml) - expectedError error - }{ - { - name: "expand-list-error.yaml", - expectedError: fmt.Errorf("environment variable \"EXTRA_LIST_^VALUE_2\" has invalid name: must match regex %s", envvar.ValidationRegexp), - }, - { - name: "expand-list-map-error.yaml", - expectedError: fmt.Errorf("environment variable \"EXTRA_LIST_MAP_V#ALUE_2\" has invalid name: must match regex %s", envvar.ValidationRegexp), - }, - { - name: "expand-map-error.yaml", - expectedError: fmt.Errorf("environment variable \"EX#TRA\" has invalid name: must match regex %s", envvar.ValidationRegexp), - }, - } - - for _, test := range testCases { - t.Run(test.name, func(t *testing.T) { - conf, err := confmaptest.LoadConf(filepath.Join("testdata", "errors", test.name)) - require.NoError(t, err, "Unable to get config") - - // Test that expanded configs are the same with the simple config with no env vars. - err = createConverter().Convert(context.Background(), conf) - - assert.Equal(t, test.expectedError, err) - }) - } -} - -func createConverter() confmap.Converter { - return NewFactory().Create(confmap.ConverterSettings{Logger: zap.NewNop()}) -} diff --git a/otelcol/command_test.go b/otelcol/command_test.go index 4b4860103c4..1a5680fe4ab 100644 --- a/otelcol/command_test.go +++ b/otelcol/command_test.go @@ -15,7 +15,6 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/featuregate" - "go.opentelemetry.io/collector/internal/featuregates" ) func TestNewCommandVersion(t *testing.T) { @@ -139,10 +138,6 @@ func Test_UseUnifiedEnvVarExpansionRules(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - require.NoError(t, featuregate.GlobalRegistry().Set(featuregates.UseUnifiedEnvVarExpansionRules.ID(), true)) - t.Cleanup(func() { - require.NoError(t, featuregate.GlobalRegistry().Set(featuregates.UseUnifiedEnvVarExpansionRules.ID(), false)) - }) fileProvider := newFakeProvider("file", func(_ context.Context, _ string, _ confmap.WatcherFunc) (*confmap.Retrieved, error) { return &confmap.Retrieved{}, nil }) From 04d0ce9aab4bfcadb43b8c57f2bba8223f20779a Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 2 Jul 2024 12:04:01 -0600 Subject: [PATCH 04/16] fix another test --- otelcol/command_validate_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/otelcol/command_validate_test.go b/otelcol/command_validate_test.go index 03f93f51d16..9c0240910bc 100644 --- a/otelcol/command_validate_test.go +++ b/otelcol/command_validate_test.go @@ -30,6 +30,7 @@ func TestValidateSubCommandInvalidComponents(t *testing.T) { ResolverSettings: confmap.ResolverSettings{ URIs: []string{filePath}, ProviderFactories: []confmap.ProviderFactory{fileProvider}, + DefaultScheme: "file", }, }}, flags(featuregate.GlobalRegistry())) err := cmd.Execute() From 7ca2e487ab3b9630a442697f2f6530cfce3df5c7 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 2 Jul 2024 12:56:33 -0600 Subject: [PATCH 05/16] make gotidy --- confmap/converter/expandconverter/go.mod | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/confmap/converter/expandconverter/go.mod b/confmap/converter/expandconverter/go.mod index f0d150834bb..783e6dd3ac9 100644 --- a/confmap/converter/expandconverter/go.mod +++ b/confmap/converter/expandconverter/go.mod @@ -3,16 +3,13 @@ module go.opentelemetry.io/collector/confmap/converter/expandconverter go 1.21.0 require ( - github.com/stretchr/testify v1.9.0 go.opentelemetry.io/collector v0.104.0 go.opentelemetry.io/collector/confmap v0.104.0 - go.opentelemetry.io/collector/featuregate v1.11.0 go.uber.org/goleak v1.3.0 go.uber.org/zap v1.27.0 ) require ( - github.com/davecgh/go-spew v1.1.1 // indirect github.com/go-viper/mapstructure/v2 v2.0.0-alpha.1 // indirect github.com/hashicorp/go-version v1.7.0 // indirect github.com/knadh/koanf/maps v0.1.1 // indirect @@ -20,7 +17,7 @@ require ( github.com/knadh/koanf/v2 v2.1.1 // indirect github.com/mitchellh/copystructure v1.2.0 // indirect github.com/mitchellh/reflectwalk v1.0.2 // indirect - github.com/pmezard/go-difflib v1.0.0 // indirect + go.opentelemetry.io/collector/featuregate v1.11.0 // indirect go.uber.org/multierr v1.11.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) From 315de42f261ac50d936d95cfef22745e67553691 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Thu, 1 Aug 2024 15:45:48 -0600 Subject: [PATCH 06/16] make gotidy --- confmap/converter/expandconverter/go.mod | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/confmap/converter/expandconverter/go.mod b/confmap/converter/expandconverter/go.mod index 59f8e556944..ab0149b5c12 100644 --- a/confmap/converter/expandconverter/go.mod +++ b/confmap/converter/expandconverter/go.mod @@ -3,16 +3,13 @@ module go.opentelemetry.io/collector/confmap/converter/expandconverter go 1.21.0 require ( - github.com/stretchr/testify v1.9.0 go.opentelemetry.io/collector/confmap v0.104.0 - go.opentelemetry.io/collector/featuregate v1.12.0 go.opentelemetry.io/collector/internal/globalgates v0.106.1 go.uber.org/goleak v1.3.0 go.uber.org/zap v1.27.0 ) require ( - github.com/davecgh/go-spew v1.1.1 // indirect github.com/go-viper/mapstructure/v2 v2.0.0 // indirect github.com/hashicorp/go-version v1.7.0 // indirect github.com/knadh/koanf/maps v0.1.1 // indirect @@ -20,7 +17,7 @@ require ( github.com/knadh/koanf/v2 v2.1.1 // indirect github.com/mitchellh/copystructure v1.2.0 // indirect github.com/mitchellh/reflectwalk v1.0.2 // indirect - github.com/pmezard/go-difflib v1.0.0 // indirect + go.opentelemetry.io/collector/featuregate v1.12.0 // indirect go.uber.org/multierr v1.11.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect ) From 067d3f19df82e5b64c9e931c9484edc5c3221599 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Thu, 1 Aug 2024 15:52:50 -0600 Subject: [PATCH 07/16] make crosslink --- confmap/internal/e2e/go.mod | 4 ---- 1 file changed, 4 deletions(-) diff --git a/confmap/internal/e2e/go.mod b/confmap/internal/e2e/go.mod index 27d7d253de0..1792377658f 100644 --- a/confmap/internal/e2e/go.mod +++ b/confmap/internal/e2e/go.mod @@ -32,10 +32,6 @@ 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 From e2167ad7dddbc0eff57cdf13a3a8a0baec318e41 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 6 Aug 2024 09:16:53 -0600 Subject: [PATCH 08/16] support escaping in nested expansions --- confmap/internal/e2e/expand_test.go | 8 +++++ .../e2e/testdata/expand-escaped-env.yaml | 4 +++ confmap/resolver.go | 31 ++++++++++++++++--- 3 files changed, 38 insertions(+), 5 deletions(-) diff --git a/confmap/internal/e2e/expand_test.go b/confmap/internal/e2e/expand_test.go index 2c5c6cb20f5..61a08977aaa 100644 --- a/confmap/internal/e2e/expand_test.go +++ b/confmap/internal/e2e/expand_test.go @@ -20,6 +20,8 @@ import ( func Test_EscapedEnvVars_NoDefaultScheme(t *testing.T) { const expandedValue = "some expanded value" t.Setenv("ENV_VALUE", expandedValue) + t.Setenv("ENV_LIST", "['$$ESCAPE_ME','$$ESCAPE_ME']") + t.Setenv("ENV_MAP", "{'key': '$$ESCAPE_ME'}") expectedMap := map[string]any{ "test_map": map[string]any{ @@ -38,6 +40,8 @@ func Test_EscapedEnvVars_NoDefaultScheme(t *testing.T) { "key13": "env:MAP_VALUE_2}${ENV_VALUE}{", "key14": "$" + expandedValue, "key15": "$ENV_VALUE", + "key16": []any{"$ESCAPE_ME", "$ESCAPE_ME"}, + "key17": map[string]any{"key": "$ESCAPE_ME"}, }, } @@ -59,6 +63,8 @@ func Test_EscapedEnvVars_NoDefaultScheme(t *testing.T) { func Test_EscapedEnvVars_DefaultScheme(t *testing.T) { const expandedValue = "some expanded value" t.Setenv("ENV_VALUE", expandedValue) + t.Setenv("ENV_LIST", "['$$ESCAPE_ME','$$ESCAPE_ME']") + t.Setenv("ENV_MAP", "{'key': '$$ESCAPE_ME'}") expectedMap := map[string]any{ "test_map": map[string]any{ @@ -77,6 +83,8 @@ func Test_EscapedEnvVars_DefaultScheme(t *testing.T) { "key13": "env:MAP_VALUE_2}${ENV_VALUE}{", "key14": "$" + expandedValue, "key15": "$ENV_VALUE", + "key16": []any{"$ESCAPE_ME", "$ESCAPE_ME"}, + "key17": map[string]any{"key": "$ESCAPE_ME"}, }, } diff --git a/confmap/internal/e2e/testdata/expand-escaped-env.yaml b/confmap/internal/e2e/testdata/expand-escaped-env.yaml index ca3df782cb6..74a1d9fa2af 100644 --- a/confmap/internal/e2e/testdata/expand-escaped-env.yaml +++ b/confmap/internal/e2e/testdata/expand-escaped-env.yaml @@ -29,3 +29,7 @@ test_map: key14: "$$${env:ENV_VALUE}" # $ -> $ key15: "$ENV_VALUE" + # list is escaped + key16: "${env:ENV_LIST}" + # map is escaped + key17: "${env:ENV_MAP}" diff --git a/confmap/resolver.go b/confmap/resolver.go index 3237a258c74..92b22bfbf1d 100644 --- a/confmap/resolver.go +++ b/confmap/resolver.go @@ -181,11 +181,7 @@ func (mr *Resolver) Resolve(ctx context.Context) (*Conf, error) { if err != nil { return nil, err } - if v, ok := val.(string); ok { - cfgMap[k] = strings.ReplaceAll(v, "$$", "$") - } else { - cfgMap[k] = val - } + cfgMap[k] = escapeDollarSign(val) } retMap = NewFromStringMap(cfgMap) @@ -199,6 +195,31 @@ func (mr *Resolver) Resolve(ctx context.Context) (*Conf, error) { return retMap, nil } +func escapeDollarSign(val any) any { + switch v := val.(type) { + case string: + return strings.ReplaceAll(v, "$$", "$") + case expandedValue: + v.Original = strings.ReplaceAll(v.Original, "$$", "$") + v.Value = escapeDollarSign(v.Value) + return v + case []any: + nslice := make([]any, len(v), len(v)) + for i, x := range v { + nslice[i] = escapeDollarSign(x) + } + return nslice + case map[string]any: + nmap := make(map[string]any, len(v)) + for k, x := range v { + nmap[k] = escapeDollarSign(x) + } + return nmap + default: + return val + } +} + // Watch blocks until any configuration change was detected or an unrecoverable error // happened during monitoring the configuration changes. // From eac5873faddfd7f4133da241ef6e0a82089889bd Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 6 Aug 2024 09:21:51 -0600 Subject: [PATCH 09/16] Fix lint --- confmap/resolver.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/confmap/resolver.go b/confmap/resolver.go index 92b22bfbf1d..66b3b9654ba 100644 --- a/confmap/resolver.go +++ b/confmap/resolver.go @@ -204,7 +204,7 @@ func escapeDollarSign(val any) any { v.Value = escapeDollarSign(v.Value) return v case []any: - nslice := make([]any, len(v), len(v)) + nslice := make([]any, len(v)) for i, x := range v { nslice[i] = escapeDollarSign(x) } From d5dcc55fe3e18901f98472ad40c09a8d8d7d16ef Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 6 Aug 2024 11:57:10 -0600 Subject: [PATCH 10/16] Dont escape original value --- confmap/resolver.go | 1 - 1 file changed, 1 deletion(-) diff --git a/confmap/resolver.go b/confmap/resolver.go index 66b3b9654ba..7aa7ad4aa2f 100644 --- a/confmap/resolver.go +++ b/confmap/resolver.go @@ -200,7 +200,6 @@ func escapeDollarSign(val any) any { case string: return strings.ReplaceAll(v, "$$", "$") case expandedValue: - v.Original = strings.ReplaceAll(v.Original, "$$", "$") v.Value = escapeDollarSign(v.Value) return v case []any: From 14eee2e632d4016c7d8bf724fffcd74097ee2704 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 6 Aug 2024 12:39:07 -0600 Subject: [PATCH 11/16] Improve test --- confmap/internal/e2e/expand_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/confmap/internal/e2e/expand_test.go b/confmap/internal/e2e/expand_test.go index 61a08977aaa..1ecbd906d24 100644 --- a/confmap/internal/e2e/expand_test.go +++ b/confmap/internal/e2e/expand_test.go @@ -20,8 +20,8 @@ import ( func Test_EscapedEnvVars_NoDefaultScheme(t *testing.T) { const expandedValue = "some expanded value" t.Setenv("ENV_VALUE", expandedValue) - t.Setenv("ENV_LIST", "['$$ESCAPE_ME','$$ESCAPE_ME']") - t.Setenv("ENV_MAP", "{'key': '$$ESCAPE_ME'}") + t.Setenv("ENV_LIST", "['$$ESCAPE_ME','$${ESCAPE_ME}','$${env:ESCAPE_ME}']") + t.Setenv("ENV_MAP", "{'key1':'$$ESCAPE_ME','key2':'$${ESCAPE_ME}','key3':'$${env:ESCAPE_ME}'}") expectedMap := map[string]any{ "test_map": map[string]any{ @@ -40,8 +40,8 @@ func Test_EscapedEnvVars_NoDefaultScheme(t *testing.T) { "key13": "env:MAP_VALUE_2}${ENV_VALUE}{", "key14": "$" + expandedValue, "key15": "$ENV_VALUE", - "key16": []any{"$ESCAPE_ME", "$ESCAPE_ME"}, - "key17": map[string]any{"key": "$ESCAPE_ME"}, + "key16": []any{"$ESCAPE_ME", "${ESCAPE_ME}", "${env:ESCAPE_ME}"}, + "key17": map[string]any{"key1": "$ESCAPE_ME", "key2": "${ESCAPE_ME}", "key3": "${env:ESCAPE_ME}"}, }, } @@ -63,8 +63,8 @@ func Test_EscapedEnvVars_NoDefaultScheme(t *testing.T) { func Test_EscapedEnvVars_DefaultScheme(t *testing.T) { const expandedValue = "some expanded value" t.Setenv("ENV_VALUE", expandedValue) - t.Setenv("ENV_LIST", "['$$ESCAPE_ME','$$ESCAPE_ME']") - t.Setenv("ENV_MAP", "{'key': '$$ESCAPE_ME'}") + t.Setenv("ENV_LIST", "['$$ESCAPE_ME','$${ESCAPE_ME}','$${env:ESCAPE_ME}']") + t.Setenv("ENV_MAP", "{'key1':'$$ESCAPE_ME','key2':'$${ESCAPE_ME}','key3':'$${env:ESCAPE_ME}'}") expectedMap := map[string]any{ "test_map": map[string]any{ @@ -83,8 +83,8 @@ func Test_EscapedEnvVars_DefaultScheme(t *testing.T) { "key13": "env:MAP_VALUE_2}${ENV_VALUE}{", "key14": "$" + expandedValue, "key15": "$ENV_VALUE", - "key16": []any{"$ESCAPE_ME", "$ESCAPE_ME"}, - "key17": map[string]any{"key": "$ESCAPE_ME"}, + "key16": []any{"$ESCAPE_ME", "${ESCAPE_ME}", "${env:ESCAPE_ME}"}, + "key17": map[string]any{"key1": "$ESCAPE_ME", "key2": "${ESCAPE_ME}", "key3": "${env:ESCAPE_ME}"}, }, } From 6dbef43044fbcf2614427f1dcd3a080b177cbe56 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 6 Aug 2024 13:28:55 -0600 Subject: [PATCH 12/16] Escape expandValue.Original --- confmap/internal/e2e/types_test.go | 63 ++++++++++++++++++++++++++++++ confmap/resolver.go | 11 +++--- 2 files changed, 69 insertions(+), 5 deletions(-) diff --git a/confmap/internal/e2e/types_test.go b/confmap/internal/e2e/types_test.go index be099ee9bb2..1412f169a44 100644 --- a/confmap/internal/e2e/types_test.go +++ b/confmap/internal/e2e/types_test.go @@ -47,6 +47,7 @@ func NewResolver(t testing.TB, path string) *confmap.Resolver { fileprovider.NewFactory(), envprovider.NewFactory(), }, + DefaultScheme: "env", }) require.NoError(t, err) return resolver @@ -244,6 +245,8 @@ func TestTypeCasting(t *testing.T) { } func TestStrictTypeCasting(t *testing.T) { + t.Setenv("ENV_VALUE", "testreceiver") + values := []Test{ { value: "123", @@ -393,6 +396,66 @@ func TestStrictTypeCasting(t *testing.T) { targetField: TargetFieldInlineString, expected: "inline field with [filelog,windowseventlog/application] expansion", }, + { + value: "$$ENV", + targetField: TargetFieldString, + expected: "$ENV", + }, + { + value: "$$ENV", + targetField: TargetFieldInlineString, + expected: "inline field with $ENV expansion", + }, + { + value: "$${ENV}", + targetField: TargetFieldString, + expected: "${ENV}", + }, + { + value: "$${ENV}", + targetField: TargetFieldInlineString, + expected: "inline field with ${ENV} expansion", + }, + { + value: "$${env:ENV}", + targetField: TargetFieldString, + expected: "${env:ENV}", + }, + { + value: "$${env:ENV}", + targetField: TargetFieldInlineString, + expected: "inline field with ${env:ENV} expansion", + }, + { + value: `[filelog,${env:ENV_VALUE}]`, + targetField: TargetFieldString, + expected: "[filelog,testreceiver]", + }, + { + value: `[filelog,${ENV_VALUE}]`, + targetField: TargetFieldString, + expected: "[filelog,testreceiver]", + }, + { + value: `["filelog","$${env:ENV_VALUE}"]`, + targetField: TargetFieldString, + expected: `["filelog","${env:ENV_VALUE}"]`, + }, + { + value: `["filelog","$${ENV_VALUE}"]`, + targetField: TargetFieldString, + expected: `["filelog","${ENV_VALUE}"]`, + }, + { + value: `["filelog","$$ENV_VALUE"]`, + targetField: TargetFieldString, + expected: `["filelog","$ENV_VALUE"]`, + }, + { + value: `["filelog","$ENV_VALUE"]`, + targetField: TargetFieldString, + expected: `["filelog","$ENV_VALUE"]`, + }, } previousValue := globalgates.StrictlyTypedInputGate.IsEnabled() diff --git a/confmap/resolver.go b/confmap/resolver.go index 7aa7ad4aa2f..e635ea99564 100644 --- a/confmap/resolver.go +++ b/confmap/resolver.go @@ -181,7 +181,7 @@ func (mr *Resolver) Resolve(ctx context.Context) (*Conf, error) { if err != nil { return nil, err } - cfgMap[k] = escapeDollarSign(val) + cfgMap[k] = escapeDollarSigns(val) } retMap = NewFromStringMap(cfgMap) @@ -195,23 +195,24 @@ func (mr *Resolver) Resolve(ctx context.Context) (*Conf, error) { return retMap, nil } -func escapeDollarSign(val any) any { +func escapeDollarSigns(val any) any { switch v := val.(type) { case string: return strings.ReplaceAll(v, "$$", "$") case expandedValue: - v.Value = escapeDollarSign(v.Value) + v.Original = strings.ReplaceAll(v.Original, "$$", "$") + v.Value = escapeDollarSigns(v.Value) return v case []any: nslice := make([]any, len(v)) for i, x := range v { - nslice[i] = escapeDollarSign(x) + nslice[i] = escapeDollarSigns(x) } return nslice case map[string]any: nmap := make(map[string]any, len(v)) for k, x := range v { - nmap[k] = escapeDollarSign(x) + nmap[k] = escapeDollarSigns(x) } return nmap default: From cce672dd191fa09a9fecd01637f9e5a779009d77 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 6 Aug 2024 13:42:10 -0600 Subject: [PATCH 13/16] Fix comment test --- confmap/internal/e2e/types_test.go | 55 ++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/confmap/internal/e2e/types_test.go b/confmap/internal/e2e/types_test.go index 1412f169a44..6e1304b54f7 100644 --- a/confmap/internal/e2e/types_test.go +++ b/confmap/internal/e2e/types_test.go @@ -658,7 +658,7 @@ func TestStructMappingIssue10787(t *testing.T) { }() resolver := NewResolver(t, "types_expand.yaml") - t.Setenv("ENV", `# ${hello.world} + t.Setenv("ENV", `# this is a comment logging: verbosity: detailed`) conf, err := resolver.Resolve(context.Background()) @@ -691,7 +691,58 @@ logging: var cfgStr TargetConfig[string] err = confStr.Unmarshal(&cfgStr) require.NoError(t, err) - require.Equal(t, `# ${hello.world} + require.Equal(t, `# this is a comment +logging: + verbosity: detailed`, + cfgStr.Field, + ) +} + +func TestStructMappingIssue10787_ExpandComment(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("EXPAND_ME", "an expanded env var") + t.Setenv("ENV", `# this is a comment with ${EXPAND_ME} +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, `# this is a comment with an expanded env var logging: verbosity: detailed`, cfgStr.Field, From fd8ca4b2b92fc5ea002dbf32f0076e584ac554b2 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 6 Aug 2024 13:47:08 -0600 Subject: [PATCH 14/16] Apply suggestions from code review Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> --- .chloggen/unify-env-var-gate-stable.yaml | 2 +- confmap/converter/expandconverter/go.mod | 2 +- confmap/internal/e2e/expand_test.go | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/.chloggen/unify-env-var-gate-stable.yaml b/.chloggen/unify-env-var-gate-stable.yaml index 1a6a10fb5d6..e75a001f686 100644 --- a/.chloggen/unify-env-var-gate-stable.yaml +++ b/.chloggen/unify-env-var-gate-stable.yaml @@ -7,7 +7,7 @@ change_type: breaking component: confmap # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Set the `confmap.unifyEnvVarExpansion` feature gate to Stable. Expansion of `$FOO` env vars is no longer supporter. Use `${FOO}` or `${env:FOO}` instead. +note: Set the `confmap.unifyEnvVarExpansion` feature gate to Stable. Expansion of `$FOO` env vars is no longer supported. Use `${FOO}` or `${env:FOO}` instead. # One or more tracking issues or pull requests related to the change issues: [10508] diff --git a/confmap/converter/expandconverter/go.mod b/confmap/converter/expandconverter/go.mod index ab0149b5c12..a0b3216d4b4 100644 --- a/confmap/converter/expandconverter/go.mod +++ b/confmap/converter/expandconverter/go.mod @@ -3,7 +3,7 @@ module go.opentelemetry.io/collector/confmap/converter/expandconverter go 1.21.0 require ( - go.opentelemetry.io/collector/confmap v0.104.0 + go.opentelemetry.io/collector/confmap v0.106.1 go.opentelemetry.io/collector/internal/globalgates v0.106.1 go.uber.org/goleak v1.3.0 go.uber.org/zap v1.27.0 diff --git a/confmap/internal/e2e/expand_test.go b/confmap/internal/e2e/expand_test.go index 1ecbd906d24..3efc9d7d388 100644 --- a/confmap/internal/e2e/expand_test.go +++ b/confmap/internal/e2e/expand_test.go @@ -59,7 +59,6 @@ func Test_EscapedEnvVars_NoDefaultScheme(t *testing.T) { assert.Equal(t, expectedMap, m) } -// Test_EscapedEnvVars tests that the resolver supports escaped env vars working together with expand converter. func Test_EscapedEnvVars_DefaultScheme(t *testing.T) { const expandedValue = "some expanded value" t.Setenv("ENV_VALUE", expandedValue) From 54cddc31e9ef613e94746fe05ee1a6388170a32d Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 6 Aug 2024 13:49:55 -0600 Subject: [PATCH 15/16] Update confmap/internal/e2e/expand_test.go Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> --- confmap/internal/e2e/expand_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/confmap/internal/e2e/expand_test.go b/confmap/internal/e2e/expand_test.go index 3efc9d7d388..b6a41fafc26 100644 --- a/confmap/internal/e2e/expand_test.go +++ b/confmap/internal/e2e/expand_test.go @@ -16,7 +16,6 @@ import ( "go.opentelemetry.io/collector/confmap/provider/fileprovider" ) -// Test_EscapedEnvVars tests that the resolver supports escaped env vars working together with expand converter. func Test_EscapedEnvVars_NoDefaultScheme(t *testing.T) { const expandedValue = "some expanded value" t.Setenv("ENV_VALUE", expandedValue) From bebb922486d394db0a4f173af429ec4d9632846b Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Wed, 7 Aug 2024 10:23:23 -0600 Subject: [PATCH 16/16] Update internal/globalgates/globalgates.go --- internal/globalgates/globalgates.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/globalgates/globalgates.go b/internal/globalgates/globalgates.go index afb775b42c2..346bd2d64fd 100644 --- a/internal/globalgates/globalgates.go +++ b/internal/globalgates/globalgates.go @@ -8,7 +8,7 @@ import "go.opentelemetry.io/collector/featuregate" var UseUnifiedEnvVarExpansionRules = featuregate.GlobalRegistry().MustRegister("confmap.unifyEnvVarExpansion", featuregate.StageStable, featuregate.WithRegisterFromVersion("v0.103.0"), - featuregate.WithRegisterToVersion("v0.106.0"), + featuregate.WithRegisterToVersion("v0.109.0"), featuregate.WithRegisterDescription("`${FOO}` will now be expanded as if it was `${env:FOO}` and no longer expands $ENV syntax. See https://github.com/open-telemetry/opentelemetry-collector/blob/main/docs/rfcs/env-vars.md for more details. When this feature gate is stable, expandconverter will be removed.")) const StrictlyTypedInputID = "confmap.strictlyTypedInput"