From 6ba3002569e499cc1ebba3f53b6028234f98fa66 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Thu, 13 Apr 2017 06:34:58 -0400 Subject: [PATCH] Correct handling of default and array settings In Elasticsearch 5.3.0 a bug was introduced in the merging of default settings when the target setting existed as an array. This arose due to the fact that when a target setting is an array, the setting key is broken into key.0, key.1, ..., key.n, one for each element of the array. When settings are replaced by default.key, we are looking for the target key but not the target key.0. This leads to key, and key.0, ..., key.n being present in the constructed settings object. This commit addresses two issues here. The first is that we fix the merging of the keys so that when we try to merge default.key, we also check for the presence of the flattened keys. The second is that when we try to get a setting value as an array from a settings object, we check whether or not the backing map contains the top-level key as well as the flattened keys. This latter check would have caught the first bug. For kicks, we add some tests. Relates #24074 --- .../elasticsearch/common/settings/Settings.java | 15 +++++++++++++++ .../node/InternalSettingsPreparer.java | 17 ++++++++++++----- .../common/settings/SettingsTests.java | 12 ++++++++++++ .../InternalSettingsPreparerTests.java | 13 ++++++++++++- 4 files changed, 51 insertions(+), 6 deletions(-) rename core/src/test/java/org/elasticsearch/node/{internal => }/InternalSettingsPreparerTests.java (93%) diff --git a/core/src/main/java/org/elasticsearch/common/settings/Settings.java b/core/src/main/java/org/elasticsearch/common/settings/Settings.java index 8a9805f7b9200..ad0057b21aa13 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/Settings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/Settings.java @@ -58,6 +58,7 @@ import java.util.Iterator; import java.util.LinkedHashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.NoSuchElementException; import java.util.Objects; @@ -418,6 +419,20 @@ public String[] getAsArray(String settingPrefix, String[] defaultArray) throws S public String[] getAsArray(String settingPrefix, String[] defaultArray, Boolean commaDelimited) throws SettingsException { List result = new ArrayList<>(); + final String valueFromPrefix = get(settingPrefix); + final String valueFromPreifx0 = get(settingPrefix + ".0"); + + if (valueFromPrefix != null && valueFromPreifx0 != null) { + final String message = String.format( + Locale.ROOT, + "settings object contains values for [%s=%s] and [%s=%s]", + settingPrefix, + valueFromPrefix, + settingPrefix + ".0", + valueFromPreifx0); + throw new IllegalStateException(message); + } + if (get(settingPrefix) != null) { if (commaDelimited) { String[] strings = Strings.splitStringByCommaToArray(get(settingPrefix)); diff --git a/core/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java b/core/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java index 1ab124b75a733..dad1440eeee8d 100644 --- a/core/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java +++ b/core/src/main/java/org/elasticsearch/node/InternalSettingsPreparer.java @@ -128,14 +128,21 @@ public static Environment prepareEnvironment(Settings input, Terminal terminal, } /** - * Initializes the builder with the given input settings, and loads system properties settings if allowed. - * If loadDefaults is true, system property default settings are loaded. + * Initializes the builder with the given input settings, and applies settings and default settings from the specified map (these + * settings typically come from the command line). The default settings are applied only if the setting does not exist in the specified + * output. + * + * @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 and default settings */ - private static void initializeSettings(Settings.Builder output, Settings input, Map esSettings) { + static void initializeSettings(final Settings.Builder output, final Settings input, final Map esSettings) { output.put(input); output.putProperties(esSettings, - PROPERTY_DEFAULTS_PREDICATE.and(key -> output.get(STRIP_PROPERTY_DEFAULTS_PREFIX.apply(key)) == null), - STRIP_PROPERTY_DEFAULTS_PREFIX); + PROPERTY_DEFAULTS_PREDICATE + .and(key -> output.get(STRIP_PROPERTY_DEFAULTS_PREFIX.apply(key)) == null) + .and(key -> output.get(STRIP_PROPERTY_DEFAULTS_PREFIX.apply(key) + ".0") == null), + STRIP_PROPERTY_DEFAULTS_PREFIX); output.putProperties(esSettings, PROPERTY_DEFAULTS_PREDICATE.negate(), Function.identity()); output.replacePropertyPlaceholders(); } diff --git a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java index 36202784b455b..bdc80188d8ed6 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/SettingsTests.java @@ -487,4 +487,16 @@ public void testEmpty() { MockSecureSettings secureSettings = new MockSecureSettings(); assertTrue(Settings.builder().setSecureSettings(secureSettings).build().isEmpty()); } + + public void testGetAsArrayFailsOnDuplicates() { + final Settings settings = + Settings.builder() + .put("foobar.0", "bar") + .put("foobar.1", "baz") + .put("foobar", "foo") + .build(); + final IllegalStateException e = expectThrows(IllegalStateException.class, () -> settings.getAsArray("foobar")); + assertThat(e, hasToString(containsString("settings object contains values for [foobar=foo] and [foobar.0=bar]"))); + } + } diff --git a/core/src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java b/core/src/test/java/org/elasticsearch/node/InternalSettingsPreparerTests.java similarity index 93% rename from core/src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java rename to core/src/test/java/org/elasticsearch/node/InternalSettingsPreparerTests.java index 21c1811616d55..ae9f64c2cb4d9 100644 --- a/core/src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java +++ b/core/src/test/java/org/elasticsearch/node/InternalSettingsPreparerTests.java @@ -17,7 +17,7 @@ * under the License. */ -package org.elasticsearch.node.internal; +package org.elasticsearch.node; import org.elasticsearch.cli.MockTerminal; import org.elasticsearch.cluster.ClusterName; @@ -196,4 +196,15 @@ public void testDefaultPropertiesOverride() throws Exception { Environment env = InternalSettingsPreparer.prepareEnvironment(baseEnvSettings, null, props); assertEquals("bar", env.settings().get("setting")); } + + public void testDefaultWithArray() { + final Settings.Builder output = Settings.builder().put("foobar.0", "bar").put("foobar.1", "baz"); + final Map esSettings = Collections.singletonMap("default.foobar", "foo"); + InternalSettingsPreparer.initializeSettings(output, Settings.EMPTY, esSettings); + final Settings settings = output.build(); + assertThat(settings.get("foobar.0"), equalTo("bar")); + assertThat(settings.get("foobar.1"), equalTo("baz")); + assertNull(settings.get("foobar")); + } + }