Transform expand provider to converter (#4636)

Signed-off-by: Bogdan Drutu <[email protected]>

service/config_provider.go

@@ -86,12 +86,15 @@ func newConfigProvider(configMapProvider configmapprovider.Provider, configUnmar
 
 // ConfigMapConverterFunc is a converter function for the config.Map that allows distributions
 // (in the future components as well) to build backwards compatible config converters.
-type ConfigMapConverterFunc func(*config.Map) (*config.Map, error)
+type ConfigMapConverterFunc func(*config.Map) error
 
 // NewDefaultConfigProvider returns the default ConfigProvider, and it creates configuration from a file
 // defined by the given configFile and overwrites fields using properties.
 func NewDefaultConfigProvider(configFileName string, properties []string, cfgMapConverters ...ConfigMapConverterFunc) ConfigProvider {
-	return newConfigProvider(configprovider.NewDefaultMapProvider(configFileName, properties), configunmarshaler.NewDefault(), cfgMapConverters...)
+	return newConfigProvider(
+		configprovider.NewDefaultMapProvider(configFileName, properties),
+		configunmarshaler.NewDefault(),
+		append(cfgMapConverters, configprovider.NewExpandConverter())...)
 }
 
 func (cm *configProvider) Get(ctx context.Context, factories component.Factories) (*config.Config, error) {
@@ -116,8 +119,7 @@ func (cm *configProvider) Get(ctx context.Context, factories component.Factories
 
 	// Apply all converters.
 	for _, cfgMapConv := range cm.cfgMapConverters {
-		cfgMap, err = cfgMapConv(cfgMap)
-		if err != nil {
+		if err = cfgMapConv(cfgMap); err != nil {
 			return nil, fmt.Errorf("cannot convert the config.Map: %w", err)
 		}
 	}

service/config_provider_test.go

@@ -95,7 +95,7 @@ func TestConfigProvider_Errors(t *testing.T) {
 		{
 			name:              "converter_err",
 			parserProvider:    configmapprovider.NewFile(path.Join("testdata", "otelcol-nop.yaml")),
-			cfgMapConverters:  []ConfigMapConverterFunc{func(c *config.Map) (*config.Map, error) { return nil, errors.New("converter_err") }},
+			cfgMapConverters:  []ConfigMapConverterFunc{func(c *config.Map) error { return errors.New("converter_err") }},
 			configUnmarshaler: configunmarshaler.NewDefault(),
 			expectNewErr:      true,
 		},

service/internal/configprovider/default.go

@@ -21,5 +21,5 @@ import (
 // NewDefaultMapProvider returns the default configmapprovider.Provider, and it creates configuration from a file
 // defined by the given configFile and overwrites fields using properties.
 func NewDefaultMapProvider(configFileName string, properties []string) configmapprovider.Provider {
-	return NewExpand(NewMerge(configmapprovider.NewFile(configFileName), configmapprovider.NewProperties(properties)))
+	return NewMerge(configmapprovider.NewFile(configFileName), configmapprovider.NewProperties(properties))
 }

service/internal/configprovider/expand.go

@@ -15,45 +15,21 @@
 package configprovider // import "go.opentelemetry.io/collector/service/internal/configprovider"
 
 import (
-	"context"
-	"fmt"
 	"os"
 
 	"go.opentelemetry.io/collector/config"
-	"go.opentelemetry.io/collector/config/configmapprovider"
 )
 
-type expandMapProvider struct {
-	base configmapprovider.Provider
-}
-
-// NewExpand returns a Provider, that expands all environment variables for a
-// config.Map provided by the given Provider.
-func NewExpand(base configmapprovider.Provider) configmapprovider.Provider {
-	return &expandMapProvider{
-		base: base,
-	}
-}
-
-func (emp *expandMapProvider) Retrieve(ctx context.Context, onChange func(*configmapprovider.ChangeEvent)) (configmapprovider.Retrieved, error) {
-	retr, err := emp.base.Retrieve(ctx, onChange)
-	if err != nil {
-		return nil, fmt.Errorf("failed to retrieve from base provider: %w", err)
-	}
-	cfgMap, err := retr.Get(ctx)
-	if err != nil {
-		return nil, fmt.Errorf("failed to get from base provider retrieved: %w", err)
-	}
-	for _, k := range cfgMap.AllKeys() {
-		cfgMap.Set(k, expandStringValues(cfgMap.Get(k)))
+// NewExpandConverter returns a service.ConfigMapConverterFunc, that expands all environment variables for a given config.Map.
+//
+// This does not directly return service.ConfigMapConverterFunc to avoid circular dependencies, not a problem since it is internal.
+func NewExpandConverter() func(*config.Map) error {
+	return func(cfgMap *config.Map) error {
+		for _, k := range cfgMap.AllKeys() {
+			cfgMap.Set(k, expandStringValues(cfgMap.Get(k)))
+		}
+		return nil
 	}
-	return configmapprovider.NewRetrieved(func(ctx context.Context) (*config.Map, error) {
-		return cfgMap, nil
-	}, configmapprovider.WithClose(retr.Close))
-}
-
-func (emp *expandMapProvider) Shutdown(ctx context.Context) error {
-	return emp.base.Shutdown(ctx)
 }
 
 func expandStringValues(value interface{}) interface{} {

service/internal/configprovider/expand_test.go

@@ -16,7 +16,6 @@ package configprovider
 
 import (
 	"context"
-	"errors"
 	"os"
 	"path"
 	"testing"
@@ -25,46 +24,10 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"go.opentelemetry.io/collector/config/configmapprovider"
+	"go.opentelemetry.io/collector/config/configtest"
 )
 
-func TestBaseRetrieveFailsOnRetrieve(t *testing.T) {
-	retErr := errors.New("test error")
-	exp := NewExpand(&mockProvider{retrieveErr: retErr})
-	t.Cleanup(func() { require.NoError(t, exp.Shutdown(context.Background())) })
-	_, err := exp.Retrieve(context.Background(), nil)
-	assert.Error(t, err)
-	assert.ErrorIs(t, err, retErr)
-}
-
-func TestBaseRetrieveFailsOnGet(t *testing.T) {
-	getErr := errors.New("test error")
-	exp := NewExpand(&mockProvider{retrieved: newErrGetRetrieved(getErr)})
-	t.Cleanup(func() { require.NoError(t, exp.Shutdown(context.Background())) })
-	_, err := exp.Retrieve(context.Background(), nil)
-	assert.Error(t, err)
-	assert.ErrorIs(t, err, getErr)
-}
-
-func TestBaseRetrieveFailsOnClose(t *testing.T) {
-	closeErr := errors.New("test error")
-	exp := NewExpand(&mockProvider{retrieved: newErrCloseRetrieved(closeErr)})
-	t.Cleanup(func() { require.NoError(t, exp.Shutdown(context.Background())) })
-	ret, err := exp.Retrieve(context.Background(), nil)
-	require.NoError(t, err)
-	err = ret.Close(context.Background())
-	assert.Error(t, err)
-	assert.ErrorIs(t, err, closeErr)
-}
-
-func TestBaseRetrieveFailsOnShutdown(t *testing.T) {
-	shutdownErr := errors.New("test error")
-	exp := NewExpand(&mockProvider{shutdownErr: shutdownErr})
-	err := exp.Shutdown(context.Background())
-	assert.Error(t, err)
-	assert.ErrorIs(t, err, shutdownErr)
-}
-
-func TestExpand(t *testing.T) {
+func TestNewExpandConverter(t *testing.T) {
 	var testCases = []struct {
 		name string // test case name (also file name containing config yaml)
 	}{
@@ -96,29 +59,25 @@ func TestExpand(t *testing.T) {
 
 	for _, test := range testCases {
 		t.Run(test.name, func(t *testing.T) {
-			// Retrieve the config
-			emp := NewExpand(configmapprovider.NewFile(path.Join("testdata", test.name)))
-			cp, err := emp.Retrieve(context.Background(), nil)
+			cfgMap, err := configtest.LoadConfigMap(path.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.
-			m, err := cp.Get(context.Background())
-			require.NoError(t, err)
-			assert.Equal(t, expectedCfgMap.ToStringMap(), m.ToStringMap())
+			require.NoError(t, NewExpandConverter()(cfgMap))
+			assert.Equal(t, expectedCfgMap.ToStringMap(), cfgMap.ToStringMap())
 		})
 	}
 }
 
-func TestExpand_EscapedEnvVars(t *testing.T) {
+func TestNewExpandConverter_EscapedEnvVars(t *testing.T) {
 	const receiverExtraMapValue = "some map value"
 	assert.NoError(t, os.Setenv("MAP_VALUE_2", receiverExtraMapValue))
 	defer func() {
 		assert.NoError(t, os.Unsetenv("MAP_VALUE_2"))
 	}()
 
 	// Retrieve the config
-	emp := NewExpand(configmapprovider.NewFile(path.Join("testdata", "expand-escaped-env.yaml")))
-	cp, err := emp.Retrieve(context.Background(), nil)
+	cfgMap, err := configtest.LoadConfigMap(path.Join("testdata", "expand-escaped-env.yaml"))
 	require.NoError(t, err, "Unable to get config")
 
 	expectedMap := map[string]interface{}{
@@ -138,7 +97,6 @@ func TestExpand_EscapedEnvVars(t *testing.T) {
 			// escaped $ alone
 			"recv.7": "$",
 		}}
-	m, err := cp.Get(context.Background())
-	require.NoError(t, err)
-	assert.Equal(t, expectedMap, m.ToStringMap())
+	require.NoError(t, NewExpandConverter()(cfgMap))
+	assert.Equal(t, expectedMap, cfgMap.ToStringMap())
 }

service/servicetest/configprovider.go

@@ -21,13 +21,12 @@ import (
 	"go.opentelemetry.io/collector/config"
 	"go.opentelemetry.io/collector/config/configmapprovider"
 	"go.opentelemetry.io/collector/config/configunmarshaler"
-	"go.opentelemetry.io/collector/service/internal/configprovider"
 )
 
 // LoadConfig loads a config.Config  from file, and does NOT validate the configuration.
 func LoadConfig(fileName string, factories component.Factories) (*config.Config, error) {
 	// Read yaml config from file
-	cp, err := configprovider.NewExpand(configmapprovider.NewFile(fileName)).Retrieve(context.Background(), nil)
+	cp, err := configmapprovider.NewFile(fileName).Retrieve(context.Background(), nil)
 	if err != nil {
 		return nil, err
 	}