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.

Sign up for GitHub

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

Jump to bottom

[Security Solution][Alerts] after legacy actions migration, triggered through enable/disable API, actions disappear from UI #154567

Closed
vitaliidm opened this issue Apr 6, 2023 · 3 comments
Closed

[Security Solution][Alerts] after legacy actions migration, triggered through enable/disable API, actions disappear from UI #154567

vitaliidm opened this issue Apr 6, 2023 · 3 comments
Labels
8.8 candidate bug Fixes for quality problems that affect the customer experience Team:Detection Alerts Security Detection Alerts Area Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.8.0

Comments

@vitaliidm
Copy link
Contributor

Describe the bug:

After legacy actions migration, triggered through enable/disable API, actions disappear from UI

Kibana/Elasticsearch Stack version:
At least 8.7+, likely since legacy actions migration introduced

Steps to reproduce:

  1. Create rule with legacy action. Instructions can found in this PR description
  2. Go to rule actions page. Ensure legacy action is shown
  3. Enable rule
  4. Go to rule actions page again. No actions shown

Current behavior:
No actions shown once legacy ones migrated through enable/disable API

Expected behavior:
Migrated actions should be displayed on rules action page

Video

Screen.Recording.2023-04-06.at.12.02.10.smaller.mov

Any additional context (logs, chat logs, magical formulas, etc.):

Likely it happens because rule's attribute muteAll is still set to true after migration.
It wasn't fixed in #153101, since the next PR introduces action level throttle/notifyWhen. And legacy actions already have all needed information within them. So, once it merged, we should expect to see action on UI.
It would also be nice to add e2e/cypress test to cover this case, once we establish it works as expected
@vitaliidm vitaliidm added bug Fixes for quality problems that affect the customer experience triage_needed Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Apr 6, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@vitaliidm vitaliidm added Team:Detection Alerts Security Detection Alerts Area Team 8.8 candidate v8.8.0 and removed triage_needed labels Apr 6, 2023
@vitaliidm vitaliidm mentioned this issue Apr 6, 2023
Merged
2 tasks
vitaliidm added a commit that referenced this issue Apr 19, 2023
@vitaliidm @kibanamachine @spong
[RAM][Security Solution][Alerts] moves legacy actions migration to ru…
f8c16c1 
…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
@vitaliidm @kibanamachine
@spong @saarikabhasi
[RAM][Security Solution][Alerts] moves legacy actions migration to ru…
57cbaca 
…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>
@vitaliidm
Copy link
Contributor Author

vitaliidm commented Apr 20, 2023 •
edited

Issue appears to fixed in #154990

Screen.Recording.2023-04-20.at.11.04.48.mov

I also noticed muteAll is not getting set to true on rule creation anymore, which is probably a side effect from changes in above PR. I think this is a correct behaviour. Unless user explicitly mutes rules, we shouldn't mute them

@e40pud , have you explicitly set muteAll to false on creation w/o actions?

cc: @marshallmain

@vitaliidm vitaliidm mentioned this issue Apr 20, 2023
Merged
3 tasks
@e40pud
Copy link
Contributor

e40pud commented Apr 20, 2023

@vitaliidm This was fixed by this PR #154804 from @maximpn

nikitaindik pushed a commit to nikitaindik/kibana that referenced this issue Apr 25, 2023
@vitaliidm @kibanamachine
@spong @nikitaindik
[RAM][Security Solution][Alerts] moves legacy actions migration to ru…
887129d 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
8.8 candidate bug Fixes for quality problems that affect the customer experience 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
Milestone
No milestone
Development

No branches or pull requests
3 participants
@e40pud @elasticmachine @vitaliidm