Skip to content

Conversation

@dnhatn
Copy link
Member

@dnhatn dnhatn commented Mar 20, 2023

This pull request introduces a new search shards endpoint that returns a list of shards where a search request will be executed. Unlike the existing cluster search_shards API, this API performs the can_match phase on both the coordinator and data nodes, which excludes non-matching shards from the results.

Once this pull request is merged, I will work on a follow-up to switch over to using this API in CCS search.

Relates #93730

@dnhatn dnhatn added >enhancement :Search/Search Search-related issues that do not fall into other categories v8.8.0 labels Mar 20, 2023
@elasticsearchmachine
Copy link
Collaborator

Hi @dnhatn, I've created a changelog YAML for you.

@dnhatn dnhatn changed the title Add search_shards endpoint Add search shards endpoint Mar 20, 2023
@elasticsearchmachine
Copy link
Collaborator

Hi @dnhatn, I've created a changelog YAML for you.

@dnhatn dnhatn marked this pull request as ready for review March 20, 2023 05:00
@dnhatn dnhatn requested a review from javanna March 20, 2023 05:00
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Mar 20, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@dnhatn dnhatn changed the title Add search shards endpoint Add new search shards endpoint Mar 20, 2023
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple of comments, while we decide what to do about privileges.

@dnhatn
Copy link
Member Author

dnhatn commented Apr 18, 2023

While the view_index_metadata privilege would suffice, and we could extend the current API to support can_match, we decided to go with the new transport action after discussing with Luca. This is because we don't want to add the query to an API that may be deprecated in the future.

@dnhatn
Copy link
Member Author

dnhatn commented Apr 19, 2023

@javanna I've updated this PR to call the can_match phase directly. However, I intentionally left out support for PIT to keep this PR small. I plan to add it in a follow-up. Can you please take a look? Thank you!

@dnhatn dnhatn requested a review from javanna April 19, 2023 05:38
@dnhatn
Copy link
Member Author

dnhatn commented May 5, 2023

@ywangd Thank you so much for your help regarding the privilege. Our discussion was great, and based on your suggestions, I have made these two changes:

  1. I have changed the transport action name to indices:admin/search/search_shards. This ensures that the action now requires the manage_index privilege.

  2. I have registered this action under the read_cross_cluster privilege group. As a result, users will now need to have the read_cross_cluster privilege to call this API across clusters.

As a result of these changes, this API now requires the same privilege as the pre-existing cluster_search_shards API.

Would you mind reviewing these changes? Thank you!

@dnhatn dnhatn requested a review from ywangd May 5, 2023 01:16
Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Nhat, thanks for the discussion. I appreciate you spending time reaching out to us on this topic. The action and privileges look right to me. 👍

@dnhatn
Copy link
Member Author

dnhatn commented May 5, 2023

@javanna @ywangd Thank you so much for reviews.

@dnhatn dnhatn merged commit 23e0ad3 into elastic:main May 5, 2023
@dnhatn dnhatn deleted the search-shards branch May 5, 2023 05:52
dnhatn added a commit that referenced this pull request May 9, 2023
While integrating the new search_shards API with CCS, I found that we had forgotten to add it to the allow list in the CrossClusterAccessServerTransportFilter so that we can call it in CCS. Also, I added the system privilege to this API so it can be called with the system user in field-caps and other APIs. Since we have many tests that will cover this API once it is integrated, I don't add any security-related tests specifically for this API in this PR. Relates #94534
dnhatn added a commit that referenced this pull request May 9, 2023
We should use the search_coordinator threadpool when performing the can_match phase in the new search_shards API. Relates #94534
dnhatn added a commit that referenced this pull request May 9, 2023
While integrating the new search_shards API with CCS, I encountered an issue where the search_shards operation returned a wrong result when using the _index:cluster:index_name filter. The reason for this failure is that we are not sending the cluster alias to the search_shards API, resulting in the search context lacking a cluster alias to match the index pattern filter. Relates #94534
dnhatn added a commit that referenced this pull request May 17, 2023
The preFiltered flag aimed to facilitate a smooth transition from the old response to the new response. However, instead of serializing this flag over the wire, we prefer an alternative approach. Since the new API has not been used thus far, there is no need to handle backward compatibility or introduce a new transport version. Relates #94534
dnhatn added a commit that referenced this pull request May 17, 2023
If the query cannot be rewritten to match_none, then search_shards API should skip the can_match phase and return all groups of shard copies. Relates #94534
dnhatn added a commit that referenced this pull request May 17, 2023
This PR integrates CCS with the new search_shards API. With this change, we will be able to skip shards on the coordinator on remote clusters using the timestamps stored in the cluster state. Relates #94534 Closes #93730
dnhatn added a commit that referenced this pull request Jun 1, 2023
When security is enabled, CCS requests for a remote wildcard pointing data streams are not resolved correctly. This issue happens because search_shards requests do not override the includeDataStreams method, causing the RBAC engine to exclude data streams while resolving the index pattern. Relates #95894 Relates #94534
floragunncom pushed a commit to floragunncom/search-guard that referenced this pull request Mar 3, 2024
floragunncom pushed a commit to floragunncom/search-guard that referenced this pull request Mar 3, 2024
floragunncom pushed a commit to floragunncom/search-guard that referenced this pull request Mar 3, 2024
floragunncom pushed a commit to floragunncom/search-guard that referenced this pull request Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>enhancement :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v8.9.0

5 participants