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

[Detection Engine] Adds 8.0 rules #123786

Merged

Conversation

brokensound77
Copy link
Contributor

Summary

Pull updates to detection rules from https://github.com/elastic/detection-rules/tree/b6d1c1476ba78a06413baf0fc4c8aeadab2a24c7.

Checklist

Delete any items that are not applicable to this PR.

@brokensound77 brokensound77 requested a review from a team as a code owner January 26, 2022 02:21
@brokensound77 brokensound77 requested a review from spong January 26, 2022 02:21
@brokensound77 brokensound77 added auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v8.0.0 v8.1.0 labels Jan 26, 2022
@@ -1,198 +0,0 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was deprecated as of 8.0

"logs-endpoint.events.*"
],
"language": "kuery",
"language": "eql",
Copy link
Member

Choose a reason for hiding this comment

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

Looks like because of the language/type change this rule is failing to update, and must be deleted before it can be updated/re-installed. The tricky thing here is that the error doesn't tell you it's the Interactive Terminal Spawned via Python rule that is failing the update, so it's not clear to the user how they can fix it and the update 1 rule callout will persist and can't be dismissed.

I'll chat with the team tomorrow on supporting language/type changes on update tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, thanks. Good point on the error.

This seems odd though - we have converted existing rules from kuery/query to eql/eql many times before with no issues

Copy link
Member

Choose a reason for hiding this comment

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

As part of RAC we've moved from a single alerting rule type (siem.signals) to separate dedicated types for each security rule, e.g. siem.queryRule and siem.eqlRule, so my guess is we lost the ability to migrate rule types on upgrade as part of that change as it's trying to update a siem.queryRule to have language/type values that are not valid for its schema.

cc @elastic/security-detections-response-alerts folks in case they have any context here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@brokensound77 would it be acceptable to revert changes for this particular rule? We have at most one day until the last BC for 8.0 where we can merge non-blockers (quoting @MadameSheema: "FYI 8.0-rc2 BC2 built has started and tomorrow a new 8.0-rc2 BC is going to be built (BC3)").

Copy link
Member

Choose a reason for hiding this comment

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

Issue for tracking: #123859

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rule change reverted in elastic/detection-rules#1731. I will revert the file here as well which should resolve the issue until a permanent fix is introduced

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

LGTM! Thanks for working with us while we take care of #123859!

@brokensound77 brokensound77 enabled auto-merge (squash) January 26, 2022 22:15
@brokensound77
Copy link
Contributor Author

@elasticmachine merge upstream

@brokensound77 brokensound77 merged commit 36722fa into elastic:main Jan 27, 2022
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 27, 2022
* [Detection Rules] Add 8.0 rules
* rollback changes for python tty rule elastic/detection-rules#1731

(cherry picked from commit 36722fa)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.0

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jan 27, 2022
* [Detection Rules] Add 8.0 rules
* rollback changes for python tty rule elastic/detection-rules#1731

(cherry picked from commit 36722fa)

Co-authored-by: Justin Ibarra <[email protected]>
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 v8.0.0 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants