-
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][Alerts] Alert suppression per rule execution #142686
Conversation
This reverts commit 1b45b4b.
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.
platform changes look good
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.
Great work, @marshallmain. This feature will bring more capabilities to our customers!
Thank you for addressing my comments.
I left a few minor as well, that can be addressed later. The only big question I have it's regarding licensing. What's the plan on it? Will it be added in 8.6 release? As currently it's only handled on UI
<> | ||
{label} | ||
<EuiToolTip position="top" content={i18n.ALERT_SUPPRESSION_INSUFFICIENT_LICENSE}> | ||
<EuiIcon type={'alert'} size="l" color="#BD271E" /> |
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.
can we use value for colour from eui
theme?
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.
Yeah, this should come from the theme
}) | ||
); | ||
|
||
export const minimumLicenseForSuppression = 'platinum'; |
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 can see check for license is only present in UI.
What about having it on API level of Security and Platform? Is there a consideration to have it in 8.6?
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 switched from validating the params on create/edit of a rule to checking the license at runtime in the executor to avoid edge cases around license expiration/degradation. E.g. if the license expires, rules would be exportable but then would fail to import, patch
would allow editing a rule while leaving suppression params untouched but update
would not, etc.
The behavior now is that if a license is insufficient and suppression is enabled then rules will continue to work, but the suppression configuration will be ignored. If the platinum license is restored, then suppression will start to work again on those rules automatically. This way we can keep the functionality behind the license appropriately but not cause issues with rule management if the license level does drop.
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.
Thanks for explaining, that makes sense 👍
I switched from validating the params on create/edit of a rule to checking the license at runtime in the executor to avoid edge cases around license expiration/degradation
Btw, can we use there minimumLicenseForSuppression
instead of hardcoded platinum
value?
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.
Oops, yeah that should be minimumLicenseForSuppression
as well. I'll add it to the follow up list.
[ALERT_SUPPRESSION_TERMS]: bucket.terms, | ||
[ALERT_SUPPRESSION_START]: bucket.start, | ||
[ALERT_SUPPRESSION_END]: bucket.end, | ||
[ALERT_SUPPRESSION_DOCS_COUNT]: bucket.count - 1, |
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.
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.
Yeah the idea is to display here the number of alerts that were not created due to suppression, so we subtract 1 because the created alert is included in bucket.count
as well.
}); | ||
}); | ||
|
||
it('should not count documents that were covered by previous alerts', async () => { |
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 would suggest to test state involved executions through actual rule run, as it involves storing state in task manager and operating with it.
As for preview, it's stored in memory during preview execution, so we don't have full coverage of this functionality
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.
Good idea, will add in a follow up
x-pack/plugins/security_solution/public/detections/components/alerts_table/actions.tsx
Outdated
Show resolved
Hide resolved
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.
Code changes in the rules area LGTM! While testing locally found a couple of minor UI bugs, though.
Values should be comma separated:
Long values overflow outside of the screen:
The total number of alerts displayed on the chart (11) doesn't match the total number of alerts in the table (9). Not sure where that difference comes from. Probably not related to this PR at all.
Good point, updated to handle the array of values correctly (and updated screenshots in the PR description to match).
I've seen this before if one event has multiple |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Oh, yeah, that was the reason. Then I think it is okay to have one event counted multiple times. And thanks for fixing the layout issues 👍 |
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.
Rule registry changes look good to me at this time! We'll meet later this week with y'all to discuss proposed changes but we won't block this PR from merging.
…ression and new terms multi fields (#145775) ## Summary Adds new tour highlighting new rule capabilities in 8.6 - new terms multi fields (#143943) and alert suppression (#142686). I tried using the generic `RulesFeatureTour` again (main...marshallmain:kibana:failed-tour) but it still crashes the page. I also looked at integrating this tour with the Guided onboarding tour for rules management (#145223), but concluded that they should be separate since guided onboarding is experimental and this tour should be displayed to users even if they are not new users. This PR is essentially a copy of the new terms tour in 8.4 (#138469).
…ression and new terms multi fields (elastic#145775) ## Summary Adds new tour highlighting new rule capabilities in 8.6 - new terms multi fields (elastic#143943) and alert suppression (elastic#142686). I tried using the generic `RulesFeatureTour` again (elastic/kibana@main...marshallmain:kibana:failed-tour) but it still crashes the page. I also looked at integrating this tour with the Guided onboarding tour for rules management (elastic#145223), but concluded that they should be separate since guided onboarding is experimental and this tour should be displayed to users even if they are not new users. This PR is essentially a copy of the new terms tour in 8.4 (elastic#138469). (cherry picked from commit 13c1b0b)
…r suppression and new terms multi fields (#145775) (#146479) # Backport This will backport the following commits from `main` to `8.6`: - [[Security Solution][Alerts] Add tour to rule management page for suppression and new terms multi fields (#145775)](#145775) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Marshall Main","email":"55718608+marshallmain@users.noreply.github.com"},"sourceCommit":{"committedDate":"2022-11-28T21:35:02Z","message":"[Security Solution][Alerts] Add tour to rule management page for suppression and new terms multi fields (#145775)\n\n## Summary\r\n\r\nAdds new tour highlighting new rule capabilities in 8.6 - new terms\r\nmulti fields (#143943) and alert\r\nsuppression (https://github.com/elastic/kibana/pull/142686).\r\n\r\nI tried using the generic `RulesFeatureTour` again\r\n(https://github.com/elastic/kibana/compare/main...marshallmain:kibana:failed-tour)\r\nbut it still crashes the page.\r\n\r\nI also looked at integrating this tour with the Guided onboarding tour\r\nfor rules management (https://github.com/elastic/kibana/pull/145223),\r\nbut concluded that they should be separate since guided onboarding is\r\nexperimental and this tour should be displayed to users even if they are\r\nnot new users.\r\n\r\nThis PR is essentially a copy of the new terms tour in 8.4\r\n(https://github.com/elastic/kibana/pull/138469).","sha":"13c1b0b863b7d8b324d33f2aaf45d90d5c8c108e","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team: SecuritySolution","Team:Detection Alerts","v8.6.0","v8.7.0"],"number":145775,"url":"https://github.com/elastic/kibana/pull/145775","mergeCommit":{"message":"[Security Solution][Alerts] Add tour to rule management page for suppression and new terms multi fields (#145775)\n\n## Summary\r\n\r\nAdds new tour highlighting new rule capabilities in 8.6 - new terms\r\nmulti fields (#143943) and alert\r\nsuppression (https://github.com/elastic/kibana/pull/142686).\r\n\r\nI tried using the generic `RulesFeatureTour` again\r\n(https://github.com/elastic/kibana/compare/main...marshallmain:kibana:failed-tour)\r\nbut it still crashes the page.\r\n\r\nI also looked at integrating this tour with the Guided onboarding tour\r\nfor rules management (https://github.com/elastic/kibana/pull/145223),\r\nbut concluded that they should be separate since guided onboarding is\r\nexperimental and this tour should be displayed to users even if they are\r\nnot new users.\r\n\r\nThis PR is essentially a copy of the new terms tour in 8.4\r\n(https://github.com/elastic/kibana/pull/138469).","sha":"13c1b0b863b7d8b324d33f2aaf45d90d5c8c108e"}},"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":"https://github.com/elastic/kibana/pull/145775","number":145775,"mergeCommit":{"message":"[Security Solution][Alerts] Add tour to rule management page for suppression and new terms multi fields (#145775)\n\n## Summary\r\n\r\nAdds new tour highlighting new rule capabilities in 8.6 - new terms\r\nmulti fields (#143943) and alert\r\nsuppression (https://github.com/elastic/kibana/pull/142686).\r\n\r\nI tried using the generic `RulesFeatureTour` again\r\n(https://github.com/elastic/kibana/compare/main...marshallmain:kibana:failed-tour)\r\nbut it still crashes the page.\r\n\r\nI also looked at integrating this tour with the Guided onboarding tour\r\nfor rules management (https://github.com/elastic/kibana/pull/145223),\r\nbut concluded that they should be separate since guided onboarding is\r\nexperimental and this tour should be displayed to users even if they are\r\nnot new users.\r\n\r\nThis PR is essentially a copy of the new terms tour in 8.4\r\n(https://github.com/elastic/kibana/pull/138469).","sha":"13c1b0b863b7d8b324d33f2aaf45d90d5c8c108e"}}]}] BACKPORT--> Co-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com>
Summary
Addresses #130699
This PR implements alert throttling per rule execution for query and saved query rules. The implementation is very similar in concept to threshold rules. We allow users to pick one or more fields to group source documents by and use a composite aggregation to collect documents bucketed by those fields. We create 1 alert for each bucket based on the first document in the bucket and add metadata to the alert that represents how to retrieve the rest of the documents in the bucket.
The metadata fields are:
kibana.alert.suppression.terms
:Array<{field: string; value: string | number | null}>
An array of objects, each object represents one of the terms used to group these alertskibana.alert.suppression.start
:Date
The timestamp of the first document in the bucketkibana.alert.suppression.end
:Date
The timestamp of the last document in the bucketkibana.alert.suppression.docs_count
:number
The number of suppressed alertsThere is one new rule parameter, currently implemented at the solution level, to enable this feature:
alertSuppression.groupBy
:string[]
.Similar to threshold rules, the throttled query rules keep track of created alerts in the rule state in order to filter out duplicate documents in subsequent rule executions. When a throttled alert is created, we store the bucket information including field names, values, and end date in the rule state. Subsequent rule executions convert this state into a filter that excludes documents that have already been covered by existing alerts. This is necessary because consecutive rule executions will typically query overlapping time ranges.
Licensing
Alert suppression is licensed as a platinum-level feature. However, the rule management APIs do not prevent the suppression configuration fields from being set without a platinum license. Instead, the license is checked at rule execution time: if the license is insufficient, then the suppression configuration is ignored and the rule executes without suppressing alerts. This avoids potential issues with rule management and license downgrades - if a customer has rules enabled with suppression and the license expires/downgrades, they are not prevented from exporting/importing/editing their rules in any way, but the suppression will no longer be applied until the license is reinstated.
We check the license in the UI and display the suppression configuration accordingly. If the license is insufficient, then in rule creation the suppression configuration is disabled. When editing a rule, if the license is insufficient and the rule does not already have suppression configured, then suppression configuration is also disabled. If the license is insufficient but a rule does have suppression configured already, then we allow editing the suppression configuration in the UI.
The rule details page shows an indicator if suppression is configured and the license is insufficient to notify users that the suppression configuration will not be applied during rule execution.
Screenshots
Rule Create/Edit With License
Rule Details With License
Rule Create, or Rule Edit of a rule without existing suppression configuration, Without License
Editing a rule that has existing suppression configuration, but without the correct license, still allows changing the configuration (to allow removing the params)
Rule Details Without License
Alerts table
Known issues