[Security Solution][Exceptions] Handle promise rejections when building artifacts #73831

madirey · 2020-07-30T16:37:34Z

Summary

We weren't properly handling rejected promises when building artifacts, causing Kibana to crash if there's a failure in retrieving the exception list entries.

We first encountered this when building lists that contain 10,000 items, because the pagination was going 1 page past the end of the exception items, resulting in a max window exceeded error from ES. This resulted in an unhandled promise exception that crashed Kibana, necessitating a complete server restart.

The pagination was fixed here: https://github.com/elastic/kibana/pull/73610/files#diff-a4ce84a07372d3782adb00b2f9f1a665R83-R106

... but if we encounter some other error in the findExceptionListItem call chain, we still risk crashing Kibana.

This PR fixes the code so that the promise rejection is handled properly. It was tested manually by forcing a promise rejection in the ExceptionListClient and verifying that 1. an unhandled promise rejection occurred before the fix, and 2. it did NOT occur after the fix.

Before

After

Checklist

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

…mise-rejection

elasticmachine · 2020-07-30T16:37:36Z

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

elasticmachine · 2020-07-30T16:37:36Z

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

dhurley14 · 2020-07-30T17:15:09Z

...curity_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.test.ts

+        mockType: ManifestManagerMockType.ListClientPromiseRejection,
+      });
+      await expect(manifestManager.buildNewManifest()).rejects.toThrow();
+    });


I think this test passes without the changes manifest_manager.ts https://github.com/elastic/kibana/pull/73831/files#diff-21a709e8a87929d93a7efdb95905d024R85-R95

We zoomed. It passes either way, but an UnhandledPromiseRejectionWarning is printed when you run it against the pre-change code, and it's absent now. Doesn't appear to be a way to force jest to behave differently...

@madirey explained to me, as posted in the comment, the test will pass without those changes, but the changes prevent the rejection warning from appearing when running the tests. The rejection does get handled further down the line when building the manifest.

madirey · 2020-07-30T17:57:16Z

@elasticmachine merge upstream

dhurley14

LGTM!

bkimmel · 2020-07-30T18:02:43Z

...curity_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.mock.ts

  packageConfigService?: jest.Mocked<PackageConfigServiceInterface>;
  savedObjectsClient?: ReturnType<typeof savedObjectsClientMock.create>;
 }): ManifestManager => {
  let cache = new LRU<string, Buffer>({ max: 10, maxAge: 1000 * 60 * 60 });
-  if (opts?.cache !== undefined) {
+  if (opts?.cache != null) {


❔ think you want !== instead of !=

@bkimmel Maybe I misunderstood, but I thought the prevailing recommendation was that != null is preferable to !== undefined because it catches both null and undefined...

It does, but coercively. == and it's != cousin hurt to read. That fancy new nullish coalescer ?? could come in handy, since it's made to handle undefined/null like if(opts?.cache ?? false) maybe,

peluja1012

Thanks for catching this, Madi!

kibanamachine · 2020-07-30T19:49:16Z

💚 Build Succeeded

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

Build metrics

✅ unchanged

History

💔 Build #65583 failed 957f344

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

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: (54 commits) [ML] Migrate to React BrowserRouter and Kibana provided History. (elastic#71941) [Discover] Improve saveSearch functional test handling (elastic#73626) [Metrics UI] Fix all threshold alert conditions disappearing due to alert prefill (elastic#73708) [Metrics UI] Fix alert previews of ungrouped alerts (elastic#73735) [SIEM] Fixes "include building block button" to operate (elastic#73900) [Metrics UI] Fix alert management to open without refresh (elastic#73739) [Security Solution][Lists] - Tests cleanup and remove unnecessary import (elastic#73865) [Ingest Management] main branch uses epr-snapshot. Others production (elastic#73555) [Canvas][tech-debt] Fix SVG not shrinking vertically properly (elastic#73867) [Maps] upgrade turf (elastic#73816) [Security Solution][Telemetry] Concurrent telemetry requests (elastic#73558) [Security Solution][Exceptions] - Update how nested entries are displayed in exceptions viewer (elastic#73745) [Security Solution][Exceptions] Adds autocomplete workaround for .text fields (elastic#73761) [Metrics UI] Fix previewing of No Data results (elastic#73753) Closes elastic#72914 by hiding anomaly detection settings links when the ml plugin is disabled. (elastic#73638) [Ingest Manager] Fix config selection in enrollment flyout from config list page (elastic#73833) [DOCS] Fixes typo in Alerting actions (elastic#73756) [APM] fixes linking errors to ML and Discover (elastic#73758) Handle promise rejections when building artifacts (elastic#73831) [Security Solution][Detections] Change from sha1 to sha256 (elastic#73741) ...

* master: (38 commits) [Discover] Context unskip date nanos functional tests (elastic#73781) [ML] Migrate to React BrowserRouter and Kibana provided History. (elastic#71941) [Discover] Improve saveSearch functional test handling (elastic#73626) [Metrics UI] Fix all threshold alert conditions disappearing due to alert prefill (elastic#73708) [Metrics UI] Fix alert previews of ungrouped alerts (elastic#73735) [SIEM] Fixes "include building block button" to operate (elastic#73900) [Metrics UI] Fix alert management to open without refresh (elastic#73739) [Security Solution][Lists] - Tests cleanup and remove unnecessary import (elastic#73865) [Ingest Management] main branch uses epr-snapshot. Others production (elastic#73555) [Canvas][tech-debt] Fix SVG not shrinking vertically properly (elastic#73867) [Maps] upgrade turf (elastic#73816) [Security Solution][Telemetry] Concurrent telemetry requests (elastic#73558) [Security Solution][Exceptions] - Update how nested entries are displayed in exceptions viewer (elastic#73745) [Security Solution][Exceptions] Adds autocomplete workaround for .text fields (elastic#73761) [Metrics UI] Fix previewing of No Data results (elastic#73753) Closes elastic#72914 by hiding anomaly detection settings links when the ml plugin is disabled. (elastic#73638) [Ingest Manager] Fix config selection in enrollment flyout from config list page (elastic#73833) [DOCS] Fixes typo in Alerting actions (elastic#73756) [APM] fixes linking errors to ML and Discover (elastic#73758) Handle promise rejections when building artifacts (elastic#73831) ...

madirey added 2 commits July 30, 2020 12:27

Handle promise rejections when building artifacts

61d243a

Merge branch 'master' of github.com:elastic/kibana into unhandled-pro…

957f344

…mise-rejection

madirey added v8.0.0 Team:Endpoint Response Endpoint Response Team Feature:Endpoint Elastic Endpoint feature v7.10.0 7.9.0 labels Jul 30, 2020

madirey requested review from a team as code owners July 30, 2020 16:37

dhurley14 reviewed Jul 30, 2020

View reviewed changes

spalger added v7.9.0 and removed 7.9.0 labels Jul 30, 2020

madirey added the release_note:skip Skip the PR/issue when compiling release notes label Jul 30, 2020

Merge branch 'master' into unhandled-promise-rejection

bdcfdc7

dhurley14 approved these changes Jul 30, 2020

View reviewed changes

bkimmel reviewed Jul 30, 2020

View reviewed changes

peluja1012 approved these changes Jul 30, 2020

View reviewed changes

madirey merged commit c670afa into elastic:master Jul 30, 2020

madirey deleted the unhandled-promise-rejection branch July 30, 2020 20:00

This was referenced Jul 30, 2020

[7.x] Handle promise rejections when building artifacts (#73831) #73868

Merged

[7.9] Handle promise rejections when building artifacts (#73831) #73869

Merged

madirey added a commit to madirey/kibana that referenced this pull request Jul 30, 2020

Handle promise rejections when building artifacts (elastic#73831)

a7856cb

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Exceptions] Handle promise rejections when building artifacts #73831

[Security Solution][Exceptions] Handle promise rejections when building artifacts #73831

madirey commented Jul 30, 2020 •

edited

Loading

elasticmachine commented Jul 30, 2020

elasticmachine commented Jul 30, 2020

dhurley14 Jul 30, 2020

madirey Jul 30, 2020

dhurley14 Jul 30, 2020

madirey commented Jul 30, 2020

dhurley14 left a comment

bkimmel Jul 30, 2020

madirey Jul 30, 2020

bkimmel Jul 31, 2020

peluja1012 left a comment

kibanamachine commented Jul 30, 2020

[Security Solution][Exceptions] Handle promise rejections when building artifacts #73831

[Security Solution][Exceptions] Handle promise rejections when building artifacts #73831

Conversation

madirey commented Jul 30, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jul 30, 2020

elasticmachine commented Jul 30, 2020

dhurley14 Jul 30, 2020

Choose a reason for hiding this comment

madirey Jul 30, 2020

Choose a reason for hiding this comment

dhurley14 Jul 30, 2020

Choose a reason for hiding this comment

madirey commented Jul 30, 2020

dhurley14 left a comment

Choose a reason for hiding this comment

bkimmel Jul 30, 2020

Choose a reason for hiding this comment

madirey Jul 30, 2020

Choose a reason for hiding this comment

bkimmel Jul 31, 2020

Choose a reason for hiding this comment

peluja1012 left a comment

Choose a reason for hiding this comment

kibanamachine commented Jul 30, 2020

💚 Build Succeeded

Build metrics

History

madirey commented Jul 30, 2020 •

edited

Loading