From dd48eed0f6befb082f1c9654270566c4d1ea9d43 Mon Sep 17 00:00:00 2001 From: Ryan Fitzpatrick Date: Thu, 13 Jan 2022 17:24:57 +0000 Subject: [PATCH] Add double dollar config source parsing compatibility --- CHANGELOG.md | 10 +- internal/configprovider/manager.go | 125 ++++++++++++------ internal/configprovider/manager_test.go | 8 +- .../testdata/envvar_cfgsrc_mix.yaml | 4 + .../testdata/envvar_cfgsrc_mix_expected.yaml | 4 + tests/general/envvar_expansion_test.go | 21 +++ .../testdata/env_config_source_labels.yaml | 30 +++++ .../env_config_source_labels.yaml | 28 ++-- .../resource_metrics/envvar_labels.yaml | 37 +++--- .../incompat_env_config_source_labels.yaml | 27 ++++ 10 files changed, 224 insertions(+), 70 deletions(-) create mode 100644 tests/general/testdata/resource_metrics/incompat_env_config_source_labels.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index ef0e278a8a..2e5707fe03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,17 @@ ## Unreleased +### 🛑 Breaking changes 🛑 + +- This version adopts OpenTelemetry Core version 0.42.0, and in doing so the configuration parsing process has changed slightly. The Splunk OpenTelemetry Collector used to [evaluate user configuration twice](https://github.com/signalfx/splunk-otel-collector/issues/628) and this required escaping desired `$` literals with an additional `$` character to prevent unwanted environment variable expansion. This version no longer doubly evaluates configuration so any `$$` instances in your configuration as a workaround should be updated to `$`. [Config source directives](./internal/configsource) that include and additional `$` are provided with a temporary, backward-compatible `$${config_source:value}` and `$$config_source:value` parsing rule controlled by `SPLUNK_DOUBLE_DOLLAR_CONFIG_SOURCE_COMPATIBLE` environment variable (default `"true"`) to continue supporting the updating configs from [#930](https://github.com/signalfx/splunk-otel-collector/pull/930) and [#935](https://github.com/signalfx/splunk-otel-collector/pull/935). This functionality will be removed in a future release (#1099) + ### 🚀 New components 🚀 -- [`docker_observer`](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/extension/observer/dockerobserver) to detect and create container endpoints, to be used with the [`receiver_creator`](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/receivercreator). +- [`docker_observer`](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/extension/observer/dockerobserver) to detect and create container endpoints, to be used with the [`receiver_creator`](https://github.com/open-telemetry/opentelemetry-collector-contrib/tree/main/receiver/receivercreator) (#1044) + +### 💡 Enhancements 💡 + +- Update Collector Core and Contrib deps to 0.42.0 (#1099) ## v0.41.0 diff --git a/internal/configprovider/manager.go b/internal/configprovider/manager.go index 62827f608b..d009afdffb 100644 --- a/internal/configprovider/manager.go +++ b/internal/configprovider/manager.go @@ -18,8 +18,10 @@ import ( "context" "errors" "fmt" + "log" "net/url" "os" + "strconv" "strings" "sync" @@ -42,6 +44,8 @@ const ( // typeAndNameSeparator is the separator that is used between type and name in type/name // composite keys. typeAndNameSeparator = '/' + // dollarDollarCompatEnvVar is a temporary env var to disable backward compatibility (true by default) + dollarDollarCompatEnvVar = "SPLUNK_DOUBLE_DOLLAR_CONFIG_SOURCE_COMPATIBLE" ) // private error types to help with testability @@ -49,6 +53,13 @@ type ( errUnknownConfigSource struct{ error } ) +var ddBackwardCompatible = func() bool { + if v, err := strconv.ParseBool(strings.ToLower(os.Getenv(dollarDollarCompatEnvVar))); err == nil { + return v + } + return true +}() + // Manager is used to inject data from config sources into a configuration and also // to monitor for updates on the items injected into the configuration. All methods // of a Manager must be called only once and have an expected sequence: @@ -160,20 +171,11 @@ type ( // // For an overview about the internals of the Manager refer to the package README.md. type Manager struct { - // configSources is map from ConfigSource names (as defined in the configuration) - // and the respective instances. configSources map[string]configsource.ConfigSource - // watchingCh is used to notify users of the Manager that the WatchForUpdate function - // is ready and waiting for notifications. - watchingCh chan struct{} - // closeCh is used to notify the Manager WatchForUpdate function that the manager - // is being closed. - closeCh chan struct{} - // watchers keeps track of all WatchForUpdate functions for retrieved values. - watchers []configsource.Watchable - // watchersWG is used to ensure that Close waits for all WatchForUpdate calls - // to complete. - watchersWG sync.WaitGroup + watchingCh chan struct{} + closeCh chan struct{} + watchers []configsource.Watchable + watchersWG sync.WaitGroup } // NewManager creates a new instance of a Manager to be used to inject data from @@ -376,37 +378,49 @@ func (m *Manager) parseStringValue(ctx context.Context, s string) (interface{}, switch { case s[j+1] == expandPrefixChar: - // Escaping the prefix so $$ becomes a single $ without attempting - // to treat the string after it as a config source or env var. - expandableContent = string(expandPrefixChar) - w = 1 // consumed a single char + // temporary backward compatibility to support updated $${config_source:value} functionality + // in provided configs from 0.37.0 until 0.42.0 + bwCompatibilityRequired := false + + var expanded, sourceName string + var ww int + if ddBackwardCompatible && len(s[j+1:]) > 2 { + if s[j+2] == '{' { + if expanded, ww, sourceName = getBracketedExpandableContent(s, j+2); sourceName != "" { + bwCompatibilityRequired = true + } + } else { + if expanded, ww, sourceName = getBareExpandableContent(s, j+2); sourceName != "" { + if len(expanded) > (len(sourceName) + 1) { + if !strings.Contains(expanded[len(sourceName)+1:], "$") { + bwCompatibilityRequired = true + } + } + } + } + } - case s[j+1] == '{': - // Bracketed usage, consume everything until first '}' exactly as os.Expand. - expandableContent, w = scanToClosingBracket(s[j+1:]) - expandableContent = strings.Trim(expandableContent, " ") // Allow for some spaces. - delimIndex := strings.Index(expandableContent, string(configSourceNameDelimChar)) - if len(expandableContent) > 1 && delimIndex > -1 { - // Bracket expandableContent contains ':' treating it as a config source. - cfgSrcName = expandableContent[:delimIndex] + if bwCompatibilityRequired { + log.Printf( + `Deprecated config source directive %q has been replaced with %q. Please update your config as necessary as this will be removed in future release. To disable this replacement set the SPLUNK_DOUBLE_DOLLAR_CONFIG_SOURCE_COMPATIBLE environment variable to "false" before restarting the Collector.`, + s[j:j+2+ww], s[j+1:j+2+ww], + ) + expandableContent = expanded + w = ww + 1 + cfgSrcName = sourceName + } else { + // Escaping the prefix so $$ becomes a single $ without attempting + // to treat the string after it as a config source or env var. + expandableContent = string(expandPrefixChar) + w = 1 // consumed a single char } + case s[j+1] == '{': + expandableContent, w, cfgSrcName = getBracketedExpandableContent(s, j+1) + default: - // Non-bracketed usage, ie.: found the prefix char, it can be either a config - // source or an environment variable. - var name string - name, w = getTokenName(s[j+1:]) - expandableContent = name // Assume for now that it is an env var. - - // Peek next char after name, if it is a config source name delimiter treat the remaining of the - // string as a config source. - possibleDelimCharIndex := j + w + 1 - if possibleDelimCharIndex < len(s) && s[possibleDelimCharIndex] == configSourceNameDelimChar { - // This is a config source, since it is not delimited it will consume until end of the string. - cfgSrcName = name - expandableContent = s[j+1:] - w = len(expandableContent) // Set consumed bytes to the length of expandableContent - } + expandableContent, w, cfgSrcName = getBareExpandableContent(s, j+1) + } // At this point expandableContent contains a string to be expanded, evaluate and expand it. @@ -473,6 +487,37 @@ func (m *Manager) parseStringValue(ctx context.Context, s string) (interface{}, return string(buf) + s[i:], nil } +func getBracketedExpandableContent(s string, i int) (expandableContent string, consumed int, cfgSrcName string) { + // Bracketed usage, consume everything until first '}' exactly as os.Expand. + expandableContent, consumed = scanToClosingBracket(s[i:]) + expandableContent = strings.Trim(expandableContent, " ") // Allow for some spaces. + delimIndex := strings.Index(expandableContent, string(configSourceNameDelimChar)) + if len(expandableContent) > 1 && delimIndex > -1 { + // Bracket expandableContent contains ':' treating it as a config source. + cfgSrcName = expandableContent[:delimIndex] + } + return +} + +func getBareExpandableContent(s string, i int) (expandableContent string, consumed int, cfgSrcName string) { + // Non-bracketed usage, ie.: found the prefix char, it can be either a config + // source or an environment variable. + var name string + name, consumed = getTokenName(s[i:]) + expandableContent = name // Assume for now that it is an env var. + + // Peek next char after name, if it is a config source name delimiter treat the remaining of the + // string as a config source. + possibleDelimCharIndex := i + consumed + if possibleDelimCharIndex < len(s) && s[possibleDelimCharIndex] == configSourceNameDelimChar { + // This is a config source, since it is not delimited it will consume until end of the string. + cfgSrcName = name + expandableContent = s[i:] + consumed = len(expandableContent) // Set consumed bytes to the length of expandableContent + } + return +} + // retrieveConfigSourceData retrieves data from the specified config source and injects them into // the configuration. The Manager tracks sessions and watcher objects as needed. func (m *Manager) retrieveConfigSourceData(ctx context.Context, cfgSrcName, cfgSrcInvocation string) (interface{}, error) { diff --git a/internal/configprovider/manager_test.go b/internal/configprovider/manager_test.go index d59322891a..a4bbbdd7ba 100644 --- a/internal/configprovider/manager_test.go +++ b/internal/configprovider/manager_test.go @@ -432,7 +432,6 @@ func TestConfigSourceManager_EnvVarHandling(t *testing.T) { } return nil } - manager := newManager(map[string]configsource.ConfigSource{ "tstcfgsrc": &tstCfgSrc, }) @@ -586,7 +585,12 @@ func TestManager_expandString(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := manager.parseStringValue(ctx, tt.input) - require.IsType(t, tt.wantErr, err) + if tt.wantErr != nil { + require.Error(t, err) + require.IsType(t, tt.wantErr, err) + } else { + require.NoError(t, err) + } require.Equal(t, tt.want, got) }) } diff --git a/internal/configprovider/testdata/envvar_cfgsrc_mix.yaml b/internal/configprovider/testdata/envvar_cfgsrc_mix.yaml index 6560b52de0..134a76724f 100644 --- a/internal/configprovider/testdata/envvar_cfgsrc_mix.yaml +++ b/internal/configprovider/testdata/envvar_cfgsrc_mix.yaml @@ -10,6 +10,10 @@ envvar_legacy_05: $1/test cfgsrc_suffix: prefix-$tstcfgsrc:int_key cfgsrc_middle: prefix-${tstcfgsrc:int_key}-suffix cfgsrc_in_str: integer ${tstcfgsrc:int_key} injected as string +cfgsrc_workaround_suffix: prefix-$$tstcfgsrc:int_key +cfgsrc_braces_workaround_suffix: prefix-$${ tstcfgsrc:int_key } +cfgsrc_braces_workaround_middle: prefix-$${tstcfgsrc:int_key}-suffix +cfgsrc_braces_workaround_in_str: integer $${ tstcfgsrc:int_key} injected as string cfgsrc_params0: ${tstcfgsrc:params_key?p0=true&p1=$envvar&p2=42} cfgsrc_params1: "${tstcfgsrc:params_key?p0=false&p1=42&p2=$envvar}" cfgsrc_params2: $tstcfgsrc:params_key?p0=$$envvar diff --git a/internal/configprovider/testdata/envvar_cfgsrc_mix_expected.yaml b/internal/configprovider/testdata/envvar_cfgsrc_mix_expected.yaml index e9e9ba862c..002ceb7b38 100644 --- a/internal/configprovider/testdata/envvar_cfgsrc_mix_expected.yaml +++ b/internal/configprovider/testdata/envvar_cfgsrc_mix_expected.yaml @@ -10,6 +10,10 @@ envvar_legacy_05: /test cfgsrc_suffix: prefix-42 cfgsrc_middle: prefix-42-suffix cfgsrc_in_str: integer 42 injected as string +cfgsrc_workaround_suffix: prefix-42 +cfgsrc_braces_workaround_suffix: prefix-42 +cfgsrc_braces_workaround_middle: prefix-42-suffix +cfgsrc_braces_workaround_in_str: integer 42 injected as string cfgsrc_params0: p0: true p1: envvar_value diff --git a/tests/general/envvar_expansion_test.go b/tests/general/envvar_expansion_test.go index 46be4cee9e..7e9611b831 100644 --- a/tests/general/envvar_expansion_test.go +++ b/tests/general/envvar_expansion_test.go @@ -58,3 +58,24 @@ func TestExpandedDollarSignsViaEnvConfigSource(t *testing.T) { expectedResourceMetrics := tc.ResourceMetrics("env_config_source_labels.yaml") require.NoError(t, tc.OTLPMetricsReceiverSink.AssertAllMetricsReceived(t, *expectedResourceMetrics, 30*time.Second)) } + +func TestIncompatibleExpandedDollarSignsViaEnvConfigSource(t *testing.T) { + tc := testutils.NewTestcase(t) + defer tc.PrintLogsOnFailure() + defer tc.ShutdownOTLPMetricsReceiverSink() + + tc.SkipIfNotContainer() + + _, shutdown := tc.SplunkOtelCollectorWithEnv( + "env_config_source_labels.yaml", + map[string]string{ + "SPLUNK_DOUBLE_DOLLAR_CONFIG_SOURCE_COMPATIBLE": "false", + "AN_ENVVAR": "an-envvar-value", + }, + ) + + defer shutdown() + + expectedResourceMetrics := tc.ResourceMetrics("incompat_env_config_source_labels.yaml") + require.NoError(t, tc.OTLPMetricsReceiverSink.AssertAllMetricsReceived(t, *expectedResourceMetrics, 30*time.Second)) +} diff --git a/tests/general/testdata/env_config_source_labels.yaml b/tests/general/testdata/env_config_source_labels.yaml index 77bbb454e6..a3be42ec32 100644 --- a/tests/general/testdata/env_config_source_labels.yaml +++ b/tests/general/testdata/env_config_source_labels.yaml @@ -22,33 +22,63 @@ processors: - action: add_label new_label: single-dollar new_value: ${env:AN_ENVVAR}-suffix + - action: add_label + new_label: single-dollar-no-curly-braces + new_value: prefix-$env:AN_ENVVAR - action: add_label new_label: double-dollar new_value: $${env:AN_ENVVAR}-suffix + - action: add_label + new_label: double-dollar-no-curly-braces + new_value: prefix-$$env:AN_ENVVAR - action: add_label new_label: triple-dollar new_value: $$${env:AN_ENVVAR}-suffix + - action: add_label + new_label: triple-dollar-no-curly-braces + new_value: prefix-$$$env:AN_ENVVAR - action: add_label new_label: quadruple-dollar new_value: $$$${env:AN_ENVVAR}-suffix + - action: add_label + new_label: quadruple-dollar-no-curly-braces + new_value: prefix-$$$$env:AN_ENVVAR - action: add_label new_label: quintuple-dollar new_value: $$$$${env:AN_ENVVAR}-suffix + - action: add_label + new_label: quintuple-dollar-no-curly-braces + new_value: prefix-$$$$$env:AN_ENVVAR - action: add_label new_label: sextuple-dollar new_value: $$$$$${env:AN_ENVVAR}-suffix + - action: add_label + new_label: sextuple-dollar-no-curly-braces + new_value: prefix-$$$$$$env:AN_ENVVAR - action: add_label new_label: septuple-dollar new_value: $$$$$$${env:AN_ENVVAR}-suffix + - action: add_label + new_label: septuple-dollar-no-curly-braces + new_value: prefix-$$$$$$$env:AN_ENVVAR - action: add_label new_label: octuple-dollar new_value: $$$$$$$${env:AN_ENVVAR}-suffix + - action: add_label + new_label: octuple-dollar-no-curly-braces + new_value: prefix-$$$$$$$$env:AN_ENVVAR - action: add_label new_label: nonuple-dollar new_value: $$$$$$$$${env:AN_ENVVAR}-suffix + - action: add_label + new_label: nonuple-dollar-no-curly-braces + new_value: prefix-$$$$$$$$$env:AN_ENVVAR - action: add_label new_label: decuple-dollar new_value: $$$$$$$$$${env:AN_ENVVAR}-suffix + - action: add_label + new_label: decuple-dollar-no-curly-braces + new_value: prefix-$$$$$$$$$$env:AN_ENVVAR exporters: otlp: diff --git a/tests/general/testdata/resource_metrics/env_config_source_labels.yaml b/tests/general/testdata/resource_metrics/env_config_source_labels.yaml index 6bddf4dc88..9ddfcffafc 100644 --- a/tests/general/testdata/resource_metrics/env_config_source_labels.yaml +++ b/tests/general/testdata/resource_metrics/env_config_source_labels.yaml @@ -5,13 +5,23 @@ resource_metrics: type: IntGauge labels: state: used - single-dollar: -suffix + single-dollar: an-envvar-value-suffix + single-dollar-no-curly-braces: prefix-an-envvar-value double-dollar: an-envvar-value-suffix - triple-dollar: suffix - quadruple-dollar: ${env:AN_ENVVAR}-suffix - quintuple-dollar: $-suffix - sextuple-dollar: $an-envvar-value-suffix - septuple-dollar: $suffix - octuple-dollar: $${env:AN_ENVVAR}-suffix - nonuple-dollar: $$-suffix - decuple-dollar: $$an-envvar-value-suffix + double-dollar-no-curly-braces: prefix-an-envvar-value + triple-dollar: $an-envvar-value-suffix + triple-dollar-no-curly-braces: prefix-$an-envvar-value + quadruple-dollar: $an-envvar-value-suffix + quadruple-dollar-no-curly-braces: prefix-$an-envvar-value + quintuple-dollar: $$an-envvar-value-suffix + quintuple-dollar-no-curly-braces: prefix-$$an-envvar-value + sextuple-dollar: $$an-envvar-value-suffix + sextuple-dollar-no-curly-braces: prefix-$$an-envvar-value + septuple-dollar: $$$an-envvar-value-suffix + septuple-dollar-no-curly-braces: prefix-$$$an-envvar-value + octuple-dollar: $$$an-envvar-value-suffix + octuple-dollar-no-curly-braces: prefix-$$$an-envvar-value + nonuple-dollar: $$$$an-envvar-value-suffix + nonuple-dollar-no-curly-braces: prefix-$$$$an-envvar-value + decuple-dollar: $$$$an-envvar-value-suffix + decuple-dollar-no-curly-braces: prefix-$$$$an-envvar-value diff --git a/tests/general/testdata/resource_metrics/envvar_labels.yaml b/tests/general/testdata/resource_metrics/envvar_labels.yaml index 09b3bd8141..ba83fa96ba 100644 --- a/tests/general/testdata/resource_metrics/envvar_labels.yaml +++ b/tests/general/testdata/resource_metrics/envvar_labels.yaml @@ -7,21 +7,22 @@ resource_metrics: state: used single-dollar-no-curly-braces: an-envvar-value-suffix single-dollar-curly-braces: an-envvar-value-suffix - double-dollar-no-curly-braces: an-envvar-value-suffix - double-dollar-curly-braces: an-envvar-value-suffix - triple-dollar-no-curly-braces: -envvar-value-suffix - triple-dollar-curly-braces: -envvar-value-suffix - quadruple-dollar-no-curly-braces: $AN_ENVVAR-suffix - quadruple-dollar-curly-braces: ${AN_ENVVAR}-suffix - quintuple-dollar-no-curly-braces: $an-envvar-value-suffix - quintuple-dollar-curly-braces: $an-envvar-value-suffix - sextuple-dollar-no-curly-braces: $an-envvar-value-suffix - sextuple-dollar-curly-braces: $an-envvar-value-suffix - septuple-dollar-no-curly-braces: $-envvar-value-suffix - septuple-dollar-curly-braces: $-envvar-value-suffix - octuple-dollar-no-curly-braces: $$AN_ENVVAR-suffix - octuple-dollar-curly-braces: $${AN_ENVVAR}-suffix - nonuple-dollar-no-curly-braces: $$an-envvar-value-suffix - nonuple-dollar-curly-braces: $$an-envvar-value-suffix - decuple-dollar-no-curly-braces: $$an-envvar-value-suffix - decuple-dollar-curly-braces: $$an-envvar-value-suffix + double-dollar-no-curly-braces: $AN_ENVVAR-suffix + double-dollar-curly-braces: ${AN_ENVVAR}-suffix + triple-dollar-no-curly-braces: $an-envvar-value-suffix + triple-dollar-curly-braces: $an-envvar-value-suffix + quadruple-dollar-no-curly-braces: $$AN_ENVVAR-suffix + quadruple-dollar-curly-braces: $${AN_ENVVAR}-suffix + quintuple-dollar-no-curly-braces: $$an-envvar-value-suffix + quintuple-dollar-curly-braces: $$an-envvar-value-suffix + sextuple-dollar-no-curly-braces: $$$AN_ENVVAR-suffix + sextuple-dollar-curly-braces: $$${AN_ENVVAR}-suffix + septuple-dollar-no-curly-braces: $$$an-envvar-value-suffix + septuple-dollar-curly-braces: $$$an-envvar-value-suffix + octuple-dollar-no-curly-braces: $$$$AN_ENVVAR-suffix + octuple-dollar-curly-braces: $$$${AN_ENVVAR}-suffix + nonuple-dollar-no-curly-braces: $$$$an-envvar-value-suffix + nonuple-dollar-curly-braces: $$$$an-envvar-value-suffix + decuple-dollar-no-curly-braces: $$$$$AN_ENVVAR-suffix + decuple-dollar-curly-braces: $$$$${AN_ENVVAR}-suffix + diff --git a/tests/general/testdata/resource_metrics/incompat_env_config_source_labels.yaml b/tests/general/testdata/resource_metrics/incompat_env_config_source_labels.yaml new file mode 100644 index 0000000000..f7da0b085f --- /dev/null +++ b/tests/general/testdata/resource_metrics/incompat_env_config_source_labels.yaml @@ -0,0 +1,27 @@ +resource_metrics: + - instrumentation_library_metrics: + - metrics: + - name: system.memory.usage + type: IntGauge + labels: + state: used + single-dollar: an-envvar-value-suffix + single-dollar-no-curly-braces: prefix-an-envvar-value + double-dollar: ${env:AN_ENVVAR}-suffix + double-dollar-no-curly-braces: prefix-$env:AN_ENVVAR + triple-dollar: $an-envvar-value-suffix + triple-dollar-no-curly-braces: prefix-$an-envvar-value + quadruple-dollar: $${env:AN_ENVVAR}-suffix + quadruple-dollar-no-curly-braces: prefix-$$env:AN_ENVVAR + quintuple-dollar: $$an-envvar-value-suffix + quintuple-dollar-no-curly-braces: prefix-$$an-envvar-value + sextuple-dollar: $$${env:AN_ENVVAR}-suffix + sextuple-dollar-no-curly-braces: prefix-$$$env:AN_ENVVAR + septuple-dollar: $$$an-envvar-value-suffix + septuple-dollar-no-curly-braces: prefix-$$$an-envvar-value + octuple-dollar: $$$${env:AN_ENVVAR}-suffix + octuple-dollar-no-curly-braces: prefix-$$$$env:AN_ENVVAR + nonuple-dollar: $$$$an-envvar-value-suffix + nonuple-dollar-no-curly-braces: prefix-$$$$an-envvar-value + decuple-dollar: $$$$${env:AN_ENVVAR}-suffix + decuple-dollar-no-curly-braces: prefix-$$$$$env:AN_ENVVAR