From b3fc42d49339c7be093fb090c45548542a044477 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 18 Mar 2022 10:07:47 -0700 Subject: [PATCH 1/5] Allow yaml values for dynamic node settings 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 --- .../common/settings/Settings.java | 12 +-- .../node/InternalSettingsPreparer.java | 68 +++++++++++++-- .../common/settings/SettingsTests.java | 19 ----- .../node/InternalSettingsPreparerTests.java | 84 +++++++++++++++++++ .../org/elasticsearch/node/subst-broken.yml | 2 + .../elasticsearch/node/subst-entire-line.yml | 1 + .../elasticsearch/node/subst-first-line.yml | 3 + .../elasticsearch/node/subst-last-line.yml | 3 + .../org/elasticsearch/node/subst-missing.yml | 1 + .../org/elasticsearch/node/subst-multiple.yml | 1 + 10 files changed, 156 insertions(+), 38 deletions(-) create mode 100644 server/src/test/resources/org/elasticsearch/node/subst-broken.yml create mode 100644 server/src/test/resources/org/elasticsearch/node/subst-entire-line.yml create mode 100644 server/src/test/resources/org/elasticsearch/node/subst-first-line.yml create mode 100644 server/src/test/resources/org/elasticsearch/node/subst-last-line.yml create mode 100644 server/src/test/resources/org/elasticsearch/node/subst-missing.yml create mode 100644 server/src/test/resources/org/elasticsearch/node/subst-multiple.yml diff --git a/server/src/main/java/org/elasticsearch/common/settings/Settings.java b/server/src/main/java/org/elasticsearch/common/settings/Settings.java index aa847867d2f6f..b622f81f8fe64 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -440,7 +440,8 @@ public List getAsList(String key, List defaultValue, Boolean com final List valuesAsList = (List) 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 esSettings, final Functio * another setting already set on this builder. */ public Builder replacePropertyPlaceholders() { - return replacePropertyPlaceholders(System::getenv); - } - - // visible for testing - Builder replacePropertyPlaceholders(Function 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)); } diff --git a/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java b/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java index 6534f33168115..c110997c82993 100644 --- a/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java +++ b/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java @@ -15,7 +15,11 @@ import org.elasticsearch.core.SuppressForbidden; import org.elasticsearch.env.Environment; +import java.io.BufferedReader; +import java.io.ByteArrayInputStream; import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; @@ -27,14 +31,6 @@ 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 +60,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 +110,59 @@ static void initializeSettings(final Settings.Builder output, final Settings inp output.replacePropertyPlaceholders(); } + + static void loadConfigWithSubstitutions(Settings.Builder output, Path configFile, Function 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 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. */ diff --git a/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java b/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java index b78b455a91aad..64b7153d2a4ff 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/SettingsTests.java +++ b/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") diff --git a/server/src/test/java/org/elasticsearch/node/InternalSettingsPreparerTests.java b/server/src/test/java/org/elasticsearch/node/InternalSettingsPreparerTests.java index 3a7a0510e4fef..a5772f5fb7dee 100644 --- a/server/src/test/java/org/elasticsearch/node/InternalSettingsPreparerTests.java +++ b/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 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 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")); + } } diff --git a/server/src/test/resources/org/elasticsearch/node/subst-broken.yml b/server/src/test/resources/org/elasticsearch/node/subst-broken.yml new file mode 100644 index 0000000000000..31fc3e08eb42a --- /dev/null +++ b/server/src/test/resources/org/elasticsearch/node/subst-broken.yml @@ -0,0 +1,2 @@ +foo: ${no closing brace +bar: ${goodsubst} diff --git a/server/src/test/resources/org/elasticsearch/node/subst-entire-line.yml b/server/src/test/resources/org/elasticsearch/node/subst-entire-line.yml new file mode 100644 index 0000000000000..2d1f28eea6c99 --- /dev/null +++ b/server/src/test/resources/org/elasticsearch/node/subst-entire-line.yml @@ -0,0 +1 @@ +${mysubst} diff --git a/server/src/test/resources/org/elasticsearch/node/subst-first-line.yml b/server/src/test/resources/org/elasticsearch/node/subst-first-line.yml new file mode 100644 index 0000000000000..4f2ab1c7d029b --- /dev/null +++ b/server/src/test/resources/org/elasticsearch/node/subst-first-line.yml @@ -0,0 +1,3 @@ +foo: ${mysubst} +bar: v2 +baz: v3 diff --git a/server/src/test/resources/org/elasticsearch/node/subst-last-line.yml b/server/src/test/resources/org/elasticsearch/node/subst-last-line.yml new file mode 100644 index 0000000000000..bb07d9e9425cd --- /dev/null +++ b/server/src/test/resources/org/elasticsearch/node/subst-last-line.yml @@ -0,0 +1,3 @@ +foo: + bar: + baz: ${mysubst} diff --git a/server/src/test/resources/org/elasticsearch/node/subst-missing.yml b/server/src/test/resources/org/elasticsearch/node/subst-missing.yml new file mode 100644 index 0000000000000..913f7853147ce --- /dev/null +++ b/server/src/test/resources/org/elasticsearch/node/subst-missing.yml @@ -0,0 +1 @@ +foo: ${dne} diff --git a/server/src/test/resources/org/elasticsearch/node/subst-multiple.yml b/server/src/test/resources/org/elasticsearch/node/subst-multiple.yml new file mode 100644 index 0000000000000..b5e020247fa63 --- /dev/null +++ b/server/src/test/resources/org/elasticsearch/node/subst-multiple.yml @@ -0,0 +1 @@ +foo: ${s1} ${s2} value From c94013304442e39598a48b3bc6b76d4ae80ded27 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Mon, 21 Mar 2022 12:53:37 -0700 Subject: [PATCH 2/5] Update docs/changelog/85186.yaml --- docs/changelog/85186.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changelog/85186.yaml diff --git a/docs/changelog/85186.yaml b/docs/changelog/85186.yaml new file mode 100644 index 0000000000000..33cfe1fc8c290 --- /dev/null +++ b/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 From ae40288190b800afebdfab31f337699e6154a632 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Mon, 21 Mar 2022 13:00:46 -0700 Subject: [PATCH 3/5] spotless --- .../org/elasticsearch/node/InternalSettingsPreparer.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java b/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java index c110997c82993..8fc9169731f97 100644 --- a/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java +++ b/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java @@ -18,12 +18,10 @@ import java.io.BufferedReader; import java.io.ByteArrayInputStream; import java.io.IOException; -import java.io.InputStream; 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; @@ -110,8 +108,8 @@ static void initializeSettings(final Settings.Builder output, final Settings inp output.replacePropertyPlaceholders(); } - - static void loadConfigWithSubstitutions(Settings.Builder output, Path configFile, Function substitutions) throws IOException { + static void loadConfigWithSubstitutions(Settings.Builder output, Path configFile, Function substitutions) + throws IOException { long existingSize = Files.size(configFile); StringBuilder builder = new StringBuilder((int) existingSize); try (BufferedReader reader = Files.newBufferedReader(configFile, StandardCharsets.UTF_8)) { From 115516741fc7f7c128a0a9bae2c6f677a6714373 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Mon, 21 Mar 2022 15:27:20 -0700 Subject: [PATCH 4/5] remove unnecessary properties replacement --- .../org/elasticsearch/node/InternalSettingsPreparer.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java b/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java index 8fc9169731f97..283e47a92099d 100644 --- a/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java +++ b/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java @@ -66,7 +66,7 @@ public static Environment prepareEnvironment( } // 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); @@ -100,11 +100,9 @@ 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 esSettings) { + static void initializeSettings(final Settings.Builder output, final Settings input) { output.put(input); - output.putProperties(esSettings, Function.identity()); output.replacePropertyPlaceholders(); } From 8560f40032d3ca03b4f251d533b340dac1776d90 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Mon, 21 Mar 2022 16:14:44 -0700 Subject: [PATCH 5/5] fix test --- .../elasticsearch/node/InternalSettingsPreparer.java | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java b/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java index 283e47a92099d..58ae937255c21 100644 --- a/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java +++ b/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java @@ -56,15 +56,17 @@ 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 { loadConfigWithSubstitutions(output, path, System::getenv); - loadOverrides(output, properties); } 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); finalizeSettings(output, defaultNodeName); @@ -146,7 +148,7 @@ static void loadConfigWithSubstitutions(Settings.Builder output, Path configFile output.loadFromStream(configFile.getFileName().toString(), is, false); } - static void loadOverrides(Settings.Builder output, Map overrides) throws IOException { + static void loadOverrides(Settings.Builder output, Map overrides) { StringBuilder builder = new StringBuilder(); for (var entry : overrides.entrySet()) { builder.append(entry.getKey()); @@ -156,7 +158,11 @@ static void loadOverrides(Settings.Builder output, Map overrides } var is = new ByteArrayInputStream(builder.toString().getBytes(StandardCharsets.UTF_8)); // fake the resource name so it loads yaml - output.loadFromStream("overrides.yml", is, false); + try { + output.loadFromStream("overrides.yml", is, false); + } catch (IOException e) { + throw new SettingsException("Malformed setting override value", e); + } } /**