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] Dedupe alerts by querying _id before creation #119045

Merged
merged 12 commits into from
Nov 18, 2021

Conversation

marshallmain
Copy link
Contributor

Summary

New PR because the last one (#118261) was failing to report the status from CI at all, even after closing/reopening and triggering CI re-runs. Not sure what was going wrong.

This PR adds logic to query for candidate alerts by _id before attempting to create them. This fixes 2 bugs.

First bug: when a rule runs with a significant "additional lookback", it can find the same alert on multiple consecutive executions. The alerts generated on the later executions were overwriting the existing alert from the previous execution, updating the @timestamp field but leaving all other fields the same. With this change, the later executions see that the alert already exists and don't modify it.

Second bug: for a long time, it has been possible for an alert with the same _id to exist in multiple concrete indices. As a result, if a rule finds the same alert on multiple runs but the alerts index rolls over between rule runs, then the alert will be duplicated in both the old and new index. Since this new logic queries the alerts index alias for the _id, when an index rollover happens we will still detect that an alert has already been written and won't write a duplicate.

Organization

This PR also moves more of the document creation logic into the Persistence Rule type. The bulkCreateFactory was responsible for parsing the bulk response before, but the persistence rule type was defining the request and response formats. The reorganization groups logic in alertWithPersistence so that alerts to index are passed into the function and the alerts that were actually indexed are returned from the function.

@marshallmain marshallmain added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. auto-backport Deprecated - use backport:version if exact versions are needed v8.1.0 Team:Detection Alerts Security Detection Alerts Area Team labels Nov 18, 2021
@marshallmain marshallmain marked this pull request as ready for review November 18, 2021 15:49
@marshallmain marshallmain requested a review from a team as a code owner November 18, 2021 15:49
@elasticmachine
Copy link
Contributor

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

@marshallmain
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

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
ruleRegistry 139 141 +2
Unknown metric groups

API count

id before after diff
ruleRegistry 165 167 +2

History

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

@marshallmain marshallmain merged commit 8c8c62e into elastic:main Nov 18, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Nov 18, 2021
…astic#119045)

* Dedupe alerts by querying _id before creation

* Update alert chunk size

* Use aggregations to find existing alert _ids

* Remove tightly coupled tests

* Add api integration test for alert deduplication

* Remove unused import

* Cleaner util implementation

* Skip flaky test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
8.0

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Nov 19, 2021
…19045) (#119122)

* Dedupe alerts by querying _id before creation

* Update alert chunk size

* Use aggregations to find existing alert _ids

* Remove tightly coupled tests

* Add api integration test for alert deduplication

* Remove unused import

* Cleaner util implementation

* Skip flaky test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com>
TinLe pushed a commit to TinLe/kibana that referenced this pull request Nov 20, 2021
…astic#119045)

* Dedupe alerts by querying _id before creation

* Update alert chunk size

* Use aggregations to find existing alert _ids

* Remove tightly coupled tests

* Add api integration test for alert deduplication

* Remove unused import

* Cleaner util implementation

* Skip flaky test

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
dmlemeshko pushed a commit that referenced this pull request Nov 29, 2021
…19045)

* Dedupe alerts by querying _id before creation

* Update alert chunk size

* Use aggregations to find existing alert _ids

* Remove tightly coupled tests

* Add api integration test for alert deduplication

* Remove unused import

* Cleaner util implementation

* Skip flaky test

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
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Detection Alerts Security Detection Alerts Area Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants