From 2beed98471f066a50da6d5f6952122fa1f067638 Mon Sep 17 00:00:00 2001 From: Pablo Baeyens Date: Fri, 2 Aug 2024 11:36:15 +0200 Subject: [PATCH] [confmap] Validate providers scheme when building a Resolver (#10786) #### Description Validate providers schemes when building a Resolver. I don't consider this a breaking change since the providers would be useless if they don't follow this pattern. --- .../mx-psi_restrict-providers-scheme.yaml | 25 +++++++++++++++++++ confmap/expand_test.go | 13 +++------- confmap/resolver.go | 12 ++++++++- confmap/resolver_test.go | 9 +++++-- 4 files changed, 46 insertions(+), 13 deletions(-) create mode 100644 .chloggen/mx-psi_restrict-providers-scheme.yaml diff --git a/.chloggen/mx-psi_restrict-providers-scheme.yaml b/.chloggen/mx-psi_restrict-providers-scheme.yaml new file mode 100644 index 00000000000..e96b51f6408 --- /dev/null +++ b/.chloggen/mx-psi_restrict-providers-scheme.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: Check that providers have a correct scheme when building a confmap.Resolver. + +# One or more tracking issues or pull requests related to the change +issues: [10786] + +# (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: [api] diff --git a/confmap/expand_test.go b/confmap/expand_test.go index 53b244374b1..5b87d747543 100644 --- a/confmap/expand_test.go +++ b/confmap/expand_test.go @@ -61,11 +61,8 @@ func TestResolverDoneNotExpandOldEnvVars(t *testing.T) { envProvider := newFakeProvider("env", func(context.Context, string, WatcherFunc) (*Retrieved, error) { return NewRetrieved("some string") }) - emptySchemeProvider := newFakeProvider("", func(context.Context, string, WatcherFunc) (*Retrieved, error) { - return NewRetrieved("some string") - }) - resolver, err := NewResolver(ResolverSettings{URIs: []string{"test:"}, ProviderFactories: []ProviderFactory{fileProvider, envProvider, emptySchemeProvider}, ConverterFactories: nil}) + resolver, err := NewResolver(ResolverSettings{URIs: []string{"test:"}, ProviderFactories: []ProviderFactory{fileProvider, envProvider}, ConverterFactories: nil}) require.NoError(t, err) // Test that expanded configs are the same with the simple config with no env vars. @@ -509,12 +506,8 @@ func TestResolverExpandInvalidScheme(t *testing.T) { panic("must not be called") }) - resolver, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, testProvider}, ConverterFactories: nil}) - require.NoError(t, err) - - _, err = resolver.Resolve(context.Background()) - - assert.EqualError(t, err, `invalid uri: "g_c_s:VALUE"`) + _, err := NewResolver(ResolverSettings{URIs: []string{"input:"}, ProviderFactories: []ProviderFactory{provider, testProvider}, ConverterFactories: nil}) + assert.ErrorContains(t, err, "invalid 'confmap.Provider' scheme") } func TestResolverExpandInvalidOpaqueValue(t *testing.T) { diff --git a/confmap/resolver.go b/confmap/resolver.go index 7b7de3d5092..f3a7c7092e3 100644 --- a/confmap/resolver.go +++ b/confmap/resolver.go @@ -96,7 +96,17 @@ func NewResolver(set ResolverSettings) (*Resolver, error) { providers := make(map[string]Provider, len(set.ProviderFactories)) for _, factory := range set.ProviderFactories { provider := factory.Create(set.ProviderSettings) - providers[provider.Scheme()] = provider + scheme := provider.Scheme() + // Check that the scheme follows the pattern. + if !regexp.MustCompile(schemePattern).MatchString(scheme) { + return nil, fmt.Errorf("invalid 'confmap.Provider' scheme %q", scheme) + } + // Check that the scheme is unique. + if _, ok := providers[scheme]; ok { + return nil, fmt.Errorf("duplicate 'confmap.Provider' scheme %q", scheme) + } + + providers[scheme] = provider } if set.DefaultScheme != "" { diff --git a/confmap/resolver_test.go b/confmap/resolver_test.go index fecb973d089..e939d124acb 100644 --- a/confmap/resolver_test.go +++ b/confmap/resolver_test.go @@ -113,11 +113,16 @@ func (m *mockConverter) Convert(context.Context, *Conf) error { return errors.New("converter_err") } -func TestNewResolverInvalidScheme(t *testing.T) { - _, err := NewResolver(ResolverSettings{URIs: []string{"s_3:has invalid char"}, ProviderFactories: []ProviderFactory{newMockProvider(&mockProvider{scheme: "s_3"})}}) +func TestNewResolverInvalidSchemeInURI(t *testing.T) { + _, err := NewResolver(ResolverSettings{URIs: []string{"s_3:has invalid char"}, ProviderFactories: []ProviderFactory{newMockProvider(&mockProvider{scheme: "s3"})}}) assert.EqualError(t, err, `invalid uri: "s_3:has invalid char"`) } +func TestNewResolverDuplicateScheme(t *testing.T) { + _, err := NewResolver(ResolverSettings{URIs: []string{"mock:something"}, ProviderFactories: []ProviderFactory{newMockProvider(&mockProvider{scheme: "mock"}), newMockProvider(&mockProvider{scheme: "mock"})}}) + assert.EqualError(t, err, `duplicate 'confmap.Provider' scheme "mock"`) +} + func TestResolverErrors(t *testing.T) { tests := []struct { name string