[Security Solution] Update bulk edit api to include skipped rules in response #144461

jpdjere · 2022-11-02T17:00:33Z

Addresses: #145093
Related to: #139802

Summary

Extends Bulk Edit API to return a new skipped property for rules whose updating was skipped. See #145093 for details on when a rule is skipped.
In x-pack/plugins/alerting/server/rules_client/rules_client.ts, refactored the methods bulkEdit and bulkEditOcc to smaller methods, following an immutable approach.
- Updated all related tests and expanded coverage. (unit, integration and e2e)
Update success toast message so that the user is informed if rules were skipped.

Skipped.bulk.edit.rules.mp4

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
- [DOCS] Document the new skipped response parameter of the _bulk_action endpoint security-docs#2684
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

...ecurity_solution/server/lib/detection_engine/rule_management/api/rules/bulk_actions/route.ts

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

jpdjere · 2022-11-03T18:17:07Z

...security_solution/public/detection_engine/rule_management/logic/bulk_actions/translations.ts

-    return `${i18n.RULES_BULK_EDIT_SUCCESS_DESCRIPTION(summary.succeeded)}. ${
-      i18n.RULES_BULK_EDIT_SUCCESS_INDEX_EDIT_DESCRIPTION
+    return `${i18n.RULES_BULK_EDIT_SUCCESS_DESCRIPTION(summary.succeeded, summary.skipped)} ${
+      summary.skipped > 0 ? i18n.RULES_BULK_EDIT_SUCCESS_DATA_VIEW_RULES_SKIPPED_DETAIL : ''


Only add the explanation about rules with data views being ignored if the action is index_pattern related and the API call responded with skipped rules.

...ecurity_solution/server/lib/detection_engine/rule_management/api/rules/bulk_actions/route.ts

elasticmachine · 2022-11-03T18:28:53Z

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

elasticmachine · 2022-11-03T18:28:54Z

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

maximpn

@jpdjere The changes look good

There is a couple of comments which needs attention.

...ecurity_solution/server/lib/detection_engine/rule_management/api/rules/bulk_actions/route.ts

x-pack/plugins/security_solution/cypress/e2e/detection_rules/bulk_edit_rules.cy.ts

jpdjere · 2022-11-07T19:29:10Z

@banderror @maximpn

As per discussed today, I added the bulk_edit_skip_reason to the skipped rules return in the response, which we can later use in the UI to indicate what caused the rule edit to be skipped.

However, @banderror, because of your comments on adding a case for skipping a rule update in the case of adding a rule that already exists, I realised that the skipping logic as is in the PR can only be used for rule params and not rule attributes.

If we want to skip the editing of rule attributes, then we do need to edit the rules_client, and apply a similar logic that we do for the rules params, in which we pass in the rules_params_modifier function to the rules client as a callback and check for exceptions within that function (for example, if the existing tags array already contains the tag that the user wants to add).

I already started doing this implementation but would like to work on it on a separate PR, and get this PR merged up to its present scope.

Let me know what you think.

banderror

There are quite a bunch of comments, let me post what I have by now and continue the review.

x-pack/plugins/security_solution/cypress/e2e/detection_rules/bulk_edit_rules.cy.ts

...security_solution/public/detection_engine/rule_management/logic/bulk_actions/translations.ts

banderror · 2022-11-08T14:33:52Z

...curity_solution/public/detection_engine/rule_management_ui/components/rules_table/helpers.ts

+    skipped: 0,
    succeeded: details.exported_rules_count,
    failed: details.missing_rules_count,
    total: details.exported_rules_count + details.missing_rules_count,


Curious why we treat missing_rules_count as failed rather than skipped?

I'm not sure how this should work. Whar are missing rules when attempting to export? I guess we can consider them as skipped, but would like to understand this fully before making the change.

...ecurity_solution/server/lib/detection_engine/rule_management/api/rules/bulk_actions/route.ts

banderror · 2022-11-08T15:00:22Z

If we want to skip the editing of rule attributes, then we do need to edit the rules_client, and apply a similar logic that we do for the rules params, in which we pass in the rules_params_modifier function to the rules client as a callback and check for exceptions within that function (for example, if the existing tags array already contains the tag that the user wants to add).

I already started doing this implementation but would like to work on it on a separate PR, and get this PR merged up to its present scope.

Let me know what you think.

@jpdjere I really don't think we should implement skipping via throwing exceptions.

Simplify return skipped rules Add tests for rule params modifier Extend Cypress tests Refactored rule_params_modifier and add bulk edit response types Add skipping logic for atrributes Wire up changes in API Add tests for should_skip_attributes

Bulk edit test update Update tests for retryIfBulkEditConflicts Fix types Correct number of rules to test Extract out updateRuleAttributesInMemory Refactor bulkEditOcc Refactored types Simplified skip_reason logic Update client type

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' Update retry_if_bulk_edit_conflicts and add coverage WIP refactor bulk_edit.test.ts WIP bulk_edit.test.ts Correct beforeEach

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

…lled

kibana-ci · 2022-12-01T14:58:49Z

💔 Build Failed

Failed CI Steps

Jest Tests #15

Test Failures

[job] [logs] Jest Tests #15 / Perform bulk action route rule skipping returns 500 with skipped rules if some rules are skipped, but some errors are reported
[job] [logs] Jest Tests #15 / Perform bulk action route rule skipping returns partial failure error with skipped rules if some rule updates fail and others are skipped
[job] [logs] Jest Tests #15 / Perform bulk action route rules execution failures return error message limited to length of 1000, to prevent large response size
[job] [logs] Jest Tests #15 / Perform bulk action route rules execution failures returns error if disable rule throws error
[job] [logs] Jest Tests #15 / Perform bulk action route rules execution failures returns error if machine learning rule validation fails
[job] [logs] Jest Tests #15 / Perform bulk action route rules execution failures returns error if machine learning rule validation fails in dry run mode
[job] [logs] Jest Tests #15 / Perform bulk action route rules execution failures returns partial failure error if one if rules from ids params can`t be fetched
[job] [logs] Jest Tests #15 / Perform bulk action route rules execution failures returns partial failure error if update of few rules fail

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`alerting`	27	28	+1

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`alerting`	408	413	+5

Async chunks

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

id	before	after	diff
`securitySolution`	9.6MB	9.6MB	+1.2KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`alerting`	28	29	+1

Unknown metric groups

API count

id	before	after	diff
`alerting`	417	422	+5

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1
`securitySolution`	77	76	-1
total			-0

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	59	65	+6
`osquery`	109	115	+6
`securitySolution`	442	448	+6
total			+20

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	68	74	+6
`osquery`	110	117	+7
`securitySolution`	519	524	+5
total			+20

History

💚 Build #92059 succeeded e0861b3355c78738b356a1a7cde81085740ea164
💛 Build #90988 was flaky c5d1c86905c33df5c7fd2a6ad8a4793681e81ae0
💛 Build #90600 was flaky 99e41f7aee0fad3e9d9fd6508de76e40641977c3
💛 Build #90336 was flaky 52f7758e3932a300bfd89b06d3253fcee81bf25d

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

cc @jpdjere

jpdjere · 2022-12-12T23:28:41Z

Closing in favour of #147345 after ResponseOps refactored rules_client

…int (#2731) Relates to: #2684 Feature tickets: elastic/kibana#145093 elastic/kibana#139802 Implemented in PR: elastic/kibana#144461

jpdjere mentioned this pull request Nov 3, 2022

[Security Solution] Fix success bulk edit toast message #144497

Merged

2 tasks

jpdjere commented Nov 3, 2022

View reviewed changes

...ecurity_solution/server/lib/detection_engine/rule_management/api/rules/bulk_actions/route.ts Outdated Show resolved Hide resolved

maximpn reviewed Nov 3, 2022

View reviewed changes

x-pack/plugins/security_solution/public/detection_engine/rule_management/api/api.ts Outdated Show resolved Hide resolved

jpdjere force-pushed the update-bulk-edit-api-skipped-results branch from 6b3369b to 67982c7 Compare November 3, 2022 18:16

jpdjere commented Nov 3, 2022

View reviewed changes

...ecurity_solution/server/lib/detection_engine/rule_management/api/rules/bulk_actions/route.ts Outdated Show resolved Hide resolved

jpdjere marked this pull request as ready for review November 3, 2022 18:27

jpdjere requested a review from a team as a code owner November 3, 2022 18:27

jpdjere requested review from maximpn and banderror November 3, 2022 18:27

jpdjere self-assigned this Nov 3, 2022

jpdjere added Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team labels Nov 3, 2022

jpdjere added enhancement New value added to drive a business result v8.6.0 release_note:enhancement labels Nov 3, 2022

jpdjere requested a review from a team as a code owner November 3, 2022 19:15

jpdjere requested a review from a team as a code owner November 4, 2022 12:03

jpdjere force-pushed the update-bulk-edit-api-skipped-results branch from 73a39f0 to fe2dd59 Compare November 4, 2022 13:25

maximpn reviewed Nov 4, 2022

View reviewed changes

jpdjere force-pushed the update-bulk-edit-api-skipped-results branch 2 times, most recently from c493263 to 3f59997 Compare November 7, 2022 12:50

jpdjere requested a review from maximpn November 7, 2022 13:35

jpdjere force-pushed the update-bulk-edit-api-skipped-results branch from 6c452e1 to e66aeed Compare November 8, 2022 12:05

banderror reviewed Nov 8, 2022

View reviewed changes

jpdjere added 23 commits December 1, 2022 15:33

Add skipped rules to bulk edit api

89d85b8

Updated toast message

8142b91

Simplify return skipped rules Add tests for rule params modifier Extend Cypress tests Refactored rule_params_modifier and add bulk edit response types Add skipping logic for atrributes Wire up changes in API Add tests for should_skip_attributes

Add multiple_actions integration tests

16de5af

Refactored Cypress tests

8025c34

Fix dataView e2e test

e769156

Bulk edit test update Update tests for retryIfBulkEditConflicts Fix types Correct number of rules to test Extract out updateRuleAttributesInMemory Refactor bulkEditOcc Refactored types Simplified skip_reason logic Update client type

Update ule_params_modifier.test.ts

36b36b9

Update bulk_actions/route.test.ts

dbea04a

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix' Update retry_if_bulk_edit_conflicts and add coverage WIP refactor bulk_edit.test.ts WIP bulk_edit.test.ts Correct beforeEach

Extend bulk_edit.test.ts test coverage

c0ef8ff

Expand integration test coverage

69ad42e

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

Updated tests

a506512

Fix rules params modifier test

61a1aad

Refactored explainBulkEditSuccess

4dcd290

Addressed comments

8149300

Expanded typing

9940d2b

Reorganized types

47d1ff1

Refactored pMap callback function

13320ce

Updated tests to assert that bulkMarkApiKeysForInvalidation is not ca…

77d0e93

…lled

Adressed comments

1857a08

Fix types

f779d88

Expanded applyBulkEditOperation tests

c78a7a8

Clarified variables names in test config

303120f

Remove unnecessary type

2eb1200

refactored bulk edit response types

181d69c

jpdjere force-pushed the update-bulk-edit-api-skipped-results branch from d8325be to 181d69c Compare December 1, 2022 14:33

banderror linked an issue Dec 2, 2022 that may be closed by this pull request

[Security Solution] Extend bulk edit rules API to return skipped rules in response #145093

Closed

spong mentioned this pull request Dec 12, 2022

[RAM] Adds auto-incrementing revision field to rules #147398

Merged

7 tasks

jpdjere closed this Dec 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Update bulk edit api to include skipped rules in response #144461

[Security Solution] Update bulk edit api to include skipped rules in response #144461

jpdjere commented Nov 2, 2022 •

edited by banderror

Loading

jpdjere Nov 3, 2022 •

edited

Loading

elasticmachine commented Nov 3, 2022

elasticmachine commented Nov 3, 2022

maximpn left a comment

jpdjere commented Nov 7, 2022

banderror left a comment

banderror Nov 8, 2022

jpdjere Nov 22, 2022

banderror commented Nov 8, 2022

kibana-ci commented Dec 1, 2022 •

edited

Loading

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

jpdjere commented Dec 12, 2022

[Security Solution] Update bulk edit api to include skipped rules in response #144461

[Security Solution] Update bulk edit api to include skipped rules in response #144461

Conversation

jpdjere commented Nov 2, 2022 • edited by banderror Loading

Summary

Checklist

For maintainers

jpdjere Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Nov 3, 2022

elasticmachine commented Nov 3, 2022

maximpn left a comment

Choose a reason for hiding this comment

jpdjere commented Nov 7, 2022

banderror left a comment

Choose a reason for hiding this comment

banderror Nov 8, 2022

Choose a reason for hiding this comment

jpdjere Nov 22, 2022

Choose a reason for hiding this comment

banderror commented Nov 8, 2022

kibana-ci commented Dec 1, 2022 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Public APIs missing exports

API count

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

jpdjere commented Dec 12, 2022

jpdjere commented Nov 2, 2022 •

edited by banderror

Loading

jpdjere Nov 3, 2022 •

edited

Loading

kibana-ci commented Dec 1, 2022 •

edited

Loading