Skip to content

Commit b4610c8

Browse files
original-brownbearoriginal-brownbear
authored
Remove needless sending of OriginalIndices in SearchFreeContextRequest (elastic#117245)
We don't need to use this request, the handler for freeing of scroll requests literally goes to the same transport handler and doesn't come with the list of indices. The original security need for keeping the list of indices around is long gone.
1 parent 908cf9a commit b4610c8

File tree

10 files changed

+44
-110
lines changed

10 files changed

+44
-110
lines changed

‎server/src/internalClusterTest/java/org/elasticsearch/action/IndicesRequestIT.java‎

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -556,11 +556,7 @@ public void testUpdateSettings() {
556556
}
557557

558558
public void testSearchQueryThenFetch() throws Exception {
559-
interceptTransportActions(
560-
SearchTransportService.QUERY_ACTION_NAME,
561-
SearchTransportService.FETCH_ID_ACTION_NAME,
562-
SearchTransportService.FREE_CONTEXT_ACTION_NAME
563-
);
559+
interceptTransportActions(SearchTransportService.QUERY_ACTION_NAME, SearchTransportService.FETCH_ID_ACTION_NAME);
564560

565561
String[] randomIndicesOrAliases = randomIndicesOrAliases();
566562
for (int i = 0; i < randomIndicesOrAliases.length; i++) {
@@ -580,16 +576,13 @@ public void testSearchQueryThenFetch() throws Exception {
580576
SearchTransportService.QUERY_ACTION_NAME,
581577
SearchTransportService.FETCH_ID_ACTION_NAME
582578
);
583-
// free context messages are not necessarily sent, but if they are, check their indices
584-
assertIndicesSubsetOptionalRequests(Arrays.asList(searchRequest.indices()), SearchTransportService.FREE_CONTEXT_ACTION_NAME);
585579
}
586580

587581
public void testSearchDfsQueryThenFetch() throws Exception {
588582
interceptTransportActions(
589583
SearchTransportService.DFS_ACTION_NAME,
590584
SearchTransportService.QUERY_ID_ACTION_NAME,
591-
SearchTransportService.FETCH_ID_ACTION_NAME,
592-
SearchTransportService.FREE_CONTEXT_ACTION_NAME
585+
SearchTransportService.FETCH_ID_ACTION_NAME
593586
);
594587

595588
String[] randomIndicesOrAliases = randomIndicesOrAliases();
@@ -611,8 +604,6 @@ public void testSearchDfsQueryThenFetch() throws Exception {
611604
SearchTransportService.QUERY_ID_ACTION_NAME,
612605
SearchTransportService.FETCH_ID_ACTION_NAME
613606
);
614-
// free context messages are not necessarily sent, but if they are, check their indices
615-
assertIndicesSubsetOptionalRequests(Arrays.asList(searchRequest.indices()), SearchTransportService.FREE_CONTEXT_ACTION_NAME);
616607
}
617608

618609
private static void assertSameIndices(IndicesRequest originalRequest, String... actions) {

‎server/src/main/java/org/elasticsearch/action/search/AbstractSearchAsyncAction.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -711,7 +711,7 @@ private void raisePhaseFailure(SearchPhaseExecutionException exception) {
711711
try {
712712
SearchShardTarget searchShardTarget = entry.getSearchShardTarget();
713713
Transport.Connection connection = getConnection(searchShardTarget.getClusterAlias(), searchShardTarget.getNodeId());
714-
sendReleaseSearchContext(entry.getContextId(), connection, getOriginalIndices(entry.getShardIndex()));
714+
sendReleaseSearchContext(entry.getContextId(), connection);
715715
} catch (Exception inner) {
716716
inner.addSuppressed(exception);
717717
logger.trace("failed to release context", inner);
@@ -727,10 +727,10 @@ private void raisePhaseFailure(SearchPhaseExecutionException exception) {
727727
* @see org.elasticsearch.search.fetch.FetchSearchResult#getContextId()
728728
*
729729
*/
730-
void sendReleaseSearchContext(ShardSearchContextId contextId, Transport.Connection connection, OriginalIndices originalIndices) {
730+
void sendReleaseSearchContext(ShardSearchContextId contextId, Transport.Connection connection) {
731731
assert isPartOfPointInTime(contextId) == false : "Must not release point in time context [" + contextId + "]";
732732
if (connection != null) {
733-
searchTransportService.sendFreeContext(connection, contextId, originalIndices);
733+
searchTransportService.sendFreeContext(connection, contextId, ActionListener.noop());
734734
}
735735
}
736736

‎server/src/main/java/org/elasticsearch/action/search/DfsQueryPhase.java‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -119,11 +119,7 @@ public void onFailure(Exception exception) {
119119
// the query might not have been executed at all (for example because thread pool rejected
120120
// execution) and the search context that was created in dfs phase might not be released.
121121
// release it again to be in the safe side
122-
context.sendReleaseSearchContext(
123-
querySearchRequest.contextId(),
124-
connection,
125-
context.getOriginalIndices(shardIndex)
126-
);
122+
context.sendReleaseSearchContext(querySearchRequest.contextId(), connection);
127123
}
128124
}
129125
}

‎server/src/main/java/org/elasticsearch/action/search/SearchPhase.java‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -97,11 +97,7 @@ protected static void releaseIrrelevantSearchContext(SearchPhaseResult searchPha
9797
context.getLogger().trace("trying to release search context [{}]", phaseResult.getContextId());
9898
SearchShardTarget shardTarget = phaseResult.getSearchShardTarget();
9999
Transport.Connection connection = context.getConnection(shardTarget.getClusterAlias(), shardTarget.getNodeId());
100-
context.sendReleaseSearchContext(
101-
phaseResult.getContextId(),
102-
connection,
103-
context.getOriginalIndices(phaseResult.getShardIndex())
104-
);
100+
context.sendReleaseSearchContext(phaseResult.getContextId(), connection);
105101
} catch (Exception e) {
106102
context.getLogger().trace("failed to release context", e);
107103
}

‎server/src/main/java/org/elasticsearch/action/search/SearchTransportService.java‎

Lines changed: 7 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,10 @@
1313
import org.apache.logging.log4j.Logger;
1414
import org.elasticsearch.action.ActionListener;
1515
import org.elasticsearch.action.ActionListenerResponseHandler;
16-
import org.elasticsearch.action.IndicesRequest;
1716
import org.elasticsearch.action.OriginalIndices;
1817
import org.elasticsearch.action.admin.cluster.node.tasks.cancel.CancelTasksRequest;
1918
import org.elasticsearch.action.admin.cluster.node.tasks.get.TransportGetTaskAction;
2019
import org.elasticsearch.action.support.ChannelActionListener;
21-
import org.elasticsearch.action.support.IndicesOptions;
2220
import org.elasticsearch.client.internal.OriginSettingClient;
2321
import org.elasticsearch.client.internal.node.NodeClient;
2422
import org.elasticsearch.cluster.node.DiscoveryNode;
@@ -124,24 +122,6 @@ public SearchTransportService(
124122
this.responseWrapper = responseWrapper;
125123
}
126124

127-
private static final ActionListenerResponseHandler<SearchFreeContextResponse> SEND_FREE_CONTEXT_LISTENER =
128-
new ActionListenerResponseHandler<>(
129-
ActionListener.noop(),
130-
SearchFreeContextResponse::readFrom,
131-
TransportResponseHandler.TRANSPORT_WORKER
132-
);
133-
134-
public void sendFreeContext(Transport.Connection connection, final ShardSearchContextId contextId, OriginalIndices originalIndices) {
135-
transportService.sendRequest(
136-
connection,
137-
FREE_CONTEXT_ACTION_NAME,
138-
new SearchFreeContextRequest(originalIndices, contextId),
139-
TransportRequestOptions.EMPTY,
140-
// no need to respond if it was freed or not
141-
SEND_FREE_CONTEXT_LISTENER
142-
);
143-
}
144-
145125
public void sendFreeContext(
146126
Transport.Connection connection,
147127
ShardSearchContextId contextId,
@@ -370,43 +350,6 @@ private static class ClearScrollContextsRequest extends TransportRequest {
370350
}
371351
}
372352

373-
static class SearchFreeContextRequest extends ScrollFreeContextRequest implements IndicesRequest {
374-
private final OriginalIndices originalIndices;
375-
376-
SearchFreeContextRequest(OriginalIndices originalIndices, ShardSearchContextId id) {
377-
super(id);
378-
this.originalIndices = originalIndices;
379-
}
380-
381-
SearchFreeContextRequest(StreamInput in) throws IOException {
382-
super(in);
383-
originalIndices = OriginalIndices.readOriginalIndices(in);
384-
}
385-
386-
@Override
387-
public void writeTo(StreamOutput out) throws IOException {
388-
super.writeTo(out);
389-
OriginalIndices.writeOriginalIndices(originalIndices, out);
390-
}
391-
392-
@Override
393-
public String[] indices() {
394-
if (originalIndices == null) {
395-
return null;
396-
}
397-
return originalIndices.indices();
398-
}
399-
400-
@Override
401-
public IndicesOptions indicesOptions() {
402-
if (originalIndices == null) {
403-
return null;
404-
}
405-
return originalIndices.indicesOptions();
406-
}
407-
408-
}
409-
410353
public static class SearchFreeContextResponse extends TransportResponse {
411354

412355
private static final SearchFreeContextResponse FREED = new SearchFreeContextResponse(true);
@@ -456,12 +399,13 @@ public static void registerRequestHandler(TransportService transportService, Sea
456399
SearchFreeContextResponse::readFrom
457400
);
458401

459-
transportService.registerRequestHandler(
460-
FREE_CONTEXT_ACTION_NAME,
461-
freeContextExecutor,
462-
SearchFreeContextRequest::new,
463-
freeContextHandler
464-
);
402+
// TODO: remove this handler once the lowest compatible version stops using it
403+
transportService.registerRequestHandler(FREE_CONTEXT_ACTION_NAME, freeContextExecutor, in -> {
404+
var res = new ScrollFreeContextRequest(in);
405+
// this handler exists for BwC purposes only, we don't need the original indices to free the context
406+
OriginalIndices.readOriginalIndices(in);
407+
return res;
408+
}, freeContextHandler);
465409
TransportActionProxy.registerProxyAction(transportService, FREE_CONTEXT_ACTION_NAME, false, SearchFreeContextResponse::readFrom);
466410

467411
transportService.registerRequestHandler(

‎server/src/test/java/org/elasticsearch/action/search/AbstractSearchAsyncActionTests.java‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -112,11 +112,7 @@ long buildTookInMillis() {
112112
}
113113

114114
@Override
115-
public void sendReleaseSearchContext(
116-
ShardSearchContextId contextId,
117-
Transport.Connection connection,
118-
OriginalIndices originalIndices
119-
) {
115+
public void sendReleaseSearchContext(ShardSearchContextId contextId, Transport.Connection connection) {
120116
releasedContexts.add(contextId);
121117
}
122118

‎server/src/test/java/org/elasticsearch/action/search/MockSearchPhaseContext.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ protected void executePhaseOnShard(
155155
}
156156

157157
@Override
158-
public void sendReleaseSearchContext(ShardSearchContextId contextId, Transport.Connection connection, OriginalIndices originalIndices) {
158+
public void sendReleaseSearchContext(ShardSearchContextId contextId, Transport.Connection connection) {
159159
releasedSearchContexts.add(contextId);
160160
}
161161

‎server/src/test/java/org/elasticsearch/action/search/SearchAsyncActionTests.java‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,11 @@ public void testFanOutAndCollect() throws InterruptedException {
296296
AtomicInteger numFreedContext = new AtomicInteger();
297297
SearchTransportService transportService = new SearchTransportService(null, null, null) {
298298
@Override
299-
public void sendFreeContext(Transport.Connection connection, ShardSearchContextId contextId, OriginalIndices originalIndices) {
299+
public void sendFreeContext(
300+
Transport.Connection connection,
301+
ShardSearchContextId contextId,
302+
ActionListener<SearchFreeContextResponse> listener
303+
) {
300304
numFreedContext.incrementAndGet();
301305
assertTrue(nodeToContextMap.containsKey(connection.getNode()));
302306
assertTrue(nodeToContextMap.get(connection.getNode()).remove(contextId));
@@ -363,7 +367,7 @@ public void run() {
363367
for (int i = 0; i < results.getNumShards(); i++) {
364368
TestSearchPhaseResult result = results.getAtomicArray().get(i);
365369
assertEquals(result.node.getId(), result.getSearchShardTarget().getNodeId());
366-
sendReleaseSearchContext(result.getContextId(), new MockConnection(result.node), OriginalIndices.NONE);
370+
sendReleaseSearchContext(result.getContextId(), new MockConnection(result.node));
367371
}
368372
responseListener.onResponse(testResponse);
369373
if (latchTriggered.compareAndSet(false, true) == false) {
@@ -421,8 +425,13 @@ public void testFanOutAndFail() throws InterruptedException {
421425
);
422426
AtomicInteger numFreedContext = new AtomicInteger();
423427
SearchTransportService transportService = new SearchTransportService(null, null, null) {
428+
424429
@Override
425-
public void sendFreeContext(Transport.Connection connection, ShardSearchContextId contextId, OriginalIndices originalIndices) {
430+
public void sendFreeContext(
431+
Transport.Connection connection,
432+
ShardSearchContextId contextId,
433+
ActionListener<SearchFreeContextResponse> listener
434+
) {
426435
assertNotNull(contextId);
427436
numFreedContext.incrementAndGet();
428437
assertTrue(nodeToContextMap.containsKey(connection.getNode()));

‎test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@
7878
import java.util.stream.Collectors;
7979
import java.util.stream.Stream;
8080

81-
import static org.elasticsearch.action.search.SearchTransportService.FREE_CONTEXT_ACTION_NAME;
81+
import static org.elasticsearch.action.search.SearchTransportService.FREE_CONTEXT_SCROLL_ACTION_NAME;
8282
import static org.elasticsearch.cluster.coordination.ClusterBootstrapService.INITIAL_MASTER_NODES_SETTING;
8383
import static org.elasticsearch.discovery.SettingsBasedSeedHostsProvider.DISCOVERY_SEED_HOSTS_SETTING;
8484
import static org.elasticsearch.test.NodeRoles.dataNode;
@@ -482,7 +482,7 @@ protected void ensureNoInitializingShards() {
482482
*/
483483
protected void ensureAllFreeContextActionsAreConsumed() throws Exception {
484484
logger.info("--> waiting for all free_context tasks to complete within a reasonable time");
485-
safeGet(clusterAdmin().prepareListTasks().setActions(FREE_CONTEXT_ACTION_NAME + "*").setWaitForCompletion(true).execute());
485+
safeGet(clusterAdmin().prepareListTasks().setActions(FREE_CONTEXT_SCROLL_ACTION_NAME + "*").setWaitForCompletion(true).execute());
486486
}
487487

488488
/**

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,19 @@ public class RBACEngine implements AuthorizationEngine {
129129
private static final String DELETE_SUB_REQUEST_REPLICA = TransportDeleteAction.NAME + "[r]";
130130

131131
private static final Logger logger = LogManager.getLogger(RBACEngine.class);
132+
133+
private static final Set<String> SCROLL_RELATED_ACTIONS = Set.of(
134+
TransportSearchScrollAction.TYPE.name(),
135+
SearchTransportService.FETCH_ID_SCROLL_ACTION_NAME,
136+
SearchTransportService.QUERY_FETCH_SCROLL_ACTION_NAME,
137+
SearchTransportService.QUERY_SCROLL_ACTION_NAME,
138+
SearchTransportService.FREE_CONTEXT_ACTION_NAME,
139+
SearchTransportService.FREE_CONTEXT_SCROLL_ACTION_NAME,
140+
TransportClearScrollAction.NAME,
141+
"indices:data/read/sql/close_cursor",
142+
SearchTransportService.CLEAR_SCROLL_CONTEXTS_ACTION_NAME
143+
);
144+
132145
private final Settings settings;
133146
private final CompositeRolesStore rolesStore;
134147
private final FieldPermissionsCache fieldPermissionsCache;
@@ -320,7 +333,7 @@ public void authorizeIndexAction(
320333
// need to validate that the action is allowed and then move on
321334
listener.onResponse(role.checkIndicesAction(action) ? IndexAuthorizationResult.EMPTY : IndexAuthorizationResult.DENIED);
322335
} else if (request instanceof IndicesRequest == false) {
323-
if (isScrollRelatedAction(action)) {
336+
if (SCROLL_RELATED_ACTIONS.contains(action)) {
324337
// scroll is special
325338
// some APIs are indices requests that are not actually associated with indices. For example,
326339
// search scroll request, is categorized under the indices context, but doesn't hold indices names
@@ -1000,17 +1013,6 @@ public int hashCode() {
10001013
}
10011014
}
10021015

1003-
private static boolean isScrollRelatedAction(String action) {
1004-
return action.equals(TransportSearchScrollAction.TYPE.name())
1005-
|| action.equals(SearchTransportService.FETCH_ID_SCROLL_ACTION_NAME)
1006-
|| action.equals(SearchTransportService.QUERY_FETCH_SCROLL_ACTION_NAME)
1007-
|| action.equals(SearchTransportService.QUERY_SCROLL_ACTION_NAME)
1008-
|| action.equals(SearchTransportService.FREE_CONTEXT_SCROLL_ACTION_NAME)
1009-
|| action.equals(TransportClearScrollAction.NAME)
1010-
|| action.equals("indices:data/read/sql/close_cursor")
1011-
|| action.equals(SearchTransportService.CLEAR_SCROLL_CONTEXTS_ACTION_NAME);
1012-
}
1013-
10141016
private static boolean isAsyncRelatedAction(String action) {
10151017
return action.equals(SubmitAsyncSearchAction.NAME)
10161018
|| action.equals(GetAsyncSearchAction.NAME)

0 commit comments

Comments
 (0)