[Security Solution][Legacy Actions] - Update legacy action migration to account for more edge cases #130511

yctercero · 2022-04-18T21:37:29Z

Summary

Updates the legacy actions migration code to account for edge cases we had not initially caught. Thanks to testing from some teammates, they reported seeing the following behavior:

Rules created pre 7.16 with no actions still create the legacy action sidecar (but not a siem.notifications legacy actions alert) which upon migration to 7.16+ was not being deleted
Rules created pre 7.16 with actions that run on every rule run create the legacy action sidecar(but not a siem.notifications legacy actions alert) which upon migration to 7.16+ was not being deleted
Rules created pre 7.16 with actions that were never enabled until 8.x did not have a siem.notifications legacy actions alert type created

Because the legacy migration code relied on checking if a corresponding siem.notifications SO existed to kick off the necessary cleanup/migration, the above edge cases were not being caught.

Working to migrate the legacy actions can definitely scramble your head a bit so I'll try to break it down below and made sure to add plenty of tests (e2e, unit) to check the logic.

Actions	Pre migrated rule	Post migrated rule
None	- NO `siem.notifications` SO created - `siem-detection-engine-rule-actions` SO created - Empty actions array on rule params	- `siem-detection-engine-rule-actions` DELETED - `throttle` is `null` - `notifyWhen` is `onActiveAlert` - `actions` is `[]`
Rule run	- NO `siem.notifications` SO created - `siem-detection-engine-rule-actions` SO created - action lives on rule params	- `siem-detection-engine-rule-actions` DELETED - actions continue to live on rule params - `throttle` is `null` - `notifyWhen` is `onActiveAlert`
Hourly	- `siem.notifications` SO created - `siem-detection-engine-rule-actions` SO created - actions array on rule params empty	- s`iem-detection-engine-rule-actions` DELETED - `siem.notifications` DELETED - actions moved to live on rule params - `throttle` is `1h` - `notifyWhen` is `onThrottleInterval`
Daily	- `siem.notifications` SO created - `siem-detection-engine-rule-actions` SO created - actions array on rule params empty	- `siem-detection-engine-rule-actions` DELETED - `siem.notifications` DELETED - actions moved to live on rule params - `throttle` is `1d` - `notifyWhen` is `onThrottleInterval`
Weekly	- `siem.notifications` SO created - `siem-detection-engine-rule-actions` SO created - actions array on rule params empty	- `siem-detection-engine-rule-actions` DELETED - `siem.notifications` DELETED - actions moved to live on rule params - `throttle` is `7d` - `notifyWhen` is `onThrottleInterval`

Testing

Check out 7.15 branch locally
Create 5 enabled rules: with no actions, with an action triggered per each rule execution, with an action triggered every hour, every day, every week
Use the scripts below in Dev Tools to check that conditions outlined in the above table are true
Check out the 7.17 branch locally (required to go to 7.17 before jumping to 8.0)
Upgrade the stack to 7.17 - run ES, then run Kibana
Check out this feature branch and upgrade the stack to 8.2.0 - run ES, then run Kibana
Execute the same requests in Dev Tools and verified that results match those outlined in table above under "Pre migrated rule"
Bulk enabled all the rules
Execute the same requests in Dev Tools and verified that results match those outlined in table above under "Post migrated rule"

Useful scripts:

## Gets the alerting rule type created for the legacy action
GET .kibana/_search
{
  "query": {
    "term": {
      "alert.alertTypeId": "siem.notifications"
    }
  }
}

## Gets the siem sidecar actions
GET .kibana/_search
{
  "query": {
    "term": {
      "type": {
        "value": "siem-detection-engine-rule-actions"
      }
    }
  }
}

## Gets the rules pre 8.0
## For legacy actions with an interval other than on every rule run, the `actions` array is empty and 
## `throttle : null`, and `notifyWhen : onActiveAlert`.
GET .kibana/_search
{
  "query": {
    "term": {
      "alert.alertTypeId": "siem.signals"
    }
  }
}

## Gets the query rules post 8.0
## For NON legacy actions that have successfully migrated actions, the `actions` array is filled out on the rule,
## the `throttle` corresponds to the selected interval and `notifyWhen: onThrottleInterval`
GET .kibana/_search
{
  "query": {
    "term": {
      "alert.alertTypeId": "siem.queryRule"
    }
  }
}

Followup Needed

Performance testing

Checklist

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…s_migration_logic

…-fix'

…s_migration_logic

…ro/kibana into updating_actions_migration_logic

…s_migration_logic

elasticmachine · 2022-04-21T03:27:51Z

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

MadameSheema · 2022-04-21T12:39:35Z

@elasticmachine merge upstream

.../plugins/security_solution/server/lib/detection_engine/routes/__mocks__/request_responses.ts

…ons_migration_logic

…ro/kibana into updating_actions_migration_logic

…s_migration_logic

madirey · 2022-04-26T18:19:21Z

x-pack/plugins/security_solution/server/lib/detection_engine/rules/utils.test.ts

+      jest.resetAllMocks();
+    });
+
+    test('it does no cleanup or migration if no legacy reminants found', async () => {


Small typo here (remnants)

banderror

Tested locally, it worked fine and as expected. No sidecar objects after enabling the rules in the branch!

The code LGTM -- this implementation is of some really high quality. The tests look awesome, thank you for adding them and for putting so much effort into this fix.

Suggestion: it would be great to put the table from the PR description somewhere close to the legacyMigrate implementation, or to some dev docs. It really helps with understanding what this migration is all about.

banderror · 2022-04-27T21:50:40Z

x-pack/plugins/security_solution/server/lib/detection_engine/rules/utils.test.ts

@@ -588,4 +651,415 @@ describe('utils', () => {
      ]);
    });
  });
+
+  describe('#legacyMigrate', () => {


Tests are awesome here, clean and easy to understand. Thank you @yctercero for working on them, appreciate a lot!

banderror · 2022-04-27T21:52:15Z

x-pack/plugins/security_solution/server/lib/detection_engine/rules/utils.ts

+  // If the legacy notification rule type ("siem.notification") exist,
+  // migration and cleanup are needed
+  if (siemNotificationsExist) {
+    await rulesClient.delete({ id: siemNotification.data[0].id });
+  }
+  // If legacy notification sidecar ("siem-detection-engine-rule-actions")
+  // exist, migration and cleanup are needed
+  if (legacyRuleNotificationSOsExist) {
+    // Delete the legacy sidecar SO
+    await savedObjectsClient.delete(
+      legacyRuleActionsSavedObjectType,
+      legacyRuleActionsSO.saved_objects[0].id
+    );
+
+    // If "siem-detection-engine-rule-actions" notes that `ruleThrottle` is
+    // "no_actions" or "rule", rule has no actions or rule is set to run
+    // action on every rule run. In these cases, sidecar deletion is the only
+    // cleanup needed and updates to the "throttle" and "notifyWhen". "siem.notification" are
+    // not created for these action types
+    if (
+      legacyRuleActionsSO.saved_objects[0].attributes.ruleThrottle === 'no_actions' ||
+      legacyRuleActionsSO.saved_objects[0].attributes.ruleThrottle === 'rule'
+    ) {
+      return rule;
+    }
+
+    // Use "legacyRuleActionsSO" instead of "siemNotification" as "siemNotification" is not created
+    // until a rule is run and added to task manager. That means that if by chance a user has a rule
+    // with actions which they have yet to enable, the actions would be lost. Instead,
+    // "legacyRuleActionsSO" is created on rule creation (pre 7.15) and we can rely on it to be there


I really appreciate all the comments here!

...test/detection_engine_api_integration/security_and_spaces/tests/legacy_actions_migrations.ts

banderror · 2022-04-27T21:57:17Z

...test/detection_engine_api_integration/security_and_spaces/tests/legacy_actions_migrations.ts

+    it('migrates legacy actions for rule with no actions', async () => {
+      const soId = '9095ee90-b075-11ec-bb3f-1f063f8e06cf';
+      const ruleId = '2297be91-894c-4831-830f-b424a0ec84f0';
+      const legacySidecarId = '926668d0-b075-11ec-bb3f-1f063f8e06cf';
+
+      // check for legacy sidecar action
+      const sidecarActionSO = await getLegacyActionSOById(es, legacySidecarId);
+      expect(sidecarActionSO.hits.hits.length).to.eql(1);
+
+      // check for legacy notification SO
+      // should not have been created for a rule with no actions
+      const legacyNotificationSO = await getLegacyActionNotificationSOById(es, soId);
+      expect(legacyNotificationSO.hits.hits.length).to.eql(0);


These tests are awesome too!

x-pack/test/functional/es_archives/legacy_actions/data.json

…s_migration_logic

kibana-ci · 2022-05-04T15:45:24Z

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	442	443	+1

Total ESLint disabled count

id	before	after	diff
`securitySolution`	511	512	+1

History

💔 Build #42224 failed 2dd9b78
💔 Build #40654 failed 1c1da12
💔 Build #40639 failed 8ad20a3
💚 Build #40306 succeeded c648433
💚 Build #39799 succeeded b6b3c71

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

cc @yctercero

kibanamachine · 2022-05-04T15:45:45Z

⚪ Backport skipped

The pull request was not backported as there were no branches to backport to. If this is a mistake, please apply the desired version labels or run the backport tool manually.

Manual backport

To create the backport manually run:

node scripts/backport --pr 130511

Questions ?

Please refer to the Backport tool documentation

…to account for more edge cases (elastic#130511) ## Summary Updates the legacy actions migration code to account for edge cases we had not initially caught. Thanks to testing from some teammates, they reported seeing the following behavior: - Rules created pre 7.16 with no actions still create the legacy action sidecar (but not a `siem.notifications` legacy actions alert) which upon migration to 7.16+ was not being deleted - Rules created pre 7.16 with actions that run on every rule run create the legacy action sidecar(but not a `siem.notifications` legacy actions alert) which upon migration to 7.16+ was not being deleted - Rules created pre 7.16 with actions that were never enabled until 8.x did not have a `siem.notifications` legacy actions alert type created Because the legacy migration code relied on checking if a corresponding `siem.notifications` SO existed to kick off the necessary cleanup/migration, the above edge cases were not being caught.

yctercero and others added 14 commits April 1, 2022 07:31

updating actions migration logic with added unit tests

55e9c82

updating e2d and unit tests

f5428e8

Merge branch 'main' of github.com:elastic/kibana into updating_action…

b5d71fc

…s_migration_logic

cleanup

5d18fdd

cleanup

bfa25f3

cleanup

524e102

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

e756f27

…-fix'

need updates to jest tests, cleanup

bc1c965

Merge branch 'main' of github.com:elastic/kibana into updating_action…

0b4cbd5

…s_migration_logic

Merge branch 'updating_actions_migration_logic' of github.com:ycterce…

8ac5522

…ro/kibana into updating_actions_migration_logic

cleanup debugs

f617c83

working on updting jest tests

4cfe863

Merge branch 'main' of github.com:elastic/kibana into updating_action…

f4e861f

…s_migration_logic

updated code and tests to add logic for multiple actions

1cf24fa

yctercero changed the title ~~Updating actions migration logic~~ [Security Solution][Legacy Actions] - Update legacy action migration to account for more edge cases Apr 20, 2022

yctercero self-assigned this Apr 20, 2022

Merge branch 'main' of github.com:elastic/kibana into updating_action…

5c37cab

…s_migration_logic

yctercero added auto-backport Deprecated - use backport:version if exact versions are needed ci:deploy-cloud labels Apr 20, 2022

yctercero added 3 commits April 20, 2022 10:52

Merge branch 'main' of github.com:elastic/kibana into updating_action…

fb9cc25

…s_migration_logic

lint cleanup

0f599b9

update test util for consistency

1689670

yctercero marked this pull request as ready for review April 21, 2022 03:27

yctercero requested review from a team as code owners April 21, 2022 03:27

Merge branch 'main' into updating_actions_migration_logic

b6b3c71

marshallmain reviewed Apr 25, 2022

View reviewed changes

.../plugins/security_solution/server/lib/detection_engine/routes/__mocks__/request_responses.ts Outdated Show resolved Hide resolved

yctercero added 5 commits April 25, 2022 10:01

Merge branch 'main' of github.com:yctercero/kibana into updating_acti…

e2ed435

…ons_migration_logic

updating tests per feedback

dc2af16

Merge branch 'updating_actions_migration_logic' of github.com:ycterce…

c648433

…ro/kibana into updating_actions_migration_logic

Merge branch 'main' of github.com:elastic/kibana into updating_action…

8ad20a3

…s_migration_logic

fix errors post merge

1c1da12

madirey reviewed Apr 26, 2022

View reviewed changes

banderror approved these changes Apr 27, 2022

View reviewed changes

yctercero mentioned this pull request May 3, 2022

[Security Solution][Actions] - Actions are not consistently migrating on rule disable/enable post 8.0 upgrade #127918

Closed

yctercero added 2 commits May 3, 2022 13:00

Merge branch 'main' of github.com:elastic/kibana into updating_action…

68e8dff

…s_migration_logic

update test

2dd9b78

yctercero enabled auto-merge (squash) May 3, 2022 20:08

fix esarchiver reference

b98f3da

yctercero merged commit 29705c0 into elastic:main May 4, 2022

kibanamachine added the backport:skip This commit does not require backporting label May 4, 2022

yctercero deleted the updating_actions_migration_logic branch May 4, 2022 15:47

tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022

yctercero mentioned this pull request Apr 13, 2023

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

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Legacy Actions] - Update legacy action migration to account for more edge cases #130511

[Security Solution][Legacy Actions] - Update legacy action migration to account for more edge cases #130511

yctercero commented Apr 18, 2022 •

edited

Loading

elasticmachine commented Apr 21, 2022

MadameSheema commented Apr 21, 2022

madirey Apr 26, 2022

banderror left a comment •

edited

Loading

banderror Apr 27, 2022

banderror Apr 27, 2022

banderror Apr 27, 2022

kibana-ci commented May 4, 2022

ESLint disabled line counts

Total ESLint disabled count

kibanamachine commented May 4, 2022

[Security Solution][Legacy Actions] - Update legacy action migration to account for more edge cases #130511

[Security Solution][Legacy Actions] - Update legacy action migration to account for more edge cases #130511

Conversation

yctercero commented Apr 18, 2022 • edited Loading

Summary

Testing

Followup Needed

Checklist

For maintainers

elasticmachine commented Apr 21, 2022

MadameSheema commented Apr 21, 2022

madirey Apr 26, 2022

Choose a reason for hiding this comment

banderror left a comment • edited Loading

Choose a reason for hiding this comment

banderror Apr 27, 2022

Choose a reason for hiding this comment

banderror Apr 27, 2022

Choose a reason for hiding this comment

banderror Apr 27, 2022

Choose a reason for hiding this comment

kibana-ci commented May 4, 2022

💚 Build Succeeded

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

kibanamachine commented May 4, 2022

⚪ Backport skipped

Manual backport

Questions ?

yctercero commented Apr 18, 2022 •

edited

Loading

banderror left a comment •

edited

Loading