From 8c8486116686aaa23bbb9d8391b7fc762c369173 Mon Sep 17 00:00:00 2001 From: Antoine Toulme Date: Wed, 7 Feb 2024 15:27:38 -0800 Subject: [PATCH] [otlpexporter] Validate the configuration explicitly --- exporter/otlpexporter/config.go | 9 +++ exporter/otlpexporter/config_test.go | 36 ++++++++++++ exporter/otlpexporter/factory.go | 15 +---- exporter/otlpexporter/factory_test.go | 20 +------ exporter/otlpexporter/otlp.go | 9 +-- .../testdata/invalid_configs.yaml | 55 +++++++++++++++++++ 6 files changed, 108 insertions(+), 36 deletions(-) create mode 100644 exporter/otlpexporter/testdata/invalid_configs.yaml diff --git a/exporter/otlpexporter/config.go b/exporter/otlpexporter/config.go index a657fa684c9..61440c3a9e3 100644 --- a/exporter/otlpexporter/config.go +++ b/exporter/otlpexporter/config.go @@ -4,6 +4,8 @@ package otlpexporter // import "go.opentelemetry.io/collector/exporter/otlpexporter" import ( + "errors" + "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/config/configgrpc" "go.opentelemetry.io/collector/config/configretry" @@ -19,4 +21,11 @@ type Config struct { configgrpc.ClientConfig `mapstructure:",squash"` // squash ensures fields are correctly decoded in embedded struct. } +func (c *Config) Validate() error { + if c.SanitizedEndpoint() == "" { + return errors.New(`requires a non-empty "endpoint"`) + } + return nil +} + var _ component.Config = (*Config)(nil) diff --git a/exporter/otlpexporter/config_test.go b/exporter/otlpexporter/config_test.go index ae2fac324d8..f65ce61c27e 100644 --- a/exporter/otlpexporter/config_test.go +++ b/exporter/otlpexporter/config_test.go @@ -78,3 +78,39 @@ func TestUnmarshalConfig(t *testing.T) { }, }, cfg) } + +func TestUnmarshalInvalidConfig(t *testing.T) { + cm, err := confmaptest.LoadConf(filepath.Join("testdata", "invalid_configs.yaml")) + require.NoError(t, err) + factory := NewFactory() + for _, test := range []struct { + name string + errorMsg string + }{ + { + name: "no_endpoint", + errorMsg: `requires a non-empty "endpoint"`, + }, + { + name: "http_endpoint", + errorMsg: `requires a non-empty "endpoint"`, + }, + { + name: "invalid_timeout", + errorMsg: `'timeout' must be non-negative`, + }, + { + name: "invalid_retry", + errorMsg: `'randomization_factor' must be within [0, 1]`, + }, + } { + t.Run(test.name, func(t *testing.T) { + cfg := factory.CreateDefaultConfig() + sub, err := cm.Sub(test.name) + require.NoError(t, err) + assert.NoError(t, component.UnmarshalConfig(sub, cfg)) + assert.ErrorContains(t, component.ValidateConfig(cfg), test.errorMsg) + }) + } + +} diff --git a/exporter/otlpexporter/factory.go b/exporter/otlpexporter/factory.go index dfb06073d93..8a72aab8cdc 100644 --- a/exporter/otlpexporter/factory.go +++ b/exporter/otlpexporter/factory.go @@ -48,10 +48,7 @@ func createTracesExporter( set exporter.CreateSettings, cfg component.Config, ) (exporter.Traces, error) { - oce, err := newExporter(cfg, set) - if err != nil { - return nil, err - } + oce := newExporter(cfg, set) oCfg := cfg.(*Config) return exporterhelper.NewTracesExporter(ctx, set, cfg, oce.pushTraces, @@ -68,10 +65,7 @@ func createMetricsExporter( set exporter.CreateSettings, cfg component.Config, ) (exporter.Metrics, error) { - oce, err := newExporter(cfg, set) - if err != nil { - return nil, err - } + oce := newExporter(cfg, set) oCfg := cfg.(*Config) return exporterhelper.NewMetricsExporter(ctx, set, cfg, oce.pushMetrics, @@ -89,10 +83,7 @@ func createLogsExporter( set exporter.CreateSettings, cfg component.Config, ) (exporter.Logs, error) { - oce, err := newExporter(cfg, set) - if err != nil { - return nil, err - } + oce := newExporter(cfg, set) oCfg := cfg.(*Config) return exporterhelper.NewLogsExporter(ctx, set, cfg, oce.pushLogs, diff --git a/exporter/otlpexporter/factory_test.go b/exporter/otlpexporter/factory_test.go index a3232a4094b..e601361c0c1 100644 --- a/exporter/otlpexporter/factory_test.go +++ b/exporter/otlpexporter/factory_test.go @@ -50,20 +50,10 @@ func TestCreateMetricsExporter(t *testing.T) { func TestCreateTracesExporter(t *testing.T) { endpoint := testutil.GetAvailableLocalAddress(t) tests := []struct { - name string - config *Config - mustFailOnCreate bool - mustFailOnStart bool + name string + config *Config + mustFailOnStart bool }{ - { - name: "NoEndpoint", - config: &Config{ - ClientConfig: configgrpc.ClientConfig{ - Endpoint: "", - }, - }, - mustFailOnCreate: true, - }, { name: "UseSecure", config: &Config{ @@ -178,10 +168,6 @@ func TestCreateTracesExporter(t *testing.T) { factory := NewFactory() set := exportertest.NewNopCreateSettings() consumer, err := factory.CreateTracesExporter(context.Background(), set, tt.config) - if tt.mustFailOnCreate { - assert.NotNil(t, err) - return - } assert.NoError(t, err) assert.NotNil(t, consumer) err = consumer.Start(context.Background(), componenttest.NewNopHost()) diff --git a/exporter/otlpexporter/otlp.go b/exporter/otlpexporter/otlp.go index 495213825b5..21864d7723a 100644 --- a/exporter/otlpexporter/otlp.go +++ b/exporter/otlpexporter/otlp.go @@ -5,7 +5,6 @@ package otlpexporter // import "go.opentelemetry.io/collector/exporter/otlpexpor import ( "context" - "errors" "fmt" "runtime" "time" @@ -47,17 +46,13 @@ type baseExporter struct { userAgent string } -func newExporter(cfg component.Config, set exporter.CreateSettings) (*baseExporter, error) { +func newExporter(cfg component.Config, set exporter.CreateSettings) *baseExporter { oCfg := cfg.(*Config) - if oCfg.Endpoint == "" { - return nil, errors.New("OTLP exporter config requires an Endpoint") - } - userAgent := fmt.Sprintf("%s/%s (%s/%s)", set.BuildInfo.Description, set.BuildInfo.Version, runtime.GOOS, runtime.GOARCH) - return &baseExporter{config: oCfg, settings: set.TelemetrySettings, userAgent: userAgent}, nil + return &baseExporter{config: oCfg, settings: set.TelemetrySettings, userAgent: userAgent} } // start actually creates the gRPC connection. The client construction is deferred till this point as this diff --git a/exporter/otlpexporter/testdata/invalid_configs.yaml b/exporter/otlpexporter/testdata/invalid_configs.yaml new file mode 100644 index 00000000000..73c24e3f13e --- /dev/null +++ b/exporter/otlpexporter/testdata/invalid_configs.yaml @@ -0,0 +1,55 @@ +no_endpoint: + timeout: 10s + sending_queue: + enabled: true + num_consumers: 2 + queue_size: 10 + retry_on_failure: + enabled: true + initial_interval: 10s + randomization_factor: 0.7 + multiplier: 1.3 + max_interval: 60s + max_elapsed_time: 10m +http_endpoint: + endpoint: http:// + timeout: 10s + sending_queue: + enabled: true + num_consumers: 2 + queue_size: 10 + retry_on_failure: + enabled: true + initial_interval: 10s + randomization_factor: 0.7 + multiplier: 1.3 + max_interval: 60s + max_elapsed_time: 10m +invalid_timeout: + endpoint: example.com:443 + timeout: -5s + sending_queue: + enabled: true + num_consumers: 2 + queue_size: 10 + retry_on_failure: + enabled: true + initial_interval: 10s + randomization_factor: 0.7 + multiplier: 1.3 + max_interval: 60s + max_elapsed_time: 10m +invalid_retry: + endpoint: example.com:443 + timeout: 30s + sending_queue: + enabled: true + num_consumers: 2 + queue_size: 10 + retry_on_failure: + enabled: true + initial_interval: 10s + randomization_factor: -5 + multiplier: 1.3 + max_interval: 60s + max_elapsed_time: 10m