-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[data.search] Add Kibana request to search strategy dependencies #98566
[data.search] Add Kibana request to search strategy dependencies #98566
Conversation
This would definitely work for our use case. The solution we had come up with made use of similar flags and it seems like this would be very easy to integrate on our end. Just to add some context for anyone else reading this discussion, as part of the RAC effort we are adding RBAC for alerts. Our auth client works in conjunction with the security plugin and requires that we pass through the entire Kibana request in order for it to determine which features the user has access to. I had been a bit nervous about exposing the internal user flag to end users, so I like what's proposed here of limiting it's scope to other search strategies. I definitely see the use in allowing configuration of dependencies (as opposed to sending everything through by default), but we appreciate this effort of providing a temporary workaround to unblock the RAC efforts here. |
Pinging @elastic/kibana-app-services (Team:AppServices) |
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.
Thank you! that's exactly what we need for moving forward. 🏃♂️ 🏃♂️ ⭐ ⭐ ⭐
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! It allows our team to move forward with RBAC and make the necessary calls using internal Kib user. Thanks for these changes to unblock us!
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
but 5¢:
We eventually want to create some sort of registry that allows search strategies to register which dependencies they actually need, or even have them provide a function that takes in the raw request and returns the list of dependencies. But exposing the raw KibanaRequest might be a good short-term fix to unblock #96912.
I'd probably consider now adding something like getRequestContext
on ISearchStrategy
so that when creating a search strategy consumers specified what exactly they need from raw request.
export const esSearchStrategyProvider = (
config$: Observable<SharedGlobalConfig>,
logger: Logger,
usage?: SearchUsage,
): ISearchStrategy<.... , MyContext> => ({
search: (req, options, context: MyContext) => {...},
getRequestContext: (req: RawKibanaRequest): MyContext => {...}
})
What I think is nice here, is that it will be easier from a high-level view to see that search strategy relies on some specifics from raw request. There will be a single place where raw requests can be used. MyContext
will be typed specific to a search strategy. Search strategies will be a bit easier to refactor. It also will be easier to unit test strategies methods because we will need to mock just MyContext
instead of the whole kibana Request.
This is exactly along the lines of what I was thinking. I'll open an issue for this. |
@elasticmachine merge upstream |
merge conflict between base and head |
@elasticmachine merge upstream |
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
c2c3dcb
to
19aef72
Compare
💚 Build SucceededMetrics [docs]Public APIs missing comments
History
To update your PR or re-run it, just comment with: cc @lukasolson |
…stic#98566) * [data.search] Add Kibana request to search strategy dependencies * Don't register internal strategy * Update docs
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
) (#100425) * [data.search] Add Kibana request to search strategy dependencies * Don't register internal strategy * Update docs Co-authored-by: Lukas Olson <olson.lukas@gmail.com>
…stic#98566) * [data.search] Add Kibana request to search strategy dependencies * Don't register internal strategy * Update docs
Summary
Resolves #96912.
This PR adds the raw
KibanaRequest
object to the list of dependencies for the search strategy, and provides another argument for the ES search strategy provider so that other strategies can opt into using the internal ES client.Thoughts:
KibanaRequest
might be a good short-term fix to unblock [Search Strategy] Include kibana request as a dependency #96912.