From bfe3e3500b33a8bd3ce7fe1c63647361ba792da8 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Fri, 26 Mar 2021 14:18:55 -0400 Subject: [PATCH 01/10] First cut at deprecation warning for changed default --- .../action/support/DestructiveOperations.java | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java index 5a72520f03357..a6dfaf0325762 100644 --- a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java +++ b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java @@ -8,23 +8,44 @@ package org.elasticsearch.action.support; +import org.elasticsearch.common.logging.DeprecationCategory; +import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; import java.util.Arrays; +import java.util.concurrent.atomic.AtomicBoolean; /** * Helper for dealing with destructive operations and wildcard usage. */ public final class DestructiveOperations { + private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(DestructiveOperations.class); + + private static final AtomicBoolean deprecationLogged = new AtomicBoolean(false); + /** * Setting which controls whether wildcard usage (*, prefix*, _all) is allowed. */ public static final Setting REQUIRES_NAME_SETTING = - Setting.boolSetting("action.destructive_requires_name", false, Property.Dynamic, Property.NodeScope); + Setting.boolSetting("action.destructive_requires_name", + (settings) -> { + if (deprecationLogged.getAndSet(true) == false) { + deprecationLogger.deprecate( + DeprecationCategory.SETTINGS, + "destructive_requires_name_default", + "setting [action.destructive_requires_name] will default to true in 8.0, " + + "set to false to preserve current behavior" + ); + } + return "false"; + }, + Property.Dynamic, + Property.NodeScope); + /** * The "match none" pattern, "*,-*", will never actually be destructive * because it operates on no indices. If plugins or other components add From 7b12df452f555233ad0947204cfa2c7d12050f64 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Fri, 26 Mar 2021 15:07:34 -0400 Subject: [PATCH 02/10] Simplify deprecation warning --- .../action/support/DestructiveOperations.java | 26 +++++++------------ 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java index a6dfaf0325762..3126a14964d37 100644 --- a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java +++ b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java @@ -16,7 +16,6 @@ import org.elasticsearch.common.settings.Settings; import java.util.Arrays; -import java.util.concurrent.atomic.AtomicBoolean; /** * Helper for dealing with destructive operations and wildcard usage. @@ -25,26 +24,11 @@ public final class DestructiveOperations { private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(DestructiveOperations.class); - private static final AtomicBoolean deprecationLogged = new AtomicBoolean(false); - /** * Setting which controls whether wildcard usage (*, prefix*, _all) is allowed. */ public static final Setting REQUIRES_NAME_SETTING = - Setting.boolSetting("action.destructive_requires_name", - (settings) -> { - if (deprecationLogged.getAndSet(true) == false) { - deprecationLogger.deprecate( - DeprecationCategory.SETTINGS, - "destructive_requires_name_default", - "setting [action.destructive_requires_name] will default to true in 8.0, " + - "set to false to preserve current behavior" - ); - } - return "false"; - }, - Property.Dynamic, - Property.NodeScope); + Setting.boolSetting("action.destructive_requires_name", false, Property.Dynamic, Property.NodeScope); /** * The "match none" pattern, "*,-*", will never actually be destructive @@ -58,6 +42,14 @@ public final class DestructiveOperations { private volatile boolean destructiveRequiresName; public DestructiveOperations(Settings settings, ClusterSettings clusterSettings) { + if (REQUIRES_NAME_SETTING.exists(settings) == false) { + deprecationLogger.deprecate( + DeprecationCategory.SETTINGS, + "destructive_requires_name_default", + "setting [action.destructive_requires_name] will default to true in 8.0, " + + "set explicitly to false to preserve current behavior" + ); + } destructiveRequiresName = REQUIRES_NAME_SETTING.get(settings); clusterSettings.addSettingsUpdateConsumer(REQUIRES_NAME_SETTING, this::setDestructiveRequiresName); } From e1fc9779d75501f5060fc44db0a7bc34da8cedb3 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Fri, 26 Mar 2021 17:34:01 -0400 Subject: [PATCH 03/10] Catch deprecation warnings in tests --- .../org/elasticsearch/action/ActionModuleTests.java | 9 +++++++++ .../replication/TransportReplicationActionTests.java | 3 +++ .../transport/TransportClientHeadersTests.java | 3 +++ .../client/transport/TransportClientTests.java | 12 +++++++++--- .../cluster/metadata/AutoExpandReplicasTests.java | 6 ++++++ .../routing/allocation/FailedNodeRoutingTests.java | 3 +++ .../indices/IndicesServiceCloseTests.java | 2 ++ .../test/java/org/elasticsearch/node/NodeTests.java | 2 ++ .../snapshots/SnapshotResiliencyTests.java | 1 + .../org/elasticsearch/test/ESSingleNodeTestCase.java | 2 ++ .../org/elasticsearch/xpack/CcrIntegTestCase.java | 2 ++ .../xpack/core/search/CCSPointInTimeIT.java | 3 +++ 12 files changed, 45 insertions(+), 3 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java index 2a6fef18803e8..d9efa603d757d 100644 --- a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java @@ -117,6 +117,9 @@ public List routes() { } })); assertThat(e.getMessage(), startsWith("Cannot replace existing handler for [/] for method: GET")); + + assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + + "set explicitly to false to preserve current behavior"); } public void testPluginCantOverwriteBuiltinRestHandler() throws IOException { @@ -145,6 +148,9 @@ public String getName() { singletonList(dupsMainAction), null, null, usageService, null); Exception e = expectThrows(IllegalArgumentException.class, () -> actionModule.initRestHandlers(null)); assertThat(e.getMessage(), startsWith("Cannot replace existing handler for [/] for method: GET")); + + assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + + "set explicitly to false to preserve current behavior"); } finally { threadPool.shutdown(); } @@ -192,6 +198,9 @@ public List routes() { } })); assertThat(e.getMessage(), startsWith("Cannot replace existing handler for [/_dummy] for method: GET")); + + assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + + "set explicitly to false to preserve current behavior"); } finally { threadPool.shutdown(); } diff --git a/server/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java b/server/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java index f872ae4689bbe..759b0dcf7f064 100644 --- a/server/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java @@ -534,6 +534,9 @@ public void testClosedIndexOnReroute() { assertPhase(task, "failed"); assertFalse(request.isRetrySet.get()); + + assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + + "set explicitly to false to preserve current behavior"); } public void testStalePrimaryShardOnReroute() { diff --git a/server/src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java b/server/src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java index a6273287a251c..7253a6f153b48 100644 --- a/server/src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java +++ b/server/src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java @@ -15,6 +15,7 @@ import org.elasticsearch.action.admin.cluster.node.liveness.TransportLivenessAction; import org.elasticsearch.action.admin.cluster.state.ClusterStateAction; import org.elasticsearch.action.admin.cluster.state.ClusterStateResponse; +import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.client.AbstractClientHeadersTestCase; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.ClusterName; @@ -78,6 +79,7 @@ protected Client buildClient(Settings headersSettings, ActionType[] testedAction .put("node.name", "transport_client_" + this.getTestName()) .put(NetworkModule.TRANSPORT_TYPE_SETTING.getKey(), transport) .put(headersSettings) + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), "false") .build(), InternalTransportServiceInterceptor.TestPlugin.class); InternalTransportServiceInterceptor.TestPlugin plugin = client.injector.getInstance(PluginsService.class) .filterPlugins(InternalTransportServiceInterceptor.TestPlugin.class).stream().findFirst().get(); @@ -97,6 +99,7 @@ public void testWithSniffing() throws Exception { .put("client.transport.nodes_sampler_interval", "1s") .put(NetworkModule.TRANSPORT_TYPE_SETTING.getKey(), transport) .put(HEADER_SETTINGS) + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), "false") .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toString()).build(), InternalTransportServiceInterceptor.TestPlugin.class)) { InternalTransportServiceInterceptor.TestPlugin plugin = client.injector.getInstance(PluginsService.class) diff --git a/server/src/test/java/org/elasticsearch/client/transport/TransportClientTests.java b/server/src/test/java/org/elasticsearch/client/transport/TransportClientTests.java index a70c35beb3270..1a7b38ca3c60b 100644 --- a/server/src/test/java/org/elasticsearch/client/transport/TransportClientTests.java +++ b/server/src/test/java/org/elasticsearch/client/transport/TransportClientTests.java @@ -9,6 +9,7 @@ package org.elasticsearch.client.transport; import org.elasticsearch.action.admin.cluster.health.ClusterHealthRequest; +import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.common.io.stream.NamedWriteable; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.io.stream.NamedWriteableRegistry.Entry; @@ -35,7 +36,9 @@ public class TransportClientTests extends ESTestCase { public void testThatUsingAClosedClientThrowsAnException() throws ExecutionException, InterruptedException { - final TransportClient client = new MockTransportClient(Settings.EMPTY); + final TransportClient client = new MockTransportClient(Settings.builder() + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) + .build()); client.close(); final IllegalStateException e = expectThrows(IllegalStateException.class, () -> client.admin().cluster().health(new ClusterHealthRequest()).actionGet()); @@ -50,7 +53,8 @@ public void testThatUsingAClosedClientThrowsAnException() throws ExecutionExcept public void testPluginNamedWriteablesRegistered() { Settings baseSettings = Settings.builder() .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir()) - .build(); + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) + .build(); try (TransportClient client = new MockTransportClient(baseSettings, Arrays.asList(MockPlugin.class))) { assertNotNull(client.namedWriteableRegistry.getReader(MockPlugin.MockNamedWriteable.class, MockPlugin.MockNamedWriteable.NAME)); } @@ -59,6 +63,7 @@ public void testPluginNamedWriteablesRegistered() { public void testSettingsContainsTransportClient() { final Settings baseSettings = Settings.builder() .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir()) + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) .build(); try (TransportClient client = new MockTransportClient(baseSettings, Arrays.asList(MockPlugin.class))) { final Settings settings = TransportSettings.DEFAULT_FEATURES_SETTING.get(client.settings()); @@ -70,7 +75,8 @@ public void testSettingsContainsTransportClient() { public void testDefaultHeader() { final Settings baseSettings = Settings.builder() .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir()) - .build(); + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) + .build(); try (TransportClient client = new MockTransportClient(baseSettings, Arrays.asList(MockPlugin.class))) { final ThreadContext threadContext = client.threadPool().getThreadContext(); assertEquals("true", threadContext.getHeader("test")); diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasTests.java index f5fc9764ff8fa..c1376f4efff2b 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasTests.java @@ -195,6 +195,9 @@ public void testAutoExpandWhenNodeLeavesAndPossiblyRejoins() throws InterruptedE } } ); + + assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + + "set explicitly to false to preserve current behavior"); } finally { terminate(threadPool); } @@ -247,6 +250,9 @@ public void testOnlyAutoExpandAllocationFilteringAfterAllNodesUpgraded() { // remove old node and check that auto-expansion takes allocation filtering into account state = cluster.removeNodes(state, Collections.singletonList(oldNode)); assertThat(state.routingTable().index("index").shard(0).size(), equalTo(1)); + + assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + + "set explicitly to false to preserve current behavior"); } finally { terminate(threadPool); } diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/FailedNodeRoutingTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/FailedNodeRoutingTests.java index a273a605b0090..9a23466b9e1da 100644 --- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/FailedNodeRoutingTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/FailedNodeRoutingTests.java @@ -185,6 +185,9 @@ public void testRandomClusterPromotesNewestReplica() throws InterruptedException keepGoing = randomBoolean(); } terminate(threadPool); + + assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + + "set explicitly to false to preserve current behavior"); } private static Version getNodeVersion(ShardRouting shardRouting, ClusterState state) { diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java b/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java index 45b7de58b002b..f786b5ae5d14a 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java +++ b/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java @@ -10,6 +10,7 @@ import org.apache.lucene.document.LongPoint; import org.apache.lucene.search.Query; +import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.routing.allocation.DiskThresholdSettings; import org.elasticsearch.common.bytes.BytesArray; @@ -73,6 +74,7 @@ private Node startNode() throws NodeValidationException { .putList(DISCOVERY_SEED_HOSTS_SETTING.getKey()) // empty list disables a port scan for other nodes .putList(INITIAL_MASTER_NODES_SETTING.getKey(), nodeName) .put(IndicesQueryCache.INDICES_QUERIES_CACHE_ALL_SEGMENTS_SETTING.getKey(), true) + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) .build(); Node node = new MockNode(settings, diff --git a/server/src/test/java/org/elasticsearch/node/NodeTests.java b/server/src/test/java/org/elasticsearch/node/NodeTests.java index fd0865061e377..d5bb8504cfcd2 100644 --- a/server/src/test/java/org/elasticsearch/node/NodeTests.java +++ b/server/src/test/java/org/elasticsearch/node/NodeTests.java @@ -9,6 +9,7 @@ import org.apache.lucene.util.LuceneTestCase; import org.apache.lucene.util.SetOnce; +import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.bootstrap.BootstrapCheck; import org.elasticsearch.bootstrap.BootstrapContext; import org.elasticsearch.cluster.ClusterName; @@ -145,6 +146,7 @@ private static Settings.Builder baseSettings() { .put(ClusterName.CLUSTER_NAME_SETTING.getKey(), InternalTestCluster.clusterName("single-node-cluster", randomLong())) .put(Environment.PATH_HOME_SETTING.getKey(), tempDir) .put(NetworkModule.TRANSPORT_TYPE_KEY, getTestTransportType()) + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) .put(dataNode()); } diff --git a/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java index 177f0f220b60a..4e5cb7035c22e 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java @@ -1205,6 +1205,7 @@ private Environment createEnvironment(String nodeName) { .putList(ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING.getKey(), ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING.get(Settings.EMPTY)) .put(MappingUpdatedAction.INDICES_MAX_IN_FLIGHT_UPDATES_SETTING.getKey(), 1000) // o.w. some tests might block + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) .build()); } diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java index 8ee8111722b63..0cb8b97447cdb 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java @@ -12,6 +12,7 @@ import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder; import org.elasticsearch.action.admin.indices.get.GetIndexResponse; +import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.client.Client; import org.elasticsearch.client.Requests; @@ -210,6 +211,7 @@ private Node newNode() { .put(HierarchyCircuitBreakerService.USE_REAL_MEMORY_USAGE_SETTING.getKey(), false) .putList(DISCOVERY_SEED_HOSTS_SETTING.getKey()) // empty list disables a port scan for other nodes .putList(INITIAL_MASTER_NODES_SETTING.getKey(), nodeName) + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) .put(nodeSettings()) // allow test cases to provide their own settings or override these .build(); diff --git a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/CcrIntegTestCase.java b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/CcrIntegTestCase.java index f09321d3a628f..0d6ba4242ef10 100644 --- a/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/CcrIntegTestCase.java +++ b/x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/CcrIntegTestCase.java @@ -22,6 +22,7 @@ import org.elasticsearch.action.search.SearchRequest; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.support.ActiveShardCount; +import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.action.support.master.AcknowledgedRequest; import org.elasticsearch.analysis.common.CommonAnalysisPlugin; import org.elasticsearch.client.Client; @@ -277,6 +278,7 @@ private NodeConfigurationSource createNodeConfigurationSource(final String leade builder.put(XPackSettings.WATCHER_ENABLED.getKey(), false); builder.put(XPackSettings.MACHINE_LEARNING_ENABLED.getKey(), false); builder.put(LicenseService.SELF_GENERATED_LICENSE_TYPE.getKey(), "trial"); + builder.put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false); // Let cluster state api return quickly in order to speed up auto follow tests: builder.put(CcrSettings.CCR_WAIT_FOR_METADATA_TIMEOUT.getKey(), TimeValue.timeValueMillis(100)); if (leaderCluster) { diff --git a/x-pack/plugin/core/src/internalClusterTest/java/org/elasticsearch/xpack/core/search/CCSPointInTimeIT.java b/x-pack/plugin/core/src/internalClusterTest/java/org/elasticsearch/xpack/core/search/CCSPointInTimeIT.java index 6dd4b29bba497..398d933138ed1 100644 --- a/x-pack/plugin/core/src/internalClusterTest/java/org/elasticsearch/xpack/core/search/CCSPointInTimeIT.java +++ b/x-pack/plugin/core/src/internalClusterTest/java/org/elasticsearch/xpack/core/search/CCSPointInTimeIT.java @@ -86,6 +86,9 @@ public void testBasic() { .get(); assertNoFailures(resp); assertHitCount(resp, (includeLocalIndex ? localNumDocs : 0) + remoteNumDocs); + + assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + + "set explicitly to false to preserve current behavior"); } finally { closePointInTime(pitId); } From 72109940a0be61c734ba297282ea94d2e2f1d6e2 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Mon, 29 Mar 2021 10:15:40 -0400 Subject: [PATCH 04/10] Only issue a deprecation warning with wildcard deletion attempt --- .../action/support/DestructiveOperations.java | 36 +++++++++++-------- .../action/ActionModuleTests.java | 9 ----- .../TransportReplicationActionTests.java | 3 -- .../TransportClientHeadersTests.java | 3 -- .../transport/TransportClientTests.java | 12 ++----- .../metadata/AutoExpandReplicasTests.java | 6 ---- .../allocation/FailedNodeRoutingTests.java | 3 -- .../indices/IndicesServiceCloseTests.java | 2 -- .../org/elasticsearch/node/NodeTests.java | 2 -- .../snapshots/SnapshotResiliencyTests.java | 1 - .../test/ESSingleNodeTestCase.java | 2 -- .../xpack/core/search/CCSPointInTimeIT.java | 3 -- 12 files changed, 24 insertions(+), 58 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java index 3126a14964d37..40f2d9d925b4d 100644 --- a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java +++ b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java @@ -41,20 +41,16 @@ public final class DestructiveOperations { private volatile boolean destructiveRequiresName; + private volatile boolean isDestructiveRequiresNameSet; + public DestructiveOperations(Settings settings, ClusterSettings clusterSettings) { - if (REQUIRES_NAME_SETTING.exists(settings) == false) { - deprecationLogger.deprecate( - DeprecationCategory.SETTINGS, - "destructive_requires_name_default", - "setting [action.destructive_requires_name] will default to true in 8.0, " + - "set explicitly to false to preserve current behavior" - ); - } destructiveRequiresName = REQUIRES_NAME_SETTING.get(settings); + isDestructiveRequiresNameSet = REQUIRES_NAME_SETTING.exists(settings); clusterSettings.addSettingsUpdateConsumer(REQUIRES_NAME_SETTING, this::setDestructiveRequiresName); } private void setDestructiveRequiresName(boolean destructiveRequiresName) { + this.isDestructiveRequiresNameSet = true; this.destructiveRequiresName = destructiveRequiresName; } @@ -62,25 +58,35 @@ private void setDestructiveRequiresName(boolean destructiveRequiresName) { * Fail if there is wildcard usage in indices and the named is required for destructive operations. */ public void failDestructive(String[] aliasesOrIndices) { - if (destructiveRequiresName == false) { - return; - } - if (aliasesOrIndices == null || aliasesOrIndices.length == 0) { - throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); + handleWildcardOperation(); } else if (aliasesOrIndices.length == 1) { if (hasWildcardUsage(aliasesOrIndices[0])) { - throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); + handleWildcardOperation(); } } else if (Arrays.equals(aliasesOrIndices, MATCH_NONE_PATTERN) == false) { for (String aliasesOrIndex : aliasesOrIndices) { if (hasWildcardUsage(aliasesOrIndex)) { - throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); + handleWildcardOperation(); } } } } + private void handleWildcardOperation() { + if (isDestructiveRequiresNameSet == false) { + deprecationLogger.deprecate( + DeprecationCategory.SETTINGS, + "destructive_requires_name_default", + "setting [action.destructive_requires_name] will default to true in 8.0, " + + "set explicitly to false to preserve current behavior" + ); + } + if (destructiveRequiresName) { + throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); + } + } + private static boolean hasWildcardUsage(String aliasOrIndex) { return "_all".equals(aliasOrIndex) || aliasOrIndex.indexOf('*') != -1; } diff --git a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java index d9efa603d757d..2a6fef18803e8 100644 --- a/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java +++ b/server/src/test/java/org/elasticsearch/action/ActionModuleTests.java @@ -117,9 +117,6 @@ public List routes() { } })); assertThat(e.getMessage(), startsWith("Cannot replace existing handler for [/] for method: GET")); - - assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + - "set explicitly to false to preserve current behavior"); } public void testPluginCantOverwriteBuiltinRestHandler() throws IOException { @@ -148,9 +145,6 @@ public String getName() { singletonList(dupsMainAction), null, null, usageService, null); Exception e = expectThrows(IllegalArgumentException.class, () -> actionModule.initRestHandlers(null)); assertThat(e.getMessage(), startsWith("Cannot replace existing handler for [/] for method: GET")); - - assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + - "set explicitly to false to preserve current behavior"); } finally { threadPool.shutdown(); } @@ -198,9 +192,6 @@ public List routes() { } })); assertThat(e.getMessage(), startsWith("Cannot replace existing handler for [/_dummy] for method: GET")); - - assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + - "set explicitly to false to preserve current behavior"); } finally { threadPool.shutdown(); } diff --git a/server/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java b/server/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java index 759b0dcf7f064..f872ae4689bbe 100644 --- a/server/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java @@ -534,9 +534,6 @@ public void testClosedIndexOnReroute() { assertPhase(task, "failed"); assertFalse(request.isRetrySet.get()); - - assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + - "set explicitly to false to preserve current behavior"); } public void testStalePrimaryShardOnReroute() { diff --git a/server/src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java b/server/src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java index 7253a6f153b48..a6273287a251c 100644 --- a/server/src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java +++ b/server/src/test/java/org/elasticsearch/client/transport/TransportClientHeadersTests.java @@ -15,7 +15,6 @@ import org.elasticsearch.action.admin.cluster.node.liveness.TransportLivenessAction; import org.elasticsearch.action.admin.cluster.state.ClusterStateAction; import org.elasticsearch.action.admin.cluster.state.ClusterStateResponse; -import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.client.AbstractClientHeadersTestCase; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.ClusterName; @@ -79,7 +78,6 @@ protected Client buildClient(Settings headersSettings, ActionType[] testedAction .put("node.name", "transport_client_" + this.getTestName()) .put(NetworkModule.TRANSPORT_TYPE_SETTING.getKey(), transport) .put(headersSettings) - .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), "false") .build(), InternalTransportServiceInterceptor.TestPlugin.class); InternalTransportServiceInterceptor.TestPlugin plugin = client.injector.getInstance(PluginsService.class) .filterPlugins(InternalTransportServiceInterceptor.TestPlugin.class).stream().findFirst().get(); @@ -99,7 +97,6 @@ public void testWithSniffing() throws Exception { .put("client.transport.nodes_sampler_interval", "1s") .put(NetworkModule.TRANSPORT_TYPE_SETTING.getKey(), transport) .put(HEADER_SETTINGS) - .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), "false") .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toString()).build(), InternalTransportServiceInterceptor.TestPlugin.class)) { InternalTransportServiceInterceptor.TestPlugin plugin = client.injector.getInstance(PluginsService.class) diff --git a/server/src/test/java/org/elasticsearch/client/transport/TransportClientTests.java b/server/src/test/java/org/elasticsearch/client/transport/TransportClientTests.java index 1a7b38ca3c60b..a70c35beb3270 100644 --- a/server/src/test/java/org/elasticsearch/client/transport/TransportClientTests.java +++ b/server/src/test/java/org/elasticsearch/client/transport/TransportClientTests.java @@ -9,7 +9,6 @@ package org.elasticsearch.client.transport; import org.elasticsearch.action.admin.cluster.health.ClusterHealthRequest; -import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.common.io.stream.NamedWriteable; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; import org.elasticsearch.common.io.stream.NamedWriteableRegistry.Entry; @@ -36,9 +35,7 @@ public class TransportClientTests extends ESTestCase { public void testThatUsingAClosedClientThrowsAnException() throws ExecutionException, InterruptedException { - final TransportClient client = new MockTransportClient(Settings.builder() - .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) - .build()); + final TransportClient client = new MockTransportClient(Settings.EMPTY); client.close(); final IllegalStateException e = expectThrows(IllegalStateException.class, () -> client.admin().cluster().health(new ClusterHealthRequest()).actionGet()); @@ -53,8 +50,7 @@ public void testThatUsingAClosedClientThrowsAnException() throws ExecutionExcept public void testPluginNamedWriteablesRegistered() { Settings baseSettings = Settings.builder() .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir()) - .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) - .build(); + .build(); try (TransportClient client = new MockTransportClient(baseSettings, Arrays.asList(MockPlugin.class))) { assertNotNull(client.namedWriteableRegistry.getReader(MockPlugin.MockNamedWriteable.class, MockPlugin.MockNamedWriteable.NAME)); } @@ -63,7 +59,6 @@ public void testPluginNamedWriteablesRegistered() { public void testSettingsContainsTransportClient() { final Settings baseSettings = Settings.builder() .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir()) - .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) .build(); try (TransportClient client = new MockTransportClient(baseSettings, Arrays.asList(MockPlugin.class))) { final Settings settings = TransportSettings.DEFAULT_FEATURES_SETTING.get(client.settings()); @@ -75,8 +70,7 @@ public void testSettingsContainsTransportClient() { public void testDefaultHeader() { final Settings baseSettings = Settings.builder() .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir()) - .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) - .build(); + .build(); try (TransportClient client = new MockTransportClient(baseSettings, Arrays.asList(MockPlugin.class))) { final ThreadContext threadContext = client.threadPool().getThreadContext(); assertEquals("true", threadContext.getHeader("test")); diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasTests.java index c1376f4efff2b..f5fc9764ff8fa 100644 --- a/server/src/test/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/metadata/AutoExpandReplicasTests.java @@ -195,9 +195,6 @@ public void testAutoExpandWhenNodeLeavesAndPossiblyRejoins() throws InterruptedE } } ); - - assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + - "set explicitly to false to preserve current behavior"); } finally { terminate(threadPool); } @@ -250,9 +247,6 @@ public void testOnlyAutoExpandAllocationFilteringAfterAllNodesUpgraded() { // remove old node and check that auto-expansion takes allocation filtering into account state = cluster.removeNodes(state, Collections.singletonList(oldNode)); assertThat(state.routingTable().index("index").shard(0).size(), equalTo(1)); - - assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + - "set explicitly to false to preserve current behavior"); } finally { terminate(threadPool); } diff --git a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/FailedNodeRoutingTests.java b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/FailedNodeRoutingTests.java index 9a23466b9e1da..a273a605b0090 100644 --- a/server/src/test/java/org/elasticsearch/cluster/routing/allocation/FailedNodeRoutingTests.java +++ b/server/src/test/java/org/elasticsearch/cluster/routing/allocation/FailedNodeRoutingTests.java @@ -185,9 +185,6 @@ public void testRandomClusterPromotesNewestReplica() throws InterruptedException keepGoing = randomBoolean(); } terminate(threadPool); - - assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + - "set explicitly to false to preserve current behavior"); } private static Version getNodeVersion(ShardRouting shardRouting, ClusterState state) { diff --git a/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java b/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java index f786b5ae5d14a..45b7de58b002b 100644 --- a/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java +++ b/server/src/test/java/org/elasticsearch/indices/IndicesServiceCloseTests.java @@ -10,7 +10,6 @@ import org.apache.lucene.document.LongPoint; import org.apache.lucene.search.Query; -import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.routing.allocation.DiskThresholdSettings; import org.elasticsearch.common.bytes.BytesArray; @@ -74,7 +73,6 @@ private Node startNode() throws NodeValidationException { .putList(DISCOVERY_SEED_HOSTS_SETTING.getKey()) // empty list disables a port scan for other nodes .putList(INITIAL_MASTER_NODES_SETTING.getKey(), nodeName) .put(IndicesQueryCache.INDICES_QUERIES_CACHE_ALL_SEGMENTS_SETTING.getKey(), true) - .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) .build(); Node node = new MockNode(settings, diff --git a/server/src/test/java/org/elasticsearch/node/NodeTests.java b/server/src/test/java/org/elasticsearch/node/NodeTests.java index d5bb8504cfcd2..fd0865061e377 100644 --- a/server/src/test/java/org/elasticsearch/node/NodeTests.java +++ b/server/src/test/java/org/elasticsearch/node/NodeTests.java @@ -9,7 +9,6 @@ import org.apache.lucene.util.LuceneTestCase; import org.apache.lucene.util.SetOnce; -import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.bootstrap.BootstrapCheck; import org.elasticsearch.bootstrap.BootstrapContext; import org.elasticsearch.cluster.ClusterName; @@ -146,7 +145,6 @@ private static Settings.Builder baseSettings() { .put(ClusterName.CLUSTER_NAME_SETTING.getKey(), InternalTestCluster.clusterName("single-node-cluster", randomLong())) .put(Environment.PATH_HOME_SETTING.getKey(), tempDir) .put(NetworkModule.TRANSPORT_TYPE_KEY, getTestTransportType()) - .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) .put(dataNode()); } diff --git a/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java b/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java index 4e5cb7035c22e..177f0f220b60a 100644 --- a/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java +++ b/server/src/test/java/org/elasticsearch/snapshots/SnapshotResiliencyTests.java @@ -1205,7 +1205,6 @@ private Environment createEnvironment(String nodeName) { .putList(ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING.getKey(), ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING.get(Settings.EMPTY)) .put(MappingUpdatedAction.INDICES_MAX_IN_FLIGHT_UPDATES_SETTING.getKey(), 1000) // o.w. some tests might block - .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) .build()); } diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java index 0cb8b97447cdb..8ee8111722b63 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java @@ -12,7 +12,6 @@ import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder; import org.elasticsearch.action.admin.indices.get.GetIndexResponse; -import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.client.Client; import org.elasticsearch.client.Requests; @@ -211,7 +210,6 @@ private Node newNode() { .put(HierarchyCircuitBreakerService.USE_REAL_MEMORY_USAGE_SETTING.getKey(), false) .putList(DISCOVERY_SEED_HOSTS_SETTING.getKey()) // empty list disables a port scan for other nodes .putList(INITIAL_MASTER_NODES_SETTING.getKey(), nodeName) - .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) .put(nodeSettings()) // allow test cases to provide their own settings or override these .build(); diff --git a/x-pack/plugin/core/src/internalClusterTest/java/org/elasticsearch/xpack/core/search/CCSPointInTimeIT.java b/x-pack/plugin/core/src/internalClusterTest/java/org/elasticsearch/xpack/core/search/CCSPointInTimeIT.java index 398d933138ed1..6dd4b29bba497 100644 --- a/x-pack/plugin/core/src/internalClusterTest/java/org/elasticsearch/xpack/core/search/CCSPointInTimeIT.java +++ b/x-pack/plugin/core/src/internalClusterTest/java/org/elasticsearch/xpack/core/search/CCSPointInTimeIT.java @@ -86,9 +86,6 @@ public void testBasic() { .get(); assertNoFailures(resp); assertHitCount(resp, (includeLocalIndex ? localNumDocs : 0) + remoteNumDocs); - - assertWarnings("setting [action.destructive_requires_name] will default to true in 8.0, " + - "set explicitly to false to preserve current behavior"); } finally { closePointInTime(pitId); } From cc4d0ef2cb3206c4feb04e053d7264e8a79d4208 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Mon, 29 Mar 2021 10:34:08 -0400 Subject: [PATCH 05/10] Allow wildcard deletions in test base classes --- .../src/main/java/org/elasticsearch/test/ESIntegTestCase.java | 2 ++ .../main/java/org/elasticsearch/test/ESSingleNodeTestCase.java | 2 ++ .../main/java/org/elasticsearch/test/InternalTestCluster.java | 2 ++ 3 files changed, 6 insertions(+) diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java index 7efbab650cb5b..08b09731630e3 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESIntegTestCase.java @@ -45,6 +45,7 @@ import org.elasticsearch.action.search.ClearScrollResponse; import org.elasticsearch.action.search.SearchResponse; import org.elasticsearch.action.support.DefaultShardOperationFailedException; +import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.bootstrap.JavaVersion; import org.elasticsearch.client.AdminClient; @@ -1849,6 +1850,7 @@ private int getNumClientNodes() { */ protected Settings nodeSettings(int nodeOrdinal) { Settings.Builder builder = Settings.builder() + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) // Default the watermarks to absurdly low to prevent the tests // from failing on nodes without enough disk space .put(DiskThresholdSettings.CLUSTER_ROUTING_ALLOCATION_LOW_DISK_WATERMARK_SETTING.getKey(), "1b") diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java index 8ee8111722b63..8d47239e8291c 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java @@ -12,6 +12,7 @@ import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder; import org.elasticsearch.action.admin.indices.get.GetIndexResponse; +import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.client.Client; import org.elasticsearch.client.Requests; @@ -190,6 +191,7 @@ private Node newNode() { Settings settings = Settings.builder() .put(ClusterName.CLUSTER_NAME_SETTING.getKey(), InternalTestCluster.clusterName("single-node-cluster", random().nextLong())) + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) .put(Environment.PATH_HOME_SETTING.getKey(), tempDir) .put(Environment.PATH_REPO_SETTING.getKey(), tempDir.resolve("repo")) // TODO: use a consistent data path for custom paths diff --git a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java index 2c64d3986fc91..00e28a9f247bf 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java +++ b/test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java @@ -26,6 +26,7 @@ import org.elasticsearch.action.admin.cluster.node.stats.NodeStats; import org.elasticsearch.action.admin.indices.stats.CommonStatsFlags; import org.elasticsearch.action.admin.indices.stats.CommonStatsFlags.Flag; +import org.elasticsearch.action.support.DestructiveOperations; import org.elasticsearch.action.support.replication.TransportReplicationAction; import org.elasticsearch.client.Client; import org.elasticsearch.client.transport.TransportClient; @@ -402,6 +403,7 @@ public InternalTestCluster( // TODO: currently we only randomize "cluster.no_master_block" between "write" and "metadata_write", as "all" is fragile // and fails shards when a master abdicates, which breaks many tests. builder.put(NoMasterBlockService.NO_MASTER_BLOCK_SETTING.getKey(), randomFrom(random,"write", "metadata_write")); + builder.put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false); defaultSettings = builder.build(); executor = EsExecutors.newScaling("internal_test_cluster_executor", 0, Integer.MAX_VALUE, 0, TimeUnit.SECONDS, EsExecutors.daemonThreadFactory("test_" + clusterName), new ThreadContext(Settings.EMPTY)); From 287947ce935ecfa4da6696f3379a5f069fbd0e40 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Mon, 29 Mar 2021 10:57:53 -0400 Subject: [PATCH 06/10] Allow wildcard deletions in java rest tests --- .../elasticsearch/gradle/testclusters/ElasticsearchNode.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java b/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java index ff47d5ce94292..2cd1a2d5b3fea 100644 --- a/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java +++ b/buildSrc/src/main/java/org/elasticsearch/gradle/testclusters/ElasticsearchNode.java @@ -1213,6 +1213,8 @@ private void createConfiguration() { // Don't wait for state, just start up quickly. This will also allow new and old nodes in the BWC case to become the master baseConfig.put("discovery.initial_state_timeout", "0s"); + baseConfig.put("action.destructive_requires_name", "false"); + HashSet overriden = new HashSet<>(baseConfig.keySet()); overriden.retainAll(settings.keySet()); overriden.removeAll(OVERRIDABLE_SETTINGS); From c7cd6475777343e10af540fadbb3075c59f5153b Mon Sep 17 00:00:00 2001 From: William Brafford Date: Mon, 29 Mar 2021 11:33:10 -0400 Subject: [PATCH 07/10] Allow wildcard deletions in docker tests --- distribution/docker/docker-compose.yml | 2 ++ qa/remote-clusters/docker-compose-oss.yml | 2 ++ qa/remote-clusters/docker-compose.yml | 2 ++ 3 files changed, 6 insertions(+) diff --git a/distribution/docker/docker-compose.yml b/distribution/docker/docker-compose.yml index f187186c59409..62704cc6510c8 100644 --- a/distribution/docker/docker-compose.yml +++ b/distribution/docker/docker-compose.yml @@ -30,6 +30,7 @@ services: - xpack.http.ssl.verification_mode=certificate - xpack.security.transport.ssl.verification_mode=certificate - xpack.license.self_generated.type=trial + - action.destructive_requires_name=false volumes: - ./build/repo:/tmp/es-repo - ./build/certs/testnode.pem:/usr/share/elasticsearch/config/testnode.pem @@ -72,6 +73,7 @@ services: - xpack.http.ssl.verification_mode=certificate - xpack.security.transport.ssl.verification_mode=certificate - xpack.license.self_generated.type=trial + - action.destructive_requires_name=false volumes: - ./build/repo:/tmp/es-repo - ./build/certs/testnode.pem:/usr/share/elasticsearch/config/testnode.pem diff --git a/qa/remote-clusters/docker-compose-oss.yml b/qa/remote-clusters/docker-compose-oss.yml index 1a77d1bc9999d..911058f123a6d 100644 --- a/qa/remote-clusters/docker-compose-oss.yml +++ b/qa/remote-clusters/docker-compose-oss.yml @@ -16,6 +16,7 @@ services: - cluster.routing.allocation.disk.watermark.high=1b - cluster.routing.allocation.disk.watermark.flood_stage=1b - node.store.allow_mmap=false + - action.destructive_requires_name=false volumes: - ./build/oss-repo:/tmp/es-repo - ./build/logs/oss-1:/usr/share/elasticsearch/logs @@ -50,6 +51,7 @@ services: - cluster.routing.allocation.disk.watermark.high=1b - cluster.routing.allocation.disk.watermark.flood_stage=1b - node.store.allow_mmap=false + - action.destructive_requires_name=false volumes: - ./build/oss-repo:/tmp/es-repo - ./build/logs/oss-2:/usr/share/elasticsearch/logs diff --git a/qa/remote-clusters/docker-compose.yml b/qa/remote-clusters/docker-compose.yml index debf863c58067..bcbbe9c37d259 100644 --- a/qa/remote-clusters/docker-compose.yml +++ b/qa/remote-clusters/docker-compose.yml @@ -30,6 +30,7 @@ services: - xpack.http.ssl.verification_mode=certificate - xpack.security.transport.ssl.verification_mode=certificate - xpack.license.self_generated.type=trial + - action.destructive_requires_name=false volumes: - ./build/repo:/tmp/es-repo - ./build/certs/testnode.pem:/usr/share/elasticsearch/config/testnode.pem @@ -82,6 +83,7 @@ services: - xpack.http.ssl.verification_mode=certificate - xpack.security.transport.ssl.verification_mode=certificate - xpack.license.self_generated.type=trial + - action.destructive_requires_name=false volumes: - ./build/repo:/tmp/es-repo - ./build/certs/testnode.pem:/usr/share/elasticsearch/config/testnode.pem From 66a68b8e67dc9e3d529124293a1324dea7313e7f Mon Sep 17 00:00:00 2001 From: William Brafford Date: Tue, 30 Mar 2021 10:42:34 -0400 Subject: [PATCH 08/10] Add tests and address PR feedback --- .../action/support/DestructiveOperations.java | 16 +++--- .../support/DestructiveOperationsTests.java | 52 +++++++++++++++++++ 2 files changed, 61 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java index 40f2d9d925b4d..450d9721ea776 100644 --- a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java +++ b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java @@ -30,6 +30,10 @@ public final class DestructiveOperations { public static final Setting REQUIRES_NAME_SETTING = Setting.boolSetting("action.destructive_requires_name", false, Property.Dynamic, Property.NodeScope); + static final String DESTRUCTIVE_REQUIRES_NAME_DEPRECATION_WARNING = "setting [action.destructive_requires_name] will " + + "default to true in 8.0, " + + "set explicitly to false to preserve current behavior"; + /** * The "match none" pattern, "*,-*", will never actually be destructive * because it operates on no indices. If plugins or other components add @@ -38,7 +42,6 @@ public final class DestructiveOperations { * indices, relying on the core code to handle this situation. */ private static final String[] MATCH_NONE_PATTERN = {"*", "-*"}; - private volatile boolean destructiveRequiresName; private volatile boolean isDestructiveRequiresNameSet; @@ -59,27 +62,26 @@ private void setDestructiveRequiresName(boolean destructiveRequiresName) { */ public void failDestructive(String[] aliasesOrIndices) { if (aliasesOrIndices == null || aliasesOrIndices.length == 0) { - handleWildcardOperation(); + checkWildCardOK(); } else if (aliasesOrIndices.length == 1) { if (hasWildcardUsage(aliasesOrIndices[0])) { - handleWildcardOperation(); + checkWildCardOK(); } } else if (Arrays.equals(aliasesOrIndices, MATCH_NONE_PATTERN) == false) { for (String aliasesOrIndex : aliasesOrIndices) { if (hasWildcardUsage(aliasesOrIndex)) { - handleWildcardOperation(); + checkWildCardOK(); } } } } - private void handleWildcardOperation() { + private void checkWildCardOK() { if (isDestructiveRequiresNameSet == false) { deprecationLogger.deprecate( DeprecationCategory.SETTINGS, "destructive_requires_name_default", - "setting [action.destructive_requires_name] will default to true in 8.0, " + - "set explicitly to false to preserve current behavior" + DESTRUCTIVE_REQUIRES_NAME_DEPRECATION_WARNING ); } if (destructiveRequiresName) { diff --git a/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTests.java b/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTests.java index 1d88393124822..4e3b6a1c7ae82 100644 --- a/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTests.java +++ b/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTests.java @@ -71,6 +71,58 @@ public void testNonDestructive() { } } + /** + * Test that we get a deprecation warning if we try a wildcard deletion and + * action.destructive_requires_name is unset. + */ + public void testDeprecationOfDefaultValue() { + DestructiveOperations destructiveOperations = new DestructiveOperations(Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, Set.of(DestructiveOperations.REQUIRES_NAME_SETTING))); + { + destructiveOperations.failDestructive(new String[]{"index-*"}); + assertWarnings(DestructiveOperations.DESTRUCTIVE_REQUIRES_NAME_DEPRECATION_WARNING); + } + { + destructiveOperations.failDestructive(new String[]{"index"}); + // no warning + } + { + destructiveOperations.failDestructive(new String[]{}); + assertWarnings(DestructiveOperations.DESTRUCTIVE_REQUIRES_NAME_DEPRECATION_WARNING); + } + } + + /** + * Test that we get a deprecation warning if we try a wildcard deletion and + * action.destructive_requires_name is unset. + */ + public void testDeprecationWarningClusterSettingsSync() { + ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, Set.of(DestructiveOperations.REQUIRES_NAME_SETTING)); + DestructiveOperations destructiveOperations = new DestructiveOperations(Settings.EMPTY, clusterSettings); + String warning = DestructiveOperations.DESTRUCTIVE_REQUIRES_NAME_DEPRECATION_WARNING; + + destructiveOperations.failDestructive(new String[]{"index-*"}); + assertWarnings(warning); + + clusterSettings.applySettings(Settings.builder() + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), "true") + .build()); + + assertFailsDestructive(new String[]{"index-*"}); + + clusterSettings.applySettings(Settings.EMPTY); + + destructiveOperations.failDestructive(new String[]{"index-*"}); + + // TODO - should warn again + + clusterSettings.applySettings(Settings.builder() + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), "false") + .build()); + + destructiveOperations.failDestructive(new String[]{"index-*"}); + } + private void assertFailsDestructive(String[] aliasesOrIndices) { IllegalArgumentException e = expectThrows( IllegalArgumentException.class, From 84ca19c8c534de69c197a975321da01e73fa861e Mon Sep 17 00:00:00 2001 From: William Brafford Date: Tue, 30 Mar 2021 11:00:21 -0400 Subject: [PATCH 09/10] Code format tweak --- .../elasticsearch/action/support/DestructiveOperations.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java index 450d9721ea776..5b50c72d79b60 100644 --- a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java +++ b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java @@ -31,8 +31,7 @@ public final class DestructiveOperations { Setting.boolSetting("action.destructive_requires_name", false, Property.Dynamic, Property.NodeScope); static final String DESTRUCTIVE_REQUIRES_NAME_DEPRECATION_WARNING = "setting [action.destructive_requires_name] will " + - "default to true in 8.0, " + - "set explicitly to false to preserve current behavior"; + "default to true in 8.0, set explicitly to false to preserve current behavior"; /** * The "match none" pattern, "*,-*", will never actually be destructive From 901ae71d5023f84f86c3a50b277167b8bcd04bab Mon Sep 17 00:00:00 2001 From: William Brafford Date: Tue, 30 Mar 2021 14:48:41 -0400 Subject: [PATCH 10/10] Removing setting should restore deprecation warning --- .../action/support/DestructiveOperations.java | 20 +++++--- .../support/DestructiveOperationsTests.java | 46 +++++++++++-------- 2 files changed, 40 insertions(+), 26 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java index 5b50c72d79b60..a37087113f4a6 100644 --- a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java +++ b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java @@ -8,6 +8,7 @@ package org.elasticsearch.action.support; +import org.elasticsearch.common.collect.List; import org.elasticsearch.common.logging.DeprecationCategory; import org.elasticsearch.common.logging.DeprecationLogger; import org.elasticsearch.common.settings.ClusterSettings; @@ -48,18 +49,25 @@ public final class DestructiveOperations { public DestructiveOperations(Settings settings, ClusterSettings clusterSettings) { destructiveRequiresName = REQUIRES_NAME_SETTING.get(settings); isDestructiveRequiresNameSet = REQUIRES_NAME_SETTING.exists(settings); - clusterSettings.addSettingsUpdateConsumer(REQUIRES_NAME_SETTING, this::setDestructiveRequiresName); + clusterSettings.addSettingsUpdateConsumer( + this::setDestructiveRequiresName, + List.of(REQUIRES_NAME_SETTING) + ); } - private void setDestructiveRequiresName(boolean destructiveRequiresName) { - this.isDestructiveRequiresNameSet = true; - this.destructiveRequiresName = destructiveRequiresName; + private void setDestructiveRequiresName(Settings settings) { + this.isDestructiveRequiresNameSet = REQUIRES_NAME_SETTING.exists(settings); + this.destructiveRequiresName = REQUIRES_NAME_SETTING.get(settings); } /** * Fail if there is wildcard usage in indices and the named is required for destructive operations. */ public void failDestructive(String[] aliasesOrIndices) { + if (this.isDestructiveRequiresNameSet && this.destructiveRequiresName == false) { + return; + } + if (aliasesOrIndices == null || aliasesOrIndices.length == 0) { checkWildCardOK(); } else if (aliasesOrIndices.length == 1) { @@ -76,14 +84,14 @@ public void failDestructive(String[] aliasesOrIndices) { } private void checkWildCardOK() { - if (isDestructiveRequiresNameSet == false) { + if (this.isDestructiveRequiresNameSet == false) { deprecationLogger.deprecate( DeprecationCategory.SETTINGS, "destructive_requires_name_default", DESTRUCTIVE_REQUIRES_NAME_DEPRECATION_WARNING ); } - if (destructiveRequiresName) { + if (this.destructiveRequiresName) { throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); } } diff --git a/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTests.java b/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTests.java index 4e3b6a1c7ae82..e170955a6b8f8 100644 --- a/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTests.java +++ b/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTests.java @@ -93,34 +93,40 @@ public void testDeprecationOfDefaultValue() { } /** - * Test that we get a deprecation warning if we try a wildcard deletion and - * action.destructive_requires_name is unset. + * Test that applying settings enables and disables the deprecation warning. */ public void testDeprecationWarningClusterSettingsSync() { ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, Set.of(DestructiveOperations.REQUIRES_NAME_SETTING)); DestructiveOperations destructiveOperations = new DestructiveOperations(Settings.EMPTY, clusterSettings); - String warning = DestructiveOperations.DESTRUCTIVE_REQUIRES_NAME_DEPRECATION_WARNING; - - destructiveOperations.failDestructive(new String[]{"index-*"}); - assertWarnings(warning); - - clusterSettings.applySettings(Settings.builder() - .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), "true") - .build()); - assertFailsDestructive(new String[]{"index-*"}); - - clusterSettings.applySettings(Settings.EMPTY); - - destructiveOperations.failDestructive(new String[]{"index-*"}); + { + // empty settings gives us a warning + destructiveOperations.failDestructive(new String[]{"index-*"}); + assertWarnings(DestructiveOperations.DESTRUCTIVE_REQUIRES_NAME_DEPRECATION_WARNING); + } + { + // setting to "true" removes the deprecation warning and fails the operation + clusterSettings.applySettings(Settings.builder() + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), "true") + .build()); - // TODO - should warn again + assertFailsDestructive(new String[]{"index-*"}); + } + { + // restoring the empty setting restores the warning + clusterSettings.applySettings(Settings.EMPTY); - clusterSettings.applySettings(Settings.builder() - .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), "false") - .build()); + destructiveOperations.failDestructive(new String[]{"index-*"}); + assertWarnings(DestructiveOperations.DESTRUCTIVE_REQUIRES_NAME_DEPRECATION_WARNING); + } + { + // explicitly set to false: no warning, no failure + clusterSettings.applySettings(Settings.builder() + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), "false") + .build()); - destructiveOperations.failDestructive(new String[]{"index-*"}); + destructiveOperations.failDestructive(new String[]{"index-*"}); + } } private void assertFailsDestructive(String[] aliasesOrIndices) {