From 61e36402d4251945bfa329690e29b98b9e6df6db Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Mon, 6 Jan 2025 16:27:41 -0700 Subject: [PATCH 1/7] Add validation logic to confmap --- confmap/confmap.go | 78 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 77 insertions(+), 1 deletion(-) diff --git a/confmap/confmap.go b/confmap/confmap.go index aab1730687b..e4cf67107be 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -15,6 +15,7 @@ import ( "github.com/knadh/koanf/maps" "github.com/knadh/koanf/providers/confmap" "github.com/knadh/koanf/v2" + "go.uber.org/multierr" encoder "go.opentelemetry.io/collector/confmap/internal/mapstructure" ) @@ -77,6 +78,77 @@ func (fn unmarshalOptionFunc) apply(set *unmarshalOption) { fn(set) } +// ConfigValidator defines an optional interface for configurations to implement to do validation. +type ConfigValidator interface { + // Validate the configuration and returns an error if invalid. + Validate() error +} + +// As interface types are only used for static typing, a common idiom to find the reflection Type +// for an interface type Foo is to use a *Foo value. +var configValidatorType = reflect.TypeOf((*ConfigValidator)(nil)).Elem() + +func validate(v reflect.Value) error { + // Validate the value itself. + k := v.Kind() + switch k { + case reflect.Invalid: + return nil + case reflect.Ptr, reflect.Interface: + return validate(v.Elem()) + case reflect.Struct: + var errs error + errs = multierr.Append(errs, callValidateIfPossible(v)) + // Reflect on the pointed data and check each of its fields. + for i := 0; i < v.NumField(); i++ { + if !v.Type().Field(i).IsExported() { + continue + } + errs = multierr.Append(errs, validate(v.Field(i))) + } + return errs + case reflect.Slice, reflect.Array: + var errs error + errs = multierr.Append(errs, callValidateIfPossible(v)) + // Reflect on the pointed data and check each of its fields. + for i := 0; i < v.Len(); i++ { + errs = multierr.Append(errs, validate(v.Index(i))) + } + return errs + case reflect.Map: + var errs error + errs = multierr.Append(errs, callValidateIfPossible(v)) + iter := v.MapRange() + for iter.Next() { + errs = multierr.Append(errs, validate(iter.Key())) + errs = multierr.Append(errs, validate(iter.Value())) + } + return errs + default: + return callValidateIfPossible(v) + } +} + +func callValidateIfPossible(v reflect.Value) error { + // If the value type implements ConfigValidator just call Validate + if v.Type().Implements(configValidatorType) { + return v.Interface().(ConfigValidator).Validate() + } + + // If the pointer type implements ConfigValidator call Validate on the pointer to the current value. + if reflect.PointerTo(v.Type()).Implements(configValidatorType) { + // If not addressable, then create a new *V pointer and set the value to current v. + if !v.CanAddr() { + pv := reflect.New(reflect.PointerTo(v.Type()).Elem()) + pv.Elem().Set(v) + v = pv.Elem() + } + return v.Addr().Interface().(ConfigValidator).Validate() + } + + return nil +} + // Unmarshal unmarshalls the config into a struct using the given options. // Tags on the fields of the structure must be properly set. func (l *Conf) Unmarshal(result any, opts ...UnmarshalOption) error { @@ -84,7 +156,11 @@ func (l *Conf) Unmarshal(result any, opts ...UnmarshalOption) error { for _, opt := range opts { opt.apply(&set) } - return decodeConfig(l, result, !set.ignoreUnused, l.skipTopLevelUnmarshaler) + err := decodeConfig(l, result, !set.ignoreUnused, l.skipTopLevelUnmarshaler) + if err != nil { + return err + } + return validate(reflect.ValueOf(result)) } type marshalOption struct{} From 79bd5818e21942e879d62e528c676385e21d0529 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 7 Jan 2025 14:43:33 -0700 Subject: [PATCH 2/7] Make validation conditional --- confmap/confmap.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/confmap/confmap.go b/confmap/confmap.go index e4cf67107be..a94b6410d5a 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -60,7 +60,8 @@ type UnmarshalOption interface { } type unmarshalOption struct { - ignoreUnused bool + ignoreUnused bool + invokeValidate bool } // WithIgnoreUnused sets an option to ignore errors if existing @@ -72,6 +73,16 @@ func WithIgnoreUnused() UnmarshalOption { }) } +// WithInvokeValidate sets an option to invoke the Validate method +// of unmarshalled types that implement ConfigValidator. +// When used, config validation with be executed automatically +// as part of unmarshalling. +func WithInvokeValidate() UnmarshalOption { + return unmarshalOptionFunc(func(uo *unmarshalOption) { + uo.invokeValidate = true + }) +} + type unmarshalOptionFunc func(*unmarshalOption) func (fn unmarshalOptionFunc) apply(set *unmarshalOption) { @@ -160,7 +171,11 @@ func (l *Conf) Unmarshal(result any, opts ...UnmarshalOption) error { if err != nil { return err } - return validate(reflect.ValueOf(result)) + + if set.invokeValidate { + return validate(reflect.ValueOf(result)) + } + return nil } type marshalOption struct{} From 9af542ce7baa654784b5458640828de025ddc7b4 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 7 Jan 2025 14:43:39 -0700 Subject: [PATCH 3/7] Add tests --- confmap/confmap_test.go | 105 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) diff --git a/confmap/confmap_test.go b/confmap/confmap_test.go index f7e98f859f4..00f9492474b 100644 --- a/confmap/confmap_test.go +++ b/confmap/confmap_test.go @@ -964,3 +964,108 @@ func TestStringyTypes(t *testing.T) { assert.Equal(t, tt.isStringy, isStringyStructure(to)) } } + +type validateConfig struct { + String string `mapstructure:"string"` + Nested nestedValidateConfig `mapstructure:"nested"` + SliceNested []nestedValidateConfig `mapstructure:"slice"` + MapNested map[string]nestedValidateConfig `mapstructure:"map"` +} + +func (c *validateConfig) Validate() error { + if c.String == "error" { + return errors.New("error in validateConfig because String") + } + return nil +} + +type nestedValidateConfig struct { + String string `mapstructure:"string"` +} + +func (c *nestedValidateConfig) Validate() error { + if c.String == "error" { + return errors.New("error in nestedValidateConfig because String") + } + return nil +} + +func TestUnmarshalInvokesValidate(t *testing.T) { + tests := []struct { + name string + input map[string]any + err error + }{ + { + name: "Validation passes", + input: map[string]any{ + "string": "value", + "nested": nestedValidateConfig{ + String: "value", + }, + "slice": []nestedValidateConfig{ + { + String: "value", + }, + }, + "map": map[string]nestedValidateConfig{ + "key": { + String: "value", + }, + }, + }, + err: nil, + }, + { + name: "Validation fails because String", + input: map[string]any{ + "string": "error", + }, + err: errors.New("error in validateConfig because String"), + }, + { + name: "Validation fails because nested String", + input: map[string]any{ + "nested": nestedValidateConfig{ + String: "error", + }, + }, + err: errors.New("error in nestedValidateConfig because String"), + }, + { + name: "Validation fails because nested slice String", + input: map[string]any{ + "slice": []nestedValidateConfig{ + { + String: "error", + }, + }, + }, + err: errors.New("error in nestedValidateConfig because String"), + }, + { + name: "Validation fails because nested map String", + input: map[string]any{ + "map": map[string]nestedValidateConfig{ + "key": { + String: "error", + }, + }, + }, + err: errors.New("error in nestedValidateConfig because String"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + conf := NewFromStringMap(tt.input) + c := &validateConfig{} + err := conf.Unmarshal(c, WithInvokeValidate()) + if tt.err == nil { + require.NoError(t, err) + } else { + require.Equal(t, tt.err, err) + } + }) + } +} From 189837a0da6a9d7e5486ceb30d4ecada3855f31e Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Tue, 7 Jan 2025 14:45:14 -0700 Subject: [PATCH 4/7] changelog --- .chloggen/tyler.confmap-configvalidator.yaml | 25 ++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .chloggen/tyler.confmap-configvalidator.yaml diff --git a/.chloggen/tyler.confmap-configvalidator.yaml b/.chloggen/tyler.confmap-configvalidator.yaml new file mode 100644 index 00000000000..0a6d57f6987 --- /dev/null +++ b/.chloggen/tyler.confmap-configvalidator.yaml @@ -0,0 +1,25 @@ +# Use this changelog template to create an entry for release notes. + +# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' +change_type: enhancement + +# The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) +component: confmap + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Add ability for confmap to invoke `ConfigValidator.Validate` as part of `conf.Unmarshal`. + +# One or more tracking issues or pull requests related to the change +issues: [12031] + +# (Optional) One or more lines of additional information to render under the primary note. +# These lines will be padded with 2 spaces and then inserted directly into the document. +# Use pipe (|) for multiline entries. +subtext: + +# Optional: The change log or logs in which this entry should be included. +# e.g. '[user]' or '[user, api]' +# Include 'user' if the change is relevant to end users. +# Include 'api' if there is a change to a library API. +# Default: '[user]' +change_logs: [api] From aec40a0bc0bd836280a714005a40c0ca3fc4b3c2 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Wed, 8 Jan 2025 12:43:38 -0700 Subject: [PATCH 5/7] Add test for custom unmarshal --- confmap/confmap_test.go | 73 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/confmap/confmap_test.go b/confmap/confmap_test.go index 00f9492474b..77e8629e1c2 100644 --- a/confmap/confmap_test.go +++ b/confmap/confmap_test.go @@ -1069,3 +1069,76 @@ func TestUnmarshalInvokesValidate(t *testing.T) { }) } } + +type topConfig struct { + CustomUnmarshalConfig customUnmarshalConfig `mapstructure:"custom"` +} + +type customUnmarshalConfig struct { + String string `mapstructure:"string"` + Nested nestedValidateConfig `mapstructure:"nested"` +} + +// Unmarshal a confmap.Conf into the config struct. +func (c *customUnmarshalConfig) Unmarshal(conf *Conf) error { + err := conf.Unmarshal(c) + if err != nil { + return err + } + if c.String == "" { + c.String = "some fancy value" + } + return nil +} + +func TestCustomUnmarshalInvokesValidate(t *testing.T) { + input := map[string]any{ + "custom": map[string]any{ + "nested": map[string]any{ + "string": "error", + }, + }, + } + conf := NewFromStringMap(input) + c := &topConfig{} + err := conf.Unmarshal(c, WithInvokeValidate()) + require.Equal(t, errors.New("error in nestedValidateConfig because String"), err) + + input = map[string]any{ + "custom": map[string]any{ + "nested": map[string]any{ + "string": "value", + }, + }, + } + conf = NewFromStringMap(input) + c = &topConfig{} + err = conf.Unmarshal(c, WithInvokeValidate()) + require.NoError(t, err) + assert.Equal(t, "some fancy value", c.CustomUnmarshalConfig.String) + assert.Equal(t, "value", c.CustomUnmarshalConfig.Nested.String) +} + +type manualUnmarshalConfig struct { + Nested nestedValidateConfig `mapstructure:"nested"` +} + +// Unmarshal a confmap.Conf into the config struct. +func (c *manualUnmarshalConfig) Unmarshal(_ *Conf) error { + c.Nested = nestedValidateConfig{ + String: "error", + } + return nil +} + +func TestValidateIsInvokedWhenManuallyUnmarshalling(t *testing.T) { + input := map[string]any{ + "nested": map[string]any{ + "string": "anything", + }, + } + conf := NewFromStringMap(input) + c := &manualUnmarshalConfig{} + err := conf.Unmarshal(c, WithInvokeValidate()) + require.Equal(t, errors.New("error in nestedValidateConfig because String"), err) +} From 2ff2f85c9b4a78929bc9c437e91b95178f4f8eb0 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Wed, 8 Jan 2025 12:43:45 -0700 Subject: [PATCH 6/7] Rename to Validator --- confmap/confmap.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/confmap/confmap.go b/confmap/confmap.go index a94b6410d5a..024321b9bce 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -74,7 +74,7 @@ func WithIgnoreUnused() UnmarshalOption { } // WithInvokeValidate sets an option to invoke the Validate method -// of unmarshalled types that implement ConfigValidator. +// of unmarshalled types that implement Validator. // When used, config validation with be executed automatically // as part of unmarshalling. func WithInvokeValidate() UnmarshalOption { @@ -89,15 +89,15 @@ func (fn unmarshalOptionFunc) apply(set *unmarshalOption) { fn(set) } -// ConfigValidator defines an optional interface for configurations to implement to do validation. -type ConfigValidator interface { +// Validator defines an optional interface for configurations to implement to do validation. +type Validator interface { // Validate the configuration and returns an error if invalid. Validate() error } // As interface types are only used for static typing, a common idiom to find the reflection Type // for an interface type Foo is to use a *Foo value. -var configValidatorType = reflect.TypeOf((*ConfigValidator)(nil)).Elem() +var configValidatorType = reflect.TypeOf((*Validator)(nil)).Elem() func validate(v reflect.Value) error { // Validate the value itself. @@ -141,12 +141,12 @@ func validate(v reflect.Value) error { } func callValidateIfPossible(v reflect.Value) error { - // If the value type implements ConfigValidator just call Validate + // If the value type implements Validator just call Validate if v.Type().Implements(configValidatorType) { - return v.Interface().(ConfigValidator).Validate() + return v.Interface().(Validator).Validate() } - // If the pointer type implements ConfigValidator call Validate on the pointer to the current value. + // If the pointer type implements Validator call Validate on the pointer to the current value. if reflect.PointerTo(v.Type()).Implements(configValidatorType) { // If not addressable, then create a new *V pointer and set the value to current v. if !v.CanAddr() { @@ -154,7 +154,7 @@ func callValidateIfPossible(v reflect.Value) error { pv.Elem().Set(v) v = pv.Elem() } - return v.Addr().Interface().(ConfigValidator).Validate() + return v.Addr().Interface().(Validator).Validate() } return nil From ae9859866874e4a4eabd47be86782dec9dee19e4 Mon Sep 17 00:00:00 2001 From: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com> Date: Wed, 8 Jan 2025 13:17:05 -0700 Subject: [PATCH 7/7] Update otelcol to use confmap validator --- confmap/confmap.go | 44 +- otelcol/collector.go | 9 +- otelcol/config.go | 120 ----- otelcol/config_test.go | 421 +++++++++--------- otelcol/internal/configunmarshaler/configs.go | 4 +- otelcol/unmarshaler.go | 88 +++- 6 files changed, 329 insertions(+), 357 deletions(-) diff --git a/confmap/confmap.go b/confmap/confmap.go index 024321b9bce..bb17ae5983b 100644 --- a/confmap/confmap.go +++ b/confmap/confmap.go @@ -15,8 +15,6 @@ import ( "github.com/knadh/koanf/maps" "github.com/knadh/koanf/providers/confmap" "github.com/knadh/koanf/v2" - "go.uber.org/multierr" - encoder "go.opentelemetry.io/collector/confmap/internal/mapstructure" ) @@ -108,36 +106,52 @@ func validate(v reflect.Value) error { case reflect.Ptr, reflect.Interface: return validate(v.Elem()) case reflect.Struct: - var errs error - errs = multierr.Append(errs, callValidateIfPossible(v)) + err := callValidateIfPossible(v) + if err != nil { + return err + } // Reflect on the pointed data and check each of its fields. for i := 0; i < v.NumField(); i++ { if !v.Type().Field(i).IsExported() { continue } - errs = multierr.Append(errs, validate(v.Field(i))) + err = validate(v.Field(i)) + if err != nil { + return err + } } - return errs case reflect.Slice, reflect.Array: - var errs error - errs = multierr.Append(errs, callValidateIfPossible(v)) + err := callValidateIfPossible(v) + if err != nil { + return err + } // Reflect on the pointed data and check each of its fields. for i := 0; i < v.Len(); i++ { - errs = multierr.Append(errs, validate(v.Index(i))) + err = validate(v.Index(i)) + if err != nil { + return err + } } - return errs case reflect.Map: - var errs error - errs = multierr.Append(errs, callValidateIfPossible(v)) + err := callValidateIfPossible(v) + if err != nil { + return err + } iter := v.MapRange() for iter.Next() { - errs = multierr.Append(errs, validate(iter.Key())) - errs = multierr.Append(errs, validate(iter.Value())) + err = validate(iter.Key()) + if err != nil { + return err + } + err = validate(iter.Value()) + if err != nil { + return err + } } - return errs default: return callValidateIfPossible(v) } + return nil } func callValidateIfPossible(v reflect.Value) error { diff --git a/otelcol/collector.go b/otelcol/collector.go index dac8c860ec3..5bd4fa92a2f 100644 --- a/otelcol/collector.go +++ b/otelcol/collector.go @@ -168,10 +168,6 @@ func (col *Collector) setupConfigurationComponents(ctx context.Context) error { return fmt.Errorf("failed to get config: %w", err) } - if err = cfg.Validate(); err != nil { - return fmt.Errorf("invalid configuration: %w", err) - } - col.serviceConfig = &cfg.Service conf := confmap.New() @@ -253,12 +249,11 @@ func (col *Collector) DryRun(ctx context.Context) error { if err != nil { return fmt.Errorf("failed to initialize factories: %w", err) } - cfg, err := col.configProvider.Get(ctx, factories) + _, err = col.configProvider.Get(ctx, factories) if err != nil { return fmt.Errorf("failed to get config: %w", err) } - - return cfg.Validate() + return nil } func newFallbackLogger(options []zap.Option) (*zap.Logger, error) { diff --git a/otelcol/config.go b/otelcol/config.go index 33975957282..8502a6e7695 100644 --- a/otelcol/config.go +++ b/otelcol/config.go @@ -5,7 +5,6 @@ package otelcol // import "go.opentelemetry.io/collector/otelcol" import ( "errors" - "fmt" "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/service" @@ -36,122 +35,3 @@ type Config struct { Service service.Config `mapstructure:"service"` } - -// Validate returns an error if the config is invalid. -// -// This function performs basic validation of configuration. There may be more subtle -// invalid cases that we currently don't check for but which we may want to add in -// the future (e.g. disallowing receiving and exporting on the same endpoint). -func (cfg *Config) Validate() error { - // There must be at least one property set in the configuration file. - if len(cfg.Receivers) == 0 && len(cfg.Exporters) == 0 && len(cfg.Processors) == 0 && len(cfg.Connectors) == 0 && len(cfg.Extensions) == 0 { - return errEmptyConfigurationFile - } - - // Currently, there is no default receiver enabled. - // The configuration must specify at least one receiver to be valid. - if len(cfg.Receivers) == 0 { - return errMissingReceivers - } - - // Validate the receiver configuration. - for recvID, recvCfg := range cfg.Receivers { - if err := component.ValidateConfig(recvCfg); err != nil { - return fmt.Errorf("receivers::%s: %w", recvID, err) - } - } - - // Currently, there is no default exporter enabled. - // The configuration must specify at least one exporter to be valid. - if len(cfg.Exporters) == 0 { - return errMissingExporters - } - - // Validate the exporter configuration. - for expID, expCfg := range cfg.Exporters { - if err := component.ValidateConfig(expCfg); err != nil { - return fmt.Errorf("exporters::%s: %w", expID, err) - } - } - - // Validate the processor configuration. - for procID, procCfg := range cfg.Processors { - if err := component.ValidateConfig(procCfg); err != nil { - return fmt.Errorf("processors::%s: %w", procID, err) - } - } - - // Validate the connector configuration. - for connID, connCfg := range cfg.Connectors { - if err := component.ValidateConfig(connCfg); err != nil { - return fmt.Errorf("connectors::%s: %w", connID, err) - } - - if _, ok := cfg.Exporters[connID]; ok { - return fmt.Errorf("connectors::%s: ambiguous ID: Found both %q exporter and %q connector. "+ - "Change one of the components' IDs to eliminate ambiguity (e.g. rename %q connector to %q)", - connID, connID, connID, connID, connID.String()+"/connector") - } - if _, ok := cfg.Receivers[connID]; ok { - return fmt.Errorf("connectors::%s: ambiguous ID: Found both %q receiver and %q connector. "+ - "Change one of the components' IDs to eliminate ambiguity (e.g. rename %q connector to %q)", - connID, connID, connID, connID, connID.String()+"/connector") - } - } - - // Validate the extension configuration. - for extID, extCfg := range cfg.Extensions { - if err := component.ValidateConfig(extCfg); err != nil { - return fmt.Errorf("extensions::%s: %w", extID, err) - } - } - - if err := cfg.Service.Validate(); err != nil { - return err - } - - // Check that all enabled extensions in the service are configured. - for _, ref := range cfg.Service.Extensions { - // Check that the name referenced in the Service extensions exists in the top-level extensions. - if cfg.Extensions[ref] == nil { - return fmt.Errorf("service::extensions: references extension %q which is not configured", ref) - } - } - - // Check that all pipelines reference only configured components. - for pipelineID, pipeline := range cfg.Service.Pipelines { - // Validate pipeline receiver name references. - for _, ref := range pipeline.Receivers { - // Check that the name referenced in the pipeline's receivers exists in the top-level receivers. - if _, ok := cfg.Receivers[ref]; ok { - continue - } - - if _, ok := cfg.Connectors[ref]; ok { - continue - } - return fmt.Errorf("service::pipelines::%s: references receiver %q which is not configured", pipelineID.String(), ref) - } - - // Validate pipeline processor name references. - for _, ref := range pipeline.Processors { - // Check that the name referenced in the pipeline's processors exists in the top-level processors. - if cfg.Processors[ref] == nil { - return fmt.Errorf("service::pipelines::%s: references processor %q which is not configured", pipelineID.String(), ref) - } - } - - // Validate pipeline exporter name references. - for _, ref := range pipeline.Exporters { - // Check that the name referenced in the pipeline's Exporters exists in the top-level Exporters. - if _, ok := cfg.Exporters[ref]; ok { - continue - } - if _, ok := cfg.Connectors[ref]; ok { - continue - } - return fmt.Errorf("service::pipelines::%s: references exporter %q which is not configured", pipelineID.String(), ref) - } - } - return nil -} diff --git a/otelcol/config_test.go b/otelcol/config_test.go index adcf0eca4c1..559885c7642 100644 --- a/otelcol/config_test.go +++ b/otelcol/config_test.go @@ -5,10 +5,7 @@ package otelcol import ( "errors" - "fmt" - "testing" - "github.com/stretchr/testify/assert" "go.opentelemetry.io/contrib/config" "go.uber.org/zap/zapcore" @@ -36,215 +33,215 @@ func (c *errConfig) Validate() error { return c.validateErr } -func TestConfigValidate(t *testing.T) { - testCases := []struct { - name string // test case name (also file name containing config yaml) - cfgFn func() *Config - expected error - }{ - { - name: "valid", - cfgFn: generateConfig, - expected: nil, - }, - { - name: "custom-service-telemetrySettings-encoding", - cfgFn: func() *Config { - cfg := generateConfig() - cfg.Service.Telemetry.Logs.Encoding = "json" - return cfg - }, - expected: nil, - }, - { - name: "empty configuration file", - cfgFn: func() *Config { - cfg := generateConfig() - cfg.Receivers = nil - cfg.Connectors = nil - cfg.Processors = nil - cfg.Exporters = nil - cfg.Extensions = nil - return cfg - }, - expected: errEmptyConfigurationFile, - }, - { - name: "missing-exporters", - cfgFn: func() *Config { - cfg := generateConfig() - cfg.Exporters = nil - return cfg - }, - expected: errMissingExporters, - }, - { - name: "missing-receivers", - cfgFn: func() *Config { - cfg := generateConfig() - cfg.Receivers = nil - return cfg - }, - expected: errMissingReceivers, - }, - { - name: "invalid-extension-reference", - cfgFn: func() *Config { - cfg := generateConfig() - cfg.Service.Extensions = append(cfg.Service.Extensions, component.MustNewIDWithName("nop", "2")) - return cfg - }, - expected: errors.New(`service::extensions: references extension "nop/2" which is not configured`), - }, - { - name: "invalid-receiver-reference", - cfgFn: func() *Config { - cfg := generateConfig() - pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] - pipe.Receivers = append(pipe.Receivers, component.MustNewIDWithName("nop", "2")) - return cfg - }, - expected: errors.New(`service::pipelines::traces: references receiver "nop/2" which is not configured`), - }, - { - name: "invalid-processor-reference", - cfgFn: func() *Config { - cfg := generateConfig() - pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] - pipe.Processors = append(pipe.Processors, component.MustNewIDWithName("nop", "2")) - return cfg - }, - expected: errors.New(`service::pipelines::traces: references processor "nop/2" which is not configured`), - }, - { - name: "invalid-exporter-reference", - cfgFn: func() *Config { - cfg := generateConfig() - pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] - pipe.Exporters = append(pipe.Exporters, component.MustNewIDWithName("nop", "2")) - return cfg - }, - expected: errors.New(`service::pipelines::traces: references exporter "nop/2" which is not configured`), - }, - { - name: "invalid-receiver-config", - cfgFn: func() *Config { - cfg := generateConfig() - cfg.Receivers[component.MustNewID("nop")] = &errConfig{ - validateErr: errInvalidRecvConfig, - } - return cfg - }, - expected: fmt.Errorf(`receivers::nop: %w`, errInvalidRecvConfig), - }, - { - name: "invalid-exporter-config", - cfgFn: func() *Config { - cfg := generateConfig() - cfg.Exporters[component.MustNewID("nop")] = &errConfig{ - validateErr: errInvalidExpConfig, - } - return cfg - }, - expected: fmt.Errorf(`exporters::nop: %w`, errInvalidExpConfig), - }, - { - name: "invalid-processor-config", - cfgFn: func() *Config { - cfg := generateConfig() - cfg.Processors[component.MustNewID("nop")] = &errConfig{ - validateErr: errInvalidProcConfig, - } - return cfg - }, - expected: fmt.Errorf(`processors::nop: %w`, errInvalidProcConfig), - }, - { - name: "invalid-extension-config", - cfgFn: func() *Config { - cfg := generateConfig() - cfg.Extensions[component.MustNewID("nop")] = &errConfig{ - validateErr: errInvalidExtConfig, - } - return cfg - }, - expected: fmt.Errorf(`extensions::nop: %w`, errInvalidExtConfig), - }, - { - name: "invalid-connector-config", - cfgFn: func() *Config { - cfg := generateConfig() - cfg.Connectors[component.MustNewIDWithName("nop", "conn")] = &errConfig{ - validateErr: errInvalidConnConfig, - } - return cfg - }, - expected: fmt.Errorf(`connectors::nop/conn: %w`, errInvalidConnConfig), - }, - { - name: "ambiguous-connector-name-as-receiver", - cfgFn: func() *Config { - cfg := generateConfig() - cfg.Receivers[component.MustNewID("nop2")] = &errConfig{} - cfg.Connectors[component.MustNewID("nop2")] = &errConfig{} - pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] - pipe.Receivers = append(pipe.Receivers, component.MustNewIDWithName("nop", "2")) - pipe.Exporters = append(pipe.Exporters, component.MustNewIDWithName("nop", "2")) - return cfg - }, - expected: errors.New(`connectors::nop2: ambiguous ID: Found both "nop2" receiver and "nop2" connector. Change one of the components' IDs to eliminate ambiguity (e.g. rename "nop2" connector to "nop2/connector")`), - }, - { - name: "ambiguous-connector-name-as-exporter", - cfgFn: func() *Config { - cfg := generateConfig() - cfg.Exporters[component.MustNewID("nop2")] = &errConfig{} - cfg.Connectors[component.MustNewID("nop2")] = &errConfig{} - pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] - pipe.Receivers = append(pipe.Receivers, component.MustNewIDWithName("nop", "2")) - pipe.Exporters = append(pipe.Exporters, component.MustNewIDWithName("nop", "2")) - return cfg - }, - expected: errors.New(`connectors::nop2: ambiguous ID: Found both "nop2" exporter and "nop2" connector. Change one of the components' IDs to eliminate ambiguity (e.g. rename "nop2" connector to "nop2/connector")`), - }, - { - name: "invalid-connector-reference-as-receiver", - cfgFn: func() *Config { - cfg := generateConfig() - pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] - pipe.Receivers = append(pipe.Receivers, component.MustNewIDWithName("nop", "conn2")) - return cfg - }, - expected: errors.New(`service::pipelines::traces: references receiver "nop/conn2" which is not configured`), - }, - { - name: "invalid-connector-reference-as-receiver", - cfgFn: func() *Config { - cfg := generateConfig() - pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] - pipe.Exporters = append(pipe.Exporters, component.MustNewIDWithName("nop", "conn2")) - return cfg - }, - expected: errors.New(`service::pipelines::traces: references exporter "nop/conn2" which is not configured`), - }, - { - name: "invalid-service-config", - cfgFn: func() *Config { - cfg := generateConfig() - cfg.Service.Pipelines = nil - return cfg - }, - expected: fmt.Errorf(`service::pipelines config validation failed: %w`, errors.New(`service must have at least one pipeline`)), - }, - } - - for _, tt := range testCases { - t.Run(tt.name, func(t *testing.T) { - cfg := tt.cfgFn() - assert.Equal(t, tt.expected, cfg.Validate()) - }) - } -} +//func TestConfigValidate(t *testing.T) { +// testCases := []struct { +// name string // test case name (also file name containing config yaml) +// cfgFn func() *Config +// expected error +// }{ +// { +// name: "valid", +// cfgFn: generateConfig, +// expected: nil, +// }, +// { +// name: "custom-service-telemetrySettings-encoding", +// cfgFn: func() *Config { +// cfg := generateConfig() +// cfg.Service.Telemetry.Logs.Encoding = "json" +// return cfg +// }, +// expected: nil, +// }, +// { +// name: "empty configuration file", +// cfgFn: func() *Config { +// cfg := generateConfig() +// cfg.Receivers = nil +// cfg.Connectors = nil +// cfg.Processors = nil +// cfg.Exporters = nil +// cfg.Extensions = nil +// return cfg +// }, +// expected: errEmptyConfigurationFile, +// }, +// { +// name: "missing-exporters", +// cfgFn: func() *Config { +// cfg := generateConfig() +// cfg.Exporters = nil +// return cfg +// }, +// expected: errMissingExporters, +// }, +// { +// name: "missing-receivers", +// cfgFn: func() *Config { +// cfg := generateConfig() +// cfg.Receivers = nil +// return cfg +// }, +// expected: errMissingReceivers, +// }, +// { +// name: "invalid-extension-reference", +// cfgFn: func() *Config { +// cfg := generateConfig() +// cfg.Service.Extensions = append(cfg.Service.Extensions, component.MustNewIDWithName("nop", "2")) +// return cfg +// }, +// expected: errors.New(`service::extensions: references extension "nop/2" which is not configured`), +// }, +// { +// name: "invalid-receiver-reference", +// cfgFn: func() *Config { +// cfg := generateConfig() +// pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] +// pipe.Receivers = append(pipe.Receivers, component.MustNewIDWithName("nop", "2")) +// return cfg +// }, +// expected: errors.New(`service::pipelines::traces: references receiver "nop/2" which is not configured`), +// }, +// { +// name: "invalid-processor-reference", +// cfgFn: func() *Config { +// cfg := generateConfig() +// pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] +// pipe.Processors = append(pipe.Processors, component.MustNewIDWithName("nop", "2")) +// return cfg +// }, +// expected: errors.New(`service::pipelines::traces: references processor "nop/2" which is not configured`), +// }, +// { +// name: "invalid-exporter-reference", +// cfgFn: func() *Config { +// cfg := generateConfig() +// pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] +// pipe.Exporters = append(pipe.Exporters, component.MustNewIDWithName("nop", "2")) +// return cfg +// }, +// expected: errors.New(`service::pipelines::traces: references exporter "nop/2" which is not configured`), +// }, +// { +// name: "invalid-receiver-config", +// cfgFn: func() *Config { +// cfg := generateConfig() +// cfg.Receivers[component.MustNewID("nop")] = &errConfig{ +// validateErr: errInvalidRecvConfig, +// } +// return cfg +// }, +// expected: fmt.Errorf(`receivers::nop: %w`, errInvalidRecvConfig), +// }, +// { +// name: "invalid-exporter-config", +// cfgFn: func() *Config { +// cfg := generateConfig() +// cfg.Exporters[component.MustNewID("nop")] = &errConfig{ +// validateErr: errInvalidExpConfig, +// } +// return cfg +// }, +// expected: fmt.Errorf(`exporters::nop: %w`, errInvalidExpConfig), +// }, +// { +// name: "invalid-processor-config", +// cfgFn: func() *Config { +// cfg := generateConfig() +// cfg.Processors[component.MustNewID("nop")] = &errConfig{ +// validateErr: errInvalidProcConfig, +// } +// return cfg +// }, +// expected: fmt.Errorf(`processors::nop: %w`, errInvalidProcConfig), +// }, +// { +// name: "invalid-extension-config", +// cfgFn: func() *Config { +// cfg := generateConfig() +// cfg.Extensions[component.MustNewID("nop")] = &errConfig{ +// validateErr: errInvalidExtConfig, +// } +// return cfg +// }, +// expected: fmt.Errorf(`extensions::nop: %w`, errInvalidExtConfig), +// }, +// { +// name: "invalid-connector-config", +// cfgFn: func() *Config { +// cfg := generateConfig() +// cfg.Connectors[component.MustNewIDWithName("nop", "conn")] = &errConfig{ +// validateErr: errInvalidConnConfig, +// } +// return cfg +// }, +// expected: fmt.Errorf(`connectors::nop/conn: %w`, errInvalidConnConfig), +// }, +// { +// name: "ambiguous-connector-name-as-receiver", +// cfgFn: func() *Config { +// cfg := generateConfig() +// cfg.Receivers[component.MustNewID("nop2")] = &errConfig{} +// cfg.Connectors[component.MustNewID("nop2")] = &errConfig{} +// pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] +// pipe.Receivers = append(pipe.Receivers, component.MustNewIDWithName("nop", "2")) +// pipe.Exporters = append(pipe.Exporters, component.MustNewIDWithName("nop", "2")) +// return cfg +// }, +// expected: errors.New(`connectors::nop2: ambiguous ID: Found both "nop2" receiver and "nop2" connector. Change one of the components' IDs to eliminate ambiguity (e.g. rename "nop2" connector to "nop2/connector")`), +// }, +// { +// name: "ambiguous-connector-name-as-exporter", +// cfgFn: func() *Config { +// cfg := generateConfig() +// cfg.Exporters[component.MustNewID("nop2")] = &errConfig{} +// cfg.Connectors[component.MustNewID("nop2")] = &errConfig{} +// pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] +// pipe.Receivers = append(pipe.Receivers, component.MustNewIDWithName("nop", "2")) +// pipe.Exporters = append(pipe.Exporters, component.MustNewIDWithName("nop", "2")) +// return cfg +// }, +// expected: errors.New(`connectors::nop2: ambiguous ID: Found both "nop2" exporter and "nop2" connector. Change one of the components' IDs to eliminate ambiguity (e.g. rename "nop2" connector to "nop2/connector")`), +// }, +// { +// name: "invalid-connector-reference-as-receiver", +// cfgFn: func() *Config { +// cfg := generateConfig() +// pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] +// pipe.Receivers = append(pipe.Receivers, component.MustNewIDWithName("nop", "conn2")) +// return cfg +// }, +// expected: errors.New(`service::pipelines::traces: references receiver "nop/conn2" which is not configured`), +// }, +// { +// name: "invalid-connector-reference-as-receiver", +// cfgFn: func() *Config { +// cfg := generateConfig() +// pipe := cfg.Service.Pipelines[pipeline.NewID(pipeline.SignalTraces)] +// pipe.Exporters = append(pipe.Exporters, component.MustNewIDWithName("nop", "conn2")) +// return cfg +// }, +// expected: errors.New(`service::pipelines::traces: references exporter "nop/conn2" which is not configured`), +// }, +// { +// name: "invalid-service-config", +// cfgFn: func() *Config { +// cfg := generateConfig() +// cfg.Service.Pipelines = nil +// return cfg +// }, +// expected: fmt.Errorf(`service::pipelines config validation failed: %w`, errors.New(`service must have at least one pipeline`)), +// }, +// } +// +// for _, tt := range testCases { +// t.Run(tt.name, func(t *testing.T) { +// cfg := tt.cfgFn() +// assert.Equal(t, tt.expected, cfg.Validate()) +// }) +// } +//} func generateConfig() *Config { return &Config{ diff --git a/otelcol/internal/configunmarshaler/configs.go b/otelcol/internal/configunmarshaler/configs.go index e60ab5d29b4..8ebceb38d3c 100644 --- a/otelcol/internal/configunmarshaler/configs.go +++ b/otelcol/internal/configunmarshaler/configs.go @@ -25,7 +25,7 @@ func NewConfigs[F component.Factory](factories map[component.Type]F) *Configs[F] func (c *Configs[F]) Unmarshal(conf *confmap.Conf) error { rawCfgs := make(map[component.ID]map[string]any) - if err := conf.Unmarshal(&rawCfgs); err != nil { + if err := conf.Unmarshal(&rawCfgs, confmap.WithInvokeValidate()); err != nil { return err } @@ -50,7 +50,7 @@ func (c *Configs[F]) Unmarshal(conf *confmap.Conf) error { // Now that the default config struct is created we can Unmarshal into it, // and it will apply user-defined config on top of the default. - if err := sub.Unmarshal(&cfg); err != nil { + if err := sub.Unmarshal(&cfg, confmap.WithInvokeValidate()); err != nil { return errorUnmarshalError(id, err) } diff --git a/otelcol/unmarshaler.go b/otelcol/unmarshaler.go index 8f2b8e583a1..985d4fe289f 100644 --- a/otelcol/unmarshaler.go +++ b/otelcol/unmarshaler.go @@ -4,6 +4,8 @@ package otelcol // import "go.opentelemetry.io/collector/otelcol" import ( + "fmt" + "go.opentelemetry.io/collector/confmap" "go.opentelemetry.io/collector/connector" "go.opentelemetry.io/collector/exporter" @@ -24,6 +26,90 @@ type configSettings struct { Service service.Config `mapstructure:"service"` } +func (c *configSettings) Validate() error { + receivers := c.Receivers.Configs() + processors := c.Processors.Configs() + exporters := c.Exporters.Configs() + connectors := c.Connectors.Configs() + extensions := c.Extensions.Configs() + + // There must be at least one property set in the configuration file. + if len(receivers) == 0 && len(exporters) == 0 && len(processors) == 0 && len(connectors) == 0 && len(extensions) == 0 { + return errEmptyConfigurationFile + } + + // Currently, there is no default receiver enabled. + // The configuration must specify at least one receiver to be valid. + if len(receivers) == 0 { + return errMissingReceivers + } + + // Currently, there is no default exporter enabled. + // The configuration must specify at least one exporter to be valid. + if len(exporters) == 0 { + return errMissingExporters + } + + // Validate the connector configuration. + for connID, _ := range connectors { + if _, ok := exporters[connID]; ok { + return fmt.Errorf("connectors::%s: ambiguous ID: Found both %q exporter and %q connector. "+ + "Change one of the components' IDs to eliminate ambiguity (e.g. rename %q connector to %q)", + connID, connID, connID, connID, connID.String()+"/connector") + } + if _, ok := receivers[connID]; ok { + return fmt.Errorf("connectors::%s: ambiguous ID: Found both %q receiver and %q connector. "+ + "Change one of the components' IDs to eliminate ambiguity (e.g. rename %q connector to %q)", + connID, connID, connID, connID, connID.String()+"/connector") + } + } + + // Check that all enabled extensions in the service are configured. + for _, ref := range c.Service.Extensions { + // Check that the name referenced in the Service extensions exists in the top-level extensions. + if extensions[ref] == nil { + return fmt.Errorf("service::extensions: references extension %q which is not configured", ref) + } + } + + // Check that all pipelines reference only configured components. + for pipelineID, pipeline := range c.Service.Pipelines { + // Validate pipeline receiver name references. + for _, ref := range pipeline.Receivers { + // Check that the name referenced in the pipeline's receivers exists in the top-level receivers. + if _, ok := receivers[ref]; ok { + continue + } + + if _, ok := connectors[ref]; ok { + continue + } + return fmt.Errorf("service::pipelines::%s: references receiver %q which is not configured", pipelineID.String(), ref) + } + + // Validate pipeline processor name references. + for _, ref := range pipeline.Processors { + // Check that the name referenced in the pipeline's processors exists in the top-level processors. + if processors[ref] == nil { + return fmt.Errorf("service::pipelines::%s: references processor %q which is not configured", pipelineID.String(), ref) + } + } + + // Validate pipeline exporter name references. + for _, ref := range pipeline.Exporters { + // Check that the name referenced in the pipeline's Exporters exists in the top-level Exporters. + if _, ok := exporters[ref]; ok { + continue + } + if _, ok := connectors[ref]; ok { + continue + } + return fmt.Errorf("service::pipelines::%s: references exporter %q which is not configured", pipelineID.String(), ref) + } + } + return nil +} + // unmarshal the configSettings from a confmap.Conf. // After the config is unmarshalled, `Validate()` must be called to validate. func unmarshal(v *confmap.Conf, factories Factories) (*configSettings, error) { @@ -43,5 +129,5 @@ func unmarshal(v *confmap.Conf, factories Factories) (*configSettings, error) { }, } - return cfg, v.Unmarshal(&cfg) + return cfg, v.Unmarshal(&cfg, confmap.WithInvokeValidate()) }