From 2dfa032fdae0afe68c01b20e1913e4a7a2ee4b3f Mon Sep 17 00:00:00 2001 From: Bogdan Drutu Date: Mon, 21 Nov 2022 18:00:14 -0800 Subject: [PATCH] Add recursive validation check for configs Signed-off-by: Bogdan Drutu --- .chloggen/deepvalidate.yaml | 11 ++ component/config.go | 69 +++++++++++- component/config_test.go | 207 ++++++++++++++++++++++++++++++++++++ 3 files changed, 284 insertions(+), 3 deletions(-) create mode 100755 .chloggen/deepvalidate.yaml create mode 100644 component/config_test.go diff --git a/.chloggen/deepvalidate.yaml b/.chloggen/deepvalidate.yaml new file mode 100755 index 00000000000..0c6ffbc0071 --- /dev/null +++ b/.chloggen/deepvalidate.yaml @@ -0,0 +1,11 @@ +# 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: component + +# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). +note: Add recursive validation check for configs + +# One or more tracking issues or pull requests related to the change +issues: [4584] diff --git a/component/config.go b/component/config.go index 619e9b45857..a7ed0efc637 100644 --- a/component/config.go +++ b/component/config.go @@ -15,6 +15,10 @@ package component // import "go.opentelemetry.io/collector/component" import ( + "reflect" + + "go.uber.org/multierr" + "go.opentelemetry.io/collector/confmap" ) @@ -24,6 +28,10 @@ type Config interface { privateConfig() } +// 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() + // ConfigValidator defines an optional interface for configurations to implement to do validation. type ConfigValidator interface { // Validate the configuration and returns an error if invalid. @@ -33,12 +41,67 @@ type ConfigValidator interface { // ValidateConfig validates a config, by doing this: // - Call Validate on the config itself if the config implements ConfigValidator. func ValidateConfig(cfg Config) error { - validator, ok := cfg.(ConfigValidator) - if !ok { + return validate(reflect.ValueOf(cfg)) +} + +func validate(v reflect.Value) error { + // Validate the value itself. + switch v.Kind() { + case reflect.Invalid: return nil + case reflect.Ptr: + 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.PtrTo(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.PtrTo(v.Type()).Elem()) + pv.Elem().Set(v) + v = pv.Elem() + } + return v.Addr().Interface().(ConfigValidator).Validate() } - return validator.Validate() + return nil } // Type is the component type as it is used in the config. diff --git a/component/config_test.go b/component/config_test.go new file mode 100644 index 00000000000..a0c33ed1922 --- /dev/null +++ b/component/config_test.go @@ -0,0 +1,207 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package component + +import ( + "errors" + "reflect" + "testing" + + "github.com/stretchr/testify/assert" +) + +type configChildStruct struct { + Child errConfig + ChildPtr *errConfig +} + +type configChildSlice struct { + Child []errConfig + ChildPtr []*errConfig +} + +type configChildMapValue struct { + Child map[string]errConfig + ChildPtr map[string]*errConfig +} + +type configChildMapKey struct { + Child map[errType]string + ChildPtr map[*errType]string +} + +type configChildTypeDef struct { + Child errType + ChildPtr *errType +} + +type errConfig struct { + err error +} + +func (e *errConfig) Validate() error { + return e.err +} + +type errType string + +func (e errType) Validate() error { + if e == "" { + return nil + } + return errors.New(string(e)) +} + +func newErrType(etStr string) *errType { + et := errType(etStr) + return &et +} + +type errMapType map[string]string + +func (e errMapType) Validate() error { + return errors.New(e["err"]) +} + +func newErrMapType() *errMapType { + et := errMapType(nil) + return &et +} + +func TestValidateConfig(t *testing.T) { + var tests = []struct { + name string + cfg any + expected error + }{ + { + name: "struct", + cfg: errConfig{err: errors.New("struct")}, + expected: errors.New("struct"), + }, + { + name: "pointer struct", + cfg: &errConfig{err: errors.New("pointer struct")}, + expected: errors.New("pointer struct"), + }, + { + name: "type", + cfg: errType("type"), + expected: errors.New("type"), + }, + { + name: "pointer child", + cfg: newErrType("pointer type"), + expected: errors.New("pointer type"), + }, + { + name: "nil", + cfg: nil, + expected: nil, + }, + { + name: "nil map type", + cfg: errMapType(nil), + expected: errors.New(""), + }, + { + name: "nil pointer map type", + cfg: newErrMapType(), + expected: errors.New(""), + }, + { + name: "child struct", + cfg: configChildStruct{Child: errConfig{err: errors.New("child struct")}}, + expected: errors.New("child struct"), + }, + { + name: "pointer child struct", + cfg: &configChildStruct{Child: errConfig{err: errors.New("pointer child struct")}}, + expected: errors.New("pointer child struct"), + }, + { + name: "child struct pointer", + cfg: &configChildStruct{ChildPtr: &errConfig{err: errors.New("child struct pointer")}}, + expected: errors.New("child struct pointer"), + }, + { + name: "child slice", + cfg: configChildSlice{Child: []errConfig{{}, {err: errors.New("child slice")}}}, + expected: errors.New("child slice"), + }, + { + name: "pointer child slice", + cfg: &configChildSlice{Child: []errConfig{{}, {err: errors.New("pointer child slice")}}}, + expected: errors.New("pointer child slice"), + }, + { + name: "child slice pointer", + cfg: &configChildSlice{ChildPtr: []*errConfig{{}, {err: errors.New("child slice pointer")}}}, + expected: errors.New("child slice pointer"), + }, + { + name: "child map value", + cfg: configChildMapValue{Child: map[string]errConfig{"test": {err: errors.New("child map")}}}, + expected: errors.New("child map"), + }, + { + name: "pointer child map value", + cfg: &configChildMapValue{Child: map[string]errConfig{"test": {err: errors.New("pointer child map")}}}, + expected: errors.New("pointer child map"), + }, + { + name: "child map value pointer", + cfg: &configChildMapValue{ChildPtr: map[string]*errConfig{"test": {err: errors.New("child map pointer")}}}, + expected: errors.New("child map pointer"), + }, + { + name: "child map key", + cfg: configChildMapKey{Child: map[errType]string{"child map key": ""}}, + expected: errors.New("child map key"), + }, + { + name: "pointer child map key", + cfg: &configChildMapKey{Child: map[errType]string{"pointer child map key": ""}}, + expected: errors.New("pointer child map key"), + }, + { + name: "child map key pointer", + cfg: &configChildMapKey{ChildPtr: map[*errType]string{newErrType("child map key pointer"): ""}}, + expected: errors.New("child map key pointer"), + }, + { + name: "child type", + cfg: configChildTypeDef{Child: "child type"}, + expected: errors.New("child type"), + }, + { + name: "pointer child type", + cfg: &configChildTypeDef{Child: "pointer child type"}, + expected: errors.New("pointer child type"), + }, + { + name: "child type pointer", + cfg: &configChildTypeDef{ChildPtr: newErrType("child type pointer")}, + expected: errors.New("child type pointer"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validate(reflect.ValueOf(tt.cfg)) + assert.Equal(t, tt.expected, err) + }) + } +}