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

Author: jasontedor

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

@@ -57,6 +57,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;
@@ -442,6 +443,20 @@ public String[] getAsArray(String settingPrefix, String[] defaultArray) throws S
     public String[] getAsArray(String settingPrefix, String[] defaultArray, Boolean commaDelimited) throws SettingsException {
         List<String> 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));

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

@@ -125,14 +125,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<String, String> esSettings) {
+    static void initializeSettings(final Settings.Builder output, final Settings input, final Map<String, String> 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();
     }

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

@@ -562,4 +562,16 @@ public void testSecureSettingConflict() {
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> setting.get(settings));
         assertTrue(e.getMessage().contains("must be stored inside the Elasticsearch keystore"));
     }
+
+    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]")));
+    }
+
 }

core/src/test/java/org/elasticsearch/node/internal/InternalSettingsPreparerTests.java renamed to 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<String, String> 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"));
+    }
+
 }