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

META - [RAM][SECURITYSOLUTION][ALERTS] - Integrate Alert summary inside of security solution rule page #151916

Closed
19 tasks done
XavierM opened this issue Feb 22, 2023 · 7 comments
Closed
19 tasks done
Assignees
Labels
8.8 candidate Team:Detection Engine Security Solution Detection Engine Area Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)

Comments

@XavierM
Copy link
Contributor

XavierM commented Feb 22, 2023

As an user I would like to to use the new Alert Summary from response ops in the security solution rule creation/update page.

API:

UI:

Actions per alert:

Some research:

When we know everything work as expected, it will be nice to do some research if we can use directly the alerting framework API directly.

@XavierM XavierM added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team:Detection Alerts Security Detection Alerts Area Team labels Feb 22, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@e40pud
Copy link
Contributor

e40pud commented Apr 3, 2023

Hi @XavierM, I had a chat with @banderror today and we wanted to clarify couple questions before the release of this feature.

In order to be compatible our rule's APIs should support rule level throttle parameter. Since we gonna have per-action frequency field there are two questions about conversion between those two:

  1. It is possible through current API to modify rule level throttle field. What does it mean for per-action frequency? Are we going to apply passed throttle to each action.frequency?
  2. It is possible that user will modify the per-action frequency via new UI changes that I am integrating and have different throttle per-action. What value will rule level throttle field hold inside in this case? Are we going to apply one of the per-action frequencys to rule level throttle (like first action frequency or last modified frequency throttle)?

cc @marshallmain

@banderror
Copy link
Contributor

Hey @XavierM, one more question from my side is about the action's context.

If an action is configured as "trigger me per each generated alert" (as opposed to the current "trigger me per all alerts generated during the rule execution"), how the user is going to access this alert via the placeholders that can be used in the action's body?

Screenshot 2023-04-03 at 19 05 47

Are we going to introduce a new property like context.alert (a single value)? Are we going to keep the same context.alerts property (an array), but populate it with only one value? Any other option? How are we going to explain it to the user (hints in the UI, docs)? We'd need to take into account backward compatibility with existing rules as well.

Previously, I asked a related question here, but it's kinda orthogonal.

@e40pud
Copy link
Contributor

e40pud commented Apr 4, 2023

One more question about the message we would like to show to user for "For each alert" option. We show 'Rule {{context.rule.name}} generated {{state.signals_count}} alerts' for "Summary of alerts", but it might not work "For each alert" action's frequency.

Screenshot 2023-04-04 at 16 30 24

cc @peluja1012

@e40pud
Copy link
Contributor

e40pud commented Apr 17, 2023

Last week we discussed that there will be some breaking changes. Here is the overview of what is going to be different which also covers my questions above.

It is possible through current API to modify rule level throttle field. What does it mean for per-action frequency? Are we going to apply passed throttle to each action.frequency`?

It is possible that user will modify the per-action frequency via new UI changes that I am integrating and have different throttle per-action. What value will rule level throttle field hold inside in this case? Are we going to apply one of the per-action frequencies to rule level throttle (like first action frequency or last modified frequency throttle)?

Currently, rules can have two states in alert framework:

  1. Rule level throttle and notifyWhen are undefined and all actions have frequency attribute.
  2. Rule level throttle and notifyWhen set to one of the allowed values and all actions DON'T have frequency attribute.

With the new changes, we will make sure actions are always switched to the new format with the frequency attribute set. There are several possible cases here:

  1. If actions have frequency attribute set, we just use those values. On security Solution side, the rule level throttle will be set to either existing value which was set during the interaction with the older version kibana or to the value converted from the first action: actions[0].frquency.
  2. If actions do not have frequency attribute set, then we will convert rule level throttle and notifyWhen into action level frequency attribute.
  3. If for some reason the state is broken (both rule level throttle and notifyWhen, and action level frequency attributes are not set), we will set action frequency attribute to default value: { summary: true, throttle: null, notifyWhen: 'onActiveAlert' } for all the actions.

Above changes, will introduce breaking behaviour for the users who use the Rule APIs via external tools:

  1. Users, cannot rely on the rule level throttle and notifyWhen values. Those attributes won't represent the accurate actions frequencies state (because each action will have own frequency).
  2. When user adds actions via bulk update, only those actions will reflect passed throttle. The frequency attribute of the existing actions won't be changed in this case. This means, that old behaviour where user could update throttle for all actions via bulk update won't work after these changes.

@XavierM @peluja1012 @marshallmain

@e40pud
Copy link
Contributor

e40pud commented Apr 17, 2023

Regarding the question from @banderror about accessing the alert in action's body, there is an alert object which we can use for that. I tried to use {{alert.id}} and it worked as expected.

Screenshot 2023-04-17 at 16 02 20

cc @ymao1

vitaliidm added a commit that referenced this issue Apr 19, 2023
…lesClient (#153101)

## Summary
- this PR is the first part of work related to conditional logic
actions. The rest of PRs, will be merged after this one. As they depend
on a work implemented here. [List of
tickets](elastic/security-team#2894 (comment))
- addresses #151919
- moves code related to legacy actions migration from D&R to
RulesClient,
[details](#151919 (comment))
- similarly to D&R part, now rulesClient read APIs, would return legacy
actions within rule
- similarly, every mutation API in rulesClient, would migrate legacy
actions, and remove sidecar SO
- each migrated legacy action will have also [`frequency`
object](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/alerting/server/types.ts#L234-L238),
that would allow to have notifyWhen/throttle on action level once
#151916 is implemented, which is
targeted in 8.8, right after this PR.
But before it's merged, `frequency` is getting removed in
[update/bulk_edit/create
APIs](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/alerting/server/rules_client/methods/update.ts#L151-L160).
Hence it's not reflected in most of the tests at this point.
- cleanup of legacy actions related code in D&R
- adds unit tests for RulesClient
- keeps functional/e2e tests in D&R

Changes in behaviour, introduced in this PR:
- since, migration happens within single rulesClient API call, revision
in migrated rule will increment by `1` only.
- legacy actions from sidecar SO, now will be merged with rules actions,
if there any.
Before, in the previous implementation, there was inconsistency in a way
how legacy and rules actions were treated.
- On read: actions existing in rule, [would take precedence over legacy
ones
](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_actions.ts#L94-L114)
- On migration: SO actions [only
saved](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/rule_actions/legacy_action_migration.ts#L114).
If any actions present in rule, they will be lost. Here is an example
video from **main** branch
      <details>
<summary>Here is an example video from MAIN branch, where action in rule
is overwritten by legacy action</summary>
      

https://user-images.githubusercontent.com/92328789/230397535-d3fcd644-7cf9-4970-a573-18fd8c9f2235.mov

      </details>

So, depends on sequence of events, different actions could be saved for
identical use case: rule has both legacy and existing action.
- if rule migrated through update API, existing in rule action will be
saved
- if rule migrated through enable/disable API, bulk edit, legacy action
will be saved

In this implementation, both existing in rule and legacy actions will be
merged, to prevent loss of actions
      <details>
<summary>Here is an example video from this PR branch, where actions are
merged</summary>
<video
src="https://user-images.githubusercontent.com/92328789/230405569-f1da38e9-4e36-46a8-9654-f664e0a31063.mov"
/>
      </details>
      
- when duplicating rule, we don't migrate source rule anymore. It can
lead to unwanted API key regeneration, with possible loss of privileges,
earlier associated with the source rule. As part of UX, when duplicating
any entity, users would not be expecting source entity to be changed
  
TODO:
- performance improvement issue for future
#154438
- currently, in main branch, when migration is performed through rule
enabling, actions not showing anymore in UI.
Relevant ticket is #154567
I haven't fixed it in this PR, as in[ the next one
](#153113), we will display
notifyWhen/throttle on action level in UI, rather than on rule level.
So, once that PR is merged, actions should be displayed in new UI
  
### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
saarikabhasi pushed a commit to saarikabhasi/kibana that referenced this issue Apr 19, 2023
…lesClient (elastic#153101)

## Summary
- this PR is the first part of work related to conditional logic
actions. The rest of PRs, will be merged after this one. As they depend
on a work implemented here. [List of
tickets](elastic/security-team#2894 (comment))
- addresses elastic#151919
- moves code related to legacy actions migration from D&R to
RulesClient,
[details](elastic#151919 (comment))
- similarly to D&R part, now rulesClient read APIs, would return legacy
actions within rule
- similarly, every mutation API in rulesClient, would migrate legacy
actions, and remove sidecar SO
- each migrated legacy action will have also [`frequency`
object](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/alerting/server/types.ts#L234-L238),
that would allow to have notifyWhen/throttle on action level once
elastic#151916 is implemented, which is
targeted in 8.8, right after this PR.
But before it's merged, `frequency` is getting removed in
[update/bulk_edit/create
APIs](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/alerting/server/rules_client/methods/update.ts#L151-L160).
Hence it's not reflected in most of the tests at this point.
- cleanup of legacy actions related code in D&R
- adds unit tests for RulesClient
- keeps functional/e2e tests in D&R

Changes in behaviour, introduced in this PR:
- since, migration happens within single rulesClient API call, revision
in migrated rule will increment by `1` only.
- legacy actions from sidecar SO, now will be merged with rules actions,
if there any.
Before, in the previous implementation, there was inconsistency in a way
how legacy and rules actions were treated.
- On read: actions existing in rule, [would take precedence over legacy
ones
](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_actions.ts#L94-L114)
- On migration: SO actions [only
saved](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/rule_actions/legacy_action_migration.ts#L114).
If any actions present in rule, they will be lost. Here is an example
video from **main** branch
      <details>
<summary>Here is an example video from MAIN branch, where action in rule
is overwritten by legacy action</summary>
      

https://user-images.githubusercontent.com/92328789/230397535-d3fcd644-7cf9-4970-a573-18fd8c9f2235.mov

      </details>

So, depends on sequence of events, different actions could be saved for
identical use case: rule has both legacy and existing action.
- if rule migrated through update API, existing in rule action will be
saved
- if rule migrated through enable/disable API, bulk edit, legacy action
will be saved

In this implementation, both existing in rule and legacy actions will be
merged, to prevent loss of actions
      <details>
<summary>Here is an example video from this PR branch, where actions are
merged</summary>
<video
src="https://user-images.githubusercontent.com/92328789/230405569-f1da38e9-4e36-46a8-9654-f664e0a31063.mov"
/>
      </details>
      
- when duplicating rule, we don't migrate source rule anymore. It can
lead to unwanted API key regeneration, with possible loss of privileges,
earlier associated with the source rule. As part of UX, when duplicating
any entity, users would not be expecting source entity to be changed
  
TODO:
- performance improvement issue for future
elastic#154438
- currently, in main branch, when migration is performed through rule
enabling, actions not showing anymore in UI.
Relevant ticket is elastic#154567
I haven't fixed it in this PR, as in[ the next one
](elastic#153113), we will display
notifyWhen/throttle on action level in UI, rather than on rule level.
So, once that PR is merged, actions should be displayed in new UI
  
### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
e40pud added a commit that referenced this issue Apr 23, 2023
…ecurity solution rule page (#154990)

## Summary

[Main ticket](#151916)
This PR dependant on [these
changes](#153101)

These changes cover next two tickets:
- [RAM][SECURITYSOLUTION][ALERTS] - Integrate per-action frequency field
in security solution APIs #154532
- [RAM][SECURITYSOLUTION][ALERTS] - Integrate per-action frequency UI in
security solution #154534

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.



![](https://user-images.githubusercontent.com/616158/227377473-f34a330e-81ce-42b4-af1b-e6e302c6319d.png)


## NOTES:
- There will be no more "Perform no actions" option which mutes all the
actions of the rule. For back compatibility, we need to show that rule
is muted in the UI cc @peluja1012 @ARWNightingale
- The ability to generate per-alert action will be done as part of
#153611


## 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**:
- We always gonna set rule level `throttle` to `undefined` from now on
- If each action has `frequency` attribute set, then we just use those
values
- If actions do not have `frequency` attribute set (or for some reason
there is a mix of actions with some of them having `frequency` attribute
and some not), then we transform rule level `throttle` into `frequency`
and set it for each action in the rule

On rule **bulk editing**:
- We always gonna set rule level `throttle` to `undefined`
- If each action has `frequency` attribute set, then we just use those
values
- If actions do not have `frequency` attribute set, then we transform
rule level `throttle` into `frequency` and set it for each action in the
rule
- If user passed only `throttle` attribute with empty actions (`actions
= []`), this will only remove all actions from the rule

This 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.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Maxim Palenov <maxim.palenov@elastic.co>
nikitaindik pushed a commit to nikitaindik/kibana that referenced this issue Apr 25, 2023
…lesClient (elastic#153101)

## Summary
- this PR is the first part of work related to conditional logic
actions. The rest of PRs, will be merged after this one. As they depend
on a work implemented here. [List of
tickets](elastic/security-team#2894 (comment))
- addresses elastic#151919
- moves code related to legacy actions migration from D&R to
RulesClient,
[details](elastic#151919 (comment))
- similarly to D&R part, now rulesClient read APIs, would return legacy
actions within rule
- similarly, every mutation API in rulesClient, would migrate legacy
actions, and remove sidecar SO
- each migrated legacy action will have also [`frequency`
object](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/alerting/server/types.ts#L234-L238),
that would allow to have notifyWhen/throttle on action level once
elastic#151916 is implemented, which is
targeted in 8.8, right after this PR.
But before it's merged, `frequency` is getting removed in
[update/bulk_edit/create
APIs](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/alerting/server/rules_client/methods/update.ts#L151-L160).
Hence it's not reflected in most of the tests at this point.
- cleanup of legacy actions related code in D&R
- adds unit tests for RulesClient
- keeps functional/e2e tests in D&R

Changes in behaviour, introduced in this PR:
- since, migration happens within single rulesClient API call, revision
in migrated rule will increment by `1` only.
- legacy actions from sidecar SO, now will be merged with rules actions,
if there any.
Before, in the previous implementation, there was inconsistency in a way
how legacy and rules actions were treated.
- On read: actions existing in rule, [would take precedence over legacy
ones
](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/normalization/rule_actions.ts#L94-L114)
- On migration: SO actions [only
saved](https://github.com/elastic/kibana/blob/8.7/x-pack/plugins/security_solution/server/lib/detection_engine/rule_management/logic/rule_actions/legacy_action_migration.ts#L114).
If any actions present in rule, they will be lost. Here is an example
video from **main** branch
      <details>
<summary>Here is an example video from MAIN branch, where action in rule
is overwritten by legacy action</summary>
      

https://user-images.githubusercontent.com/92328789/230397535-d3fcd644-7cf9-4970-a573-18fd8c9f2235.mov

      </details>

So, depends on sequence of events, different actions could be saved for
identical use case: rule has both legacy and existing action.
- if rule migrated through update API, existing in rule action will be
saved
- if rule migrated through enable/disable API, bulk edit, legacy action
will be saved

In this implementation, both existing in rule and legacy actions will be
merged, to prevent loss of actions
      <details>
<summary>Here is an example video from this PR branch, where actions are
merged</summary>
<video
src="https://user-images.githubusercontent.com/92328789/230405569-f1da38e9-4e36-46a8-9654-f664e0a31063.mov"
/>
      </details>
      
- when duplicating rule, we don't migrate source rule anymore. It can
lead to unwanted API key regeneration, with possible loss of privileges,
earlier associated with the source rule. As part of UX, when duplicating
any entity, users would not be expecting source entity to be changed
  
TODO:
- performance improvement issue for future
elastic#154438
- currently, in main branch, when migration is performed through rule
enabling, actions not showing anymore in UI.
Relevant ticket is elastic#154567
I haven't fixed it in this PR, as in[ the next one
](elastic#153113), we will display
notifyWhen/throttle on action level in UI, rather than on rule level.
So, once that PR is merged, actions should be displayed in new UI
  
### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
nikitaindik pushed a commit to nikitaindik/kibana that referenced this issue Apr 25, 2023
…ecurity solution rule page (elastic#154990)

## Summary

[Main ticket](elastic#151916)
This PR dependant on [these
changes](elastic#153101)

These changes cover next two tickets:
- [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

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.



![](https://user-images.githubusercontent.com/616158/227377473-f34a330e-81ce-42b4-af1b-e6e302c6319d.png)


## NOTES:
- There will be no more "Perform no actions" option which mutes all the
actions of the rule. For back compatibility, we need to show that rule
is muted in the UI cc @peluja1012 @ARWNightingale
- The ability to generate per-alert action will be done as part of
elastic#153611


## 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**:
- We always gonna set rule level `throttle` to `undefined` from now on
- If each action has `frequency` attribute set, then we just use those
values
- If actions do not have `frequency` attribute set (or for some reason
there is a mix of actions with some of them having `frequency` attribute
and some not), then we transform rule level `throttle` into `frequency`
and set it for each action in the rule

On rule **bulk editing**:
- We always gonna set rule level `throttle` to `undefined`
- If each action has `frequency` attribute set, then we just use those
values
- If actions do not have `frequency` attribute set, then we transform
rule level `throttle` into `frequency` and set it for each action in the
rule
- If user passed only `throttle` attribute with empty actions (`actions
= []`), this will only remove all actions from the rule

This 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.

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Maxim Palenov <maxim.palenov@elastic.co>
@yctercero yctercero added Team:Detection Engine Security Solution Detection Engine Area and removed Team:Detection Alerts Security Detection Alerts Area Team labels May 13, 2023
@e40pud
Copy link
Contributor

e40pud commented May 15, 2023

@XavierM closing this ticket. Short interval UI will be addressed in 8.9 here #156644

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.8 candidate Team:Detection Engine Security Solution Detection Engine Area Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)
Projects
No open projects
Development

No branches or pull requests

6 participants