Separate out validation of groups of settings (#34184)

Today, a setting can declare that its validity depends on the values of other
related settings. However, the validity of a setting is not always checked
against the correct values of its dependent settings because those settings'
correct values may not be available when the validator runs.

This commit separates the validation of a settings updates into two phases,
with separate methods on the `Setting.Validator` interface. In the first phase
the setting's validity is checked in isolation, and in the second phase it is
checked again against the values of its related settings. Most settings only
use the first phase, and only the few settings with dependencies make use of
the second phase.

plugins/examples/custom-settings/src/main/java/org/elasticsearch/example/customsettings/ExampleCustomSettingsConfig.java

@@ -49,7 +49,7 @@ public class ExampleCustomSettingsConfig {
     /**
      * A string setting that can be dynamically updated and that is validated by some logic
      */
-    static final Setting<String> VALIDATED_SETTING = Setting.simpleString("custom.validated", (value, settings) -> {
+    static final Setting<String> VALIDATED_SETTING = Setting.simpleString("custom.validated", value -> {
         if (value != null && value.contains("forbidden")) {
             throw new IllegalArgumentException("Setting must not contain [forbidden]");
         }

server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetaData.java

@@ -157,6 +157,10 @@ static Setting<Integer> buildNumberOfShardsSetting() {
     public static final Setting<Integer> INDEX_NUMBER_OF_ROUTING_SHARDS_SETTING =
         Setting.intSetting("index.number_of_routing_shards", INDEX_NUMBER_OF_SHARDS_SETTING,
                            1, new Setting.Validator<Integer>() {
+            @Override
+            public void validate(Integer value) {
+            }
+
             @Override
             public void validate(Integer numRoutingShards, Map<Setting<Integer>, Integer> settings) {
                 Integer numShards = settings.get(INDEX_NUMBER_OF_SHARDS_SETTING);
@@ -223,14 +227,14 @@ public Iterator<Setting<Integer>> settings() {
     public static final String INDEX_ROUTING_INCLUDE_GROUP_PREFIX = "index.routing.allocation.include";
     public static final String INDEX_ROUTING_EXCLUDE_GROUP_PREFIX = "index.routing.allocation.exclude";
     public static final Setting.AffixSetting<String> INDEX_ROUTING_REQUIRE_GROUP_SETTING =
-        Setting.prefixKeySetting(INDEX_ROUTING_REQUIRE_GROUP_PREFIX + ".", (key) ->
-            Setting.simpleString(key, (value, map) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope));
+        Setting.prefixKeySetting(INDEX_ROUTING_REQUIRE_GROUP_PREFIX + ".", key ->
+            Setting.simpleString(key, value -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope));
     public static final Setting.AffixSetting<String> INDEX_ROUTING_INCLUDE_GROUP_SETTING =
-        Setting.prefixKeySetting(INDEX_ROUTING_INCLUDE_GROUP_PREFIX + ".", (key) ->
-            Setting.simpleString(key, (value, map) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope));
+        Setting.prefixKeySetting(INDEX_ROUTING_INCLUDE_GROUP_PREFIX + ".", key ->
+            Setting.simpleString(key, value -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope));
     public static final Setting.AffixSetting<String> INDEX_ROUTING_EXCLUDE_GROUP_SETTING =
-        Setting.prefixKeySetting(INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + ".", (key) ->
-            Setting.simpleString(key, (value, map) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope));
+        Setting.prefixKeySetting(INDEX_ROUTING_EXCLUDE_GROUP_PREFIX + ".", key ->
+            Setting.simpleString(key, value -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.IndexScope));
     public static final Setting.AffixSetting<String> INDEX_ROUTING_INITIAL_RECOVERY_GROUP_SETTING =
         Setting.prefixKeySetting("index.routing.allocation.initial_recovery.", key -> Setting.simpleString(key));
         // this is only setable internally not a registered setting!!

server/src/main/java/org/elasticsearch/cluster/routing/allocation/DiskThresholdSettings.java

@@ -93,6 +93,10 @@ public DiskThresholdSettings(Settings settings, ClusterSettings clusterSettings)
 
     static final class LowDiskWatermarkValidator implements Setting.Validator<String> {
 
+        @Override
+        public void validate(String value) {
+        }
+
         @Override
         public void validate(String value, Map<Setting<String>, String> settings) {
             final String highWatermarkRaw = settings.get(CLUSTER_ROUTING_ALLOCATION_HIGH_DISK_WATERMARK_SETTING);
@@ -112,6 +116,10 @@ public Iterator<Setting<String>> settings() {
 
     static final class HighDiskWatermarkValidator implements Setting.Validator<String> {
 
+        @Override
+        public void validate(String value) {
+        }
+
         @Override
         public void validate(String value, Map<Setting<String>, String> settings) {
             final String lowWatermarkRaw = settings.get(CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING);
@@ -131,6 +139,10 @@ public Iterator<Setting<String>> settings() {
 
     static final class FloodStageValidator implements Setting.Validator<String> {
 
+        @Override
+        public void validate(String value) {
+        }
+
         @Override
         public void validate(String value, Map<Setting<String>, String> settings) {
             final String lowWatermarkRaw = settings.get(CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING);

server/src/main/java/org/elasticsearch/cluster/routing/allocation/decider/FilterAllocationDecider.java

@@ -72,14 +72,14 @@ public class FilterAllocationDecider extends AllocationDecider {
     private static final String CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX = "cluster.routing.allocation.include";
     private static final String CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX = "cluster.routing.allocation.exclude";
     public static final Setting.AffixSetting<String> CLUSTER_ROUTING_REQUIRE_GROUP_SETTING =
-        Setting.prefixKeySetting(CLUSTER_ROUTING_REQUIRE_GROUP_PREFIX + ".", (key) ->
-            Setting.simpleString(key, (value, map) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope));
+        Setting.prefixKeySetting(CLUSTER_ROUTING_REQUIRE_GROUP_PREFIX + ".", key ->
+            Setting.simpleString(key, value -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope));
     public static final Setting.AffixSetting<String> CLUSTER_ROUTING_INCLUDE_GROUP_SETTING =
-        Setting.prefixKeySetting(CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX + ".", (key) ->
-            Setting.simpleString(key, (value, map) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope));
+        Setting.prefixKeySetting(CLUSTER_ROUTING_INCLUDE_GROUP_PREFIX + ".", key ->
+            Setting.simpleString(key, value -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope));
     public static final Setting.AffixSetting<String>CLUSTER_ROUTING_EXCLUDE_GROUP_SETTING =
-        Setting.prefixKeySetting(CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX + ".", (key) ->
-            Setting.simpleString(key, (value, map) -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope));
+        Setting.prefixKeySetting(CLUSTER_ROUTING_EXCLUDE_GROUP_PREFIX + ".", key ->
+            Setting.simpleString(key, value -> IP_VALIDATOR.accept(key, value), Property.Dynamic, Property.NodeScope));
 
     /**
      * The set of {@link RecoverySource.Type} values for which the

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

@@ -723,7 +723,7 @@ private boolean updateSettings(Settings toApply, Settings.Builder target, Settin
             } else if (get(key) == null) {
                 throw new IllegalArgumentException(type + " setting [" + key + "], not recognized");
             } else if (isDelete == false && canUpdate.test(key)) {
-                validate(key, toApply, false); // we might not have a full picture here do to a dependency validation
+                get(key).validateWithoutDependencies(toApply); // we might not have a full picture here do to a dependency validation
                 settingsBuilder.copy(key, toApply);
                 updates.copy(key, toApply);
                 changed |= toApply.get(key).equals(target.get(key)) == false;

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

@@ -186,7 +186,7 @@ private void checkPropertyRequiresIndexScope(final EnumSet<Property> properties,
      * @param properties properties for this setting like scope, filtering...
      */
     public Setting(Key key, Function<Settings, String> defaultValue, Function<String, T> parser, Property... properties) {
-        this(key, defaultValue, parser, (v, s) -> {}, properties);
+        this(key, defaultValue, parser, v -> {}, properties);
     }
 
     /**
@@ -246,7 +246,7 @@ public Setting(String key, Function<Settings, String> defaultValue, Function<Str
      * @param properties properties for this setting like scope, filtering...
      */
     public Setting(Key key, Setting<T> fallbackSetting, Function<String, T> parser, Property... properties) {
-        this(key, fallbackSetting, fallbackSetting::getRaw, parser, (v, m) -> {}, properties);
+        this(key, fallbackSetting, fallbackSetting::getRaw, parser, v -> {}, properties);
     }
 
     /**
@@ -354,6 +354,14 @@ boolean hasComplexMatcher() {
         return isGroupSetting();
     }
 
+    /**
+     * Validate the current setting value only without dependencies with {@link Setting.Validator#validate(Object)}.
+     * @param settings a settings object for settings that has a default value depending on another setting if available
+     */
+    void validateWithoutDependencies(Settings settings) {
+        validator.validate(get(settings, false));
+    }
+
     /**
      * Returns the default value string representation for this setting.
      * @param settings a settings object for settings that has a default value depending on another setting if available
@@ -414,6 +422,7 @@ private T get(Settings settings, boolean validate) {
                 } else {
                     map = Collections.emptyMap();
                 }
+                validator.validate(parsed);
                 validator.validate(parsed, map);
             }
             return parsed;
@@ -805,26 +814,39 @@ public Map<String, T> getAsMap(Settings settings) {
     }
 
     /**
-     * Represents a validator for a setting. The {@link #validate(Object, Map)} method is invoked with the value of this setting and a map
-     * from the settings specified by {@link #settings()}} to their values. All these values come from the same {@link Settings} instance.
+     * Represents a validator for a setting. The {@link #validate(Object)} method is invoked early in the update setting process with the
+     * value of this setting for a fail-fast validation. Later on, the {@link #validate(Object, Map)} method is invoked with the value of
+     * this setting and a map from the settings specified by {@link #settings()}} to their values. All these values come from the same
+     * {@link Settings} instance.
      *
      * @param <T> the type of the {@link Setting}
      */
     @FunctionalInterface
     public interface Validator<T> {
 
         /**
-         * The validation routine for this validator.
+         * Validate this setting's value in isolation.
+         *
+         * @param value the value of this setting
+         */
+        void validate(T value);
+
+        /**
+         * Validate this setting against its dependencies, specified by {@link #settings()}. The default implementation does nothing,
+         * accepting any value as valid as long as it passes the validation in {@link #validate(Object)}.
          *
          * @param value    the value of this setting
          * @param settings a map from the settings specified by {@link #settings()}} to their values
          */
-        void validate(T value, Map<Setting<T>, T> settings);
+        default void validate(T value, Map<Setting<T>, T> settings) {
+        }
 
         /**
-         * The settings needed by this validator.
+         * The settings on which the validity of this setting depends. The values of the specified settings are passed to
+         * {@link #validate(Object, Map)}. By default this returns an empty iterator, indicating that this setting does not depend on any
+         * other settings.
          *
-         * @return the settings needed to validate; these can be used for cross-settings validation
+         * @return the settings on which the validity of this setting depends.
          */
         default Iterator<Setting<T>> settings() {
             return Collections.emptyIterator();
@@ -1021,8 +1043,8 @@ public static Setting<String> simpleString(String key, Property... properties) {
         return new Setting<>(key, s -> "", Function.identity(), properties);
     }
 
-    public static Setting<String> simpleString(String key, Function<String, String> parser, Property... properties) {
-        return new Setting<>(key, s -> "", parser, properties);
+    public static Setting<String> simpleString(String key, Validator<String> validator, Property... properties) {
+        return new Setting<>(new SimpleKey(key), null, s -> "", Function.identity(), validator, properties);
     }
 
     public static Setting<String> simpleString(String key, Setting<String> fallback, Property... properties) {
@@ -1037,10 +1059,6 @@ public static Setting<String> simpleString(
         return new Setting<>(key, fallback, parser, properties);
     }
 
-    public static Setting<String> simpleString(String key, Validator<String> validator, Property... properties) {
-        return new Setting<>(new SimpleKey(key), null, s -> "", Function.identity(), validator, properties);
-    }
-
     /**
      * Creates a new Setting instance with a String value
      *
@@ -1279,9 +1297,9 @@ private ListSetting(
             super(
                     new ListKey(key),
                     fallbackSetting,
-                    (s) -> Setting.arrayToParsableString(defaultStringValue.apply(s)),
+                    s -> Setting.arrayToParsableString(defaultStringValue.apply(s)),
                     parser,
-                    (v,s) -> {},
+                    v -> {},
                     properties);
             this.defaultStringValue = defaultStringValue;
         }
@@ -1339,7 +1357,7 @@ public static Setting<TimeValue> timeSetting(
                 fallbackSetting,
                 fallbackSetting::getRaw,
                 minTimeValueParser(key, minValue),
-                (v, s) -> {},
+                v -> {},
                 properties);
     }
 

server/src/main/java/org/elasticsearch/threadpool/AutoQueueAdjustingExecutorBuilder.java

@@ -75,8 +75,12 @@ public final class AutoQueueAdjustingExecutorBuilder extends ExecutorBuilder<Aut
         this.minQueueSizeSetting = new Setting<>(
             minSizeKey,
             Integer.toString(minQueueSize),
-            (s) -> Setting.parseInt(s, 0, minSizeKey),
+            s -> Setting.parseInt(s, 0, minSizeKey),
             new Setting.Validator<Integer>() {
+                @Override
+                public void validate(Integer value) {
+                }
+
                 @Override
                 public void validate(Integer value, Map<Setting<Integer>, Integer> settings) {
                     if (value > settings.get(tempMaxQueueSizeSetting)) {
@@ -94,8 +98,12 @@ public Iterator<Setting<Integer>> settings() {
         this.maxQueueSizeSetting = new Setting<>(
                 maxSizeKey,
                 Integer.toString(maxQueueSize),
-                (s) -> Setting.parseInt(s, 0, maxSizeKey),
+                s -> Setting.parseInt(s, 0, maxSizeKey),
                 new Setting.Validator<Integer>() {
+                    @Override
+                    public void validate(Integer value) {
+                    }
+
                     @Override
                     public void validate(Integer value, Map<Setting<Integer>, Integer> settings) {
                         if (value < settings.get(tempMinQueueSizeSetting)) {

server/src/main/java/org/elasticsearch/transport/RemoteClusterAware.java

@@ -121,7 +121,6 @@ public String getKey(final String key) {
                         if (Strings.hasLength(s)) {
                             parsePort(s);
                         }
-                        return s;
                     },
                     Setting.Property.Deprecated,
                     Setting.Property.Dynamic,