[Security Solution] Support rule type change during upgrade by xcrzx · Pull Request #200199 · elastic/kibana

xcrzx · 2024-11-14T14:51:27Z

Resolves: #180395

Summary

Supports rule type change when upgrading a rule to a newer version, covering scenarios where the current version's rule type differs from the target version. Regardless of the feature flag, the rule should be updated to the target version.

Behavior with Rule Customization OFF

No changes expected. A rule with a rule type change can be updated through:

The single rule update option ✅
The bulk update option ✅
The Update All option ✅

Behavior with Rule Customization ON

Regardless of whether the rule has customizations:

The rule cannot be updated using the single update option ❌
The rule is excluded from bulk updates ❌
The rule is excluded from Update All ❌
The rule can only be updated after manually reviewing changes via the update flyout ✅

The update flyout includes a warning message, advising users to back up any customizations the rule may have. The flyout is in read-only mode:

elasticmachine · 2024-11-14T15:14:19Z

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

elasticmachine · 2024-11-14T15:14:20Z

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

elasticmachine · 2024-11-14T15:14:22Z

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

maximpn

@xcrzx thanks for addressing rule type change 👍

I don't any critical comments though some of them worth to be checked before approval. The only concern that I have is related to rules without customization. We still show a callout for such rules and users might be confused by the message

Testing didn't reveal any issues. Rule upgrade works w/ and w/o feature flag enabled.

maximpn · 2024-11-18T10:13:23Z

x-pack/plugins/security_solution/public/detection_engine/rule_management/api/api.ts

nit: Shouldn't it be PerformUpgradeSpecificRulesRequest?

maximpn · 2024-11-18T10:16:13Z

...ngine/rule_management_ui/components/rules_table/upgrade_prebuilt_rules_table/translations.ts

I'm curious on your thoughts on shouldn't we use translation keys like xpack.securitySolution.ruleManagement... or xpack.securitySolution.detectionEngine.ruleManagement...?

Rule management is considered a part of Detection Engine though Rule Management and Detection Engine are kind of independent things.

maximpn · 2024-11-18T10:36:11Z

...components/rules_table/upgrade_prebuilt_rules_table/upgrade_prebuilt_rules_table_context.tsx

nit: Maybe we could move out upgradeRulesToResolved and upgradeRulesToTarget to a separate hooks? This will make UpgradePrebuiltRulesTableContextProvider smaller and simpler to comprehend.

maximpn · 2024-11-18T10:41:32Z

...components/rules_table/upgrade_prebuilt_rules_table/upgrade_prebuilt_rules_table_context.tsx

Suggested change

let updateTabContent = (

<PerFieldRuleDiffTab

header={hasRuleTypeChange ? <RuleTypeChangeCallout /> : null}

ruleDiff={ruleUpgradeState.diff}

/>

);

// Show the resolver tab only if rule customization is enabled and there

// is no rule type change. In case of rule type change users can't resolve

// conflicts, only accept the target rule.

if (isPrebuiltRulesCustomizationEnabled && !hasRuleTypeChange) {

updateTabContent = (

<RuleUpgradeConflictsResolverTab

ruleUpgradeState={ruleUpgradeState}

setRuleFieldResolvedValue={setRuleFieldResolvedValue}

/>

);

}

// Show the resolver tab only if rule customization is enabled and there

// is no rule type change. In case of rule type change users can't resolve

// conflicts, only accept the target rule.

const updateTabContent =

isPrebuiltRulesCustomizationEnabled && !hasRuleTypeChange ? (

<RuleUpgradeConflictsResolverTab

ruleUpgradeState={ruleUpgradeState}

setRuleFieldResolvedValue={setRuleFieldResolvedValue}

/>

) : (

<PerFieldRuleDiffTab

header={hasRuleTypeChange ? <RuleTypeChangeCallout /> : null}

ruleDiff={ruleUpgradeState.diff}

/>

);

maximpn · 2024-11-18T10:47:57Z

.../public/detection_engine/rule_management/components/rule_details/per_field_rule_diff_tab.tsx

Technically it's not a header according to the use case scenario. Semantically headers should be wrapped in h1-6 tags. In fact we display a warning here. On top of that the component already has a header RuleDiffHeaderBar. Maybe we could use standard children prop or just rename it to headerAppend?

I think you are confusing heading elements with headers.

maximpn · 2024-11-18T11:04:10Z

...components/rules_table/upgrade_prebuilt_rules_table/upgrade_prebuilt_rules_table_context.tsx

Using diff.fields.version?.target_version or rulesUpgradeState[ruleId].current_rule.version looks weird or I miss something here.

The logic is straightforward. There is a diff if we have some fields changed in a new prebuilt rules package version. When one or more fields changed it's expected to have version bumped. This way version will always be in the diff and it equals to target_rule.version.

Could we use version: rulesUpgradeState[ruleId].target_rule.version here?

I don't know why this logic was written in this specific way, but using version: rulesUpgradeState[ruleId].target_rule.version seems more straightforward to me

xcrzx · 2024-11-18T15:23:00Z

The only concern that I have is related to rules without customization. We still show a callout for such rules and users might be confused by the message

As discussed during demo, I'll add a separate task to introduce a better handling of the situation when rules are not customized. But currently, it is not a priority

maximpn

@xcrzx thanks for addressing my comments 🙏

I retested the PR w/ and w/o prebuiltRulesCustomizationEnabled feature flag enabled and it works as expected.

elasticmachine · 2024-11-27T16:30:59Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 3faa6d1

Failed CI Steps

Test Failures

[job] [logs] Jest Tests #12 / AgentStatusFilter Shows tour and inactive count if first time seeing newly inactive agents
[job] [logs] FTR Configs #28 / management Index patterns on aliases discover verify hits should be able to discover and verify no of hits for alias2
[job] [logs] FTR Configs #28 / management Index patterns on aliases discover verify hits should be able to discover and verify no of hits for alias2

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	6264	6265	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	13.4MB	13.4MB	+939.0B

History

💔 Build #254863 failed 8ddaf76
💔 Build #254536 failed fc8cad8
💔 Build #254447 failed 78dadbd
💛 Build #254357 was flaky 07db9c46b0aa85b6c28b7232a6eae5d9094e34eb

cc @xcrzx

kibanamachine · 2024-11-27T18:00:01Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12055840470

…200199) (cherry picked from commit 1dd2400)

kibanamachine · 2024-11-27T18:05:00Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…00199) (#202048) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] Support rule type change during upgrade (#200199)](#200199)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Dmitrii Shevchenko <dmitrii.shevchenko@elastic.co>

…200199)

xcrzx self-assigned this Nov 14, 2024

xcrzx marked this pull request as ready for review November 14, 2024 15:14

xcrzx requested a review from a team as a code owner November 14, 2024 15:14

xcrzx requested a review from maximpn November 14, 2024 15:14

maximpn reviewed Nov 18, 2024

View reviewed changes

banderror added v8.18.0 and removed v8.17.0 labels Nov 23, 2024

xcrzx requested a review from maximpn November 26, 2024 10:33

Support rule type change during upgrade

78dadbd

maximpn approved these changes Nov 26, 2024

View reviewed changes

Dmitrii Shevchenko added 2 commits November 26, 2024 16:17

Merge branch 'main' into rule-type-change

fc8cad8

Merge branch 'main' into rule-type-change

afc762a

maximpn mentioned this pull request Nov 27, 2024

[Security Solution] Add ES|QL Query editable component #199887

Merged

Dmitrii Shevchenko added 2 commits November 27, 2024 11:57

Merge branch 'main' into rule-type-change

8ddaf76

Merge branch 'main' into rule-type-change

3faa6d1

xcrzx merged commit 1dd2400 into elastic:main Nov 27, 2024

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 27, 2024

[Security Solution] Support rule type change during upgrade (elastic#…

9990a66

…200199) (cherry picked from commit 1dd2400)

kibanamachine mentioned this pull request Nov 27, 2024

[8.x] [Security Solution] Support rule type change during upgrade (#200199) #202048

Merged

xcrzx deleted the rule-type-change branch November 28, 2024 09:51

CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024

[Security Solution] Support rule type change during upgrade (elastic#…

48d9e97

…200199)

-      let updateTabContent = (
-        <PerFieldRuleDiffTab
-          header={hasRuleTypeChange ? <RuleTypeChangeCallout /> : null}
-          ruleDiff={ruleUpgradeState.diff}
-        />
-      );
-      // Show the resolver tab only if rule customization is enabled and there
-      // is no rule type change. In case of rule type change users can't resolve
-      // conflicts, only accept the target rule.
-      if (isPrebuiltRulesCustomizationEnabled && !hasRuleTypeChange) {
-        updateTabContent = (
-          <RuleUpgradeConflictsResolverTab
-            ruleUpgradeState={ruleUpgradeState}
-            setRuleFieldResolvedValue={setRuleFieldResolvedValue}
-          />
-        );
-      }
+      // Show the resolver tab only if rule customization is enabled and there
+      // is no rule type change. In case of rule type change users can't resolve
+      // conflicts, only accept the target rule.
+      const updateTabContent =
+        isPrebuiltRulesCustomizationEnabled && !hasRuleTypeChange ? (
+          <RuleUpgradeConflictsResolverTab
+            ruleUpgradeState={ruleUpgradeState}
+            setRuleFieldResolvedValue={setRuleFieldResolvedValue}
+          />
+        ) : (
+          <PerFieldRuleDiffTab
+            header={hasRuleTypeChange ? <RuleTypeChangeCallout /> : null}
+            ruleDiff={ruleUpgradeState.diff}
+          />
+        );

Conversation

xcrzx commented Nov 14, 2024 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

elasticmachine commented Nov 14, 2024

Uh oh!

elasticmachine commented Nov 14, 2024

Uh oh!

elasticmachine commented Nov 14, 2024

Uh oh!

maximpn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xcrzx commented Nov 18, 2024

Uh oh!

maximpn left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

kibanamachine commented Nov 27, 2024

Uh oh!

kibanamachine commented Nov 27, 2024

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

xcrzx commented Nov 14, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Nov 27, 2024 •

edited

Loading