Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Security Solution][Detections] Cleanup usages of old bulk rule CRUD endpoints #126068

Merged
merged 2 commits into from
Mar 23, 2022
Merged

[Security Solution][Detections] Cleanup usages of old bulk rule CRUD endpoints #126068

merged 2 commits into from
Mar 23, 2022

Conversation

xcrzx
Copy link
Contributor

@xcrzx xcrzx commented Feb 21, 2022 •
edited
Loading

Related to: #125574

Summary

  • Replaced usages of outdated bulk actions (enableRulesAction, duplicateRulesAction, etc) with the generic executeRulesBulkAction.
  • Fixed telemetry collection. Now events are sent correctly for all rule activation events, including bulk action (previously, we didn't send telemetry when enabling rules by query).
  • Display success notification for rule actions even if they affect a single rule. Due to in-memory sorting, the modified rule can change its position, and it is not always clear whether the action took effect or not.
  • Fixed multiple inconsistencies in the success/error toast display. We were not showing success toasts for some actions like delete or disable. For other actions like duplicate, we showed toasts when the action was performed by IDs but not by a query.
  • Fixed inconsistencies in toasts wording. All toasts now display title + description indicating the number of affected rules:
    Screenshot 2022-03-15 at 13 51 40

Unused bulk CRUD endpoints

After the cleanup, some API endpoints are not used anymore. We can deprecate them:

  • ${DETECTION_ENGINE_RULES_URL}/_bulk_update
  • ${DETECTION_ENGINE_RULES_URL}/_bulk_delete
  • ${DETECTION_ENGINE_RULES_URL}/_bulk_create

@xcrzx xcrzx added technical debt Improvement of the software architecture and operational architecture Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Monitoring Security Solution Detection Rule Monitoring area Team:Detection Rule Management Security Detection Rule Management Team v8.2.0 labels Feb 21, 2022
@xcrzx xcrzx self-assigned this Feb 21, 2022
@xcrzx xcrzx force-pushed the bulk-actions-cleanup branch 5 times, most recently from 4896bfd to 6ace2de Compare February 23, 2022 17:42
@xcrzx xcrzx force-pushed the bulk-actions-cleanup branch 3 times, most recently from 9500867 to 26081a6 Compare March 8, 2022 16:55
@xcrzx xcrzx force-pushed the bulk-actions-cleanup branch 6 times, most recently from 7eaf46e to ac964fd Compare March 14, 2022 15:27
@xcrzx xcrzx marked this pull request as ready for review March 14, 2022 15:32
@xcrzx xcrzx requested a review from a team as a code owner March 14, 2022 15:32
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@xcrzx xcrzx added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Mar 14, 2022
@xcrzx xcrzx force-pushed the bulk-actions-cleanup branch 2 times, most recently from da3ef3e to 0f1ac6e Compare March 15, 2022 12:51
@xcrzx xcrzx mentioned this pull request Mar 15, 2022
Closed
@xcrzx xcrzx mentioned this pull request Mar 15, 2022
Closed
9 tasks
@xcrzx xcrzx force-pushed the bulk-actions-cleanup branch 2 times, most recently from 1ea64d9 to ba3696a Compare March 21, 2022 09:28
spong
spong reviewed Mar 23, 2022
View reviewed changes
...ns/security_solution/public/detections/components/rules/rule_actions_overflow/index.test.tsx
expect.anything(),
expect.anything()
expect(executeRulesBulkAction).toHaveBeenCalledWith(
expect.objectContaining({ action: 'duplicate', search: { ids: ['id'] } })
);
});
});

test('it calls editRuleAction after the rule is duplicated', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

minor nit: Test titles still using editRuleAction/duplicateRulesAction which are no longer the functions being called here. Tests still read fine/make sense to me, so no change necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I missed that, will rename it in one of my following PRs

spong
spong approved these changes Mar 23, 2022
View reviewed changes
Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Checked out, desk tested locally, and code reviewed -- LGTM! 👍 Thank you so much for all the cleanup here @xcrzx! 🙂 It's soooo much easier to follow the code around these actions now, and makes these actions even easier to consume as well. Just a couple small nits around copy but everything else looks good to me and tested without issue! 🚀

xcrzx and others added 2 commits March 23, 2022 11:30
@xcrzx
Cleanup usages of old bulk CRUD endpoints
9bdb28e
@xcrzx @spong
Apply suggestions from code review
0c850a8 
Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
@xcrzx xcrzx force-pushed the bulk-actions-cleanup branch from 6d2fa2b to 0c850a8 Compare March 23, 2022 10:30
@xcrzx xcrzx enabled auto-merge (squash) March 23, 2022 10:56
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Default Accessibility Tests / Maps app meets ally validations single cancel modal

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 4.8MB 4.8MB +1.3KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 246.6KB 246.5KB -132.0B

History

  • 💛 Build #32078 was flaky ba3696a2c052261b67e46b81f5bb9a7db6a24cd8
  • 💚 Build #31264 succeeded 1ea64d9524cc578098a82ee33670cbcb02b99161
  • 💚 Build #30423 succeeded 0f1ac6e3eb2535f80887ac397c83c2c89d17fa0a
  • 💚 Build #30343 succeeded da3ef3e7fe0498d26fa1ad139e7e13f41936e7d5
  • 💔 Build #30031 failed ac964fd46fef86b1ef4cbe9bfaf21fda3f8125fa
  • 💔 Build #29913 failed 523ec2ddc5279bbd73253d3e728a0a1072d35ad0

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @xcrzx

@xcrzx
Copy link
Contributor Author

xcrzx commented Mar 23, 2022

@elasticmachine merge upstream

@xcrzx xcrzx merged commit 6be9d1e into elastic:main Mar 23, 2022
@banderror banderror mentioned this pull request Apr 11, 2022
Open
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spong spong spong approved these changes

Assignees

@xcrzx xcrzx

Labels
backport:skip This commit does not require backporting Feature:Rule Monitoring Security Solution Detection Rule Monitoring area release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. technical debt Improvement of the software architecture and operational architecture v8.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xcrzx @elasticmachine @kibana-ci @spong