-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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][Exceptions] Allows bulk close on exception to close acknowledged alerts #110147
Conversation
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
💚 Build SucceededMetrics [docs]Async chunks
Unknown metric groupsReferences to deprecated APIs
History
To update your PR or re-run it, just comment with: cc @dplumlee |
alias: null, | ||
negate: false, | ||
disabled: false, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, the meta field has a legacy behind it where at one point we found we were accidentally broken when it wasn't there and we had to ship and add it.
I don't know if that is still true today and that's why we still have to use it or if it's for a particular UI usage or other reasoning such as, "All the rest have them so I thought it was for a good reason?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was talking to @spong a couple days ago about this and it seemed as if we only use it nowadays to populate the global kql query bar and as these functions are used currently as a "straight to ES query" helper functions it doesn't seem to cause any issues, but any thoughts appreciated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine as is for now. We might revisit all of them later.
}, | ||
]; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could write a unit test for this one like the other one right? I would add one for this if it is the future?
The comment at the top looks odd with the TODO, isn't this the one that shouldn't be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the rule registry ones will eventually be removed in favor of the regular functions once the aliases are finalized so I've just been writing tests for the ones that will stay. The functions are identical other than the placeholder alias names so I figured it wouldn't be an issue short term since they'll be removed within another couple weeks likely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
- I opened dev tools and ran some queries that were similar to the should below in this PR to make sure it looked okay.
DELETE delme-frank-1
PUT delme-frank-1
PUT delme-frank-1
{
"mappings": {
"properties": {
"signal": {
"properties": {
"status": {
"type": "keyword"
}
}
},
"@timestamp": {
"type": "keyword"
}
}
}
}
PUT delme-frank-1/_doc/1
{
"@timestamp": "2021-08-26T18:24:20.047Z"
}
PUT delme-frank-1/_doc/2
{
"signal.status": "open"
}
GET delme-frank-1/_search
GET delme-frank-1/_search
{
"query": {
"bool": {
"should": [
{
"term": {
"signal.status": "open"
}
},
{
"term": {
"signal.status": "acknowledged"
}
},
{
"term": {
"signal.status": "in-progress"
}
}
]
}
}
}
Got back:
{
"took" : 0,
"timed_out" : false,
"_shards" : {
"total" : 1,
"successful" : 1,
"skipped" : 0,
"failed" : 0
},
"hits" : {
"total" : {
"value" : 1,
"relation" : "eq"
},
"max_score" : 0.2876821,
"hits" : [
{
"_index" : "delme-frank-1",
"_id" : "2",
"_score" : 0.2876821,
"_source" : {
"signal.status" : "open"
}
}
]
}
}
So I think this works well.
…se acknowledged alerts (elastic#110147)
…se acknowledged alerts (elastic#110147)
…se acknowledged alerts (#110147) (#110331) Co-authored-by: Davis Plumlee <[email protected]>
…se acknowledged alerts (#110147) (#110330) Co-authored-by: Davis Plumlee <[email protected]>
Summary
Up till now, when the bulk close checkbox is selected on a created or updated rule exception, it just closes any
open
alerts that meet the criteria. This updates the status filters we use for the POST queries to include bothacknowledged
andin-progress
(for backwards compatibility) so all alerts that aren't already closed will be on submit.Important Note
This functionality is currently broken on pre-7.15 endpoint alerts and a related ticket has been opened. Once that logic is fixed, it should automatically work with these changes and fix the issue for endpoint alerts as well.
Checklist
Delete any items that are not applicable to this PR.
For maintainers