open-telemetry / opentelemetry-specification

Specifications for OpenTelemetry
https://opentelemetry.io
Apache License 2.0
3.74k stars 887 forks source link

[file configuration] Escaping environment variable substitution #3914

Open pellared opened 8 months ago

pellared commented 8 months ago

What are you trying to achieve?

Reference: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/file-configuration.md#environment-variable-substitution

I would like to set a string value to ${NOT_ENV_VAR} and make sure it is not escaped.

Additional context.

I think that the user should take advantage of https://yaml.org/spec/1.2.2/#57-escaped-characters and do it e.g. like this:

key: "$\x7bNOT_ENV_VAR\x7d"

See: https://yaml-online-parser.appspot.com/?yaml=key%3A+%22%24%5Cx7bNOT_ENV_VAR%5Cx7d%22%0A&type=json

Even if this looks obvious, I would prefer to have it documented as the specification may also help in validating the implementation (and help in writing unit tests).

CC @jack-berg

pellared commented 8 months ago

FYI @marcalff

marcalff commented 8 months ago

I don't think this will work.

Whether the original yaml text contains \x7b or {, the yaml parser will return a scalar that contains { anyway, so code that inspects a scalar -- after the yaml parser is done -- will see an environment variable.

The $ sign is missing by the way, to make it an environment variable reference.

To escape variables, a new escape syntax is needed, independent of yaml.

pellared commented 8 months ago

The $ sign is missing by the way, to make it an environment variable reference.

Updated. Thanks.

I don't think this will work. [...] the yaml parser will return a scalar that contains

It is an implementation detail. I think the spec diverges so much from YAML (it is a superset) that for most SIGs it would require a dedicated parser.

Side note: That is one of the reasons why I am currently prejudiced towards having env var substitution for file configuration. For me, it is no longer a YAML. It makes the implementations more exposed to security bugs. It increases the implementation complexity. I am not convinced if the cost of this feature is worth its value. However, my opinions are never rock solid 😆

To escape variables, a new escape syntax is needed, independent of yaml.

Then we will be diverging from YAML even more. As literally we would add new escape syntax that is not defined by YAML.

trask commented 8 months ago

I think the spec diverges so much from YAML (it is a superset) that for most SIGs it would require a dedicated parser.

are you able to do env var substitution after YAML parsing, e.g. https://github.com/open-telemetry/opentelemetry-java/pull/5914?

pellared commented 8 months ago

I think the spec diverges so much from YAML (it is a superset) that for most SIGs it would require a dedicated parser.

are you able to do env var substitution after YAML parsing, e.g. open-telemetry/opentelemetry-java#5914?

If I understand correctly the code that you refer to, it makes the substitution during (not after, not before) the YAML parsing by extending StandardConstructor. It alters the default YAML parsing behavior.

In Go, we could create custom types that would support env var substitution and use them instead of primitives like int, string, float64 .

To sum up, probably most SIGs can indeed implement it.

marcalff commented 8 months ago

From:

https://github.com/open-telemetry/oteps/blob/main/text/0225-configuration.md#environment-variable-substitution

Pointing to:

https://opentelemetry.io/docs/collector/configuration/#environment-variables

Use $$ to indicate a literal $. For example, representing $DataVisualization would look like the following:

To be consistent with the collector, we can escape a single $ sign with $$ as well.

some_yaml_node: "$${NOT_ENV_VAR}"
jack-berg commented 7 months ago

From #3960:

In the Collector we support $${ENV} for this.

Should strive for consistency with collector unless there is a strong reason not to, in order to support #3963.