Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add validate interface in configmodels to force each component do configuration validation #2802

Merged
merged 1 commit into from
Mar 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions component/componenttest/nop_receiver.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package componenttest

import (
"context"
"fmt"

"go.opentelemetry.io/collector/component"
"go.opentelemetry.io/collector/component/componenthelper"
Expand All @@ -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(),
},
}
}

Expand Down
2 changes: 1 addition & 1 deletion component/componenttest/nop_receiver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 7 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
31 changes: 30 additions & 1 deletion config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,23 @@ package config

import (
"errors"
"fmt"
"testing"

"github.com/stretchr/testify/assert"
)

type NopConfig struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to be public.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll move

Let's merge as first draft, I will followup with some cleanups.

@bogdandrutu , I am going to expand the same checks to all the other type of components in the next PR. Sounds good to you?

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)
Expand Down Expand Up @@ -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 {
Expand All @@ -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"},
Expand Down
19 changes: 18 additions & 1 deletion config/receiver.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,27 @@ 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 and any other interface.
type CustomConfigOptions interface {
CustomConfigValidator
}

// 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.
type CustomValidator func() error

// Receivers is a map of names to Receivers.
type Receivers map[string]Receiver

Expand All @@ -30,7 +47,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 {
Expand Down
4 changes: 4 additions & 0 deletions internal/testcomponents/example_receiver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
8 changes: 8 additions & 0 deletions receiver/hostmetricsreceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -24,3 +25,10 @@ type Config struct {
scraperhelper.ScraperControllerSettings `mapstructure:",squash"`
Scrapers map[string]internal.Config `mapstructure:"-"`
}

var _ config.CustomConfigOptions = (*Config)(nil)

// Validate checks the receiver configuration is valid
func (cfg *Config) Validate() error {
return nil
}
7 changes: 7 additions & 0 deletions receiver/jaegerreceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,10 @@ type Config struct {
Protocols `mapstructure:"protocols"`
RemoteSampling *RemoteSamplingConfig `mapstructure:"remote_sampling"`
}

var _ config.CustomConfigOptions = (*Config)(nil)

// Validate checks the receiver configuration is valid
func (cfg *Config) Validate() error {
return nil
}
7 changes: 7 additions & 0 deletions receiver/kafkareceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,10 @@ type Config struct {

Authentication kafkaexporter.Authentication `mapstructure:"auth"`
}

var _ config.CustomConfigOptions = (*Config)(nil)

// Validate checks the receiver configuration is valid
func (cfg *Config) Validate() error {
return nil
}
15 changes: 11 additions & 4 deletions receiver/opencensusreceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
bogdandrutu marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}
Expand All @@ -49,3 +49,10 @@ func (rOpts *Config) buildOptions() ([]ocOption, error) {

return opts, nil
}

var _ config.CustomConfigOptions = (*Config)(nil)

// Validate checks the receiver configuration is valid
func (cfg *Config) Validate() error {
return nil
}
7 changes: 7 additions & 0 deletions receiver/otlpreceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 checks the receiver configuration is valid
func (cfg *Config) Validate() error {
return nil
}
7 changes: 7 additions & 0 deletions receiver/prometheusreceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 checks the receiver configuration is valid
func (cfg *Config) Validate() error {
return nil
}
3 changes: 0 additions & 3 deletions receiver/prometheusreceiver/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
5 changes: 2 additions & 3 deletions receiver/prometheusreceiver/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
9 changes: 6 additions & 3 deletions receiver/receiverhelper/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
9 changes: 9 additions & 0 deletions receiver/zipkinreceiver/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 checks the receiver configuration is valid
func (cfg *Config) Validate() error {
return nil
}
5 changes: 3 additions & 2 deletions service/internal/builder/factories_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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"},
}
})
}
Expand Down
7 changes: 3 additions & 4 deletions service/internal/builder/receivers_builder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{}

Expand All @@ -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{}

Expand Down Expand Up @@ -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)
Expand Down