fix: Resolve environment variable substitution for mixed quotes

dol · dol · commit 88917329720f · 2025-06-19T08:13:00.000+02:00
Fixes #7429 - Environment variable substitution no longer breaks parsing of YAML values containing both single and double quotes. Values that are not intended for substitution are now parsed correctly as literal strings.
diff --git a/sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/DeclarativeConfiguration.java b/sdk-extensions/incubator/src/main/java/io/opentelemetry/sdk/extension/incubator/fileconfig/DeclarativeConfiguration.java
@@ -24,21 +24,25 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.logging.Logger;
 import java.util.regex.MatchResult;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import javax.annotation.Nullable;
 import org.snakeyaml.engine.v2.api.Load;
 import org.snakeyaml.engine.v2.api.LoadSettings;
+import org.snakeyaml.engine.v2.api.YamlUnicodeReader;
 import org.snakeyaml.engine.v2.common.ScalarStyle;
-import org.snakeyaml.engine.v2.constructor.StandardConstructor;
-import org.snakeyaml.engine.v2.exceptions.ConstructorException;
-import org.snakeyaml.engine.v2.exceptions.YamlEngineException;
+import org.snakeyaml.engine.v2.composer.Composer;
 import org.snakeyaml.engine.v2.nodes.MappingNode;
 import org.snakeyaml.engine.v2.nodes.Node;
 import org.snakeyaml.engine.v2.nodes.NodeTuple;
 import org.snakeyaml.engine.v2.nodes.ScalarNode;
+import org.snakeyaml.engine.v2.nodes.Tag;
+import org.snakeyaml.engine.v2.parser.ParserImpl;
+import org.snakeyaml.engine.v2.resolver.ScalarResolver;
+import org.snakeyaml.engine.v2.scanner.StreamReader;
 import org.snakeyaml.engine.v2.schema.CoreSchema;
 
 /**
@@ -126,8 +130,9 @@ public static OpenTelemetrySdk create(
   /**
    * Parse the {@code configuration} YAML and return the {@link OpenTelemetryConfigurationModel}.
    *
-   * <p>Before parsing, environment variable substitution is performed as described in {@link
-   * EnvSubstitutionConstructor}.
+   * <p>During parsing, environment variable substitution is performed as defined in the
+   * <a href="https://opentelemetry.io/docs/specs/otel/configuration/data-model/#environment-variable-substitution">
+   * OpenTelemetry Configuration Data Model specification</a>.
    *
    * @throws DeclarativeConfigException if unable to parse
    */
@@ -149,7 +154,7 @@ static OpenTelemetryConfigurationModel parse(
   // Visible for testing
   static Object loadYaml(InputStream inputStream, Map<String, String> environmentVariables) {
     LoadSettings settings = LoadSettings.builder().setSchema(new CoreSchema()).build();
-    Load yaml = new Load(settings, new EnvSubstitutionConstructor(settings, environmentVariables));
+    Load yaml = new EnvLoad(settings, environmentVariables);
     return yaml.loadFromInputStream(inputStream);
   }
 
@@ -241,89 +246,102 @@ static <M, R> R createAndMaybeCleanup(Factory<M, R> factory, SpiHelper spiHelper
     }
   }
 
+  private static final class EnvLoad extends Load {
+
+    private final LoadSettings settings;
+    private final Map<String, String> environmentVariables;
+
+    public EnvLoad(LoadSettings settings, Map<String, String> environmentVariables) {
+      super(settings);
+      this.settings = settings;
+      this.environmentVariables = environmentVariables;
+    }
+
+    @Override
+    public Object loadFromInputStream(InputStream yamlStream) {
+      Objects.requireNonNull(yamlStream, "InputStream cannot be null");
+      return loadOne(new EnvComposer(settings,
+          new ParserImpl(settings, new StreamReader(settings, new YamlUnicodeReader(yamlStream))),
+          environmentVariables));
+    }
+  }
+
   /**
-   * {@link StandardConstructor} which substitutes environment variables.
+   * A YAML Composer that performs environment variable substitution according to the
+   * <a href="https://opentelemetry.io/docs/specs/otel/configuration/data-model/#environment-variable-substitution">
+   * OpenTelemetry Configuration Data Model specification</a>.
    *
-   * <p>Environment variables follow the syntax {@code ${VARIABLE}}, where {@code VARIABLE} is an
-   * environment variable matching the regular expression {@code [a-zA-Z_]+[a-zA-Z0-9_]*}.
+   * <p>This composer supports:
+   * <ul>
+   *   <li>Environment variable references: {@code ${ENV_VAR}} or {@code ${env:ENV_VAR}}
+   *   <li>Default values: {@code ${ENV_VAR:-default_value}}
+   *   <li>Escape sequences: {@code $$} is replaced with a single {@code $}
+   * </ul>
    *
-   * <p>Environment variable substitution only takes place on scalar values of maps. References to
-   * environment variables in keys or sets are ignored.
+   * <p>Environment variable substitution only applies to scalar values. Mapping keys are not
+   * candidates for substitution. Referenced environment variables that are undefined, null, or
+   * empty are replaced with empty values unless a default value is provided.
    *
-   * <p>If a referenced environment variable is not defined, it is replaced with {@code ""}.
+   * <p>The {@code $} character serves as an escape sequence where {@code $$} in the input is
+   * translated to a single {@code $} in the output. This prevents environment variable
+   * substitution for the escaped content.
    */
-  private static final class EnvSubstitutionConstructor extends StandardConstructor {
+  private static final class EnvComposer extends Composer {
 
-    // Load is not thread safe but this instance is always used on the same thread
     private final Load load;
     private final Map<String, String> environmentVariables;
+    private final ScalarResolver scalarResolver;
+
+    private static final String ESCAPE_SEQUENCE = "$$";
+    private static final int ESCAPE_SEQUENCE_LENGTH = ESCAPE_SEQUENCE.length();
+    private static final char ESCAPE_SEQUENCE_REPLACEMENT = '$';
 
-    private EnvSubstitutionConstructor(
-        LoadSettings loadSettings, Map<String, String> environmentVariables) {
-      super(loadSettings);
-      load = new Load(loadSettings);
+    public EnvComposer(LoadSettings settings, ParserImpl parser,
+        Map<String, String> environmentVariables) {
+      super(settings, parser);
+      this.load = new Load(settings);
       this.environmentVariables = environmentVariables;
+      this.scalarResolver = settings.getSchema().getScalarResolver();
     }
 
-    /**
-     * Implementation is same as {@link
-     * org.snakeyaml.engine.v2.constructor.BaseConstructor#constructMapping(MappingNode)} except we
-     * override the resolution of values with our custom {@link #constructValueObject(Node)}, which
-     * performs environment variable substitution.
-     */
     @Override
-    @SuppressWarnings({"ReturnValueIgnored", "CatchingUnchecked"})
-    protected Map<Object, Object> constructMapping(MappingNode node) {
-      Map<Object, Object> mapping = settings.getDefaultMap().apply(node.getValue().size());
-      List<NodeTuple> nodeValue = node.getValue();
-      for (NodeTuple tuple : nodeValue) {
-        Node keyNode = tuple.getKeyNode();
-        Object key = constructObject(keyNode);
-        if (key != null) {
-          try {
-            key.hashCode(); // check circular dependencies
-          } catch (Exception e) {
-            throw new ConstructorException(
-                "while constructing a mapping",
-                node.getStartMark(),
-                "found unacceptable key " + key,
-                tuple.getKeyNode().getStartMark(),
-                e);
-          }
-        }
-        Node valueNode = tuple.getValueNode();
-        Object value = constructValueObject(valueNode);
-        if (keyNode.isRecursive()) {
-          if (settings.getAllowRecursiveKeys()) {
-            postponeMapFilling(mapping, key, value);
-          } else {
-            throw new YamlEngineException(
-                "Recursive key for mapping is detected but it is not configured to be allowed.");
-          }
-        } else {
-          mapping.put(key, value);
-        }
+    protected void composeMappingChildren(List<NodeTuple> children, MappingNode node) {
+      Node itemKey = composeKeyNode(node);
+      Node itemValue = composeValueNode(node);
+      // Only apply environment variable substitution to ScalarNodes
+      if (!(itemValue instanceof ScalarNode)) {
+        children.add(new NodeTuple(itemKey, itemValue));
+        return;
       }
 
-      return mapping;
-    }
+      String envSubstitution = envSubstitution(((ScalarNode) itemValue).getValue());
 
-    private static final String ESCAPE_SEQUENCE = "$$";
-    private static final int ESCAPE_SEQUENCE_LENGTH = ESCAPE_SEQUENCE.length();
-    private static final char ESCAPE_SEQUENCE_REPLACEMENT = '$';
-
-    private Object constructValueObject(Node node) {
-      Object value = constructObject(node);
-      if (!(node instanceof ScalarNode)) {
-        return value;
+      // If the environment variable substitution does not change the value, do not modify the node
+      if (envSubstitution.equals(((ScalarNode) itemValue).getValue())) {
+        children.add(new NodeTuple(itemKey, itemValue));
+        return;
       }
-      if (!(value instanceof String)) {
-        return value;
+
+      Object envSubstitutionObj = load.loadFromString(envSubstitution);
+      Tag tag = itemValue.getTag();
+      ScalarStyle scalarStyle = ((ScalarNode) itemValue).getScalarStyle();
+
+      Tag resolvedTag = envSubstitutionObj == null ? Tag.NULL
+          : scalarResolver.resolve(envSubstitutionObj.toString(), true);
+
+      if ((!scalarStyle.equals(ScalarStyle.SINGLE_QUOTED) && !scalarStyle.equals(
+          ScalarStyle.DOUBLE_QUOTED)) || itemValue.getTag().equals(resolvedTag)) {
+        tag = resolvedTag;
       }
 
-      String val = (String) value;
-      ScalarStyle scalarStyle = ((ScalarNode) node).getScalarStyle();
+      boolean resolved = true;
+      itemValue = new ScalarNode(tag, resolved, envSubstitution,
+          scalarStyle, itemValue.getStartMark(),
+          itemValue.getEndMark());
+      children.add(new NodeTuple(itemKey, itemValue));
+    }
 
+    private String envSubstitution(String val) {
       // Iterate through val left to right, search for escape sequence "$$"
       // For the substring of val between the last escape sequence and the next found, perform
       // environment variable substitution
@@ -346,13 +364,7 @@ private Object constructValueObject(Node node) {
         }
       }
 
-      // If the value was double quoted, retain the double quotes so we don't change a value
-      // intended to be a string to a different type after environment variable substitution
-      if (scalarStyle == ScalarStyle.DOUBLE_QUOTED) {
-        newVal.insert(0, "\"");
-        newVal.append("\"");
-      }
-      return load.loadFromString(newVal.toString());
+      return newVal.toString();
     }
 
     private StringBuilder envVarSubstitution(
diff --git a/sdk-extensions/incubator/src/test/java/io/opentelemetry/sdk/extension/incubator/fileconfig/DeclarativeConfigurationParseTest.java b/sdk-extensions/incubator/src/test/java/io/opentelemetry/sdk/extension/incubator/fileconfig/DeclarativeConfigurationParseTest.java
@@ -92,6 +92,7 @@
 import java.util.HashMap;
 import java.util.Map;
 import javax.annotation.Nullable;
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
@@ -102,9 +103,9 @@ class DeclarativeConfigurationParseTest {
   @Test
   void parse_BadInputStream() {
     assertThatThrownBy(
-            () ->
-                DeclarativeConfiguration.parseAndCreate(
-                    new ByteArrayInputStream("foo".getBytes(StandardCharsets.UTF_8))))
+        () ->
+            DeclarativeConfiguration.parseAndCreate(
+                new ByteArrayInputStream("foo".getBytes(StandardCharsets.UTF_8))))
         .isInstanceOf(DeclarativeConfigException.class)
         .hasMessage("Unable to parse configuration input stream");
   }
@@ -855,6 +856,26 @@ void parse_nullBoxedPrimitivesParsedToNull() {
                                 .withTraceIdRatioBased(new TraceIdRatioBasedSamplerModel()))));
   }
 
+  @Test
+  void parse_quotedInput() {
+    String yaml = "resource:\n"
+        + "  attributes:\n"
+        + "    - name: single_quote\n"
+        + "      value: '\"single\"'\n"
+        + "    - name: double_quote\n"
+        + "      value: \"\\\"double\\\"\"";
+
+    OpenTelemetryConfigurationModel model =
+        DeclarativeConfiguration.parse(
+            new ByteArrayInputStream(yaml.getBytes(StandardCharsets.UTF_8)));
+
+    Assertions.assertNotNull(model.getResource());
+    assertThat(model.getResource().getAttributes())
+        .containsExactly(
+            new AttributeNameValueModel().withName("single_quote").withValue("\"single\""),
+            new AttributeNameValueModel().withName("double_quote").withValue("\"double\""));
+  }
+
   @ParameterizedTest
   @MethodSource("coreSchemaValuesArgs")
   void coreSchemaValues(String rawYaml, Object expectedYamlResult) {
@@ -928,7 +949,7 @@ private static java.util.stream.Stream<Arguments> envVarSubstitutionArgs() {
         // Undefined / empty environment variable
         Arguments.of("key1: ${EMPTY_STR}\n", mapOf(entry("key1", null))),
         Arguments.of("key1: ${STR_3}\n", mapOf(entry("key1", null))),
-        Arguments.of("key1: ${STR_1} ${STR_3}\n", mapOf(entry("key1", "value1"))),
+        Arguments.of("key1: ${STR_1} ${STR_3}\n", mapOf(entry("key1", "value1 "))),
         // Environment variable keys must match pattern: [a-zA-Z_]+[a-zA-Z0-9_]*
         Arguments.of("key1: ${VAR&}\n", mapOf(entry("key1", "${VAR&}"))),
         // Environment variable substitution only takes place in scalar values of maps
@@ -940,6 +961,7 @@ private static java.util.stream.Stream<Arguments> envVarSubstitutionArgs() {
             "key1:\n - ${STR_1}\n", mapOf(entry("key1", Collections.singletonList("${STR_1}")))),
         // Quoted environment variables
         Arguments.of("key1: \"${HEX}\"\n", mapOf(entry("key1", "0xdeadbeef"))),
+        Arguments.of("key1: \"${HEX} ${HEX}\"\n", mapOf(entry("key1", "0xdeadbeef 0xdeadbeef"))),
         Arguments.of("key1: \"${STR_1}\"\n", mapOf(entry("key1", "value1"))),
         Arguments.of("key1: \"${EMPTY_STR}\"\n", mapOf(entry("key1", ""))),
         Arguments.of("key1: \"${BOOL}\"\n", mapOf(entry("key1", "true"))),
