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] Update cache invalidation logic to handle error responses #146271

Merged
merged 1 commit into from
Nov 28, 2022
Merged

[Security Solution] Update cache invalidation logic to handle error responses #146271

merged 1 commit into from
Nov 28, 2022

Conversation

xcrzx
Copy link
Contributor

@xcrzx xcrzx commented Nov 24, 2022 •
edited
Loading

Resolves: #146277

Summary

Previously, we invalidated the rules table cache only after successful server-side state mutations. So when an action like bulk edit was successfully updating some rules and failing for others, the table continued showing outdated results.

This PR moves the cache invalidation from the onSuccess handlers to the onSettled handlers to prevent showing partially stale data after failed updates.

@xcrzx xcrzx added bug Fixes for quality problems that affect the customer experience 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.6.0 v8.7.0 labels Nov 24, 2022
@xcrzx xcrzx self-assigned this Nov 24, 2022
@xcrzx xcrzx marked this pull request as ready for review November 24, 2022 12:01
@xcrzx xcrzx requested a review from a team as a code owner November 24, 2022 12:01
@xcrzx xcrzx requested a review from jpdjere November 24, 2022 12:01
@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:fix auto-backport Deprecated: Automatically backport this PR after it's merged labels Nov 24, 2022
@banderror banderror added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. Feature:Rule Management Security Solution Detection Rule Management labels Nov 24, 2022
jpdjere
jpdjere approved these changes Nov 24, 2022
View reviewed changes
Copy link
Contributor

@jpdjere jpdjere left a comment

Choose a reason for hiding this comment

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

Tested locally and works as expected now.

Left a question about moving types on which I would like your opinion, but otherwise LGTM 👍

x-pack/plugins/security_solution/public/detection_engine/rule_management/api/api.ts
@@ -206,14 +206,22 @@ export interface BulkActionAggregatedError {
rules: Array<{ id: string; name?: string }>;
}

export interface BulkActionAttributes {
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 adding these types and discriminating between successful and error responses 👍

Only thing: I also added these types for the bulk edit response while working on the skipped rules PR. Do you think it makes sense to move them to the /common folder and then import them to both server and public?

https://github.com/elastic/kibana/pull/144461/files#diff-0bddfc1ba78df99099e714716ea1bb5d969c5d6da65946d2f1cbb964382f2405R34

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpdjere Yes, if the types are the same on the front end and server side, it makes sense to move them to the common folder. Ideally, we should put there a runtime io-ts schema, which could be used for response validation, and derive the type from it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Allright, if we get merged this before than my PR I can update it on my side and refactor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allright, if we get merged this before than my PR I can update it on my side and refactor.

Yes, as this one targets 8.6, it would be safer to do refactoring in the main. Thank you, @jpdjere 👍

@xcrzx xcrzx enabled auto-merge (squash) November 28, 2022 10:28
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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 9.6MB 9.6MB +48.0B
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 59 65 +6
osquery 109 115 +6
securitySolution 443 449 +6
total +20

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 68 74 +6
osquery 110 117 +7
securitySolution 520 526 +6
total +21

History

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

cc @xcrzx

@xcrzx xcrzx merged commit e9bc603 into elastic:main Nov 28, 2022
@kibanamachine kibanamachine mentioned this pull request Nov 28, 2022
Merged
kibanamachine pushed a commit that referenced this pull request Nov 28, 2022
@xcrzx
[Security Solution] Update cache invalidation logic to handle error r…
ce932d0 
…esponses (#146271)

**Resolves: #146277

## Summary

Previously, we invalidated the rules table cache only after successful
server-side state mutations. So when an action like bulk edit was
successfully updating some rules and failing for others, the table
continued showing outdated results.

This PR moves the cache invalidation from the `onSuccess` handlers to
the `onSettled` handlers to prevent showing partially stale data after
failed updates.

(cherry picked from commit e9bc603)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.6

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

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

@xcrzx xcrzx deleted the cache-invalidation-fix branch November 28, 2022 12:58
kibanamachine added a commit that referenced this pull request Nov 28, 2022
@kibanamachine @xcrzx
[8.6] [Security Solution] Update cache invalidation logic to handle e…
8320b80 
…rror responses (#146271) (#146380)

# Backport

This will backport the following commits from `main` to `8.6`:
- [[Security Solution] Update cache invalidation logic to handle error
responses (#146271)](#146271)

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

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

<!--BACKPORT [{"author":{"name":"Dmitrii
Shevchenko","email":"dmitrii.shevchenko@elastic.co"},"sourceCommit":{"committedDate":"2022-11-28T11:50:40Z","message":"[Security
Solution] Update cache invalidation logic to handle error responses
(#146271)\n\n**Resolves:
#146277
Summary\r\n\r\nPreviously, we invalidated the rules table cache only
after successful\r\nserver-side state mutations. So when an action like
bulk edit was\r\nsuccessfully updating some rules and failing for
others, the table\r\ncontinued showing outdated results.\r\n\r\nThis PR
moves the cache invalidation from the `onSuccess` handlers to\r\nthe
`onSettled` handlers to prevent showing partially stale data
after\r\nfailed
updates.","sha":"e9bc60355858c82fe39676622004f8e9cdfa61a2","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:fix","impact:low","Team:Detections
and Resp","Team: SecuritySolution","auto-backport","Feature:Rule
Management","Team:Detection
Rules","v8.6.0","v8.7.0"],"number":146271,"url":"#146271
Solution] Update cache invalidation logic to handle error responses
(#146271)\n\n**Resolves:
#146277
Summary\r\n\r\nPreviously, we invalidated the rules table cache only
after successful\r\nserver-side state mutations. So when an action like
bulk edit was\r\nsuccessfully updating some rules and failing for
others, the table\r\ncontinued showing outdated results.\r\n\r\nThis PR
moves the cache invalidation from the `onSuccess` handlers to\r\nthe
`onSettled` handlers to prevent showing partially stale data
after\r\nfailed
updates.","sha":"e9bc60355858c82fe39676622004f8e9cdfa61a2"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"#146271
Solution] Update cache invalidation logic to handle error responses
(#146271)\n\n**Resolves:
#146277
Summary\r\n\r\nPreviously, we invalidated the rules table cache only
after successful\r\nserver-side state mutations. So when an action like
bulk edit was\r\nsuccessfully updating some rules and failing for
others, the table\r\ncontinued showing outdated results.\r\n\r\nThis PR
moves the cache invalidation from the `onSuccess` handlers to\r\nthe
`onSettled` handlers to prevent showing partially stale data
after\r\nfailed
updates.","sha":"e9bc60355858c82fe39676622004f8e9cdfa61a2"}}]}]
BACKPORT-->

Co-authored-by: Dmitrii Shevchenko <dmitrii.shevchenko@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpdjere jpdjere jpdjere approved these changes

Assignees

@xcrzx xcrzx

Labels
auto-backport Deprecated: Automatically backport this PR after it's merged bug Fixes for quality problems that affect the customer experience Feature:Rule Management Security Solution Detection Rule Management impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. release_note:fix 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.6.0 v8.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Security Solution] Rules are not updated if a bulk request fails partially
6 participants
@xcrzx @elasticmachine @kibana-ci @kibanamachine @jpdjere @banderror