-
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
[Security Solution][Detections] Adds Bulk edit API #120472
[Security Solution][Detections] Adds Bulk edit API #120472
Conversation
@elasticmachine merge upstream |
merge conflict between base and head |
@elasticmachine merge upstream |
Brainstormed the issue described above with @vitaliidm @xcrzx @banderror. Ideas:
|
@elasticmachine merge upstream |
@banderror , @xcrzx
Kibana request has events.completed$ observable, that will get fired when request either aborted or completed. I've implemented handling of it in one of the commits: ffb568c
Looks like it's what we need, it will return 429, when number of requests exceeds limit. So, this method needs a refactoring:
I working on refactoring and additional checking/testing. Would it be preferable to include rate limit part in this PR or maybe in a separate? |
@elasticmachine merge upstream |
Checked as well APM transactions In patchRules method, rulesClient.update does 4 requests under the hood: authorization check, getting rule and 2 requests for update. With limited rate as 5, for ~ 600 rules, timing for requests are following:
I see median request time increased to 23s from ~15s, in comparison with a single request. Visually, kibana seems slower under this load. But can take requests, and not crushing as previously, without rate limit Further optimization could be:
Having #53144 implemented, looks as the most preferable option in future. |
@vitaliidm Why the response code is 500 with the rate limit enabled? Shouldn't it be 429? |
@xcrzx , I sent only 5 requests, which within rate limit. 500 error was because of conflict for some rules updates If I start run with 10 requests in parallel for example:
5 will be completed with 429 also, it's captured in functional test https://github.com/elastic/kibana/pull/120472/files#diff-033a39080b5edd054aecd3ddfa4a06fc9d7e145e71bda681a8bd3647efb21956R303 |
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.
Tested the _bulk_action
endpoint against concurrent load locally; everything seems to work fine, and Kibana doesn't hang anymore. Request abortion also works as expected. Thank you, Vitalii 👍
I added a small comment about unsubscription, not sure how it should work tbh, so maybe require further investigation.
|
||
// subscribing to completed$, because it handles both cases when request was completed and aborted. | ||
// when route is finished by timeout, aborted$ is not getting fired | ||
request.events.completed$.subscribe(() => abortController.abort()); |
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.
completed$.subscribe()
returns an unsubscribe
method which contains clean up logic. Do we need to call it?
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 think we should be fine and don't need to unsubscribe.
Under the hood completed$
const completed$ = merge<void, void>(finish$, aborted$).pipe(shareReplay(1), first());
If I not mistaken, first
operator will take care of unsubscribing
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: cc @vitaliidm |
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.
approving to satisfy onboarding team block here
Adds edit action to rules _bulk_action API Adresses: elastic/kibana#120472 Preview [here](https://security-docs_1317.docs-preview.app.elstc.co/guide/en/security/master/bulk-actions-rules-api.html).
Adds edit action to rules _bulk_action API Adresses: elastic/kibana#120472 Preview [here](https://security-docs_1317.docs-preview.app.elstc.co/guide/en/security/master/bulk-actions-rules-api.html). (cherry picked from commit 878cb38)
…1651) Adds edit action to rules _bulk_action API Adresses: elastic/kibana#120472 Preview [here](https://security-docs_1317.docs-preview.app.elstc.co/guide/en/security/master/bulk-actions-rules-api.html). (cherry picked from commit 878cb38) Co-authored-by: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com>
Summary
edit
to/rules/_bulk_action
API. Draft documentation is available by [Security Solution][Detections]Rules _bulk_action edit API security-docs#1317/rules/_bulk_action
APIsecurity_solution/server/endpoint/routes/limited_concurrency.ts
tosecurity_solution/server/routes/limited_concurrency.ts
. Cleaned up unused constants in endpoint directoryregisterLimitedConcurrencyRoutes
method: rate limit is applying per route path now, rather than for all routes, as previouslyExample
where PUT_ENCODED_USER:PASSWORD equals to base64 encoded string 'user:password'
Checklist
Delete any items that are not applicable to this PR.
For maintainers