Repository navigation
ESQL: List/get query API - #124832
ESQL: List/get query API#124832
Conversation
d554936 to
58ccb18
Compare
58ccb18 to
be048dc
Compare
be048dc to
8ad4d6d
Compare
|
Hi @GalLalouche, I've created a changelog YAML for you. |
87d1040 to
cb4ec47
Compare
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
|
||
| @Override | ||
| public List<Route> routes() { | ||
| return List.of(new Route(GET, "/_query/queries/{id}"), new Route(GET, "/_query/queries")); |
There was a problem hiding this comment.
The repetition in the endpoint looks surprising to me. Why not /_queries or may be /_cluster/queries?
There was a problem hiding this comment.
For the time being we're looking to keep the ESQL APIs under the ESQL prefix(_query) and avoid potential clashes with other rest APIs.
The decision hasn't been finalized though.
There was a problem hiding this comment.
Let's sort this one out after this PR.
There was a problem hiding this comment.
We can move this - we haven't finalized this API.
costin
left a comment
There was a problem hiding this comment.
Good first PR.
Add a yaml integration rest test with that checks the output for each endpoint, including working and failing tasks
Moving forward, please create a meta PR for this for follow-up issues:
- query cancellation
- filtering (my doc didn't include any, let's use the task API ones by filtering over nodes and id)
- have a subtask to integrate the user in
- double check that X-Opaque-Id /trace.id information is sent over (and displayed if available) - could be used for filtering also
- follow-up item for memory tracking
- follow-up item for bytes / records read
The meta issue will better reflect the overall status including tasks that are blocking and incoming feedback and keep the code get merged in.
|
|
||
| public class EsqlGetQueryAction extends ActionType<EsqlGetQueryResponse> { | ||
| public static final EsqlGetQueryAction INSTANCE = new EsqlGetQueryAction(); | ||
| public static final String NAME = "cluster:monitor/xpack/esql/get_queries"; |
There was a problem hiding this comment.
As a follow-up, please reach out to the data store / security team to double check what type of security is needed (similar to cat/tasks API) both for listing and cancelling queries.
There might a need to add another permission or reuse an existing one.
| return id != null | ||
| ? (channel -> client.execute( | ||
| EsqlGetQueryAction.INSTANCE, | ||
| new EsqlGetQueryRequest(new TaskId(id)), | ||
| new RestToXContentListener<>(channel) | ||
| )) | ||
| : (channel -> client.execute( | ||
| EsqlListQueriesAction.INSTANCE, | ||
| new EsqlListQueriesRequest(), | ||
| new RestToXContentListener<>(channel) | ||
| )); | ||
| } |
There was a problem hiding this comment.
An alternative is to move the conditional when creating the parameters and have only one consumer creation:
ActionType<?> action = id != null ? RestEsqlGetQueryAction.INSTANCE : RestEsqlListQueriesAction.INSTANCE;
Object request = id != null ? new EsqlGetQueryRequest(new TaskId(id)) : new EsqlListQueriesRequest();
return channel -> client.execute(
action,
request,
new RestToXContentListener<>(channel)
);
|
|
||
| @Override | ||
| public List<Route> routes() { | ||
| return List.of(new Route(GET, "/_query/queries/{id}"), new Route(GET, "/_query/queries")); |
There was a problem hiding this comment.
For the time being we're looking to keep the ESQL APIs under the ESQL prefix(_query) and avoid potential clashes with other rest APIs.
The decision hasn't been finalized though.
Done: #125662 |
9a159a4 to
6686702
Compare
jfreden
left a comment
There was a problem hiding this comment.
Thanks for the review request and sorry for the delay!
My understanding is that there are two new actions EsqlGetQueryAction and EsqlListQueriesAction that use the TransportListTasksAction under the hood and filters either on id or on the type of query (esql). From a least privilege principle point of view I think the existing monitor privilege is too wide, since we get more than we need with it. Adding a monitor_esql would probably make sense, in the same way we did for these features.
I think that would require a new esql origin and then register it here, to execute the actual TransportListTasksAction without the API caller having the monitor privilege.
WDYT?
|
Thanks @jfreden! I've added the necessary privileges by following the other ones in the classes you mentioned. Could you PTAL? |
| new ListTasksRequestBuilder(nodeClient).setActions( | ||
| EsqlQueryAction.NAME, | ||
| EsqlQueryAction.NAME + AsyncTaskManagementService.ASYNC_ACTION_SUFFIX | ||
| ).setDetailed(true).execute(new ActionListener<>() { |
There was a problem hiding this comment.
If the caller only has monitor_esql I think this would fail because it doesn't have cluster:monitor/tasks/lists (or cluster:monitor*) that's required here. To work around that the executeAsyncWithOrigin can be used. It would be nice with an integration test that covers this. Maybe modify existing to only have the monitor_esql privilege?
There was a problem hiding this comment.
Nice catch! I fixed it and added a couple of integration tests.
| assertThat(resp.getResponse().getStatusLine().getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST)); | ||
| } | ||
|
|
||
| public void testListQueryAllowed() throws Exception { |
| nodeClient, | ||
| ESQL_ORIGIN, | ||
| TransportGetTaskAction.TYPE, | ||
| new GetTaskRequest().setTaskId(request.id()), |
There was a problem hiding this comment.
Is it possible to validate that this is for EsqlQueryAction or an async EsqlQueryAction? Otherwise you might be able to get any task here? Or maybe I'm missing additional validation?
There was a problem hiding this comment.
Not properly, since we would need said task to have started but not yet finished by the time we ask for it. There are other tests that check that it works, but they use a special hack to inject a semaphore in the middle of executing the query. IMO, combining both checks (semaphore + custom privileges) would be too much of a hassle.
There was a problem hiding this comment.
I think we could check the type of the returned task a few lines down. So if you ask for a non-ESQL task it'd reject it.
jfreden
left a comment
There was a problem hiding this comment.
Thanks for implementing the suggested changes! LGTM!
|
|
||
| @Override | ||
| public List<Route> routes() { | ||
| return List.of(new Route(GET, "/_query/queries/{id}"), new Route(GET, "/_query/queries")); |
There was a problem hiding this comment.
Let's sort this one out after this PR.
|
|
||
| @Override | ||
| public List<Route> routes() { | ||
| return List.of(new Route(GET, "/_query/queries/{id}"), new Route(GET, "/_query/queries")); |
There was a problem hiding this comment.
We can move this - we haven't finalized this API.
| nodeClient, | ||
| ESQL_ORIGIN, | ||
| TransportGetTaskAction.TYPE, | ||
| new GetTaskRequest().setTaskId(request.id()), |
There was a problem hiding this comment.
I think we could check the type of the returned task a few lines down. So if you ask for a non-ESQL task it'd reject it.
This PR adds two new REST endpoints, for listing queries and getting information on a current query.
Changes from the API specified in the above issues:
List queries response:
Get query response: