Allow yaml values for dynamic node settings (#85186)

Environment variables are allowed as substitutions within
elasticsearch.yml. Additionally, command line settings are added into
the parsed settings. However, both of these are raw strings applied
after the node yml file has been parsed.

This commit moves environment substitution to occur before parsing
elasticsearch.yml, and override processing to happen with a separate
yaml parser so that both can allow yaml parsing. Note that environment
substitution is not the only type of substitution (there is also setting
substitution, which needs parsed setting keys), so the existing
replacement mechanism is not touched here except to remove environment
handling.

closes #65577

docs/changelog/85186.yaml

@@ -0,0 +1,6 @@
+pr: 85186
+summary: Allow yaml values for dynamic node settings
+area: Infra/Core
+type: enhancement
+issues:
+ - 65577

server/src/main/java/org/elasticsearch/common/settings/Settings.java

@@ -440,7 +440,8 @@ public List<String> getAsList(String key, List<String> defaultValue, Boolean com
                 final List<String> valuesAsList = (List<String>) valueFromPrefix;
                 return valuesAsList;
             } else if (commaDelimited) {
-                String[] strings = Strings.splitStringByCommaToArray(get(key));
+                String value = get(key);
+                String[] strings = Strings.splitStringByCommaToArray(value);
                 if (strings.length > 0) {
                     for (String string : strings) {
                         result.add(string.trim());
@@ -1210,19 +1211,10 @@ public Builder putProperties(final Map<String, String> esSettings, final Functio
          * another setting already set on this builder.
          */
         public Builder replacePropertyPlaceholders() {
-            return replacePropertyPlaceholders(System::getenv);
-        }
-
-        // visible for testing
-        Builder replacePropertyPlaceholders(Function<String, String> getenv) {
             PropertyPlaceholder propertyPlaceholder = new PropertyPlaceholder("${", "}", false);
             PropertyPlaceholder.PlaceholderResolver placeholderResolver = new PropertyPlaceholder.PlaceholderResolver() {
                 @Override
                 public String resolvePlaceholder(String placeholderName) {
-                    final String value = getenv.apply(placeholderName);
-                    if (value != null) {
-                        return value;
-                    }
                     return Settings.toString(map.get(placeholderName));
                 }
 

server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java

@@ -15,26 +15,20 @@
 import org.elasticsearch.core.SuppressForbidden;
 import org.elasticsearch.env.Environment;
 
+import java.io.BufferedReader;
+import java.io.ByteArrayInputStream;
 import java.io.IOException;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
 public class InternalSettingsPreparer {
 
-    // TODO: refactor this method out, it used to exist for the transport client
-    public static Settings prepareSettings(Settings input) {
-        Settings.Builder output = Settings.builder();
-        initializeSettings(output, input, Collections.emptyMap());
-        finalizeSettings(output, () -> null);
-        return output.build();
-    }
-
     /**
      * Prepares the settings by gathering all elasticsearch system properties, optionally loading the configuration settings.
      *
@@ -62,16 +56,19 @@ public static Environment prepareEnvironment(
 
         Settings.Builder output = Settings.builder(); // start with a fresh output
         Path path = configFile.resolve("elasticsearch.yml");
+
         if (Files.exists(path)) {
             try {
-                output.loadFromPath(path);
+                loadConfigWithSubstitutions(output, path, System::getenv);
             } catch (IOException e) {
                 throw new SettingsException("Failed to load settings from " + path.toString(), e);
             }
         }
 
+        loadOverrides(output, properties);
+
         // re-initialize settings now that the config file has been loaded
-        initializeSettings(output, input, properties);
+        initializeSettings(output, input);
         finalizeSettings(output, defaultNodeName);
 
         return new Environment(output.build(), configFile);
@@ -105,14 +102,69 @@ private static Path resolveConfigDir(String esHome) {
      *
      * @param output the settings builder to apply the input and default settings to
      * @param input the input settings
-     * @param esSettings a map from which to apply settings
      */
-    static void initializeSettings(final Settings.Builder output, final Settings input, final Map<String, String> esSettings) {
+    static void initializeSettings(final Settings.Builder output, final Settings input) {
         output.put(input);
-        output.putProperties(esSettings, Function.identity());
         output.replacePropertyPlaceholders();
     }
 
+    static void loadConfigWithSubstitutions(Settings.Builder output, Path configFile, Function<String, String> substitutions)
+        throws IOException {
+        long existingSize = Files.size(configFile);
+        StringBuilder builder = new StringBuilder((int) existingSize);
+        try (BufferedReader reader = Files.newBufferedReader(configFile, StandardCharsets.UTF_8)) {
+            String line;
+            while ((line = reader.readLine()) != null) {
+                int dollarNdx;
+                int nextNdx = 0;
+                while ((dollarNdx = line.indexOf("${", nextNdx)) != -1) {
+                    int closeNdx = line.indexOf('}', dollarNdx + 2);
+                    if (closeNdx == -1) {
+                        // No close substitution was found. Break to leniently copy the rest of the line as is.
+                        break;
+                    }
+                    // copy up to the dollar
+                    if (dollarNdx > nextNdx) {
+                        builder.append(line, nextNdx, dollarNdx);
+                    }
+                    nextNdx = closeNdx + 1;
+
+                    String substKey = line.substring(dollarNdx + 2, closeNdx);
+                    String substValue = substitutions.apply(substKey);
+                    if (substValue != null) {
+                        builder.append(substValue);
+                    } else {
+                        // the substitution name doesn't exist, defer to setting based substitution after yaml parsing
+                        builder.append(line, dollarNdx, nextNdx);
+                    }
+                }
+                if (nextNdx < line.length()) {
+                    builder.append(line, nextNdx, line.length());
+                }
+                builder.append(System.lineSeparator());
+            }
+        }
+        var is = new ByteArrayInputStream(builder.toString().getBytes(StandardCharsets.UTF_8));
+        output.loadFromStream(configFile.getFileName().toString(), is, false);
+    }
+
+    static void loadOverrides(Settings.Builder output, Map<String, String> overrides) {
+        StringBuilder builder = new StringBuilder();
+        for (var entry : overrides.entrySet()) {
+            builder.append(entry.getKey());
+            builder.append(": ");
+            builder.append(entry.getValue());
+            builder.append(System.lineSeparator());
+        }
+        var is = new ByteArrayInputStream(builder.toString().getBytes(StandardCharsets.UTF_8));
+        // fake the resource name so it loads yaml
+        try {
+            output.loadFromStream("overrides.yml", is, false);
+        } catch (IOException e) {
+            throw new SettingsException("Malformed setting override value", e);
+        }
+    }
+
     /**
      * Finish preparing settings by replacing forced settings and any defaults that need to be added.
      */

server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java

@@ -59,16 +59,6 @@ public void testReplacePropertiesPlaceholderSystemProperty() {
         assertThat(settings.get("setting1"), equalTo(value));
     }
 
-    public void testReplacePropertiesPlaceholderSystemPropertyList() {
-        final String hostname = randomAlphaOfLength(16);
-        final String hostip = randomAlphaOfLength(16);
-        final Settings settings = Settings.builder()
-            .putList("setting1", "${HOSTNAME}", "${HOSTIP}")
-            .replacePropertyPlaceholders(name -> name.equals("HOSTNAME") ? hostname : name.equals("HOSTIP") ? hostip : null)
-            .build();
-        assertThat(settings.getAsList("setting1"), contains(hostname, hostip));
-    }
-
     public void testReplacePropertiesPlaceholderSystemVariablesHaveNoEffect() {
         final String value = System.getProperty("java.home");
         assertNotNull(value);
@@ -79,15 +69,6 @@ public void testReplacePropertiesPlaceholderSystemVariablesHaveNoEffect() {
         assertThat(e, hasToString(containsString("Could not resolve placeholder 'java.home'")));
     }
 
-    public void testReplacePropertiesPlaceholderByEnvironmentVariables() {
-        final String hostname = randomAlphaOfLength(16);
-        final Settings implicitEnvSettings = Settings.builder()
-            .put("setting1", "${HOSTNAME}")
-            .replacePropertyPlaceholders(name -> "HOSTNAME".equals(name) ? hostname : null)
-            .build();
-        assertThat(implicitEnvSettings.get("setting1"), equalTo(hostname));
-    }
-
     public void testGetAsSettings() {
         Settings settings = Settings.builder()
             .put("bar", "hello world")

server/src/test/java/org/elasticsearch/node/InternalSettingsPreparerTests.java

@@ -29,6 +29,7 @@
 import java.util.function.Supplier;
 
 import static java.util.Collections.emptyMap;
+import static org.hamcrest.Matchers.equalTo;
 
 public class InternalSettingsPreparerTests extends ESTestCase {
     private static final Supplier<String> DEFAULT_NODE_NAME_SHOULDNT_BE_CALLED = () -> { throw new AssertionError("shouldn't be called"); };
@@ -134,4 +135,87 @@ public void testDefaultPropertiesDoNothing() throws Exception {
         assertNull(env.settings().get("setting"));
     }
 
+    private Path copyConfig(String resourceName) throws IOException {
+        InputStream yaml = getClass().getResourceAsStream(resourceName);
+        Path configDir = homeDir.resolve("config");
+        Files.createDirectory(configDir);
+        Path configFile = configDir.resolve("elasticsearch.yaml");
+        Files.copy(yaml, configFile);
+        return configFile;
+    }
+
+    private Settings loadConfigWithSubstitutions(Path configFile, Map<String, String> env) throws IOException {
+        Settings.Builder output = Settings.builder();
+        InternalSettingsPreparer.loadConfigWithSubstitutions(output, configFile, env::get);
+        return output.build();
+    }
+
+    public void testSubstitutionEntireLine() throws Exception {
+        Path config = copyConfig("subst-entire-line.yml");
+        Settings settings = loadConfigWithSubstitutions(config, Map.of("mysubst", "foo: bar"));
+        assertThat(settings.get("foo"), equalTo("bar"));
+    }
+
+    public void testSubstitutionFirstLine() throws Exception {
+        Path config = copyConfig("subst-first-line.yml");
+        Settings settings = loadConfigWithSubstitutions(config, Map.of("mysubst", "v1"));
+        assertThat(settings.get("foo"), equalTo("v1"));
+        assertThat(settings.get("bar"), equalTo("v2"));
+        assertThat(settings.get("baz"), equalTo("v3"));
+    }
+
+    public void testSubstitutionLastLine() throws Exception {
+        Path config = copyConfig("subst-last-line.yml");
+        Settings settings = loadConfigWithSubstitutions(config, Map.of("mysubst", "kazaam"));
+        assertThat(settings.get("foo.bar.baz"), equalTo("kazaam"));
+    }
+
+    public void testSubstitutionMultiple() throws Exception {
+        Path config = copyConfig("subst-multiple.yml");
+        Settings settings = loadConfigWithSubstitutions(config, Map.of("s1", "substituted", "s2", "line"));
+        assertThat(settings.get("foo"), equalTo("substituted line value"));
+    }
+
+    public void testSubstitutionMissingLenient() throws Exception {
+        Path config = copyConfig("subst-missing.yml");
+        Settings settings = loadConfigWithSubstitutions(config, Map.of());
+        assertThat(settings.get("foo"), equalTo("${dne}"));
+    }
+
+    public void testSubstitutionBrokenLenient() throws Exception {
+        Path config = copyConfig("subst-broken.yml");
+        Settings settings = loadConfigWithSubstitutions(config, Map.of("goodsubst", "replaced"));
+        assertThat(settings.get("foo"), equalTo("${no closing brace"));
+        assertThat(settings.get("bar"), equalTo("replaced"));
+    }
+
+    public void testOverridesOverride() throws Exception {
+        Settings.Builder output = Settings.builder().put("foo", "bar");
+        InternalSettingsPreparer.loadOverrides(output, Map.of("foo", "baz"));
+        Settings settings = output.build();
+        assertThat(settings.get("foo"), equalTo("baz"));
+    }
+
+    public void testOverridesEmpty() throws Exception {
+        Settings.Builder output = Settings.builder().put("foo", "bar");
+        InternalSettingsPreparer.loadOverrides(output, Map.of());
+        Settings settings = output.build();
+        assertThat(settings.get("foo"), equalTo("bar"));
+    }
+
+    public void testOverridesNew() throws Exception {
+        Settings.Builder output = Settings.builder().put("foo", "bar");
+        InternalSettingsPreparer.loadOverrides(output, Map.of("baz", "wat"));
+        Settings settings = output.build();
+        assertThat(settings.get("foo"), equalTo("bar"));
+        assertThat(settings.get("baz"), equalTo("wat"));
+    }
+
+    public void testOverridesMultiple() throws Exception {
+        Settings.Builder output = Settings.builder().put("foo1", "bar").put("foo2", "baz");
+        InternalSettingsPreparer.loadOverrides(output, Map.of("foo1", "wat", "foo2", "yas"));
+        Settings settings = output.build();
+        assertThat(settings.get("foo1"), equalTo("wat"));
+        assertThat(settings.get("foo2"), equalTo("yas"));
+    }
 }

server/src/test/resources/org/elasticsearch/node/subst-broken.yml

@@ -0,0 +1,2 @@
+foo: ${no closing brace
+bar: ${goodsubst}

server/src/test/resources/org/elasticsearch/node/subst-entire-line.yml

@@ -0,0 +1 @@
+${mysubst}

server/src/test/resources/org/elasticsearch/node/subst-first-line.yml

@@ -0,0 +1,3 @@
+foo: ${mysubst}
+bar: v2
+baz: v3

server/src/test/resources/org/elasticsearch/node/subst-last-line.yml

@@ -0,0 +1,3 @@
+foo:
+  bar:
+    baz: ${mysubst}

server/src/test/resources/org/elasticsearch/node/subst-missing.yml

@@ -0,0 +1 @@
+foo: ${dne}

server/src/test/resources/org/elasticsearch/node/subst-multiple.yml

@@ -0,0 +1 @@
+foo: ${s1} ${s2} value