From 4fb1c4fe5ad7017d60b2b6aae08f55f407805386 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Mon, 27 Jun 2016 17:18:26 +0200 Subject: [PATCH] Validate settings against dynamic updaters on the master (#19088) Today all settings are only validated against their validators that are available when settings are registered. Yet, some settings updaters have validators that are dynamic ie. their validation depends on other variables that are only available at runtime. We do not run those validators when settings are updated causing index updates to fail on the data nodes instead of on the master. Relates to #19046 --- .../cluster/settings/SettingsUpdater.java | 2 +- .../MetaDataUpdateSettingsService.java | 27 ++++++++---- .../settings/AbstractScopedSettings.java | 10 ++--- .../org/elasticsearch/index/IndexModule.java | 11 +++++ .../elasticsearch/index/IndexSettings.java | 3 +- .../elasticsearch/indices/IndicesService.java | 3 +- .../snapshots/RestoreService.java | 3 +- .../common/settings/ScopedSettingsTests.java | 4 +- .../indices/cluster/ClusterStateChanges.java | 3 +- .../indices/settings/UpdateSettingsIT.java | 42 +++++++++++++++++++ 10 files changed, 86 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java b/core/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java index 652401194bb..575fbcd3b98 100644 --- a/core/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java +++ b/core/src/main/java/org/elasticsearch/action/admin/cluster/settings/SettingsUpdater.java @@ -77,7 +77,7 @@ final class SettingsUpdater { Settings settings = build.metaData().settings(); // now we try to apply things and if they are invalid we fail // this dryRun will validate & parse settings but won't actually apply them. - clusterSettings.dryRun(settings); + clusterSettings.validateUpdate(settings); return build; } diff --git a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java index 3daf6c4dd3a..54f6ad0705a 100644 --- a/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java +++ b/core/src/main/java/org/elasticsearch/cluster/metadata/MetaDataUpdateSettingsService.java @@ -19,6 +19,7 @@ package org.elasticsearch.cluster.metadata; +import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.Version; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.admin.indices.settings.put.UpdateSettingsClusterStateUpdateRequest; @@ -43,7 +44,10 @@ import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.index.Index; +import org.elasticsearch.index.NodeServicesProvider; +import org.elasticsearch.indices.IndicesService; +import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; @@ -61,17 +65,20 @@ public class MetaDataUpdateSettingsService extends AbstractComponent implements private final AllocationService allocationService; - private final IndexNameExpressionResolver indexNameExpressionResolver; private final IndexScopedSettings indexScopedSettings; + private final IndicesService indicesService; + private final NodeServicesProvider nodeServiceProvider; @Inject - public MetaDataUpdateSettingsService(Settings settings, ClusterService clusterService, AllocationService allocationService, IndexScopedSettings indexScopedSettings, IndexNameExpressionResolver indexNameExpressionResolver) { + public MetaDataUpdateSettingsService(Settings settings, ClusterService clusterService, AllocationService allocationService, + IndexScopedSettings indexScopedSettings, IndicesService indicesService, NodeServicesProvider nodeServicesProvider) { super(settings); this.clusterService = clusterService; - this.indexNameExpressionResolver = indexNameExpressionResolver; this.clusterService.add(this); this.allocationService = allocationService; this.indexScopedSettings = indexScopedSettings; + this.indicesService = indicesService; + this.nodeServiceProvider = nodeServicesProvider; } @Override @@ -266,11 +273,15 @@ public class MetaDataUpdateSettingsService extends AbstractComponent implements // now, reroute in case things change that require it (like number of replicas) RoutingAllocation.Result routingResult = allocationService.reroute(updatedState, "settings update"); updatedState = ClusterState.builder(updatedState).routingResult(routingResult).build(); - for (Index index : openIndices) { - indexScopedSettings.dryRun(updatedState.metaData().getIndexSafe(index).getSettings()); - } - for (Index index : closeIndices) { - indexScopedSettings.dryRun(updatedState.metaData().getIndexSafe(index).getSettings()); + try { + for (Index index : openIndices) { + indicesService.verifyIndexMetadata(nodeServiceProvider, updatedState.getMetaData().getIndexSafe(index)); + } + for (Index index : closeIndices) { + indicesService.verifyIndexMetadata(nodeServiceProvider, updatedState.getMetaData().getIndexSafe(index)); + } + } catch (IOException ex) { + ExceptionsHelper.convertToElastic(ex); } return updatedState; } diff --git a/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java b/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java index 817e109bf4d..efa4c1316ac 100644 --- a/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java +++ b/core/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java @@ -115,18 +115,18 @@ public abstract class AbstractScopedSettings extends AbstractComponent { } /** - * Applies the given settings to all listeners and rolls back the result after application. This + * Validates the given settings by running it through all update listeners without applying it. This * method will not change any settings but will fail if any of the settings can't be applied. */ - public synchronized Settings dryRun(Settings settings) { + public synchronized Settings validateUpdate(Settings settings) { final Settings current = Settings.builder().put(this.settings).put(settings).build(); final Settings previous = Settings.builder().put(this.settings).put(this.lastSettingsApplied).build(); List exceptions = new ArrayList<>(); for (SettingUpdater settingUpdater : settingUpdaters) { try { - if (settingUpdater.hasChanged(current, previous)) { - settingUpdater.getValue(current, previous); - } + // ensure running this through the updater / dynamic validator + // don't check if the value has changed we wanna test this anyways + settingUpdater.getValue(current, previous); } catch (RuntimeException ex) { exceptions.add(ex); logger.debug("failed to prepareCommit settings for [{}]", ex, settingUpdater); diff --git a/core/src/main/java/org/elasticsearch/index/IndexModule.java b/core/src/main/java/org/elasticsearch/index/IndexModule.java index d233faf4f19..f6227ca3276 100644 --- a/core/src/main/java/org/elasticsearch/index/IndexModule.java +++ b/core/src/main/java/org/elasticsearch/index/IndexModule.java @@ -126,6 +126,17 @@ public final class IndexModule { indexSettings.getScopedSettings().addSettingsUpdateConsumer(setting, consumer); } + /** + * Adds a Setting, it's consumer and validator for this index. + */ + public void addSettingsUpdateConsumer(Setting setting, Consumer consumer, Consumer validator) { + ensureNotFrozen(); + if (setting == null) { + throw new IllegalArgumentException("setting must not be null"); + } + indexSettings.getScopedSettings().addSettingsUpdateConsumer(setting, consumer, validator); + } + /** * Returns the index {@link Settings} for this index */ diff --git a/core/src/main/java/org/elasticsearch/index/IndexSettings.java b/core/src/main/java/org/elasticsearch/index/IndexSettings.java index 2c20697d757..df348a5d6a1 100644 --- a/core/src/main/java/org/elasticsearch/index/IndexSettings.java +++ b/core/src/main/java/org/elasticsearch/index/IndexSettings.java @@ -275,6 +275,7 @@ public final class IndexSettings { scopedSettings.addSettingsUpdateConsumer(INDEX_REFRESH_INTERVAL_SETTING, this::setRefreshInterval); scopedSettings.addSettingsUpdateConsumer(MAX_REFRESH_LISTENERS_PER_SHARD, this::setMaxRefreshListeners); scopedSettings.addSettingsUpdateConsumer(MAX_SLICES_PER_SCROLL, this::setMaxSlicesPerScroll); + } private void setTranslogFlushThresholdSize(ByteSizeValue byteSizeValue) { @@ -545,5 +546,5 @@ public final class IndexSettings { this.maxSlicesPerScroll = value; } - IndexScopedSettings getScopedSettings() { return scopedSettings;} + public IndexScopedSettings getScopedSettings() { return scopedSettings;} } diff --git a/core/src/main/java/org/elasticsearch/indices/IndicesService.java b/core/src/main/java/org/elasticsearch/indices/IndicesService.java index edd5e5d8e4b..89cfad98c4f 100644 --- a/core/src/main/java/org/elasticsearch/indices/IndicesService.java +++ b/core/src/main/java/org/elasticsearch/indices/IndicesService.java @@ -425,12 +425,13 @@ public class IndicesService extends AbstractLifecycleComponent // this will also fail if some plugin fails etc. which is nice since we can verify that early final IndexService service = createIndexService("metadata verification", nodeServicesProvider, metaData, indicesQueryCache, indicesFieldDataCache, Collections.emptyList()); + closeables.add(() -> service.close("metadata verification", false)); for (ObjectCursor typeMapping : metaData.getMappings().values()) { // don't apply the default mapping, it has been applied when the mapping was created service.mapperService().merge(typeMapping.value.type(), typeMapping.value.source(), MapperService.MergeReason.MAPPING_RECOVERY, true); } - closeables.add(() -> service.close("metadata verification", false)); + service.getIndexSettings().getScopedSettings().validateUpdate(metaData.getSettings()); } finally { IOUtils.close(closeables); } diff --git a/core/src/main/java/org/elasticsearch/snapshots/RestoreService.java b/core/src/main/java/org/elasticsearch/snapshots/RestoreService.java index 44ca6fb972e..74434413ae9 100644 --- a/core/src/main/java/org/elasticsearch/snapshots/RestoreService.java +++ b/core/src/main/java/org/elasticsearch/snapshots/RestoreService.java @@ -39,7 +39,6 @@ import org.elasticsearch.cluster.metadata.MetaData; import org.elasticsearch.cluster.metadata.MetaDataCreateIndexService; import org.elasticsearch.cluster.metadata.MetaDataIndexUpgradeService; import org.elasticsearch.cluster.metadata.RepositoriesMetaData; -import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.routing.IndexRoutingTable; import org.elasticsearch.cluster.routing.IndexShardRoutingTable; import org.elasticsearch.cluster.routing.RestoreSource; @@ -436,7 +435,7 @@ public class RestoreService extends AbstractComponent implements ClusterStateLis if (request.includeGlobalState()) { if (metaData.persistentSettings() != null) { Settings settings = metaData.persistentSettings(); - clusterSettings.dryRun(settings); + clusterSettings.validateUpdate(settings); mdBuilder.persistentSettings(settings); } if (metaData.templates() != null) { diff --git a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java index 664f8cb96ab..dee20d6b32e 100644 --- a/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java +++ b/core/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java @@ -98,7 +98,7 @@ public class ScopedSettingsTests extends ESTestCase { assertEquals(0, aC.get()); assertEquals(0, bC.get()); try { - service.dryRun(Settings.builder().put("foo.bar", 2).put("foo.bar.baz", -15).build()); + service.validateUpdate(Settings.builder().put("foo.bar", 2).put("foo.bar.baz", -15).build()); fail("invalid value"); } catch (IllegalArgumentException ex) { assertEquals("illegal value can't update [foo.bar.baz] from [1] to [-15]", ex.getMessage()); @@ -108,7 +108,7 @@ public class ScopedSettingsTests extends ESTestCase { assertEquals(0, consumer2.get()); assertEquals(0, aC.get()); assertEquals(0, bC.get()); - service.dryRun(Settings.builder().put("foo.bar", 2).put("foo.bar.baz", 15).build()); + service.validateUpdate(Settings.builder().put("foo.bar", 2).put("foo.bar.baz", 15).build()); assertEquals(0, consumer.get()); assertEquals(0, consumer2.get()); assertEquals(0, aC.get()); diff --git a/core/src/test/java/org/elasticsearch/indices/cluster/ClusterStateChanges.java b/core/src/test/java/org/elasticsearch/indices/cluster/ClusterStateChanges.java index fc5b68f87d1..09441f70110 100644 --- a/core/src/test/java/org/elasticsearch/indices/cluster/ClusterStateChanges.java +++ b/core/src/test/java/org/elasticsearch/indices/cluster/ClusterStateChanges.java @@ -19,7 +19,6 @@ package org.elasticsearch.indices.cluster; -import org.elasticsearch.Version; import org.elasticsearch.action.ActionResponse; import org.elasticsearch.action.admin.cluster.reroute.ClusterRerouteRequest; import org.elasticsearch.action.admin.cluster.reroute.TransportClusterRerouteAction; @@ -156,7 +155,7 @@ public class ClusterStateChanges { metaDataIndexUpgradeService, nodeServicesProvider, indicesService); MetaDataDeleteIndexService deleteIndexService = new MetaDataDeleteIndexService(settings, clusterService, allocationService); MetaDataUpdateSettingsService metaDataUpdateSettingsService = new MetaDataUpdateSettingsService(settings, clusterService, - allocationService, IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, new IndexNameExpressionResolver(settings)); + allocationService, IndexScopedSettings.DEFAULT_SCOPED_SETTINGS, indicesService, nodeServicesProvider); MetaDataCreateIndexService createIndexService = new MetaDataCreateIndexService(settings, clusterService, indicesService, allocationService, new AliasValidator(settings), Collections.emptySet(), environment, nodeServicesProvider, IndexScopedSettings.DEFAULT_SCOPED_SETTINGS); diff --git a/core/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java b/core/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java index e7e3cb32226..2a67742fc68 100644 --- a/core/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java +++ b/core/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java @@ -29,7 +29,9 @@ import org.elasticsearch.action.admin.cluster.node.stats.NodesStatsResponse; import org.elasticsearch.action.admin.indices.settings.get.GetSettingsResponse; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.Priority; +import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.index.IndexModule; import org.elasticsearch.index.IndexService; import org.elasticsearch.index.engine.VersionConflictEngineException; import org.elasticsearch.index.MergePolicyConfig; @@ -37,9 +39,13 @@ import org.elasticsearch.index.MergeSchedulerConfig; import org.elasticsearch.index.store.IndexStore; import org.elasticsearch.index.store.Store; import org.elasticsearch.indices.IndicesService; +import org.elasticsearch.plugins.Plugin; import org.elasticsearch.test.ESIntegTestCase; import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.List; import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_BLOCKS_METADATA; import static org.elasticsearch.cluster.metadata.IndexMetaData.SETTING_BLOCKS_READ; @@ -53,6 +59,42 @@ import static org.hamcrest.Matchers.nullValue; public class UpdateSettingsIT extends ESIntegTestCase { + + public void testInvalidDynamicUpdate() { + createIndex("test"); + IllegalArgumentException exception = expectThrows(IllegalArgumentException.class, () -> + client().admin().indices().prepareUpdateSettings("test") + .setSettings(Settings.builder() + .put("index.dummy", "boom") + ) + .execute().actionGet()); + assertEquals(exception.getCause().getMessage(), "this setting goes boom"); + IndexMetaData indexMetaData = client().admin().cluster().prepareState().execute().actionGet().getState().metaData().index("test"); + assertNotEquals(indexMetaData.getSettings().get("index.dummy"), "invalid dynamic value"); + } + + @Override + protected Collection> nodePlugins() { + return pluginList(DummySettingPlugin.class); + } + + public static class DummySettingPlugin extends Plugin { + public static final Setting DUMMY_SETTING = Setting.simpleString("index.dummy", + Setting.Property.IndexScope, Setting.Property.Dynamic); + @Override + public void onIndexModule(IndexModule indexModule) { + indexModule.addSettingsUpdateConsumer(DUMMY_SETTING, (s) -> {}, (s) -> { + if (s.equals("boom")) + throw new IllegalArgumentException("this setting goes boom"); + }); + } + + @Override + public List> getSettings() { + return Collections.singletonList(DUMMY_SETTING); + } + } + public void testResetDefault() { createIndex("test");