[Security Solution] Rule Updates in bulk with conflicts #196776

jpdjere · 2024-10-17T22:12:34Z

Resolves: #180589

Summary

Handles bulk updating of rules with conflicts in the Rule Upgrades table. See detailed requirements implemented in ticket linked above.
Changes default pick_version of both the /upgrade/_perform endpoint, and of the request payloads for that endpoint from the frontend, from TARGET to MERGED, when the isPrebuiltRulesCustomizationEnabled is true.
Also: handles issue in /upgrade/_perform endpoint with the index and data_view_id fields. See file: x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/api/perform_rule_upgrade/diffable_rule_fields_mappings.ts.

See demo video: https://www.loom.com/share/90d94d2a8f16442b9a43a425eeab6697

New copy in warning modal

Newly added tooltips:

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)

For maintainers

This was checked for breaking API changes and was labeled appropriately
This will appear in the Release Notes and follow the guidelines

jpdjere · 2024-10-18T19:24:52Z

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

-export const performUpgradeAllRules = async (): Promise<PerformRuleUpgradeResponseBody> =>
-  KibanaServices.get().http.fetch(PERFORM_RULE_UPGRADE_URL, {
-    method: 'POST',
-    version: '1',
-    body: JSON.stringify({
-      mode: 'ALL_RULES',
-      pick_version: 'TARGET',
-    }),
-  });
-


Deleting this since from now onwards the frontend client will never be just sending a request to update ALL rules, since some of them might have conflicts: we want to filter those out before making the request, which is done in this PR.

API users can still send this type request, and will get detailed errors if there are conflicts.

elasticmachine · 2024-10-18T19:49:35Z

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

elasticmachine · 2024-10-18T19:49:36Z

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

elasticmachine · 2024-10-18T19:49:37Z

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

jpdjere · 2024-10-18T19:56:58Z

@approksiu @ARWNightingale

Hi! I worked on this feature and it's basically done. You can take a look at the video as a demo.

Only question is: I wrote the texts of the modal myself. Want to take a look at it and decide if we should involve the docs team here for input?

banderror · 2024-10-22T14:40:29Z

Only question is: I wrote the texts of the modal myself. Want to take a look at it and decide if we should involve the docs team here for input?

@jpdjere By default, we involve the Docs team for reviewing any UI copies. For their convenience, it would be great if you could please add screenshots of every copy to the PR description, and tag @joepeeples near every corresponding place in the code.

xcrzx · 2024-10-22T15:39:55Z

Hi @jpdjere, what is the scope of this PR? I'm looking at the linked ticket requirements:

Bulk Upgrade Action:

When users select multiple rules for a bulk upgrade, only those without conflicts should be upgraded. The system should automatically exclude rules with conflicts from the upgrade process.
If a user selects rules that include conflicts, a modal window should appear (similar to the one used during bulk edits). This modal will inform the user that some selected rules contain conflicts and cannot be upgraded in bulk. The user will then be prompted to proceed with upgrading only the rules without conflicts.

This doesn't seem to be implemented:

Screen.Recording.2024-10-22.at.17.34.09.mov

xcrzx · 2024-10-22T15:45:33Z

The implementation also doesn't match the Upgrade All button behavior:

Upgrade All Button Behavior:

The "Upgrade All" button should now only upgrade rules that do not contain conflicts. The system should automatically exclude rules with conflicts from the upgrade process.
The UI needs to be updated to clearly communicate this to users, ensuring they understand that only conflict-free rules will be upgraded when using this option.

All rules are being upgraded, regardless of conflicts.

Screen.Recording.2024-10-22.at.17.41.32.mov

The page also crashes in the end.

jpdjere · 2024-10-22T16:40:47Z

@xcrzx Are you sure you pulled the right branch and pulled the latest changes? Everything you mentioned above is what I implemented; see the video that I posted in the PR description above.

Also, since the PR changes the default pick_version in the request payload from TARGET to MERGE (when FF is on), the update should fail anyways for all rules that have conflicts. And I don't see that happening in the videos you posted above. That's why I suspect you might be running an older commit.

xcrzx · 2024-10-22T17:05:36Z

@xcrzx Are you sure you pulled the right branch and pulled the latest changes? Everything you mentioned above is what I implemented; see the video that I posted in the PR description above.

@jpdjere You are right, apologies, I was on another branch 🤦‍♂️

xcrzx

Tested the PR locally with both rule customization enabled and disabled, using both UI and API calls. No issues found apart from minor usability concerns. Posting suggestions mostly on code structure and organisation, separation of concerns.

Also, the PR implements the rule upgrade path for rules without conflicts. What is the plan for conflict resolution?

The initial issue contains the following:

Reviewing Conflicts:

Rules that contain conflicts, including those with solvable conflicts, must be reviewed before they can be upgraded. Users need to open the upgrade preview, review the proposed changes, and then decide whether to accept or decline the changes.

Currently, it's not possible to accept or decline the changes. Is this going to be implemented separately?

Additionally, the PR doesn't contain any tests. What is the plan for covering the introduced functionality with tests?

xcrzx · 2024-10-21T10:43:12Z

...components/rules_table/upgrade_prebuilt_rules_table/upgrade_prebuilt_rules_table_buttons.tsx

@@ -67,3 +90,7 @@ export const UpgradePrebuiltRulesTableButtons = ({
    </EuiFlexGroup>
  );
 };
+
+function isAllRuleHaveConflicts(rules: Array<{ diff: { num_fields_with_conflicts: number } }>) {


A slightly more grammatically correct name for the function could be:

Suggested change

function isAllRuleHaveConflicts(rules: Array<{ diff: { num_fields_with_conflicts: number } }>) {

function doAllRulesHaveConflicts(rules: Array<{ diff: { num_fields_with_conflicts: number } }>) {

xcrzx · 2024-10-21T10:45:35Z

...components/rules_table/upgrade_prebuilt_rules_table/upgrade_prebuilt_rules_table_buttons.tsx

+  const isAllSelectedRulesHaveConflicts =
+    isPrebuiltRulesCustomizationEnabled && isAllRuleHaveConflicts(selectedRules);
+  const isAllRulesHaveConflicts =
+    isPrebuiltRulesCustomizationEnabled && isAllRuleHaveConflicts(ruleUpgradeInfos);


And here:

isAllSelectedRulesHaveConflicts → doAllSelectedRulesHaveConflicts

isAllRulesHaveConflicts → doAllRulesHaveConflicts

...curity_solution/public/detections/components/modals/upgrade_conflicts_modal/translations.tsx

...gins/security_solution/public/detections/components/modals/upgrade_conflicts_modal/index.tsx

xcrzx · 2024-10-22T15:13:50Z

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

+  /**
+   * Rule upgrade state (all rules available for upgrade)
+   */
+  ruleUpgradeInfos: RuleUpgradeInfoForReview[];
  /**
   * Rule upgrade state after applying `filterOptions`
   */


We can improve naming here for more clarity:
ruleUpgradeInfos -> allUpgradeableRules
rulesUpgradeState -> filteredUpgradeableRules

Not directly related to this PR, but the rulesUpgradeState seems to contain not only the currently filtered rules but also some additional state, such as final rule versions. Mixing these two states together feels like poor design, but perhaps I'm missing some context here. I'd be happy to discuss the state composition in more detail, let's sync up on that.

Hey @xcrzx,

do you still have concerns regarding rulesUpgradeState type and would like to discuss that?

Yes, let's sync on this. I'll send an invite

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

xcrzx · 2024-10-23T11:18:15Z

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

+  const getRulesWithConflicts = useCallback(
+    (ruleIds?: RuleSignatureId[]) => {
+      // If no rules are selected (update all rules case), then check all rules
+      const rulesSelectedForUpgrade = ruleIds ?? Object.keys(rulesUpgradeState);
+      const rulesToUpgrade = rulesSelectedForUpgrade.map((ruleId) => rulesUpgradeState[ruleId]);


We need to handle a situation where ruleId is not found in rulesUpgradeState. Let's at least add an invariant, something like:

const rule = rulesUpgradeState[ruleId]; invariant(rule, `Rule with ID ${ruleId} not found in rulesUpgradeState`);

This way, we get an easily recognizable error message instead of cannot read property xxx of undefined later on.

Added exception handling and toast in the UI:

xcrzx · 2024-10-23T11:40:57Z

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

+        // Prepare payload for upgrade with rules with no conflicts
+        const ruleIdsWithConflicts = rulesWithConflicts.map((rule) => rule.rule_id);
+        const rulesToUpgradeWithNoConflicts = isPrebuiltRulesCustomizationEnabled
+          ? rulesToUpgrade.filter((rule) => !ruleIdsWithConflicts.includes(rule.rule_id))


Set.has will be orders of magnitude faster for a larger number of rules than Array.includes, as it has O(1) time complexity compared to O(n) for arrays. I'd suggest using a Set for ruleIdsWithConflicts.

...curity_solution/public/detections/components/modals/upgrade_conflicts_modal/translations.tsx

…management_ui/components/rules_table/upgrade_prebuilt_rules_table/modals/upgrade_conflicts_modal/translations.tsx Co-authored-by: Nastasha Solomon <[email protected]>

…management_ui/components/rules_table/upgrade_prebuilt_rules_table/translations.ts Co-authored-by: Nastasha Solomon <[email protected]>

…tion_engine/rules/translations.ts Co-authored-by: Nastasha Solomon <[email protected]>

…management_ui/components/rules_table/upgrade_prebuilt_rules_table/translations.ts Co-authored-by: Nastasha Solomon <[email protected]>

xcrzx

Thanks for addressing my comments and adding happy path tests, @jpdjere!

I tested the upgrade flow locally with both FF on and off, no issues.

kibanamachine · 2024-11-11T12:29:41Z

Starting backport for target branches: 8.x

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

elasticmachine · 2024-11-11T12:30:11Z

💚 Build Succeeded

Buildkite Build
Commit: 0d5a113

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	6133	6135	+2

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	+3.1KB

History

💛 Build #249646 was flaky fe8eb54
💔 Build #249598 failed dea2e97
💚 Build #247652 succeeded 4abce24
💔 Build #247636 failed 1a1b841
💔 Build #247615 failed 1862661
💚 Build #245679 succeeded 48a6939

cc @jpdjere

kibanamachine · 2024-11-11T12:34:37Z

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts You might need to backport the following PRs to 8.x: - [Search Playground] Change route on selector changed in between search/chat modes (#197431) - [Entity Analytics] [Entity Store] Fix Asset Criticality index issue when setting up entity engines concurrently (#199486) - [APM] Attempt to fix service maps (#192859) - [OpenAPI][Fleet] Add missing operation summaries (#199548) - Update serverless link targets (#199396)

Manual backport

To create the backport manually run:

node scripts/backport --pr 196776

Questions ?

Please refer to the Backport tool documentation

xcrzx · 2024-11-11T13:53:42Z

💚 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

Resolves: elastic#180589 ## Summary - Handles bulk updating of rules with conflicts in the Rule Upgrades table. See detailed requirements implemented in ticket linked above. - Changes default `pick_version` of both the `/upgrade/_perform` endpoint, and of the request payloads for that endpoint from the frontend, from `TARGET` to `MERGED`, when the `isPrebuiltRulesCustomizationEnabled` is `true`. - **Also:** handles issue in `/upgrade/_perform` endpoint with the `index` and `data_view_id` fields. See file: `x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/api/perform_rule_upgrade/diffable_rule_fields_mappings.ts`. **See demo video:** https://www.loom.com/share/90d94d2a8f16442b9a43a425eeab6697 **New copy in warning modal** <img width="1660" alt="image" src="https://github.com/user-attachments/assets/85632192-142a-4e12-b396-1eb2320ca3f4"> **Newly added tooltips:** ![image](https://github.com/user-attachments/assets/7ada117e-57a7-4699-ad08-312c734586d9) ![image](https://github.com/user-attachments/assets/c8ed80ac-c1c3-48f1-8f8e-2433415a6772) ![image](https://github.com/user-attachments/assets/d77ed6f0-5d65-4933-9012-f6cd153a9350) ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels) - [ ] This will appear in the **Release Notes** and follow the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Nastasha Solomon <[email protected]> Co-authored-by: Dmitrii <[email protected]> (cherry picked from commit 99160f5) # Conflicts: # .github/CODEOWNERS

… (#199649) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] Rule Updates in bulk with conflicts (#196776)](#196776)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Juan Pablo Djeredjian <[email protected]>

Resolves: elastic#180589 ## Summary - Handles bulk updating of rules with conflicts in the Rule Upgrades table. See detailed requirements implemented in ticket linked above. - Changes default `pick_version` of both the `/upgrade/_perform` endpoint, and of the request payloads for that endpoint from the frontend, from `TARGET` to `MERGED`, when the `isPrebuiltRulesCustomizationEnabled` is `true`. - **Also:** handles issue in `/upgrade/_perform` endpoint with the `index` and `data_view_id` fields. See file: `x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/api/perform_rule_upgrade/diffable_rule_fields_mappings.ts`. **See demo video:** https://www.loom.com/share/90d94d2a8f16442b9a43a425eeab6697 **New copy in warning modal** <img width="1660" alt="image" src="https://github.com/user-attachments/assets/85632192-142a-4e12-b396-1eb2320ca3f4"> **Newly added tooltips:** ![image](https://github.com/user-attachments/assets/7ada117e-57a7-4699-ad08-312c734586d9) ![image](https://github.com/user-attachments/assets/c8ed80ac-c1c3-48f1-8f8e-2433415a6772) ![image](https://github.com/user-attachments/assets/d77ed6f0-5d65-4933-9012-f6cd153a9350) ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#_add_your_labels) - [ ] This will appear in the **Release Notes** and follow the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: Nastasha Solomon <[email protected]> Co-authored-by: Dmitrii <[email protected]>

jpdjere commented Oct 18, 2024

View reviewed changes

jpdjere self-assigned this Oct 18, 2024

jpdjere marked this pull request as ready for review October 18, 2024 19:49

jpdjere requested review from a team as code owners October 18, 2024 19:49

jpdjere requested a review from xcrzx October 18, 2024 19:49

jpdjere added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Oct 18, 2024

banderror removed the v8.16.0 label Oct 22, 2024

banderror added the ui-copy Review of UI copy with docs team is recommended label Oct 22, 2024

banderror requested a review from joepeeples October 22, 2024 14:41

xcrzx reviewed Oct 23, 2024

View reviewed changes

jpdjere and others added 11 commits November 8, 2024 16:23

Addressed comments

98d42cb

revert

c289c67

revert

ad0bb91

Added e2e tests

138a377

fix type

5879218

Update x-pack/plugins/security_solution/public/detection_engine/rule_…

f9ce6d3

…management_ui/components/rules_table/upgrade_prebuilt_rules_table/modals/upgrade_conflicts_modal/translations.tsx Co-authored-by: Nastasha Solomon <[email protected]>

Update x-pack/plugins/security_solution/public/detection_engine/rule_…

0762755

…management_ui/components/rules_table/upgrade_prebuilt_rules_table/modals/upgrade_conflicts_modal/translations.tsx Co-authored-by: Nastasha Solomon <[email protected]>

Update x-pack/plugins/security_solution/public/detection_engine/rule_…

88dce80

…management_ui/components/rules_table/upgrade_prebuilt_rules_table/translations.ts Co-authored-by: Nastasha Solomon <[email protected]>

Update x-pack/plugins/security_solution/public/detections/pages/detec…

231f0d3

…tion_engine/rules/translations.ts Co-authored-by: Nastasha Solomon <[email protected]>

Update x-pack/plugins/security_solution/public/detection_engine/rule_…

a826a69

…management_ui/components/rules_table/upgrade_prebuilt_rules_table/translations.ts Co-authored-by: Nastasha Solomon <[email protected]>

Update modal title

f360a7d

xcrzx force-pushed the bulk-upgrade-rules-changes branch from fc534b2 to f360a7d Compare November 8, 2024 15:40

Rebase fix

fe8eb54

xcrzx added backport:version Backport to applied version labels and removed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Nov 8, 2024

xcrzx approved these changes Nov 11, 2024

View reviewed changes

Merge branch 'main' into bulk-upgrade-rules-changes

0d5a113

xcrzx enabled auto-merge (squash) November 11, 2024 10:40

xcrzx disabled auto-merge November 11, 2024 10:40

xcrzx enabled auto-merge (squash) November 11, 2024 10:40

xcrzx merged commit 99160f5 into elastic:main Nov 11, 2024
43 checks passed

xcrzx mentioned this pull request Nov 11, 2024

[8.x] [Security Solution] Rule Updates in bulk with conflicts (#196776) #199649

Merged

kibanamachine mentioned this pull request Nov 11, 2024

[APM] Migrate /mobile API tests to deployment agnostic folder #199021

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Rule Updates in bulk with conflicts #196776

[Security Solution] Rule Updates in bulk with conflicts #196776

jpdjere commented Oct 17, 2024 •

edited by kibanamachine

Loading

jpdjere Oct 18, 2024

elasticmachine commented Oct 18, 2024

elasticmachine commented Oct 18, 2024

elasticmachine commented Oct 18, 2024

jpdjere commented Oct 18, 2024

banderror commented Oct 22, 2024

xcrzx commented Oct 22, 2024

xcrzx commented Oct 22, 2024

jpdjere commented Oct 22, 2024 •

edited

Loading

xcrzx commented Oct 22, 2024

xcrzx left a comment

xcrzx Oct 21, 2024

xcrzx Oct 21, 2024

xcrzx Oct 22, 2024

xcrzx Oct 22, 2024

maximpn Nov 12, 2024

xcrzx Nov 12, 2024

xcrzx Oct 23, 2024

jpdjere Oct 31, 2024

xcrzx Oct 23, 2024

xcrzx left a comment

kibanamachine commented Nov 11, 2024

elasticmachine commented Nov 11, 2024

kibanamachine commented Nov 11, 2024

xcrzx commented Nov 11, 2024

	function isAllRuleHaveConflicts(rules: Array<{ diff: { num_fields_with_conflicts: number } }>) {
	function doAllRulesHaveConflicts(rules: Array<{ diff: { num_fields_with_conflicts: number } }>) {

[Security Solution] Rule Updates in bulk with conflicts #196776

[Security Solution] Rule Updates in bulk with conflicts #196776

Conversation

jpdjere commented Oct 17, 2024 • edited by kibanamachine Loading

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

elasticmachine commented Oct 18, 2024

elasticmachine commented Oct 18, 2024

elasticmachine commented Oct 18, 2024

jpdjere commented Oct 18, 2024

banderror commented Oct 22, 2024

xcrzx commented Oct 22, 2024

xcrzx commented Oct 22, 2024

jpdjere commented Oct 22, 2024 • edited Loading

xcrzx commented Oct 22, 2024

xcrzx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xcrzx left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 11, 2024

elasticmachine commented Nov 11, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

kibanamachine commented Nov 11, 2024

💔 All backports failed

Manual backport

Questions ?

xcrzx commented Nov 11, 2024

💚 All backports created successfully

Questions ?

jpdjere commented Oct 17, 2024 •

edited by kibanamachine

Loading

jpdjere commented Oct 22, 2024 •

edited

Loading