-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add support for project_routing for _search and _async_search
#137566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for project_routing for _search and _async_search
#137566
Conversation
| searchRequest.indicesOptions(indicesOptions); | ||
|
|
||
| validateSearchRequest(request, searchRequest); | ||
| validateSearchRequest(request, searchRequest, crossProjectEnabled); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other endpoints use SearchRequest, too. Should we prohibit them from using project_routing? At the moment, we only look at whether CPS is enabled or not, not if an endpoint supports CPS, so I'm wondering about those endpoints, such as Fleet's.
The query parameter is consumed in the respective Rest*Action-s, so that's fine. It's the presence of the routing parameter in the request's body that I'm thinking about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also search with PIT id should not read/consume project_routing but that's with the PIT changes to make sure it doesn't happen, so we should be good here
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
|
Hi @pawankartik-elastic, I've created a changelog YAML for you. |
piergm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good, left some minor, non-blocking questions I'd like your opinion on.
server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java
Outdated
Show resolved
Hide resolved
| } else if (token.isValue()) { | ||
| if (FROM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { | ||
| if (PROJECT_ROUTING.match(currentFieldName, parser.getDeprecationHandler())) { | ||
| projectRouting(parser.text()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment here for future maintainers that we are going to validate the project routing and maybe throw in validateSearchRequest if it's not a CPS context, otherwise would be strange to see it parsed ok/without errors and an error thrown afterwards IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, making the change in the next push.
quux00
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress!
Some questions and suggestions left.
server/src/main/java/org/elasticsearch/action/search/SearchRequest.java
Outdated
Show resolved
Hide resolved
| ElasticsearchException validationEx = CrossProjectIndexResolutionValidator.validate( | ||
| originalIdxOpts, | ||
| null, | ||
| projectRouting, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@piergm - Can you help me understand why we want projectRouting to be passed into CrossProjectIndexResolutionValidator.validate? This method appears to be called only from Transport Actions, not the SecurityActionFilter and its sole use in the validate method is to help determine whether the index expression is qualified. Why would projectRouting being set mean that we have a qualified expression?
That doesn't seem right to me, but maybe I'm missing a use case here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would projectRouting being set mean that we have a qualified expression?
Consider these indices: foo,bar. Normally, they'd be considered unqualified, and in the flat world, we're fine with wherever they exist. However, if project_routing is specified, say, _alias:_origin, it means we're interested in the results from the origin. So you need to read both the indices and the routing info together.
Here's how it works, using Search as an example: the SAF uses project routing to resolve the projects that are in scope. Say, the routing is _alias:p1. All non-p1 projects are filtered out via CrossProjectRoutingResolver#resolve(), and index expressions are expanded/rewritten using these "resolved" projects. When the coordinator fans out the request, it only does it to those projects that are in scope wrt the routing info, i.e. p1 in this case. So the indices effectively belong to p1, and reading both the unqualified index expressions and the routing together, they can be treated as qualified.
server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java
Outdated
Show resolved
Hide resolved
BASE=7a23516cce48dcd78aed0075a398b604531f1e81 HEAD=121d0f632c058df1d808a022c58eaf2303e8c246 Branch=main
quux00
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress. Two suggested changes.
server/src/main/java/org/elasticsearch/rest/action/search/RestSearchAction.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java
Show resolved
Hide resolved
BASE=e916243b86560e938fa1d2c5f2607c28d7a49dd3 HEAD=0c7ee5d1fa023820437f540cf94528fe48d2488b Branch=main
quux00
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the new TransportVersion and serialized projectRouting is done, LGTM, so approving now.
project_routingcan either be a query parameter or be included in the request's body:At no time should it be set at both places. This PR adds support for it and the subsequent checks.