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

[file configuration] Escaping environment variable substitution #3914

Open
Tracked by #3963
pellared opened this issue Feb 29, 2024 · 7 comments
Open
Tracked by #3963

[file configuration] Escaping environment variable substitution #3914

pellared opened this issue Feb 29, 2024 · 7 comments
Labels
area:configuration Related to configuring the SDK sig-issue A specific SIG should look into this before discussing at the spec spec:miscellaneous For issues that don't match any other spec label triage:accepted:ready Ready to be implemented. Small enough or uncontroversial enough to be implemented without sponsor

Comments

@pellared
Copy link
Member

pellared commented Feb 29, 2024

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 pellared added the spec:miscellaneous For issues that don't match any other spec label label Feb 29, 2024
@pellared
Copy link
Member Author

FYI @marcalff

@marcalff
Copy link
Member

marcalff commented Feb 29, 2024

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
Copy link
Member Author

pellared commented Feb 29, 2024

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
Copy link
Member

trask commented Feb 29, 2024

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?

@pellared
Copy link
Member Author

pellared commented Mar 1, 2024

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
Copy link
Member

marcalff commented Mar 3, 2024

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 jack-berg added the [label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR label Mar 13, 2024
@jack-berg jack-berg added the area:configuration Related to configuring the SDK label Mar 25, 2024
@jack-berg
Copy link
Member

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.

@svrnm svrnm added sig-issue A specific SIG should look into this before discussing at the spec triage:accepted:ready Ready to be implemented. Small enough or uncontroversial enough to be implemented without sponsor and removed [label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR labels Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:configuration Related to configuring the SDK sig-issue A specific SIG should look into this before discussing at the spec spec:miscellaneous For issues that don't match any other spec label triage:accepted:ready Ready to be implemented. Small enough or uncontroversial enough to be implemented without sponsor
Development

No branches or pull requests

6 participants