[Security Solution][Detections] Handle conflicts on alert status update #75492

madirey · 2020-08-19T20:30:24Z

Summary

Currently if a conflict is encountered (when 2 or more users are trying to update the same alert concurrently), the entire operation will fail and the user is left wondering what happened. This PR attempts to close as many alerts as possible, while also reporting conflicts. It also improves on error messaging.

Bulk Close from Alerts Table

Bulk Close on Exception Creation

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
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
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

elasticmachine · 2020-08-19T20:30:26Z

Pinging @elastic/endpoint-response (Team:Endpoint Response)

...ins/security_solution/server/lib/detection_engine/routes/signals/open_close_signals_route.ts

yctercero · 2020-09-03T15:38:29Z

.../plugins/security_solution/public/common/components/exceptions/add_exception_modal/index.tsx

@@ -117,7 +118,7 @@ export const AddExceptionModal = memo(function AddExceptionModal({
    Array<ExceptionListItemSchema | CreateExceptionListItemSchema>
  >([]);
  const [fetchOrCreateListError, setFetchOrCreateListError] = useState<ErrorInfo | null>(null);
-  const { addError, addSuccess } = useAppToasts();
+  const { addError, addSuccess, addWarning } = useAppToasts();


Not in any way blocking this PR, but just thinking, I think this component would benefit from using useReducer. The number of useStates are growing and (just personally) working on adding error states in here recently, felt like it was getting crowded.

Makes sense to me! I don't think I will address it here, but if I get time, will take a stab at that soon.

.../plugins/security_solution/public/common/components/exceptions/add_exception_modal/index.tsx

yctercero · 2020-09-03T15:42:22Z

x-pack/plugins/security_solution/public/common/components/exceptions/use_add_exception.tsx

        }

        await addOrUpdateItems(exceptionItemsToAddOrUpdate);

        if (isSubscribed) {
          setIsLoading(false);
-          onSuccess();
+          onSuccess(updated, conflicts);


Super nit: could this also be shortened a bit to just be:

Suggested change

onSuccess(updated, conflicts);

onSuccess(response.updated ?? 0, response.version_conflicts ?? 0);

I tried this, but response was out of scope at this point. I did just uncover a related bug here (actually, the unit tests uncovered it)... let me know what you think about the updates. :)

x-pack/plugins/security_solution/public/common/translations.ts

peluja1012

LGTM. Thanks for helping identify this issue and for fixing it.

madirey · 2020-09-04T14:38:35Z

@elasticmachine merge upstream

…t-conflicts

kibanamachine · 2020-09-04T17:00:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: bc3cbb8

Build metrics

async chunks size

id	value	diff	baseline
securitySolution	9.9MB	+11.7KB	9.9MB

page load bundle size

id	value	diff	baseline
securitySolution	813.0KB	+54.0B	812.9KB

History

💔 Build #72637 failed ec685be
💔 Build #72424 failed 0f094af
💔 Build #72419 failed 3ad559d
💚 Build #72412 succeeded 0981519
💔 Build #72308 failed f0ee82b

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

* master: (47 commits) Do not require id & description when creating a logstash pipeline (elastic#76616) Remove commented src/core/tsconfig file (elastic#76792) Replaced whitelistedHosts with allowedHosts in actions ascii docs (elastic#76731) [Dashboard First] Genericize Attribute Service (elastic#76057) [ci-metrics] unify distributable file count metrics (elastic#76448) [Security Solution][Detections] Handle conflicts on alert status update (elastic#75492) [eslint] convert to @typescript-eslint/no-unused-expressions (elastic#76471) [DOCS] Add default time range filter to advanced settings (elastic#76414) [Security Solution] Refactor NetworkTopNFlow to use Search Strategy (elastic#76249) [Dashboard] Update Index Patterns when Child Index Patterns Change (elastic#76356) [ML] Add option to Advanced Settings to set default time range filter for AD jobs (elastic#76347) Add CSM app to CODEOWNERS (elastic#76793) [Security Solution][Exceptions] - Updates exception item find sort field (elastic#76685) [Security Solution][Detections][Tech Debt] - Move to using common io-ts types (elastic#75009) [Lens] Drag dimension to replace (elastic#75895) URI encode the index names we fetch in the fetchIndices lib function. (elastic#76584) [Security Solution] Resolver retrieve entity id of documents without field mapped (elastic#76562) [Ingest Manager] validate agent route using AJV instead kbn-config-schema (elastic#76546) Updated non-dev usages of node-forge (elastic#76699) [Ingest Pipelines] Processor forms for processors K-S (elastic#75638) ...

…te (#75492) (#76804) * Proceed on conflict when updating alert status * Handle conflicts * Don't let the user retry * Tweak error messages * Fix route * Update add exception modal * Reapply changes after fixing conflicts * Type errors * types * Fix remaining conflicts * Fix tests * More test fixes * Simplify onConflict evaluation * Add callback return types * Update translation paths * Add missing import Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

madirey added 8 commits August 13, 2020 01:48

Proceed on conflict when updating alert status

6cc4292

Merge branch 'master' of github.com:elastic/kibana into alert-conflicts

8c2586f

Handle conflicts

017c64a

Merge branch 'master' of github.com:elastic/kibana into alert-conflicts

8fc6a0d

Don't let the user retry

4ec2342

Tweak error messages

832d7cd

Fix route

32cae90

Update add exception modal

4a0f66d

madirey added release_note:enhancement v8.0.0 Team:Endpoint Response Endpoint Response Team v7.10.0 labels Aug 19, 2020

madirey requested review from a team as code owners August 19, 2020 20:30

spong assigned madirey Aug 21, 2020

madirey added 5 commits August 31, 2020 22:48

merge master

9e2416b

Reapply changes after fixing conflicts

7e7d9c3

Type errors

a144782

Merge master, fix conflicts

74e2a8e

types

8cd9c97

dhurley14 reviewed Sep 2, 2020

View reviewed changes

...ins/security_solution/server/lib/detection_engine/routes/signals/open_close_signals_route.ts Outdated Show resolved Hide resolved

madirey added 2 commits September 3, 2020 02:56

Fix remaining conflicts

46a18e6

Fix tests

f0ee82b

yctercero reviewed Sep 3, 2020

View reviewed changes

peluja1012 reviewed Sep 3, 2020

View reviewed changes

x-pack/plugins/security_solution/public/common/translations.ts Outdated Show resolved Hide resolved

peluja1012 reviewed Sep 3, 2020

View reviewed changes

x-pack/plugins/security_solution/public/common/translations.ts Outdated Show resolved Hide resolved

madirey added 3 commits September 3, 2020 19:46

More test fixes

0b39164

Simplify onConflict evaluation

0981519

Add callback return types

3ad559d

madirey added 2 commits September 3, 2020 20:05

Update translation paths

7297fc2

Merge branch 'master' of github.com:elastic/kibana into alert-conflicts

0f094af

peluja1012 approved these changes Sep 3, 2020

View reviewed changes

elasticmachine and others added 4 commits September 4, 2020 08:38

Merge branch 'master' into alert-conflicts

ec685be

Add missing import

270fe81

Merge branch 'master' of github.com:elastic/kibana into alert-conflicts

b4eb564

Merge branch 'alert-conflicts' of github.com:madirey/kibana into aler…

bc3cbb8

…t-conflicts

madirey merged commit a7326e6 into elastic:master Sep 4, 2020

madirey deleted the alert-conflicts branch September 4, 2020 17:11

madirey mentioned this pull request Sep 4, 2020

[7.x] [Security Solution][Detections] Handle conflicts on alert status update (#75492) #76804

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Handle conflicts on alert status update #75492

[Security Solution][Detections] Handle conflicts on alert status update #75492

madirey commented Aug 19, 2020 •

edited

Loading

elasticmachine commented Aug 19, 2020

yctercero Sep 3, 2020

madirey Sep 3, 2020

yctercero Sep 3, 2020

madirey Sep 3, 2020

peluja1012 left a comment

madirey commented Sep 4, 2020

kibanamachine commented Sep 4, 2020

	onSuccess(updated, conflicts);
	onSuccess(response.updated ?? 0, response.version_conflicts ?? 0);

[Security Solution][Detections] Handle conflicts on alert status update #75492

[Security Solution][Detections] Handle conflicts on alert status update #75492

Conversation

madirey commented Aug 19, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 19, 2020

yctercero Sep 3, 2020

Choose a reason for hiding this comment

madirey Sep 3, 2020

Choose a reason for hiding this comment

yctercero Sep 3, 2020

Choose a reason for hiding this comment

madirey Sep 3, 2020

Choose a reason for hiding this comment

peluja1012 left a comment

Choose a reason for hiding this comment

madirey commented Sep 4, 2020

kibanamachine commented Sep 4, 2020

💚 Build Succeeded

Build metrics

async chunks size

page load bundle size

History

madirey commented Aug 19, 2020 •

edited

Loading