From 94f06ff79221353e7fc275073c8226317dfafb68 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Tue, 12 Jul 2022 20:37:49 -0400 Subject: [PATCH 1/3] Add transport action immutable state checks --- .../master/TransportMasterNodeAction.java | 33 ++++++ .../TransportMasterNodeActionTests.java | 112 +++++++++++++++++- 2 files changed, 144 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java b/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java index 777b0c2356a72..26f9de8dd380b 100644 --- a/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java +++ b/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java @@ -22,6 +22,7 @@ import org.elasticsearch.cluster.block.ClusterBlock; import org.elasticsearch.cluster.block.ClusterBlockException; import org.elasticsearch.cluster.block.ClusterBlockLevel; +import org.elasticsearch.cluster.metadata.ImmutableStateMetadata; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.node.DiscoveryNodes; @@ -42,7 +43,9 @@ import org.elasticsearch.transport.TransportException; import org.elasticsearch.transport.TransportService; +import java.util.ArrayList; import java.util.Collections; +import java.util.List; import java.util.Optional; import java.util.Set; import java.util.function.Predicate; @@ -172,9 +175,39 @@ protected Set modifiedKeys(Request request) { return Collections.emptySet(); } + // package private for testing + void validateForImmutableState(Request request, ClusterState state) { + Optional handlerName = immutableStateHandlerName(); + assert handlerName.isPresent(); + + Set modified = modifiedKeys(request); + List errors = new ArrayList<>(); + + for (ImmutableStateMetadata metadata : state.metadata().immutableStateMetadata().values()) { + Set conflicts = metadata.conflicts(handlerName.get(), modified); + if (conflicts.isEmpty() == false) { + errors.add(format("[%s] set as read-only by [%s]", String.join(",", conflicts), metadata.namespace())); + } + } + + if (errors.isEmpty() == false) { + throw new IllegalArgumentException( + format("Failed to process request [%s] with errors: %s", request, String.join(System.lineSeparator(), errors)) + ); + } + } + + // package private for testing + boolean supportsImmutableState() { + return immutableStateHandlerName().isPresent(); + } + @Override protected void doExecute(Task task, final Request request, ActionListener listener) { ClusterState state = clusterService.state(); + if (supportsImmutableState()) { + validateForImmutableState(request, state); + } logger.trace("starting processing request [{}] with cluster state version [{}]", request, state.version()); if (task != null) { request.setParentTask(clusterService.localNode().getId(), task.getId()); diff --git a/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java b/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java index 9770b1c42dc0f..5bef002fc3511 100644 --- a/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java @@ -14,12 +14,14 @@ import org.elasticsearch.action.ActionRequestValidationException; import org.elasticsearch.action.ActionResponse; import org.elasticsearch.action.IndicesRequest; +import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.ActionTestUtils; import org.elasticsearch.action.support.IndicesOptions; import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.action.support.ThreadedActionListener; import org.elasticsearch.action.support.replication.ClusterStateCreationUtils; +import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.NotMasterException; import org.elasticsearch.cluster.block.ClusterBlock; @@ -27,6 +29,8 @@ import org.elasticsearch.cluster.block.ClusterBlockLevel; import org.elasticsearch.cluster.block.ClusterBlocks; import org.elasticsearch.cluster.coordination.FailedToCommitClusterStateException; +import org.elasticsearch.cluster.metadata.ImmutableStateHandlerMetadata; +import org.elasticsearch.cluster.metadata.ImmutableStateMetadata; import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.metadata.Metadata; @@ -41,6 +45,7 @@ import org.elasticsearch.common.util.concurrent.EsThreadPoolExecutor; import org.elasticsearch.core.TimeValue; import org.elasticsearch.discovery.MasterNotDiscoveredException; +import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; import org.elasticsearch.indices.TestIndexNameExpressionResolver; import org.elasticsearch.node.NodeClosedException; import org.elasticsearch.rest.RestStatus; @@ -65,6 +70,7 @@ import java.util.HashSet; import java.util.Map; import java.util.Objects; +import java.util.Optional; import java.util.Set; import java.util.concurrent.CountDownLatch; import java.util.concurrent.CyclicBarrier; @@ -254,6 +260,63 @@ protected ClusterBlockException checkBlock(Request request, ClusterState state) } } + class ImmutableStateAction extends Action { + ImmutableStateAction(String actionName, TransportService transportService, ClusterService clusterService, ThreadPool threadPool) { + super(actionName, transportService, clusterService, threadPool, ThreadPool.Names.SAME); + } + + @Override + protected Optional immutableStateHandlerName() { + return Optional.of("test_immutable_state_action"); + } + } + + class FakeClusterStateUpdateAction extends TransportMasterNodeAction { + FakeClusterStateUpdateAction( + String actionName, + TransportService transportService, + ClusterService clusterService, + ThreadPool threadPool, + String executor + ) { + super( + actionName, + transportService, + clusterService, + threadPool, + new ActionFilters(new HashSet<>()), + ClusterUpdateSettingsRequest::new, + TestIndexNameExpressionResolver.newInstance(), + Response::new, + executor + ); + } + + @Override + protected void masterOperation( + Task task, + ClusterUpdateSettingsRequest request, + ClusterState state, + ActionListener listener + ) {} + + @Override + protected ClusterBlockException checkBlock(ClusterUpdateSettingsRequest request, ClusterState state) { + return null; + } + + @Override + protected Optional immutableStateHandlerName() { + return Optional.of(ImmutableClusterSettingsAction.NAME); + } + + @Override + protected Set modifiedKeys(ClusterUpdateSettingsRequest request) { + Settings allSettings = Settings.builder().put(request.persistentSettings()).put(request.transientSettings()).build(); + return allSettings.keySet(); + } + } + public void testLocalOperationWithoutBlocks() throws ExecutionException, InterruptedException { final boolean masterOperationFailure = randomBoolean(); @@ -686,7 +749,6 @@ protected ClusterBlockException checkBlock(Request request, ClusterState state) indexNameExpressionResolver.concreteIndexNamesWithSystemIndexAccess(state, request) ); } - }; PlainActionFuture listener = new PlainActionFuture<>(); @@ -697,6 +759,54 @@ protected ClusterBlockException checkBlock(Request request, ClusterState state) assertThat(ex.getCause().getCause(), instanceOf(ClusterBlockException.class)); } + public void testRejectImmutableConflictClusterStateUpdate() { + ImmutableStateHandlerMetadata hmOne = new ImmutableStateHandlerMetadata(ImmutableClusterSettingsAction.NAME, Set.of("a", "b")); + ImmutableStateHandlerMetadata hmThree = new ImmutableStateHandlerMetadata(ImmutableClusterSettingsAction.NAME, Set.of("e", "f")); + ImmutableStateMetadata omOne = ImmutableStateMetadata.builder("namespace_one").putHandler(hmOne).build(); + ImmutableStateMetadata omTwo = ImmutableStateMetadata.builder("namespace_two").putHandler(hmThree).build(); + + Metadata metadata = Metadata.builder().put(omOne).put(omTwo).build(); + + ClusterState clusterState = ClusterState.builder(new ClusterName("test")).metadata(metadata).build(); + + Action noHandler = new Action("internal:testAction", transportService, clusterService, threadPool, ThreadPool.Names.SAME); + + assertFalse(noHandler.supportsImmutableState()); + + noHandler = new ImmutableStateAction("internal:testOpAction", transportService, clusterService, threadPool); + + assertTrue(noHandler.supportsImmutableState()); + + // nothing should happen here, since the request doesn't touch any of the immutable state keys + noHandler.validateForImmutableState(new Request(), clusterState); + + ClusterUpdateSettingsRequest request = new ClusterUpdateSettingsRequest().persistentSettings( + Settings.builder().put("a", "a value").build() + ).transientSettings(Settings.builder().put("e", "e value").build()); + + FakeClusterStateUpdateAction action = new FakeClusterStateUpdateAction( + "internal:testClusterSettings", + transportService, + clusterService, + threadPool, + ThreadPool.Names.SAME + ); + + assertTrue(action.supportsImmutableState()); + + assertTrue( + expectThrows(IllegalArgumentException.class, () -> action.validateForImmutableState(request, clusterState)).getMessage() + .contains("with errors: [a] set as read-only by [namespace_one]\n" + "[e] set as read-only by [namespace_two]") + ); + + ClusterUpdateSettingsRequest okRequest = new ClusterUpdateSettingsRequest().persistentSettings( + Settings.builder().put("m", "m value").build() + ).transientSettings(Settings.builder().put("n", "n value").build()); + + // this should just work, no conflicts + action.validateForImmutableState(okRequest, clusterState); + } + private Runnable blockAllThreads(String executorName) throws Exception { final int numberOfThreads = threadPool.info(executorName).getMax(); final EsThreadPoolExecutor executor = (EsThreadPoolExecutor) threadPool.executor(executorName); From 617916b9da4d511f565381281fb8ee39660a299e Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Wed, 13 Jul 2022 09:00:37 -0400 Subject: [PATCH 2/3] Change to comma delimited error concatenation --- .../action/support/master/TransportMasterNodeAction.java | 4 ++-- .../action/support/master/TransportMasterNodeActionTests.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java b/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java index 26f9de8dd380b..3c3363e8a0fb6 100644 --- a/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java +++ b/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java @@ -186,13 +186,13 @@ void validateForImmutableState(Request request, ClusterState state) { for (ImmutableStateMetadata metadata : state.metadata().immutableStateMetadata().values()) { Set conflicts = metadata.conflicts(handlerName.get(), modified); if (conflicts.isEmpty() == false) { - errors.add(format("[%s] set as read-only by [%s]", String.join(",", conflicts), metadata.namespace())); + errors.add(format("[%s] set as read-only by [%s]", String.join(", ", conflicts), metadata.namespace())); } } if (errors.isEmpty() == false) { throw new IllegalArgumentException( - format("Failed to process request [%s] with errors: %s", request, String.join(System.lineSeparator(), errors)) + format("Failed to process request [%s] with errors: [%s]", request, String.join(", ", errors)) ); } } diff --git a/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java b/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java index 5bef002fc3511..6119a8da41820 100644 --- a/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java @@ -796,7 +796,7 @@ public void testRejectImmutableConflictClusterStateUpdate() { assertTrue( expectThrows(IllegalArgumentException.class, () -> action.validateForImmutableState(request, clusterState)).getMessage() - .contains("with errors: [a] set as read-only by [namespace_one]\n" + "[e] set as read-only by [namespace_two]") + .contains("with errors: [[a] set as read-only by [namespace_one], " + "[e] set as read-only by [namespace_two]") ); ClusterUpdateSettingsRequest okRequest = new ClusterUpdateSettingsRequest().persistentSettings( From 320ae6de747911ca4cb86b7321d27491be52f437 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 14 Jul 2022 10:48:53 -0400 Subject: [PATCH 3/3] Rebase with master --- .../master/TransportMasterNodeAction.java | 8 +++--- .../TransportMasterNodeActionTests.java | 28 +++++++++---------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java b/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java index 1159ca362490b..24e020b3f76f4 100644 --- a/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java +++ b/server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java @@ -22,8 +22,8 @@ import org.elasticsearch.cluster.block.ClusterBlock; import org.elasticsearch.cluster.block.ClusterBlockException; import org.elasticsearch.cluster.block.ClusterBlockLevel; -import org.elasticsearch.cluster.metadata.ImmutableStateMetadata; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; +import org.elasticsearch.cluster.metadata.ReservedStateMetadata; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.node.DiscoveryNodes; import org.elasticsearch.cluster.service.ClusterService; @@ -178,13 +178,13 @@ protected Set modifiedKeys(Request request) { // package private for testing void validateForImmutableState(Request request, ClusterState state) { - Optional handlerName = immutableStateHandlerName(); + Optional handlerName = reservedStateHandlerName(); assert handlerName.isPresent(); Set modified = modifiedKeys(request); List errors = new ArrayList<>(); - for (ImmutableStateMetadata metadata : state.metadata().immutableStateMetadata().values()) { + for (ReservedStateMetadata metadata : state.metadata().reservedStateMetadata().values()) { Set conflicts = metadata.conflicts(handlerName.get(), modified); if (conflicts.isEmpty() == false) { errors.add(format("[%s] set as read-only by [%s]", String.join(", ", conflicts), metadata.namespace())); @@ -200,7 +200,7 @@ void validateForImmutableState(Request request, ClusterState state) { // package private for testing boolean supportsImmutableState() { - return immutableStateHandlerName().isPresent(); + return reservedStateHandlerName().isPresent(); } @Override diff --git a/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java b/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java index 6119a8da41820..467bd6cd887e8 100644 --- a/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java +++ b/server/src/test/java/org/elasticsearch/action/support/master/TransportMasterNodeActionTests.java @@ -29,11 +29,11 @@ import org.elasticsearch.cluster.block.ClusterBlockLevel; import org.elasticsearch.cluster.block.ClusterBlocks; import org.elasticsearch.cluster.coordination.FailedToCommitClusterStateException; -import org.elasticsearch.cluster.metadata.ImmutableStateHandlerMetadata; -import org.elasticsearch.cluster.metadata.ImmutableStateMetadata; import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.metadata.Metadata; +import org.elasticsearch.cluster.metadata.ReservedStateHandlerMetadata; +import org.elasticsearch.cluster.metadata.ReservedStateMetadata; import org.elasticsearch.cluster.node.DiscoveryNode; import org.elasticsearch.cluster.node.DiscoveryNodeRole; import org.elasticsearch.cluster.node.DiscoveryNodes; @@ -45,9 +45,9 @@ import org.elasticsearch.common.util.concurrent.EsThreadPoolExecutor; import org.elasticsearch.core.TimeValue; import org.elasticsearch.discovery.MasterNotDiscoveredException; -import org.elasticsearch.immutablestate.action.ImmutableClusterSettingsAction; import org.elasticsearch.indices.TestIndexNameExpressionResolver; import org.elasticsearch.node.NodeClosedException; +import org.elasticsearch.reservedstate.action.ReservedClusterSettingsAction; import org.elasticsearch.rest.RestStatus; import org.elasticsearch.tasks.CancellableTask; import org.elasticsearch.tasks.Task; @@ -260,14 +260,14 @@ protected ClusterBlockException checkBlock(Request request, ClusterState state) } } - class ImmutableStateAction extends Action { - ImmutableStateAction(String actionName, TransportService transportService, ClusterService clusterService, ThreadPool threadPool) { + class ReservedStateAction extends Action { + ReservedStateAction(String actionName, TransportService transportService, ClusterService clusterService, ThreadPool threadPool) { super(actionName, transportService, clusterService, threadPool, ThreadPool.Names.SAME); } @Override - protected Optional immutableStateHandlerName() { - return Optional.of("test_immutable_state_action"); + protected Optional reservedStateHandlerName() { + return Optional.of("test_reserved_state_action"); } } @@ -306,8 +306,8 @@ protected ClusterBlockException checkBlock(ClusterUpdateSettingsRequest request, } @Override - protected Optional immutableStateHandlerName() { - return Optional.of(ImmutableClusterSettingsAction.NAME); + protected Optional reservedStateHandlerName() { + return Optional.of(ReservedClusterSettingsAction.NAME); } @Override @@ -760,10 +760,10 @@ protected ClusterBlockException checkBlock(Request request, ClusterState state) } public void testRejectImmutableConflictClusterStateUpdate() { - ImmutableStateHandlerMetadata hmOne = new ImmutableStateHandlerMetadata(ImmutableClusterSettingsAction.NAME, Set.of("a", "b")); - ImmutableStateHandlerMetadata hmThree = new ImmutableStateHandlerMetadata(ImmutableClusterSettingsAction.NAME, Set.of("e", "f")); - ImmutableStateMetadata omOne = ImmutableStateMetadata.builder("namespace_one").putHandler(hmOne).build(); - ImmutableStateMetadata omTwo = ImmutableStateMetadata.builder("namespace_two").putHandler(hmThree).build(); + ReservedStateHandlerMetadata hmOne = new ReservedStateHandlerMetadata(ReservedClusterSettingsAction.NAME, Set.of("a", "b")); + ReservedStateHandlerMetadata hmThree = new ReservedStateHandlerMetadata(ReservedClusterSettingsAction.NAME, Set.of("e", "f")); + ReservedStateMetadata omOne = ReservedStateMetadata.builder("namespace_one").putHandler(hmOne).build(); + ReservedStateMetadata omTwo = ReservedStateMetadata.builder("namespace_two").putHandler(hmThree).build(); Metadata metadata = Metadata.builder().put(omOne).put(omTwo).build(); @@ -773,7 +773,7 @@ public void testRejectImmutableConflictClusterStateUpdate() { assertFalse(noHandler.supportsImmutableState()); - noHandler = new ImmutableStateAction("internal:testOpAction", transportService, clusterService, threadPool); + noHandler = new ReservedStateAction("internal:testOpAction", transportService, clusterService, threadPool); assertTrue(noHandler.supportsImmutableState());