From 55d37503c2c643340f8109574af51c2a26870c3b Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Mon, 21 Mar 2022 17:28:22 -0700 Subject: [PATCH] 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 | 6 ++ .../common/settings/Settings.java | 12 +-- .../node/InternalSettingsPreparer.java | 80 ++++++++++++++---- .../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 + 11 files changed, 169 insertions(+), 43 deletions(-) create mode 100644 docs/changelog/85186.yaml 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/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 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..58ae937255c21 100644 --- a/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java +++ b/server/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java @@ -15,11 +15,13 @@ 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; @@ -27,14 +29,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. * @@ -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 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 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) { + 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. */ 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