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

[RAM][Security Solution][Alerts] moves legacy actions migration to rulesClient #153101

Merged
merged 126 commits into from
Apr 19, 2023

Conversation

vitaliidm
Copy link
Contributor

@vitaliidm vitaliidm commented Mar 10, 2023

Summary

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

    • On migration: SO actions only saved. If any actions present in rule, they will be lost. Here is an example video from main branch

      Here is an example video from MAIN branch, where action in rule is overwritten by legacy action
      Screen.Recording.2023-04-06.at.14.45.12.mov

    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


    Here is an example video from this PR branch, where actions are merged
    Screen.Recording.2023-04-06.at.15.18.00.mov

  • 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:

Checklist

Delete any items that are not applicable to this PR.

For maintainers

vitaliidm and others added 30 commits February 28, 2023 15:55
…ithub.com/vitaliidm/kibana into alerts/legacy-actions-migration-to-alerting

# Conflicts:
#	x-pack/plugins/security_solution/server/lib/detection_engine/rule_actions_legacy/logic/rule_actions/legacy_rules_client_migration_hook.ts
…ithub.com/vitaliidm/kibana into alerts/legacy-actions-migration-to-alerting
…ithub.com/vitaliidm/kibana into alerts/legacy-actions-migration-to-alerting
…ithub.com/vitaliidm/kibana into alerts/legacy-actions-migration-to-alerting
…ithub.com/vitaliidm/kibana into alerts/legacy-actions-migration-to-alerting
@vitaliidm
Copy link
Contributor Author

  • removed migration for a number of methods and left only for the following: single and bulk update, enable/disable, delete.
  • in enable method, we calling create with overwrite=true to prevent AAD issues with key encrypting
  • added test that covers successful rule execution after its enabling

* @param params
* @returns
*/
export const migrateLegacyActions: MigrateLegacyActions = async (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a try/catch here and log the error to not block the main method? what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

{}
);

if (isEmpty(actionReference)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to throw an error, maybe we can just ignore it and return empty array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added try/catch in formatLegacyActions and migrateLegacyActions on top level. So any thrown errors will be caught and logged

Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

Please make sure, to mute any errors during migration/formatting, so we do not block our user form the main method that they wanted to do.

Also I tested it locally and running and everything is working as expected and I did not see any re-generation of API key.

const actionSavedObjects = results.flat().flatMap((r) => r.saved_objects);

if (errors.length) {
throw new AggregateError(errors, 'Error fetching rule actions');
Copy link
Contributor

Choose a reason for hiding this comment

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

let's mute the error, we should log it for sure!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added try/catch in formatLegacyActions and migrateLegacyActions on top level. So any thrown errors will be caught and logged

Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

Codeowners for security-solution-platform LGTM. I haven't gotten to test all the cases on my local yet, but do not want to block this as I know other PR's are waiting on this part to go in.

I saw we do increment the rule version - is that also true for migration on a read? Would that appear strange to the user that on a read, their rule appears to have been modified? (I know technically it was, but they won't necessarily know why or in what way).

@vitaliidm
Copy link
Contributor Author

I saw we do increment the rule version - is that also true for migration on a read? Would that appear strange to the user that on a read, their rule appears to have been modified? (I know technically it was, but they won't necessarily know why or in what way).

@yctercero
Rule revision is incremented on every update, independent to migration of legacy rules. And not incremented on read at all.

since, migration happens within single rulesClient API call, revision in migrated rule will increment by 1 only.

In current implementation in Security Solution, when user updates rule, we do 2 updates under the hood:

  1. migrate legacy actions, if any present
  2. actual rule update

So, in legacy actions migration case revision is getting incremented by 2. Here is a PR, when revision increment was introduced: #147398

But since, migration is moved to rulesClient, we perform only single update. So, revision will be incremented by 1 only.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #3 / timeline flyout button the (+) button popover menu owns focus

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 432 398 -34

References to deprecated APIs

id before after diff
alerting 76 86 +10
securitySolution 390 380 -10
total -0

Total ESLint disabled count

id before after diff
securitySolution 512 478 -34

History

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

cc @vitaliidm

@vitaliidm vitaliidm merged commit f8c16c1 into elastic:main Apr 19, 2023
saarikabhasi pushed a commit to saarikabhasi/kibana that referenced this pull request 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 pull request 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 pull request 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 pull request 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>
@vitaliidm vitaliidm deleted the alerts/legacy-actions-migration-v2 branch March 4, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Detection Alerts Security Detection Alerts Area Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants