diff --git a/server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesNodeRequest.java b/server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesNodeRequest.java index 6c1734bde401..514d1caa17fc 100644 --- a/server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesNodeRequest.java +++ b/server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesNodeRequest.java @@ -25,11 +25,12 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; -class FieldCapabilitiesNodeRequest extends ActionRequest implements IndicesRequest { +class FieldCapabilitiesNodeRequest extends ActionRequest implements IndicesRequest.RemoteClusterShardRequest { private final List shardIds; private final String[] fields; @@ -215,4 +216,9 @@ public int hashCode() { result = 31 * result + Arrays.hashCode(allowedTypes); return result; } + + @Override + public Collection shards() { + return shardIds(); + } } diff --git a/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java b/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java index a31593d06a52..be9516c5d733 100644 --- a/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java +++ b/server/src/main/java/org/elasticsearch/action/search/TransportOpenPointInTimeAction.java @@ -47,6 +47,8 @@ import org.elasticsearch.transport.TransportService; import java.io.IOException; +import java.util.Collection; +import java.util.List; import java.util.Map; import java.util.concurrent.Executor; import java.util.function.BiFunction; @@ -280,7 +282,7 @@ boolean buildPointInTimeFromSearchResults() { } } - private static final class ShardOpenReaderRequest extends TransportRequest implements IndicesRequest { + private static final class ShardOpenReaderRequest extends TransportRequest implements IndicesRequest.RemoteClusterShardRequest { final ShardId shardId; final OriginalIndices originalIndices; final TimeValue keepAlive; @@ -319,6 +321,11 @@ public String[] indices() { public IndicesOptions indicesOptions() { return originalIndices.indicesOptions(); } + + @Override + public Collection shards() { + return List.of(getShardId()); + } } private static final class ShardOpenReaderResponse extends SearchPhaseResult { diff --git a/server/src/main/java/org/elasticsearch/action/support/broadcast/unpromotable/BroadcastUnpromotableRequest.java b/server/src/main/java/org/elasticsearch/action/support/broadcast/unpromotable/BroadcastUnpromotableRequest.java index 312a9843c9e2..28703486d322 100644 --- a/server/src/main/java/org/elasticsearch/action/support/broadcast/unpromotable/BroadcastUnpromotableRequest.java +++ b/server/src/main/java/org/elasticsearch/action/support/broadcast/unpromotable/BroadcastUnpromotableRequest.java @@ -20,6 +20,8 @@ import org.elasticsearch.index.shard.ShardId; import java.io.IOException; +import java.util.Collection; +import java.util.List; import java.util.Objects; import static org.elasticsearch.action.ValidateActions.addValidationError; @@ -28,7 +30,7 @@ /** * A request that is broadcast to the unpromotable assigned replicas of a primary. */ -public class BroadcastUnpromotableRequest extends ActionRequest implements IndicesRequest { +public class BroadcastUnpromotableRequest extends ActionRequest implements IndicesRequest.RemoteClusterShardRequest { /** * Holds the index shard routing table that will be used by {@link TransportBroadcastUnpromotableAction} to broadcast the requests to @@ -105,4 +107,9 @@ public boolean failShardOnError() { public IndicesOptions indicesOptions() { return strictSingleIndexNoExpandForbidClosed(); } + + @Override + public Collection shards() { + return List.of(shardId); + } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/termsenum/action/NodeTermsEnumRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/termsenum/action/NodeTermsEnumRequest.java index 84a28d9c8d1e..42eba2e838a3 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/termsenum/action/NodeTermsEnumRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/termsenum/action/NodeTermsEnumRequest.java @@ -19,6 +19,7 @@ import org.elasticsearch.transport.TransportRequest; import java.io.IOException; +import java.util.Collection; import java.util.Collections; import java.util.Set; @@ -26,7 +27,7 @@ * Internal terms enum request executed directly against a specific node, querying potentially many * shards in one request */ -public class NodeTermsEnumRequest extends TransportRequest implements IndicesRequest { +public class NodeTermsEnumRequest extends TransportRequest implements IndicesRequest.RemoteClusterShardRequest { private final String field; private final String string; @@ -181,4 +182,9 @@ public IndicesOptions indicesOptions() { public boolean remove(ShardId shardId) { return shardIds.remove(shardId); } + + @Override + public Collection shards() { + return shardIds(); + } } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequest.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequest.java index ab2df4a2ba6a..e467157373a7 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequest.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/DataNodeRequest.java @@ -32,11 +32,12 @@ import org.elasticsearch.xpack.esql.session.EsqlConfiguration; import java.io.IOException; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Objects; -final class DataNodeRequest extends TransportRequest implements IndicesRequest { +final class DataNodeRequest extends TransportRequest implements IndicesRequest.RemoteClusterShardRequest { private static final PlanNameRegistry planNameRegistry = new PlanNameRegistry(); private final String sessionId; private final EsqlConfiguration configuration; @@ -140,6 +141,11 @@ List shardIds() { return shardIds; } + @Override + public Collection shards() { + return shardIds(); + } + /** * Returns a map from index UUID to alias filters */ @@ -179,4 +185,5 @@ public boolean equals(Object o) { public int hashCode() { return Objects.hash(sessionId, configuration, clusterAlias, shardIds, aliasFilters, plan); } + } diff --git a/x-pack/plugin/security/qa/consistency-checks/src/test/java/org/elasticsearch/xpack/security/CrossClusterShardTests.java b/x-pack/plugin/security/qa/consistency-checks/src/test/java/org/elasticsearch/xpack/security/CrossClusterShardTests.java index 9283c5ae4a43..f849ff49c4d2 100644 --- a/x-pack/plugin/security/qa/consistency-checks/src/test/java/org/elasticsearch/xpack/security/CrossClusterShardTests.java +++ b/x-pack/plugin/security/qa/consistency-checks/src/test/java/org/elasticsearch/xpack/security/CrossClusterShardTests.java @@ -13,6 +13,7 @@ import org.elasticsearch.common.inject.TypeLiteral; import org.elasticsearch.datastreams.DataStreamsPlugin; import org.elasticsearch.index.rankeval.RankEvalPlugin; +import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.ingest.IngestTestPlugin; import org.elasticsearch.ingest.common.IngestCommonPlugin; import org.elasticsearch.node.Node; @@ -47,40 +48,24 @@ import java.lang.reflect.Parameter; import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; +import java.lang.reflect.TypeVariable; import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Set; +import java.util.stream.Collectors; /** * This test helps to ensure that RCS 2.0 transport actions and transport request handlers are correctly marked with the * IndicesRequest.RemoteClusterShardRequest interface. This interface is used to identify transport actions and request handlers * that operate on shards directly and can be used across clusters. This test will fail if a new transport action or request handler * is added that operates on shards directly and is not marked with the IndicesRequest.RemoteClusterShardRequest interface. - * This is a best effort and no guarantee that all transport actions and request handlers are correctly marked. + * This is a best effort and not a guarantee that all transport actions and request handlers are correctly marked. */ public class CrossClusterShardTests extends ESSingleNodeTestCase { - Set IGNORED_ACTIONS = Set.of( - // TODO: go through this list and add comment or add interface - "org.elasticsearch.xpack.profiling.action.TransportGetStackTracesAction", - "org.elasticsearch.action.search.TransportSearchAction", - "org.elasticsearch.xpack.rollup.action.TransportRollupSearchAction.TransportHandler", - "org.elasticsearch.xpack.profiling.action.TransportGetFlamegraphAction", - "org.elasticsearch.action.fieldcaps.TransportFieldCapabilitiesAction.NodeTransportHandler", - "org.elasticsearch.xpack.rollup.action.TransportRollupSearchAction", - "org.elasticsearch.action.search.TransportOpenPointInTimeAction.ShardOpenReaderRequestHandler", - "org.elasticsearch.xpack.profiling.action.TransportGetTopNFunctionsAction", - "org.elasticsearch.action.support.broadcast.unpromotable.TransportBroadcastUnpromotableAction.UnpromotableTransportHandler", - "org.elasticsearch.xpack.esql.plugin.ComputeService.DataNodeRequestHandler", - "org.elasticsearch.action.search.TransportOpenPointInTimeAction", - "org.elasticsearch.xpack.core.termsenum.action.TransportTermsEnumAction.NodeTransportHandler", - "org.elasticsearch.action.admin.indices.validate.query.TransportValidateQueryAction", - "org.elasticsearch.action.support.broadcast.node.TransportBroadcastByNodeAction.BroadcastByNodeTransportRequestHandler" - - ); @Override protected Collection> getPlugins() { @@ -133,10 +118,11 @@ public void testCheckForNewShardLevelTransportActions() throws Exception { for (Class clazz : indicesRequest) { for (Method method : clazz.getDeclaredMethods()) { // not the most efficient way to check for shard related methods, but it's good enough for this test - // alternatively we could check methods that return some collection of ShardId or similar - // really we need a better way to inspect the request/action relationship if (method.getName().toLowerCase(Locale.ROOT).contains("shard")) { - candidateRequests.add(method.getDeclaringClass().getCanonicalName()); + // only care if the return type is a ShardId or a collection of ShardIds + if(ShardId.class.getCanonicalName().equals(getTypeFromMaybeGeneric(method.getGenericReturnType()))) { + candidateRequests.add(method.getDeclaringClass().getCanonicalName()); + } } } } @@ -157,7 +143,7 @@ public void testCheckForNewShardLevelTransportActions() throws Exception { // Find any transport actions that have methods related to shards, these are the candidate actions for the marker interface for (TransportAction transportAction : candidateActions) { - String actionRequestType = getRequestTypeName(transportAction.getClass().getGenericSuperclass()); + String actionRequestType = getTypeFromMaybeGeneric(transportAction.getClass().getGenericSuperclass()); if (candidateRequests.contains(actionRequestType)) { actionsWithShardRequests.add(new FinalCandidate(transportAction.getClass().getCanonicalName(), actionRequestType)); } @@ -178,48 +164,36 @@ public void testCheckForNewShardLevelTransportActions() throws Exception { } } - // Sanity check that this test actually needs to ignore the actions it lists - for (String ignoredAction : IGNORED_ACTIONS) { - if (actionsWithShardRequests.stream().noneMatch(action -> action.actionClassName.equals(ignoredAction))) { - fail( - String.format( - "The action [%s] is in the IGNORED_ACTIONS list but is not needed. " - + "Please remove [%s] from the IGNORED_ACTIONS.", - ignoredAction, ignoredAction - ) - ); - } - } - - // filter out any ignored actions - actionsWithShardRequests.removeIf(action -> IGNORED_ACTIONS.contains(action.actionClassName)); - if (actionsWithShardRequests.isEmpty() == false) { fail(String.format(""" - This test failed. You likely just added an index level transport action or transport request handler with an associated - transport request with `shard` in a method name. Transport actions or transport request handlers which - operate on shards directly and can be used across clusters must meet some additional requirements in order to be - handled correctly by all Elasticsearch infrastructure, so please make sure you have read the javadoc on the - IndicesRequest.RemoteClusterShardRequest interface and implemented. If this does apply to your change, please add the - canonical class name to the set of IGNORED_ACTIONS in this test. Failed action (request): %s - """, actionsWithShardRequests)); + This test failed. You likely just added an index level transport action(s) or transport request handler(s) + [%s] + with an associated TransportRequest with `shard` in a method name. Transport actions or transport request handlers which + operate directly on shards and can be used across clusters must meet some additional requirements in order to be + handled correctly by the Elasticsearch security infrastructure. Please review the javadoc for + IndicesRequest.RemoteClusterShardRequest and implement the interface on the transport request(s) + [%s] + """, actionsWithShardRequests.stream().map(FinalCandidate::actionClassName).collect(Collectors.joining(", ")), + actionsWithShardRequests.stream().map(FinalCandidate::requestClassName).collect(Collectors.joining(", ")))); } } - private static String getRequestTypeName(Type type) { + /** + * @return The canonical class name of the first parameter type of a generic type, + * or the canonical class name of the class if it's not a generic type + */ + private static String getTypeFromMaybeGeneric(Type type) { if (type instanceof ParameterizedType parameterizedType) { Type[] typeArguments = parameterizedType.getActualTypeArguments(); - return getRequestTypeName(typeArguments[0]); + return getTypeFromMaybeGeneric(typeArguments[0]); + } else if (type instanceof TypeVariable) { + //too complex to handle this case, and is likely a CRTP pattern which we will catch the children of this class + return ""; } else if (type instanceof Class) { return ((Class) type).getCanonicalName(); } - throw new RuntimeException("Unknown type: " + type); + throw new RuntimeException("Unknown type: " + type.getClass()); } - private record FinalCandidate(String actionClassName, String requestClassName) { - @Override - public String toString() { - return actionClassName + " (" + requestClassName + ")"; - } - } + private record FinalCandidate(String actionClassName, String requestClassName) {} }