From 69aab76689cdd71fdb069ba3d191561b8e3cd06e Mon Sep 17 00:00:00 2001 From: xiami Date: Mon, 29 Mar 2021 15:31:54 -0700 Subject: [PATCH] Add validate interface in configmodels to force each component do configuration validation --- component/componenttest/nop_receiver.go | 18 +++++++++-- component/componenttest/nop_receiver_test.go | 2 +- config/config.go | 7 +++++ config/config_test.go | 31 ++++++++++++++++++- config/receiver.go | 22 ++++++++++++- internal/testcomponents/example_receiver.go | 4 +++ receiver/hostmetricsreceiver/config.go | 8 +++++ receiver/jaegerreceiver/config.go | 7 +++++ receiver/kafkareceiver/config.go | 7 +++++ receiver/opencensusreceiver/config.go | 15 ++++++--- receiver/otlpreceiver/config.go | 7 +++++ receiver/prometheusreceiver/config.go | 7 +++++ receiver/prometheusreceiver/factory.go | 3 -- receiver/prometheusreceiver/factory_test.go | 5 ++- receiver/receiverhelper/factory_test.go | 9 ++++-- receiver/zipkinreceiver/config.go | 9 ++++++ service/internal/builder/factories_test.go | 5 +-- .../builder/receivers_builder_test.go | 7 ++--- 18 files changed, 149 insertions(+), 24 deletions(-) diff --git a/component/componenttest/nop_receiver.go b/component/componenttest/nop_receiver.go index 839e97de797..1db938a0722 100644 --- a/component/componenttest/nop_receiver.go +++ b/component/componenttest/nop_receiver.go @@ -16,6 +16,7 @@ package componenttest import ( "context" + "fmt" "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/component/componenthelper" @@ -38,10 +39,23 @@ func (f *nopReceiverFactory) Type() config.Type { return "nop" } +type NopConfig struct { + config.ReceiverSettings +} + +func (nc *NopConfig) Validate() error { + if nc.TypeVal != "nop" { + return fmt.Errorf("invalid receiver config") + } + return nil +} + // CreateDefaultConfig creates the default configuration for the Receiver. func (f *nopReceiverFactory) CreateDefaultConfig() config.Receiver { - return &config.ReceiverSettings{ - TypeVal: f.Type(), + return &NopConfig{ + ReceiverSettings: config.ReceiverSettings{ + TypeVal: f.Type(), + }, } } diff --git a/component/componenttest/nop_receiver_test.go b/component/componenttest/nop_receiver_test.go index 0e9904c6107..db29340d5ad 100644 --- a/component/componenttest/nop_receiver_test.go +++ b/component/componenttest/nop_receiver_test.go @@ -31,7 +31,7 @@ func TestNewNopReceiverFactory(t *testing.T) { require.NotNil(t, factory) assert.Equal(t, config.Type("nop"), factory.Type()) cfg := factory.CreateDefaultConfig() - assert.Equal(t, &config.ReceiverSettings{TypeVal: factory.Type()}, cfg) + assert.Equal(t, &NopConfig{ReceiverSettings: config.ReceiverSettings{TypeVal: factory.Type()}}, cfg) traces, err := factory.CreateTracesReceiver(context.Background(), component.ReceiverCreateParams{}, cfg, consumertest.NewTracesNop()) require.NoError(t, err) diff --git a/config/config.go b/config/config.go index 7f292da4481..d158a9fd81f 100644 --- a/config/config.go +++ b/config/config.go @@ -108,6 +108,13 @@ func (cfg *Config) validateServicePipelines() error { if cfg.Receivers[ref] == nil { return fmt.Errorf("pipeline %q references receiver %q which does not exist", pipeline.Name, ref) } + + // Validate the receiver configuration by the custom configuration validator + recCfg := cfg.Receivers[ref] + if err := recCfg.Validate(); err != nil { + return fmt.Errorf("pipeline %q references receiver %q which has invalid configuration with error: %v", pipeline.Name, ref, err) + } + } // Validate pipeline processor name references diff --git a/config/config_test.go b/config/config_test.go index fb88a641d5a..2909ccdcec8 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -16,11 +16,23 @@ package config import ( "errors" + "fmt" "testing" "github.com/stretchr/testify/assert" ) +type NopConfig struct { + ReceiverSettings +} + +func (nc *NopConfig) Validate() error { + if nc.TypeVal != "nop" { + return fmt.Errorf("invalid receiver config") + } + return nil +} + func TestConfigValidate(t *testing.T) { var testCases = []struct { name string // test case name (also file name containing config yaml) @@ -118,6 +130,19 @@ func TestConfigValidate(t *testing.T) { }, expected: errMissingServicePipelines, }, + { + name: "invalid-receiver-config", + cfgFn: func() *Config { + cfg := generateConfig() + cfg.Receivers["nop"] = &NopConfig{ + ReceiverSettings: ReceiverSettings{ + TypeVal: "invalid_rec_type", + }, + } + return cfg + }, + expected: fmt.Errorf(`pipeline "traces" references receiver "nop" which has invalid configuration with error: invalid receiver config`), + }, } for _, test := range testCases { @@ -131,7 +156,11 @@ func TestConfigValidate(t *testing.T) { func generateConfig() *Config { return &Config{ Receivers: map[string]Receiver{ - "nop": &ReceiverSettings{TypeVal: "nop"}, + "nop": &NopConfig{ + ReceiverSettings: ReceiverSettings{ + TypeVal: "nop", + }, + }, }, Exporters: map[string]Exporter{ "nop": &ExporterSettings{TypeVal: "nop"}, diff --git a/config/receiver.go b/config/receiver.go index bfa80f1c2f9..0a8cf20e605 100644 --- a/config/receiver.go +++ b/config/receiver.go @@ -16,10 +16,30 @@ package config // Receiver is the configuration of a receiver. Specific receivers must implement this // interface and will typically embed ReceiverSettings struct or a struct that extends it. +// Embedded CustomConfigOptions will force each receiver to implement Validate() function type Receiver interface { NamedEntity + CustomConfigOptions } +// CustomConfigOptions defines the interfaces for configuration customization options +// Different custom interfaces can be added like CustomConfigValidator, CustomConfigUnmarshaller, etc. +type CustomConfigOptions interface { + CustomConfigValidator + // CustomConfigUnmarshaller +} + +// CustomConfigValidator defines the interface for the custom configuration validation on each component +type CustomConfigValidator interface { + Validate() error +} + +// CustomValidator is a function that runs the customized validation +// on component level configuration. +// intoCfg interface{} +// An empty interface wrapping a pointer to the config struct. +type CustomValidator func() error + // Receivers is a map of names to Receivers. type Receivers map[string]Receiver @@ -30,7 +50,7 @@ type ReceiverSettings struct { NameVal string `mapstructure:"-"` } -var _ Receiver = (*ReceiverSettings)(nil) +var _ NamedEntity = (*ReceiverSettings)(nil) // Name gets the receiver name. func (rs *ReceiverSettings) Name() string { diff --git a/internal/testcomponents/example_receiver.go b/internal/testcomponents/example_receiver.go index 7f5a76e14c2..aa90b9f4c85 100644 --- a/internal/testcomponents/example_receiver.go +++ b/internal/testcomponents/example_receiver.go @@ -36,6 +36,10 @@ type ExampleReceiver struct { ExtraListSetting []string `mapstructure:"extra_list"` } +func (ecfg *ExampleReceiver) Validate() error { + return nil +} + const recvType = "examplereceiver" // ExampleReceiverFactory is factory for ExampleReceiver. diff --git a/receiver/hostmetricsreceiver/config.go b/receiver/hostmetricsreceiver/config.go index 82626aba81d..f0a498c2b9c 100644 --- a/receiver/hostmetricsreceiver/config.go +++ b/receiver/hostmetricsreceiver/config.go @@ -15,6 +15,7 @@ package hostmetricsreceiver import ( + "go.opentelemetry.io/collector/config" "go.opentelemetry.io/collector/receiver/hostmetricsreceiver/internal" "go.opentelemetry.io/collector/receiver/scraperhelper" ) @@ -24,3 +25,10 @@ type Config struct { scraperhelper.ScraperControllerSettings `mapstructure:",squash"` Scrapers map[string]internal.Config `mapstructure:"-"` } + +var _ config.CustomConfigOptions = (*Config)(nil) + +// Validate implements the custom validation check on configuration +func (cfg *Config) Validate() error { + return nil +} diff --git a/receiver/jaegerreceiver/config.go b/receiver/jaegerreceiver/config.go index 4efa9658c14..fe57abb2158 100644 --- a/receiver/jaegerreceiver/config.go +++ b/receiver/jaegerreceiver/config.go @@ -72,3 +72,10 @@ type Config struct { Protocols `mapstructure:"protocols"` RemoteSampling *RemoteSamplingConfig `mapstructure:"remote_sampling"` } + +var _ config.CustomConfigOptions = (*Config)(nil) + +// Validate implements the custom validation check on configuration +func (cfg *Config) Validate() error { + return nil +} diff --git a/receiver/kafkareceiver/config.go b/receiver/kafkareceiver/config.go index 3e1050ce25a..c4d9c9a9ebe 100644 --- a/receiver/kafkareceiver/config.go +++ b/receiver/kafkareceiver/config.go @@ -41,3 +41,10 @@ type Config struct { Authentication kafkaexporter.Authentication `mapstructure:"auth"` } + +var _ config.CustomConfigOptions = (*Config)(nil) + +// Validate implements the custom validation check on configuration +func (cfg *Config) Validate() error { + return nil +} diff --git a/receiver/opencensusreceiver/config.go b/receiver/opencensusreceiver/config.go index fbe379a3f5c..ac79b14b6bd 100644 --- a/receiver/opencensusreceiver/config.go +++ b/receiver/opencensusreceiver/config.go @@ -33,13 +33,13 @@ type Config struct { CorsOrigins []string `mapstructure:"cors_allowed_origins"` } -func (rOpts *Config) buildOptions() ([]ocOption, error) { +func (cfg *Config) buildOptions() ([]ocOption, error) { var opts []ocOption - if len(rOpts.CorsOrigins) > 0 { - opts = append(opts, withCorsOrigins(rOpts.CorsOrigins)) + if len(cfg.CorsOrigins) > 0 { + opts = append(opts, withCorsOrigins(cfg.CorsOrigins)) } - grpcServerOptions, err := rOpts.GRPCServerSettings.ToServerOption() + grpcServerOptions, err := cfg.GRPCServerSettings.ToServerOption() if err != nil { return nil, err } @@ -49,3 +49,10 @@ func (rOpts *Config) buildOptions() ([]ocOption, error) { return opts, nil } + +var _ config.CustomConfigOptions = (*Config)(nil) + +// Validate implements the custom validation check on configuration +func (cfg *Config) Validate() error { + return nil +} diff --git a/receiver/otlpreceiver/config.go b/receiver/otlpreceiver/config.go index 25c5a5b05b1..e4596719241 100644 --- a/receiver/otlpreceiver/config.go +++ b/receiver/otlpreceiver/config.go @@ -32,3 +32,10 @@ type Config struct { // Protocols is the configuration for the supported protocols, currently gRPC and HTTP (Proto and JSON). Protocols `mapstructure:"protocols"` } + +var _ config.CustomConfigOptions = (*Config)(nil) + +// Validate implements the custom validation check on configuration +func (cfg *Config) Validate() error { + return nil +} diff --git a/receiver/prometheusreceiver/config.go b/receiver/prometheusreceiver/config.go index d63745eeae3..8aaf8585b12 100644 --- a/receiver/prometheusreceiver/config.go +++ b/receiver/prometheusreceiver/config.go @@ -36,3 +36,10 @@ type Config struct { // structure, ie.: it will error if an unknown key is present. ConfigPlaceholder interface{} `mapstructure:"config"` } + +var _ config2.CustomConfigOptions = (*Config)(nil) + +// Validate implements the custom validation check on configuration +func (cfg *Config) Validate() error { + return nil +} diff --git a/receiver/prometheusreceiver/factory.go b/receiver/prometheusreceiver/factory.go index c195497ce73..5e2d3b7b00f 100644 --- a/receiver/prometheusreceiver/factory.go +++ b/receiver/prometheusreceiver/factory.go @@ -102,8 +102,5 @@ func createMetricsReceiver( nextConsumer consumer.Metrics, ) (component.MetricsReceiver, error) { config := cfg.(*Config) - if config.PrometheusConfig == nil || len(config.PrometheusConfig.ScrapeConfigs) == 0 { - return nil, errNilScrapeConfig - } return newPrometheusReceiver(params.Logger, config, nextConsumer), nil } diff --git a/receiver/prometheusreceiver/factory_test.go b/receiver/prometheusreceiver/factory_test.go index 9141561dddb..a167afeb004 100644 --- a/receiver/prometheusreceiver/factory_test.go +++ b/receiver/prometheusreceiver/factory_test.go @@ -40,9 +40,8 @@ func TestCreateReceiver(t *testing.T) { // The default config does not provide scrape_config so we expect that metrics receiver // creation must also fail. creationParams := component.ReceiverCreateParams{Logger: zap.NewNop()} - mReceiver, err := createMetricsReceiver(context.Background(), creationParams, cfg, nil) - assert.Equal(t, err, errNilScrapeConfig) - assert.Nil(t, mReceiver) + mReceiver, _ := createMetricsReceiver(context.Background(), creationParams, cfg, nil) + assert.NotNil(t, mReceiver) } func TestFactoryCanParseServiceDiscoveryConfigs(t *testing.T) { diff --git a/receiver/receiverhelper/factory_test.go b/receiver/receiverhelper/factory_test.go index 900fee3af34..817c8b5300f 100644 --- a/receiver/receiverhelper/factory_test.go +++ b/receiver/receiverhelper/factory_test.go @@ -23,15 +23,18 @@ import ( "github.com/stretchr/testify/assert" "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/component/componenttest" "go.opentelemetry.io/collector/config" "go.opentelemetry.io/collector/consumer" ) const typeStr = "test" -var defaultCfg = &config.ReceiverSettings{ - TypeVal: typeStr, - NameVal: typeStr, +var defaultCfg = &componenttest.NopConfig{ + ReceiverSettings: config.ReceiverSettings{ + TypeVal: typeStr, + NameVal: typeStr, + }, } func TestNewFactory(t *testing.T) { diff --git a/receiver/zipkinreceiver/config.go b/receiver/zipkinreceiver/config.go index 2914c220b15..5e9a0dd109a 100644 --- a/receiver/zipkinreceiver/config.go +++ b/receiver/zipkinreceiver/config.go @@ -29,4 +29,13 @@ type Config struct { // If enabled the zipkin receiver will attempt to parse string tags/binary annotations into int/bool/float. // Disabled by default ParseStringTags bool `mapstructure:"parse_string_tags"` + + config.CustomConfigOptions +} + +var _ config.CustomConfigOptions = (*Config)(nil) + +// Validate implements the custom validation check on configuration +func (cfg *Config) Validate() error { + return nil } diff --git a/service/internal/builder/factories_test.go b/service/internal/builder/factories_test.go index f4aded13ff1..c8ed32696fd 100644 --- a/service/internal/builder/factories_test.go +++ b/service/internal/builder/factories_test.go @@ -18,6 +18,7 @@ import ( "context" "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/component/componenttest" "go.opentelemetry.io/collector/config" "go.opentelemetry.io/collector/exporter/exporterhelper" "go.opentelemetry.io/collector/extension/extensionhelper" @@ -58,8 +59,8 @@ func createTestFactories() component.Factories { func newBadReceiverFactory() component.ReceiverFactory { return receiverhelper.NewFactory("bf", func() config.Receiver { - return &config.ReceiverSettings{ - TypeVal: "bf", + return &componenttest.NopConfig{ + ReceiverSettings: config.ReceiverSettings{TypeVal: "bf"}, } }) } diff --git a/service/internal/builder/receivers_builder_test.go b/service/internal/builder/receivers_builder_test.go index d3cda88caee..1cef5027c13 100644 --- a/service/internal/builder/receivers_builder_test.go +++ b/service/internal/builder/receivers_builder_test.go @@ -25,7 +25,6 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/component/componenttest" - "go.opentelemetry.io/collector/config" "go.opentelemetry.io/collector/config/configtest" "go.opentelemetry.io/collector/consumer/pdata" "go.opentelemetry.io/collector/internal/testcomponents" @@ -261,7 +260,7 @@ func TestBuildReceivers_BuildCustom(t *testing.T) { func TestBuildReceivers_StartAll(t *testing.T) { receivers := make(Receivers) - rcvCfg := &config.ReceiverSettings{} + rcvCfg := &componenttest.NopConfig{} receiver := &testcomponents.ExampleReceiverProducer{} @@ -280,7 +279,7 @@ func TestBuildReceivers_StartAll(t *testing.T) { func TestBuildReceivers_StopAll(t *testing.T) { receivers := make(Receivers) - rcvCfg := &config.ReceiverSettings{} + rcvCfg := &componenttest.NopConfig{} receiver := &testcomponents.ExampleReceiverProducer{} @@ -337,7 +336,7 @@ func TestBuildReceivers_NotSupportedDataType(t *testing.T) { t.Run(test.configFile, func(t *testing.T) { cfg, err := configtest.LoadConfigFile(t, path.Join("testdata", test.configFile), factories) - require.Nil(t, err) + assert.Error(t, err) allExporters, err := BuildExporters(zap.NewNop(), component.DefaultApplicationStartInfo(), cfg, factories.Exporters) assert.NoError(t, err)