yaml config env var spec conformance (#1486)

* add support for env: prefix in yaml config
* align test to example in spec
* do not recursively replace env vars
  - fix env substitution normalization so that it doesn't apply 2 replacement normalizers for string nodes

Author: brettmc

src/Config/SDK/Configuration/Internal/EnvSubstitutionNormalization.php

@@ -50,8 +50,7 @@ private function doApply(NodeDefinition $node): void
                 default => FILTER_DEFAULT,
             };
             $node->beforeNormalization()->ifString()->then(fn (string $v) => $this->replaceEnvVariables($v, $filter))->end();
-        }
-        if ($node instanceof VariableNodeDefinition) {
+        } elseif ($node instanceof VariableNodeDefinition) {
             $node->beforeNormalization()->always($this->replaceEnvVariablesRecursive(...))->end();
         }
 
@@ -65,7 +64,7 @@ private function doApply(NodeDefinition $node): void
     private function replaceEnvVariables(string $value, int $filter = FILTER_DEFAULT): mixed
     {
         $replaced = preg_replace_callback(
-            '/\$\{(?<ENV_NAME>[a-zA-Z_]\w*)(?::-(?<DEFAULT_VALUE>[^\n]*))?}/',
+            '/\$\{(?:env:)?(?<ENV_NAME>[a-zA-Z_]\w*)(?::-(?<DEFAULT_VALUE>[^\n]*))?}/',
             fn (array $matches): string => $this->envReader->read($matches['ENV_NAME']) ?? $matches['DEFAULT_VALUE'] ?? '',
             $value,
             -1,

tests/Unit/Config/SDK/Configuration/ConfigurationFactoryTest.php

@@ -64,6 +64,7 @@ public function getConfig(ComponentProviderRegistry $registry, NodeBuilder $buil
                     $node
                         ->children()
                             ->scalarNode('string_key')->end()
+                            ->scalarNode('env_string_key')->end()
                             ->scalarNode('other_string_key')->end()
                             ->scalarNode('another_string_key')->end()
                             ->scalarNode('string_key_with_quoted_hex_value')->end()
@@ -76,6 +77,7 @@ public function getConfig(ComponentProviderRegistry $registry, NodeBuilder $buil
                             ->scalarNode('string_key_with_default')->end()
                             ->variableNode('undefined_key')->end()
                             ->variableNode('${STRING_VALUE}')->end()
+                            ->scalarNode('recursive_key')->end()
                         ->end()
                     ;
 
@@ -90,6 +92,8 @@ public function getConfig(ComponentProviderRegistry $registry, NodeBuilder $buil
                     'FLOAT_VALUE' => '1.1',
                     'HEX_VALUE' => '0xdeadbeef',
                     'INVALID_MAP_VALUE' => "value\nkey:value",
+                    'DO_NOT_REPLACE_ME' => 'Never use this value', // An unused environment variable
+                    'REPLACE_ME' => '${DO_NOT_REPLACE_ME}', // A valid replacement text, used verbatim, not replaced with "Never use this value"
                 ]),
             ]),
         );
@@ -98,6 +102,7 @@ public function getConfig(ComponentProviderRegistry $registry, NodeBuilder $buil
         $parsed = $factory->process([
             Yaml::parse(<<<'YAML'
                 string_key: ${STRING_VALUE}                           # Valid reference to STRING_VALUE
+                env_string_key: ${env:STRING_VALUE}                   # Valid reference to STRING_VALUE
                 other_string_key: "${STRING_VALUE}"                   # Valid reference to STRING_VALUE inside double quotes
                 another_string_key: "${BOOl_VALUE}"                   # Valid reference to BOOl_VALUE inside double quotes
                 string_key_with_quoted_hex_value: "${HEX_VALUE}"      # Valid reference to HEX_VALUE inside double quotes
@@ -110,12 +115,14 @@ public function getConfig(ComponentProviderRegistry $registry, NodeBuilder $buil
                 string_key_with_default: ${UNDEFINED_KEY:-fallback}   # UNDEFINED_KEY is not defined but a default value is included
                 undefined_key: ${UNDEFINED_KEY}                       # Invalid reference, UNDEFINED_KEY is not defined and is replaced with ""
                 ${STRING_VALUE}: value                                # Invalid reference, substitution is not valid in mapping keys and reference is ignored
+                recursive_key: ${REPLACE_ME}                          # Valid reference to REPLACE_ME
                 YAML),
         ]);
 
         $this->assertSame(
             Yaml::parse(<<<'YAML'
                 string_key: value                              # Interpreted as type string, tag URI tag:yaml.org,2002:str
+                env_string_key: value                          # Interpreted as type string, tag URI tag:yaml.org,2002:str
                 other_string_key: "value"                      # Interpreted as type string, tag URI tag:yaml.org,2002:str
                 another_string_key: "true"                     # Interpreted as type string, tag URI tag:yaml.org,2002:str
                 string_key_with_quoted_hex_value: "0xdeadbeef" # Interpreted as type string, tag URI tag:yaml.org,2002:str
@@ -129,6 +136,7 @@ public function getConfig(ComponentProviderRegistry $registry, NodeBuilder $buil
                 # undefined_key removed as null is treated as unset
                 undefined_key:                                 # Interpreted as type null, tag URI tag:yaml.org,2002:null
                 ${STRING_VALUE}: value                         # Interpreted as type string, tag URI tag:yaml.org,2002:str
+                recursive_key: ${DO_NOT_REPLACE_ME}            # Interpreted as type string, tag URI tag:yaml.org,2002:str
                 YAML),
             self::getPropertiesFromPlugin($parsed),
         );
@@ -152,6 +160,24 @@ public function test_env_substitution_string(): void
         $this->assertSame('example-service', self::getPropertiesFromPlugin($parsed)['resource']['attributes']['service.name']);
     }
 
+    #[BackupGlobals(true)]
+    #[CoversNothing]
+    public function test_env_substitution_with_env_prefix(): void
+    {
+        $_SERVER['OTEL_SERVICE_NAME'] = 'example-service';
+        $parsed = self::factory()->process([[
+            'file_format' => '0.1',
+            'resource' => [
+                'attributes' => [
+                    'service.name' => '${env:OTEL_SERVICE_NAME}',
+                ],
+            ],
+        ]]);
+
+        $this->assertInstanceOf(ComponentPlugin::class, $parsed);
+        $this->assertSame('example-service', self::getPropertiesFromPlugin($parsed)['resource']['attributes']['service.name']);
+    }
+
     #[BackupGlobals(true)]
     public function test_env_substitution_non_string(): void
     {
@@ -167,6 +193,29 @@ public function test_env_substitution_non_string(): void
         $this->assertSame(2048, self::getPropertiesFromPlugin($parsed)['attribute_limits']['attribute_value_length_limit']);
     }
 
+    /**
+     * It MUST NOT be possible to inject environment variable by environment variables.
+     * For example, see references to DO_NOT_REPLACE_ME environment variable
+     */
+    #[BackupGlobals(true)]
+    #[CoversNothing]
+    public function test_env_substitution_recursive_does_not_inject_environment_variables(): void
+    {
+        $_SERVER['DO_NOT_REPLACE_ME'] = 'Never use this value';
+        $_SERVER['REPLACE_ME'] = '${DO_NOT_REPLACE_ME}';
+        $parsed = self::factory()->process([[
+            'file_format' => '0.1',
+            'resource' => [
+                'attributes' => [
+                    'service.name' => '${REPLACE_ME}',
+                ],
+            ],
+        ]]);
+
+        $this->assertInstanceOf(ComponentPlugin::class, $parsed);
+        $this->assertSame('${DO_NOT_REPLACE_ME}', self::getPropertiesFromPlugin($parsed)['resource']['attributes']['service.name']);
+    }
+
     /**
      * If a property has a default value defined (i.e. is _not_ required) and is
      * missing or present but null, Create MUST ensure the SDK component is configured