From 1d88e19f5b295f3afca6207663778dd963503513 Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Tue, 1 Mar 2022 15:28:01 -0800 Subject: [PATCH] Change deprecated NewConfigProvider to use options and do input validation Signed-off-by: Bogdan Drutu --- CHANGELOG.md | 3 + service/collector_test.go | 66 +++++++++++++----- service/collector_windows.go | 10 ++- service/command.go | 10 ++- service/config_provider.go | 108 +++++++++++++++++++++++------- service/config_provider_test.go | 114 +++++++++++++++----------------- 6 files changed, 210 insertions(+), 101 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 138846a725b5..4ccb87f6eefa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ ### 🚩 Deprecations 🚩 +- Deprecate `service.MustNewConfigProvider` and `service.MustNewDefaultConfigProvider`in favor of `service.NewConfigProvider` (#4762) + ### 💡 Enhancements 💡 - OTLP HTTP receiver will use HTTP/2 over TLS if client supports it (#5190) @@ -42,6 +44,7 @@ - Move MapProvider to config, split providers in their own package (#5030) - API related to `pdata.AttributeValue` is deprecated in favor of `pdata.Value` (#4978) +- API related to `pdata.AttributeValue` is deprecated in favor of `pdata.Value` (#4975) - `pdata.AttributeValue` struct is deprecated in favor of `pdata.Value` - `pdata.AttributeValueType` type is deprecated in favor of `pdata.ValueType` - `pdata.AttributeValueType...` constants are deprecated in favor of `pdata.ValueType...` diff --git a/service/collector_test.go b/service/collector_test.go index 507a3fc6ee6c..2051f8231573 100644 --- a/service/collector_test.go +++ b/service/collector_test.go @@ -36,6 +36,8 @@ import ( "go.uber.org/zap/zapcore" "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/config" + "go.opentelemetry.io/collector/config/configmapprovider" "go.opentelemetry.io/collector/internal/testcomponents" "go.opentelemetry.io/collector/internal/testutil" "go.opentelemetry.io/collector/service/featuregate" @@ -60,11 +62,17 @@ func TestCollector_StartAsGoRoutine(t *testing.T) { factories, err := testcomponents.NewDefaultFactories() require.NoError(t, err) + cfgProvider, err := NewConfigProvider( + []string{ + filepath.Join("testdata", "otelcol-config.yaml"), + "yaml:service::telemetry::metrics::address: localhost:" + strconv.FormatUint(uint64(testutil.GetAvailablePort(t)), 10), + }) + require.NoError(t, err) + set := CollectorSettings{ - BuildInfo: component.NewDefaultBuildInfo(), - Factories: factories, - ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, - []string{"service.telemetry.metrics.address=localhost:" + strconv.FormatUint(uint64(testutil.GetAvailablePort(t)), 10)}), + BuildInfo: component.NewDefaultBuildInfo(), + Factories: factories, + ConfigProvider: cfgProvider, } col, err := New(set) require.NoError(t, err) @@ -98,12 +106,19 @@ func testCollectorStartHelper(t *testing.T) { } metricsPort := testutil.GetAvailablePort(t) + cfgProvider, err := NewConfigProvider( + []string{filepath.Join("testdata", "otelcol-config.yaml")}, + WithConfigMapConverters([]config.MapConverterFunc{ + configmapprovider.NewOverwritePropertiesConverter( + []string{"service.telemetry.metrics.address=localhost:" + strconv.FormatUint(uint64(metricsPort), 10)}, + ), + })) + require.NoError(t, err) + col, err := New(CollectorSettings{ - BuildInfo: component.NewDefaultBuildInfo(), - Factories: factories, - ConfigProvider: MustNewDefaultConfigProvider( - []string{filepath.Join("testdata", "otelcol-config.yaml")}, - []string{"service.telemetry.metrics.address=localhost:" + strconv.FormatUint(uint64(metricsPort), 10)}), + BuildInfo: component.NewDefaultBuildInfo(), + Factories: factories, + ConfigProvider: cfgProvider, LoggingOptions: []zap.Option{zap.Hooks(hook)}, }) require.NoError(t, err) @@ -169,10 +184,13 @@ func TestCollector_ShutdownNoop(t *testing.T) { factories, err := testcomponents.NewDefaultFactories() require.NoError(t, err) + cfgProvider, err := NewConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}) + require.NoError(t, err) + set := CollectorSettings{ BuildInfo: component.NewDefaultBuildInfo(), Factories: factories, - ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, nil), + ConfigProvider: cfgProvider, } col, err := New(set) require.NoError(t, err) @@ -190,10 +208,13 @@ func TestCollector_ShutdownBeforeRun(t *testing.T) { factories, err := testcomponents.NewDefaultFactories() require.NoError(t, err) + cfgProvider, err := NewConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}) + require.NoError(t, err) + set := CollectorSettings{ BuildInfo: component.NewDefaultBuildInfo(), Factories: factories, - ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, nil), + ConfigProvider: cfgProvider, } col, err := New(set) require.NoError(t, err) @@ -221,11 +242,14 @@ func TestCollector_ClosedStateOnStartUpError(t *testing.T) { factories, err := testcomponents.NewDefaultFactories() require.NoError(t, err) + cfgProvider, err := NewConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}) + require.NoError(t, err) + // Load a bad config causing startup to fail set := CollectorSettings{ BuildInfo: component.NewDefaultBuildInfo(), Factories: factories, - ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-invalid.yaml")}, nil), + ConfigProvider: cfgProvider, } col, err := New(set) require.NoError(t, err) @@ -256,10 +280,13 @@ func TestCollector_ReportError(t *testing.T) { factories, err := testcomponents.NewDefaultFactories() require.NoError(t, err) + cfgProvider, err := NewConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}) + require.NoError(t, err) + col, err := New(CollectorSettings{ BuildInfo: component.NewDefaultBuildInfo(), Factories: factories, - ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, nil), + ConfigProvider: cfgProvider, }) require.NoError(t, err) @@ -288,11 +315,16 @@ func TestCollector_ContextCancel(t *testing.T) { factories, err := testcomponents.NewDefaultFactories() require.NoError(t, err) + cfgProvider, err := NewConfigProvider([]string{ + filepath.Join("testdata", "otelcol-config.yaml"), + "yaml:service::telemetry::metrics::address: localhost:" + strconv.FormatUint(uint64(testutil.GetAvailablePort(t)), 10), + }) + require.NoError(t, err) + set := CollectorSettings{ - BuildInfo: component.NewDefaultBuildInfo(), - Factories: factories, - ConfigProvider: MustNewDefaultConfigProvider([]string{filepath.Join("testdata", "otelcol-config.yaml")}, - []string{"service.telemetry.metrics.address=localhost:" + strconv.FormatUint(uint64(testutil.GetAvailablePort(t)), 10)}), + BuildInfo: component.NewDefaultBuildInfo(), + Factories: factories, + ConfigProvider: cfgProvider, } col, err := New(set) require.NoError(t, err) diff --git a/service/collector_windows.go b/service/collector_windows.go index a9c74c1c55da..e3fba61bbf1a 100644 --- a/service/collector_windows.go +++ b/service/collector_windows.go @@ -29,6 +29,8 @@ import ( "golang.org/x/sys/windows/svc" "golang.org/x/sys/windows/svc/eventlog" + "go.opentelemetry.io/collector/config" + "go.opentelemetry.io/collector/config/configmapprovider" "go.opentelemetry.io/collector/service/featuregate" ) @@ -144,7 +146,13 @@ func openEventLog(serviceName string) (*eventlog.Log, error) { func newWithWindowsEventLogCore(set CollectorSettings, elog *eventlog.Log) (*Collector, error) { if set.ConfigProvider == nil { - set.ConfigProvider = MustNewDefaultConfigProvider(getConfigFlag(), getSetFlag()) + var err error + set.ConfigProvider, err = NewConfigProvider( + getConfigFlag(), + WithConfigMapConverters([]config.MapConverterFunc{configmapprovider.NewOverwritePropertiesConverter(getSetFlag())})) + if err != nil { + return nil, err + } } set.LoggingOptions = append( set.LoggingOptions, diff --git a/service/command.go b/service/command.go index d71d7ca82489..4fb43a70f803 100644 --- a/service/command.go +++ b/service/command.go @@ -17,6 +17,8 @@ package service // import "go.opentelemetry.io/collector/service" import ( "github.com/spf13/cobra" + "go.opentelemetry.io/collector/config" + "go.opentelemetry.io/collector/config/configmapprovider" "go.opentelemetry.io/collector/service/featuregate" ) @@ -29,7 +31,13 @@ func NewCommand(set CollectorSettings) *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { featuregate.Apply(gatesList) if set.ConfigProvider == nil { - set.ConfigProvider = MustNewDefaultConfigProvider(getConfigFlag(), getSetFlag()) + var err error + set.ConfigProvider, err = NewConfigProvider( + getConfigFlag(), + WithConfigMapConverters([]config.MapConverterFunc{configmapprovider.NewOverwritePropertiesConverter(getSetFlag())})) + if err != nil { + return err + } } col, err := New(set) if err != nil { diff --git a/service/config_provider.go b/service/config_provider.go index 02e9bbd3fb2b..ea69e8e16353 100644 --- a/service/config_provider.go +++ b/service/config_provider.go @@ -68,50 +68,112 @@ type ConfigProvider interface { } type configProvider struct { - locations []string - configMapProviders map[string]config.MapProvider - cfgMapConverters []config.MapConverterFunc - configUnmarshaler configunmarshaler.ConfigUnmarshaler + locations []string + configMapProviders map[string]config.MapProvider + configMapConverters []config.MapConverterFunc + configUnmarshaler configunmarshaler.ConfigUnmarshaler sync.Mutex closer config.CloseFunc watcher chan error } -// MustNewConfigProvider returns a new ConfigProvider that provides the configuration: -// * Retrieve the config.Map by merging all retrieved maps from all the config.MapProvider in order. -// * Then applies all the ConfigMapConverterFunc in the given order. -// * Then unmarshalls the final config.Config using the given configunmarshaler.ConfigUnmarshaler. -// -// The `configMapProviders` is a map of pairs . +// ConfigProviderOption is an option to change the behavior of ConfigProvider +// returned by NewConfigProvider() +type ConfigProviderOption func(opts *configProvider) + +// WithConfigMapProvider appends to the default available providers. +// If a provider with the same scheme already exists, it is getting overwritten. +func WithConfigMapProvider(c config.MapProvider) ConfigProviderOption { + return func(opts *configProvider) { + opts.configMapProviders[c.Scheme()] = c + } +} + +// WithConfigMapConverters overwrites the default converters. +func WithConfigMapConverters(c []config.MapConverterFunc) ConfigProviderOption { + return func(opts *configProvider) { + opts.configMapConverters = c + } +} + +// withConfigUnmarshaler overwrites the default unmarshaler. +// This func is private because providing custom ConfigUnmarshaler is not necessary since users can wrap/implement +// ConfigProvider if needed to change the resulted config. +func withConfigUnmarshaler(c configunmarshaler.ConfigUnmarshaler) ConfigProviderOption { + return func(opts *configProvider) { + opts.configUnmarshaler = c + } +} + +// Deprecated: [v0.48.0] use NewConfigProvider instead. func MustNewConfigProvider( locations []string, configMapProviders map[string]config.MapProvider, cfgMapConverters []config.MapConverterFunc, configUnmarshaler configunmarshaler.ConfigUnmarshaler) ConfigProvider { + opts := []ConfigProviderOption{ + WithConfigMapConverters(cfgMapConverters), + withConfigUnmarshaler(configUnmarshaler), + } + for _, c := range configMapProviders { + opts = append(opts, WithConfigMapProvider(c)) + } + cfgProvider, err := NewConfigProvider(locations, opts...) + if err != nil { + panic(err) + } + return cfgProvider +} + +// NewConfigProvider returns a new ConfigProvider that provides the configuration: +// * Retrieve the config.Map by merging all retrieved maps from all the configmapprovider.Provider in order. +// * Then applies all the config.MapConverterFunc in the given order. +// * Then unmarshalls the final config.Config using the given configunmarshaler.ConfigUnmarshaler. +func NewConfigProvider(locations []string, opts ...ConfigProviderOption) (ConfigProvider, error) { + if len(locations) == 0 { + return nil, fmt.Errorf("cannot create ConfigProvider: no locations provided") + } // Safe copy, ensures the slice cannot be changed from the caller. locationsCopy := make([]string, len(locations)) copy(locationsCopy, locations) - return &configProvider{ + + opts = append([]ConfigProviderOption{ + WithConfigMapProvider(filemapprovider.New()), + WithConfigMapProvider(envmapprovider.New()), + WithConfigMapProvider(yamlmapprovider.New()), + }, opts...) + + provider := configProvider{ locations: locationsCopy, - configMapProviders: configMapProviders, - cfgMapConverters: cfgMapConverters, - configUnmarshaler: configUnmarshaler, - watcher: make(chan error, 1), + configMapProviders: make(map[string]config.MapProvider), + configMapConverters: []config.MapConverterFunc{ + configmapprovider.NewExpandConverter(), + }, + configUnmarshaler: configunmarshaler.NewDefault(), + watcher: make(chan error, 1), + } + + // Override default values with user options + for _, o := range opts { + o(&provider) } + + return &provider, nil } -// MustNewDefaultConfigProvider returns the default ConfigProvider from slice of location strings -// (e.g. file:/path/to/config.yaml) and property overrides (e.g. service.telemetry.metrics.address=localhost:8888). +// Deprecated: [v0.48.0] use NewConfigProvider instead. func MustNewDefaultConfigProvider(configLocations []string, properties []string) ConfigProvider { - return MustNewConfigProvider( + cfgProvider, err := NewConfigProvider( configLocations, - makeConfigMapProviderMap(filemapprovider.New(), envmapprovider.New(), yamlmapprovider.New()), - []config.MapConverterFunc{ + WithConfigMapConverters([]config.MapConverterFunc{ configmapprovider.NewOverwritePropertiesConverter(properties), configmapprovider.NewExpandConverter(), - }, - configunmarshaler.NewDefault()) + })) + if err != nil { + panic(err) + } + return cfgProvider } func (cm *configProvider) Get(ctx context.Context, factories component.Factories) (*config.Config, error) { @@ -127,7 +189,7 @@ func (cm *configProvider) Get(ctx context.Context, factories component.Factories cm.closer = ret.CloseFunc // Apply all converters. - for _, cfgMapConv := range cm.cfgMapConverters { + for _, cfgMapConv := range cm.configMapConverters { if err = cfgMapConv(ctx, ret.Map); err != nil { return nil, fmt.Errorf("cannot convert the config.Map: %w", err) } diff --git a/service/config_provider_test.go b/service/config_provider_test.go index 15bcc6492d5d..9344a802d170 100644 --- a/service/config_provider_test.go +++ b/service/config_provider_test.go @@ -34,11 +34,12 @@ import ( ) type mockProvider struct { - retM *config.Map - errR error - errS error - errW error - errC error + shceme string + retM *config.Map + errR error + errS error + errW error + errC error } func (m *mockProvider) Retrieve(_ context.Context, _ string, watcher config.WatcherFunc) (config.Retrieved, error) { @@ -55,7 +56,10 @@ func (m *mockProvider) Retrieve(_ context.Context, _ string, watcher config.Watc } func (m *mockProvider) Scheme() string { - return "mock" + if m.shceme == "" { + return "mock" + } + return m.shceme } func (m *mockProvider) Shutdown(context.Context) error { @@ -77,7 +81,7 @@ func TestConfigProvider_Errors(t *testing.T) { tests := []struct { name string locations []string - parserProvider map[string]config.MapProvider + parserProvider []config.MapProvider cfgMapConverters []config.MapConverterFunc configUnmarshaler configunmarshaler.ConfigUnmarshaler expectNewErr bool @@ -85,65 +89,51 @@ func TestConfigProvider_Errors(t *testing.T) { expectShutdownErr bool }{ { - name: "retrieve_err", - locations: []string{"mock:", "not_supported:"}, - parserProvider: map[string]config.MapProvider{ - "mock": &mockProvider{}, - }, + name: "retrieve_err", + locations: []string{"mock:", "not_supported:"}, + parserProvider: []config.MapProvider{&mockProvider{}}, configUnmarshaler: configunmarshaler.NewDefault(), expectNewErr: true, }, { name: "retrieve_err", locations: []string{"mock:", "err:"}, - parserProvider: map[string]config.MapProvider{ - "mock": &mockProvider{}, - "err": &mockProvider{errR: errors.New("retrieve_err")}, + parserProvider: []config.MapProvider{ + &mockProvider{}, + &mockProvider{shceme: "err", errR: errors.New("retrieve_err")}, }, configUnmarshaler: configunmarshaler.NewDefault(), expectNewErr: true, }, { - name: "converter_err", - locations: []string{"mock:", filepath.Join("testdata", "otelcol-nop.yaml")}, - parserProvider: map[string]config.MapProvider{ - "mock": &mockProvider{}, - "file": filemapprovider.New(), - }, + name: "converter_err", + locations: []string{"mock:", filepath.Join("testdata", "otelcol-nop.yaml")}, + parserProvider: []config.MapProvider{&mockProvider{}, filemapprovider.New()}, cfgMapConverters: []config.MapConverterFunc{func(context.Context, *config.Map) error { return errors.New("converter_err") }}, configUnmarshaler: configunmarshaler.NewDefault(), expectNewErr: true, }, { - name: "unmarshal_err", - locations: []string{"mock:", filepath.Join("testdata", "otelcol-nop.yaml")}, - parserProvider: map[string]config.MapProvider{ - "mock": &mockProvider{}, - "file": filemapprovider.New(), - }, + name: "unmarshal_err", + locations: []string{"mock:", filepath.Join("testdata", "otelcol-nop.yaml")}, + parserProvider: []config.MapProvider{&mockProvider{}, filemapprovider.New()}, configUnmarshaler: &errConfigUnmarshaler{err: errors.New("unmarshal_err")}, expectNewErr: true, }, { - name: "validation_err", - locations: []string{"mock:", filepath.Join("testdata", "otelcol-invalid.yaml")}, - parserProvider: map[string]config.MapProvider{ - "mock": &mockProvider{}, - "file": filemapprovider.New(), - }, + name: "validation_err", + locations: []string{"mock:", filepath.Join("testdata", "otelcol-invalid.yaml")}, + parserProvider: []config.MapProvider{&mockProvider{}, filemapprovider.New()}, configUnmarshaler: configunmarshaler.NewDefault(), expectNewErr: true, }, { name: "watch_err", locations: []string{"mock:", "err:"}, - parserProvider: func() map[string]config.MapProvider { + parserProvider: func() []config.MapProvider { cfgMap, err := configtest.LoadConfigMap(filepath.Join("testdata", "otelcol-nop.yaml")) require.NoError(t, err) - return map[string]config.MapProvider{ - "mock": &mockProvider{}, - "err": &mockProvider{retM: cfgMap, errW: errors.New("watch_err")}, - } + return []config.MapProvider{&mockProvider{}, &mockProvider{shceme: "err", retM: cfgMap, errW: errors.New("watch_err")}} }(), configUnmarshaler: configunmarshaler.NewDefault(), expectWatchErr: true, @@ -151,12 +141,12 @@ func TestConfigProvider_Errors(t *testing.T) { { name: "close_err", locations: []string{"mock:", "err:"}, - parserProvider: func() map[string]config.MapProvider { + parserProvider: func() []config.MapProvider { cfgMap, err := configtest.LoadConfigMap(filepath.Join("testdata", "otelcol-nop.yaml")) require.NoError(t, err) - return map[string]config.MapProvider{ - "mock": &mockProvider{}, - "err": &mockProvider{retM: cfgMap, errC: errors.New("close_err")}, + return []config.MapProvider{ + &mockProvider{}, + &mockProvider{shceme: "err", retM: cfgMap, errC: errors.New("close_err")}, } }(), configUnmarshaler: configunmarshaler.NewDefault(), @@ -165,7 +155,16 @@ func TestConfigProvider_Errors(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cfgW := MustNewConfigProvider(tt.locations, tt.parserProvider, tt.cfgMapConverters, tt.configUnmarshaler) + opts := []ConfigProviderOption{ + WithConfigMapConverters(tt.cfgMapConverters), + withConfigUnmarshaler(tt.configUnmarshaler), + } + for _, c := range tt.parserProvider { + opts = append(opts, WithConfigMapProvider(c)) + } + cfgW, err := NewConfigProvider(tt.locations, opts...) + assert.NoError(t, err) + _, errN := cfgW.Get(context.Background(), factories) if tt.expectNewErr { assert.Error(t, errN) @@ -200,11 +199,8 @@ func TestConfigProvider(t *testing.T) { return &mockProvider{retM: cfgMap} }() - cfgW := MustNewConfigProvider( - []string{"watcher:"}, - map[string]config.MapProvider{"watcher": configMapProvider}, - nil, - configunmarshaler.NewDefault()) + cfgW, err := NewConfigProvider([]string{"mock:"}, WithConfigMapProvider(configMapProvider)) + require.NoError(t, err) _, errN := cfgW.Get(context.Background(), factories) assert.NoError(t, errN) @@ -228,11 +224,10 @@ func TestConfigProviderNoWatcher(t *testing.T) { require.NoError(t, errF) watcherWG := sync.WaitGroup{} - cfgW := MustNewConfigProvider( + cfgW, err := NewConfigProvider( []string{filepath.Join("testdata", "otelcol-nop.yaml")}, - map[string]config.MapProvider{"file": filemapprovider.New()}, - nil, - configunmarshaler.NewDefault()) + WithConfigMapProvider(filemapprovider.New())) + require.NoError(t, err) _, errN := cfgW.Get(context.Background(), factories) assert.NoError(t, errN) @@ -260,12 +255,12 @@ func TestConfigProvider_ShutdownClosesWatch(t *testing.T) { }() watcherWG := sync.WaitGroup{} - cfgW := MustNewConfigProvider( - []string{"watcher:"}, - map[string]config.MapProvider{"watcher": configMapProvider}, - nil, - configunmarshaler.NewDefault()) + cfgW, err := NewConfigProvider( + []string{"mock:"}, + WithConfigMapProvider(configMapProvider)) _, errN := cfgW.Get(context.Background(), factories) + require.NoError(t, err) + assert.NoError(t, errN) watcherWG.Add(1) @@ -322,8 +317,9 @@ func TestBackwardsCompatibilityForFilePath(t *testing.T) { }, } for _, tt := range tests { - provider := MustNewDefaultConfigProvider([]string{tt.location}, []string{}) - _, err := provider.Get(context.Background(), factories) + provider, err := NewConfigProvider([]string{tt.location}) + assert.NoError(t, err) + _, err = provider.Get(context.Background(), factories) assert.Contains(t, err.Error(), tt.errMessage, tt.name) }