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

[RAM][SECURITY SOLUTION][ALERTS] Research on migrating security legacy action inside of rule client #151919

Closed
XavierM opened this issue Feb 22, 2023 · 6 comments
Closed

[RAM][SECURITY SOLUTION][ALERTS] Research on migrating security legacy action inside of rule client #151919

XavierM opened this issue Feb 22, 2023 · 6 comments
Assignees
@vitaliidm
Labels
8.8 candidate Team:Detection Alerts Security Detection Alerts Area Team Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)

Comments

@XavierM
Copy link
Contributor

XavierM commented Feb 22, 2023

We would like to find a solution to integrate the migration of legacy actions from security solution inside of the platform.
@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)

@XavierM
Copy link
Contributor Author

XavierM commented Feb 22, 2023 •
edited
Loading

@vitaliidm, I talked to @mikecote and @kobelb, I think we have a plan. We would like to create an async migrate hooks in the rule type registry (We will still need to brainstorm on the name ;) ).
In the read of rule(s), we would like to format the data as expected meaning if this security rule is associated to a legacy actions, we will re-format the data as expected without any migrations. Then, when the user do an update of the rule (meaning it can be on a tag and/or enable/disable etc...), at this time we will perform the migration of the legacy action to the rule actions attributes.
We discussed a lot of different possibility but we all agree to this one. Let's discuss the detailed when you are ready.

@XavierM XavierM moved this from Awaiting Triage to Todo in AppEx: ResponseOps - Rules & Alerts Management Feb 22, 2023
@vitaliidm
Copy link
Contributor

vitaliidm commented Feb 24, 2023 •
edited
Loading

List of rule APIs that migrate legacy actions:

  1. update prebuilt rules reference,rulesClient.update
  2. bulk actions reference
  • enable, disable, delete, duplicate, edit
  1. bulk delete [DEPRECATED since 8.2] reference
  2. bulk patch [DEPRECATED since 8.2] reference,rulesClient.update
  3. bulk update [DEPRECATED since 8.2] reference,rulesClient.update
  4. delete reference
  5. patch reference, rulesClient.update
  6. update reference, rulesClient.update
  7. import rules reference,rulesClient.update

List of read APIs, where legacy actions fetched and integrated into rule

  1. Find rules reference
  2. Read rule reference
  3. Export all rules reference
  4. Export rules by ids reference

where fetched legacy actions used in rule transformation to Detections API response

@vitaliidm vitaliidm mentioned this issue Feb 28, 2023
Closed
9 tasks
@vitaliidm
Copy link
Contributor

Executing migration through the hook has few challenges we are looking to address:

  • when rule updated, new API key will be generated, which is smth we want to avoid during enable/disable calls
  • executing hook within rulesClient method, requires direct exposure of rulesClient methods as hook argument
  • another approach, when migrated rule is not saved within a hook, but merged into rule before saving, looks very complex and requires versioning of migrations. So, migrated properties can be merged safely in created rule payload

Instead, during the latest discussion, we are thinking of implementing migration through the Task Manager task.
It would run once, on the start ok Kibana. Fetch all legacy rules and SOs, delete them and migrate rules.
By using savedObjectClient directly, we would be able to update rule w/o regenerating apiKeys
Once task is completed, there should not be left any legacy actions on the instance

@XavierM , @mikecote

@vitaliidm vitaliidm mentioned this issue Mar 10, 2023
Merged
2 tasks
@mikecote
Copy link
Contributor

I spoke with @XavierM and @kobelb last week about this, we iterated our thinking and settled on the following:

  • Before executing a rule, we run the migration on read (transform the data, but not save)
  • Before updating a rule on enable, etc we run the migration on the data and save the migrated data
  • The migration code would live within the alerting plugin for the time being without a re-usable "hook". This buys us time to figure out how to make migrations flexible in a zero-downtime upgrade world (details unknown at this time)

We can meet again about the approach, if needed.

@vitaliidm vitaliidm mentioned this issue Apr 5, 2023
Open
@vitaliidm vitaliidm moved this from In Progress to In Review in AppEx: ResponseOps - Rules & Alerts Management Apr 6, 2023
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>
@vitaliidm
Copy link
Contributor

implemented in #153101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vitaliidm vitaliidm

Labels
8.8 candidate Team:Detection Alerts Security Detection Alerts Area Team Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)
Projects
No open projects
AppEx: ResponseOps - Rules & Alerts Management
Status: Done
Milestone
No milestone
Development

No branches or pull requests
5 participants
@XavierM @peluja1012 @mikecote @elasticmachine @vitaliidm