-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Integrate with the new resolve_cross_project IndicesOptions #136463
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
base: main
Are you sure you want to change the base?
Conversation
This PR builds on top of elastic#135470 to set the new resolve_cross_project indicesOptions for resolveIndex and search requests when CPS is enabled Relates: ES-13173
Pinging @elastic/es-security (Team:Security) |
Nikolaj and I discussed about this change while reviewing his PR for index expression rewriting. It is among the first few things that we want to get on. IIUC, the intention is to set this new option at the REST layer (@quux00 correct me if I am wrong) and that's what I did. We might need tweak how it configured along the way when more use cases are discovered. I feel it is OK for now. I tagged quite a few folks for reviewing with the intention to keep necessary people updated (still new to hands-on for this project). Please feel free to defer to others if this change is not particular interested for your work. Thanks a lot! |
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.
LGTM Thanks for working on this!
*/ | ||
public class CrossProjectModeDecider { | ||
private static final String CROSS_PROJECT_ENABLED_SETTING_KEY = "serverless.cross_project.enabled"; | ||
private static final org.apache.logging.log4j.Logger log = org.apache.logging.log4j.LogManager.getLogger(CrossProjectModeDecider.class); |
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.
Can we remove the logger as is not used?
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.
Yep it's a debug leftover 🤦
Thanks for catching it. Removed in d85edf3
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.
LGTM 👍
@elasticmachine update branch |
* @param clusterSupportsFeature used to check if certain features are available in this cluster | ||
* @param setSize how the size url parameter is handled. {@code udpate_by_query} and regular search differ here. | ||
* @param searchUsageHolder the holder of search usage stats | ||
* @param crossProjectEnabled whether serverless.cross_project.enabled is set to true |
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.
I just realised this: parseSearchRequest()
can be called in 3 different scenarios:
- For CPS-enabled endpoints and in a CPS context,
- For CPS-enabled endpoints and in a non-CPS context, and,
- For CPS-disabled endpoints.
Therefore, simply setting this param to false
will not help us accurately distinguish between the 2nd and the 3rd options. There are other areas in Elasticsearch where we need to represent such scenarios, and we resorted to using an Optional<Boolean>
. IIUC, we're using it for skip_unavailable
too (see RemoteClusterService
). I actually have a PR where we modified this method to do the same: #135614 (reverted and is pending Breaking Change approval due to some other reasons). Can you accommodate those changes here?
Wdyt @piergm?
This PR builds on top of #135470 to set the new resolve_cross_project indicesOptions for resolveIndex and search requests when CPS is enabled
Relates: ES-13173
Relates: #135346
Relates: #135470