Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define syntax for escaping declarative config env var substitution #4375

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jack-berg
Copy link
Member

@jack-berg jack-berg commented Jan 17, 2025

Resolves #3914.

An environment variable substitution reference which starts with $$ (i.e. $${API_KEY}) is escaped. Implementations should strip the leading $ character and not perform substitution (i.e. $${API_KEY} => ${API_KEY}).

See java implementation here: open-telemetry/opentelemetry-java#7033

@tsloughter
Copy link
Member

Why $ and not \? The latter being what I'd expect and try first when using this since it is what I'd do in a shell script.

Also doesn't this break being able to do something like $${NUM_OF_DOLLARS}?

Copy link
Member

@tsloughter tsloughter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving despite my comments since $$ is what the collector uses and need for $${DOLLARS} seem unnecessary in an SDK configuration :)

Copy link
Member

@marcalff marcalff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The escape sequence is not fully specified.

If I want ${API_KEY} in the environment variable value, this says it should be encoded as $${API_KEY}, ok.

If I want $${API_KEY} in the value, how should this be encoded ?

If '$' is used to as an escape character, then the spec must have a well defined way to print a $ non escaping character, so that any string containing $ can be represented.

@marcalff
Copy link
Member

The escape sequence is not fully specified.

If I want ${API_KEY} in the environment variable value, this says it should be encoded as $${API_KEY}, ok.

If I want $${API_KEY} in the value, how should this be encoded ?

If '$' is used to as an escape character, then the spec must have a well defined way to print a $ non escaped character, so that any string containing $ can be represented.

Resolution could be to say that $${API_KEY} is encoded as $$${API_KEY}, and so forth, but this should be in the spec and not open to interpretation.

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should do it simply in the same way as docker compose. See https://docs.docker.com/reference/compose-file/interpolation/

You can use a $$ (double-dollar sign) when your configuration needs a literal dollar sign.

Is also looks that it is what OTel Collector is doing as well:

@jack-berg
Copy link
Member Author

Maybe we should do it simply in the same way as docker compose. See https://docs.docker.com/reference/compose-file/interpolation/

You can use a $$ (double-dollar sign) when your configuration needs a literal dollar sign.

Is also looks that it is what OTel Collector is doing as well:

This is the syntax proposed in this PR.

@pellared
Copy link
Member

pellared commented Jan 22, 2025

This is the syntax proposed in this PR.

I disagree. According to this PR something like "a $$ b" is evaluated to a $$ b whereas for OTel Collector and Docker Compose it will be evaluated to a $ b. Meaning in both OTel Collector and Docker Compose $$ is always escaped to $ and not only when it is matching the env var substitution regex.

@jack-berg
Copy link
Member Author

jack-berg commented Jan 22, 2025

@tsloughter:

Approving despite my comments since $$ is what the collector uses

Yes exactly. Matching collector syntax is the motivation here.


@pellared:

I disagree. According to this PR something like "a $$ b" is evaluated to a $$ b whereas for OTel Collector and Docker Compose it will be evaluated to a $ b. Meaning in both OTel Collector and Docker Compose $$ is always escaped to $ and not only when it is matching the env var substitution regex.

I see. That's fair.

When I was writing this PR I did look at the collector's language, which is similar to docker's, and which I found to be overly simplistic:

Use $$ to indicate a literal $.

Its missing guidance on how $$ interacts with environment variable substitution - specifically, the ordering.

For example, suppose we have: $${FOO} and FOO=bar.

If we perform env env vars substitution after $$ substitution, we get $${FOO} => ${FOO} => bar, which is not what the user wants.

Alternatively, env var substitution might take place before $$ substitution, we get $${FOO} => $bar, which is not what the user wants.

So $$ subsitution can't happen before or after env var substitution. Instead, it must happen simultaneously as a part of env var substitution. Hence, my inclusion of it in the env var regular expression.

You also gave an example of "a $$ b" evaluating to "a $ b". What is the use of escaping $ outside of references to env var substitution? A user who wishes to resolve "a $ b" can just enter that value as is - no escape needed.


@marcalff

If I want $${API_KEY} in the value, how should this be encoded ?
Resolution could be to say that $${API_KEY} is encoded as $$${API_KEY}, and so forth, but this should be in the spec and not open to interpretation.

My understanding is that the regular expression / logic as proposed in this PR will indeed resolve $$${API_KEY} to be $${API_KEY}.

The reason is that the regular expression \${1,2}\{(?:env:)?(?<ENV_NAME>[a-zA-Z_][a-zA-Z0-9_]*)(:-(?<DEFAULT_VALUE>[^\n]*))?\} matches exactly 1 OR 2 $ characters. So for $$${API_KEY}, the first $ is unmatched and left alone. The following characters, $${API_KEY}, matches the expression, but since it starts with $$, it is escaped instead with the first $ dropped instead of replaced. So in total we have $ + ${API_KEY} = $${API_KEY}.

I added an additional test case to my java prototype implementation of this. Note - no changes to the implementation were required to make this test pass.

More examples in the specification might be helpful.

@tsloughter
Copy link
Member

Put this comment in the wrong repo:

Oh, now I see the collector uses $$. Then I suppose this is the way to go. May want to also mention additional escaping like allowing $${DOLLARS} to be $10?

@pellared
Copy link
Member

@jack-berg, I find the Collector and and Docker Compose docs clearer. $${FOO} is evaluated to ${FOO}.

You also gave an example of "a $$ b" evaluating to "a $ b". What is the use of escaping $ outside of references to env var substitution? A user who wishes to resolve "a $ b" can just enter that value as is - no escape needed.

This will provide different experience for users who have experience with Collector and Docker Compose who are used to always type $$ when they want to add a $ literal. For instance, $$$${FOO} should be evaluated to $${FOO}.

I agree with the intention to match with Collector (and Docker Compose) syntax 👍

@jack-berg
Copy link
Member Author

I find the Collector and and Docker Compose docs clearer. $${FOO} is evaluated to ${FOO}.

Its seems clear on paper until you try to implement it. The docker and collector specs are incomplete.

Why does $${FOO} resolve to ${FOO} and not $${FOO} => $bar (env var before $$) or $${FOO} => ${FOO} => bar (env var after $$)?

If you try to implement this, and treat $$ => $ substitution separately from env var substitution, then you need to answer the question of ordering: which comes first? And as I noted above, both possible answers of ordering yield incorrect results.

I would prefer to match the algorithm of the collector, but looking for advice on how to describe this in such a way that 11 language implementations will get it right. Despite the seemingly simple syntax as described in docs, the collector's substitution logic is quite complex / nuanced. The current env var substitution allows for simpler implementations that can leverage regular expressions to do the heavy lifting.

I considered trying to get the both of both worlds (i.e. collector behavior and a regular expression based spec), by changing the regular expression to match:

\${1,2}(\{(?:env:)?(?<ENV_NAME>[a-zA-Z_][a-zA-Z0-9_]*)(:-(?<DEFAULT_VALUE>[^\n]*))?\})?

I.e. make the whole {API_KEY:-default} part optional so the expression still matches against just $$, but this still has ordering challenges. Consider @marcalff's example of wanting a resolved string of $${API_KEY}. How do you accomplish this? With the updated regular expression and assuming API_KEY=value, $$${API_KEY}, resolves to $value. This is because the expression matches twice:

  • the first $$ matches and resolves to $
  • the second ${API_KEY} matches and resolves to value

I'm actually not sure there's a way to accomplish the collector's behavior without requiring implementations to carefully recreate its complex recursive algorithm which statefully walks values, character by character. (Not sure how I would approach trying to to specify this.) And I think a regular expression based approach is essential to having a low enough complexity bar to implement.

@pellared
Copy link
Member

Docker composer also does it recursively: https://github.com/compose-spec/compose-go/blob/main/interpolation/interpolation.go

looking for advice on how to describe this in such a way that 11 language implementations will get it right

I think "specification by example" will help achieving it. Meaning specifying expected results for given input.

regular expression based spec

Maybe this would help: https://github.com/compose-spec/compose-go/blob/main/template/template.go

collector's substitution logic is quite complex / nuanced

I'm actually not sure there's a way to accomplish the collector's behavior without requiring implementations to carefully recreate its complex recursive algorithm which statefully walks values, character by character.

As far as I remember, I was calling it out long ago during some SIG meeting that env var substitution is complex. I think that I even said that each language should have fuzz tests for such parsing because of its complexity.

I was proposing that the users could simply use envsubst so that each SDK could make the parsing of the files a lot simpler and less bug-prone.

However, the feedback was that the Configuration SIG wants to try (challenge) it given the assumption that users need it.

If this won't be working like in OTel Collector then I think we will be giving bad user experience.

Given that OTel Collector is still not stable, the alternative way could be simplifying the env var substitution in both Collector config and SDK config.

@pellared
Copy link
Member

pellared commented Jan 22, 2025

I was proposing that the users could simply use envsubst so that each SDK could make the parsing of the files a lot simpler and less bug-prone.

We could also provide a seperate OTel tool which does the env var substitution in the same way as the OTel Collector.
This way we would do not have to implement it in each language, but just reuse the OTel Collector code.
We could add env var interpolation as an opt-in feature in the SDKs (enabled via an env var of course 😉).
I think this would be a more pragmatic way of introducing YAML configs to the SDKs.

@jack-berg
Copy link
Member Author

As far as I remember, I was calling it out long ago during some SIG meeting that env var substitution is complex.

It doesn't have to be complex. With a regex-based specification, the java implementation is only 28 lines long, plus a little more boiler plate to make sure that logic is invoked by the YAML parser.

We could also provide a seperate OTel tool which does the env var substitution in the same way as the OTel Collector.

We actually already have this. The configuration validator is a tool written in go, and available as a binary or docker container, which accepts a target YAML config file and:

  • performs env var substiution
  • validates the contents against the configuration JSON schema

The problem is, not all ecosystems will want or be able to bundle in a binary specify to a particular platform / architecture. So this tool is available to help implementations, but we cannot rely on it being used everywhere.

If this won't be working like in OTel Collector then I think we will be giving bad user experience.
Given that OTel Collector is still not stable, the alternative way could be simplifying the env var substitution in both Collector config and SDK config.

As of this PR, the differences I know of are:

  • collector substitutes $$ for $ even if not part of an env var substitution ref
  • collector supports reference values from other provides besides env vars, which configuration has punted on to reduce maintenance burden.

So there are already differences between collector and language implementations. Its probably safe to say that the env var substitution for declarative config is a subset of the collector. And this seems fine to me. Especially if its a strict subset (i.e. declarative config takes care not to introduce new things the collector will not add).

Not sure if @open-telemetry/collector-maintainers would be open to it, but it doesn't seem like much is lost by removing generic support for replacing $$ with $ outside of env var substitution references. WDYT?

I get the appeal "docker does this" argument, but docker env var substitution breaks from collector substitution and declarative configuration is other key ways. All try to have a syntax inspired by shell-style substitution, but each supports shell-style to varying extents, and the collector extends it with the provider concept which is distinctly different.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[file configuration] Escaping environment variable substitution
6 participants