diff --git a/.changelog/5408.fixed b/.changelog/5408.fixed new file mode 100644 index 00000000000..eaaea1c00a3 --- /dev/null +++ b/.changelog/5408.fixed @@ -0,0 +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. 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. diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/_resource.py b/opentelemetry-configuration/src/opentelemetry/configuration/_resource.py index 199324405a8..79196c0d97e 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/_resource.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/_resource.py @@ -113,6 +113,15 @@ def create_resource(config: ResourceConfig | None) -> Resource: if config.attributes: for attr in config.attributes: + # An unset ${VAR} with no default substitutes an empty value that + # the YAML parser reads as null. Skip such attributes rather than + # inserting a None (or coercing it into garbage like "None"/0). + if attr.value is None: + _logger.warning( + "Ignoring resource attribute '%s' with empty value", + attr.name, + ) + continue config_attrs[attr.name] = _coerce_attribute_value(attr) schema_url = config.schema_url diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/__init__.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/__init__.py index 7be89f5ffd8..2c8c4ba48e6 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/__init__.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/__init__.py @@ -36,7 +36,6 @@ create_tracer_provider, ) from opentelemetry.configuration.file._env_substitution import ( - EnvSubstitutionError, substitute_env_vars, ) from opentelemetry.configuration.file._loader import load_config_file @@ -47,7 +46,6 @@ "substitute_env_vars", "ConfigurationError", "MissingDependencyError", - "EnvSubstitutionError", "create_resource", "create_propagator", "configure_propagator", diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py index 9d3a1b815aa..96696d8c395 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py @@ -3,38 +3,31 @@ """Environment variable substitution for configuration files.""" -import logging import os import re -_logger = logging.getLogger(__name__) - - -class EnvSubstitutionError(Exception): - """Raised when environment variable substitution fails. - - This occurs when a ${VAR} reference is found but the environment - variable is not set and no default value is provided. - """ - def substitute_env_vars(text: str) -> str: """Substitute environment variables in configuration text. Supports the following syntax: - - ${VAR}: Substitute with environment variable VAR. Raises error if not found. + - ${VAR}: Substitute with environment variable VAR, or an empty value if + VAR is not set. - ${VAR:-default}: Substitute with VAR if set, otherwise use default value. - $$: Escape sequence for literal $. + Per the declarative configuration specification, a referenced environment + variable that is not set and has no default is replaced with an empty + value (which the YAML parser then interprets as null). This matches the + behavior of the Java and Node.js implementations and lets configuration + files be shared across languages. + Args: text: Configuration text with potential ${VAR} placeholders. Returns: Text with environment variables substituted. - Raises: - EnvSubstitutionError: If a required environment variable is not found. - Examples: >>> os.environ['SERVICE_NAME'] = 'my-service' >>> substitute_env_vars('name: ${SERVICE_NAME}') @@ -60,15 +53,9 @@ def replace_var(match) -> str: value = os.environ.get(var_name) if value is None: - if has_default: - return default_value or "" - _logger.error( - "Environment variable '%s' not found and no default provided", - var_name, - ) - raise EnvSubstitutionError( - f"Environment variable '{var_name}' not found and no default provided" - ) + # An unset variable is replaced with its default if one is + # provided, otherwise with an empty value, per the spec. + return (default_value or "") if has_default else "" # Per spec: "It MUST NOT be possible to inject YAML structures by # environment variables." Newlines are the primary injection vector — diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py index 6131e3c1b08..c4fcdf8f886 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py @@ -82,7 +82,6 @@ def load_config_file( Raises: ConfigurationError: If file cannot be read, parsed, or validated. - EnvSubstitutionError: If required environment variable is missing. Examples: >>> config = load_config_file("otel-config.yaml") diff --git a/opentelemetry-configuration/tests/file/test_env_substitution.py b/opentelemetry-configuration/tests/file/test_env_substitution.py index 14c79f20fd5..92bbc711abf 100644 --- a/opentelemetry-configuration/tests/file/test_env_substitution.py +++ b/opentelemetry-configuration/tests/file/test_env_substitution.py @@ -7,10 +7,7 @@ import yaml -from opentelemetry.configuration.file import ( - EnvSubstitutionError, - substitute_env_vars, -) +from opentelemetry.configuration.file import substitute_env_vars class TestEnvSubstitution(unittest.TestCase): @@ -40,12 +37,17 @@ def test_substitution_with_default_override(self): result = substitute_env_vars("name: ${SERVICE_NAME:-default}") self.assertEqual(result, "name: actual") - def test_missing_variable_raises_error(self): - """Test ${VAR} raises error when variable missing.""" + def test_missing_variable_without_default_substitutes_empty(self): + """An unset ${VAR} without a default is replaced with an empty value. + + Per the declarative configuration spec, unset variables without + defaults are replaced with an empty value, which YAML then reads as + null. This matches the Java and Node.js implementations. + """ with patch.dict(os.environ, {}, clear=True): - with self.assertRaises(EnvSubstitutionError) as ctx: - substitute_env_vars("name: ${MISSING_VAR}") - self.assertIn("MISSING_VAR", str(ctx.exception)) + result = substitute_env_vars("name: ${MISSING_VAR}") + self.assertEqual(result, "name: ") + self.assertIsNone(yaml.safe_load(result)["name"]) def test_dollar_sign_escape(self): """Test $$ escapes to literal $.""" diff --git a/opentelemetry-configuration/tests/file/test_loader.py b/opentelemetry-configuration/tests/file/test_loader.py index c9dcbee8e67..ea9689008b5 100644 --- a/opentelemetry-configuration/tests/file/test_loader.py +++ b/opentelemetry-configuration/tests/file/test_loader.py @@ -137,19 +137,25 @@ def test_non_dict_root(self): finally: os.unlink(temp_path) - def test_missing_required_env_var(self): - """Test error when required env var is missing.""" + def test_unset_env_var_without_default_substitutes_empty(self): + """An unset env var without a default resolves to an empty value. + + Per the declarative configuration spec, an unset ``${VAR}`` reference + with no default is replaced with an empty value rather than raising, + so the file still loads. A default (``${ENV:-production}``) is still + applied when its variable is unset. + """ config_path = self.test_data_dir / "config_with_env_vars.yaml" with patch.dict(os.environ, {}, clear=True): - with self.assertRaises(ConfigurationError) as ctx: - load_config_file(str(config_path)) + config = load_config_file(str(config_path)) - # Should mention substitution or env var error - self.assertTrue( - "substitution" in str(ctx.exception).lower() - or "environment" in str(ctx.exception).lower() - ) + attributes = { + attribute.name: attribute.value + for attribute in config.resource.attributes + } + self.assertIsNone(attributes["service.name"]) + self.assertEqual(attributes["deployment.environment"], "production") def test_yml_extension(self): """Test .yml extension is supported.""" diff --git a/opentelemetry-configuration/tests/test_resource.py b/opentelemetry-configuration/tests/test_resource.py index 3a21872ce12..b822a37f8fa 100644 --- a/opentelemetry-configuration/tests/test_resource.py +++ b/opentelemetry-configuration/tests/test_resource.py @@ -224,6 +224,39 @@ def test_attribute_type_bool_array(self): resource = create_resource(config) self.assertEqual(list(resource.attributes["k"]), [True, False]) # type: ignore[arg-type] + def test_none_value_attribute_skipped_with_warning(self): + """An unset ${VAR} with no default yields a null value; it must be + skipped (not inserted as None or coerced) and a warning logged.""" + with self.assertLogs( + "opentelemetry.configuration._resource", level="WARNING" + ) as cm: + config = ResourceConfig( + attributes=[ + AttributeNameValue(name="empty", value=None), + AttributeNameValue(name="env", value="production"), + ] + ) + resource = create_resource(config) + self.assertNotIn("empty", resource.attributes) + self.assertEqual(resource.attributes["env"], "production") + self.assertTrue(any("empty" in msg for msg in cm.output)) + + def test_none_value_typed_attribute_skipped(self): + """A null value with a declared type must be skipped, not coerced + (int(None)/str(None) would raise or produce garbage).""" + with self.assertLogs( + "opentelemetry.configuration._resource", level="WARNING" + ): + config = ResourceConfig( + attributes=[ + AttributeNameValue( + name="k", value=None, type=AttributeType.int + ) + ] + ) + resource = create_resource(config) + self.assertNotIn("k", resource.attributes) + def test_attribute_type_bool_array_string_values(self): """bool_array must use _coerce_bool, not plain bool() — 'false' must be False.""" config = ResourceConfig(