From 8f8e3075d0987e6aaa22adb06d256bf853dc9edc Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Tue, 9 Mar 2021 12:53:46 +0100 Subject: [PATCH 1/5] Support dependent validation for index settings 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 works with such validators, letting the validator validate syntax (validate(value)) only. Relates #70141. --- .../settings/AbstractScopedSettings.java | 12 ++++-- .../common/settings/SecureSetting.java | 6 +++ .../common/settings/ScopedSettingsTests.java | 42 +++++++++++++++++++ 3 files changed, 56 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index 3958696139170..8e14fc4831d49 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -454,7 +454,7 @@ public final void validate( * Validates that all settings are registered and valid. * * @param settings the settings - * @param validateDependencies true if dependent settings should be validated + * @param validateDependencies true if dependent settings and settings where validation has dependencies should be 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 @@ -489,7 +489,7 @@ 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 validateDependencies true if dependent settings and settings where validation has dependencies should be validated * @throws IllegalArgumentException if the setting is invalid */ void validate(final String key, final Settings settings, final boolean validateDependencies) { @@ -501,7 +501,7 @@ void validate(final String key, final Settings settings, final boolean validateD * * @param key the key of the setting to validate * @param settings the settings - * @param validateDependencies true if dependent settings should be validated + * @param validateDependencies true if dependent settings and settings where validation has dependencies should be validated * @param validateInternalOrPrivateIndex true if internal index settings should be validated * @throws IllegalArgumentException if the setting is invalid */ @@ -564,7 +564,11 @@ void validate( } } } - setting.get(settings); + if (validateDependencies) { + setting.get(settings); + } else { + setting.validateWithoutDependencies(settings); + } } /** diff --git a/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java b/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java index 3447722322d28..bd19aacd9780b 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java @@ -82,6 +82,12 @@ public T get(Settings settings) { } } + @Override + void validateWithoutDependencies(Settings settings) { + // secure settings have no Setting.Validator + get(settings); + } + /** * Returns the digest of this secure setting's value or {@code null} if the setting is missing (inside the keystore). This method can be * called even after the {@code SecureSettings} have been closed, unlike {@code #get(Settings)}. The digest is used to check for changes diff --git a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 84d8ce8855842..1ef3ff2850952 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/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,46 @@ public void testDependentSettingsWithFallback() { service.validate(Settings.builder().put("fallback.test.name", "test").put("foo.test.bar", 7).build(), true); } + public void testValidatorWithDependencies() { + final boolean nodeSetting = randomBoolean(); + final String prefix = nodeSetting ? "" : "index."; + final Property scopeProperty = nodeSetting ? Property.NodeScope : Property.IndexScope; + final Setting baseSetting = Setting.simpleString(prefix + "foo.base", Property.Dynamic, scopeProperty); + final Setting.Validator validator = new Setting.Validator() { + @Override + public void validate(String value) { + } + + @Override + public void validate(String value, Map, Object> settings, boolean isPresent) { + if (Objects.equals(value, settings.get(baseSetting)) == false) { + throw new IllegalArgumentException("must have same value"); + } + } + + @Override + public Iterator> settings() { + return List.>of(baseSetting).iterator(); + } + }; + final Setting dependingSetting = Setting.simpleString(prefix + "foo.depending", validator, scopeProperty); + + final AbstractScopedSettings service = nodeSetting ? new ClusterSettings(Settings.EMPTY, Set.of(baseSetting, dependingSetting)) : + new IndexScopedSettings(Settings.EMPTY, 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")); + } + public void testTupleAffixUpdateConsumer() { String prefix = randomAlphaOfLength(3) + "foo."; String intSuffix = randomAlphaOfLength(3); From b08f641bad6a082bceeb3dcb30ecb514d708accf Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Wed, 10 Mar 2021 10:37:16 +0100 Subject: [PATCH 2/5] Skip value validation Index update settings does prevalidation primarily to ensure all keys are valid. With this change, we skip validating values during prevalidation, the final settings applied to each index are still validated in MetadataUpdateSettingsService. --- .../MetadataUpdateSettingsService.java | 2 +- .../settings/AbstractScopedSettings.java | 34 +++++++++---------- .../common/settings/ScopedSettingsTests.java | 28 ++++++++++----- 3 files changed, 37 insertions(+), 27 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java index 851267e995e1b..c6655b62e574f 100644 --- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java +++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataUpdateSettingsService.java @@ -81,7 +81,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); diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index 8e14fc4831d49..b962e4914ca89 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -414,47 +414,47 @@ public synchronized void addSettingsUpdateConsumer(Setting 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 and settings where validation has dependencies 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 @@ -462,7 +462,7 @@ public final void validate( */ public final void validate( final Settings settings, - final boolean validateDependencies, + final boolean validateValues, final boolean ignorePrivateSettings, final boolean ignoreArchivedSettings, final boolean validateInternalOrPrivateIndex) { @@ -476,7 +476,7 @@ public final void validate( continue; } try { - validate(key, settings, validateDependencies, validateInternalOrPrivateIndex); + validate(key, settings, validateValues, validateInternalOrPrivateIndex); } catch (final RuntimeException ex) { exceptions.add(ex); } @@ -501,12 +501,12 @@ void validate(final String key, final Settings settings, final boolean validateD * * @param key the key of the setting to validate * @param settings the settings - * @param validateDependencies true if dependent settings and settings where validation has dependencies 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(); @@ -537,7 +537,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 @@ -564,10 +564,8 @@ void validate( } } } - if (validateDependencies) { + if (validateValue) { setting.get(settings); - } else { - setting.validateWithoutDependencies(settings); } } diff --git a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 1ef3ff2850952..58258c918b910 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -261,12 +261,18 @@ public void testDependentSettingsWithFallback() { service.validate(Settings.builder().put("fallback.test.name", "test").put("foo.test.bar", 7).build(), true); } - public void testValidatorWithDependencies() { + public void testValidateValue() { final boolean nodeSetting = randomBoolean(); final String prefix = nodeSetting ? "" : "index."; final Property scopeProperty = nodeSetting ? Property.NodeScope : Property.IndexScope; - final Setting baseSetting = Setting.simpleString(prefix + "foo.base", Property.Dynamic, scopeProperty); - final Setting.Validator validator = new Setting.Validator() { + final Setting.Validator baseValidator = s -> { + if (s.length() > 1) { + throw new IllegalArgumentException("too long"); + } + }; + final Setting baseSetting = Setting.simpleString(prefix + "foo.base", baseValidator, + Property.Dynamic, scopeProperty); + final Setting.Validator dependingValidator = new Setting.Validator() { @Override public void validate(String value) { } @@ -283,7 +289,7 @@ public Iterator> settings() { return List.>of(baseSetting).iterator(); } }; - final Setting dependingSetting = Setting.simpleString(prefix + "foo.depending", validator, scopeProperty); + final Setting dependingSetting = Setting.simpleString(prefix + "foo.depending", dependingValidator, scopeProperty); final AbstractScopedSettings service = nodeSetting ? new ClusterSettings(Settings.EMPTY, Set.of(baseSetting, dependingSetting)) : new IndexScopedSettings(Settings.EMPTY, Set.of(baseSetting, dependingSetting)); @@ -299,6 +305,12 @@ public Iterator> settings() { 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() { @@ -892,16 +904,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.boolean.type", Settings.builder().put("index.similarity.boolean.type", "mine").build(), - false)); + true)); assertEquals("illegal value for [index.similarity.boolean] cannot redefine built-in similarity", e.getMessage()); } @@ -1009,7 +1021,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()); From b5bf9b7fa60f0a46c04cb3d57de009279f597914 Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Wed, 10 Mar 2021 11:29:54 +0100 Subject: [PATCH 3/5] Fix test --- .../elasticsearch/xpack/security/authc/pki/PkiRealmTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/pki/PkiRealmTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/pki/PkiRealmTests.java index b8b773ca2ee0d..6784d4358954b 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/pki/PkiRealmTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/pki/PkiRealmTests.java @@ -469,7 +469,7 @@ public void testPKIRealmSettingsPassValidation() throws Exception { List> 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") From 662514eb0cb4a1172c64af15f180c7c3748fa5a0 Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Wed, 10 Mar 2021 12:28:37 +0100 Subject: [PATCH 4/5] Fix variable naming --- .../common/settings/AbstractScopedSettings.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index b962e4914ca89..915e60037eaac 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -489,11 +489,11 @@ public final void validate( * * @param key the key of the setting to validate * @param settings the settings - * @param validateDependencies true if dependent settings and settings where validation has dependencies 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); } /** From 2367f373c50fc3fc892639b68e969f7e40654d80 Mon Sep 17 00:00:00 2001 From: Henning Andersen Date: Wed, 10 Mar 2021 13:04:04 +0100 Subject: [PATCH 5/5] Remove unnecessary change --- .../org/elasticsearch/common/settings/SecureSetting.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java b/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java index bd19aacd9780b..3447722322d28 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/SecureSetting.java @@ -82,12 +82,6 @@ public T get(Settings settings) { } } - @Override - void validateWithoutDependencies(Settings settings) { - // secure settings have no Setting.Validator - get(settings); - } - /** * Returns the digest of this secure setting's value or {@code null} if the setting is missing (inside the keystore). This method can be * called even after the {@code SecureSettings} have been closed, unlike {@code #get(Settings)}. The digest is used to check for changes