From 03c00303b8dddeb6db7c13eed80490521262e7b1 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Tue, 9 Mar 2021 22:07:42 +0000 Subject: [PATCH 01/19] Add configsource component Create the new component with minimal code addition. --- component/configsource.go | 80 +++++++++++++++++++++++++++++ config/configmodels/configsource.go | 22 ++++++++ 2 files changed, 102 insertions(+) create mode 100644 component/configsource.go create mode 100644 config/configmodels/configsource.go diff --git a/component/configsource.go b/component/configsource.go new file mode 100644 index 00000000000..a271b6d1646 --- /dev/null +++ b/component/configsource.go @@ -0,0 +1,80 @@ +// 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 ( + "context" + + "go.uber.org/zap" + + "go.opentelemetry.io/collector/config/configmodels" +) + +// SessionParams is passed to ConfigSource at the begin and end of +// a configuration session. +type SessionParams struct { + // LoadCount tracks the number of the configuration load session + LoadCount int +} + +// ConfigSource is the interface to be implemented by objects used by the collector +// to retrieve external configuration information. +type ConfigSource interface { + Component + + // BeginSession signals that the object is about to be used to inject data into + // the configuration. ConfigSource objects can assume that there won't be + // concurrent sessions and can use this call according to their needs: + // lock needed resources, suspend background tasks, etc. + BeginSession(ctx context.Context, sessionParams SessionParams) error + + // Apply retrieves generic values given the arguments specified on a specific + // invocation of a configuration source. The returned object is injected on + // configuration. + Apply(ctx context.Context, params interface{}) (interface{}, error) + + // EndSession signals that the configuration was fully flattened and it + // is ready to be loaded. Each ConfigSource can use this call according + // to their needs: release resources, start background tasks, update + // internal state, etc. + EndSession(ctx context.Context, sessionParams SessionParams) error +} + +// ConfigSourceCreateParams is passed to ConfigSourceFactory.Create* functions. +type ConfigSourceCreateParams struct { + // Logger that the factory can use during creation and can pass to the created + // component to be used later as well. + Logger *zap.Logger + + // ApplicationStartInfo can be used to retrieve data according to version, etc. + ApplicationStartInfo ApplicationStartInfo +} + +// ConfigSourceFactory is a factory interface for configuration sources. +type ConfigSourceFactory interface { + Factory + + // CreateDefaultConfig creates the default configuration for the ConfigSource. + // This method can be called multiple times depending on the pipeline + // configuration and should not cause side-effects that prevent the creation + // of multiple instances of the ConfigSource. + // The object returned by this method needs to pass the checks implemented by + // 'configcheck.ValidateConfig'. It is recommended to have such check in the + // tests of any implementation of the Factory interface. + CreateDefaultConfig() configmodels.ConfigSource + + // CreateConfigSource creates a configuration source based on the given config. + CreateConfigSource(ctx context.Context, params ConfigSourceCreateParams, cfg configmodels.ConfigSource) (ConfigSource, error) +} diff --git a/config/configmodels/configsource.go b/config/configmodels/configsource.go new file mode 100644 index 00000000000..4655fcd23b3 --- /dev/null +++ b/config/configmodels/configsource.go @@ -0,0 +1,22 @@ +// 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 configmodels + +// ConfigSource is the configuration of a config source. Specific configuration +// sources must implement this interface and will typically embed ConfigSourceSettings +// or a struct that extends it. +type ConfigSource interface { + NamedEntity +} From 68ceb6426a0386481262515b9b2eacc3e3004270 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Mon, 15 Mar 2021 09:31:37 -0700 Subject: [PATCH 02/19] Add ApplyConfigSources to a configuration --- component/configsource.go | 4 +- config/configsource.go | 177 +++++++++++++++++ config/configsource_test.go | 381 ++++++++++++++++++++++++++++++++++++ 3 files changed, 560 insertions(+), 2 deletions(-) create mode 100644 config/configsource.go create mode 100644 config/configsource_test.go diff --git a/component/configsource.go b/component/configsource.go index a271b6d1646..3db2e4f8ffd 100644 --- a/component/configsource.go +++ b/component/configsource.go @@ -25,7 +25,7 @@ import ( // SessionParams is passed to ConfigSource at the begin and end of // a configuration session. type SessionParams struct { - // LoadCount tracks the number of the configuration load session + // LoadCount tracks the number of the configuration load session. LoadCount int } @@ -49,7 +49,7 @@ type ConfigSource interface { // is ready to be loaded. Each ConfigSource can use this call according // to their needs: release resources, start background tasks, update // internal state, etc. - EndSession(ctx context.Context, sessionParams SessionParams) error + EndSession(ctx context.Context, sessionParams SessionParams) } // ConfigSourceCreateParams is passed to ConfigSourceFactory.Create* functions. diff --git a/config/configsource.go b/config/configsource.go new file mode 100644 index 00000000000..2e61c23990d --- /dev/null +++ b/config/configsource.go @@ -0,0 +1,177 @@ +// 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 config + +import ( + "context" + "errors" + "github.com/spf13/viper" + "go.opentelemetry.io/collector/component" + "sort" + "strings" +) + +const ( + // ConfigSourcePrefix is used to identified a configuration source invocation. + ConfigSourcePrefix = "$" +) + +// ApplyConfigSourcesParams holds the parameters for injecting data from the +// given configuration sources into a configuration. +type ApplyConfigSourcesParams struct { + // Set of configuration sources available to inject data into the configuration. + ConfigSources map[string]component.ConfigSource + // MaxRecursionDepth limits the maximum number of times that a configuration source + // can inject another into the config. + MaxRecursionDepth uint + // LoadCount tracks the number of the configuration load session. + LoadCount int +} + +// ApplyConfigSources takes a viper object with the configuration to which the +// the configuration sources will be applied and the resulting configuration is +// returned as a separated object. +func ApplyConfigSources(ctx context.Context, v *viper.Viper, params ApplyConfigSourcesParams) (*viper.Viper, error) { + + sessionParams := component.SessionParams{ + LoadCount: params.LoadCount, + } + + // Notify all sources that we are about to start. + for _, cfgSrc := range params.ConfigSources { + if err := cfgSrc.BeginSession(ctx, sessionParams); err != nil { + return nil, err // TODO: wrap error with more information. + } + defer cfgSrc.EndSession(ctx, sessionParams) + } + + srcCfg := v + var dstCfg *viper.Viper + + var err error + done := false + for i := uint(0); i <= params.MaxRecursionDepth && !done; i++ { + dstCfg = NewViper() + done, err = applyConfigSources(ctx, srcCfg, dstCfg, params.ConfigSources) + if err != nil { + return nil, err + } + srcCfg = dstCfg + } + + if !done { + return nil, errors.New("couldn't fully expand the configuration") + } + + return dstCfg, nil +} + +func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSources map[string]component.ConfigSource) (bool, error) { + // Expand any item env vars in the config, do it every time so env vars + // added on previous pass are also handled. + expandEnvConfig(srcCfg) + + done := true + appliedTags := make(map[string]struct{}) + + // Apply tags from the deepest config sources to the lowest so nested invocations are properly resolved. + allKeys := srcCfg.AllKeys() + sort.Slice(allKeys, deepestConfigSourcesFirst(allKeys)) + + for _, k := range allKeys { + dstKey, cfgSrcName, paramsKey := extractCfgSrcInvocation(k) + if cfgSrcName == "" { + // Nothing to apply take the key and value as it is. + dstCfg.Set(k, srcCfg.Get(k)) + continue + } + + if _, ok := appliedTags[dstKey]; ok { + // Already applied this key. + continue + } + + appliedTags[dstKey] = struct{}{} + cfgSrc, ok := cfgSources[cfgSrcName] + if !ok { + return false, errors.New("unknown config source") // TODO: error similar to other config errors. + } + + applyParams := srcCfg.Get(paramsKey) + actualCfg, err := cfgSrc.Apply(ctx, applyParams) + if err != nil { + return false, err // TODO: Add config source name to the error. + } + + // The injection may require further expansion, assume that we are not done yet. + done = false + if dstKey != "" { + dstCfg.Set(dstKey, actualCfg) + continue + } + + // This is at the root level, have to inject the top keys one by one. + rootMap, ok := actualCfg.(map[string]interface{}) + if !ok { + return false, errors.New("only a map can be injected at the root level") // TODO: better error info. + } + for k, v := range rootMap { + dstCfg.Set(k, v) + } + } + + return done, nil +} + +func deepestConfigSourcesFirst(keys []string) func(int, int) bool { + return func(i, j int) bool { + iLastSrcIdx := strings.LastIndex(keys[i], ConfigSourcePrefix) + jLastSrcIdx := strings.LastIndex(keys[j], ConfigSourcePrefix) + if iLastSrcIdx != jLastSrcIdx { + // At least one of the keys has a config source. + return iLastSrcIdx > jLastSrcIdx + } + + // It can be one of the following cases: + // 1. None has a config source invocation + // 2. Both have at the same level, ie.: they are siblings in the config. + // In any case it doesn't matter which one is considered "smaller". + return true + } +} + +func extractCfgSrcInvocation(k string) (dstKey, cfgSrcName, paramsKey string) { + firstPrefixIdx := strings.Index(k, ConfigSourcePrefix) + if firstPrefixIdx == -1 { + // No config source to be applied. + return + } + + // Check for a deeper one. + tagPrefixIdx := strings.LastIndex(k, ConfigSourcePrefix) + dstKey = strings.TrimSuffix(k[:tagPrefixIdx], ViperDelimiter) + + cfgSrcFromStart := k[tagPrefixIdx+len(ConfigSourcePrefix):] + prefixToTagEndLen := strings.Index(cfgSrcFromStart, ViperDelimiter) + if prefixToTagEndLen > -1 { + cfgSrcName = cfgSrcFromStart[:prefixToTagEndLen] + } else { + cfgSrcName = cfgSrcFromStart + } + + paramsKey = k[:tagPrefixIdx+len(cfgSrcName)+len(ConfigSourcePrefix)] + + return +} diff --git a/config/configsource_test.go b/config/configsource_test.go new file mode 100644 index 00000000000..b248a770397 --- /dev/null +++ b/config/configsource_test.go @@ -0,0 +1,381 @@ +package config + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "go.opentelemetry.io/collector/component" +) + +func Test_applyConfigSources_no_error(t *testing.T) { + cfgSources := map[string]component.ConfigSource{ + "cfgsrc": &cfgSrcMock{ + keys: map[string]interface{}{ + "int": 1, + "str": "str", + "bool": true, + "interface": map[string]interface{}{ + "i0": 0, + "str1": "1", + }, + "recurse": "$cfgsrc", + }, + }, + } + + tests := []struct { + name string + srcCfg map[string]interface{} + dstCfg map[string]interface{} + done bool + }{ + { + name: "done", + srcCfg: map[string]interface{}{"a": 0}, + dstCfg: map[string]interface{}{"a": 0}, + done: true, + }, + { + name: "root", + srcCfg: map[string]interface{}{ + cfgSrcKey("cfgsrc"): map[string]interface{}{ + "key": "interface", + }, + }, + dstCfg: map[string]interface{}{ + "i0": 0, + "str1": "1", + }, + }, + { + name: "basic", + srcCfg: map[string]interface{}{ + "a": map[string]interface{}{ + "a": "b", + "c": map[string]interface{}{cfgSrcKey("cfgsrc"): nil}, + }, + }, + dstCfg: map[string]interface{}{ + "a": map[string]interface{}{ + "a": "b", + "c": "nil_param", + }, + }, + }, + { + name: "nested", + srcCfg: map[string]interface{}{ + "a": map[string]interface{}{ + cfgSrcKey("cfgsrc"): map[string]interface{}{ + "b": map[string]interface{}{ + cfgSrcKey("cfgsrc"): map[string]interface{}{ + "key": "bool", + "int": 1, + "map": map[string]interface{}{ + "str": "force_already_visied_cfgsrc", + }, + }, + }, + }, + }, + }, + dstCfg: map[string]interface{}{ + "a": map[string]interface{}{ + cfgSrcKey("cfgsrc"): map[string]interface{}{ + "b": true, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srcV := NewViper() + require.NoError(t, srcV.MergeConfigMap(tt.srcCfg)) + + dstV := NewViper() + done, err := applyConfigSources(context.Background(), srcV, dstV, cfgSources) + assert.NoError(t, err) + assert.Equal(t, tt.done, done) + assert.Equal(t, tt.dstCfg, dstV.AllSettings()) + }) + } +} + +func Test_applyConfigSources_error(t *testing.T) { + cfgSources := map[string]component.ConfigSource{ + "cfgsrc": &cfgSrcMock{ + keys: map[string]interface{}{ + "int": 1, + "str": "str", + "bool": true, + "interface": map[string]interface{}{ + "i0": 0, + "str1": "1", + }, + "recurse": "$cfgsrc", + }, + }, + } + + tests := []struct { + name string + srcCfg map[string]interface{} + dstCfg map[string]interface{} + done bool + }{ + { + name: "done", + srcCfg: map[string]interface{}{"a": 0}, + dstCfg: map[string]interface{}{"a": 0}, + done: true, + }, + { + name: "root", + srcCfg: map[string]interface{}{ + cfgSrcKey("cfgsrc"): map[string]interface{}{ + "key": "interface", + }, + }, + dstCfg: map[string]interface{}{ + "i0": 0, + "str1": "1", + }, + }, + { + name: "basic", + srcCfg: map[string]interface{}{ + "a": map[string]interface{}{ + "a": "b", + "c": map[string]interface{}{cfgSrcKey("cfgsrc"): nil}, + }, + }, + dstCfg: map[string]interface{}{ + "a": map[string]interface{}{ + "a": "b", + "c": "nil_param", + }, + }, + }, + { + name: "nested", + srcCfg: map[string]interface{}{ + "a": map[string]interface{}{ + cfgSrcKey("cfgsrc"): map[string]interface{}{ + "b": map[string]interface{}{ + cfgSrcKey("cfgsrc"): map[string]interface{}{ + "key": "bool", + "int": 1, + "map": map[string]interface{}{ + "str": "force_already_visied_cfgsrc", + }, + }, + }, + }, + }, + }, + dstCfg: map[string]interface{}{ + "a": map[string]interface{}{ + cfgSrcKey("cfgsrc"): map[string]interface{}{ + "b": true, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + srcV := NewViper() + require.NoError(t, srcV.MergeConfigMap(tt.srcCfg)) + + done, err := applyConfigSources(context.Background(), srcV, NewViper(), cfgSources) + assert.NoError(t, err) + assert.False(t, done) + // TODO: Check error + }) + } +} + +func Test_deepestConfigSourcesFirst(t *testing.T) { + tests := []struct { + name string + keys []string + expected bool + }{ + { + name: "left_cfgsrc", + keys: []string{ + concatKeys("a", "b", "c", cfgSrcKey("d"), "e"), + concatKeys("a", "b", "c", "f", "g", "h"), + }, + expected: true, + }, + { + name: "right_cfgsrc", + keys: []string{ + concatKeys("a", "b", "c", "f", "g", "h"), + concatKeys("a", "b", "c", cfgSrcKey("d"), "e"), + }, + expected: false, + }, + { + name: "left_cfgsrc_deepest", + keys: []string{ + concatKeys("a", "b", "c", "d", cfgSrcKey("e")), + concatKeys("a", "b", "c", cfgSrcKey("f"), "g"), + }, + expected: true, + }, + { + name: "right_cfgsrc_deepest", + keys: []string{ + concatKeys("a", "b", "c", cfgSrcKey("f"), "g"), + concatKeys("a", "b", "c", "d", cfgSrcKey("e")), + }, + expected: false, + }, + { + name: "left_nested_cfgsrc", + keys: []string{ + concatKeys("a", cfgSrcKey("b"), "c", "d", cfgSrcKey("e")), + concatKeys("a", cfgSrcKey("b"), "c", "f", "g"), + }, + expected: true, + }, + { + name: "right_nested_cfgsrc", + keys: []string{ + concatKeys("a", cfgSrcKey("b"), "c", "f", "g"), + concatKeys("a", cfgSrcKey("b"), "c", "d", cfgSrcKey("e")), + }, + expected: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + lessFn := deepestConfigSourcesFirst(tt.keys) + assert.Equal(t, tt.expected, lessFn(0, 1)) + }) + } +} + +func Test_extractCfgSrcInvocation(t *testing.T) { + tests := []struct { + key string + dstKey string + cfgSrc string + paramsKey string + }{ + { + key: concatKeys("a"), + }, + { + key: concatKeys("a", "b"), + }, + { + key: concatKeys("simple", cfgSrcKey("cfgsrc")), + dstKey: "simple", + cfgSrc: "cfgsrc", + paramsKey: concatKeys("simple", cfgSrcKey("cfgsrc")), + }, + { + key: concatKeys("simple_a", cfgSrcKey("cfgsrc"), "a"), + dstKey: "simple_a", + cfgSrc: "cfgsrc", + paramsKey: concatKeys("simple_a", cfgSrcKey("cfgsrc")), + }, + { + key: concatKeys("nested", cfgSrcKey("top"), "a", cfgSrcKey("deepest")), + dstKey: concatKeys("nested", cfgSrcKey("top"), "a"), + cfgSrc: "deepest", + paramsKey: concatKeys("nested", cfgSrcKey("top"), "a", cfgSrcKey("deepest")), + }, + { + key: concatKeys("nested_a", cfgSrcKey("top"), "a", cfgSrcKey("deepest"), "a"), + dstKey: concatKeys("nested_a", cfgSrcKey("top"), "a"), + cfgSrc: "deepest", + paramsKey: concatKeys("nested_a", cfgSrcKey("top"), "a", cfgSrcKey("deepest")), + }, + { + key: concatKeys(cfgSrcKey("cfgSrcTopLvl")), + cfgSrc: "cfgSrcTopLvl", + paramsKey: cfgSrcKey("cfgSrcTopLvl"), + }, + { + key: concatKeys(cfgSrcKey("cfgSrcTopLvl_a"), "a"), + cfgSrc: "cfgSrcTopLvl_a", + paramsKey: cfgSrcKey("cfgSrcTopLvl_a"), + }, + { + key: concatKeys("typical", "a", "b", cfgSrcKey("cfgsrc")), + dstKey: concatKeys("typical", "a", "b"), + cfgSrc: "cfgsrc", + paramsKey: concatKeys("typical", "a", "b", cfgSrcKey("cfgsrc")), + }, + { + key: concatKeys("typical_a", "a", "b", cfgSrcKey("cfgsrc"), "a", "b"), + dstKey: concatKeys("typical_a", "a", "b"), + cfgSrc: "cfgsrc", + paramsKey: concatKeys("typical_a", "a", "b", cfgSrcKey("cfgsrc")), + }, + } + + for _, tt := range tests { + t.Run(tt.key, func(t *testing.T) { + dstKey, cfgSrc, paramsKey := extractCfgSrcInvocation(tt.key) + + assert.Equal(t, tt.dstKey, dstKey) + assert.Equal(t, tt.cfgSrc, cfgSrc) + assert.Equal(t, tt.paramsKey, paramsKey) + }) + } +} + +func concatKeys(keys ...string) (s string) { + s = keys[0] + for _, key := range keys[1:] { + s += ViperDelimiter + key + } + return s +} + +func cfgSrcKey(key string) string { + return ConfigSourcePrefix + key +} + +type cfgSrcMock struct { + keys map[string]interface{} +} + +func (c cfgSrcMock) Start(_ context.Context, _ component.Host) error { + return nil +} + +func (c cfgSrcMock) Shutdown(_ context.Context) error { + return nil +} + +func (c cfgSrcMock) BeginSession(_ context.Context, _ component.SessionParams) error { + return nil +} + +func (c cfgSrcMock) Apply(_ context.Context, params interface{}) (interface{}, error) { + if params == nil { + return "nil_param", nil + } + switch v := params.(type) { + case string: + return v, nil + case map[string]interface{}: + return c.keys[v["key"].(string)], nil + } + return nil, nil +} + +func (c cfgSrcMock) EndSession(_ context.Context, _ component.SessionParams) { +} From 8f01de358ef7c4ecaa940a1e20fc43a5cdd36e4a Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Mon, 15 Mar 2021 19:33:19 -0700 Subject: [PATCH 03/19] Add ApplyConfigSources to a configuration --- config/{ => configsource}/configsource.go | 74 ++++-- .../{ => configsource}/configsource_test.go | 218 +++++++++++------- .../testdata/err_begin_session.yaml | 4 + .../testdata/err_begin_session_sources.yaml | 2 + config/configsource/testdata/multiple.yaml | 5 + .../testdata/multiple_expected.yaml | 4 + .../testdata/multiple_sources.yaml | 7 + config/configsource/testdata/recursion.yaml | 4 + .../testdata/recursion_sources.yaml | 6 + config/configsource/testdata/simple.yaml | 4 + .../testdata/simple_expected.yaml | 2 + .../configsource/testdata/simple_sources.yaml | 3 + 12 files changed, 240 insertions(+), 93 deletions(-) rename config/{ => configsource}/configsource.go (71%) rename config/{ => configsource}/configsource_test.go (59%) create mode 100644 config/configsource/testdata/err_begin_session.yaml create mode 100644 config/configsource/testdata/err_begin_session_sources.yaml create mode 100644 config/configsource/testdata/multiple.yaml create mode 100644 config/configsource/testdata/multiple_expected.yaml create mode 100644 config/configsource/testdata/multiple_sources.yaml create mode 100644 config/configsource/testdata/recursion.yaml create mode 100644 config/configsource/testdata/recursion_sources.yaml create mode 100644 config/configsource/testdata/simple.yaml create mode 100644 config/configsource/testdata/simple_expected.yaml create mode 100644 config/configsource/testdata/simple_sources.yaml diff --git a/config/configsource.go b/config/configsource/configsource.go similarity index 71% rename from config/configsource.go rename to config/configsource/configsource.go index 2e61c23990d..7735400f29a 100644 --- a/config/configsource.go +++ b/config/configsource/configsource.go @@ -12,15 +12,18 @@ // See the License for the specific language governing permissions and // limitations under the License. -package config +package configsource import ( "context" - "errors" - "github.com/spf13/viper" - "go.opentelemetry.io/collector/component" + "fmt" "sort" "strings" + + "github.com/spf13/viper" + + "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/config" ) const ( @@ -50,10 +53,16 @@ func ApplyConfigSources(ctx context.Context, v *viper.Viper, params ApplyConfigS } // Notify all sources that we are about to start. - for _, cfgSrc := range params.ConfigSources { + for cfgSrcName, cfgSrc := range params.ConfigSources { if err := cfgSrc.BeginSession(ctx, sessionParams); err != nil { - return nil, err // TODO: wrap error with more information. + return nil, &cfgSrcError{ + msg: fmt.Sprintf("config source %s begin session error: %v", cfgSrcName, err), + code: errCfgSrcBeginSession, + } } + + // The scope of usage of the cfgSrc is the whole function so it is fine to defer + // the EndSession call to the function exit. defer cfgSrc.EndSession(ctx, sessionParams) } @@ -62,8 +71,8 @@ func ApplyConfigSources(ctx context.Context, v *viper.Viper, params ApplyConfigS var err error done := false - for i := uint(0); i <= params.MaxRecursionDepth && !done; i++ { - dstCfg = NewViper() + for i := -1; i <= int(params.MaxRecursionDepth) && !done; i++ { + dstCfg = config.NewViper() done, err = applyConfigSources(ctx, srcCfg, dstCfg, params.ConfigSources) if err != nil { return nil, err @@ -72,16 +81,42 @@ func ApplyConfigSources(ctx context.Context, v *viper.Viper, params ApplyConfigS } if !done { - return nil, errors.New("couldn't fully expand the configuration") + return nil, &cfgSrcError{ + msg: "config source recursion chain is too deep, couldn't fully expand the configuration", + code: errCfgSrcChainTooLong, + } } return dstCfg, nil } +// cfgSrcError private error type used to accurate identify the type of error in tests. +type cfgSrcError struct { + msg string // human readable error message. + code cfgSrcErrorCode // internal error code. +} + +func (e *cfgSrcError) Error() string { + return e.msg +} + +// These are errors that can be returned by ApplyConfigSources function. +// Note that error codes are not part public API, they are for unit testing only. +type cfgSrcErrorCode int + +const ( + _ cfgSrcErrorCode = iota // skip 0, start errors codes from 1. + errCfgSrcChainTooLong + errOnlyMapAtRootLevel + errCfgSrcBeginSession + errCfgSrcNotFound + errCfgSrcApply +) + func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSources map[string]component.ConfigSource) (bool, error) { // Expand any item env vars in the config, do it every time so env vars // added on previous pass are also handled. - expandEnvConfig(srcCfg) + config.ExpandEnvConfig(srcCfg) done := true appliedTags := make(map[string]struct{}) @@ -106,13 +141,19 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou appliedTags[dstKey] = struct{}{} cfgSrc, ok := cfgSources[cfgSrcName] if !ok { - return false, errors.New("unknown config source") // TODO: error similar to other config errors. + return false, &cfgSrcError{ + msg: fmt.Sprintf("config source %s not found", cfgSrcName), + code: errCfgSrcNotFound, + } } applyParams := srcCfg.Get(paramsKey) actualCfg, err := cfgSrc.Apply(ctx, applyParams) if err != nil { - return false, err // TODO: Add config source name to the error. + return false, &cfgSrcError{ + msg: fmt.Sprintf("error applying config source %s: %v", cfgSrcName, err), + code: errCfgSrcApply, + } } // The injection may require further expansion, assume that we are not done yet. @@ -125,7 +166,10 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou // This is at the root level, have to inject the top keys one by one. rootMap, ok := actualCfg.(map[string]interface{}) if !ok { - return false, errors.New("only a map can be injected at the root level") // TODO: better error info. + return false, &cfgSrcError{ + msg: "only a map can be injected at the root level", + code: errOnlyMapAtRootLevel, + } } for k, v := range rootMap { dstCfg.Set(k, v) @@ -161,10 +205,10 @@ func extractCfgSrcInvocation(k string) (dstKey, cfgSrcName, paramsKey string) { // Check for a deeper one. tagPrefixIdx := strings.LastIndex(k, ConfigSourcePrefix) - dstKey = strings.TrimSuffix(k[:tagPrefixIdx], ViperDelimiter) + dstKey = strings.TrimSuffix(k[:tagPrefixIdx], config.ViperDelimiter) cfgSrcFromStart := k[tagPrefixIdx+len(ConfigSourcePrefix):] - prefixToTagEndLen := strings.Index(cfgSrcFromStart, ViperDelimiter) + prefixToTagEndLen := strings.Index(cfgSrcFromStart, config.ViperDelimiter) if prefixToTagEndLen > -1 { cfgSrcName = cfgSrcFromStart[:prefixToTagEndLen] } else { diff --git a/config/configsource_test.go b/config/configsource/configsource_test.go similarity index 59% rename from config/configsource_test.go rename to config/configsource/configsource_test.go index b248a770397..939bfc3397a 100644 --- a/config/configsource_test.go +++ b/config/configsource/configsource_test.go @@ -1,19 +1,94 @@ -package config +// 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 configsource import ( "context" + "errors" + "path" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/config" ) +func TestApplyConfigSources(t *testing.T) { + tests := []struct { + name string + cfgSrcErrorCode cfgSrcErrorCode + applyParams ApplyConfigSourcesParams + }{ + { + name: "simple", + }, + { + name: "err_begin_session", + cfgSrcErrorCode: errCfgSrcBeginSession, + }, + { + name: "recursion", + cfgSrcErrorCode: errCfgSrcChainTooLong, + applyParams: ApplyConfigSourcesParams{ + MaxRecursionDepth: 2, + }, + }, + { + name: "multiple", + cfgSrcErrorCode: errCfgSrcChainTooLong, + }, + { + name: "multiple", + applyParams: ApplyConfigSourcesParams{ + MaxRecursionDepth: 1, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + v := config.NewViper() + v.SetConfigFile(path.Join("testdata", tt.name+".yaml")) + require.NoError(t, v.ReadInConfig()) + + tt.applyParams.ConfigSources = loadCfgSrcs(t, path.Join("testdata", tt.name+"_sources.yaml")) + dst, err := ApplyConfigSources(context.Background(), v, tt.applyParams) + + if tt.cfgSrcErrorCode != cfgSrcErrorCode(0) { + assert.Nil(t, dst) + assert.Equal(t, tt.cfgSrcErrorCode, err.(*cfgSrcError).code) + return + } + + require.NoError(t, err) + require.NotNil(t, dst) + + expectedViper := config.NewViper() + expectedViper.SetConfigFile(path.Join("testdata", tt.name+"_expected.yaml")) + require.NoError(t, expectedViper.ReadInConfig()) + assert.Equal(t, expectedViper.AllSettings(), dst.AllSettings()) + }) + } +} + func Test_applyConfigSources_no_error(t *testing.T) { cfgSources := map[string]component.ConfigSource{ "cfgsrc": &cfgSrcMock{ - keys: map[string]interface{}{ + Keys: map[string]interface{}{ "int": 1, "str": "str", "bool": true, @@ -94,10 +169,10 @@ func Test_applyConfigSources_no_error(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - srcV := NewViper() + srcV := config.NewViper() require.NoError(t, srcV.MergeConfigMap(tt.srcCfg)) - dstV := NewViper() + dstV := config.NewViper() done, err := applyConfigSources(context.Background(), srcV, dstV, cfgSources) assert.NoError(t, err) assert.Equal(t, tt.done, done) @@ -109,94 +184,57 @@ func Test_applyConfigSources_no_error(t *testing.T) { func Test_applyConfigSources_error(t *testing.T) { cfgSources := map[string]component.ConfigSource{ "cfgsrc": &cfgSrcMock{ - keys: map[string]interface{}{ - "int": 1, - "str": "str", + Keys: map[string]interface{}{ "bool": true, - "interface": map[string]interface{}{ - "i0": 0, - "str1": "1", - }, - "recurse": "$cfgsrc", }, }, } tests := []struct { - name string - srcCfg map[string]interface{} - dstCfg map[string]interface{} - done bool + name string + srcCfg map[string]interface{} + errCode cfgSrcErrorCode }{ { - name: "done", - srcCfg: map[string]interface{}{"a": 0}, - dstCfg: map[string]interface{}{"a": 0}, - done: true, - }, - { - name: "root", + name: "err_missing_cfgsrc", srcCfg: map[string]interface{}{ - cfgSrcKey("cfgsrc"): map[string]interface{}{ + cfgSrcKey("cfgsrc/1"): map[string]interface{}{ "key": "interface", }, }, - dstCfg: map[string]interface{}{ - "i0": 0, - "str1": "1", - }, + errCode: errCfgSrcNotFound, }, { - name: "basic", + name: "err_cfgsrc_apply", srcCfg: map[string]interface{}{ - "a": map[string]interface{}{ - "a": "b", - "c": map[string]interface{}{cfgSrcKey("cfgsrc"): nil}, - }, - }, - dstCfg: map[string]interface{}{ - "a": map[string]interface{}{ - "a": "b", - "c": "nil_param", + "root": map[string]interface{}{ + cfgSrcKey("cfgsrc"): map[string]interface{}{ + "missing_key_param": "", + }, }, }, + errCode: errCfgSrcApply, }, { - name: "nested", + name: "err_only_map_at_root", srcCfg: map[string]interface{}{ - "a": map[string]interface{}{ - cfgSrcKey("cfgsrc"): map[string]interface{}{ - "b": map[string]interface{}{ - cfgSrcKey("cfgsrc"): map[string]interface{}{ - "key": "bool", - "int": 1, - "map": map[string]interface{}{ - "str": "force_already_visied_cfgsrc", - }, - }, - }, - }, - }, - }, - dstCfg: map[string]interface{}{ - "a": map[string]interface{}{ - cfgSrcKey("cfgsrc"): map[string]interface{}{ - "b": true, - }, + cfgSrcKey("cfgsrc"): map[string]interface{}{ + "key": "bool", }, }, + errCode: errOnlyMapAtRootLevel, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - srcV := NewViper() + srcV := config.NewViper() require.NoError(t, srcV.MergeConfigMap(tt.srcCfg)) - done, err := applyConfigSources(context.Background(), srcV, NewViper(), cfgSources) - assert.NoError(t, err) + done, err := applyConfigSources(context.Background(), srcV, config.NewViper(), cfgSources) assert.False(t, done) - // TODO: Check error + require.Error(t, err) + assert.Equal(t, tt.errCode, err.(*cfgSrcError).code) }) } } @@ -266,59 +304,59 @@ func Test_deepestConfigSourcesFirst(t *testing.T) { func Test_extractCfgSrcInvocation(t *testing.T) { tests := []struct { - key string + name string dstKey string cfgSrc string paramsKey string }{ { - key: concatKeys("a"), + name: concatKeys("a"), }, { - key: concatKeys("a", "b"), + name: concatKeys("a", "b"), }, { - key: concatKeys("simple", cfgSrcKey("cfgsrc")), + name: concatKeys("simple", cfgSrcKey("cfgsrc")), dstKey: "simple", cfgSrc: "cfgsrc", paramsKey: concatKeys("simple", cfgSrcKey("cfgsrc")), }, { - key: concatKeys("simple_a", cfgSrcKey("cfgsrc"), "a"), + name: concatKeys("simple_a", cfgSrcKey("cfgsrc"), "a"), dstKey: "simple_a", cfgSrc: "cfgsrc", paramsKey: concatKeys("simple_a", cfgSrcKey("cfgsrc")), }, { - key: concatKeys("nested", cfgSrcKey("top"), "a", cfgSrcKey("deepest")), + name: concatKeys("nested", cfgSrcKey("top"), "a", cfgSrcKey("deepest")), dstKey: concatKeys("nested", cfgSrcKey("top"), "a"), cfgSrc: "deepest", paramsKey: concatKeys("nested", cfgSrcKey("top"), "a", cfgSrcKey("deepest")), }, { - key: concatKeys("nested_a", cfgSrcKey("top"), "a", cfgSrcKey("deepest"), "a"), + name: concatKeys("nested_a", cfgSrcKey("top"), "a", cfgSrcKey("deepest"), "a"), dstKey: concatKeys("nested_a", cfgSrcKey("top"), "a"), cfgSrc: "deepest", paramsKey: concatKeys("nested_a", cfgSrcKey("top"), "a", cfgSrcKey("deepest")), }, { - key: concatKeys(cfgSrcKey("cfgSrcTopLvl")), + name: concatKeys(cfgSrcKey("cfgSrcTopLvl")), cfgSrc: "cfgSrcTopLvl", paramsKey: cfgSrcKey("cfgSrcTopLvl"), }, { - key: concatKeys(cfgSrcKey("cfgSrcTopLvl_a"), "a"), + name: concatKeys(cfgSrcKey("cfgSrcTopLvl_a"), "a"), cfgSrc: "cfgSrcTopLvl_a", paramsKey: cfgSrcKey("cfgSrcTopLvl_a"), }, { - key: concatKeys("typical", "a", "b", cfgSrcKey("cfgsrc")), + name: concatKeys("typical", "a", "b", cfgSrcKey("cfgsrc")), dstKey: concatKeys("typical", "a", "b"), cfgSrc: "cfgsrc", paramsKey: concatKeys("typical", "a", "b", cfgSrcKey("cfgsrc")), }, { - key: concatKeys("typical_a", "a", "b", cfgSrcKey("cfgsrc"), "a", "b"), + name: concatKeys("typical_a", "a", "b", cfgSrcKey("cfgsrc"), "a", "b"), dstKey: concatKeys("typical_a", "a", "b"), cfgSrc: "cfgsrc", paramsKey: concatKeys("typical_a", "a", "b", cfgSrcKey("cfgsrc")), @@ -326,8 +364,8 @@ func Test_extractCfgSrcInvocation(t *testing.T) { } for _, tt := range tests { - t.Run(tt.key, func(t *testing.T) { - dstKey, cfgSrc, paramsKey := extractCfgSrcInvocation(tt.key) + t.Run(tt.name, func(t *testing.T) { + dstKey, cfgSrc, paramsKey := extractCfgSrcInvocation(tt.name) assert.Equal(t, tt.dstKey, dstKey) assert.Equal(t, tt.cfgSrc, cfgSrc) @@ -339,7 +377,7 @@ func Test_extractCfgSrcInvocation(t *testing.T) { func concatKeys(keys ...string) (s string) { s = keys[0] for _, key := range keys[1:] { - s += ViperDelimiter + key + s += config.ViperDelimiter + key } return s } @@ -349,7 +387,8 @@ func cfgSrcKey(key string) string { } type cfgSrcMock struct { - keys map[string]interface{} + ErrOnBeginSession bool `mapstructure:"err_begin_session"` + Keys map[string]interface{} `mapstructure:"keys"` } func (c cfgSrcMock) Start(_ context.Context, _ component.Host) error { @@ -361,6 +400,9 @@ func (c cfgSrcMock) Shutdown(_ context.Context) error { } func (c cfgSrcMock) BeginSession(_ context.Context, _ component.SessionParams) error { + if c.ErrOnBeginSession { + return errors.New("request to error on BeginSession") + } return nil } @@ -372,10 +414,30 @@ func (c cfgSrcMock) Apply(_ context.Context, params interface{}) (interface{}, e case string: return v, nil case map[string]interface{}: - return c.keys[v["key"].(string)], nil + keyVal, ok := v["key"] + if !ok { + return nil, errors.New("missing 'key' parameter") + } + return c.Keys[keyVal.(string)], nil } return nil, nil } func (c cfgSrcMock) EndSession(_ context.Context, _ component.SessionParams) { } + +// TODO: Create proper factories and methods to load generic ConfigSources, temporary test helper. +func loadCfgSrcs(t *testing.T, file string) map[string]component.ConfigSource { + v := config.NewViper() + v.SetConfigFile(file) + require.NoError(t, v.ReadInConfig()) + + cfgSrcMap := make(map[string]component.ConfigSource) + for cfgSrcName := range v.AllSettings() { + cfgSrc := &cfgSrcMock{} + require.NoError(t, v.Sub(cfgSrcName).UnmarshalExact(cfgSrc)) + cfgSrcMap[cfgSrcName] = cfgSrc + } + + return cfgSrcMap +} diff --git a/config/configsource/testdata/err_begin_session.yaml b/config/configsource/testdata/err_begin_session.yaml new file mode 100644 index 00000000000..377315d2177 --- /dev/null +++ b/config/configsource/testdata/err_begin_session.yaml @@ -0,0 +1,4 @@ +simple: + test: + $cfgsrc: + key: simple diff --git a/config/configsource/testdata/err_begin_session_sources.yaml b/config/configsource/testdata/err_begin_session_sources.yaml new file mode 100644 index 00000000000..eb937aecea7 --- /dev/null +++ b/config/configsource/testdata/err_begin_session_sources.yaml @@ -0,0 +1,2 @@ +cfgsrc: + err_begin_session: true diff --git a/config/configsource/testdata/multiple.yaml b/config/configsource/testdata/multiple.yaml new file mode 100644 index 00000000000..12c035fdba9 --- /dev/null +++ b/config/configsource/testdata/multiple.yaml @@ -0,0 +1,5 @@ +one: + $cfgsrc/0: deep_0 +two: + $cfgsrc/1: + key: recurse_to_cfgsrc/0 diff --git a/config/configsource/testdata/multiple_expected.yaml b/config/configsource/testdata/multiple_expected.yaml new file mode 100644 index 00000000000..802c23c6e21 --- /dev/null +++ b/config/configsource/testdata/multiple_expected.yaml @@ -0,0 +1,4 @@ +one: + deep_0 +two: + deep_1 diff --git a/config/configsource/testdata/multiple_sources.yaml b/config/configsource/testdata/multiple_sources.yaml new file mode 100644 index 00000000000..f7ce6e1bf47 --- /dev/null +++ b/config/configsource/testdata/multiple_sources.yaml @@ -0,0 +1,7 @@ +cfgsrc/0: + keys: + some: value +cfgsrc/1: + keys: + recurse_to_cfgsrc/0: + $cfgsrc/0: deep_1 diff --git a/config/configsource/testdata/recursion.yaml b/config/configsource/testdata/recursion.yaml new file mode 100644 index 00000000000..e1473b36e79 --- /dev/null +++ b/config/configsource/testdata/recursion.yaml @@ -0,0 +1,4 @@ +recurse: + test: + $cfgsrc: + key: "recurse" diff --git a/config/configsource/testdata/recursion_sources.yaml b/config/configsource/testdata/recursion_sources.yaml new file mode 100644 index 00000000000..a8f64578422 --- /dev/null +++ b/config/configsource/testdata/recursion_sources.yaml @@ -0,0 +1,6 @@ +cfgsrc: + keys: + recurse: + test: + $cfgsrc: + key: recurse diff --git a/config/configsource/testdata/simple.yaml b/config/configsource/testdata/simple.yaml new file mode 100644 index 00000000000..377315d2177 --- /dev/null +++ b/config/configsource/testdata/simple.yaml @@ -0,0 +1,4 @@ +simple: + test: + $cfgsrc: + key: simple diff --git a/config/configsource/testdata/simple_expected.yaml b/config/configsource/testdata/simple_expected.yaml new file mode 100644 index 00000000000..39f79ceffdd --- /dev/null +++ b/config/configsource/testdata/simple_expected.yaml @@ -0,0 +1,2 @@ +simple: + test: str diff --git a/config/configsource/testdata/simple_sources.yaml b/config/configsource/testdata/simple_sources.yaml new file mode 100644 index 00000000000..a02657ea07e --- /dev/null +++ b/config/configsource/testdata/simple_sources.yaml @@ -0,0 +1,3 @@ +cfgsrc: + keys: + simple: str From 5fe5b7c8f64dde5b6acaad7d2be40b048357f7a2 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Tue, 16 Mar 2021 14:44:29 -0700 Subject: [PATCH 04/19] Add ConfigSourceSettings --- config/configmodels/configsource.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/config/configmodels/configsource.go b/config/configmodels/configsource.go index 4655fcd23b3..e5e875a6505 100644 --- a/config/configmodels/configsource.go +++ b/config/configmodels/configsource.go @@ -20,3 +20,30 @@ package configmodels type ConfigSource interface { NamedEntity } + +// ConfigSources is a map of names to ConfigSource instances. +type ConfigSources map[string]ConfigSource + +// ConfigSourceSettings defines common settings for a ConfigSource configuration. +// Specific ConfigSource types can embed this struct and extend it with more fields if needed. +type ConfigSourceSettings struct { + TypeVal Type `mapstructure:"-"` + NameVal string `mapstructure:"-"` +} + +var _ ConfigSource = (*ConfigSourceSettings)(nil) + +// Name gets the ConfigSource name. +func (css *ConfigSourceSettings) Name() string { + return css.NameVal +} + +// SetName sets the ConfigSource name. +func (css *ConfigSourceSettings) SetName(name string) { + css.NameVal = name +} + +// Type sets the ConfigSource type. +func (css *ConfigSourceSettings) Type() Type { + return css.TypeVal +} From 0628c7f3000391969d863226c32b6c7f698836a8 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Tue, 16 Mar 2021 14:45:37 -0700 Subject: [PATCH 05/19] Add local expandEnvConfig to configsource package Avoids exposing it from config package. --- config/configsource/configsource.go | 48 ++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/config/configsource/configsource.go b/config/configsource/configsource.go index 7735400f29a..ae824cf399a 100644 --- a/config/configsource/configsource.go +++ b/config/configsource/configsource.go @@ -17,6 +17,7 @@ package configsource import ( "context" "fmt" + "os" "sort" "strings" @@ -116,7 +117,7 @@ const ( func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSources map[string]component.ConfigSource) (bool, error) { // Expand any item env vars in the config, do it every time so env vars // added on previous pass are also handled. - config.ExpandEnvConfig(srcCfg) + expandEnvConfig(srcCfg) done := true appliedTags := make(map[string]struct{}) @@ -219,3 +220,48 @@ func extractCfgSrcInvocation(k string) (dstKey, cfgSrcName, paramsKey string) { return } + +// Copied from config package to avoid exposing as public API. +// TODO: Add local tests covering the code below. + +// expandEnvConfig creates a new viper config with expanded values for all the values (simple, list or map value). +// It does not expand the keys. +func expandEnvConfig(v *viper.Viper) { + for _, k := range v.AllKeys() { + v.Set(k, expandStringValues(v.Get(k))) + } +} + +func expandStringValues(value interface{}) interface{} { + switch v := value.(type) { + default: + return v + case string: + return expandEnv(v) + case []interface{}: + nslice := make([]interface{}, 0, len(v)) + for _, vint := range v { + nslice = append(nslice, expandStringValues(vint)) + } + return nslice + case map[interface{}]interface{}: + nmap := make(map[interface{}]interface{}, len(v)) + for k, vint := range v { + nmap[k] = expandStringValues(vint) + } + return nmap + } +} + +func expandEnv(s string) string { + return os.Expand(s, func(str string) string { + // This allows escaping environment variable substitution via $$, e.g. + // - $FOO will be substituted with env var FOO + // - $$FOO will be replaced with $FOO + // - $$$FOO will be replaced with $ + substituted env var FOO + if str == "$" { + return "$" + } + return os.Getenv(str) + }) +} From a4539ecaf0dbcd646e94f39c4e44195c08950ede Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Tue, 16 Mar 2021 23:22:16 +0000 Subject: [PATCH 06/19] Relocate configsource to config/interal --- config/{ => internal}/configsource/configsource.go | 0 config/{ => internal}/configsource/configsource_test.go | 0 .../{ => internal}/configsource/testdata/err_begin_session.yaml | 0 .../configsource/testdata/err_begin_session_sources.yaml | 0 config/{ => internal}/configsource/testdata/multiple.yaml | 0 .../{ => internal}/configsource/testdata/multiple_expected.yaml | 0 config/{ => internal}/configsource/testdata/multiple_sources.yaml | 0 config/{ => internal}/configsource/testdata/recursion.yaml | 0 .../{ => internal}/configsource/testdata/recursion_sources.yaml | 0 config/{ => internal}/configsource/testdata/simple.yaml | 0 config/{ => internal}/configsource/testdata/simple_expected.yaml | 0 config/{ => internal}/configsource/testdata/simple_sources.yaml | 0 12 files changed, 0 insertions(+), 0 deletions(-) rename config/{ => internal}/configsource/configsource.go (100%) rename config/{ => internal}/configsource/configsource_test.go (100%) rename config/{ => internal}/configsource/testdata/err_begin_session.yaml (100%) rename config/{ => internal}/configsource/testdata/err_begin_session_sources.yaml (100%) rename config/{ => internal}/configsource/testdata/multiple.yaml (100%) rename config/{ => internal}/configsource/testdata/multiple_expected.yaml (100%) rename config/{ => internal}/configsource/testdata/multiple_sources.yaml (100%) rename config/{ => internal}/configsource/testdata/recursion.yaml (100%) rename config/{ => internal}/configsource/testdata/recursion_sources.yaml (100%) rename config/{ => internal}/configsource/testdata/simple.yaml (100%) rename config/{ => internal}/configsource/testdata/simple_expected.yaml (100%) rename config/{ => internal}/configsource/testdata/simple_sources.yaml (100%) diff --git a/config/configsource/configsource.go b/config/internal/configsource/configsource.go similarity index 100% rename from config/configsource/configsource.go rename to config/internal/configsource/configsource.go diff --git a/config/configsource/configsource_test.go b/config/internal/configsource/configsource_test.go similarity index 100% rename from config/configsource/configsource_test.go rename to config/internal/configsource/configsource_test.go diff --git a/config/configsource/testdata/err_begin_session.yaml b/config/internal/configsource/testdata/err_begin_session.yaml similarity index 100% rename from config/configsource/testdata/err_begin_session.yaml rename to config/internal/configsource/testdata/err_begin_session.yaml diff --git a/config/configsource/testdata/err_begin_session_sources.yaml b/config/internal/configsource/testdata/err_begin_session_sources.yaml similarity index 100% rename from config/configsource/testdata/err_begin_session_sources.yaml rename to config/internal/configsource/testdata/err_begin_session_sources.yaml diff --git a/config/configsource/testdata/multiple.yaml b/config/internal/configsource/testdata/multiple.yaml similarity index 100% rename from config/configsource/testdata/multiple.yaml rename to config/internal/configsource/testdata/multiple.yaml diff --git a/config/configsource/testdata/multiple_expected.yaml b/config/internal/configsource/testdata/multiple_expected.yaml similarity index 100% rename from config/configsource/testdata/multiple_expected.yaml rename to config/internal/configsource/testdata/multiple_expected.yaml diff --git a/config/configsource/testdata/multiple_sources.yaml b/config/internal/configsource/testdata/multiple_sources.yaml similarity index 100% rename from config/configsource/testdata/multiple_sources.yaml rename to config/internal/configsource/testdata/multiple_sources.yaml diff --git a/config/configsource/testdata/recursion.yaml b/config/internal/configsource/testdata/recursion.yaml similarity index 100% rename from config/configsource/testdata/recursion.yaml rename to config/internal/configsource/testdata/recursion.yaml diff --git a/config/configsource/testdata/recursion_sources.yaml b/config/internal/configsource/testdata/recursion_sources.yaml similarity index 100% rename from config/configsource/testdata/recursion_sources.yaml rename to config/internal/configsource/testdata/recursion_sources.yaml diff --git a/config/configsource/testdata/simple.yaml b/config/internal/configsource/testdata/simple.yaml similarity index 100% rename from config/configsource/testdata/simple.yaml rename to config/internal/configsource/testdata/simple.yaml diff --git a/config/configsource/testdata/simple_expected.yaml b/config/internal/configsource/testdata/simple_expected.yaml similarity index 100% rename from config/configsource/testdata/simple_expected.yaml rename to config/internal/configsource/testdata/simple_expected.yaml diff --git a/config/configsource/testdata/simple_sources.yaml b/config/internal/configsource/testdata/simple_sources.yaml similarity index 100% rename from config/configsource/testdata/simple_sources.yaml rename to config/internal/configsource/testdata/simple_sources.yaml From ac6d1342365607fc64fc800109ec14eedb83358e Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Tue, 16 Mar 2021 23:39:58 +0000 Subject: [PATCH 07/19] Move ConfigSource from component package to under configsource --- .../internal/configsource/component}/configsource.go | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename {component => config/internal/configsource/component}/configsource.go (100%) diff --git a/component/configsource.go b/config/internal/configsource/component/configsource.go similarity index 100% rename from component/configsource.go rename to config/internal/configsource/component/configsource.go From 254ae27559fe6f31f9384270e3629d9966b644fd Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Tue, 16 Mar 2021 23:40:33 +0000 Subject: [PATCH 08/19] Fixes after files moved --- .../internal/configsource/component/configsource.go | 7 ++++--- config/internal/configsource/configsource.go | 2 +- config/internal/configsource/configsource_test.go | 11 ++++++----- go.sum | 1 + 4 files changed, 12 insertions(+), 9 deletions(-) diff --git a/config/internal/configsource/component/configsource.go b/config/internal/configsource/component/configsource.go index 3db2e4f8ffd..e40171e94fb 100644 --- a/config/internal/configsource/component/configsource.go +++ b/config/internal/configsource/component/configsource.go @@ -19,6 +19,7 @@ import ( "go.uber.org/zap" + "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/config/configmodels" ) @@ -32,7 +33,7 @@ type SessionParams struct { // ConfigSource is the interface to be implemented by objects used by the collector // to retrieve external configuration information. type ConfigSource interface { - Component + component.Component // BeginSession signals that the object is about to be used to inject data into // the configuration. ConfigSource objects can assume that there won't be @@ -59,12 +60,12 @@ type ConfigSourceCreateParams struct { Logger *zap.Logger // ApplicationStartInfo can be used to retrieve data according to version, etc. - ApplicationStartInfo ApplicationStartInfo + ApplicationStartInfo component.ApplicationStartInfo } // ConfigSourceFactory is a factory interface for configuration sources. type ConfigSourceFactory interface { - Factory + component.Factory // CreateDefaultConfig creates the default configuration for the ConfigSource. // This method can be called multiple times depending on the pipeline diff --git a/config/internal/configsource/configsource.go b/config/internal/configsource/configsource.go index ae824cf399a..8dd9ec8bb4d 100644 --- a/config/internal/configsource/configsource.go +++ b/config/internal/configsource/configsource.go @@ -23,8 +23,8 @@ import ( "github.com/spf13/viper" - "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/config" + "go.opentelemetry.io/collector/config/internal/configsource/component" ) const ( diff --git a/config/internal/configsource/configsource_test.go b/config/internal/configsource/configsource_test.go index 939bfc3397a..c145150e4bd 100644 --- a/config/internal/configsource/configsource_test.go +++ b/config/internal/configsource/configsource_test.go @@ -23,8 +23,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.opentelemetry.io/collector/component" + publiccomponent "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/config" + "go.opentelemetry.io/collector/config/internal/configsource/component" ) func TestApplyConfigSources(t *testing.T) { @@ -391,15 +392,15 @@ type cfgSrcMock struct { Keys map[string]interface{} `mapstructure:"keys"` } -func (c cfgSrcMock) Start(_ context.Context, _ component.Host) error { +func (c cfgSrcMock) Start(context.Context, publiccomponent.Host) error { return nil } -func (c cfgSrcMock) Shutdown(_ context.Context) error { +func (c cfgSrcMock) Shutdown(context.Context) error { return nil } -func (c cfgSrcMock) BeginSession(_ context.Context, _ component.SessionParams) error { +func (c cfgSrcMock) BeginSession(context.Context, component.SessionParams) error { if c.ErrOnBeginSession { return errors.New("request to error on BeginSession") } @@ -423,7 +424,7 @@ func (c cfgSrcMock) Apply(_ context.Context, params interface{}) (interface{}, e return nil, nil } -func (c cfgSrcMock) EndSession(_ context.Context, _ component.SessionParams) { +func (c cfgSrcMock) EndSession(context.Context, component.SessionParams) { } // TODO: Create proper factories and methods to load generic ConfigSources, temporary test helper. diff --git a/go.sum b/go.sum index 53ab801473e..7b421830932 100644 --- a/go.sum +++ b/go.sum @@ -1030,6 +1030,7 @@ go.opencensus.io v0.22.4/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw= go.opencensus.io v0.22.5/go.mod h1:5pWMHQbX5EPX2/62yrJeAkowc+lfs/XD7Uxpq3pI6kk= go.opencensus.io v0.23.0 h1:gqCw0LfLxScz8irSi8exQc7fyQ0fKQU/qnC/X8+V/1M= go.opencensus.io v0.23.0/go.mod h1:XItmlyltB5F7CS4xOC1DcqMoFqwtC6OG2xF7mCv7P7E= +go.opentelemetry.io v0.1.0 h1:EANZoRCOP+A3faIlw/iN6YEWoYb1vleZRKm1EvH8T48= go.uber.org/atomic v1.3.2/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.5.0/go.mod h1:sABNBOSYdrvTF6hTgEIbc7YasKWGhgEQZyfxyTvoXHQ= From 00817b5c23173fea3c7c90b0b7dbaa1be66162ee Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Tue, 16 Mar 2021 23:55:52 +0000 Subject: [PATCH 09/19] Make configmodels.ConfigSource internal --- .../internal/configsource/component/configsource.go | 2 +- .../configsource}/configmodels/configsource.go | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) rename config/{ => internal/configsource}/configmodels/configsource.go (86%) diff --git a/config/internal/configsource/component/configsource.go b/config/internal/configsource/component/configsource.go index e40171e94fb..474970ed011 100644 --- a/config/internal/configsource/component/configsource.go +++ b/config/internal/configsource/component/configsource.go @@ -20,7 +20,7 @@ import ( "go.uber.org/zap" "go.opentelemetry.io/collector/component" - "go.opentelemetry.io/collector/config/configmodels" + "go.opentelemetry.io/collector/config/internal/configsource/configmodels" ) // SessionParams is passed to ConfigSource at the begin and end of diff --git a/config/configmodels/configsource.go b/config/internal/configsource/configmodels/configsource.go similarity index 86% rename from config/configmodels/configsource.go rename to config/internal/configsource/configmodels/configsource.go index e5e875a6505..a2f24b25d92 100644 --- a/config/configmodels/configsource.go +++ b/config/internal/configsource/configmodels/configsource.go @@ -14,11 +14,15 @@ package configmodels +import ( + "go.opentelemetry.io/collector/config/configmodels" +) + // ConfigSource is the configuration of a config source. Specific configuration // sources must implement this interface and will typically embed ConfigSourceSettings // or a struct that extends it. type ConfigSource interface { - NamedEntity + configmodels.NamedEntity } // ConfigSources is a map of names to ConfigSource instances. @@ -27,8 +31,8 @@ type ConfigSources map[string]ConfigSource // ConfigSourceSettings defines common settings for a ConfigSource configuration. // Specific ConfigSource types can embed this struct and extend it with more fields if needed. type ConfigSourceSettings struct { - TypeVal Type `mapstructure:"-"` - NameVal string `mapstructure:"-"` + TypeVal configmodels.Type `mapstructure:"-"` + NameVal string `mapstructure:"-"` } var _ ConfigSource = (*ConfigSourceSettings)(nil) @@ -44,6 +48,6 @@ func (css *ConfigSourceSettings) SetName(name string) { } // Type sets the ConfigSource type. -func (css *ConfigSourceSettings) Type() Type { +func (css *ConfigSourceSettings) Type() configmodels.Type { return css.TypeVal } From f0a99b18098c857d985b6e7062b381e7109da5c7 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Wed, 17 Mar 2021 03:07:35 +0000 Subject: [PATCH 10/19] Add comments and some renames --- .../configsource/component/configsource.go | 36 ++++++----- .../configsource/configmodels/configsource.go | 27 -------- config/internal/configsource/configsource.go | 61 ++++++++++++++----- .../configsource/configsource_test.go | 4 +- 4 files changed, 71 insertions(+), 57 deletions(-) diff --git a/config/internal/configsource/component/configsource.go b/config/internal/configsource/component/configsource.go index 474970ed011..49f71a62f2a 100644 --- a/config/internal/configsource/component/configsource.go +++ b/config/internal/configsource/component/configsource.go @@ -23,38 +23,46 @@ import ( "go.opentelemetry.io/collector/config/internal/configsource/configmodels" ) -// SessionParams is passed to ConfigSource at the begin and end of -// a configuration session. -type SessionParams struct { - // LoadCount tracks the number of the configuration load session. - LoadCount int -} - // ConfigSource is the interface to be implemented by objects used by the collector // to retrieve external configuration information. type ConfigSource interface { component.Component - // BeginSession signals that the object is about to be used to inject data into + // BeginSession signals that the ConfigSource is about to be used to inject data into // the configuration. ConfigSource objects can assume that there won't be // concurrent sessions and can use this call according to their needs: // lock needed resources, suspend background tasks, etc. - BeginSession(ctx context.Context, sessionParams SessionParams) error + BeginSession(ctx context.Context) error // Apply retrieves generic values given the arguments specified on a specific // invocation of a configuration source. The returned object is injected on - // configuration. + // configuration. When Apply is called params will be the value associated to + // an invocation of the ConfigSource. For example, the following YAML: + // + // $my_config_src: + // param0: true + // param1: "some string" + // + // will have a call to Apply in which params is: + // + // map[string]interface{}{ + // "param0": true, + // "param1": "some string", + // } + // + // A ConfigSource implementation should unmarshall, or cast it, as appropriate + // and report errors if it doesn't fit their usage expectation. Apply(ctx context.Context, params interface{}) (interface{}, error) // EndSession signals that the configuration was fully flattened and it // is ready to be loaded. Each ConfigSource can use this call according // to their needs: release resources, start background tasks, update // internal state, etc. - EndSession(ctx context.Context, sessionParams SessionParams) + EndSession(ctx context.Context) } -// ConfigSourceCreateParams is passed to ConfigSourceFactory.Create* functions. -type ConfigSourceCreateParams struct { +// CreateConfigSourceParams is passed to ConfigSourceFactory.CreateConfigSource function. +type CreateConfigSourceParams struct { // Logger that the factory can use during creation and can pass to the created // component to be used later as well. Logger *zap.Logger @@ -77,5 +85,5 @@ type ConfigSourceFactory interface { CreateDefaultConfig() configmodels.ConfigSource // CreateConfigSource creates a configuration source based on the given config. - CreateConfigSource(ctx context.Context, params ConfigSourceCreateParams, cfg configmodels.ConfigSource) (ConfigSource, error) + CreateConfigSource(ctx context.Context, params CreateConfigSourceParams, cfg configmodels.ConfigSource) (ConfigSource, error) } diff --git a/config/internal/configsource/configmodels/configsource.go b/config/internal/configsource/configmodels/configsource.go index a2f24b25d92..702039b8304 100644 --- a/config/internal/configsource/configmodels/configsource.go +++ b/config/internal/configsource/configmodels/configsource.go @@ -24,30 +24,3 @@ import ( type ConfigSource interface { configmodels.NamedEntity } - -// ConfigSources is a map of names to ConfigSource instances. -type ConfigSources map[string]ConfigSource - -// ConfigSourceSettings defines common settings for a ConfigSource configuration. -// Specific ConfigSource types can embed this struct and extend it with more fields if needed. -type ConfigSourceSettings struct { - TypeVal configmodels.Type `mapstructure:"-"` - NameVal string `mapstructure:"-"` -} - -var _ ConfigSource = (*ConfigSourceSettings)(nil) - -// Name gets the ConfigSource name. -func (css *ConfigSourceSettings) Name() string { - return css.NameVal -} - -// SetName sets the ConfigSource name. -func (css *ConfigSourceSettings) SetName(name string) { - css.NameVal = name -} - -// Type sets the ConfigSource type. -func (css *ConfigSourceSettings) Type() configmodels.Type { - return css.TypeVal -} diff --git a/config/internal/configsource/configsource.go b/config/internal/configsource/configsource.go index 8dd9ec8bb4d..01edafea511 100644 --- a/config/internal/configsource/configsource.go +++ b/config/internal/configsource/configsource.go @@ -28,7 +28,7 @@ import ( ) const ( - // ConfigSourcePrefix is used to identified a configuration source invocation. + // ConfigSourcePrefix is used to identify a configuration source invocation. ConfigSourcePrefix = "$" ) @@ -40,8 +40,6 @@ type ApplyConfigSourcesParams struct { // MaxRecursionDepth limits the maximum number of times that a configuration source // can inject another into the config. MaxRecursionDepth uint - // LoadCount tracks the number of the configuration load session. - LoadCount int } // ApplyConfigSources takes a viper object with the configuration to which the @@ -49,13 +47,9 @@ type ApplyConfigSourcesParams struct { // returned as a separated object. func ApplyConfigSources(ctx context.Context, v *viper.Viper, params ApplyConfigSourcesParams) (*viper.Viper, error) { - sessionParams := component.SessionParams{ - LoadCount: params.LoadCount, - } - // Notify all sources that we are about to start. for cfgSrcName, cfgSrc := range params.ConfigSources { - if err := cfgSrc.BeginSession(ctx, sessionParams); err != nil { + if err := cfgSrc.BeginSession(ctx); err != nil { return nil, &cfgSrcError{ msg: fmt.Sprintf("config source %s begin session error: %v", cfgSrcName, err), code: errCfgSrcBeginSession, @@ -64,7 +58,7 @@ func ApplyConfigSources(ctx context.Context, v *viper.Viper, params ApplyConfigS // The scope of usage of the cfgSrc is the whole function so it is fine to defer // the EndSession call to the function exit. - defer cfgSrc.EndSession(ctx, sessionParams) + defer cfgSrc.EndSession(ctx) } srcCfg := v @@ -122,10 +116,23 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou done := true appliedTags := make(map[string]struct{}) - // Apply tags from the deepest config sources to the lowest so nested invocations are properly resolved. + // It is possible to have config sources injection depending on other config sources: + // + // $lower_cfgsrc: + // a: "just an example" + // b: + // $deeper_cfgsrc: + // c: true + // + // By injecting the deepest ones first they can be resolved in the proper order. + // See function deepestConfigSourcesFirst for more info. + // + // TODO: Bug when lower injection happening after deeper but without the inject values. allKeys := srcCfg.AllKeys() sort.Slice(allKeys, deepestConfigSourcesFirst(allKeys)) + // Inspect all key from original configuration and set the proper value on the + // destination config. for _, k := range allKeys { dstKey, cfgSrcName, paramsKey := extractCfgSrcInvocation(k) if cfgSrcName == "" { @@ -158,6 +165,8 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou } // The injection may require further expansion, assume that we are not done yet. + // This is pessimistic, alternatively we could explore the injected configuration + // to check if it injected other configuration sources or not. done = false if dstKey != "" { dstCfg.Set(dstKey, actualCfg) @@ -180,6 +189,23 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou return done, nil } +// deepestConfigSourcesFirst function returns a compare function to be used +// with slice.Sort to ensure that the "deepest" config source invocation appears +// first in the sorted slice. The configuration: +// +// $lower: +// a: "just an example" +// b: +// $deeper: +// c: true +// +// will have two keys: +// +// $lower::a +// $lower::b::$deeper::c +// +// by using deepestConfigSourcesFirst the sorted slice will have "$lower::b::$deeper::c" +// before "$lower::a". func deepestConfigSourcesFirst(keys []string) func(int, int) bool { return func(i, j int) bool { iLastSrcIdx := strings.LastIndex(keys[i], ConfigSourcePrefix) @@ -197,15 +223,22 @@ func deepestConfigSourcesFirst(keys []string) func(int, int) bool { } } +// extractCfgSrcInvocation breaks down a key from the configuration if it contains the ConfigSourcePrefix. +// If the key contains the prefix, the return values are as follows: +// +// - dstKey: the key into which the result applying the config source will be injected. +// - cfgSrcName: the name of the config source to be applied. +// - paramsKey: the key of the parameters to be passed on the call to the config source Apply method. +// +// In case the prefix is not present on the key all returned strings have their default value. func extractCfgSrcInvocation(k string) (dstKey, cfgSrcName, paramsKey string) { - firstPrefixIdx := strings.Index(k, ConfigSourcePrefix) - if firstPrefixIdx == -1 { + // Check for the deepest config source prefix. + tagPrefixIdx := strings.LastIndex(k, ConfigSourcePrefix) + if tagPrefixIdx == -1 { // No config source to be applied. return } - // Check for a deeper one. - tagPrefixIdx := strings.LastIndex(k, ConfigSourcePrefix) dstKey = strings.TrimSuffix(k[:tagPrefixIdx], config.ViperDelimiter) cfgSrcFromStart := k[tagPrefixIdx+len(ConfigSourcePrefix):] diff --git a/config/internal/configsource/configsource_test.go b/config/internal/configsource/configsource_test.go index c145150e4bd..56c6bac0d49 100644 --- a/config/internal/configsource/configsource_test.go +++ b/config/internal/configsource/configsource_test.go @@ -400,7 +400,7 @@ func (c cfgSrcMock) Shutdown(context.Context) error { return nil } -func (c cfgSrcMock) BeginSession(context.Context, component.SessionParams) error { +func (c cfgSrcMock) BeginSession(context.Context) error { if c.ErrOnBeginSession { return errors.New("request to error on BeginSession") } @@ -424,7 +424,7 @@ func (c cfgSrcMock) Apply(_ context.Context, params interface{}) (interface{}, e return nil, nil } -func (c cfgSrcMock) EndSession(context.Context, component.SessionParams) { +func (c cfgSrcMock) EndSession(context.Context) { } // TODO: Create proper factories and methods to load generic ConfigSources, temporary test helper. From 2d3785ca310db98d404337a55b65b93733ca702d Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Wed, 17 Mar 2021 07:21:21 +0000 Subject: [PATCH 11/19] Fix deepestConfigSourcesFirst Fix whitespace --- config/internal/configsource/configsource.go | 37 +++++++++++++------ .../configsource/configsource_test.go | 9 +++++ 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/config/internal/configsource/configsource.go b/config/internal/configsource/configsource.go index 01edafea511..58a5e68a91f 100644 --- a/config/internal/configsource/configsource.go +++ b/config/internal/configsource/configsource.go @@ -115,7 +115,7 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou done := true appliedTags := make(map[string]struct{}) - + // It is possible to have config sources injection depending on other config sources: // // $lower_cfgsrc: @@ -131,8 +131,7 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou allKeys := srcCfg.AllKeys() sort.Slice(allKeys, deepestConfigSourcesFirst(allKeys)) - // Inspect all key from original configuration and set the proper value on the - // destination config. + // Inspect all key from original configuration and set the proper value on the destination config. for _, k := range allKeys { dstKey, cfgSrcName, paramsKey := extractCfgSrcInvocation(k) if cfgSrcName == "" { @@ -142,7 +141,8 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou } if _, ok := appliedTags[dstKey]; ok { - // Already applied this key. + // Already applied this key. If the config source has multiple parameters + // there will be multiple keys for the same application. continue } @@ -189,7 +189,7 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou return done, nil } -// deepestConfigSourcesFirst function returns a compare function to be used +// deepestConfigSourcesFirst function returns a "less" function to be used // with slice.Sort to ensure that the "deepest" config source invocation appears // first in the sorted slice. The configuration: // @@ -208,19 +208,34 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou // before "$lower::a". func deepestConfigSourcesFirst(keys []string) func(int, int) bool { return func(i, j int) bool { - iLastSrcIdx := strings.LastIndex(keys[i], ConfigSourcePrefix) - jLastSrcIdx := strings.LastIndex(keys[j], ConfigSourcePrefix) + iBranch := strings.Split(keys[i], config.ViperDelimiter) + jBranch := strings.Split(keys[j], config.ViperDelimiter) + + iLastSrcIdx := lastIndexConfigSource(iBranch) + jLastSrcIdx := lastIndexConfigSource(jBranch) + if iLastSrcIdx != jLastSrcIdx { - // At least one of the keys has a config source. + // Both have at least one of the keys has a config source. + // Return the deepest one. return iLastSrcIdx > jLastSrcIdx } // It can be one of the following cases: // 1. None has a config source invocation - // 2. Both have at the same level, ie.: they are siblings in the config. - // In any case it doesn't matter which one is considered "smaller". - return true + // 2. Only one has a config source + // Return the deepest "branch". + return len(iBranch) > len(jBranch) + } +} + +func lastIndexConfigSource(keyNodes []string) int { + lastIndexConfigSource := -1 + for i, node := range keyNodes { + if strings.HasPrefix(node, ConfigSourcePrefix) { + lastIndexConfigSource = i + } } + return lastIndexConfigSource } // extractCfgSrcInvocation breaks down a key from the configuration if it contains the ConfigSourcePrefix. diff --git a/config/internal/configsource/configsource_test.go b/config/internal/configsource/configsource_test.go index 56c6bac0d49..a1b8778f712 100644 --- a/config/internal/configsource/configsource_test.go +++ b/config/internal/configsource/configsource_test.go @@ -18,6 +18,7 @@ import ( "context" "errors" "path" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -294,6 +295,14 @@ func Test_deepestConfigSourcesFirst(t *testing.T) { }, expected: false, }, + { + name: "left_deepest_not_by_length", + keys: []string{ + concatKeys("a", strings.Repeat("a", 100), cfgSrcKey("b")), + concatKeys("a", "a", "a", cfgSrcKey("b")), + }, + expected: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 5bd5266d37dd51615d89511c16a9b2a98a3b74e8 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Wed, 17 Mar 2021 07:29:05 +0000 Subject: [PATCH 12/19] Fix whitespace --- config/internal/configsource/configsource.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/internal/configsource/configsource.go b/config/internal/configsource/configsource.go index 58a5e68a91f..deff33a1ace 100644 --- a/config/internal/configsource/configsource.go +++ b/config/internal/configsource/configsource.go @@ -115,7 +115,7 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou done := true appliedTags := make(map[string]struct{}) - + // It is possible to have config sources injection depending on other config sources: // // $lower_cfgsrc: From 36b27273fbeadb465bf884c7f86e47983966940c Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Wed, 17 Mar 2021 19:08:07 +0000 Subject: [PATCH 13/19] Do not allow nested config source invocations --- config/internal/configsource/configsource.go | 17 +++++++ .../configsource/configsource_test.go | 51 ++++++++++--------- .../configsource/testdata/nested.yaml | 5 ++ .../testdata/nested_expected.yaml | 2 + .../configsource/testdata/nested_sources.yaml | 6 +++ .../configsource/testdata/nil_param.yaml | 2 + .../testdata/nil_param_expected.yaml | 1 + .../testdata/nil_param_sources.yaml | 3 ++ 8 files changed, 62 insertions(+), 25 deletions(-) create mode 100644 config/internal/configsource/testdata/nested.yaml create mode 100644 config/internal/configsource/testdata/nested_expected.yaml create mode 100644 config/internal/configsource/testdata/nested_sources.yaml create mode 100644 config/internal/configsource/testdata/nil_param.yaml create mode 100644 config/internal/configsource/testdata/nil_param_expected.yaml create mode 100644 config/internal/configsource/testdata/nil_param_sources.yaml diff --git a/config/internal/configsource/configsource.go b/config/internal/configsource/configsource.go index deff33a1ace..d7c7fd0c9a0 100644 --- a/config/internal/configsource/configsource.go +++ b/config/internal/configsource/configsource.go @@ -106,6 +106,7 @@ const ( errCfgSrcBeginSession errCfgSrcNotFound errCfgSrcApply + errNestedCfgSrc ) func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSources map[string]component.ConfigSource) (bool, error) { @@ -134,6 +135,22 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou // Inspect all key from original configuration and set the proper value on the destination config. for _, k := range allKeys { dstKey, cfgSrcName, paramsKey := extractCfgSrcInvocation(k) + + // Nested injection is not supported. That is the following is not supported: + // + // $lower: + // lower_param0: false + // lower_param1: + // $deeper: + // key: deeperkey + // + if _, parentCfgSrcName, _ := extractCfgSrcInvocation(dstKey); parentCfgSrcName != "" { + return false, &cfgSrcError{ + msg: fmt.Sprintf("nested config source usage at %s this is not supported", paramsKey), + code: errNestedCfgSrc, + } + } + if cfgSrcName == "" { // Nothing to apply take the key and value as it is. dstCfg.Set(k, srcCfg.Get(k)) diff --git a/config/internal/configsource/configsource_test.go b/config/internal/configsource/configsource_test.go index a1b8778f712..54ca2fba7e8 100644 --- a/config/internal/configsource/configsource_test.go +++ b/config/internal/configsource/configsource_test.go @@ -35,9 +35,16 @@ func TestApplyConfigSources(t *testing.T) { cfgSrcErrorCode cfgSrcErrorCode applyParams ApplyConfigSourcesParams }{ + { + name: "nil_param", + }, { name: "simple", }, + { + name: "nested", + cfgSrcErrorCode: errNestedCfgSrc, + }, { name: "err_begin_session", cfgSrcErrorCode: errCfgSrcBeginSession, @@ -142,31 +149,6 @@ func Test_applyConfigSources_no_error(t *testing.T) { }, }, }, - { - name: "nested", - srcCfg: map[string]interface{}{ - "a": map[string]interface{}{ - cfgSrcKey("cfgsrc"): map[string]interface{}{ - "b": map[string]interface{}{ - cfgSrcKey("cfgsrc"): map[string]interface{}{ - "key": "bool", - "int": 1, - "map": map[string]interface{}{ - "str": "force_already_visied_cfgsrc", - }, - }, - }, - }, - }, - }, - dstCfg: map[string]interface{}{ - "a": map[string]interface{}{ - cfgSrcKey("cfgsrc"): map[string]interface{}{ - "b": true, - }, - }, - }, - }, } for _, tt := range tests { @@ -226,6 +208,25 @@ func Test_applyConfigSources_error(t *testing.T) { }, errCode: errOnlyMapAtRootLevel, }, + { + name: "nested_cfgsrc", + srcCfg: map[string]interface{}{ + "a": map[string]interface{}{ + cfgSrcKey("cfgsrc"): map[string]interface{}{ + "b": map[string]interface{}{ + cfgSrcKey("cfgsrc"): map[string]interface{}{ + "key": "bool", + "int": 1, + "map": map[string]interface{}{ + "str": "force_already_visied_cfgsrc", + }, + }, + }, + }, + }, + }, + errCode: errNestedCfgSrc, + }, } for _, tt := range tests { diff --git a/config/internal/configsource/testdata/nested.yaml b/config/internal/configsource/testdata/nested.yaml new file mode 100644 index 00000000000..19eb9bc3fde --- /dev/null +++ b/config/internal/configsource/testdata/nested.yaml @@ -0,0 +1,5 @@ +$lower: + err_begin_session: false # Just to force a configuration key at this point. + key: + $deeper: + key: deeperkey diff --git a/config/internal/configsource/testdata/nested_expected.yaml b/config/internal/configsource/testdata/nested_expected.yaml new file mode 100644 index 00000000000..5a2ed115cd5 --- /dev/null +++ b/config/internal/configsource/testdata/nested_expected.yaml @@ -0,0 +1,2 @@ +$lower: + lowerkey diff --git a/config/internal/configsource/testdata/nested_sources.yaml b/config/internal/configsource/testdata/nested_sources.yaml new file mode 100644 index 00000000000..6a7776079f4 --- /dev/null +++ b/config/internal/configsource/testdata/nested_sources.yaml @@ -0,0 +1,6 @@ +lower: + keys: + lowerkey: lowervalue +deeper: + keys: + deeperkey: lowerkey diff --git a/config/internal/configsource/testdata/nil_param.yaml b/config/internal/configsource/testdata/nil_param.yaml new file mode 100644 index 00000000000..753f8f30018 --- /dev/null +++ b/config/internal/configsource/testdata/nil_param.yaml @@ -0,0 +1,2 @@ +a: + $cfgsrc: diff --git a/config/internal/configsource/testdata/nil_param_expected.yaml b/config/internal/configsource/testdata/nil_param_expected.yaml new file mode 100644 index 00000000000..da4118b9774 --- /dev/null +++ b/config/internal/configsource/testdata/nil_param_expected.yaml @@ -0,0 +1 @@ +a: nil_param \ No newline at end of file diff --git a/config/internal/configsource/testdata/nil_param_sources.yaml b/config/internal/configsource/testdata/nil_param_sources.yaml new file mode 100644 index 00000000000..3cc9b154424 --- /dev/null +++ b/config/internal/configsource/testdata/nil_param_sources.yaml @@ -0,0 +1,3 @@ +cfgsrc: + keys: + not: used From b463f247699c87bd23ff5b47bf33ee9d8023dea6 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Thu, 18 Mar 2021 04:44:20 +0000 Subject: [PATCH 14/19] Undo accidental change to go.sum --- go.sum | 1 - 1 file changed, 1 deletion(-) diff --git a/go.sum b/go.sum index 7b421830932..53ab801473e 100644 --- a/go.sum +++ b/go.sum @@ -1030,7 +1030,6 @@ go.opencensus.io v0.22.4/go.mod h1:yxeiOL68Rb0Xd1ddK5vPZ/oVn4vY4Ynel7k9FzqtOIw= go.opencensus.io v0.22.5/go.mod h1:5pWMHQbX5EPX2/62yrJeAkowc+lfs/XD7Uxpq3pI6kk= go.opencensus.io v0.23.0 h1:gqCw0LfLxScz8irSi8exQc7fyQ0fKQU/qnC/X8+V/1M= go.opencensus.io v0.23.0/go.mod h1:XItmlyltB5F7CS4xOC1DcqMoFqwtC6OG2xF7mCv7P7E= -go.opentelemetry.io v0.1.0 h1:EANZoRCOP+A3faIlw/iN6YEWoYb1vleZRKm1EvH8T48= go.uber.org/atomic v1.3.2/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.4.0/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/atomic v1.5.0/go.mod h1:sABNBOSYdrvTF6hTgEIbc7YasKWGhgEQZyfxyTvoXHQ= From 2539389d48abb0e71e2a19fdb6ea06df8731f476 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Thu, 18 Mar 2021 05:09:21 +0000 Subject: [PATCH 15/19] PR Feedback --- .../configsource/component/configsource.go | 33 ++++++++++++------- config/internal/configsource/configsource.go | 10 +++--- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/config/internal/configsource/component/configsource.go b/config/internal/configsource/component/configsource.go index 49f71a62f2a..718c046e98b 100644 --- a/config/internal/configsource/component/configsource.go +++ b/config/internal/configsource/component/configsource.go @@ -29,34 +29,43 @@ type ConfigSource interface { component.Component // BeginSession signals that the ConfigSource is about to be used to inject data into - // the configuration. ConfigSource objects can assume that there won't be - // concurrent sessions and can use this call according to their needs: - // lock needed resources, suspend background tasks, etc. + // the configuration. The difference between BeginSession and the component.Start method + // is that the latter is used by the host to manage the life-time of a ConfigSource and to + // provide a way for a ConfigSource to have a reference to the host so it can notify the + // host when configuration changes are detected. + // + // A ConfigSource should use the BeginSession call according to their needs: + // lock resources, suspend background or watcher tasks, etc. An implementation, for + // instance, can use the begin of a session to prevent torn configurations, by acquiring + // a lock (or some other mechanism) that prevents concurrent changes to the configura during + // the middle of a session. + // + // The code managing the session must guarantee that no ConfigSource instance participates + // in concurrent sessions. BeginSession(ctx context.Context) error - // Apply retrieves generic values given the arguments specified on a specific - // invocation of a configuration source. The returned object is injected on - // configuration. When Apply is called params will be the value associated to - // an invocation of the ConfigSource. For example, the following YAML: + // Apply goes to the configuration source, sending the given parameters as a map + // and returns the resulting configuration value or snippet. A configuration snippet + // will become a map, as follows: // // $my_config_src: // param0: true // param1: "some string" // - // will have a call to Apply in which params is: + // Becomes a call with the following payload as params: // // map[string]interface{}{ // "param0": true, // "param1": "some string", // } // - // A ConfigSource implementation should unmarshall, or cast it, as appropriate - // and report errors if it doesn't fit their usage expectation. + // A ConfigSource should then unmarshal or cast the params. An error should be + // returned when the params don't fit the expected usage. Apply(ctx context.Context, params interface{}) (interface{}, error) // EndSession signals that the configuration was fully flattened and it - // is ready to be loaded. Each ConfigSource can use this call according - // to their needs: release resources, start background tasks, update + // is ready to be loaded. Each ConfigSource should use this call according + // to their needs: release resources, start background or watcher tasks, update // internal state, etc. EndSession(ctx context.Context) } diff --git a/config/internal/configsource/configsource.go b/config/internal/configsource/configsource.go index d7c7fd0c9a0..a3044991353 100644 --- a/config/internal/configsource/configsource.go +++ b/config/internal/configsource/configsource.go @@ -51,7 +51,7 @@ func ApplyConfigSources(ctx context.Context, v *viper.Viper, params ApplyConfigS for cfgSrcName, cfgSrc := range params.ConfigSources { if err := cfgSrc.BeginSession(ctx); err != nil { return nil, &cfgSrcError{ - msg: fmt.Sprintf("config source %s begin session error: %v", cfgSrcName, err), + msg: fmt.Sprintf("config source %q begin session error: %v", cfgSrcName, err), code: errCfgSrcBeginSession, } } @@ -77,7 +77,7 @@ func ApplyConfigSources(ctx context.Context, v *viper.Viper, params ApplyConfigS if !done { return nil, &cfgSrcError{ - msg: "config source recursion chain is too deep, couldn't fully expand the configuration", + msg: fmt.Sprintf("config source recursion chain is deeper than the allowed maximum of %d", params.MaxRecursionDepth), code: errCfgSrcChainTooLong, } } @@ -146,7 +146,7 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou // if _, parentCfgSrcName, _ := extractCfgSrcInvocation(dstKey); parentCfgSrcName != "" { return false, &cfgSrcError{ - msg: fmt.Sprintf("nested config source usage at %s this is not supported", paramsKey), + msg: fmt.Sprintf("nested config source usage at %q this is not supported", paramsKey), code: errNestedCfgSrc, } } @@ -167,7 +167,7 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou cfgSrc, ok := cfgSources[cfgSrcName] if !ok { return false, &cfgSrcError{ - msg: fmt.Sprintf("config source %s not found", cfgSrcName), + msg: fmt.Sprintf("config source %q not found", cfgSrcName), code: errCfgSrcNotFound, } } @@ -176,7 +176,7 @@ func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSou actualCfg, err := cfgSrc.Apply(ctx, applyParams) if err != nil { return false, &cfgSrcError{ - msg: fmt.Sprintf("error applying config source %s: %v", cfgSrcName, err), + msg: fmt.Sprintf("error applying config source %q: %v", cfgSrcName, err), code: errCfgSrcApply, } } From 1d9a4ca5934a3bc334381948fa62c9b5da898509 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Mon, 22 Mar 2021 19:34:13 +0000 Subject: [PATCH 16/19] Reducing scope: keeping only config source interfaces --- config/internal/configsource/configsource.go | 332 ------------- .../configsource/configsource_test.go | 454 ------------------ .../testdata/err_begin_session.yaml | 4 - .../testdata/err_begin_session_sources.yaml | 2 - .../configsource/testdata/multiple.yaml | 5 - .../testdata/multiple_expected.yaml | 4 - .../testdata/multiple_sources.yaml | 7 - .../configsource/testdata/nested.yaml | 5 - .../testdata/nested_expected.yaml | 2 - .../configsource/testdata/nested_sources.yaml | 6 - .../configsource/testdata/nil_param.yaml | 2 - .../testdata/nil_param_expected.yaml | 1 - .../testdata/nil_param_sources.yaml | 3 - .../configsource/testdata/recursion.yaml | 4 - .../testdata/recursion_sources.yaml | 6 - .../configsource/testdata/simple.yaml | 4 - .../testdata/simple_expected.yaml | 2 - .../configsource/testdata/simple_sources.yaml | 3 - 18 files changed, 846 deletions(-) delete mode 100644 config/internal/configsource/configsource.go delete mode 100644 config/internal/configsource/configsource_test.go delete mode 100644 config/internal/configsource/testdata/err_begin_session.yaml delete mode 100644 config/internal/configsource/testdata/err_begin_session_sources.yaml delete mode 100644 config/internal/configsource/testdata/multiple.yaml delete mode 100644 config/internal/configsource/testdata/multiple_expected.yaml delete mode 100644 config/internal/configsource/testdata/multiple_sources.yaml delete mode 100644 config/internal/configsource/testdata/nested.yaml delete mode 100644 config/internal/configsource/testdata/nested_expected.yaml delete mode 100644 config/internal/configsource/testdata/nested_sources.yaml delete mode 100644 config/internal/configsource/testdata/nil_param.yaml delete mode 100644 config/internal/configsource/testdata/nil_param_expected.yaml delete mode 100644 config/internal/configsource/testdata/nil_param_sources.yaml delete mode 100644 config/internal/configsource/testdata/recursion.yaml delete mode 100644 config/internal/configsource/testdata/recursion_sources.yaml delete mode 100644 config/internal/configsource/testdata/simple.yaml delete mode 100644 config/internal/configsource/testdata/simple_expected.yaml delete mode 100644 config/internal/configsource/testdata/simple_sources.yaml diff --git a/config/internal/configsource/configsource.go b/config/internal/configsource/configsource.go deleted file mode 100644 index a3044991353..00000000000 --- a/config/internal/configsource/configsource.go +++ /dev/null @@ -1,332 +0,0 @@ -// 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 configsource - -import ( - "context" - "fmt" - "os" - "sort" - "strings" - - "github.com/spf13/viper" - - "go.opentelemetry.io/collector/config" - "go.opentelemetry.io/collector/config/internal/configsource/component" -) - -const ( - // ConfigSourcePrefix is used to identify a configuration source invocation. - ConfigSourcePrefix = "$" -) - -// ApplyConfigSourcesParams holds the parameters for injecting data from the -// given configuration sources into a configuration. -type ApplyConfigSourcesParams struct { - // Set of configuration sources available to inject data into the configuration. - ConfigSources map[string]component.ConfigSource - // MaxRecursionDepth limits the maximum number of times that a configuration source - // can inject another into the config. - MaxRecursionDepth uint -} - -// ApplyConfigSources takes a viper object with the configuration to which the -// the configuration sources will be applied and the resulting configuration is -// returned as a separated object. -func ApplyConfigSources(ctx context.Context, v *viper.Viper, params ApplyConfigSourcesParams) (*viper.Viper, error) { - - // Notify all sources that we are about to start. - for cfgSrcName, cfgSrc := range params.ConfigSources { - if err := cfgSrc.BeginSession(ctx); err != nil { - return nil, &cfgSrcError{ - msg: fmt.Sprintf("config source %q begin session error: %v", cfgSrcName, err), - code: errCfgSrcBeginSession, - } - } - - // The scope of usage of the cfgSrc is the whole function so it is fine to defer - // the EndSession call to the function exit. - defer cfgSrc.EndSession(ctx) - } - - srcCfg := v - var dstCfg *viper.Viper - - var err error - done := false - for i := -1; i <= int(params.MaxRecursionDepth) && !done; i++ { - dstCfg = config.NewViper() - done, err = applyConfigSources(ctx, srcCfg, dstCfg, params.ConfigSources) - if err != nil { - return nil, err - } - srcCfg = dstCfg - } - - if !done { - return nil, &cfgSrcError{ - msg: fmt.Sprintf("config source recursion chain is deeper than the allowed maximum of %d", params.MaxRecursionDepth), - code: errCfgSrcChainTooLong, - } - } - - return dstCfg, nil -} - -// cfgSrcError private error type used to accurate identify the type of error in tests. -type cfgSrcError struct { - msg string // human readable error message. - code cfgSrcErrorCode // internal error code. -} - -func (e *cfgSrcError) Error() string { - return e.msg -} - -// These are errors that can be returned by ApplyConfigSources function. -// Note that error codes are not part public API, they are for unit testing only. -type cfgSrcErrorCode int - -const ( - _ cfgSrcErrorCode = iota // skip 0, start errors codes from 1. - errCfgSrcChainTooLong - errOnlyMapAtRootLevel - errCfgSrcBeginSession - errCfgSrcNotFound - errCfgSrcApply - errNestedCfgSrc -) - -func applyConfigSources(ctx context.Context, srcCfg, dstCfg *viper.Viper, cfgSources map[string]component.ConfigSource) (bool, error) { - // Expand any item env vars in the config, do it every time so env vars - // added on previous pass are also handled. - expandEnvConfig(srcCfg) - - done := true - appliedTags := make(map[string]struct{}) - - // It is possible to have config sources injection depending on other config sources: - // - // $lower_cfgsrc: - // a: "just an example" - // b: - // $deeper_cfgsrc: - // c: true - // - // By injecting the deepest ones first they can be resolved in the proper order. - // See function deepestConfigSourcesFirst for more info. - // - // TODO: Bug when lower injection happening after deeper but without the inject values. - allKeys := srcCfg.AllKeys() - sort.Slice(allKeys, deepestConfigSourcesFirst(allKeys)) - - // Inspect all key from original configuration and set the proper value on the destination config. - for _, k := range allKeys { - dstKey, cfgSrcName, paramsKey := extractCfgSrcInvocation(k) - - // Nested injection is not supported. That is the following is not supported: - // - // $lower: - // lower_param0: false - // lower_param1: - // $deeper: - // key: deeperkey - // - if _, parentCfgSrcName, _ := extractCfgSrcInvocation(dstKey); parentCfgSrcName != "" { - return false, &cfgSrcError{ - msg: fmt.Sprintf("nested config source usage at %q this is not supported", paramsKey), - code: errNestedCfgSrc, - } - } - - if cfgSrcName == "" { - // Nothing to apply take the key and value as it is. - dstCfg.Set(k, srcCfg.Get(k)) - continue - } - - if _, ok := appliedTags[dstKey]; ok { - // Already applied this key. If the config source has multiple parameters - // there will be multiple keys for the same application. - continue - } - - appliedTags[dstKey] = struct{}{} - cfgSrc, ok := cfgSources[cfgSrcName] - if !ok { - return false, &cfgSrcError{ - msg: fmt.Sprintf("config source %q not found", cfgSrcName), - code: errCfgSrcNotFound, - } - } - - applyParams := srcCfg.Get(paramsKey) - actualCfg, err := cfgSrc.Apply(ctx, applyParams) - if err != nil { - return false, &cfgSrcError{ - msg: fmt.Sprintf("error applying config source %q: %v", cfgSrcName, err), - code: errCfgSrcApply, - } - } - - // The injection may require further expansion, assume that we are not done yet. - // This is pessimistic, alternatively we could explore the injected configuration - // to check if it injected other configuration sources or not. - done = false - if dstKey != "" { - dstCfg.Set(dstKey, actualCfg) - continue - } - - // This is at the root level, have to inject the top keys one by one. - rootMap, ok := actualCfg.(map[string]interface{}) - if !ok { - return false, &cfgSrcError{ - msg: "only a map can be injected at the root level", - code: errOnlyMapAtRootLevel, - } - } - for k, v := range rootMap { - dstCfg.Set(k, v) - } - } - - return done, nil -} - -// deepestConfigSourcesFirst function returns a "less" function to be used -// with slice.Sort to ensure that the "deepest" config source invocation appears -// first in the sorted slice. The configuration: -// -// $lower: -// a: "just an example" -// b: -// $deeper: -// c: true -// -// will have two keys: -// -// $lower::a -// $lower::b::$deeper::c -// -// by using deepestConfigSourcesFirst the sorted slice will have "$lower::b::$deeper::c" -// before "$lower::a". -func deepestConfigSourcesFirst(keys []string) func(int, int) bool { - return func(i, j int) bool { - iBranch := strings.Split(keys[i], config.ViperDelimiter) - jBranch := strings.Split(keys[j], config.ViperDelimiter) - - iLastSrcIdx := lastIndexConfigSource(iBranch) - jLastSrcIdx := lastIndexConfigSource(jBranch) - - if iLastSrcIdx != jLastSrcIdx { - // Both have at least one of the keys has a config source. - // Return the deepest one. - return iLastSrcIdx > jLastSrcIdx - } - - // It can be one of the following cases: - // 1. None has a config source invocation - // 2. Only one has a config source - // Return the deepest "branch". - return len(iBranch) > len(jBranch) - } -} - -func lastIndexConfigSource(keyNodes []string) int { - lastIndexConfigSource := -1 - for i, node := range keyNodes { - if strings.HasPrefix(node, ConfigSourcePrefix) { - lastIndexConfigSource = i - } - } - return lastIndexConfigSource -} - -// extractCfgSrcInvocation breaks down a key from the configuration if it contains the ConfigSourcePrefix. -// If the key contains the prefix, the return values are as follows: -// -// - dstKey: the key into which the result applying the config source will be injected. -// - cfgSrcName: the name of the config source to be applied. -// - paramsKey: the key of the parameters to be passed on the call to the config source Apply method. -// -// In case the prefix is not present on the key all returned strings have their default value. -func extractCfgSrcInvocation(k string) (dstKey, cfgSrcName, paramsKey string) { - // Check for the deepest config source prefix. - tagPrefixIdx := strings.LastIndex(k, ConfigSourcePrefix) - if tagPrefixIdx == -1 { - // No config source to be applied. - return - } - - dstKey = strings.TrimSuffix(k[:tagPrefixIdx], config.ViperDelimiter) - - cfgSrcFromStart := k[tagPrefixIdx+len(ConfigSourcePrefix):] - prefixToTagEndLen := strings.Index(cfgSrcFromStart, config.ViperDelimiter) - if prefixToTagEndLen > -1 { - cfgSrcName = cfgSrcFromStart[:prefixToTagEndLen] - } else { - cfgSrcName = cfgSrcFromStart - } - - paramsKey = k[:tagPrefixIdx+len(cfgSrcName)+len(ConfigSourcePrefix)] - - return -} - -// Copied from config package to avoid exposing as public API. -// TODO: Add local tests covering the code below. - -// expandEnvConfig creates a new viper config with expanded values for all the values (simple, list or map value). -// It does not expand the keys. -func expandEnvConfig(v *viper.Viper) { - for _, k := range v.AllKeys() { - v.Set(k, expandStringValues(v.Get(k))) - } -} - -func expandStringValues(value interface{}) interface{} { - switch v := value.(type) { - default: - return v - case string: - return expandEnv(v) - case []interface{}: - nslice := make([]interface{}, 0, len(v)) - for _, vint := range v { - nslice = append(nslice, expandStringValues(vint)) - } - return nslice - case map[interface{}]interface{}: - nmap := make(map[interface{}]interface{}, len(v)) - for k, vint := range v { - nmap[k] = expandStringValues(vint) - } - return nmap - } -} - -func expandEnv(s string) string { - return os.Expand(s, func(str string) string { - // This allows escaping environment variable substitution via $$, e.g. - // - $FOO will be substituted with env var FOO - // - $$FOO will be replaced with $FOO - // - $$$FOO will be replaced with $ + substituted env var FOO - if str == "$" { - return "$" - } - return os.Getenv(str) - }) -} diff --git a/config/internal/configsource/configsource_test.go b/config/internal/configsource/configsource_test.go deleted file mode 100644 index 54ca2fba7e8..00000000000 --- a/config/internal/configsource/configsource_test.go +++ /dev/null @@ -1,454 +0,0 @@ -// 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 configsource - -import ( - "context" - "errors" - "path" - "strings" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - publiccomponent "go.opentelemetry.io/collector/component" - "go.opentelemetry.io/collector/config" - "go.opentelemetry.io/collector/config/internal/configsource/component" -) - -func TestApplyConfigSources(t *testing.T) { - tests := []struct { - name string - cfgSrcErrorCode cfgSrcErrorCode - applyParams ApplyConfigSourcesParams - }{ - { - name: "nil_param", - }, - { - name: "simple", - }, - { - name: "nested", - cfgSrcErrorCode: errNestedCfgSrc, - }, - { - name: "err_begin_session", - cfgSrcErrorCode: errCfgSrcBeginSession, - }, - { - name: "recursion", - cfgSrcErrorCode: errCfgSrcChainTooLong, - applyParams: ApplyConfigSourcesParams{ - MaxRecursionDepth: 2, - }, - }, - { - name: "multiple", - cfgSrcErrorCode: errCfgSrcChainTooLong, - }, - { - name: "multiple", - applyParams: ApplyConfigSourcesParams{ - MaxRecursionDepth: 1, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - v := config.NewViper() - v.SetConfigFile(path.Join("testdata", tt.name+".yaml")) - require.NoError(t, v.ReadInConfig()) - - tt.applyParams.ConfigSources = loadCfgSrcs(t, path.Join("testdata", tt.name+"_sources.yaml")) - dst, err := ApplyConfigSources(context.Background(), v, tt.applyParams) - - if tt.cfgSrcErrorCode != cfgSrcErrorCode(0) { - assert.Nil(t, dst) - assert.Equal(t, tt.cfgSrcErrorCode, err.(*cfgSrcError).code) - return - } - - require.NoError(t, err) - require.NotNil(t, dst) - - expectedViper := config.NewViper() - expectedViper.SetConfigFile(path.Join("testdata", tt.name+"_expected.yaml")) - require.NoError(t, expectedViper.ReadInConfig()) - assert.Equal(t, expectedViper.AllSettings(), dst.AllSettings()) - }) - } -} - -func Test_applyConfigSources_no_error(t *testing.T) { - cfgSources := map[string]component.ConfigSource{ - "cfgsrc": &cfgSrcMock{ - Keys: map[string]interface{}{ - "int": 1, - "str": "str", - "bool": true, - "interface": map[string]interface{}{ - "i0": 0, - "str1": "1", - }, - "recurse": "$cfgsrc", - }, - }, - } - - tests := []struct { - name string - srcCfg map[string]interface{} - dstCfg map[string]interface{} - done bool - }{ - { - name: "done", - srcCfg: map[string]interface{}{"a": 0}, - dstCfg: map[string]interface{}{"a": 0}, - done: true, - }, - { - name: "root", - srcCfg: map[string]interface{}{ - cfgSrcKey("cfgsrc"): map[string]interface{}{ - "key": "interface", - }, - }, - dstCfg: map[string]interface{}{ - "i0": 0, - "str1": "1", - }, - }, - { - name: "basic", - srcCfg: map[string]interface{}{ - "a": map[string]interface{}{ - "a": "b", - "c": map[string]interface{}{cfgSrcKey("cfgsrc"): nil}, - }, - }, - dstCfg: map[string]interface{}{ - "a": map[string]interface{}{ - "a": "b", - "c": "nil_param", - }, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - srcV := config.NewViper() - require.NoError(t, srcV.MergeConfigMap(tt.srcCfg)) - - dstV := config.NewViper() - done, err := applyConfigSources(context.Background(), srcV, dstV, cfgSources) - assert.NoError(t, err) - assert.Equal(t, tt.done, done) - assert.Equal(t, tt.dstCfg, dstV.AllSettings()) - }) - } -} - -func Test_applyConfigSources_error(t *testing.T) { - cfgSources := map[string]component.ConfigSource{ - "cfgsrc": &cfgSrcMock{ - Keys: map[string]interface{}{ - "bool": true, - }, - }, - } - - tests := []struct { - name string - srcCfg map[string]interface{} - errCode cfgSrcErrorCode - }{ - { - name: "err_missing_cfgsrc", - srcCfg: map[string]interface{}{ - cfgSrcKey("cfgsrc/1"): map[string]interface{}{ - "key": "interface", - }, - }, - errCode: errCfgSrcNotFound, - }, - { - name: "err_cfgsrc_apply", - srcCfg: map[string]interface{}{ - "root": map[string]interface{}{ - cfgSrcKey("cfgsrc"): map[string]interface{}{ - "missing_key_param": "", - }, - }, - }, - errCode: errCfgSrcApply, - }, - { - name: "err_only_map_at_root", - srcCfg: map[string]interface{}{ - cfgSrcKey("cfgsrc"): map[string]interface{}{ - "key": "bool", - }, - }, - errCode: errOnlyMapAtRootLevel, - }, - { - name: "nested_cfgsrc", - srcCfg: map[string]interface{}{ - "a": map[string]interface{}{ - cfgSrcKey("cfgsrc"): map[string]interface{}{ - "b": map[string]interface{}{ - cfgSrcKey("cfgsrc"): map[string]interface{}{ - "key": "bool", - "int": 1, - "map": map[string]interface{}{ - "str": "force_already_visied_cfgsrc", - }, - }, - }, - }, - }, - }, - errCode: errNestedCfgSrc, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - srcV := config.NewViper() - require.NoError(t, srcV.MergeConfigMap(tt.srcCfg)) - - done, err := applyConfigSources(context.Background(), srcV, config.NewViper(), cfgSources) - assert.False(t, done) - require.Error(t, err) - assert.Equal(t, tt.errCode, err.(*cfgSrcError).code) - }) - } -} - -func Test_deepestConfigSourcesFirst(t *testing.T) { - tests := []struct { - name string - keys []string - expected bool - }{ - { - name: "left_cfgsrc", - keys: []string{ - concatKeys("a", "b", "c", cfgSrcKey("d"), "e"), - concatKeys("a", "b", "c", "f", "g", "h"), - }, - expected: true, - }, - { - name: "right_cfgsrc", - keys: []string{ - concatKeys("a", "b", "c", "f", "g", "h"), - concatKeys("a", "b", "c", cfgSrcKey("d"), "e"), - }, - expected: false, - }, - { - name: "left_cfgsrc_deepest", - keys: []string{ - concatKeys("a", "b", "c", "d", cfgSrcKey("e")), - concatKeys("a", "b", "c", cfgSrcKey("f"), "g"), - }, - expected: true, - }, - { - name: "right_cfgsrc_deepest", - keys: []string{ - concatKeys("a", "b", "c", cfgSrcKey("f"), "g"), - concatKeys("a", "b", "c", "d", cfgSrcKey("e")), - }, - expected: false, - }, - { - name: "left_nested_cfgsrc", - keys: []string{ - concatKeys("a", cfgSrcKey("b"), "c", "d", cfgSrcKey("e")), - concatKeys("a", cfgSrcKey("b"), "c", "f", "g"), - }, - expected: true, - }, - { - name: "right_nested_cfgsrc", - keys: []string{ - concatKeys("a", cfgSrcKey("b"), "c", "f", "g"), - concatKeys("a", cfgSrcKey("b"), "c", "d", cfgSrcKey("e")), - }, - expected: false, - }, - { - name: "left_deepest_not_by_length", - keys: []string{ - concatKeys("a", strings.Repeat("a", 100), cfgSrcKey("b")), - concatKeys("a", "a", "a", cfgSrcKey("b")), - }, - expected: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - lessFn := deepestConfigSourcesFirst(tt.keys) - assert.Equal(t, tt.expected, lessFn(0, 1)) - }) - } -} - -func Test_extractCfgSrcInvocation(t *testing.T) { - tests := []struct { - name string - dstKey string - cfgSrc string - paramsKey string - }{ - { - name: concatKeys("a"), - }, - { - name: concatKeys("a", "b"), - }, - { - name: concatKeys("simple", cfgSrcKey("cfgsrc")), - dstKey: "simple", - cfgSrc: "cfgsrc", - paramsKey: concatKeys("simple", cfgSrcKey("cfgsrc")), - }, - { - name: concatKeys("simple_a", cfgSrcKey("cfgsrc"), "a"), - dstKey: "simple_a", - cfgSrc: "cfgsrc", - paramsKey: concatKeys("simple_a", cfgSrcKey("cfgsrc")), - }, - { - name: concatKeys("nested", cfgSrcKey("top"), "a", cfgSrcKey("deepest")), - dstKey: concatKeys("nested", cfgSrcKey("top"), "a"), - cfgSrc: "deepest", - paramsKey: concatKeys("nested", cfgSrcKey("top"), "a", cfgSrcKey("deepest")), - }, - { - name: concatKeys("nested_a", cfgSrcKey("top"), "a", cfgSrcKey("deepest"), "a"), - dstKey: concatKeys("nested_a", cfgSrcKey("top"), "a"), - cfgSrc: "deepest", - paramsKey: concatKeys("nested_a", cfgSrcKey("top"), "a", cfgSrcKey("deepest")), - }, - { - name: concatKeys(cfgSrcKey("cfgSrcTopLvl")), - cfgSrc: "cfgSrcTopLvl", - paramsKey: cfgSrcKey("cfgSrcTopLvl"), - }, - { - name: concatKeys(cfgSrcKey("cfgSrcTopLvl_a"), "a"), - cfgSrc: "cfgSrcTopLvl_a", - paramsKey: cfgSrcKey("cfgSrcTopLvl_a"), - }, - { - name: concatKeys("typical", "a", "b", cfgSrcKey("cfgsrc")), - dstKey: concatKeys("typical", "a", "b"), - cfgSrc: "cfgsrc", - paramsKey: concatKeys("typical", "a", "b", cfgSrcKey("cfgsrc")), - }, - { - name: concatKeys("typical_a", "a", "b", cfgSrcKey("cfgsrc"), "a", "b"), - dstKey: concatKeys("typical_a", "a", "b"), - cfgSrc: "cfgsrc", - paramsKey: concatKeys("typical_a", "a", "b", cfgSrcKey("cfgsrc")), - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - dstKey, cfgSrc, paramsKey := extractCfgSrcInvocation(tt.name) - - assert.Equal(t, tt.dstKey, dstKey) - assert.Equal(t, tt.cfgSrc, cfgSrc) - assert.Equal(t, tt.paramsKey, paramsKey) - }) - } -} - -func concatKeys(keys ...string) (s string) { - s = keys[0] - for _, key := range keys[1:] { - s += config.ViperDelimiter + key - } - return s -} - -func cfgSrcKey(key string) string { - return ConfigSourcePrefix + key -} - -type cfgSrcMock struct { - ErrOnBeginSession bool `mapstructure:"err_begin_session"` - Keys map[string]interface{} `mapstructure:"keys"` -} - -func (c cfgSrcMock) Start(context.Context, publiccomponent.Host) error { - return nil -} - -func (c cfgSrcMock) Shutdown(context.Context) error { - return nil -} - -func (c cfgSrcMock) BeginSession(context.Context) error { - if c.ErrOnBeginSession { - return errors.New("request to error on BeginSession") - } - return nil -} - -func (c cfgSrcMock) Apply(_ context.Context, params interface{}) (interface{}, error) { - if params == nil { - return "nil_param", nil - } - switch v := params.(type) { - case string: - return v, nil - case map[string]interface{}: - keyVal, ok := v["key"] - if !ok { - return nil, errors.New("missing 'key' parameter") - } - return c.Keys[keyVal.(string)], nil - } - return nil, nil -} - -func (c cfgSrcMock) EndSession(context.Context) { -} - -// TODO: Create proper factories and methods to load generic ConfigSources, temporary test helper. -func loadCfgSrcs(t *testing.T, file string) map[string]component.ConfigSource { - v := config.NewViper() - v.SetConfigFile(file) - require.NoError(t, v.ReadInConfig()) - - cfgSrcMap := make(map[string]component.ConfigSource) - for cfgSrcName := range v.AllSettings() { - cfgSrc := &cfgSrcMock{} - require.NoError(t, v.Sub(cfgSrcName).UnmarshalExact(cfgSrc)) - cfgSrcMap[cfgSrcName] = cfgSrc - } - - return cfgSrcMap -} diff --git a/config/internal/configsource/testdata/err_begin_session.yaml b/config/internal/configsource/testdata/err_begin_session.yaml deleted file mode 100644 index 377315d2177..00000000000 --- a/config/internal/configsource/testdata/err_begin_session.yaml +++ /dev/null @@ -1,4 +0,0 @@ -simple: - test: - $cfgsrc: - key: simple diff --git a/config/internal/configsource/testdata/err_begin_session_sources.yaml b/config/internal/configsource/testdata/err_begin_session_sources.yaml deleted file mode 100644 index eb937aecea7..00000000000 --- a/config/internal/configsource/testdata/err_begin_session_sources.yaml +++ /dev/null @@ -1,2 +0,0 @@ -cfgsrc: - err_begin_session: true diff --git a/config/internal/configsource/testdata/multiple.yaml b/config/internal/configsource/testdata/multiple.yaml deleted file mode 100644 index 12c035fdba9..00000000000 --- a/config/internal/configsource/testdata/multiple.yaml +++ /dev/null @@ -1,5 +0,0 @@ -one: - $cfgsrc/0: deep_0 -two: - $cfgsrc/1: - key: recurse_to_cfgsrc/0 diff --git a/config/internal/configsource/testdata/multiple_expected.yaml b/config/internal/configsource/testdata/multiple_expected.yaml deleted file mode 100644 index 802c23c6e21..00000000000 --- a/config/internal/configsource/testdata/multiple_expected.yaml +++ /dev/null @@ -1,4 +0,0 @@ -one: - deep_0 -two: - deep_1 diff --git a/config/internal/configsource/testdata/multiple_sources.yaml b/config/internal/configsource/testdata/multiple_sources.yaml deleted file mode 100644 index f7ce6e1bf47..00000000000 --- a/config/internal/configsource/testdata/multiple_sources.yaml +++ /dev/null @@ -1,7 +0,0 @@ -cfgsrc/0: - keys: - some: value -cfgsrc/1: - keys: - recurse_to_cfgsrc/0: - $cfgsrc/0: deep_1 diff --git a/config/internal/configsource/testdata/nested.yaml b/config/internal/configsource/testdata/nested.yaml deleted file mode 100644 index 19eb9bc3fde..00000000000 --- a/config/internal/configsource/testdata/nested.yaml +++ /dev/null @@ -1,5 +0,0 @@ -$lower: - err_begin_session: false # Just to force a configuration key at this point. - key: - $deeper: - key: deeperkey diff --git a/config/internal/configsource/testdata/nested_expected.yaml b/config/internal/configsource/testdata/nested_expected.yaml deleted file mode 100644 index 5a2ed115cd5..00000000000 --- a/config/internal/configsource/testdata/nested_expected.yaml +++ /dev/null @@ -1,2 +0,0 @@ -$lower: - lowerkey diff --git a/config/internal/configsource/testdata/nested_sources.yaml b/config/internal/configsource/testdata/nested_sources.yaml deleted file mode 100644 index 6a7776079f4..00000000000 --- a/config/internal/configsource/testdata/nested_sources.yaml +++ /dev/null @@ -1,6 +0,0 @@ -lower: - keys: - lowerkey: lowervalue -deeper: - keys: - deeperkey: lowerkey diff --git a/config/internal/configsource/testdata/nil_param.yaml b/config/internal/configsource/testdata/nil_param.yaml deleted file mode 100644 index 753f8f30018..00000000000 --- a/config/internal/configsource/testdata/nil_param.yaml +++ /dev/null @@ -1,2 +0,0 @@ -a: - $cfgsrc: diff --git a/config/internal/configsource/testdata/nil_param_expected.yaml b/config/internal/configsource/testdata/nil_param_expected.yaml deleted file mode 100644 index da4118b9774..00000000000 --- a/config/internal/configsource/testdata/nil_param_expected.yaml +++ /dev/null @@ -1 +0,0 @@ -a: nil_param \ No newline at end of file diff --git a/config/internal/configsource/testdata/nil_param_sources.yaml b/config/internal/configsource/testdata/nil_param_sources.yaml deleted file mode 100644 index 3cc9b154424..00000000000 --- a/config/internal/configsource/testdata/nil_param_sources.yaml +++ /dev/null @@ -1,3 +0,0 @@ -cfgsrc: - keys: - not: used diff --git a/config/internal/configsource/testdata/recursion.yaml b/config/internal/configsource/testdata/recursion.yaml deleted file mode 100644 index e1473b36e79..00000000000 --- a/config/internal/configsource/testdata/recursion.yaml +++ /dev/null @@ -1,4 +0,0 @@ -recurse: - test: - $cfgsrc: - key: "recurse" diff --git a/config/internal/configsource/testdata/recursion_sources.yaml b/config/internal/configsource/testdata/recursion_sources.yaml deleted file mode 100644 index a8f64578422..00000000000 --- a/config/internal/configsource/testdata/recursion_sources.yaml +++ /dev/null @@ -1,6 +0,0 @@ -cfgsrc: - keys: - recurse: - test: - $cfgsrc: - key: recurse diff --git a/config/internal/configsource/testdata/simple.yaml b/config/internal/configsource/testdata/simple.yaml deleted file mode 100644 index 377315d2177..00000000000 --- a/config/internal/configsource/testdata/simple.yaml +++ /dev/null @@ -1,4 +0,0 @@ -simple: - test: - $cfgsrc: - key: simple diff --git a/config/internal/configsource/testdata/simple_expected.yaml b/config/internal/configsource/testdata/simple_expected.yaml deleted file mode 100644 index 39f79ceffdd..00000000000 --- a/config/internal/configsource/testdata/simple_expected.yaml +++ /dev/null @@ -1,2 +0,0 @@ -simple: - test: str diff --git a/config/internal/configsource/testdata/simple_sources.yaml b/config/internal/configsource/testdata/simple_sources.yaml deleted file mode 100644 index a02657ea07e..00000000000 --- a/config/internal/configsource/testdata/simple_sources.yaml +++ /dev/null @@ -1,3 +0,0 @@ -cfgsrc: - keys: - simple: str From 17cc92f20fb5bac07169f1b23a4681c5e47f36c5 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Tue, 23 Mar 2021 18:55:54 +0000 Subject: [PATCH 17/19] Remove boiler plate code that can be added later --- .../configsource.go => component.go} | 36 +------------------ .../configsource/configmodels/configsource.go | 26 -------------- 2 files changed, 1 insertion(+), 61 deletions(-) rename config/internal/configsource/{component/configsource.go => component.go} (64%) delete mode 100644 config/internal/configsource/configmodels/configsource.go diff --git a/config/internal/configsource/component/configsource.go b/config/internal/configsource/component.go similarity index 64% rename from config/internal/configsource/component/configsource.go rename to config/internal/configsource/component.go index 718c046e98b..2cdcbe55d8e 100644 --- a/config/internal/configsource/component/configsource.go +++ b/config/internal/configsource/component.go @@ -12,22 +12,15 @@ // See the License for the specific language governing permissions and // limitations under the License. -package component +package configsource import ( "context" - - "go.uber.org/zap" - - "go.opentelemetry.io/collector/component" - "go.opentelemetry.io/collector/config/internal/configsource/configmodels" ) // ConfigSource is the interface to be implemented by objects used by the collector // to retrieve external configuration information. type ConfigSource interface { - component.Component - // BeginSession signals that the ConfigSource is about to be used to inject data into // the configuration. The difference between BeginSession and the component.Start method // is that the latter is used by the host to manage the life-time of a ConfigSource and to @@ -69,30 +62,3 @@ type ConfigSource interface { // internal state, etc. EndSession(ctx context.Context) } - -// CreateConfigSourceParams is passed to ConfigSourceFactory.CreateConfigSource function. -type CreateConfigSourceParams struct { - // Logger that the factory can use during creation and can pass to the created - // component to be used later as well. - Logger *zap.Logger - - // ApplicationStartInfo can be used to retrieve data according to version, etc. - ApplicationStartInfo component.ApplicationStartInfo -} - -// ConfigSourceFactory is a factory interface for configuration sources. -type ConfigSourceFactory interface { - component.Factory - - // CreateDefaultConfig creates the default configuration for the ConfigSource. - // This method can be called multiple times depending on the pipeline - // configuration and should not cause side-effects that prevent the creation - // of multiple instances of the ConfigSource. - // The object returned by this method needs to pass the checks implemented by - // 'configcheck.ValidateConfig'. It is recommended to have such check in the - // tests of any implementation of the Factory interface. - CreateDefaultConfig() configmodels.ConfigSource - - // CreateConfigSource creates a configuration source based on the given config. - CreateConfigSource(ctx context.Context, params CreateConfigSourceParams, cfg configmodels.ConfigSource) (ConfigSource, error) -} diff --git a/config/internal/configsource/configmodels/configsource.go b/config/internal/configsource/configmodels/configsource.go deleted file mode 100644 index 702039b8304..00000000000 --- a/config/internal/configsource/configmodels/configsource.go +++ /dev/null @@ -1,26 +0,0 @@ -// 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 configmodels - -import ( - "go.opentelemetry.io/collector/config/configmodels" -) - -// ConfigSource is the configuration of a config source. Specific configuration -// sources must implement this interface and will typically embed ConfigSourceSettings -// or a struct that extends it. -type ConfigSource interface { - configmodels.NamedEntity -} From 707075fc6c1a3faed58d2bdcf635ee6138cb17cb Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Tue, 23 Mar 2021 19:31:28 +0000 Subject: [PATCH 18/19] Reduce the interface to a minimum --- config/internal/configsource/component.go | 58 +++++++++-------------- 1 file changed, 23 insertions(+), 35 deletions(-) diff --git a/config/internal/configsource/component.go b/config/internal/configsource/component.go index 2cdcbe55d8e..d0991f44db8 100644 --- a/config/internal/configsource/component.go +++ b/config/internal/configsource/component.go @@ -21,44 +21,32 @@ import ( // ConfigSource is the interface to be implemented by objects used by the collector // to retrieve external configuration information. type ConfigSource interface { - // BeginSession signals that the ConfigSource is about to be used to inject data into - // the configuration. The difference between BeginSession and the component.Start method - // is that the latter is used by the host to manage the life-time of a ConfigSource and to - // provide a way for a ConfigSource to have a reference to the host so it can notify the - // host when configuration changes are detected. + // NewSession must create a Session object that will be used to inject data into + // a configuration. // - // A ConfigSource should use the BeginSession call according to their needs: - // lock resources, suspend background or watcher tasks, etc. An implementation, for - // instance, can use the begin of a session to prevent torn configurations, by acquiring - // a lock (or some other mechanism) that prevents concurrent changes to the configura during - // the middle of a session. + // The Session object should use its creation according to their ConfigSource needs: + // lock resources, suspend background tasks, etc. An implementation, for instance, + // can use the creation of the Session object to prevent torn configurations, + // by acquiring a lock (or some other mechanism) that prevents concurrent changes to the + // configuration during the middle of a session. // - // The code managing the session must guarantee that no ConfigSource instance participates - // in concurrent sessions. - BeginSession(ctx context.Context) error + // The code managing the returned Session object must guarantee that the object is not used + // concurrently. + NewSession(ctx context.Context) (Session, error) +} - // Apply goes to the configuration source, sending the given parameters as a map - // and returns the resulting configuration value or snippet. A configuration snippet - // will become a map, as follows: - // - // $my_config_src: - // param0: true - // param1: "some string" - // - // Becomes a call with the following payload as params: - // - // map[string]interface{}{ - // "param0": true, - // "param1": "some string", - // } +// Session is the interface used to inject configuration data from a ConfigSource. A Session +// object is created from a ConfigSource. The code using Session objects must guarantee that +// methods of a single instance are not called concurrently. +type Session interface { + // Apply goes to the configuration source, and according to the specified selector and + // parameters retrieves a configuration value that is injected into the configuration. // - // A ConfigSource should then unmarshal or cast the params. An error should be - // returned when the params don't fit the expected usage. - Apply(ctx context.Context, params interface{}) (interface{}, error) + // The selector is a string that is required on all invocations, the params are optional. + Apply(ctx context.Context, selector string, params interface{}) (interface{}, error) - // EndSession signals that the configuration was fully flattened and it - // is ready to be loaded. Each ConfigSource should use this call according - // to their needs: release resources, start background or watcher tasks, update - // internal state, etc. - EndSession(ctx context.Context) + // Close signals that the object won't be used anymore to inject data into a configuration. + // Each Session object should use this call according to their needs: release resources, + // close communication channels, etc. + Close(ctx context.Context) } From 481a89fb578957261eeb8753b63e7cca3cab9637 Mon Sep 17 00:00:00 2001 From: Paulo Janotti Date: Tue, 23 Mar 2021 20:02:11 +0000 Subject: [PATCH 19/19] Return error from Session.Close --- config/internal/configsource/component.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/internal/configsource/component.go b/config/internal/configsource/component.go index d0991f44db8..1321f311a75 100644 --- a/config/internal/configsource/component.go +++ b/config/internal/configsource/component.go @@ -48,5 +48,5 @@ type Session interface { // Close signals that the object won't be used anymore to inject data into a configuration. // Each Session object should use this call according to their needs: release resources, // close communication channels, etc. - Close(ctx context.Context) + Close(ctx context.Context) error }