Fix bug requiring $$ when using config source variables #1058
Conversation
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.
ac23d0f to
7c1465a
Compare
|
There's an existing integration test that would need to be updated/could prove useful for this: https://github.com/signalfx/splunk-otel-collector/blob/main/tests/general/envvar_config_source_test.go#L33 |
| } | ||
|
|
||
| func dollarDollarRegex() *regexp.Regexp { | ||
| return regexp.MustCompile(`\$\${(.+?:.+?)}`) |
There was a problem hiding this comment.
Is it preferable to obtain via a function and not a global var?
| // 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 { |
There was a problem hiding this comment.
It's not only ${foo:MY_VAR} that is affected. Any env variables added to the config currently has to have the following $${ENV_VAR} format to work properly, and, if user wants to have a dollar sign, it must be escaped as $$$$.
I'm not sure if we want to implicitly translate any of the use cases.
I would suggest just adding a warning message for any encountered value with $${ and $$$$ but do not change them implicitly. Also we should make a clear statement about fixing this bug as a breaking change, add a line to changelog and to the upgrade guideline.
@pmcollins @rmfitzpatrick WDYT?
There was a problem hiding this comment.
Added comment assuming that the use cases I outlined are fixed as well, let me know if it's not the case
There was a problem hiding this comment.
It's not only
${foo:MY_VAR}that is affected. Any env variables added to the config currently has to have the following$${ENV_VAR}format to work properly, and, if user wants to have a dollar sign, it must be escaped as$$$$.
I don't think this is accurate and $ENVVAR and ${ENVVAR} should be functional as is and after these changes (for example https://github.com/signalfx/splunk-otel-collector/blob/main/tests/general/testdata/config_with_envvars.yaml#L9 and https://github.com/signalfx/splunk-otel-collector/blob/main/cmd/otelcol/config/collector/agent_config.yaml#L25).
If users want to have a $ literal I think it makes sense that they'd need to escape it somehow and $$ makes the most sense to me. This should be clearly documented and ideally vetted with unit/integration tests.
There was a problem hiding this comment.
I don't think this is accurate and
$ENVVAR and $ {ENVVAR} should be functional as is and after these changes (for example https://github.com/signalfx/splunk-otel-collector/blob/main/tests/general/testdata/config_with_envvars.yaml#L9 and https://github.com/signalfx/splunk-otel-collector/blob/main/cmd/otelcol/config/collector/agent_config.yaml#L25).
The bug is that any env var is evaluated twice. So ${ENV_VAR} and $${ENV_VAR} both have the same result. Once ${ENV_VAR} evaluated first, second evaluation is skipped because then it's just a string.
If users want to have a $ literal I think it makes sense that they'd need to escape it somehow and $$ makes the most sense to me. This should be clearly documented and ideally vetted with unit/integration tests.
Yes, it should be $$ not $$$$ which we have to set because of this bug in the helm chart but it's not needed when contrib image is used
There was a problem hiding this comment.
Probably we can keep the implicit translation for some time, it shouldn't break existing configs, but in order to cover all the use cases we probably should just convert $$ -> $.
I would still clearly mark it in the changelog and translation guidelines and set expectation when this translation is removed. Let's say in 0.45.0?
There was a problem hiding this comment.
local testing with these changes:
config_sources:
env:
defaults:
HOST_METRICS_SCRAPERS_TO_EXPAND: {}
HOST_METRICS_SCRAPERS_DEFAULT_TO_USE: { cpu: null }
receivers:
hostmetrics:
collection_interval: $$HOST_METRICS_COLLECTION_INTERVAL
scrapers: ${env:HOST_METRICS_SCRAPERS_TO_EXPAND}
hostmetrics/default-env-config-source:
collection_interval: $${HOST_METRICS_COLLECTION_INTERVAL}
scrapers: $${env:HOST_METRICS_SCRAPERS_DEFAULT_TO_USE}
http://localhost:55554/debug/configz/effective:
receivers:
hostmetrics:
collection_interval: $HOST_METRICS_COLLECTION_INTERVAL
scrapers:
cpu: {}
disk: {}
filesystem: {}
load: {}
memory: {}
network: {}
paging: {}
processes: {}
hostmetrics/default-env-config-source:
collection_interval: ${HOST_METRICS_COLLECTION_INTERVAL}
scrapers:
cpu: null
What should the expected behavior be if not this?
There was a problem hiding this comment.
AFAIR from my testing sometime ago http://localhost:55554/debug/configz/effective doesn't show results of the second evaluation
There was a problem hiding this comment.
collector should not be able to start with the following part using contrib build
receivers:
hostmetrics:
collection_interval: $$HOST_METRICS_COLLECTION_INTERVAL
|
Given the double evaluation is fixed in #1099, i'm ok if we keep keep only |
|
Superseded by #1099 |
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 was because theexpandMapProvider, which lives incore and expands
${MY_ENV_VAR}-style variables, would run before the configsource providers, and would replace any
${env:MY_ENV_VAR}s with "" butconvert any
$${env:MY_ENV_VAR}s to${env:MY_ENV_VAR}. The fix proposed hereis to reverse the order in which these providers run: now the config source
providers run first, then
expandMapProviderruns. In addition, this changefixes up any
$${env:MY_ENV_VAR}workarounds users may have put in place.