From bfb2dc13539ec27f6305744248e9718521e08efa Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Mon, 9 Dec 2019 12:39:26 -0500 Subject: [PATCH] Enable dependent settings values to be validated (#49942) Today settings can declare dependencies on another setting. This declaration is implemented so that if the declared setting is not set when the declaring setting is, settings validation fails. Yet, in some cases we want not only that the setting is set, but that it also has a specific value. For example, with the monitoring exporter settings, if xpack.monitoring.exporters.my_exporter.host is set, we not only want that xpack.monitoring.exporters.my_exporter.type is set, but that it is also set to local. This commit extends the settings infrastructure so that this declaration is possible. The use of this in the monitoring exporter settings will be implemented in a follow-up. --- .../azure/AzureStorageSettings.java | 21 +++--- .../settings/AbstractScopedSettings.java | 12 ++-- .../common/settings/Setting.java | 65 ++++++++++++++++--- .../transport/RemoteClusterService.java | 8 +-- .../transport/SniffConnectionStrategy.java | 4 +- .../common/settings/ScopedSettingsTests.java | 55 ++++++++++++++-- .../indices/settings/UpdateSettingsIT.java | 4 +- .../core/security/authc/RealmSettings.java | 4 +- 8 files changed, 139 insertions(+), 34 deletions(-) diff --git a/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageSettings.java b/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageSettings.java index 5d1e8d57ae2..278048a987d 100644 --- a/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageSettings.java +++ b/plugins/repository-azure/src/main/java/org/elasticsearch/repositories/azure/AzureStorageSettings.java @@ -62,29 +62,34 @@ final class AzureStorageSettings { public static final AffixSetting MAX_RETRIES_SETTING = Setting.affixKeySetting(AZURE_CLIENT_PREFIX_KEY, "max_retries", (key) -> Setting.intSetting(key, RetryPolicy.DEFAULT_CLIENT_RETRY_COUNT, Setting.Property.NodeScope), - ACCOUNT_SETTING, KEY_SETTING); + () -> ACCOUNT_SETTING, () -> KEY_SETTING); /** * Azure endpoint suffix. Default to core.windows.net (CloudStorageAccount.DEFAULT_DNS). */ public static final AffixSetting ENDPOINT_SUFFIX_SETTING = Setting.affixKeySetting(AZURE_CLIENT_PREFIX_KEY, "endpoint_suffix", - key -> Setting.simpleString(key, Property.NodeScope), ACCOUNT_SETTING, KEY_SETTING); + key -> Setting.simpleString(key, Property.NodeScope), () -> ACCOUNT_SETTING, () -> KEY_SETTING); public static final AffixSetting TIMEOUT_SETTING = Setting.affixKeySetting(AZURE_CLIENT_PREFIX_KEY, "timeout", - (key) -> Setting.timeSetting(key, TimeValue.timeValueMinutes(-1), Property.NodeScope), ACCOUNT_SETTING, KEY_SETTING); + (key) -> Setting.timeSetting(key, TimeValue.timeValueMinutes(-1), Property.NodeScope), () -> ACCOUNT_SETTING, () -> KEY_SETTING); /** The type of the proxy to connect to azure through. Can be direct (no proxy, default), http or socks */ public static final AffixSetting PROXY_TYPE_SETTING = Setting.affixKeySetting(AZURE_CLIENT_PREFIX_KEY, "proxy.type", (key) -> new Setting<>(key, "direct", s -> Proxy.Type.valueOf(s.toUpperCase(Locale.ROOT)), Property.NodeScope) - , ACCOUNT_SETTING, KEY_SETTING); + , () -> ACCOUNT_SETTING, () -> KEY_SETTING); /** The host name of a proxy to connect to azure through. */ public static final AffixSetting PROXY_HOST_SETTING = Setting.affixKeySetting(AZURE_CLIENT_PREFIX_KEY, "proxy.host", - (key) -> Setting.simpleString(key, Property.NodeScope), KEY_SETTING, ACCOUNT_SETTING, PROXY_TYPE_SETTING); + (key) -> Setting.simpleString(key, Property.NodeScope), () -> KEY_SETTING, () -> ACCOUNT_SETTING, () -> PROXY_TYPE_SETTING); /** The port of a proxy to connect to azure through. */ - public static final Setting PROXY_PORT_SETTING = Setting.affixKeySetting(AZURE_CLIENT_PREFIX_KEY, "proxy.port", - (key) -> Setting.intSetting(key, 0, 0, 65535, Setting.Property.NodeScope), ACCOUNT_SETTING, KEY_SETTING, PROXY_TYPE_SETTING, - PROXY_HOST_SETTING); + public static final Setting PROXY_PORT_SETTING = Setting.affixKeySetting( + AZURE_CLIENT_PREFIX_KEY, + "proxy.port", + (key) -> Setting.intSetting(key, 0, 0, 65535, Setting.Property.NodeScope), + () -> ACCOUNT_SETTING, + () -> KEY_SETTING, + () -> PROXY_TYPE_SETTING, + () -> PROXY_HOST_SETTING); private final String account; private final String connectString; 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 9db52b9eb93..9a329daf049 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -529,20 +529,24 @@ public abstract class AbstractScopedSettings { } throw new IllegalArgumentException(msg); } else { - Set> settingsDependencies = setting.getSettingsDependencies(key); + Set settingsDependencies = setting.getSettingsDependencies(key); if (setting.hasComplexMatcher()) { setting = setting.getConcreteSetting(key); } if (validateDependencies && settingsDependencies.isEmpty() == false) { - for (final Setting settingDependency : settingsDependencies) { - if (settingDependency.existsOrFallbackExists(settings) == false) { + for (final Setting.SettingDependency settingDependency : settingsDependencies) { + final Setting dependency = settingDependency.getSetting(); + // validate the dependent setting is set + if (dependency.existsOrFallbackExists(settings) == false) { final String message = String.format( Locale.ROOT, "missing required setting [%s] for setting [%s]", - settingDependency.getKey(), + dependency.getKey(), setting.getKey()); throw new IllegalArgumentException(message); } + // validate the dependent setting value + settingDependency.validate(setting.getKey(), setting.get(settings), dependency.get(settings)); } } // the only time that validateInternalOrPrivateIndex should be true is if this call is coming via the update settings API diff --git a/server/src/main/java/org/elasticsearch/common/settings/Setting.java b/server/src/main/java/org/elasticsearch/common/settings/Setting.java index d1bad329bc9..92930a28df1 100644 --- a/server/src/main/java/org/elasticsearch/common/settings/Setting.java +++ b/server/src/main/java/org/elasticsearch/common/settings/Setting.java @@ -563,11 +563,37 @@ public class Setting implements ToXContentObject { return this; } + /** + * Allows a setting to declare a dependency on another setting being set. Optionally, a setting can validate the value of the dependent + * setting. + */ + public interface SettingDependency { + + /** + * The setting to declare a dependency on. + * + * @return the setting + */ + Setting getSetting(); + + /** + * Validates the dependent setting value. + * + * @param key the key for this setting + * @param value the value of this setting + * @param dependency the value of the dependent setting + */ + default void validate(String key, Object value, Object dependency) { + + } + + } + /** * 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> getSettingsDependencies(String key) { + public Set getSettingsDependencies(final String key) { return Collections.emptySet(); } @@ -670,13 +696,23 @@ public class Setting implements ToXContentObject { }; } + /** + * Allows an affix setting to declare a dependency on another affix setting. + */ + public interface AffixSettingDependency extends SettingDependency { + + @Override + AffixSetting getSetting(); + + } + public static class AffixSetting extends Setting { private final AffixKey key; private final BiFunction> delegateFactory; - private final Set dependencies; + private final Set dependencies; public AffixSetting(AffixKey key, Setting delegate, BiFunction> delegateFactory, - AffixSetting... dependencies) { + AffixSettingDependency... dependencies) { super(key, delegate.defaultValue, delegate.parser, delegate.properties.toArray(new Property[0])); this.key = key; this.delegateFactory = delegateFactory; @@ -692,12 +728,25 @@ public class Setting implements ToXContentObject { } @Override - public Set> getSettingsDependencies(String settingsKey) { + public Set getSettingsDependencies(String settingsKey) { if (dependencies.isEmpty()) { return Collections.emptySet(); } else { String namespace = key.getNamespace(settingsKey); - return dependencies.stream().map(s -> (Setting)s.getConcreteSettingForNamespace(namespace)).collect(Collectors.toSet()); + return dependencies.stream() + .map(s -> + new SettingDependency() { + @Override + public Setting getSetting() { + return s.getSetting().getConcreteSettingForNamespace(namespace); + } + + @Override + public void validate(final String key, final Object value, final Object dependency) { + s.validate(key, value, dependency); + }; + }) + .collect(Collectors.toSet()); } } @@ -1638,19 +1687,19 @@ public class Setting implements ToXContentObject { * out of the box unless {@link #getConcreteSetting(String)} is used to pull the updater. */ public static AffixSetting affixKeySetting(String prefix, String suffix, Function> delegateFactory, - AffixSetting... dependencies) { + AffixSettingDependency... dependencies) { BiFunction> delegateFactoryWithNamespace = (ns, k) -> delegateFactory.apply(k); return affixKeySetting(new AffixKey(prefix, suffix), delegateFactoryWithNamespace, dependencies); } public static AffixSetting affixKeySetting(String prefix, String suffix, BiFunction> delegateFactory, - AffixSetting... dependencies) { + AffixSettingDependency... dependencies) { Setting delegate = delegateFactory.apply("_na_", "_na_"); return new AffixSetting<>(new AffixKey(prefix, suffix), delegate, delegateFactory, dependencies); } private static AffixSetting affixKeySetting(AffixKey key, BiFunction> delegateFactory, - AffixSetting... dependencies) { + AffixSettingDependency... dependencies) { Setting delegate = delegateFactory.apply("_na_", "_na_"); return new AffixSetting<>(key, delegate, delegateFactory, dependencies); } diff --git a/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java b/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java index fc34dab5814..9198f406fa4 100644 --- a/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java +++ b/server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java @@ -121,7 +121,7 @@ public final class RemoteClusterService extends RemoteClusterAware implements Cl "search.remote.", "skip_unavailable", key -> boolSetting(key, false, Setting.Property.Deprecated, Setting.Property.Dynamic, Setting.Property.NodeScope), - SniffConnectionStrategy.REMOTE_CLUSTER_SEEDS); + () -> SniffConnectionStrategy.REMOTE_CLUSTER_SEEDS); public static final SettingUpgrader SEARCH_REMOTE_CLUSTER_SKIP_UNAVAILABLE_UPGRADER = new SettingUpgrader() { @@ -149,19 +149,19 @@ public final class RemoteClusterService extends RemoteClusterAware implements Cl : SEARCH_REMOTE_CLUSTER_SKIP_UNAVAILABLE.getConcreteSetting(key.replaceAll("^cluster", "search")), Setting.Property.Dynamic, Setting.Property.NodeScope), - SniffConnectionStrategy.REMOTE_CLUSTER_SEEDS); + () -> SniffConnectionStrategy.REMOTE_CLUSTER_SEEDS); public static final Setting.AffixSetting REMOTE_CLUSTER_PING_SCHEDULE = Setting.affixKeySetting( "cluster.remote.", "transport.ping_schedule", key -> timeSetting(key, TransportSettings.PING_SCHEDULE, Setting.Property.Dynamic, Setting.Property.NodeScope), - SniffConnectionStrategy.REMOTE_CLUSTER_SEEDS); + () -> SniffConnectionStrategy.REMOTE_CLUSTER_SEEDS); public static final Setting.AffixSetting REMOTE_CLUSTER_COMPRESS = Setting.affixKeySetting( "cluster.remote.", "transport.compress", key -> boolSetting(key, TransportSettings.TRANSPORT_COMPRESS, Setting.Property.Dynamic, Setting.Property.NodeScope), - SniffConnectionStrategy.REMOTE_CLUSTER_SEEDS); + () -> SniffConnectionStrategy.REMOTE_CLUSTER_SEEDS); private final TransportService transportService; private final Map remoteClusters = ConcurrentCollections.newConcurrentMap(); diff --git a/server/src/main/java/org/elasticsearch/transport/SniffConnectionStrategy.java b/server/src/main/java/org/elasticsearch/transport/SniffConnectionStrategy.java index 6a0df95cc2e..efd44862f57 100644 --- a/server/src/main/java/org/elasticsearch/transport/SniffConnectionStrategy.java +++ b/server/src/main/java/org/elasticsearch/transport/SniffConnectionStrategy.java @@ -147,7 +147,7 @@ public class SniffConnectionStrategy extends RemoteConnectionStrategy { Setting.Property.Deprecated, Setting.Property.Dynamic, Setting.Property.NodeScope), - REMOTE_CLUSTER_SEEDS); + () -> REMOTE_CLUSTER_SEEDS); /** * A proxy address for the remote cluster. By default this is not set, meaning that Elasticsearch will connect directly to the nodes in @@ -168,7 +168,7 @@ public class SniffConnectionStrategy extends RemoteConnectionStrategy { SEARCH_REMOTE_CLUSTERS_PROXY.getConcreteSettingForNamespace(ns), Setting.Property.Dynamic, Setting.Property.NodeScope), - REMOTE_CLUSTER_SEEDS); + () -> REMOTE_CLUSTER_SEEDS); public static final Setting SEARCH_REMOTE_CONNECTIONS_PER_CLUSTER = intSetting("search.remote.connections_per_cluster", 3, 1, Setting.Property.NodeScope, Setting.Property.Deprecated); 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 efb46db59de..39e178aaca1 100644 --- a/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -179,9 +179,9 @@ public class ScopedSettingsTests extends ESTestCase { Setting.AffixSetting stringSetting = Setting.affixKeySetting("foo.", "name", (k) -> Setting.simpleString(k, Property.Dynamic, Property.NodeScope)); Setting.AffixSetting intSetting = Setting.affixKeySetting("foo.", "bar", - (k) -> Setting.intSetting(k, 1, Property.Dynamic, Property.NodeScope), stringSetting); + (k) -> Setting.intSetting(k, 1, Property.Dynamic, Property.NodeScope), () -> stringSetting); - AbstractScopedSettings service = new ClusterSettings(Settings.EMPTY,new HashSet<>(Arrays.asList(intSetting, stringSetting))); + AbstractScopedSettings service = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(intSetting, stringSetting))); IllegalArgumentException iae = expectThrows(IllegalArgumentException.class, () -> service.validate(Settings.builder().put("foo.test.bar", 7).build(), true)); @@ -195,6 +195,50 @@ public class ScopedSettingsTests extends ESTestCase { service.validate(Settings.builder().put("foo.test.bar", 7).build(), false); } + public void testDependentSettingsValidate() { + Setting.AffixSetting stringSetting = Setting.affixKeySetting( + "foo.", + "name", + (k) -> Setting.simpleString(k, Property.Dynamic, Property.NodeScope)); + Setting.AffixSetting intSetting = Setting.affixKeySetting( + "foo.", + "bar", + (k) -> Setting.intSetting(k, 1, Property.Dynamic, Property.NodeScope), + new Setting.AffixSettingDependency() { + + @Override + public Setting.AffixSetting getSetting() { + return stringSetting; + } + + @Override + public void validate(final String key, final Object value, final Object dependency) { + if ("valid".equals(dependency) == false) { + throw new SettingsException("[" + key + "] is set but [name] is [" + dependency + "]"); + } + } + }); + + AbstractScopedSettings service = new ClusterSettings(Settings.EMPTY, new HashSet<>(Arrays.asList(intSetting, stringSetting))); + + SettingsException iae = expectThrows( + SettingsException.class, + () -> service.validate(Settings.builder().put("foo.test.bar", 7).put("foo.test.name", "invalid").build(), true)); + assertEquals("[foo.test.bar] is set but [name] is [invalid]", iae.getMessage()); + + service.validate(Settings.builder() + .put("foo.test.bar", 7) + .put("foo.test.name", "valid") + .build(), + true); + + service.validate(Settings.builder() + .put("foo.test.bar", 7) + .put("foo.test.name", "invalid") + .build(), + false); + } + public void testDependentSettingsWithFallback() { Setting.AffixSetting nameFallbackSetting = Setting.affixKeySetting("fallback.", "name", k -> Setting.simpleString(k, Property.Dynamic, Property.NodeScope)); @@ -208,8 +252,11 @@ public class ScopedSettingsTests extends ESTestCase { : nameFallbackSetting.getConcreteSetting(k.replaceAll("^foo", "fallback")), Property.Dynamic, Property.NodeScope)); - Setting.AffixSetting barSetting = - Setting.affixKeySetting("foo.", "bar", k -> Setting.intSetting(k, 1, Property.Dynamic, Property.NodeScope), nameSetting); + Setting.AffixSetting 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))); diff --git a/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java b/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java index 066fd96455a..8f43990ad6a 100644 --- a/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java +++ b/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java @@ -93,12 +93,12 @@ public class UpdateSettingsIT extends ESIntegTestCase { public static final Setting.AffixSetting DUMMY_ACCOUNT_USER = Setting.affixKeySetting("index.acc.", "user", k -> Setting.simpleString(k, Setting.Property.IndexScope, Setting.Property.Dynamic)); public static final Setting DUMMY_ACCOUNT_PW = Setting.affixKeySetting("index.acc.", "pw", - k -> Setting.simpleString(k, Setting.Property.IndexScope, Setting.Property.Dynamic), DUMMY_ACCOUNT_USER); + k -> Setting.simpleString(k, Setting.Property.IndexScope, Setting.Property.Dynamic), () -> DUMMY_ACCOUNT_USER); public static final Setting.AffixSetting DUMMY_ACCOUNT_USER_CLUSTER = Setting.affixKeySetting("cluster.acc.", "user", k -> Setting.simpleString(k, Setting.Property.NodeScope, Setting.Property.Dynamic)); public static final Setting DUMMY_ACCOUNT_PW_CLUSTER = Setting.affixKeySetting("cluster.acc.", "pw", - k -> Setting.simpleString(k, Setting.Property.NodeScope, Setting.Property.Dynamic), DUMMY_ACCOUNT_USER_CLUSTER); + k -> Setting.simpleString(k, Setting.Property.NodeScope, Setting.Property.Dynamic), () -> DUMMY_ACCOUNT_USER_CLUSTER); @Override public void onIndexModule(IndexModule indexModule) { diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/RealmSettings.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/RealmSettings.java index fda2cf614c8..8b4b7fa8848 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/RealmSettings.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/RealmSettings.java @@ -20,7 +20,7 @@ import java.util.stream.Collectors; /** * Provides a number of utility methods for interacting with {@link Settings} and {@link Setting} inside a {@link Realm}. - * Settings for realms use an {@link Setting#affixKeySetting(String, String, Function, Setting.AffixSetting[]) affix} style, + * Settings for realms use an {@link Setting#affixKeySetting(String, String, Function, Setting.AffixSettingDependency[]) affix} style, * where the type of the realm is part of the prefix, and name of the realm is the variable portion * (That is to set the order in a file realm named "file1", then full setting key would be * {@code xpack.security.authc.realms.file.file1.order}. @@ -74,7 +74,7 @@ public class RealmSettings { * The {@code Function} takes the realm-type as an argument. * @param suffix The suffix of the setting (everything following the realm name in the affix setting) * @param delegateFactory A factory to produce the concrete setting. - * See {@link Setting#affixKeySetting(String, String, Function, Setting.AffixSetting[])} + * See {@link Setting#affixKeySetting(String, String, Function, Setting.AffixSettingDependency[])} */ public static Function> affixSetting(String suffix, Function> delegateFactory) { return realmType -> Setting.affixKeySetting(realmSettingPrefix(realmType), suffix, delegateFactory);