Include fallback settings when checking dependencies (#33522)

Today when checking settings dependencies, we do not check if fallback
settings are present. This means, for example, that if
cluster.remote.*.seeds falls back to search.remote.*.seeds, and
cluster.remote.*.skip_unavailable and search.remote.*.skip_unavailable
depend on cluster.remote.*.seeds, and we have set search.remote.*.seeds
and search.remote.*.skip_unavailable, then validation will fail because
it is expected that cluster.ermote.*.seeds is set here. This commit
addresses this by also checking fallback settings when validating
dependencies. To do this, we adjust the settings exist method to also
check for fallback settings, a case that it was not handling previously.

Author: jasontedor

qa/ccs-unavailable-clusters/src/test/java/org/elasticsearch/search/CrossClusterSearchUnavailableClusterIT.java

@@ -235,7 +235,7 @@ public void testSkipUnavailableDependsOnSeeds() throws IOException {
                         () -> client().performRequest(request));
                 assertEquals(400, responseException.getResponse().getStatusLine().getStatusCode());
                 assertThat(responseException.getMessage(),
-                        containsString("Missing required setting [cluster.remote.remote1.seeds] " +
+                        containsString("missing required setting [cluster.remote.remote1.seeds] " +
                                 "for setting [cluster.remote.remote1.skip_unavailable]"));
             }
 
@@ -251,7 +251,7 @@ public void testSkipUnavailableDependsOnSeeds() throws IOException {
                 ResponseException responseException = expectThrows(ResponseException.class,
                         () -> client().performRequest(request));
                 assertEquals(400, responseException.getResponse().getStatusLine().getStatusCode());
-                assertThat(responseException.getMessage(), containsString("Missing required setting [cluster.remote.remote1.seeds] " +
+                assertThat(responseException.getMessage(), containsString("missing required setting [cluster.remote.remote1.seeds] " +
                         "for setting [cluster.remote.remote1.skip_unavailable]"));
             }
 

server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java

@@ -32,6 +32,7 @@
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.CopyOnWriteArrayList;
@@ -461,16 +462,19 @@ void validate(
             }
             throw new IllegalArgumentException(msg);
         } else  {
-            Set<String> settingsDependencies = setting.getSettingsDependencies(key);
+            Set<Setting<?>> settingsDependencies = setting.getSettingsDependencies(key);
             if (setting.hasComplexMatcher()) {
                 setting = setting.getConcreteSetting(key);
             }
             if (validateDependencies && settingsDependencies.isEmpty() == false) {
-                Set<String> settingKeys = settings.keySet();
-                for (String requiredSetting : settingsDependencies) {
-                    if (settingKeys.contains(requiredSetting) == false) {
-                        throw new IllegalArgumentException("Missing required setting ["
-                            + requiredSetting + "] for setting [" + setting.getKey() + "]");
+                for (final Setting<?> settingDependency : settingsDependencies) {
+                    if (settingDependency.existsOrFallbackExists(settings) == false) {
+                        final String message = String.format(
+                                Locale.ROOT,
+                                "missing required setting [%s] for setting [%s]",
+                                settingDependency.getKey(),
+                                setting.getKey());
+                        throw new IllegalArgumentException(message);
                     }
                 }
             }

server/src/main/java/org/elasticsearch/common/settings/Setting.java

@@ -366,12 +366,25 @@ public T getDefault(Settings settings) {
     }
 
     /**
-     * Returns <code>true</code> iff this setting is present in the given settings object. Otherwise <code>false</code>
+     * Returns true if and only if this setting is present in the given settings instance. Note that fallback settings are excluded.
+     *
+     * @param settings the settings
+     * @return true if the setting is present in the given settings instance, otherwise false
      */
-    public boolean exists(Settings settings) {
+    public boolean exists(final Settings settings) {
         return settings.keySet().contains(getKey());
     }
 
+    /**
+     * Returns true if and only if this setting including fallback settings is present in the given settings instance.
+     *
+     * @param settings the settings
+     * @return true if the setting including fallback settings is present in the given settings instance, otherwise false
+     */
+    public boolean existsOrFallbackExists(final Settings settings) {
+        return settings.keySet().contains(getKey()) || (fallbackSetting != null && fallbackSetting.existsOrFallbackExists(settings));
+    }
+
     /**
      * Returns the settings value. If the setting is not present in the given settings object the default value is returned
      * instead.
@@ -511,7 +524,7 @@ public Setting<T> getConcreteSetting(String key) {
      * Returns a set of settings that are required at validation time. Unless all of the dependencies are present in the settings
      * object validation of setting must fail.
      */
-    public Set<String> getSettingsDependencies(String key) {
+    public Set<Setting<?>> getSettingsDependencies(String key) {
         return Collections.emptySet();
     }
 
@@ -634,12 +647,12 @@ private Stream<String> matchStream(Settings settings) {
             return settings.keySet().stream().filter(this::match).map(key::getConcreteString);
         }
 
-        public Set<String> getSettingsDependencies(String settingsKey) {
+        public Set<Setting<?>> getSettingsDependencies(String settingsKey) {
             if (dependencies.isEmpty()) {
                 return Collections.emptySet();
             } else {
                 String namespace = key.getNamespace(settingsKey);
-                return dependencies.stream().map(s -> s.key.toConcreteKey(namespace).key).collect(Collectors.toSet());
+                return dependencies.stream().map(s -> (Setting<?>)s.getConcreteSettingForNamespace(namespace)).collect(Collectors.toSet());
             }
         }
 
@@ -914,40 +927,6 @@ public String toString() {
         }
     }
 
-    private static class ListSetting<T> extends Setting<List<T>> {
-        private final Function<Settings, List<String>> defaultStringValue;
-
-        private ListSetting(String key, Function<Settings, List<String>> defaultStringValue, Function<String, List<T>> parser,
-                            Property... properties) {
-            super(new ListKey(key), (s) -> Setting.arrayToParsableString(defaultStringValue.apply(s)), parser,
-                properties);
-            this.defaultStringValue = defaultStringValue;
-        }
-
-        @Override
-        String innerGetRaw(final Settings settings) {
-            List<String> array = settings.getAsList(getKey(), null);
-            return array == null ? defaultValue.apply(settings) : arrayToParsableString(array);
-        }
-
-        @Override
-        boolean hasComplexMatcher() {
-            return true;
-        }
-
-        @Override
-        public void diff(Settings.Builder builder, Settings source, Settings defaultSettings) {
-            if (exists(source) == false) {
-                List<String> asList = defaultSettings.getAsList(getKey(), null);
-                if (asList == null) {
-                    builder.putList(getKey(), defaultStringValue.apply(defaultSettings));
-                } else {
-                    builder.putList(getKey(), asList);
-                }
-            }
-        }
-    }
-
     private final class Updater implements AbstractScopedSettings.SettingUpdater<T> {
         private final Consumer<T> consumer;
         private final Logger logger;
@@ -1209,26 +1188,44 @@ public static Setting<ByteSizeValue> memorySizeSetting(String key, String defaul
         return new Setting<>(key, (s) -> defaultPercentage, (s) -> MemorySizeValue.parseBytesSizeValueOrHeapRatio(s, key), properties);
     }
 
-    public static <T> Setting<List<T>> listSetting(String key, List<String> defaultStringValue, Function<String, T> singleValueParser,
-                                                   Property... properties) {
-        return listSetting(key, (s) -> defaultStringValue, singleValueParser, properties);
+    public static <T> Setting<List<T>> listSetting(
+            final String key,
+            final List<String> defaultStringValue,
+            final Function<String, T> singleValueParser,
+            final Property... properties) {
+        return listSetting(key, null, singleValueParser, (s) -> defaultStringValue, properties);
     }
 
     // TODO this one's two argument get is still broken
-    public static <T> Setting<List<T>> listSetting(String key, Setting<List<T>> fallbackSetting, Function<String, T> singleValueParser,
-                                                   Property... properties) {
-        return listSetting(key, (s) -> parseableStringToList(fallbackSetting.getRaw(s)), singleValueParser, properties);
+    public static <T> Setting<List<T>> listSetting(
+            final String key,
+            final Setting<List<T>> fallbackSetting,
+            final Function<String, T> singleValueParser,
+            final Property... properties) {
+        return listSetting(key, fallbackSetting, singleValueParser, (s) -> parseableStringToList(fallbackSetting.getRaw(s)), properties);
+    }
+
+    public static <T> Setting<List<T>> listSetting(
+            final String key,
+            final Function<String, T> singleValueParser,
+            final Function<Settings, List<String>> defaultStringValue,
+            final Property... properties) {
+        return listSetting(key, null, singleValueParser, defaultStringValue, properties);
     }
 
-    public static <T> Setting<List<T>> listSetting(String key, Function<Settings, List<String>> defaultStringValue,
-                                                   Function<String, T> singleValueParser, Property... properties) {
+    public static <T> Setting<List<T>> listSetting(
+            final String key,
+            final @Nullable Setting<List<T>> fallbackSetting,
+            final Function<String, T> singleValueParser,
+            final Function<Settings, List<String>> defaultStringValue,
+            final Property... properties) {
         if (defaultStringValue.apply(Settings.EMPTY) == null) {
             throw new IllegalArgumentException("default value function must not return null");
         }
         Function<String, List<T>> parser = (s) ->
                 parseableStringToList(s).stream().map(singleValueParser).collect(Collectors.toList());
 
-        return new ListSetting<>(key, defaultStringValue, parser, properties);
+        return new ListSetting<>(key, fallbackSetting, defaultStringValue, parser, properties);
     }
 
     private static List<String> parseableStringToList(String parsableString) {
@@ -1266,6 +1263,51 @@ private static String arrayToParsableString(List<String> array) {
         }
     }
 
+    private static class ListSetting<T> extends Setting<List<T>> {
+
+        private final Function<Settings, List<String>> defaultStringValue;
+
+        private ListSetting(
+                final String key,
+                final @Nullable Setting<List<T>> fallbackSetting,
+                final Function<Settings, List<String>> defaultStringValue,
+                final Function<String, List<T>> parser,
+                final Property... properties) {
+            super(
+                    new ListKey(key),
+                    fallbackSetting,
+                    (s) -> Setting.arrayToParsableString(defaultStringValue.apply(s)),
+                    parser,
+                    (v,s) -> {},
+                    properties);
+            this.defaultStringValue = defaultStringValue;
+        }
+
+        @Override
+        String innerGetRaw(final Settings settings) {
+            List<String> array = settings.getAsList(getKey(), null);
+            return array == null ? defaultValue.apply(settings) : arrayToParsableString(array);
+        }
+
+        @Override
+        boolean hasComplexMatcher() {
+            return true;
+        }
+
+        @Override
+        public void diff(Settings.Builder builder, Settings source, Settings defaultSettings) {
+            if (exists(source) == false) {
+                List<String> asList = defaultSettings.getAsList(getKey(), null);
+                if (asList == null) {
+                    builder.putList(getKey(), defaultStringValue.apply(defaultSettings));
+                } else {
+                    builder.putList(getKey(), asList);
+                }
+            }
+        }
+
+    }
+
     static void logSettingUpdate(Setting setting, Settings current, Settings previous, Logger logger) {
         if (logger.isInfoEnabled()) {
             if (setting.isFiltered()) {

server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java

@@ -171,7 +171,7 @@ public void testDependentSettings() {
 
         IllegalArgumentException iae = expectThrows(IllegalArgumentException.class,
             () -> service.validate(Settings.builder().put("foo.test.bar", 7).build(), true));
-        assertEquals("Missing required setting [foo.test.name] for setting [foo.test.bar]", iae.getMessage());
+        assertEquals("missing required setting [foo.test.name] for setting [foo.test.bar]", iae.getMessage());
 
         service.validate(Settings.builder()
             .put("foo.test.name", "test")
@@ -181,6 +181,34 @@ public void testDependentSettings() {
         service.validate(Settings.builder().put("foo.test.bar", 7).build(), false);
     }
 
+    public void testDependentSettingsWithFallback() {
+        Setting.AffixSetting<String> nameFallbackSetting =
+                Setting.affixKeySetting("fallback.", "name", k -> Setting.simpleString(k, Property.Dynamic, Property.NodeScope));
+        Setting.AffixSetting<String> nameSetting = Setting.affixKeySetting(
+                "foo.",
+                "name",
+                k -> Setting.simpleString(
+                        k,
+                        "_na_".equals(k)
+                                ? nameFallbackSetting.getConcreteSettingForNamespace(k)
+                                : nameFallbackSetting.getConcreteSetting(k.replaceAll("^foo", "fallback")),
+                        Property.Dynamic,
+                        Property.NodeScope));
+        Setting.AffixSetting<Integer> barSetting =
+                Setting.affixKeySetting("foo.", "bar", k -> Setting.intSetting(k, 1, Property.Dynamic, Property.NodeScope), nameSetting);
+
+        final AbstractScopedSettings service =
+                new ClusterSettings(Settings.EMPTY,new HashSet<>(Arrays.asList(nameFallbackSetting, nameSetting, barSetting)));
+
+        final IllegalArgumentException e = expectThrows(
+                IllegalArgumentException.class,
+                () -> service.validate(Settings.builder().put("foo.test.bar", 7).build(), true));
+        assertThat(e, hasToString(containsString("missing required setting [foo.test.name] for setting [foo.test.bar]")));
+
+        service.validate(Settings.builder().put("foo.test.name", "test").put("foo.test.bar", 7).build(), true);
+        service.validate(Settings.builder().put("fallback.test.name", "test").put("foo.test.bar", 7).build(), true);
+    }
+
     public void testTupleAffixUpdateConsumer() {
         String prefix = randomAlphaOfLength(3) + "foo.";
         String intSuffix = randomAlphaOfLength(3);

server/src/test/java/org/elasticsearch/common/settings/SettingTests.java

@@ -856,4 +856,30 @@ public void testAffixNamespacesWithGroupSetting() {
         assertThat(affixSetting.getNamespaces(Settings.builder().put("prefix.infix.suffix", "anything").build()), hasSize(1));
         assertThat(affixSetting.getNamespaces(Settings.builder().put("prefix.infix.suffix.anything", "anything").build()), hasSize(1));
     }
+
+    public void testExists() {
+        final Setting<?> fooSetting = Setting.simpleString("foo", Property.NodeScope);
+        assertFalse(fooSetting.exists(Settings.EMPTY));
+        assertTrue(fooSetting.exists(Settings.builder().put("foo", "bar").build()));
+    }
+
+    public void testExistsWithFallback() {
+        final int count = randomIntBetween(1, 16);
+        Setting<String> current = Setting.simpleString("fallback0", Property.NodeScope);
+        for (int i = 1; i < count; i++) {
+            final Setting<String> next =
+                    new Setting<>(new Setting.SimpleKey("fallback" + i), current, Function.identity(), Property.NodeScope);
+            current = next;
+        }
+        final Setting<String> fooSetting = new Setting<>(new Setting.SimpleKey("foo"), current, Function.identity(), Property.NodeScope);
+        assertFalse(fooSetting.exists(Settings.EMPTY));
+        if (randomBoolean()) {
+            assertTrue(fooSetting.exists(Settings.builder().put("foo", "bar").build()));
+        } else {
+            final String setting = "fallback" + randomIntBetween(0, count - 1);
+            assertFalse(fooSetting.exists(Settings.builder().put(setting, "bar").build()));
+            assertTrue(fooSetting.existsOrFallbackExists(Settings.builder().put(setting, "bar").build()));
+        }
+    }
+
 }