-
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
[RAM][SECURITYSOLUTION][ALERTS] - Integrate Alert summary inside of security solution rule page #154990
[RAM][SECURITYSOLUTION][ALERTS] - Integrate Alert summary inside of security solution rule page #154990
Conversation
…ecurity solution rule page * [RAM][SECURITYSOLUTION][ALERTS] - Integrate per-action frequency field in security solution APIs elastic#154532 * [RAM][SECURITYSOLUTION][ALERTS] - Integrate per-action frequency UI in security solution elastic#154534
Initially I had two separate draft PRs for tickets above, but because of extra work I had to do to support old UI with the new field integration, I decided to merge those draft PRs into one instead. @maximpn @vitaliidm you already started reviewing one of them, sorry for making you looking into this once again. |
@elasticmachine merge upstream |
# Conflicts: # x-pack/plugins/alerting/server/rules_client/methods/bulk_edit.ts # x-pack/plugins/alerting/server/rules_client/methods/create.ts # x-pack/plugins/alerting/server/rules_client/methods/update.ts # x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_actions.ts # x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_converters.ts # x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/utils/utils.test.ts # x-pack/test/detection_engine_api_integration/security_and_spaces/group10/legacy_actions_migrations.ts # x-pack/test/detection_engine_api_integration/security_and_spaces/group10/perform_bulk_action.ts
@elasticmachine merge upstream |
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.
@e40pud thank you for finally adopting frequency from Alerting Framework 👍
I've tested the PR locally and it works as expected. The changes look good though my main concern is throttle
field as it may be null
and returned to the client this way. While it's really not a big deal what we use under the hood (though it's better to stick to one of the options as minimum inside Security Solution plugin) it's not a good thing to mix null
and undefined
in the API. So far we tend to avoid null
as it helps to avoid serialising fields without a meaningful value and UI part can easily fallback for undefined
when reading a field. You can check rule schemas.
On top of that this task requires removing normalisation and will lead to returning value as is so it has the same problem of representing absence of a value. So far throttle
is always transformed to one of a string literal values.
summary: schema.boolean(), | ||
throttle: schema.nullable(schema.string()), | ||
notifyWhen: schema.oneOf([ | ||
schema.literal('onActionGroupChange'), |
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.
nit: I'm rather curious than it's call for action. These are duplicates the literals from kbn-securitysolution-io-ts-alerting-types/src/frequency
. I'm not sure if we should keep them at one place and reuse but if there are some changes or renamings we have to change the literals at two unrelated files.
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.
We are not using io-ts in platform, and we want to keep using kbn-schema
. I think we are fine with that for now
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 only meant the string literals onActionGroupChange
, onActiveAlert
and onThrottleInterval
.
{ | ||
id: body.id, | ||
action_type_id: '.slack', | ||
group: 'default', | ||
params: { | ||
message: expectedExistingSlackMessage, | ||
}, | ||
frequency: { | ||
summary: true, |
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.
Just curious why is summary: true
here?
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.
summary === true
is the default state which is used. Right now, all actions send the summary of all generated alerts within the selected timeframe.
import { getAllActionMessageParams } from '../../../../../../detections/pages/detection_engine/rules/helpers'; | ||
|
||
import { RuleActionsField } from '../../../../../../detections/components/rules/rule_actions_field'; | ||
import { debouncedValidateRuleActionsField } from '../../../../../../detections/containers/detection_engine/rules/validate_rule_actions_field'; | ||
|
||
const CommonUseField = getUseField({ component: Field }); | ||
|
||
export interface RuleActionForBulkActions extends RuleAction { |
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.
nit: Have you considered BulkActionsRuleAction
name? Do we really need to export this interface? I don't see any usages. As far as I see the intention is to make frequency
field required which could be done the following way
type BulkActionsRuleAction = RuleAction & Required<Pick<RuleAction, 'frequency'>>;
which allows to avoid importing RuleActionFrequency
.
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.
Sounds good, will do those changes
const setActionFrequency = useCallback( | ||
// TODO: replace any with a concrete type | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
(key: string, value: any, index: number) => { |
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.
Why can't RuleActionParam
be used for typing value
here?
setActionAlertsFilterProperty, | ||
featureId: SecurityConnectorFeatureId, | ||
defaultActionMessage: DEFAULT_ACTION_MESSAGE, | ||
defaultSummaryMessage: DEFAULT_ACTION_MESSAGE, | ||
hideActionHeader: true, | ||
hideNotifyWhen: true, | ||
hideNotifyWhen: 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.
Interesting if this prop can be removed as notifyWhen
should be alway displayed after merging this PR.
@@ -217,11 +186,6 @@ const StepRuleActionsComponent: FC<StepRuleActionsProps> = ({ | |||
return application.capabilities.actions.show ? ( | |||
<> | |||
<DisplayActionsHeader /> | |||
<UseField |
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.
It looks like throttleFieldComponentProps
can be also removed with
<UseField
path="throttle"
componentProps={throttleFieldComponentProps}
component={GhostFormField}
/>
and
<UseField path="actions" component={GhostFormField} />
<UseField path="kibanaSiemAppUrl" component={GhostFormField} />
<UseField path="enabled" component={GhostFormField} />
looks unnecessary here.
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 point! Will remove those components.
.../security_solution/server/lib/detection_engine/rule_management/normalization/rule_actions.ts
Outdated
Show resolved
Hide resolved
} | ||
|
||
const transformedActions: T[] = []; | ||
actions.forEach((action) => { |
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.
nit: looks a bit overcomplicated and can be replaced with for of
cycle or with
return actions.map((action) => ({
...action,
frequency,
}));
where frequency
is always set and it's unnecessary to use ...{ frequency }
.
@@ -401,6 +400,11 @@ export const convertPatchAPIToInternalSchema = ( | |||
): InternalRuleUpdate => { | |||
const typeSpecificParams = patchTypeSpecificSnakeToCamel(nextParams, existingRule.params); | |||
const existingParams = existingRule.params; | |||
|
|||
const alertActions = nextParams.actions?.map(transformRuleToAlertAction) ?? existingRule.actions; | |||
const throttle = nextParams.throttle ?? transformFromAlertThrottle(existingRule, null); |
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.
nit: It's an interesting choice of providing null
as throttle
value instead of undefined
though basically both null
and undefined
mean absence of a value. I try to extrapolate of using both later on and it can be confusing. Can we stick to undefined
as minimum inside Security Solution
?
There is a task I've been working on which requires removing of throttle normalization. It will lead it to fallback to undefined
when it's not set as we don't use both null
and undefined
in our APIs according to the schemas.
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.
This is gone after Vitalii's changes been merged.
export const RuleActionFrequency = t.type({ | ||
summary: RuleActionSummary, | ||
notifyWhen: RuleActionNotifyWhen, | ||
throttle: t.union([RuleActionThrottle, t.null]), |
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.
Why do we need to return null
when throttle
is not set while it could be an optional field resolved to undefined
if there is no a value. This was it won't be serialised and sent to the client. On top of that we try to avoid to use null
in our API according to https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/common/detection_engine/rule_schema/model/rule_schemas.ts.
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.
throttle
is set to null
everywhere in security solution
, alert framework
and triggering actions
plugins when notifyWhen
attribute is not onThrottleInterval
.
Our rule schemas are also use it here https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/server/lib/detection_engine/rule_schema/model/rule_schemas.ts
Even if we want to avoid setting it to null
and just keep undefined in case it is not set, we would need to go through many places make sure everything works as expected. So, I would not do this as part of this PR.
@elasticmachine merge upstream |
There are still some errors: @e40pud , is there any ability to limit choice of units and values in per action frequency component? |
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 implementing this feature.
Approving it.
And after, we need to figure out how to handle validation of throttle if it less than 5m
There is a |
@elasticmachine merge upstream |
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.
Checked out, did some (non-exhaustive) local testing, and reviewed some of the code.
As for the code changes, overall LGTM, I just left a few comments. I think only one of them is about correctness, the rest are minor.
My brain-power was over before I started looking at the tests, so I decided to skip it at least for today. Do we have a test plan for this feature that could help review the coverage? Are there any plans to write more tests for it?
In my local testing, I checked the following:
- How the actions form works when you create or edit a rule
- Bulk editing rule actions (adding and overriding)
- Import/export, including import of legacy rules with actions and rule-level frequency.
All worked well, I was especially glad that the import worked as I'd expect. Yay!
Overall, this is a very complex PR @e40pud, and it looks very good to me. I appreciate all your efforts! I'll approve the PR in advance, but it'd be great to sync on some of the comments once you have time.
export type RuleActionFrequency = t.TypeOf<typeof RuleActionFrequency>; | ||
export const RuleActionFrequency = t.type({ | ||
summary: RuleActionSummary, | ||
notifyWhen: RuleActionNotifyWhen, |
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.
Nit: Is notifyWhen
intentionally left camel-cased for some reason? In our API we use snake_case.
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 point! I did not notice it. I will create a separate ticket and will update it to snake_case in a separate PR.
/** | ||
* Action summary indicates whether we will send a summary notification about all the generate alerts or notification per individual alert | ||
*/ | ||
export type RuleActionSummary = t.TypeOf<typeof RuleActionSummary>; |
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.
Nit: Thanks for adding this comment, could you please comment on the rest of the types in this file?
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.
Yes, will do that!
@@ -13,5 +13,5 @@ export type RuleActionThrottle = t.TypeOf<typeof RuleActionThrottle>; | |||
export const RuleActionThrottle = t.union([ | |||
t.literal('no_actions'), | |||
t.literal('rule'), | |||
TimeDuration({ allowedUnits: ['h', 'd'] }), |
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.
Are we ok with letting users specify throttle as low as 1s
? Should we limit it by specifying some lower bound, or maybe there's such a limit enforced by the Framework?
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.
See discussion with Vitalii about this above #154990 (comment)
t.type({ | ||
actions: t.array(NormalizedRuleAction), | ||
}), | ||
]), |
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.
@e40pud could you please add this list of priorities as a comment for BulkActionEditPayloadRuleActions
?
// Throttle | ||
throttle: RuleActionThrottle, |
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.
(Not sure) Probably it shouldn't be part of the baseSchema
anymore, because we won't be returning it in responses of read operations. Instead of adding it here, maybe it would be semantically better to add it to SharedCreateProps
, SharedUpdateProps
and SharedPatchProps
.
The comment is not very informative btw 🙂
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.
As we discussed rule level throttle is gonna be always undefined
it makes sense to remove the field altogether as JSON.stringify({ throttle: undefined })
gives {}
.
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 tried to remove it but reverted the change back. There are many places involved which I did not think about and I broke many tests. I will add a separate ticket to do such a change separately.
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.
Here is the ticket to cover this: #155577
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.
it makes sense to remove the field altogether
@maximpn we can't remove it from the request parameters
throttle: null, | ||
notifyWhen: 'onActiveAlert', | ||
}, | ||
} as RuleAlertAction, |
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 avoid using as
casting?
throttle: t.union([RuleActionThrottle, t.null]), | ||
notifyWhen, | ||
notifywhen: t.union([RuleActionNotifyWhen, t.null]), |
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 completely remove these from internalRuleCreateOptional
and internalRuleUpdateOptional
, or they're still needed because the normalization to action-level frequencies is implemented inside the RulesClient?
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 will explore this in a separate PR. I will create a ticket for all related topics, where we can try to remove throttle
and notifyWhen
from rule schema.
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.
Here is the ticket to cover this: #155577
value: transformToActionFrequency(action.value.actions, action.value.throttle), | ||
}, | ||
getThrottleOperation(action.value.throttle), | ||
getNotifyWhenOperation(action.value.throttle), | ||
...(action.value.throttle | ||
? [ | ||
getThrottleOperation(action.value.throttle), | ||
getNotifyWhenOperation(action.value.throttle), | ||
] | ||
: []), | ||
]; | ||
|
||
case BulkActionEditType.set_rule_actions: | ||
return [ | ||
{ | ||
field: 'actions', | ||
operation: 'set', | ||
value: action.value.actions, | ||
value: transformToActionFrequency(action.value.actions, action.value.throttle), | ||
}, | ||
getThrottleOperation(action.value.throttle), | ||
getNotifyWhenOperation(action.value.throttle), | ||
...(action.value.throttle | ||
? [ | ||
getThrottleOperation(action.value.throttle), | ||
getNotifyWhenOperation(action.value.throttle), | ||
] | ||
: []), |
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.
Let's add comments explaining why we check action.value.throttle
here and how this works.
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.
We don't really need it anymore. So, I will remove passing those two parameters.
// If each action has frequency attribute set, then ignore the rule level throttle | ||
if (actions.every((action) => action.frequency)) { | ||
return actions; | ||
} | ||
|
||
const frequency = transformToFrequency(throttle); | ||
return actions.map((action) => ({ ...action, frequency })); |
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'm not sure this matches the logic we discussed yesterday, please correct me if I'm wrong, but I thought action-level frequency should always take precedence over the rule-level one. However, { ...action, frequency }
allows to override an existing action-level frequency with the rule-level one. It seems like the correct implementation should be:
export const transformToActionFrequency = <T extends ActionWithFrequency>(
actions: T[],
throttle: string | null | undefined
): T[] => {
const defaultFrequency = transformToFrequency(throttle);
return actions.map((action) => ({
...action,
frequency: action.frequency ?? defaultFrequency,
}));
};
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.
Will update accordingly.
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.
Codeowners LGTM - this needs to get in to be tested with other PRs. Will continue review - @e40pud if you don't mind following up on anything that comes up in testing.
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.
@e40pud thank you for addressing my previous comments 🙏 I've done testing and it works like a charm 🥳
From my side though I have two questions which can be addressed later on
- Why don't remove
throttle
from the rule response schema? As rule level throttle should be alwaysundefined
andJSON.stringify({ throttle: undefined })
gives{}
it looks quite logical to remove it. - Throttle input validation as extended to accept values in seconds and minutes. There is a valid comment from @banderror regarding
1s
interval and I'm keen on knowing the side effects of this too.
I tried to remove it but reverted the change back. There are many places involved which I did not think about and I broke many tests. I will add a separate ticket to do such a change separately.
Also, as for the intervals, see my discussion with Vitalii above #154990 (comment) |
@elasticmachine merge upstream |
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.
@vitaliidm I brought this function to correctly transform the throttle, same what you did with transformToNotifyWhen
.
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.
@vitaliidm I found a bug with the throttle conversion. Throttle cannot be 'no_actions', this is a notifyWhen
value. Throttle can to be either null
or a time duration value when notifyWhen
is onThrottleInterval
.
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @e40pud |
@banderror Thanks for the review!! Here are the answers to your questions:
Her is the test plan for the Conditional Actions feature which also includes testing of legacy action migration: https://docs.google.com/document/d/1w4ZRj-JXNzTGaTBkyfHrfNIyGpSL-AylOwBefPzCso8 I also added functional tests to cover the new functionality. |
Summary
Main ticket
This PR dependant on these changes
These changes cover next two tickets:
With this PR we will integrate per-action
frequency
field which already works within alert framework and will update security solution UI to incorporate the possibility to select "summary" vs "for each alert" type of actions.NOTES:
Technical Notes:
Here are the overview of the conversions and transformations that we are going to do as part of these changes for devs who are going to review.
On rule create/read/update/patch:
throttle
toundefined
from now onfrequency
attribute set, then we just use those valuesfrequency
attribute set (or for some reason there is a mix of actions with some of them havingfrequency
attribute and some not), then we transform rule levelthrottle
intofrequency
and set it for each action in the ruleOn rule bulk editing:
throttle
toundefined
frequency
attribute set, then we just use those valuesfrequency
attribute set, then we transform rule levelthrottle
intofrequency
and set it for each action in the rulethrottle
attribute with empty actions (actions = []
), this will only remove all actions from the ruleThis will bring breaking changes which we agreed on with the Advanced Correlation Group
cc @XavierM @vitaliidm @peluja1012
Checklist
Delete any items that are not applicable to this PR.