From 7c1465abc244388451fe569a3acd7f6a3babdf0c Mon Sep 17 00:00:00 2001 From: Pablo Collins Date: Mon, 20 Dec 2021 17:17:33 -0800 Subject: [PATCH 1/2] Fix bug requiring $$ when using config source variables Previously, if a user wanted to use a config source variable (which have the form ${env:MY_ENV_VAR}) they would have to use two dollar signs, like $${env:MY_ENV_VAR}. This is because the expandMapProvider, which lives in core and expands ${MY_ENV_VAR} -style variables, would run before the config source providers, and it would replace any ${env:MY_ENV_VAR}s with "" but convert any $${env:MY_ENV_VAR} to ${env:MY_ENV_VAR}. The fix proposed here is to reverse the order in which these providers run: now the config source providers run first, then expandMapProvider runs. In addition, this change fixes up any $${env:MY_ENV_VAR} workarounds users may have put in place. --- cmd/otelcol/main.go | 69 +++++++--------- internal/collectorconfig/config_provider.go | 81 +++++++++++++++++++ .../collectorconfig/config_provider_test.go | 52 ++++++++++++ internal/collectorconfig/testdata/config.yaml | 4 + internal/configconverter/dollar_dollar.go | 48 +++++++++++ .../configconverter/dollar_dollar_test.go | 52 ++++++++++++ internal/configconverter/parser_provider.go | 2 +- .../testdata/dollar-dollar.yaml | 19 +++++ 8 files changed, 285 insertions(+), 42 deletions(-) create mode 100644 internal/collectorconfig/config_provider.go create mode 100644 internal/collectorconfig/config_provider_test.go create mode 100644 internal/collectorconfig/testdata/config.yaml create mode 100644 internal/configconverter/dollar_dollar.go create mode 100644 internal/configconverter/dollar_dollar_test.go create mode 100644 internal/configconverter/testdata/dollar-dollar.yaml diff --git a/cmd/otelcol/main.go b/cmd/otelcol/main.go index 0920b6b204..503944f9bf 100644 --- a/cmd/otelcol/main.go +++ b/cmd/otelcol/main.go @@ -18,7 +18,6 @@ package main import ( - "bytes" "flag" "fmt" "io" @@ -30,12 +29,9 @@ import ( "go.opentelemetry.io/collector/component" "go.opentelemetry.io/collector/config/configmapprovider" "go.opentelemetry.io/collector/service" - "go.uber.org/zap" + "github.com/signalfx/splunk-otel-collector/internal/collectorconfig" "github.com/signalfx/splunk-otel-collector/internal/components" - "github.com/signalfx/splunk-otel-collector/internal/configconverter" - "github.com/signalfx/splunk-otel-collector/internal/configprovider" - "github.com/signalfx/splunk-otel-collector/internal/configsources" "github.com/signalfx/splunk-otel-collector/internal/version" ) @@ -82,36 +78,43 @@ func main() { Version: version.Version, } - parserProvider := configprovider.NewConfigSourceParserProvider( - newBaseParserProvider(), - zap.NewNop(), // The service logger is not available yet, setting it to NoP. + serviceParams := service.CollectorSettings{ + BuildInfo: info, + Factories: factories, + ConfigMapProvider: newConfigMapProvider(info), + } + + if err := run(serviceParams); err != nil { + log.Fatal(err) + } +} + +func newConfigMapProvider(info component.BuildInfo) configmapprovider.Provider { + return collectorconfig.NewConfigMapProvider( info, - configsources.Get()..., + hasNoConvertFlag(), + getConfigPath(), + os.Getenv(configYamlEnvVarName), + getSetProperties(), ) +} +func hasNoConvertFlag() bool { const noConvertConfigFlag = "--no-convert-config" if hasFlag(noConvertConfigFlag) { // the collector complains about this flag if we don't remove it removeFlag(&os.Args, noConvertConfigFlag) - } else { - parserProvider = configconverter.ParserProvider( - parserProvider, - configconverter.RemoveBallastKey, - configconverter.MoveOTLPInsecureKey, - configconverter.MoveHecTLS, - configconverter.RenameK8sTagger, - ) - } - - serviceParams := service.CollectorSettings{ - BuildInfo: info, - Factories: factories, - ConfigMapProvider: parserProvider, + return true } + return false +} - if err := run(serviceParams); err != nil { - log.Fatal(err) +func getConfigPath() string { + ok, configPath := getKeyValue(os.Args[1:], "--config") + if !ok { + return os.Getenv(configEnvVarName) } + return configPath } // required to support --set functionality no longer directly parsed by the core config loader. @@ -370,22 +373,6 @@ func setDefaultEnvVars() { } } -// Returns a ParserProvider that reads configuration YAML from an environment variable when applicable. -func newBaseParserProvider() configmapprovider.Provider { - var configPath string - var ok bool - if ok, configPath = getKeyValue(os.Args[1:], "--config"); !ok { - configPath = os.Getenv(configEnvVarName) - } - configYaml := os.Getenv(configYamlEnvVarName) - - if configPath == "" && configYaml != "" { - return configmapprovider.NewExpand(configmapprovider.NewInMemory(bytes.NewBufferString(configYaml))) - } - - return configmapprovider.NewDefault(configPath, getSetProperties()) -} - func runInteractive(settings service.CollectorSettings) error { cmd := service.NewCommand(settings) if err := cmd.Execute(); err != nil { diff --git a/internal/collectorconfig/config_provider.go b/internal/collectorconfig/config_provider.go new file mode 100644 index 0000000000..bfe241fbc9 --- /dev/null +++ b/internal/collectorconfig/config_provider.go @@ -0,0 +1,81 @@ +// 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 collectorconfig + +import ( + "bytes" + + "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/config/configmapprovider" + "go.uber.org/zap" + + "github.com/signalfx/splunk-otel-collector/internal/configconverter" + "github.com/signalfx/splunk-otel-collector/internal/configprovider" + "github.com/signalfx/splunk-otel-collector/internal/configsources" +) + +func NewConfigMapProvider( + info component.BuildInfo, + hasNoConvertConfigFlag bool, + configFlagPath string, + configYamlFromEnv string, + properties []string, +) configmapprovider.Provider { + provider := newExpandProvider(info, configFlagPath, configYamlFromEnv, properties, hasNoConvertConfigFlag) + if !hasNoConvertConfigFlag { + provider = configconverter.Provider( + provider, + configconverter.RemoveBallastKey, + configconverter.MoveOTLPInsecureKey, + configconverter.MoveHecTLS, + configconverter.RenameK8sTagger, + ) + } + return provider +} + +func newExpandProvider( + info component.BuildInfo, + configPath string, + configYamlFromEnv string, + properties []string, + hasNoConvertConfigFlag bool, +) configmapprovider.Provider { + provider := newBaseProvider(configPath, configYamlFromEnv, properties) + if !hasNoConvertConfigFlag { + // we have to convert any $${foo:bar} *before* the expand provider runs, + // so we do it here rather than in NewConfigMapProvider + provider = configconverter.Provider( + provider, + configconverter.ReplaceDollarDollar, + ) + } + return configmapprovider.NewExpand(configprovider.NewConfigSourceParserProvider( + provider, + zap.NewNop(), // The service logger is not available yet, setting it to NoP. + info, + configsources.Get()..., + )) +} + +func newBaseProvider(configPath string, configYamlFromEnv string, properties []string) configmapprovider.Provider { + if configPath == "" && configYamlFromEnv != "" { + return configmapprovider.NewInMemory(bytes.NewBufferString(configYamlFromEnv)) + } + return configmapprovider.NewMerge( + configmapprovider.NewFile(configPath), + configmapprovider.NewProperties(properties), + ) +} diff --git a/internal/collectorconfig/config_provider_test.go b/internal/collectorconfig/config_provider_test.go new file mode 100644 index 0000000000..1ce98f3fad --- /dev/null +++ b/internal/collectorconfig/config_provider_test.go @@ -0,0 +1,52 @@ +// 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 collectorconfig + +import ( + "context" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/collector/component" + "go.opentelemetry.io/collector/config/configmapprovider" +) + +func TestParseConfigSource_ConfigFile(t *testing.T) { + _ = os.Setenv("CFG_SRC", "my_cfg_src_val") + _ = os.Setenv("LEGACY", "my_legacy_val") + pp := NewConfigMapProvider(component.BuildInfo{}, false, "testdata/config.yaml", "", nil) + assertProviderOK(t, pp) +} + +func TestParseConfigSource_InMemory(t *testing.T) { + cfgYaml, err := os.ReadFile("testdata/config.yaml") + require.NoError(t, err) + pp := NewConfigMapProvider(component.BuildInfo{}, false, "", string(cfgYaml), nil) + assertProviderOK(t, pp) +} + +func assertProviderOK(t *testing.T, provider configmapprovider.Provider) { + ctx := context.Background() + retrieved, err := provider.Retrieve(ctx, nil) + require.NoError(t, err) + cfgMap, err := retrieved.Get(ctx) + require.NoError(t, err) + v := cfgMap.Get("config_source_env_key") + assert.Equal(t, "my_cfg_src_val", v) + v = cfgMap.Get("legacy_env_key") + assert.Equal(t, "my_legacy_val", v) +} diff --git a/internal/collectorconfig/testdata/config.yaml b/internal/collectorconfig/testdata/config.yaml new file mode 100644 index 0000000000..4cc005e57f --- /dev/null +++ b/internal/collectorconfig/testdata/config.yaml @@ -0,0 +1,4 @@ +config_sources: + env: +config_source_env_key: ${env:CFG_SRC} +legacy_env_key: $LEGACY diff --git a/internal/configconverter/dollar_dollar.go b/internal/configconverter/dollar_dollar.go new file mode 100644 index 0000000000..fbd9837265 --- /dev/null +++ b/internal/configconverter/dollar_dollar.go @@ -0,0 +1,48 @@ +// 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 configconverter + +import ( + "log" + "regexp" + + "go.opentelemetry.io/collector/config" +) + +// ReplaceDollarDollar replaces any $${foo:MY_VAR} config source variables with +// ${foo:MY_VAR}. These might exist because of customers working around a bug +// in how the Collector expanded these variables. +func ReplaceDollarDollar(m *config.Map) *config.Map { + re := dollarDollarRegex() + for _, k := range m.AllKeys() { + v := m.Get(k) + if s, ok := v.(string); ok { + replaced := replaceDollarDollar(re, s) + if replaced != s { + log.Printf("[WARNING] the notation %q is no longer recommended. Please replace with %q.\n", v, replaced) + m.Set(k, replaced) + } + } + } + return m +} + +func dollarDollarRegex() *regexp.Regexp { + return regexp.MustCompile(`\$\${(.+?:.+?)}`) +} + +func replaceDollarDollar(re *regexp.Regexp, s string) string { + return re.ReplaceAllString(s, "${$1}") +} diff --git a/internal/configconverter/dollar_dollar_test.go b/internal/configconverter/dollar_dollar_test.go new file mode 100644 index 0000000000..e697b17dfe --- /dev/null +++ b/internal/configconverter/dollar_dollar_test.go @@ -0,0 +1,52 @@ +// 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 configconverter + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/collector/config/configmapprovider" +) + +func TestReplaceDollarDollar(t *testing.T) { + pp := &converterProvider{ + wrapped: configmapprovider.NewFile("testdata/dollar-dollar.yaml"), + cfgMapFuncs: []CfgMapFunc{ReplaceDollarDollar}, + } + r, err := pp.Retrieve(context.Background(), nil) + require.NoError(t, err) + v, err := r.Get(context.Background()) + require.NoError(t, err) + endpt := v.Get("exporters::otlp::endpoint") + assert.Equal(t, "localhost:${env:OTLP_PORT}", endpt) + insecure := v.Get("exporters::otlp::insecure") + assert.Equal(t, "$OTLP_INSECURE", insecure) + pwd := v.Get("receivers::redis::password") + assert.Equal(t, "$$ecret", pwd) + host := v.Get("receivers::redis::host") + assert.Equal(t, "ho$$tname:${env:PORT}", host) +} + +func TestRegexReplace(t *testing.T) { + assert.Equal(t, "${foo/bar:PORT}", testReplace("$${foo/bar:PORT}")) + assert.Equal(t, "$${PORT}", testReplace("$${PORT}")) +} + +func testReplace(s string) string { + return replaceDollarDollar(dollarDollarRegex(), s) +} diff --git a/internal/configconverter/parser_provider.go b/internal/configconverter/parser_provider.go index 91a3ba7f56..73391f02e1 100644 --- a/internal/configconverter/parser_provider.go +++ b/internal/configconverter/parser_provider.go @@ -35,7 +35,7 @@ type CfgMapFunc func(*config.Map) *config.Map var _ configmapprovider.Provider = (*converterProvider)(nil) -func ParserProvider(wrapped configmapprovider.Provider, funcs ...CfgMapFunc) configmapprovider.Provider { +func Provider(wrapped configmapprovider.Provider, funcs ...CfgMapFunc) configmapprovider.Provider { return &converterProvider{wrapped: wrapped, cfgMapFuncs: funcs} } diff --git a/internal/configconverter/testdata/dollar-dollar.yaml b/internal/configconverter/testdata/dollar-dollar.yaml new file mode 100644 index 0000000000..b26fd60dd7 --- /dev/null +++ b/internal/configconverter/testdata/dollar-dollar.yaml @@ -0,0 +1,19 @@ +receivers: + hostmetrics: + collection_interval: 1s + scrapers: + cpu: + redis: + password: $$ecret + host: "ho$$tname:$${env:PORT}" +exporters: + otlp: + endpoint: localhost:$${env:OTLP_PORT} + insecure: $OTLP_INSECURE +service: + pipelines: + metrics: + receivers: + - hostmetrics + exporters: + - otlp From 94852b8e73a0771ee0c42990eb533c05e66f6dfc Mon Sep 17 00:00:00 2001 From: Pablo Collins Date: Mon, 10 Jan 2022 15:38:04 -0500 Subject: [PATCH 2/2] Update ReplaceDollarDollar to handle arrays --- internal/configconverter/dollar_dollar.go | 17 ++++++--- .../configconverter/dollar_dollar_test.go | 21 ++++++---- internal/configconverter/parser_provider.go | 38 +++++++++++++++++++ .../testdata/dollar-dollar.yaml | 12 ++++++ 4 files changed, 76 insertions(+), 12 deletions(-) diff --git a/internal/configconverter/dollar_dollar.go b/internal/configconverter/dollar_dollar.go index fbd9837265..2a133bc127 100644 --- a/internal/configconverter/dollar_dollar.go +++ b/internal/configconverter/dollar_dollar.go @@ -26,14 +26,21 @@ import ( // in how the Collector expanded these variables. func ReplaceDollarDollar(m *config.Map) *config.Map { re := dollarDollarRegex() + replace := func(s string) string { + return replaceDollarDollar(re, s) + } for _, k := range m.AllKeys() { - v := m.Get(k) - if s, ok := v.(string); ok { - replaced := replaceDollarDollar(re, s) - if replaced != s { - log.Printf("[WARNING] the notation %q is no longer recommended. Please replace with %q.\n", v, replaced) + switch v := m.Get(k).(type) { + case string: + replaced := replace(v) + if replaced != v { + format := "[WARNING] the notation %q is no longer recommended. Please replace with %q.\n" + log.Printf(format, v, replaced) m.Set(k, replaced) } + case []interface{}: + replaced := replaceArray(v, replace) + m.Set(k, replaced) } } return m diff --git a/internal/configconverter/dollar_dollar_test.go b/internal/configconverter/dollar_dollar_test.go index e697b17dfe..64a2df0cb6 100644 --- a/internal/configconverter/dollar_dollar_test.go +++ b/internal/configconverter/dollar_dollar_test.go @@ -24,22 +24,29 @@ import ( ) func TestReplaceDollarDollar(t *testing.T) { - pp := &converterProvider{ + p := &converterProvider{ wrapped: configmapprovider.NewFile("testdata/dollar-dollar.yaml"), cfgMapFuncs: []CfgMapFunc{ReplaceDollarDollar}, } - r, err := pp.Retrieve(context.Background(), nil) + r, err := p.Retrieve(context.Background(), nil) require.NoError(t, err) - v, err := r.Get(context.Background()) + cfgMap, err := r.Get(context.Background()) require.NoError(t, err) - endpt := v.Get("exporters::otlp::endpoint") + endpt := cfgMap.Get("exporters::otlp::endpoint") assert.Equal(t, "localhost:${env:OTLP_PORT}", endpt) - insecure := v.Get("exporters::otlp::insecure") + insecure := cfgMap.Get("exporters::otlp::insecure") assert.Equal(t, "$OTLP_INSECURE", insecure) - pwd := v.Get("receivers::redis::password") + pwd := cfgMap.Get("receivers::redis::password") assert.Equal(t, "$$ecret", pwd) - host := v.Get("receivers::redis::host") + host := cfgMap.Get("receivers::redis::host") assert.Equal(t, "ho$$tname:${env:PORT}", host) + v := cfgMap.Get("processors::metricstransform::transforms").([]interface{})[0] + transforms := v.(map[string]interface{}) + operations := transforms["operations"].([]interface{}) + op0 := operations[0].(map[string]interface{}) + assert.Equal(t, "${env:MYVAR}", op0["new_value"]) + op1 := operations[0].(map[string]interface{}) + assert.Equal(t, "yyy${env:MYVAR}zzz", op1["new_value"]) } func TestRegexReplace(t *testing.T) { diff --git a/internal/configconverter/parser_provider.go b/internal/configconverter/parser_provider.go index 73391f02e1..46919fcb1c 100644 --- a/internal/configconverter/parser_provider.go +++ b/internal/configconverter/parser_provider.go @@ -72,3 +72,41 @@ func (p *converterProvider) Get(ctx context.Context) (*config.Map, error) { func (p *converterProvider) Close(ctx context.Context) error { return nil } + +func replaceArray(in []interface{}, f func(string) string) []interface{} { + var out []interface{} + for _, o := range in { + var replaced interface{} + switch v := o.(type) { + case string: + replaced = f(v) + case map[string]interface{}: + replaced = replaceMap(v, f) + case []interface{}: + replaced = replaceArray(v, f) + default: + replaced = o + } + out = append(out, replaced) + } + return out +} + +func replaceMap(in map[string]interface{}, f func(string) string) map[string]interface{} { + out := map[string]interface{}{} + for k, o := range in { + var replaced interface{} + switch v := o.(type) { + case string: + replaced = f(v) + case map[string]interface{}: + replaced = replaceMap(v, f) + case []interface{}: + replaced = replaceArray(v, f) + default: + replaced = o + } + out[k] = replaced + } + return out +} diff --git a/internal/configconverter/testdata/dollar-dollar.yaml b/internal/configconverter/testdata/dollar-dollar.yaml index b26fd60dd7..13d2cdabd8 100644 --- a/internal/configconverter/testdata/dollar-dollar.yaml +++ b/internal/configconverter/testdata/dollar-dollar.yaml @@ -6,6 +6,16 @@ receivers: redis: password: $$ecret host: "ho$$tname:$${env:PORT}" +processors: + metricstransform: + transforms: + - action: update + include: .* + match_type: regexp + operations: + - action: add_label + new_label: myvar + new_value: $${env:MYVAR} exporters: otlp: endpoint: localhost:$${env:OTLP_PORT} @@ -15,5 +25,7 @@ service: metrics: receivers: - hostmetrics + processors: + - metricstransform exporters: - otlp