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] Fix rules filtering after enabling/disabling a rule #151284

Merged

Conversation

maximpn
Copy link
Contributor

@maximpn maximpn commented Feb 15, 2023

Addresses: #151151

Summary

It fixes rules filtering after enabling or disabling a rule.

Details

The problem is caused by improper cache invalidation. Rules cache used to be modified upon enabling or disabling one or more rules but it started causing troubles after introduction a filter by enabled or disabled state. Cached rules modification is is complex and bug prone especially taking into account it will need to mirror backend logic and further plans on extending rule filers. So the simplest solution is invalidation of the whole rules cache. Though it may also lead to unfriendly UX when disabled or enabled rules "jump" in the table. The best approach is marking find rule request cached data as stale so data is refetched each time use changes filter state, sort by field or use pagination.

Before:

Screen.Recording.2023-02-14.at.16.04.27.mov

After:

Screen.Recording.2023-02-17.at.11.59.43.mov

Checklist

@maximpn maximpn added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes 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. Team:Detection Rule Management Security Detection Rule Management Team v8.7.0 labels Feb 15, 2023
@maximpn maximpn self-assigned this Feb 15, 2023
@maximpn maximpn force-pushed the fix-rules-filtering-after-enabling-or-disabling branch from c214b7f to f8247be Compare February 17, 2023 10:51
@maximpn maximpn marked this pull request as ready for review February 17, 2023 11:02
@maximpn maximpn requested a review from a team as a code owner February 17, 2023 11:02
@maximpn maximpn requested a review from banderror February 17, 2023 11: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
Copy link
Contributor Author

maximpn commented Feb 20, 2023

@elasticmachine merge upstream

Comment on lines +57 to +59
// Mark this query as immediately stale helps to avoid problems related to filtering.
// e.g. enabled and disabled state filter require data update which happens at the backend side
staleTime: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix, @maximpn. And appreciate the comment here.
Could you please also add staleTime: 0 to the useFetchRuleByIdQuery hook? So we'll be sure no stale data is displayed on the rule detail page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@maximpn maximpn force-pushed the fix-rules-filtering-after-enabling-or-disabling branch from eb31e9d to 895564e Compare February 20, 2023 15:01
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #4 / Inspect Users stats and tables inspects authentications table
  • [job] [logs] FTR Configs #29 / logstash pipelines list "before all" hook: load pipelines archive for "should return all the pipelines"

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 13.7MB 13.7MB +96.0B

History

  • 💛 Build #108888 was flaky eb31e9d61439b5ed0420136074c9addbafd3b79c
  • 💚 Build #108830 succeeded f8247be335d0a6207eb340212b00bcebfc087328
  • 💔 Build #108185 failed c214b7fdaa886127abf48b48e2d4fc05180243ad

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

cc @maximpn

@maximpn maximpn requested a review from xcrzx February 20, 2023 16:43
Copy link
Contributor

@xcrzx xcrzx left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the fix, @maximpn 👍

@maximpn maximpn merged commit 9683beb into elastic:main Feb 21, 2023
@banderror banderror removed the backport:skip This commit does not require backporting label Feb 22, 2023
@maximpn maximpn added fixed and removed fixed labels Feb 22, 2023
@maximpn
Copy link
Contributor Author

maximpn commented Feb 22, 2023

💚 All backports created successfully

Status Branch Result
8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

maximpn added a commit to maximpn/kibana that referenced this pull request Feb 22, 2023
…le (elastic#151284)

**Addresses:** elastic#151151

## Summary

It fixes rules filtering after enabling or disabling a rule.

### Details

The problem is caused by improper cache invalidation. Rules cache used to be modified upon enabling or disabling one or more rules but it started causing troubles after introduction a filter by enabled or disabled state. Cached rules modification is  is complex and bug prone especially taking into account it will need to mirror backend logic and further plans on extending rule filers. So the simplest solution is invalidation of the whole rules cache. Though it may also lead to unfriendly UX when disabled or enabled rules "jump" in the table. The best approach is marking find rule request cached data as stale so data is refetched each time use changes filter state, sort by field or use pagination.

**Before:**

https://user-images.githubusercontent.com/1938181/218776621-f8903a88-1685-4a2c-9074-02fac0623dc4.mov

**After:**

https://user-images.githubusercontent.com/3775283/219630525-af109575-3a01-4988-bb6b-690473d33b80.mov

### Checklist

- [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

(cherry picked from commit 9683beb)
@banderror banderror added impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. Feature:Rule Management Security Solution Detection Rule Management area labels Feb 22, 2023
maximpn added a commit that referenced this pull request Feb 24, 2023
…g a rule (#151284) (#151861)

# Backport

This will backport the following commits from `main` to `8.7`:
- [[Security Solution] Fix rules filtering after enabling/disabling a
rule (#151284)](#151284)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Maxim
Palenov","email":"maxim.palenov@elastic.co"},"sourceCommit":{"committedDate":"2023-02-21T09:43:25Z","message":"[Security
Solution] Fix rules filtering after enabling/disabling a rule
(#151284)\n\n**Addresses:**
https://github.com/elastic/kibana/issues/151151\r\n\r\n##
Summary\r\n\r\nIt fixes rules filtering after enabling or disabling a
rule.\r\n\r\n### Details\r\n\r\nThe problem is caused by improper cache
invalidation. Rules cache used to be modified upon enabling or disabling
one or more rules but it started causing troubles after introduction a
filter by enabled or disabled state. Cached rules modification is is
complex and bug prone especially taking into account it will need to
mirror backend logic and further plans on extending rule filers. So the
simplest solution is invalidation of the whole rules cache. Though it
may also lead to unfriendly UX when disabled or enabled rules \"jump\"
in the table. The best approach is marking find rule request cached data
as stale so data is refetched each time use changes filter state, sort
by field or use
pagination.\r\n\r\n**Before:**\r\n\r\nhttps://user-images.githubusercontent.com/1938181/218776621-f8903a88-1685-4a2c-9074-02fac0623dc4.mov\r\n\r\n**After:**\r\n\r\nhttps://user-images.githubusercontent.com/3775283/219630525-af109575-3a01-4988-bb6b-690473d33b80.mov\r\n\r\n\r\n###
Checklist\r\n\r\n- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common
scenarios","sha":"9683beba6af5f78fa88350aa5bcab95d767cd763","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:Detections
and Resp","Team: SecuritySolution","Team:Detection
Rules","v8.7.0","v8.8.0"],"number":151284,"url":"https://github.com/elastic/kibana/pull/151284","mergeCommit":{"message":"[Security
Solution] Fix rules filtering after enabling/disabling a rule
(#151284)\n\n**Addresses:**
https://github.com/elastic/kibana/issues/151151\r\n\r\n##
Summary\r\n\r\nIt fixes rules filtering after enabling or disabling a
rule.\r\n\r\n### Details\r\n\r\nThe problem is caused by improper cache
invalidation. Rules cache used to be modified upon enabling or disabling
one or more rules but it started causing troubles after introduction a
filter by enabled or disabled state. Cached rules modification is is
complex and bug prone especially taking into account it will need to
mirror backend logic and further plans on extending rule filers. So the
simplest solution is invalidation of the whole rules cache. Though it
may also lead to unfriendly UX when disabled or enabled rules \"jump\"
in the table. The best approach is marking find rule request cached data
as stale so data is refetched each time use changes filter state, sort
by field or use
pagination.\r\n\r\n**Before:**\r\n\r\nhttps://user-images.githubusercontent.com/1938181/218776621-f8903a88-1685-4a2c-9074-02fac0623dc4.mov\r\n\r\n**After:**\r\n\r\nhttps://user-images.githubusercontent.com/3775283/219630525-af109575-3a01-4988-bb6b-690473d33b80.mov\r\n\r\n\r\n###
Checklist\r\n\r\n- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common
scenarios","sha":"9683beba6af5f78fa88350aa5bcab95d767cd763"}},"sourceBranch":"main","suggestedTargetBranches":["8.7"],"targetPullRequestStates":[{"branch":"8.7","label":"v8.7.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/151284","number":151284,"mergeCommit":{"message":"[Security
Solution] Fix rules filtering after enabling/disabling a rule
(#151284)\n\n**Addresses:**
https://github.com/elastic/kibana/issues/151151\r\n\r\n##
Summary\r\n\r\nIt fixes rules filtering after enabling or disabling a
rule.\r\n\r\n### Details\r\n\r\nThe problem is caused by improper cache
invalidation. Rules cache used to be modified upon enabling or disabling
one or more rules but it started causing troubles after introduction a
filter by enabled or disabled state. Cached rules modification is is
complex and bug prone especially taking into account it will need to
mirror backend logic and further plans on extending rule filers. So the
simplest solution is invalidation of the whole rules cache. Though it
may also lead to unfriendly UX when disabled or enabled rules \"jump\"
in the table. The best approach is marking find rule request cached data
as stale so data is refetched each time use changes filter state, sort
by field or use
pagination.\r\n\r\n**Before:**\r\n\r\nhttps://user-images.githubusercontent.com/1938181/218776621-f8903a88-1685-4a2c-9074-02fac0623dc4.mov\r\n\r\n**After:**\r\n\r\nhttps://user-images.githubusercontent.com/3775283/219630525-af109575-3a01-4988-bb6b-690473d33b80.mov\r\n\r\n\r\n###
Checklist\r\n\r\n- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common
scenarios","sha":"9683beba6af5f78fa88350aa5bcab95d767cd763"}}]}]
BACKPORT-->

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Rule Management Security Solution Detection Rule Management area impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. 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. v8.7.0 v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants