opentelemetry-configuration: substitute env vars after parsing (#5406) - #5407
opentelemetry-configuration: substitute env vars after parsing (#5406)#5407ocelotl wants to merge 10 commits into
Conversation
|
|
||
| # Perform environment variable substitution | ||
| try: | ||
| content = substitute_env_vars(content) |
There was a problem hiding this comment.
This is where the bug (#5406) was. substitute_env_vars was applied to the entire raw file text at this point, before the file was parsed (yaml.safe_load / json.loads further down).
Because it operated on the unparsed text, ${VAR} references inside YAML comments — and inside mapping keys — were substituted exactly like real values. A comment such as # default is ${LEGACY_NAME} that referenced an undefined variable therefore raised EnvSubstitutionError and aborted loading, and any ${VAR} in a comment was silently rewritten.
This PR removes this pre-parse step and instead substitutes per configuration value after parsing (_parse_config_content → _substitute_env_in_yaml_node / _substitute_env_in_json_value), so comments and mapping keys are never substitution candidates, while node types are still resolved after substitution.
e079f84 to
8f8b4b2
Compare
8de1d26 to
83a2a87
Compare
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
83a2a87 to
1acbc84
Compare
8bcf494 to
0ce0994
Compare
xrmx
left a comment
xrmx
left a comment
There was a problem hiding this comment.
Added a comment but the rest LGTM
2552970 to
04471b6
Compare
04471b6 to
56e5fe5
Compare
Pull request dashboard statusWaiting on maintainers · refreshed 2026-08-24 18:34 UTC Merge when ready. Status above doesn't look right?
|
This comment has been minimized.
This comment has been minimized.
…telemetry#5406) Environment variable substitution ran on the raw configuration file text before parsing, so a ${VAR} reference inside a YAML comment was treated as a real substitution -- an undefined variable in a comment crashed loading, and documenting the substitution mechanism was impossible. Parse the file first, then substitute only within scalar values (never in comments or mapping keys), matching the configuration spec and the Java and Node.js implementations. For an unquoted standalone ${VAR} reference the YAML node's type tag is re-resolved from the substituted value so type coercion still applies (e.g. ${LIMIT} -> int); quoted or embedded references resolve to strings. JSON string values are substituted in place.
substitute_env_vars is re-exported in opentelemetry.configuration.file.__all__, so renaming its parameter is a breaking public API change. Restore the parameter name to text to satisfy the public-symbols-check while keeping the docstring that explains the configuration-value semantics.
56e5fe5 to
36d3452
Compare
An alias resolves to the same composed node as its anchor, so the walker
reached one node once per reference to it. That substituted the node
repeatedly, which re-read the previous pass's output: an anchored
$${TOKEN} became the literal ${TOKEN} on the first visit, and the second
visit resolved it as a real variable, defeating the escape and exposing
the value. Merge keys took the same path, since a merge key's value node
is the anchored mapping itself. A cyclic alias made the node tree a
graph with a loop, so the walk recursed until it raised RecursionError.
Thread a set of visited node ids through the traversal and return early
on a node already seen.
4 participants
Description
Environment variable substitution ran on the raw configuration file text
before parsing, so a
${VAR}reference inside a YAML comment was treated asa real substitution. An undefined variable referenced only in a comment raised
EnvSubstitutionErrorand aborted loading, and self-documenting a configfile's use of the substitution mechanism was impossible.
This changes the loader to parse the file first and then substitute
environment variables only within scalar values (never in comments or mapping
keys), matching the configuration spec
and the Java/Node.js implementations. Substitution is applied at the YAML node
level so node types are still interpreted after substitution: an unquoted
standalone
${VAR}takes the resolved value's YAML type (e.g.${LIMIT}→int), while quoted
"${VAR}"and embeddedfoo ${VAR}references resolve tostrings. JSON string values are substituted in place.
Fixes #5406
Type of change
How Has This Been Tested?
opentelemetry-configuration/tests/file/test_loader.py:an undefined
${VAR}in a comment no longer crashes loading; unquotedstandalone references are type-coerced (and pass JSON-schema validation for
integer fields); quoted references stay strings; embedded references resolve
to strings; mapping keys and
$$escapes are left untouched; and a valuecontaining a newline cannot inject new mapping keys.
test_env_substitution.pyfor the new post-parse, scalar-levelbehavior.
pytest opentelemetry-configuration/tests→ 372 passed;pylint10/10;ruff check/ruff format --checkclean.Does This PR Require a Contrib Repo Change?
Checklist: