[Security Solution] Disable legacy rules on upgrade to 8.x by madirey · Pull Request #121442 · elastic/kibana

madirey · 2021-12-16T18:40:56Z

Summary

This PR modifies the alerting migration for Detection Engine rules to disable each rule on upgrade to 8.x. This ensures that legacy rules will no longer fire, in the case that the customer failed to disable the rules manually before upgrading. The rules will need to be re-enabled once the upgrade is complete, which is in line with current customer guidance.

@jmikell821 Can we verify that our documentation reflects the steps above? They should no longer need to disable their rules before upgrading, but they will need to go back in and re-enable all their rules post-upgrade (from pre-8.0 to 8.x). cc: @marshallmain

@MadameSheema Should we add upgrade tests to cover this? cc: @rylnd

There is a related issue where dupes can occur after upgrading ... I'll create a separate issue for that.

For maintainers

This was checked for breaking API changes and was labeled appropriately

…rule

This reverts commit 021ec0f.

…rule

elasticmachine · 2021-12-21T19:18:56Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

elasticmachine · 2021-12-21T19:18:57Z

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

madirey · 2022-01-04T02:09:33Z

@elasticmachine merge upstream

kibanamachine · 2022-01-04T02:09:45Z

expected head sha didn’t match current head ref.

mikecote · 2022-01-04T18:56:35Z

x-pack/plugins/alerting/server/saved_objects/migrations.ts

        attributes: {
          ...doc.attributes,
          alertTypeId: ruleTypeMappings[ruleType],
+          enabled: false,


@elastic/kibana-alerting-services is this sufficient for disabling a rule during migration. Or because the task still exists, the rule runs regardless?

As of 8.1, we have this logic in place to skip running rules when they are disabled: #119239

That hasn't been backported to 8.0 though, so maybe it needs to be to support this PR

Even if we backported the above-mentioned PR, I'm wondering if 2 tasks will run when the rule does get enabled. Since we're just setting enabled to false here but the scheduled task still exists, a new task document will get created when the rule gets enabled and then both tasks will run

As part of this PR, we also delete the code that registered the old rule type so the old task still exists but it doesn't get picked up by task manager to actually execute since it can't find the executor logic.

Gotcha! Verified that this all works as expected. It will just leave an API key that is never invalidated. Is that a big deal @mikecote ?

From a platform perspective, it is ok, it will not cause any harm leaving API keys behind. If folks in the security solution are ok with this caveat, we should be ok.

madirey · 2022-01-06T15:56:38Z

@elasticmachine merge upstream

kibanamachine · 2022-01-06T15:56:40Z

merge conflict between base and head

…rule

…disable-legacy-rule

ymao1

Can we update the unit tests for x-pack/plugins/alerting/server/saved_objects/migrations.ts to test for this migration? We also have alerting migration functional tests but it sounds like there might already be security solutions functional tests to cover this?

…rule

madirey · 2022-01-10T15:29:35Z

@ymao1 Added tests!

ymao1

LGTM! Thanks for adding the tests.

kibana-ci · 2022-01-10T17:11:25Z

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`securitySolution`	67	66	-1

ESLint disabled line counts

id	before	after	diff
`securitySolution`	453	452	-1

Total ESLint disabled count

id	before	after	diff
`securitySolution`	520	518	-2

History

💚 Build #16199 succeeded 1a27a72
💚 Build #15581 succeeded 4896404
💚 Build #14632 succeeded feaa923
💔 Build #14394 failed 6b7851a
💔 Build #14294 failed 4766e64
💚 Build #13719 succeeded 1ab12e2

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

kibanamachine · 2022-01-10T19:01:46Z

💔 All backports failed

Status	Branch	Result
❌	8.0	Backport failed because of merge conflicts

How to fix

Re-run the backport manually:

node scripts/backport --pr 121442

Questions ?

Please refer to the Backport tool documentation

…21442) * Disable legacy rule and notify user to upgrade * Ensure rules are disabled on upgrade * Fix dupe detection on upgrade * Revert "Fix dupe detection on upgrade" This reverts commit 021ec0f. * Add legacy notification * Add tests for 8.0 security_solution rule migration Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 1ddb647) # Conflicts: # x-pack/plugins/alerting/server/saved_objects/migrations.ts # x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts # x-pack/plugins/security_solution/server/plugin.ts

madirey · 2022-01-11T19:57:03Z

@mikecote @ymao1 @marshallmain The behavior I'm observing is that the rule continues to run after upgrading even though it's disabled during the migration (but it's running the new code...). Does that make sense based on the current functionality? If so, then can we expect users won't need to go manually enable the rule after upgrading?

UPDATE: Please disregard, this is working as expected!

ymao1 · 2022-01-11T20:02:54Z

That would not be what I expect (and also not what I observed when I tried running it locally).

Both the alerting rule & the scheduled task with the legacy task type should exist after the migration. The first time task manager claims the task, it should mark it as unrecognized because the task type is no longer registered and therefore no longer recognized by task manager. This should prevent this task from being claimed in the future.

A new scheduled task with the new task task type should not be created until the rule is enabled by the user.

…1442) (#122695) * [Security Solution] Disable legacy rules on upgrade to 8.x (#121442) * Disable legacy rule and notify user to upgrade * Ensure rules are disabled on upgrade * Fix dupe detection on upgrade * Revert "Fix dupe detection on upgrade" This reverts commit 021ec0f. * Add legacy notification * Add tests for 8.0 security_solution rule migration Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 1ddb647) # Conflicts: # x-pack/plugins/alerting/server/saved_objects/migrations.ts # x-pack/plugins/security_solution/server/lib/detection_engine/signals/signal_rule_alert_type.ts # x-pack/plugins/security_solution/server/plugin.ts * RawRule should be RawAlert in 8.0

madirey · 2022-01-12T18:54:24Z

@ymao1 Thanks, I'm not sure what I was seeing, but I'm not seeing it now. I think this is working as expected. 👍

Disable legacy rule and notify user to upgrade

1ab12e2

madirey added release_note:deprecation v8.0.0 v8.1.0 labels Dec 16, 2021

madirey mentioned this pull request Dec 16, 2021

[Security Solution] Clean up legacy siem.signals tasks on upgrade #121257

Closed

9 tasks

madirey added 5 commits December 20, 2021 11:48

Ensure rules are disabled on upgrade

fb3655a

Merge branch 'main' of github.com:elastic/kibana into disable-legacy-…

4766e64

…rule

Fix dupe detection on upgrade

021ec0f

Revert "Fix dupe detection on upgrade"

c5853fc

This reverts commit 021ec0f.

Merge branch 'main' of github.com:elastic/kibana into disable-legacy-…

6b7851a

…rule

madirey changed the title ~~[Security Solution] Disable legacy rule and notify user to upgrade~~ [Security Solution] Disable legacy rules on upgrade to 8.x Dec 21, 2021

madirey added 2 commits December 21, 2021 13:26

Merge branch 'main' of github.com:elastic/kibana into disable-legacy-…

e0079e0

…rule

Add legacy notification

feaa923

madirey added needs_docs Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Alerts Security Detection Alerts Area Team Team:Detections and Resp Security Detection Response Team labels Dec 21, 2021

madirey marked this pull request as ready for review December 21, 2021 19:18

madirey requested review from a team as code owners December 21, 2021 19:18

Merge branch 'main' into disable-legacy-rule

4896404

marshallmain approved these changes Jan 4, 2022

View reviewed changes

mikecote reviewed Jan 4, 2022

View reviewed changes

jmikell821 mentioned this pull request Jan 5, 2022

[DOCS] Pre-upgrade steps: disable rules before upgrading to 8.0 elastic/security-docs#1374

Closed

madirey added 2 commits January 6, 2022 14:05

Merge branch 'main' of github.com:elastic/kibana into disable-legacy-…

5ba6fbe

…rule

Merge branch 'disable-legacy-rule' of github.com:madirey/kibana into …

1a27a72

…disable-legacy-rule

ymao1 reviewed Jan 6, 2022

View reviewed changes

madirey added 2 commits January 10, 2022 09:39

Merge branch 'main' of github.com:elastic/kibana into disable-legacy-…

5d07f79

…rule

Add tests for 8.0 security_solution rule migration

7d326d6

ymao1 approved these changes Jan 10, 2022

View reviewed changes

madirey added the auto-backport Deprecated - use backport:version if exact versions are needed label Jan 10, 2022

madirey merged commit 1ddb647 into elastic:main Jan 10, 2022

madirey deleted the disable-legacy-rule branch January 10, 2022 18:57

madirey mentioned this pull request Jan 11, 2022

[8.0] [Security Solution] Disable legacy rules on upgrade to 8.x (#121442) #122695

Merged

madirey self-assigned this Jan 11, 2022

ymao1 mentioned this pull request Feb 15, 2022

[Task Manager] Adding list of explicitly de-registered task types #123963

Merged

1 task

Conversation

madirey commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

For maintainers

Uh oh!

elasticmachine commented Dec 21, 2021

Uh oh!

elasticmachine commented Dec 21, 2021

Uh oh!

madirey commented Jan 4, 2022

Uh oh!

kibanamachine commented Jan 4, 2022

Uh oh!

mikecote Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

ymao1 Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

ymao1 Jan 4, 2022

Choose a reason for hiding this comment

Uh oh!

marshallmain Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

ymao1 Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

mikecote Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

madirey commented Jan 6, 2022

Uh oh!

kibanamachine commented Jan 6, 2022

Uh oh!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

madirey commented Jan 10, 2022

Uh oh!

ymao1 left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Jan 10, 2022

💚 Build Succeeded

Metrics [docs]

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

Uh oh!

kibanamachine commented Jan 10, 2022

💔 All backports failed

How to fix

Questions ?

Uh oh!

madirey commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ymao1 commented Jan 11, 2022

Uh oh!

madirey commented Jan 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

madirey commented Dec 16, 2021 •

edited

Loading

madirey commented Jan 11, 2022 •

edited

Loading