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.

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] Support snoozing in rules table #153083

Merged
merged 40 commits into from
Apr 20, 2023

Conversation

maximpn
Copy link
Contributor

@maximpn maximpn commented Mar 10, 2023

Addresses: #147735

Summary

The PR adds an ability to set rule snoozing in the rules management table.

Screen recording:

Screen.Recording.2023-03-31.at.10.20.09.mov

Checklist

@maximpn maximpn added backport:skip This commit does not require backporting Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. release_note:feature Makes this part of the condensed release notes Feature:Rule Management Security Solution Detection Rule Management area Team:Detection Rule Management Security Detection Rule Management Team v8.8.0 labels Mar 10, 2023
@maximpn maximpn self-assigned this Mar 10, 2023
@maximpn maximpn force-pushed the support-snoozing-in-rules-table branch 4 times, most recently from 2a349de to d362caf Compare March 30, 2023 20:14
@maximpn maximpn marked this pull request as ready for review March 30, 2023 22:02
@maximpn maximpn requested review from a team as code owners March 30, 2023 22:02
@maximpn maximpn requested a review from banderror March 30, 2023 22:02
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@maximpn maximpn force-pushed the support-snoozing-in-rules-table branch from ce1474a to c7df0cd Compare April 4, 2023 07:57
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Hey @maximpn, thanks for working on this feature. I've done some local testing, and overall it looks fine, but there are a bunch of issues and bugs we need to fix.

Please review the issues below and let me know when they're fixed or if you'd like to chat about any of these. I'm going to review the diff later.


On page load, we do 2 calls to /internal/alerting/rules/_find. The first one returns empty results, the second call returns results as expected. What is the reason for this and can we call the endpoint just once?


Why does /internal/alerting/rules/_find return an array of actions?

Screenshot 2023-04-04 at 12 07 16


When you click the Refresh button in the table, we only call /api/detection_engine/rules/_find (we refresh the rules) but not /internal/alerting/rules/_find (we don't refresh the snooze info). We need to do both to make sure the data we show in the table is up-to-date.


Not sure I see consistency in making /internal/alerting/rules/_find calls:

  • After clicking Load Elastic prebuilt rules - YES
  • After deleting 2 prebuilt rules and clicking Install 2 prebuilt rules - NO
  • After selecting filters on the page - sometimes YES and sometimes NO
  • After changing current page - sometimes YES and sometimes NO
  • After changing page size - sometimes YES and sometimes NO

(YES mean we do call the /internal/alerting/rules/_find endpoint, NO means we don't make this call)

Given a rule with no actions. It is snoozed automatically on our side. When you edit this rule and add an action, and return to the Rules table, it continues to be shown as snoozed, although it just got unsnoozed on the BE side. You need to refresh the page to refresh its snooze info.

Same vice versa. Given a rule with some actions and frequency. It is unsnoozed automatically on our side. When you edit this rule and reset its frequency to Perform no actions, and return to the Rules table, it continues to be shown as unsnoozed, although it just got snoozed on the BE side. You need to refresh the page to refresh its snooze info.


There's an issue with z-index when you click remove all schedules.

Screenshot 2023-04-04 at 12 32 30

@maximpn maximpn force-pushed the support-snoozing-in-rules-table branch 4 times, most recently from c931a62 to 4f60502 Compare April 7, 2023 11:05
@maximpn maximpn requested review from a team as code owners April 7, 2023 11:05
@maximpn
Copy link
Contributor Author

maximpn commented Apr 7, 2023

@banderror thank you for thorough testing 🙏 You managed to find some real problems related to cache invalidation and some problems are just side effects so let me break it down

On page load, we do 2 calls to /internal/alerting/rules/_find. The first one returns empty results, the second call returns results as expected. What is the reason for this and can we call the endpoint just once?

It was caused by using two useQuery hooks so the second hook tried to load rules snooze settings for the empty rules list while rules aren't loaded yet. It has been fixed.

When you click the Refresh button in the table, we only call /api/detection_engine/rules/_find (we refresh the rules) but not /internal/alerting/rules/_find (we don't refresh the snooze info). We need to do both to make sure the data we show in the table is up-to-date.

Rule snooze settings refetching upon clicking refresh button was missing and it has been fixed.

Not sure I see consistency in making /internal/alerting/rules/_find calls:
After clicking Load Elastic prebuilt rules - YES
After deleting 2 prebuilt rules and clicking Install 2 prebuilt rules - NO
After selecting filters on the page - sometimes YES and sometimes NO
After changing current page - sometimes YES and sometimes NO
After changing page size - sometimes YES and sometimes NO
(YES mean we do call the /internal/alerting/rules/_find endpoint, NO means we don't make this call)

It works as intended while cache invalidation after deleting 2 prebuilt rules and clicking Install 2 prebuilt rules was missed and it has been fixed. Default caching policy is applied to rules snooze settings data which 5 minutes and the same for the other data we fetch via React Query (besides rules as you can see here #153529). The other cases with sometimes reflect data reusing from the cache .

Given a rule with no actions. It is snoozed automatically on our side. When you edit this rule and add an action, and return to the Rules table, it continues to be shown as snoozed, although it just got unsnoozed on the BE side. You need to refresh the page to refresh its snooze info.

Same vice versa. Given a rule with some actions and frequency. It is unsnoozed automatically on our side. When you edit this rule and reset its frequency to Perform no actions, and return to the Rules table, it continues to be shown as unsnoozed, although it just got snoozed on the BE side. You need to refresh the page to refresh its snooze info.

Cache invalidation upon rule actions saving was missing and it has been fixed.

There's an issue with z-index when you click remove all schedules.

The problem is caused by fixing z-index issues related to the timeline, literally by this line of code. I've been working on resolving this issue. It can take some time and can be optionally addressed in a separate PR to avoid inflating the diff here.

UPDATE: There is a PR to address z-index issues which solves the current problem as well (see the screenshot below). I'll keep an eye on it.

image

@maximpn maximpn force-pushed the support-snoozing-in-rules-table branch 2 times, most recently from 05a15a8 to 466fa71 Compare April 11, 2023 09:14
@maximpn
Copy link
Contributor Author

maximpn commented Apr 11, 2023

@elasticmachine merge upstream

@maximpn maximpn force-pushed the support-snoozing-in-rules-table branch from efeaa9d to 2c0b34b Compare April 19, 2023 21:42
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3825 3828 +3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
alerting 581 582 +1

Async chunks

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

id before after diff
securitySolution 9.1MB 9.1MB +4.3KB
triggersActionsUi 1.4MB 1.4MB +176.0B
total +4.4KB

Page load bundle

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

id before after diff
alerting 48.5KB 48.6KB +94.0B
Unknown metric groups

API count

id before after diff
alerting 602 603 +1

ESLint disabled line counts

id before after diff
securitySolution 394 397 +3

Total ESLint disabled count

id before after diff
securitySolution 474 477 +3

History

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

cc @maximpn

@maximpn maximpn merged commit 0cf7fd1 into elastic:main Apr 20, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 20, 2023
@maximpn maximpn deleted the support-snoozing-in-rules-table branch April 20, 2023 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Rule Management Security Solution Detection Rule Management area release_note:feature Makes this part of the condensed 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. v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants