Allow yaml values for dynamic node settings

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.
      *
@@ -64,7 +58,8 @@ public static Environment prepareEnvironment(
         Path path = configFile.resolve("elasticsearch.yml");
         if (Files.exists(path)) {
             try {
-                output.loadFromPath(path);
+                loadConfigWithSubstitutions(output, path, System::getenv);
+                loadOverrides(output, properties);
             } catch (IOException e) {
                 throw new SettingsException("Failed to load settings from " + path.toString(), e);
             }
@@ -113,6 +108,59 @@ static void initializeSettings(final Settings.Builder output, final Settings inp
         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) throws IOException {
+        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
+        output.loadFromStream("overrides.yml", is, false);
+    }
+
     /**
      * 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