Add support in the confmap.Resolver to expand embedded config URIs inside configuration

[bogdandrutu]

Proposal on how we can extend our current config provider to support expanding (including) config "parts". This will fix #4231 and will fully implement same capabilities as proposed in #4468.

This design is based on the https://docs.google.com/document/d/1XlDpS8-a4Ds6uoe6C6WgO8mCpDaQNufcHSEmBpiCdI0/edit format which we already support for the "--config" flag.

confmap.Provider changes

Extend the capability of a confmap.Provider to return a Provided value that can encapsulate a confmap.Conf or any partial config value (float64/int64/bool/string/map[string]interface{}/[]interface{} where map values or slice values have the same restrictions). The current design allows recursive expansions, so values returned by expanding one embedded config URI will be searched for expanding again.

Known caveats

1. The proposed mechanism does not allow expanding values within other values, for example this case ${http://example.com/get_config?os=${env:OSNAME}} will NOT expand first the OSNAME env var.

Examples of usages

Retrieving token from zookeeper (scheme not yet defined)

exporters:
  signalfx:
    # this will read the access_token value from "zk" config provider.
    access_token: ${zk:zk1,zk2,zk2/path/to/data}

Retrieving an entire exporter config from a file

exporters:
  # this will read the entire config for the otlp/file exporter from the given file
  otlp/file_config: ${file:path/to/config/file/for/the/exporter.yaml}

Expanding env vars

exporters:
  otlp:
    endpoint: ${env:MY_ENV}

  otlp/backwards_compatible:
    endpoint: ${MY_ENV}

Expand Converter vs embedded config

The current proposal to support embedding config URIs, does not support the following cases that are supported by the expand envvar Converter:

1. Using $ without brackets:

exporters:
  otlp/another_way_to_use_env_vars:
    endpoint: $MY_ENV

2. Expanding values within a string value and concatenate:

exporters:
  otlp/localhost_enpoint:
    endpoint: "localhost:${OTLP_PORT_ENV}"

Because of these missing features the Expand Converter will be maintain for a period of time until a final decision between supporting or not the 2 missing features is made. If we decide to not support any of these features we can start at least by logging errors and disable the functionality behind a featuregate until we know if this is needed or not.

Signed-off-by: Bogdan Drutu [email protected]

[codeboten]

I like where this functionality is going, I would love to hear from folks that are implementing downstream providers whether they have any concerns with this approach. To be clear this mean NewExpandConverter remains as the public API to use moving forward?

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[codecov]

Codecov Report

Merging #4742 (3665334) into main (dc42194) will increase coverage by 0.16%.
The diff coverage is 99.03%.

@@            Coverage Diff             @@
##             main    #4742      +/-   ##
==========================================
+ Coverage   91.65%   91.82%   +0.16%     
==========================================
  Files         192      191       -1     
  Lines       11467    11521      +54     
==========================================
+ Hits        10510    10579      +69     
+ Misses        764      751      -13     
+ Partials      193      191       -2     

Impacted Files	Coverage Δ
confmap/resolver.go	97.22% <98.71%> (+4.91%)	⬆️
confmap/provider.go	100.00% <100.00%> (ø)
confmap/provider/internal/provider.go	100.00% <100.00%> (ø)
...ernal/internalconsumertest/err_or_sink_consumer.go

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[tigrannajaryan]

To confirm: we don't want to apply expandValue recursively to the returned result here?

[bogdandrutu]

How many times would we do this? Until no changes (can get into infinite loop, maybe try for 10 times if still resolving then return error?)?

I thought it is not useful, but I can see now point in having it (after you asked for it). Let's discuss the behavior.

[tigrannajaryan]

I can imagine 2 use cases:

• If you have a local config file where env variables are used (quite common, e.g. in Splunk distro) you may want to specify that the value must be retrieved from a remote source instead of providing it directly.
• The opposite: a config fetched from a remote location, but which refers to a local env variables (e.g. refer to $PATH or $TEMP).

How many times would we do this? Until no changes (can get into infinite loop, maybe try for 10 times if still resolving then return error?)?

Yeah, probably repeat until no more changes with some limit to avoid infinite looping. Any reasonable use case I can imagine should be 2 rounds, so limiting to something like 5 or 19 attempts should be fine.

[mx-psi]

I would suggest not introducing recursive expanding on this PR. We can forbid usage of $, { and } in the URI on this PR and deal with this problem later

[bogdandrutu]

There are two problems, and probably we should address both, with different solutions:

1. Embedding ${http://my.example.com?os=${env:OS_ENV}}
2. Recursive ${file:/path/to/my/file} which returns a section of the config as a map[string]interface{} for example like {"key": ${env:VALUE}}. Then should we expand ${env:VALUE}?

For 1. I think we can simply error now. For 2. I think we cannot error, we need to resolve that, and that is what @tigrannajaryan was referring to.

[tigrannajaryan]

Yes, I had the second case in my mind.
I am fine with deferring this to a future PR.

[mx-psi]

👍 Thanks both for clarifying. I think Tigran's suggestions above make sense and it's fine to support this now or later, whatever is easier

[jeromeinsf]

I am not clear at this point whether this can actually be introduced later. Will introducing embedding and recursive feature be an implementation detail or will it affect the user contract (like allowed symbols) ?

[mx-psi]

@jeromeinsf I think everyone agrees on leaving the implementation of (1) for later and restricting the allowed characters now. For (2), I believe @bogdandrutu wants to add support now with some recursion limit

[bogdandrutu]

(2) - Done

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 14 days.

[mx-psi]

I only have one remaining question

[mx-psi]

Something like ${HOST_IP}:4317 does not get expanded then? Why don't we use os.Expand like we are using on the expand provider?

[bogdandrutu]

Because os.Expand only supports returning strings (not a partial sub-map).

See https://pkg.go.dev/os#Expand
func Expand(s string, mapping func(string) string) string

[mx-psi]

Right, but the problem of dealing with ${HOST_IP}:4317 still remains then? That would only be supported by environment variables?

[bogdandrutu]

Do we want to support this? What is a good use-case? I don't see a reason to have ${HOST_IP}:4317, why hardcoded the port vs providing everything?

[mx-psi]

Here are some examples on contrib using this with environment variables:

• sapm exporter (see endpoint)
• kubeletstats receiver (see endpoint)
• basicauth extension (see inline)
• Splunk's distro examples (this one is a host:port example actually :))

Why should environment variables work differently from other providers?

[mx-psi]

The ${THING_A}${THING_B} pattern is also interesting. Right now, IIUC, we interpret this as "get the thing with URI THING_A}${THING_B", which also seems counter to what I would expect to happen (I guess this relates to #5706 too).

[bogdandrutu]

To make progress, I disabled this feature from the Resolver. For the moment no way to enable this, but will send another PR that addresses the remaining issues and use a featuregate to enable this maybe.

[mx-psi]

Makes sense. Sorry to bring this up so late, I hadn't realized until now

[mx-psi]

I created #5706 for the character set allowed in URIs. I think we can deal with this independently, since we potentially already have this problem with the environment variables (in principle any character except for = and NUL is allowed)