From 897d3a58b59058b4807d3f8f0b734e683d126b91 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Mon, 20 May 2024 15:48:24 -0600 Subject: [PATCH 01/17] Add ability to set default provider --- confmap/expand.go | 19 +++++++++++++++---- confmap/expand_test.go | 16 ++++++++++++++++ confmap/resolver.go | 32 ++++++++++++++++++++++---------- confmap/resolver_test.go | 11 +++++++++++ 4 files changed, 64 insertions(+), 14 deletions(-) diff --git a/confmap/expand.go b/confmap/expand.go index a913e768942..001104c4995 100644 --- a/confmap/expand.go +++ b/confmap/expand.go @@ -98,10 +98,17 @@ func findURI(input string) 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 } func (mr *Resolver) findAndExpandURI(ctx context.Context, input string) (any, bool, error) { uri := findURI(input) if uri == "" { - // No URI found, return. + if mr.defaultProvider != nil { + ret, err := mr.defaultProvider.Retrieve(ctx, input[2:len(input)-1], mr.onChange) + if err != nil { + return nil, false, err + } + return mr.handleRetrieved(ret) + } return input, false, nil } if uri == input { @@ -138,6 +145,12 @@ func toString(strURI string, input any) (string, error) { } } +func (mr *Resolver) handleRetrieved(ret *Retrieved) (any, bool, error) { + mr.closers = append(mr.closers, ret.Close) + val, err := ret.AsRaw() + return val, true, err +} + func (mr *Resolver) expandURI(ctx context.Context, uri string) (any, bool, error) { lURI, err := newLocation(uri[2 : len(uri)-1]) if err != nil { @@ -150,9 +163,7 @@ func (mr *Resolver) expandURI(ctx context.Context, uri string) (any, bool, error if err != nil { return nil, false, err } - mr.closers = append(mr.closers, ret.Close) - val, err := ret.AsRaw() - return val, true, err + return mr.handleRetrieved(ret) } type location struct { diff --git a/confmap/expand_test.go b/confmap/expand_test.go index c1dc6646152..b98b92668a3 100644 --- a/confmap/expand_test.go +++ b/confmap/expand_test.go @@ -307,6 +307,9 @@ func TestResolverExpandStringValues(t *testing.T) { func newEnvProvider() ProviderFactory { return newFakeProvider("env", func(_ context.Context, uri string, _ WatcherFunc) (*Retrieved, error) { + if uri[0:4] != "env:" { + uri = "env:" + uri + } switch uri { case "env:COMPLEX_VALUE": return NewRetrieved([]any{"localhost:3042"}) @@ -459,3 +462,16 @@ func TestResolverExpandStringValueInvalidReturnValue(t *testing.T) { _, err = resolver.Resolve(context.Background()) assert.EqualError(t, err, `expanding ${test:PORT}, expected convertable to string value type, got ['ӛ']([]interface {})`) } + +func TestResolverDefaultProviderExpand(t *testing.T) { + provider := newFakeProvider("input", func(context.Context, string, WatcherFunc) (*Retrieved, error) { + return NewRetrieved(map[string]any{"foo": "${HOST}"}) + }) + + resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider}, DefaultProvider: newEnvProvider(), ConverterFactories: nil}) + require.NoError(t, err) + + cfgMap, err := resolver.Resolve(context.Background()) + require.NoError(t, err) + assert.Equal(t, map[string]any{"foo": "localhost"}, cfgMap.ToStringMap()) +} diff --git a/confmap/resolver.go b/confmap/resolver.go index 304dc3f71c3..3b7aad71635 100644 --- a/confmap/resolver.go +++ b/confmap/resolver.go @@ -20,9 +20,10 @@ var driverLetterRegexp = regexp.MustCompile("^[A-z]:") // Resolver resolves a configuration as a Conf. type Resolver struct { - uris []location - providers map[string]Provider - converters []Converter + uris []location + providers map[string]Provider + defaultProvider Provider + converters []Converter closers []CloseFunc watcher chan error @@ -34,11 +35,16 @@ type ResolverSettings struct { // It is required to have at least one location. URIs []string - // ProviderFactories is a list of Provider creation functions. - // It is required to have at least one ProviderFactory - // if a Provider is not given. + // ProviderFactories is a slice of Provider creation functions. + // It is required to have at least one provider. ProviderFactories []ProviderFactory + // DefaultProvider is the provider that is used if ${} syntax is used but no schema is provided. + // If no DefaultProvider is set, ${} with no schema will not be expanded. + // It is strongly recommended to set an EnvProvider as the default to align with the + // OpenTelemetry Configuration Specification + DefaultProvider ProviderFactory + // ProviderSettings contains settings that will be passed to Provider // factories when instantiating Providers. ProviderSettings ProviderSettings @@ -93,6 +99,11 @@ func NewResolver(set ResolverSettings) (*Resolver, error) { providers[provider.Scheme()] = provider } + var defaultProvider Provider + if set.DefaultProvider != nil { + defaultProvider = set.DefaultProvider.Create(set.ProviderSettings) + } + converters := make([]Converter, len(set.ConverterFactories)) for i, factory := range set.ConverterFactories { converters[i] = factory.Create(set.ConverterSettings) @@ -119,10 +130,11 @@ func NewResolver(set ResolverSettings) (*Resolver, error) { } return &Resolver{ - uris: uris, - providers: providers, - converters: converters, - watcher: make(chan error, 1), + uris: uris, + providers: providers, + defaultProvider: defaultProvider, + converters: converters, + watcher: make(chan error, 1), }, nil } diff --git a/confmap/resolver_test.go b/confmap/resolver_test.go index 3a162c85247..43334de6d43 100644 --- a/confmap/resolver_test.go +++ b/confmap/resolver_test.go @@ -395,3 +395,14 @@ func TestProvidesDefaultLogger(t *testing.T) { require.NoError(t, err) require.NotNil(t, provider.logger) } + +func TestResolverDefaultProviderSet(t *testing.T) { + provider := newEnvProvider() + r, err := NewResolver(ResolverSettings{ + URIs: []string{"env:"}, + ProviderFactories: []ProviderFactory{provider}, + DefaultProvider: provider, + }) + require.NoError(t, err) + require.NotNil(t, r.defaultProvider) +} From 015c62e568ac884cd5fd6e0d341d0546febfb17f Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Mon, 20 May 2024 15:57:06 -0600 Subject: [PATCH 02/17] changelog --- .chloggen/confmap-default-provider.yaml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .chloggen/confmap-default-provider.yaml diff --git a/.chloggen/confmap-default-provider.yaml b/.chloggen/confmap-default-provider.yaml new file mode 100644 index 00000000000..97c67d2f7a6 --- /dev/null +++ b/.chloggen/confmap-default-provider.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: enhancement + +# 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: Allow setting a default Provider on a Resolver to use when `${}` syntax is used without a schema + +# One or more tracking issues or pull requests related to the change +issues: [10182] + +# (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 ab08025829ccda18aa9883b0488e892d8989031f Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 21 May 2024 12:07:36 -0600 Subject: [PATCH 03/17] experiment with findURI --- confmap/expand.go | 63 +++++++++++++++++++++--------------------- confmap/expand_test.go | 16 +++++++---- 2 files changed, 43 insertions(+), 36 deletions(-) diff --git a/confmap/expand.go b/confmap/expand.go index 001104c4995..862b7d1473d 100644 --- a/confmap/expand.go +++ b/confmap/expand.go @@ -76,47 +76,45 @@ func (mr *Resolver) expandValue(ctx context.Context, value any) (any, bool, erro return value, false, nil } +//test: "${http://example.com/?os=${env:OS}}" + // findURI attempts to find the first expandable URI in input. It returns an expandable // URI, or an empty string if none are found. // Note: findURI is only called when input contains a closing bracket. -func findURI(input string) string { +func (mr *Resolver) findURI(input string) (string, bool) { closeIndex := strings.Index(input, "}") remaining := input[closeIndex+1:] openIndex := strings.LastIndex(input[:closeIndex+1], "${") // if there is a missing "${" or the uri does not contain ":", check the next URI. - if openIndex < 0 || !strings.Contains(input[openIndex:closeIndex+1], ":") { + if openIndex < 0 { // if remaining does not contain "}", there are no URIs left: stop recursion. if !strings.Contains(remaining, "}") { - return "" + + return "", false } - return findURI(remaining) + return mr.findURI(remaining) } - return input[openIndex : closeIndex+1] + potentialURI := input[openIndex : closeIndex+1] + return potentialURI, strings.Contains(potentialURI, ":") } // 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 } func (mr *Resolver) findAndExpandURI(ctx context.Context, input string) (any, bool, error) { - uri := findURI(input) - if uri == "" { - if mr.defaultProvider != nil { - ret, err := mr.defaultProvider.Retrieve(ctx, input[2:len(input)-1], mr.onChange) - if err != nil { - return nil, false, err - } - return mr.handleRetrieved(ret) - } + uri, schemeFound := mr.findURI(input) + if uri == "" || (!schemeFound && mr.defaultProvider == nil) { + // No URI found, return. return input, false, nil } if uri == input { // If the value is a single URI, then the return value can be anything. // This is the case `foo: ${file:some_extra_config.yml}`. - return mr.expandURI(ctx, input) + return mr.expandURI(ctx, input, schemeFound) } - expanded, changed, err := mr.expandURI(ctx, uri) + expanded, changed, err := mr.expandURI(ctx, uri, schemeFound) if err != nil { return input, false, err } @@ -145,25 +143,28 @@ func toString(strURI string, input any) (string, error) { } } -func (mr *Resolver) handleRetrieved(ret *Retrieved) (any, bool, error) { - mr.closers = append(mr.closers, ret.Close) - val, err := ret.AsRaw() - return val, true, err -} - -func (mr *Resolver) expandURI(ctx context.Context, uri string) (any, bool, error) { - lURI, err := newLocation(uri[2 : len(uri)-1]) - if err != nil { - return nil, false, err - } - if strings.Contains(lURI.opaqueValue, "$") { - return nil, false, fmt.Errorf("the uri %q contains unsupported characters ('$')", lURI.asString()) +func (mr *Resolver) expandURI(ctx context.Context, uri string, hasScheme bool) (any, bool, error) { + var ret *Retrieved + var err error + uri = uri[2 : len(uri)-1] + if !hasScheme && mr.defaultProvider != nil { + ret, err = mr.defaultProvider.Retrieve(ctx, uri, mr.onChange) + } else { + lURI, locErr := newLocation(uri) + if locErr != nil { + return nil, false, locErr + } + if strings.Contains(lURI.opaqueValue, "$") { + return nil, false, fmt.Errorf("the uri %q contains unsupported characters ('$')", lURI.asString()) + } + ret, err = mr.retrieveValue(ctx, lURI) } - ret, err := mr.retrieveValue(ctx, lURI) if err != nil { return nil, false, err } - return mr.handleRetrieved(ret) + mr.closers = append(mr.closers, ret.Close) + val, err := ret.AsRaw() + return val, true, err } type location struct { diff --git a/confmap/expand_test.go b/confmap/expand_test.go index b98b92668a3..79ee68593dd 100644 --- a/confmap/expand_test.go +++ b/confmap/expand_test.go @@ -104,15 +104,20 @@ func TestResolverExpandStringValues(t *testing.T) { output any }{ // Embedded. + //{ + // name: "Test", + // input: "${env:HOST${HOST}${HOST}}", + // output: "idk", + //}, { name: "NoMatchOldStyle", input: "${HOST}:${PORT}", - output: "${HOST}:${PORT}", + output: "localhost:3044", }, { name: "NoMatchOldStyleNoBrackets", input: "${HOST}:$PORT", - output: "${HOST}:$PORT", + output: "localhost:$PORT", }, { name: "ComplexValue", @@ -137,7 +142,7 @@ func TestResolverExpandStringValues(t *testing.T) { { name: "EmbeddedNewAndOldStyle", input: "${env:HOST}:${PORT}", - output: "localhost:${PORT}", + output: "localhost:3044", }, { name: "Int", @@ -230,7 +235,7 @@ func TestResolverExpandStringValues(t *testing.T) { { name: "NoMatchOldStyleNested", input: "${test:localhost:${PORT}}", - output: "${test:localhost:${PORT}}", + output: "localhost:3044", }, // Partial expand. { @@ -295,7 +300,8 @@ func TestResolverExpandStringValues(t *testing.T) { return NewRetrieved(uri[5:]) }) - resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, newEnvProvider(), testProvider}, ConverterFactories: nil}) + envProvider := newEnvProvider() + resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, envProvider, testProvider}, DefaultProvider: envProvider, ConverterFactories: nil}) require.NoError(t, err) cfgMap, err := resolver.Resolve(context.Background()) From fba7f4b6aa0186f819af401d78f8bbe53ee03161 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Wed, 22 May 2024 12:14:46 -0600 Subject: [PATCH 04/17] Experiment with adding scheme to default uri --- confmap/expand.go | 45 ++++++++++++++++++++------------------------- confmap/resolver.go | 1 + 2 files changed, 21 insertions(+), 25 deletions(-) diff --git a/confmap/expand.go b/confmap/expand.go index 862b7d1473d..70e999a888e 100644 --- a/confmap/expand.go +++ b/confmap/expand.go @@ -81,40 +81,42 @@ func (mr *Resolver) expandValue(ctx context.Context, value any) (any, bool, erro // findURI attempts to find the first expandable URI in input. It returns an expandable // URI, or an empty string if none are found. // Note: findURI is only called when input contains a closing bracket. -func (mr *Resolver) findURI(input string) (string, bool) { +func (mr *Resolver) findURI(input string) (string, string) { closeIndex := strings.Index(input, "}") remaining := input[closeIndex+1:] openIndex := strings.LastIndex(input[:closeIndex+1], "${") // if there is a missing "${" or the uri does not contain ":", check the next URI. - if openIndex < 0 { + if openIndex < 0 || (!strings.Contains(input[openIndex:closeIndex+1], ":") && mr.defaultProvider == nil) { // if remaining does not contain "}", there are no URIs left: stop recursion. if !strings.Contains(remaining, "}") { - - return "", false + return "", "" } return mr.findURI(remaining) } potentialURI := input[openIndex : closeIndex+1] - return potentialURI, strings.Contains(potentialURI, ":") + if !strings.Contains(potentialURI, ":") && mr.defaultProvider != nil { + return potentialURI, strings.Replace(potentialURI, "${", fmt.Sprintf("${%v:", mr.defaultProvider.Scheme()), 1) + } + return potentialURI, potentialURI } // 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 } func (mr *Resolver) findAndExpandURI(ctx context.Context, input string) (any, bool, error) { - uri, schemeFound := mr.findURI(input) - if uri == "" || (!schemeFound && mr.defaultProvider == nil) { + unmodifiedURI, uri := mr.findURI(input) + if uri == "" { // No URI found, return. return input, false, nil } if uri == input { // If the value is a single URI, then the return value can be anything. // This is the case `foo: ${file:some_extra_config.yml}`. - return mr.expandURI(ctx, input, schemeFound) + return mr.expandURI(ctx, input) } - expanded, changed, err := mr.expandURI(ctx, uri, schemeFound) + expanded, changed, err := mr.expandURI(ctx, uri) if err != nil { return input, false, err } @@ -122,7 +124,7 @@ func (mr *Resolver) findAndExpandURI(ctx context.Context, input string) (any, bo if err != nil { return input, false, err } - return strings.ReplaceAll(input, uri, repl), changed, err + return strings.ReplaceAll(input, unmodifiedURI, repl), changed, err } // toString attempts to convert input to a string. @@ -143,22 +145,15 @@ func toString(strURI string, input any) (string, error) { } } -func (mr *Resolver) expandURI(ctx context.Context, uri string, hasScheme bool) (any, bool, error) { - var ret *Retrieved - var err error - uri = uri[2 : len(uri)-1] - if !hasScheme && mr.defaultProvider != nil { - ret, err = mr.defaultProvider.Retrieve(ctx, uri, mr.onChange) - } else { - lURI, locErr := newLocation(uri) - if locErr != nil { - return nil, false, locErr - } - if strings.Contains(lURI.opaqueValue, "$") { - return nil, false, fmt.Errorf("the uri %q contains unsupported characters ('$')", lURI.asString()) - } - ret, err = mr.retrieveValue(ctx, lURI) +func (mr *Resolver) expandURI(ctx context.Context, uri string) (any, bool, error) { + lURI, err := newLocation(uri[2 : len(uri)-1]) + if err != nil { + return nil, false, err + } + if strings.Contains(lURI.opaqueValue, "$") { + return nil, false, fmt.Errorf("the uri %q contains unsupported characters ('$')", lURI.asString()) } + ret, err := mr.retrieveValue(ctx, lURI) if err != nil { return nil, false, err } diff --git a/confmap/resolver.go b/confmap/resolver.go index 3b7aad71635..7b91a40995e 100644 --- a/confmap/resolver.go +++ b/confmap/resolver.go @@ -102,6 +102,7 @@ func NewResolver(set ResolverSettings) (*Resolver, error) { var defaultProvider Provider if set.DefaultProvider != nil { defaultProvider = set.DefaultProvider.Create(set.ProviderSettings) + providers[defaultProvider.Scheme()] = defaultProvider } converters := make([]Converter, len(set.ConverterFactories)) From 7bf59a9222981d864393637bc4d3b8a41ab2575c Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Wed, 22 May 2024 12:15:23 -0600 Subject: [PATCH 05/17] remove test comments --- confmap/expand.go | 2 -- confmap/expand_test.go | 6 ------ 2 files changed, 8 deletions(-) diff --git a/confmap/expand.go b/confmap/expand.go index 70e999a888e..73c67ca5f38 100644 --- a/confmap/expand.go +++ b/confmap/expand.go @@ -76,8 +76,6 @@ func (mr *Resolver) expandValue(ctx context.Context, value any) (any, bool, erro return value, false, nil } -//test: "${http://example.com/?os=${env:OS}}" - // findURI attempts to find the first expandable URI in input. It returns an expandable // URI, or an empty string if none are found. // Note: findURI is only called when input contains a closing bracket. diff --git a/confmap/expand_test.go b/confmap/expand_test.go index 79ee68593dd..7f0b1483759 100644 --- a/confmap/expand_test.go +++ b/confmap/expand_test.go @@ -103,12 +103,6 @@ func TestResolverExpandStringValues(t *testing.T) { input string output any }{ - // Embedded. - //{ - // name: "Test", - // input: "${env:HOST${HOST}${HOST}}", - // output: "idk", - //}, { name: "NoMatchOldStyle", input: "${HOST}:${PORT}", From 91feb7226adf5512694e1787aa9ef677a607b86a Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Wed, 22 May 2024 14:57:13 -0600 Subject: [PATCH 06/17] Add more tests --- confmap/expand_test.go | 94 ++++++++++++++++++++++++++++++++++++---- confmap/resolver_test.go | 12 +++-- 2 files changed, 94 insertions(+), 12 deletions(-) diff --git a/confmap/expand_test.go b/confmap/expand_test.go index 7f0b1483759..0fddbae59be 100644 --- a/confmap/expand_test.go +++ b/confmap/expand_test.go @@ -99,19 +99,33 @@ func TestResolverExpandMapAndSliceValues(t *testing.T) { func TestResolverExpandStringValues(t *testing.T) { tests := []struct { - name string - input string - output any + name string + input string + output any + defaultProvider bool }{ + // Embedded. { name: "NoMatchOldStyle", input: "${HOST}:${PORT}", - output: "localhost:3044", + output: "${HOST}:${PORT}", + }, + { + name: "NoMatchOldStyleDefaultProvider", + input: "${HOST}:${PORT}", + output: "localhost:3044", + defaultProvider: true, }, { name: "NoMatchOldStyleNoBrackets", input: "${HOST}:$PORT", - output: "localhost:$PORT", + output: "${HOST}:$PORT", + }, + { + name: "NoMatchOldStyleNoBracketsDefaultProvider", + input: "${HOST}:$PORT", + output: "localhost:$PORT", + defaultProvider: true, }, { name: "ComplexValue", @@ -136,7 +150,13 @@ func TestResolverExpandStringValues(t *testing.T) { { name: "EmbeddedNewAndOldStyle", input: "${env:HOST}:${PORT}", - output: "localhost:3044", + output: "localhost:${PORT}", + }, + { + name: "EmbeddedNewAndOldStyleDefaultProvider", + input: "${env:HOST}:${PORT}", + output: "localhost:3044", + defaultProvider: true, }, { name: "Int", @@ -180,11 +200,23 @@ func TestResolverExpandStringValues(t *testing.T) { input: "${test:localhost:${env:PORT}}", output: "localhost:3044", }, + { + name: "NestedDefaultProvider", + input: "${test:localhost:${PORT}}", + output: "localhost:3044", + defaultProvider: true, + }, { name: "EmbeddedInNested", input: "${test:${env:HOST}:${env:PORT}}", output: "localhost:3044", }, + { + name: "EmbeddedInNestedDefaultProvider", + input: "${test:${HOST}:${PORT}}", + output: "localhost:3044", + defaultProvider: true, + }, { name: "EmbeddedAndNested", input: "${test:localhost:${env:PORT}}?os=${env:OS}", @@ -201,35 +233,71 @@ func TestResolverExpandStringValues(t *testing.T) { input: "env:HOST}", output: "env:HOST}", }, + { + name: "NoMatchMissingOpeningBracketDefaultProvider", + input: "env:HOST}", + output: "env:HOST}", + defaultProvider: true, + }, { name: "NoMatchMissingClosingBracket", input: "${HOST", output: "${HOST", }, + { + name: "NoMatchMissingClosingBracketDefaultProvider", + input: "${HOST", + output: "${HOST", + defaultProvider: true, + }, { name: "NoMatchBracketsWithout$", input: "HO{ST}", output: "HO{ST}", }, + { + name: "NoMatchBracketsWithout$DefaultProvider", + input: "HO{ST}", + output: "HO{ST}", + defaultProvider: true, + }, { name: "NoMatchOnlyMissingClosingBracket", input: "${env:HOST${env:PORT?os=${env:OS", output: "${env:HOST${env:PORT?os=${env:OS", }, + { + name: "NoMatchOnlyMissingClosingBracketDefaultProvider", + input: "${env:HOST${env:PORT?os=${env:OS", + output: "${env:HOST${env:PORT?os=${env:OS", + defaultProvider: true, + }, { name: "NoMatchOnlyMissingOpeningBracket", input: "env:HOST}env:PORT}?os=env:OS}", output: "env:HOST}env:PORT}?os=env:OS}", }, + { + name: "NoMatchOnlyMissingOpeningBracketDefaultProvider", + input: "env:HOST}env:PORT}?os=env:OS}", + output: "env:HOST}env:PORT}?os=env:OS}", + defaultProvider: true, + }, { name: "NoMatchCloseBeforeOpen", input: "env:HOST}${env:PORT", output: "env:HOST}${env:PORT", }, + { + name: "NoMatchCloseBeforeOpenDefaultProvider", + input: "env:HOST}${env:PORT", + output: "env:HOST}${env:PORT", + defaultProvider: true, + }, { name: "NoMatchOldStyleNested", input: "${test:localhost:${PORT}}", - output: "localhost:3044", + output: "${test:localhost:${PORT}}", }, // Partial expand. { @@ -237,6 +305,12 @@ func TestResolverExpandStringValues(t *testing.T) { input: "env:HOST}${env:PORT}", output: "env:HOST}3044", }, + { + name: "PartialMatchMissingOpeningBracketFirstDefaultProvider", + input: "env:HOST}${PORT}", + output: "env:HOST}3044", + defaultProvider: true, + }, { name: "PartialMatchMissingOpeningBracketLast", input: "${env:HOST}env:PORT}", @@ -295,7 +369,11 @@ func TestResolverExpandStringValues(t *testing.T) { }) envProvider := newEnvProvider() - resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, envProvider, testProvider}, DefaultProvider: envProvider, ConverterFactories: nil}) + set := ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, envProvider, testProvider}, ConverterFactories: nil} + if tt.defaultProvider { + set.DefaultProvider = envProvider + } + resolver, err := NewResolver(set) require.NoError(t, err) cfgMap, err := resolver.Resolve(context.Background()) diff --git a/confmap/resolver_test.go b/confmap/resolver_test.go index 43334de6d43..e1a66135404 100644 --- a/confmap/resolver_test.go +++ b/confmap/resolver_test.go @@ -397,12 +397,16 @@ func TestProvidesDefaultLogger(t *testing.T) { } func TestResolverDefaultProviderSet(t *testing.T) { - provider := newEnvProvider() + envProvider := newEnvProvider() + fileProvider := newFileProvider(t) + r, err := NewResolver(ResolverSettings{ URIs: []string{"env:"}, - ProviderFactories: []ProviderFactory{provider}, - DefaultProvider: provider, + ProviderFactories: []ProviderFactory{fileProvider}, + DefaultProvider: envProvider, }) require.NoError(t, err) - require.NotNil(t, r.defaultProvider) + assert.NotNil(t, r.defaultProvider) + _, ok := r.providers["env"] + assert.True(t, ok) } From 2424bec910c6d961bd7632b5ff62af5d6a3cd5a3 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Wed, 22 May 2024 15:00:04 -0600 Subject: [PATCH 07/17] Apply suggestions from code review Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> --- .chloggen/confmap-default-provider.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.chloggen/confmap-default-provider.yaml b/.chloggen/confmap-default-provider.yaml index 97c67d2f7a6..2bc9fb28e67 100644 --- a/.chloggen/confmap-default-provider.yaml +++ b/.chloggen/confmap-default-provider.yaml @@ -7,7 +7,7 @@ change_type: enhancement component: confmap # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). -note: Allow setting a default Provider on a Resolver to use when `${}` syntax is used without a schema +note: Allow setting a default Provider on a Resolver to use when `${}` syntax is used without a scheme # One or more tracking issues or pull requests related to the change issues: [10182] @@ -22,4 +22,4 @@ subtext: # 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: [] +change_logs: [api] From 0a37fd43a052d2253dccc9283b4928b216f9b136 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Wed, 22 May 2024 15:07:56 -0600 Subject: [PATCH 08/17] cleanup findURI --- confmap/expand.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/confmap/expand.go b/confmap/expand.go index 73c67ca5f38..7def2793936 100644 --- a/confmap/expand.go +++ b/confmap/expand.go @@ -76,6 +76,14 @@ func (mr *Resolver) expandValue(ctx context.Context, value any) (any, bool, erro return value, false, nil } +func (mr *Resolver) findURIHelper(remaining string) (string, string) { + // if remaining does not contain "}", there are no URIs left: stop recursion. + if !strings.Contains(remaining, "}") { + return "", "" + } + return mr.findURI(remaining) +} + // findURI attempts to find the first expandable URI in input. It returns an expandable // URI, or an empty string if none are found. // Note: findURI is only called when input contains a closing bracket. @@ -85,16 +93,15 @@ func (mr *Resolver) findURI(input string) (string, string) { openIndex := strings.LastIndex(input[:closeIndex+1], "${") // if there is a missing "${" or the uri does not contain ":", check the next URI. - if openIndex < 0 || (!strings.Contains(input[openIndex:closeIndex+1], ":") && mr.defaultProvider == nil) { - // if remaining does not contain "}", there are no URIs left: stop recursion. - if !strings.Contains(remaining, "}") { - return "", "" - } - return mr.findURI(remaining) + if openIndex < 0 { + return mr.findURIHelper(remaining) } potentialURI := input[openIndex : closeIndex+1] - if !strings.Contains(potentialURI, ":") && mr.defaultProvider != nil { + if !strings.Contains(potentialURI, ":") { + if mr.defaultProvider == nil { + return mr.findURIHelper(remaining) + } return potentialURI, strings.Replace(potentialURI, "${", fmt.Sprintf("${%v:", mr.defaultProvider.Scheme()), 1) } return potentialURI, potentialURI From 8513223ba31db58b212670a1507dd7954ea7e047 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Thu, 23 May 2024 12:24:02 -0600 Subject: [PATCH 09/17] Use string instead of provider for default --- confmap/expand.go | 54 +++++++++++++++++++++++----------------- confmap/expand_test.go | 4 +-- confmap/resolver.go | 35 +++++++++++++------------- confmap/resolver_test.go | 19 +++++++++++--- 4 files changed, 66 insertions(+), 46 deletions(-) diff --git a/confmap/expand.go b/confmap/expand.go index 7def2793936..682e42deb75 100644 --- a/confmap/expand.go +++ b/confmap/expand.go @@ -76,42 +76,32 @@ func (mr *Resolver) expandValue(ctx context.Context, value any) (any, bool, erro return value, false, nil } -func (mr *Resolver) findURIHelper(remaining string) (string, string) { - // if remaining does not contain "}", there are no URIs left: stop recursion. - if !strings.Contains(remaining, "}") { - return "", "" - } - return mr.findURI(remaining) -} - // findURI attempts to find the first expandable URI in input. It returns an expandable // URI, or an empty string if none are found. // Note: findURI is only called when input contains a closing bracket. -func (mr *Resolver) findURI(input string) (string, string) { +func (mr *Resolver) findURI(input string) string { closeIndex := strings.Index(input, "}") remaining := input[closeIndex+1:] openIndex := strings.LastIndex(input[:closeIndex+1], "${") // if there is a missing "${" or the uri does not contain ":", check the next URI. if openIndex < 0 { - return mr.findURIHelper(remaining) - } + // if remaining does not contain "}", there are no URIs left: stop recursion. + if !strings.Contains(remaining, "}") { - potentialURI := input[openIndex : closeIndex+1] - if !strings.Contains(potentialURI, ":") { - if mr.defaultProvider == nil { - return mr.findURIHelper(remaining) + return "" } - return potentialURI, strings.Replace(potentialURI, "${", fmt.Sprintf("${%v:", mr.defaultProvider.Scheme()), 1) + return mr.findURI(remaining) } - return potentialURI, potentialURI + + return input[openIndex : closeIndex+1] } // 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 } func (mr *Resolver) findAndExpandURI(ctx context.Context, input string) (any, bool, error) { - unmodifiedURI, uri := mr.findURI(input) + uri := mr.findURI(input) if uri == "" { // No URI found, return. return input, false, nil @@ -125,11 +115,14 @@ func (mr *Resolver) findAndExpandURI(ctx context.Context, input string) (any, bo if err != nil { return input, false, err } + if !changed { + return input, false, nil + } repl, err := toString(uri, expanded) if err != nil { return input, false, err } - return strings.ReplaceAll(input, unmodifiedURI, repl), changed, err + return strings.ReplaceAll(input, uri, repl), changed, err } // toString attempts to convert input to a string. @@ -150,11 +143,26 @@ func toString(strURI string, input any) (string, error) { } } -func (mr *Resolver) expandURI(ctx context.Context, uri string) (any, bool, error) { - lURI, err := newLocation(uri[2 : len(uri)-1]) - if err != nil { - return nil, false, err +func (mr *Resolver) expandURI(ctx context.Context, input string) (any, bool, error) { + // strip ${ and } + uri := input[2 : len(input)-1] + + var lURI location + var err error + // If no ':' then we have no scheme, check if there is a default scheme configured + if !strings.Contains(uri, ":") { + // If we have no default scheme there is nothing that can be expanded, return the original input unchanged + if mr.defaultScheme == "" { + return input, false, nil + } + lURI = location{scheme: mr.defaultScheme, opaqueValue: uri} + } else { + lURI, err = newLocation(uri) + if err != nil { + return nil, false, err + } } + if strings.Contains(lURI.opaqueValue, "$") { return nil, false, fmt.Errorf("the uri %q contains unsupported characters ('$')", lURI.asString()) } diff --git a/confmap/expand_test.go b/confmap/expand_test.go index 0fddbae59be..3b99b4c003b 100644 --- a/confmap/expand_test.go +++ b/confmap/expand_test.go @@ -371,7 +371,7 @@ func TestResolverExpandStringValues(t *testing.T) { envProvider := newEnvProvider() set := ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, envProvider, testProvider}, ConverterFactories: nil} if tt.defaultProvider { - set.DefaultProvider = envProvider + set.DefaultScheme = "env" } resolver, err := NewResolver(set) require.NoError(t, err) @@ -546,7 +546,7 @@ func TestResolverDefaultProviderExpand(t *testing.T) { return NewRetrieved(map[string]any{"foo": "${HOST}"}) }) - resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider}, DefaultProvider: newEnvProvider(), ConverterFactories: nil}) + resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, newEnvProvider()}, DefaultScheme: "env", ConverterFactories: nil}) require.NoError(t, err) cfgMap, err := resolver.Resolve(context.Background()) diff --git a/confmap/resolver.go b/confmap/resolver.go index 7b91a40995e..1f139c2cdbc 100644 --- a/confmap/resolver.go +++ b/confmap/resolver.go @@ -20,10 +20,10 @@ var driverLetterRegexp = regexp.MustCompile("^[A-z]:") // Resolver resolves a configuration as a Conf. type Resolver struct { - uris []location - providers map[string]Provider - defaultProvider Provider - converters []Converter + uris []location + providers map[string]Provider + defaultScheme string + converters []Converter closers []CloseFunc watcher chan error @@ -39,11 +39,11 @@ type ResolverSettings struct { // It is required to have at least one provider. ProviderFactories []ProviderFactory - // DefaultProvider is the provider that is used if ${} syntax is used but no schema is provided. - // If no DefaultProvider is set, ${} with no schema will not be expanded. - // It is strongly recommended to set an EnvProvider as the default to align with the + // DefaultScheme is the scheme that is used if ${} syntax is used but no schema is provided. + // If no DefaultScheme is set, ${} with no schema will not be expanded. + // It is strongly recommended to set an EnvProvider as the default scheme to align with the // OpenTelemetry Configuration Specification - DefaultProvider ProviderFactory + DefaultScheme string // ProviderSettings contains settings that will be passed to Provider // factories when instantiating Providers. @@ -99,10 +99,11 @@ func NewResolver(set ResolverSettings) (*Resolver, error) { providers[provider.Scheme()] = provider } - var defaultProvider Provider - if set.DefaultProvider != nil { - defaultProvider = set.DefaultProvider.Create(set.ProviderSettings) - providers[defaultProvider.Scheme()] = defaultProvider + if set.DefaultScheme != "" { + _, ok := providers[set.DefaultScheme] + if !ok { + return nil, errors.New("invalid 'confmap.ResolverSettings' configuration: DefaultScheme not found in providers list") + } } converters := make([]Converter, len(set.ConverterFactories)) @@ -131,11 +132,11 @@ func NewResolver(set ResolverSettings) (*Resolver, error) { } return &Resolver{ - uris: uris, - providers: providers, - defaultProvider: defaultProvider, - converters: converters, - watcher: make(chan error, 1), + uris: uris, + providers: providers, + defaultScheme: set.DefaultScheme, + converters: converters, + watcher: make(chan error, 1), }, nil } diff --git a/confmap/resolver_test.go b/confmap/resolver_test.go index e1a66135404..fecb973d089 100644 --- a/confmap/resolver_test.go +++ b/confmap/resolver_test.go @@ -124,6 +124,7 @@ func TestResolverErrors(t *testing.T) { locations []string providers []Provider converters []Converter + defaultScheme string expectBuildErr bool expectResolveErr bool expectWatchErr bool @@ -136,6 +137,16 @@ func TestResolverErrors(t *testing.T) { providers: []Provider{&mockProvider{}}, expectBuildErr: true, }, + { + name: "default scheme not found", + locations: []string{"mock:", "err:"}, + providers: []Provider{ + &mockProvider{}, + &mockProvider{scheme: "err", errR: errors.New("retrieve_err")}, + }, + defaultScheme: "missing", + expectBuildErr: true, + }, { name: "retrieve location config error", locations: []string{"mock:", "err:"}, @@ -201,7 +212,7 @@ func TestResolverErrors(t *testing.T) { c := converter converterFuncs[i] = NewConverterFactory(func(_ ConverterSettings) Converter { return c }) } - resolver, err := NewResolver(ResolverSettings{URIs: tt.locations, ProviderFactories: mockProviderFuncs, ConverterFactories: converterFuncs}) + resolver, err := NewResolver(ResolverSettings{URIs: tt.locations, ProviderFactories: mockProviderFuncs, DefaultScheme: tt.defaultScheme, ConverterFactories: converterFuncs}) if tt.expectBuildErr { assert.Error(t, err) return @@ -402,11 +413,11 @@ func TestResolverDefaultProviderSet(t *testing.T) { r, err := NewResolver(ResolverSettings{ URIs: []string{"env:"}, - ProviderFactories: []ProviderFactory{fileProvider}, - DefaultProvider: envProvider, + ProviderFactories: []ProviderFactory{fileProvider, envProvider}, + DefaultScheme: "env", }) require.NoError(t, err) - assert.NotNil(t, r.defaultProvider) + assert.NotNil(t, r.defaultScheme) _, ok := r.providers["env"] assert.True(t, ok) } From 9894938d325864c398a93c80a2e8fcf24d3f67dc Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Thu, 23 May 2024 12:25:51 -0600 Subject: [PATCH 10/17] Update comment --- confmap/expand.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/confmap/expand.go b/confmap/expand.go index 682e42deb75..df150747335 100644 --- a/confmap/expand.go +++ b/confmap/expand.go @@ -84,11 +84,10 @@ func (mr *Resolver) findURI(input string) string { remaining := input[closeIndex+1:] openIndex := strings.LastIndex(input[:closeIndex+1], "${") - // if there is a missing "${" or the uri does not contain ":", check the next URI. + // if there is a missing "${" check the next URI. if openIndex < 0 { // if remaining does not contain "}", there are no URIs left: stop recursion. if !strings.Contains(remaining, "}") { - return "" } return mr.findURI(remaining) From 2e8681d13d371b5eeb51a44a64b7376aa51a27ec Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Thu, 23 May 2024 12:26:16 -0600 Subject: [PATCH 11/17] Update comment --- confmap/expand.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/confmap/expand.go b/confmap/expand.go index df150747335..fccd6d13150 100644 --- a/confmap/expand.go +++ b/confmap/expand.go @@ -76,7 +76,7 @@ func (mr *Resolver) expandValue(ctx context.Context, value any) (any, bool, erro return value, false, nil } -// findURI attempts to find the first expandable URI in input. It returns an expandable +// 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. func (mr *Resolver) findURI(input string) string { From 37275079b28159755fdab03dca66714b754065f0 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 28 May 2024 12:49:53 -0600 Subject: [PATCH 12/17] Fix behavior change that caused scheme to not be expanded --- confmap/expand.go | 14 +++++++++----- confmap/expand_test.go | 22 ++++++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/confmap/expand.go b/confmap/expand.go index f7213325106..f481f22d02b 100644 --- a/confmap/expand.go +++ b/confmap/expand.go @@ -79,18 +79,21 @@ 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. -func (mr *Resolver) findURI(input string) string { +func findURI(input string, skipNoSchemeUri bool) string { closeIndex := strings.Index(input, "}") remaining := input[closeIndex+1:] openIndex := strings.LastIndex(input[:closeIndex+1], "${") - // if there is a missing "${" check the next URI. - if openIndex < 0 { + // if there is any of: + // - a missing "${" + // - no-scheme URIs are not allowed AND the value between `${}` does not contain a `:` + // then check the next URI. + if openIndex < 0 || (skipNoSchemeUri && !strings.Contains(input[openIndex:closeIndex+1], ":")) { // if remaining does not contain "}", there are no URIs left: stop recursion. if !strings.Contains(remaining, "}") { return "" } - return mr.findURI(remaining) + return findURI(remaining, skipNoSchemeUri) } return input[openIndex : closeIndex+1] @@ -100,7 +103,8 @@ func (mr *Resolver) findURI(input string) string { // 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 } func (mr *Resolver) findAndExpandURI(ctx context.Context, input string) (any, bool, error) { - uri := mr.findURI(input) + // no-scheme URIs are skipped if a default scheme is not set + uri := findURI(input, mr.defaultScheme == "") if uri == "" { // No URI found, return. return input, false, nil diff --git a/confmap/expand_test.go b/confmap/expand_test.go index db15d08ae42..407ff74a71f 100644 --- a/confmap/expand_test.go +++ b/confmap/expand_test.go @@ -356,6 +356,28 @@ func TestResolverExpandStringValues(t *testing.T) { input: "${env:HOST${env:PORT}?os=${env:OS&pr=${env:PR}", output: "${env:HOST3044?os=${env:OS&pr=amd", }, + { + name: "SchemeAfterNoSchemeIsExpanded", + input: "${HOST}${env:PORT}", + output: "${HOST}3044", + }, + { + name: "SchemeAfterNoSchemeIsExpandedDefaultProvider", + input: "${HOST}${env:PORT}", + output: "localhost3044", + defaultProvider: true, + }, + { + name: "SchemeBeforeNoSchemeIsExpanded", + input: "${env:HOST}${PORT}", + output: "localhost${PORT}", + }, + { + name: "SchemeBeforeNoSchemeIsExpandedDefaultProvider", + input: "${env:HOST}${PORT}", + output: "localhost3044", + defaultProvider: true, + }, } for _, tt := range tests { From 806e9d24bf4913df96d7b6976e65c5588be7280f Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 28 May 2024 12:54:00 -0600 Subject: [PATCH 13/17] Fix lint --- confmap/expand.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/confmap/expand.go b/confmap/expand.go index f481f22d02b..2d8e7f5ea0f 100644 --- a/confmap/expand.go +++ b/confmap/expand.go @@ -79,7 +79,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. -func findURI(input string, skipNoSchemeUri bool) string { +func findURI(input string, skipNoSchemeURI bool) string { closeIndex := strings.Index(input, "}") remaining := input[closeIndex+1:] openIndex := strings.LastIndex(input[:closeIndex+1], "${") @@ -88,12 +88,12 @@ func findURI(input string, skipNoSchemeUri bool) string { // - a missing "${" // - no-scheme URIs are not allowed AND the value between `${}` does not contain a `:` // then check the next URI. - if openIndex < 0 || (skipNoSchemeUri && !strings.Contains(input[openIndex:closeIndex+1], ":")) { + if openIndex < 0 || (skipNoSchemeURI && !strings.Contains(input[openIndex:closeIndex+1], ":")) { // if remaining does not contain "}", there are no URIs left: stop recursion. if !strings.Contains(remaining, "}") { return "" } - return findURI(remaining, skipNoSchemeUri) + return findURI(remaining, skipNoSchemeURI) } return input[openIndex : closeIndex+1] From 63be09a6647d2480ae8c1e04501bb90ab47a5140 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 28 May 2024 13:47:16 -0600 Subject: [PATCH 14/17] cleanup --- confmap/expand.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/confmap/expand.go b/confmap/expand.go index 2d8e7f5ea0f..0974f017411 100644 --- a/confmap/expand.go +++ b/confmap/expand.go @@ -79,21 +79,21 @@ 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. -func findURI(input string, skipNoSchemeURI bool) string { +func (mr *Resolver) findURI(input string) string { closeIndex := strings.Index(input, "}") remaining := input[closeIndex+1:] openIndex := strings.LastIndex(input[:closeIndex+1], "${") // if there is any of: // - a missing "${" - // - no-scheme URIs are not allowed AND the value between `${}` does not contain a `:` + // - there is no default scheme AND no scheme is detected because no `:` is found. // then check the next URI. - if openIndex < 0 || (skipNoSchemeURI && !strings.Contains(input[openIndex:closeIndex+1], ":")) { + if openIndex < 0 || (mr.defaultScheme == "" && !strings.Contains(input[openIndex:closeIndex+1], ":")) { // if remaining does not contain "}", there are no URIs left: stop recursion. if !strings.Contains(remaining, "}") { return "" } - return findURI(remaining, skipNoSchemeURI) + return mr.findURI(remaining) } return input[openIndex : closeIndex+1] @@ -103,8 +103,7 @@ func findURI(input string, skipNoSchemeURI bool) string { // 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 } func (mr *Resolver) findAndExpandURI(ctx context.Context, input string) (any, bool, error) { - // no-scheme URIs are skipped if a default scheme is not set - uri := findURI(input, mr.defaultScheme == "") + uri := mr.findURI(input) if uri == "" { // No URI found, return. return input, false, nil @@ -118,9 +117,6 @@ func (mr *Resolver) findAndExpandURI(ctx context.Context, input string) (any, bo if err != nil { return input, false, err } - if !changed { - return input, false, nil - } repl, err := toString(expanded) if err != nil { return input, false, fmt.Errorf("expanding %v: %w", uri, err) From 6f8e1738903cbb1f49234e09e1ad1a960de96220 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 28 May 2024 15:18:28 -0600 Subject: [PATCH 15/17] More cleanup --- confmap/expand.go | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-) diff --git a/confmap/expand.go b/confmap/expand.go index 0974f017411..768395f76fd 100644 --- a/confmap/expand.go +++ b/confmap/expand.go @@ -146,20 +146,13 @@ func (mr *Resolver) expandURI(ctx context.Context, input string) (any, bool, err // strip ${ and } uri := input[2 : len(input)-1] - var lURI location - var err error - // If no ':' then we have no scheme, check if there is a default scheme configured if !strings.Contains(uri, ":") { - // If we have no default scheme there is nothing that can be expanded, return the original input unchanged - if mr.defaultScheme == "" { - return input, false, nil - } - lURI = location{scheme: mr.defaultScheme, opaqueValue: uri} - } else { - lURI, err = newLocation(uri) - if err != nil { - return nil, false, err - } + uri = fmt.Sprintf("%s:%s", mr.defaultScheme, uri) + } + + lURI, err := newLocation(uri) + if err != nil { + return nil, false, err } if strings.Contains(lURI.opaqueValue, "$") { From e89296f765352b791e4c57a5420f4016b1f9c074 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Wed, 29 May 2024 16:19:21 -0600 Subject: [PATCH 16/17] Apply suggestions from code review Co-authored-by: Evan Bradley <11745660+evan-bradley@users.noreply.github.com> --- confmap/resolver.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/confmap/resolver.go b/confmap/resolver.go index 1f139c2cdbc..7c9ed303c73 100644 --- a/confmap/resolver.go +++ b/confmap/resolver.go @@ -35,13 +35,13 @@ type ResolverSettings struct { // It is required to have at least one location. URIs []string - // ProviderFactories is a slice of Provider creation functions. - // It is required to have at least one provider. + // ProviderFactories is a slice of Provider factories. + // It is required to have at least one factory. ProviderFactories []ProviderFactory // DefaultScheme is the scheme that is used if ${} syntax is used but no schema is provided. // If no DefaultScheme is set, ${} with no schema will not be expanded. - // It is strongly recommended to set an EnvProvider as the default scheme to align with the + // It is strongly recommended to set "env" as the default scheme to align with the // OpenTelemetry Configuration Specification DefaultScheme string From 68de2db6ce845cba07af234ebe35d95228016f5a Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Wed, 29 May 2024 16:23:22 -0600 Subject: [PATCH 17/17] add comment --- confmap/expand_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/confmap/expand_test.go b/confmap/expand_test.go index 407ff74a71f..b7bfc59483d 100644 --- a/confmap/expand_test.go +++ b/confmap/expand_test.go @@ -407,6 +407,8 @@ func TestResolverExpandStringValues(t *testing.T) { func newEnvProvider() ProviderFactory { return newFakeProvider("env", func(_ context.Context, uri string, _ WatcherFunc) (*Retrieved, error) { + // When using `env` as the default scheme for tests, the uri will not include `env:`. + // Instead of duplicating the switch cases, the scheme is added instead. if uri[0:4] != "env:" { uri = "env:" + uri }