Support dependent validation for index settings (elastic#70144) (elastic#70275)

A setting validator can declare settings that the validation depends on,
but when updating index settings, we eagerly validate the settings
before submitting the cluster state update and here we do not know the
existing settings. With this commit, we ensure that the pre-validation
only validates the keys and not the values, leaving the value validation
to after we have combined existing settings with the new settings on a
per index basis.

Author: henningandersen

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

@@ -85,7 +85,7 @@ public void updateSettings(final UpdateSettingsClusterStateUpdateRequest request
 
         indexScopedSettings.validate(
                 normalizedSettings.filter(s -> Regex.isSimpleMatchPattern(s) == false), // don't validate wildcards
-                false, // don't validate dependencies here we check it below never allow to change the number of shards
+                false, // don't validate values here we check it below never allow to change the number of shards
                 true); // validate internal or private index settings
         for (String key : normalizedSettings.keySet()) {
             Setting setting = indexScopedSettings.get(key);

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

@@ -413,55 +413,55 @@ public synchronized <T> void addSettingsUpdateConsumer(Setting<T> setting, Consu
      * Validates that all settings are registered and valid.
      *
      * @param settings             the settings to validate
-     * @param validateDependencies true if dependent settings should be validated
+     * @param validateValues       true if values should be validated, otherwise only keys are validated
      * @see Setting#getSettingsDependencies(String)
      */
-    public final void validate(final Settings settings, final boolean validateDependencies) {
-        validate(settings, validateDependencies, false, false);
+    public final void validate(final Settings settings, final boolean validateValues) {
+        validate(settings, validateValues, false, false);
     }
 
     /**
      * Validates that all settings are registered and valid.
      *
      * @param settings                       the settings to validate
-     * @param validateDependencies           true if dependent settings should be validated
+     * @param validateValues                 true if values should be validated, otherwise only keys are validated
      * @param validateInternalOrPrivateIndex true if internal index settings should be validated
      * @see Setting#getSettingsDependencies(String)
      */
-    public final void validate(final Settings settings, final boolean validateDependencies, final boolean validateInternalOrPrivateIndex) {
-        validate(settings, validateDependencies, false, false, validateInternalOrPrivateIndex);
+    public final void validate(final Settings settings, final boolean validateValues, final boolean validateInternalOrPrivateIndex) {
+        validate(settings, validateValues, false, false, validateInternalOrPrivateIndex);
     }
 
     /**
      * Validates that all settings are registered and valid.
      *
      * @param settings               the settings
-     * @param validateDependencies   true if dependent settings should be validated
+     * @param validateValues         true if values should be validated, otherwise only keys are validated
      * @param ignorePrivateSettings  true if private settings should be ignored during validation
      * @param ignoreArchivedSettings true if archived settings should be ignored during validation
      * @see Setting#getSettingsDependencies(String)
      */
     public final void validate(
             final Settings settings,
-            final boolean validateDependencies,
+            final boolean validateValues,
             final boolean ignorePrivateSettings,
             final boolean ignoreArchivedSettings) {
-        validate(settings, validateDependencies, ignorePrivateSettings, ignoreArchivedSettings, false);
+        validate(settings, validateValues, ignorePrivateSettings, ignoreArchivedSettings, false);
     }
 
     /**
      * Validates that all settings are registered and valid.
      *
      * @param settings                       the settings
-     * @param validateDependencies           true if dependent settings should be validated
+     * @param validateValues                 true if values should be validated, otherwise only keys are validated
      * @param ignorePrivateSettings          true if private settings should be ignored during validation
      * @param ignoreArchivedSettings         true if archived settings should be ignored during validation
      * @param validateInternalOrPrivateIndex true if index internal settings should be validated
      * @see Setting#getSettingsDependencies(String)
      */
     public final void validate(
             final Settings settings,
-            final boolean validateDependencies,
+            final boolean validateValues,
             final boolean ignorePrivateSettings,
             final boolean ignoreArchivedSettings,
             final boolean validateInternalOrPrivateIndex) {
@@ -475,7 +475,7 @@ public final void validate(
                 continue;
             }
             try {
-                validate(key, settings, validateDependencies, validateInternalOrPrivateIndex);
+                validate(key, settings, validateValues, validateInternalOrPrivateIndex);
             } catch (final RuntimeException ex) {
                 exceptions.add(ex);
             }
@@ -488,24 +488,24 @@ public final void validate(
      *
      * @param key the key of the setting to validate
      * @param settings the settings
-     * @param validateDependencies true if dependent settings should be validated
+     * @param validateValue true if value should be validated, otherwise only keys are validated
      * @throws IllegalArgumentException if the setting is invalid
      */
-    void validate(final String key, final Settings settings, final boolean validateDependencies) {
-        validate(key, settings, validateDependencies, false);
+    void validate(final String key, final Settings settings, final boolean validateValue) {
+        validate(key, settings, validateValue, false);
     }
 
     /**
      * Validates that the settings is valid.
      *
      * @param key                            the key of the setting to validate
      * @param settings                       the settings
-     * @param validateDependencies           true if dependent settings should be validated
+     * @param validateValue                  true if value should be validated, otherwise only keys are validated
      * @param validateInternalOrPrivateIndex true if internal index settings should be validated
      * @throws IllegalArgumentException if the setting is invalid
      */
     void validate(
-            final String key, final Settings settings, final boolean validateDependencies, final boolean validateInternalOrPrivateIndex) {
+            final String key, final Settings settings, final boolean validateValue, final boolean validateInternalOrPrivateIndex) {
         Setting setting = getRaw(key);
         if (setting == null) {
             LevenshteinDistance ld = new LevenshteinDistance();
@@ -536,7 +536,7 @@ void validate(
             if (setting.hasComplexMatcher()) {
                 setting = setting.getConcreteSetting(key);
             }
-            if (validateDependencies && settingsDependencies.isEmpty() == false) {
+            if (validateValue && settingsDependencies.isEmpty() == false) {
                 for (final Setting.SettingDependency settingDependency : settingsDependencies) {
                     final Setting<?> dependency = settingDependency.getSetting();
                     // validate the dependent setting is set
@@ -563,7 +563,9 @@ void validate(
                 }
             }
         }
-        setting.get(settings);
+        if (validateValue) {
+            setting.get(settings);
+        }
     }
 
     /**

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

@@ -26,9 +26,11 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.Iterator;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
@@ -259,6 +261,59 @@ public void testDependentSettingsWithFallback() {
         service.validate(Settings.builder().put("fallback.test.name", "test").put("foo.test.bar", 7).build(), true);
     }
 
+    public void testValidateValue() {
+        final boolean nodeSetting = randomBoolean();
+        final String prefix = nodeSetting ? "" : "index.";
+        final Property scopeProperty = nodeSetting ? Property.NodeScope : Property.IndexScope;
+        final Setting.Validator<String> baseValidator = s -> {
+            if (s.length() > 1) {
+                throw new IllegalArgumentException("too long");
+            }
+        };
+        final Setting<String> baseSetting = Setting.simpleString(prefix + "foo.base", baseValidator,
+            Property.Dynamic, scopeProperty);
+        final Setting.Validator<String> dependingValidator = new Setting.Validator<String>() {
+            @Override
+            public void validate(String value) {
+            }
+
+            @Override
+            public void validate(String value, Map<Setting<?>, Object> settings, boolean isPresent) {
+                if (Objects.equals(value, settings.get(baseSetting)) == false) {
+                    throw new IllegalArgumentException("must have same value");
+                }
+            }
+
+            @Override
+            public Iterator<Setting<?>> settings() {
+                return org.elasticsearch.common.collect.List.<Setting<?>>of(baseSetting).iterator();
+            }
+        };
+        final Setting<String> dependingSetting = Setting.simpleString(prefix + "foo.depending", dependingValidator, scopeProperty);
+
+        final AbstractScopedSettings service = nodeSetting ?
+            new ClusterSettings(Settings.EMPTY, org.elasticsearch.common.collect.Set.of(baseSetting, dependingSetting)) :
+            new IndexScopedSettings(Settings.EMPTY, org.elasticsearch.common.collect.Set.of(baseSetting, dependingSetting));
+
+        service.validate(Settings.builder().put(baseSetting.getKey(), "1").put(dependingSetting.getKey(), 1).build(), true);
+        service.validate(Settings.builder().put(dependingSetting.getKey(), "1").build(), false);
+        final IllegalArgumentException e = expectThrows(
+            IllegalArgumentException.class,
+            () -> service.validate(Settings.builder().put(dependingSetting.getKey(), "1").build(), true));
+        assertThat(e.getMessage(), equalTo("must have same value"));
+
+        final IllegalArgumentException e2 = expectThrows(
+            IllegalArgumentException.class,
+            () -> service.validate(Settings.builder().put(baseSetting.getKey(), "2").put(dependingSetting.getKey(), "1").build(), true));
+        assertThat(e2.getMessage(), equalTo("must have same value"));
+
+        service.validate(Settings.builder().put(baseSetting.getKey(), "22").build(), false);
+        final IllegalArgumentException e3 = expectThrows(
+            IllegalArgumentException.class,
+            () -> service.validate(Settings.builder().put(baseSetting.getKey(), "22").build(), true));
+        assertThat(e3.getMessage(), equalTo("too long"));
+    }
+
     public void testTupleAffixUpdateConsumer() {
         String prefix = randomAlphaOfLength(3) + "foo.";
         String intSuffix = randomAlphaOfLength(3);
@@ -850,16 +905,16 @@ public void testValidate() {
         assertEquals("unknown setting [i.am.not.a.setting]" + unknownMsgSuffix, e.getMessage());
 
         e = expectThrows(IllegalArgumentException.class, () ->
-            settings.validate(Settings.builder().put("index.store.type", "boom").put("index.number_of_replicas", true).build(), false));
+            settings.validate(Settings.builder().put("index.store.type", "boom").put("index.number_of_replicas", true).build(), true));
         assertEquals("Failed to parse value [true] for setting [index.number_of_replicas]", e.getMessage());
 
         e = expectThrows(IllegalArgumentException.class, () ->
-            settings.validate("index.number_of_replicas", Settings.builder().put("index.number_of_replicas", "true").build(), false));
+            settings.validate("index.number_of_replicas", Settings.builder().put("index.number_of_replicas", "true").build(), true));
         assertEquals("Failed to parse value [true] for setting [index.number_of_replicas]", e.getMessage());
 
         e = expectThrows(IllegalArgumentException.class, () ->
             settings.validate("index.similarity.classic.type", Settings.builder().put("index.similarity.classic.type", "mine").build(),
-                false));
+                true));
         assertEquals("illegal value for [index.similarity.classic] cannot redefine built-in similarity", e.getMessage());
     }
 
@@ -967,7 +1022,7 @@ public void testLoggingUpdates() {
             IllegalArgumentException ex =
                 expectThrows(
                     IllegalArgumentException.class,
-                    () -> settings.validate(Settings.builder().put("logger._root", "boom").build(), false));
+                    () -> settings.validate(Settings.builder().put("logger._root", "boom").build(), true));
             assertEquals("Unknown level constant [BOOM].", ex.getMessage());
             assertEquals(level, LogManager.getRootLogger().getLevel());
             settings.applySettings(Settings.builder().put("logger._root", "TRACE").build());

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/pki/PkiRealmTests.java

@@ -464,7 +464,7 @@ public void testPKIRealmSettingsPassValidation() throws Exception {
         List<Setting<?>> settingList = new ArrayList<>();
         settingList.addAll(InternalRealmsSettings.getSettings());
         ClusterSettings clusterSettings = new ClusterSettings(settings, new HashSet<>(settingList));
-        clusterSettings.validate(settings, false);
+        clusterSettings.validate(settings, true);
 
         assertSettingDeprecationsAndWarnings(new Setting[]{
                 PkiRealmSettings.LEGACY_TRUST_STORE_PASSWORD.getConcreteSettingForNamespace("pki1")