Skip to content

Commit 684b19d

Browse files
ocelotlxrmx
andauthored
Substitute empty for unset config env vars without defaults (#5408)
* Substitute empty for unset config env vars without defaults Declarative config environment variable substitution rejected any ${VAR} reference to an unset variable that had no default, raising a ConfigurationError and preventing SDK initialization. The declarative configuration spec requires an unset variable without a default to be replaced with an empty value (which YAML then reads as null), matching the Java and Node.js implementations and allowing config files to be shared across languages. Replace the raise with empty substitution. EnvSubstitutionError is kept as public API for backward compatibility but is no longer raised. Update tests accordingly. Fixes #5405 * opentelemetry-configuration: remove EnvSubstitutionError, guard null resource attributes Address review feedback on #5408: - Remove the now-unused EnvSubstitutionError class and its public export. Nothing raises it since unset variables substitute an empty value. - Skip resource attributes whose value resolves to null (an unset ${VAR} with no default) and log a warning, instead of inserting a None value or coercing it into garbage. create_resource and _coerce_attribute_value did not guard None, so before this the empty substitution would have propagated a null attribute value into the SDK Resource. * opentelemetry-configuration: deprecate EnvSubstitutionError instead of removing EnvSubstitutionError shipped in the released 0.65b0, so removing it from the public API broke the public-symbols-check (griffe). Restore it as a public export marked with typing_extensions.deprecated; it is no longer raised now that an unset variable without a default substitutes an empty value. Declare typing-extensions as a direct dependency since it is now imported directly. * opentelemetry-configuration: remove EnvSubstitutionError, simplify unset substitution Per review, the beta opentelemetry-configuration package may make breaking changes, so remove EnvSubstitutionError (and its now-unnecessary typing-extensions dependency) rather than deprecating it. The public-symbols check is bypassed via the 'Approve Public API check' label. Also simplify the unset-variable branch to 'return default_value or ""', which already covers the no-default case. * Update 5408.fixed --------- Co-authored-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
1 parent 1a71171 commit 684b19d

8 files changed

Lines changed: 81 additions & 45 deletions

File tree

.changelog/5408.fixed

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
`opentelemetry-configuration`: declarative config environment variable substitution now replaces an unset variable that has no default with an empty value instead of raising an error, per the configuration spec.
2+
Resource attributes whose value resolves to null (an unset `${VAR}` with no default) are skipped with a warning instead of being inserted as a null value.

opentelemetry-configuration/src/opentelemetry/configuration/_resource.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,15 @@ def create_resource(config: ResourceConfig | None) -> Resource:
113113

114114
if config.attributes:
115115
for attr in config.attributes:
116+
# An unset ${VAR} with no default substitutes an empty value that
117+
# the YAML parser reads as null. Skip such attributes rather than
118+
# inserting a None (or coercing it into garbage like "None"/0).
119+
if attr.value is None:
120+
_logger.warning(
121+
"Ignoring resource attribute '%s' with empty value",
122+
attr.name,
123+
)
124+
continue
116125
config_attrs[attr.name] = _coerce_attribute_value(attr)
117126

118127
schema_url = config.schema_url

opentelemetry-configuration/src/opentelemetry/configuration/file/__init__.py

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@
3636
create_tracer_provider,
3737
)
3838
from opentelemetry.configuration.file._env_substitution import (
39-
EnvSubstitutionError,
4039
substitute_env_vars,
4140
)
4241
from opentelemetry.configuration.file._loader import load_config_file
@@ -47,7 +46,6 @@
4746
"substitute_env_vars",
4847
"ConfigurationError",
4948
"MissingDependencyError",
50-
"EnvSubstitutionError",
5149
"create_resource",
5250
"create_propagator",
5351
"configure_propagator",

opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py

Lines changed: 11 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -3,38 +3,31 @@
33

44
"""Environment variable substitution for configuration files."""
55

6-
import logging
76
import os
87
import re
98

10-
_logger = logging.getLogger(__name__)
11-
12-
13-
class EnvSubstitutionError(Exception):
14-
"""Raised when environment variable substitution fails.
15-
16-
This occurs when a ${VAR} reference is found but the environment
17-
variable is not set and no default value is provided.
18-
"""
19-
209

2110
def substitute_env_vars(text: str) -> str:
2211
"""Substitute environment variables in configuration text.
2312
2413
Supports the following syntax:
25-
- ${VAR}: Substitute with environment variable VAR. Raises error if not found.
14+
- ${VAR}: Substitute with environment variable VAR, or an empty value if
15+
VAR is not set.
2616
- ${VAR:-default}: Substitute with VAR if set, otherwise use default value.
2717
- $$: Escape sequence for literal $.
2818
19+
Per the declarative configuration specification, a referenced environment
20+
variable that is not set and has no default is replaced with an empty
21+
value (which the YAML parser then interprets as null). This matches the
22+
behavior of the Java and Node.js implementations and lets configuration
23+
files be shared across languages.
24+
2925
Args:
3026
text: Configuration text with potential ${VAR} placeholders.
3127
3228
Returns:
3329
Text with environment variables substituted.
3430
35-
Raises:
36-
EnvSubstitutionError: If a required environment variable is not found.
37-
3831
Examples:
3932
>>> os.environ['SERVICE_NAME'] = 'my-service'
4033
>>> substitute_env_vars('name: ${SERVICE_NAME}')
@@ -60,15 +53,9 @@ def replace_var(match) -> str:
6053
value = os.environ.get(var_name)
6154

6255
if value is None:
63-
if has_default:
64-
return default_value or ""
65-
_logger.error(
66-
"Environment variable '%s' not found and no default provided",
67-
var_name,
68-
)
69-
raise EnvSubstitutionError(
70-
f"Environment variable '{var_name}' not found and no default provided"
71-
)
56+
# An unset variable is replaced with its default if one is
57+
# provided, otherwise with an empty value, per the spec.
58+
return default_value or ""
7259

7360
# Per spec: "It MUST NOT be possible to inject YAML structures by
7461
# environment variables." Newlines are the primary injection vector —

opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,6 @@ def load_config_file(
8282
8383
Raises:
8484
ConfigurationError: If file cannot be read, parsed, or validated.
85-
EnvSubstitutionError: If required environment variable is missing.
8685
8786
Examples:
8887
>>> config = load_config_file("otel-config.yaml")

opentelemetry-configuration/tests/file/test_env_substitution.py

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,7 @@
77

88
import yaml
99

10-
from opentelemetry.configuration.file import (
11-
EnvSubstitutionError,
12-
substitute_env_vars,
13-
)
10+
from opentelemetry.configuration.file import substitute_env_vars
1411

1512

1613
class TestEnvSubstitution(unittest.TestCase):
@@ -40,12 +37,17 @@ def test_substitution_with_default_override(self):
4037
result = substitute_env_vars("name: ${SERVICE_NAME:-default}")
4138
self.assertEqual(result, "name: actual")
4239

43-
def test_missing_variable_raises_error(self):
44-
"""Test ${VAR} raises error when variable missing."""
40+
def test_missing_variable_without_default_substitutes_empty(self):
41+
"""An unset ${VAR} without a default is replaced with an empty value.
42+
43+
Per the declarative configuration spec, unset variables without
44+
defaults are replaced with an empty value, which YAML then reads as
45+
null. This matches the Java and Node.js implementations.
46+
"""
4547
with patch.dict(os.environ, {}, clear=True):
46-
with self.assertRaises(EnvSubstitutionError) as ctx:
47-
substitute_env_vars("name: ${MISSING_VAR}")
48-
self.assertIn("MISSING_VAR", str(ctx.exception))
48+
result = substitute_env_vars("name: ${MISSING_VAR}")
49+
self.assertEqual(result, "name: ")
50+
self.assertIsNone(yaml.safe_load(result)["name"])
4951

5052
def test_dollar_sign_escape(self):
5153
"""Test $$ escapes to literal $."""

opentelemetry-configuration/tests/file/test_loader.py

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -137,19 +137,25 @@ def test_non_dict_root(self):
137137
finally:
138138
os.unlink(temp_path)
139139

140-
def test_missing_required_env_var(self):
141-
"""Test error when required env var is missing."""
140+
def test_unset_env_var_without_default_substitutes_empty(self):
141+
"""An unset env var without a default resolves to an empty value.
142+
143+
Per the declarative configuration spec, an unset ``${VAR}`` reference
144+
with no default is replaced with an empty value rather than raising,
145+
so the file still loads. A default (``${ENV:-production}``) is still
146+
applied when its variable is unset.
147+
"""
142148
config_path = self.test_data_dir / "config_with_env_vars.yaml"
143149

144150
with patch.dict(os.environ, {}, clear=True):
145-
with self.assertRaises(ConfigurationError) as ctx:
146-
load_config_file(str(config_path))
151+
config = load_config_file(str(config_path))
147152

148-
# Should mention substitution or env var error
149-
self.assertTrue(
150-
"substitution" in str(ctx.exception).lower()
151-
or "environment" in str(ctx.exception).lower()
152-
)
153+
attributes = {
154+
attribute.name: attribute.value
155+
for attribute in config.resource.attributes
156+
}
157+
self.assertIsNone(attributes["service.name"])
158+
self.assertEqual(attributes["deployment.environment"], "production")
153159

154160
def test_yml_extension(self):
155161
"""Test .yml extension is supported."""

opentelemetry-configuration/tests/test_resource.py

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,39 @@ def test_attribute_type_bool_array(self):
224224
resource = create_resource(config)
225225
self.assertEqual(list(resource.attributes["k"]), [True, False]) # type: ignore[arg-type]
226226

227+
def test_none_value_attribute_skipped_with_warning(self):
228+
"""An unset ${VAR} with no default yields a null value; it must be
229+
skipped (not inserted as None or coerced) and a warning logged."""
230+
with self.assertLogs(
231+
"opentelemetry.configuration._resource", level="WARNING"
232+
) as cm:
233+
config = ResourceConfig(
234+
attributes=[
235+
AttributeNameValue(name="empty", value=None),
236+
AttributeNameValue(name="env", value="production"),
237+
]
238+
)
239+
resource = create_resource(config)
240+
self.assertNotIn("empty", resource.attributes)
241+
self.assertEqual(resource.attributes["env"], "production")
242+
self.assertTrue(any("empty" in msg for msg in cm.output))
243+
244+
def test_none_value_typed_attribute_skipped(self):
245+
"""A null value with a declared type must be skipped, not coerced
246+
(int(None)/str(None) would raise or produce garbage)."""
247+
with self.assertLogs(
248+
"opentelemetry.configuration._resource", level="WARNING"
249+
):
250+
config = ResourceConfig(
251+
attributes=[
252+
AttributeNameValue(
253+
name="k", value=None, type=AttributeType.int
254+
)
255+
]
256+
)
257+
resource = create_resource(config)
258+
self.assertNotIn("k", resource.attributes)
259+
227260
def test_attribute_type_bool_array_string_values(self):
228261
"""bool_array must use _coerce_bool, not plain bool() — 'false' must be False."""
229262
config = ResourceConfig(

0 commit comments

Comments
 (0)