Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Ingest Manager] prevent crash on unhandled rejection from setupIngestManager #74300

Merged
merged 2 commits into from
Aug 5, 2020

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Aug 4, 2020

Summary

fixes #73997 (POST /setup with unreachable Registry can crash Kibana)

From comments at the change

  // be sure to return the promise because it has the resolved/rejected status attached to it
  // otherwise, we effectively return success every time even if there are errors
  // because `return undefined` -> `Promise.resolve(undefined)` in an `async` function
  return setupIngestStatus;

master: By returning no value, await setupIngestManager resolves as a success, but hits issues later because of the silent or unhandled failures.

PR: By returning the promise await and KP do what we want

Checklist

Delete any items that are not applicable to this PR.

Testing

Jest test
afd064a to ensure failures in setupIngestManager are treated as errors by await & try/catch

In the terminal
master #73997 shows that POST /setup crashes with a bad registryUrl

this PR, they return a 502 as expected

  if (error instanceof RegistryError) {
    return 502; // Bad Gateway
  }
> curl -X POST 'http://localhost:5601/api/ingest_manager/setup' -H 'kbn-xsrf: <string>' --user elastic:changeme
{"statusCode":502,"error":"Bad Gateway","message":"Error connecting to package registry at http://localhost:9999/search?package=system&internal=true&experimental=true: request to http://localhost:9999/search?package=system&internal=true&experimental=true failed, reason: connect ECONNREFUSED 127.0.0.1:9999"}

> curl -X POST 'http://localhost:5601/api/ingest_manager/setup' -H 'kbn-xsrf: <string>' --user elastic:changeme
{"statusCode":502,"error":"Bad Gateway","message":"Error connecting to package registry at http://localhost:9999/search?package=system&internal=true&experimental=true: request to http://localhost:9999/search?package=system&internal=true&experimental=true failed, reason: connect ECONNREFUSED 127.0.0.1:9999"}

> curl -X POST 'http://localhost:5601/api/ingest_manager/setup' -H 'kbn-xsrf: <string>' --user elastic:changeme
{"statusCode":502,"error":"Bad Gateway","message":"Error connecting to package registry at http://localhost:9999/search?package=system&internal=true&experimental=true: request to http://localhost:9999/search?package=system&internal=true&experimental=true failed, reason: connect ECONNREFUSED 127.0.0.1:9999"}

> curl -X POST 'http://localhost:5601/api/ingest_manager/setup' -H 'kbn-xsrf: <string>' --user elastic:changeme
{"statusCode":502,"error":"Bad Gateway","message":"Error connecting to package registry at http://localhost:9999/search?package=system&internal=true&experimental=true: request to http://localhost:9999/search?package=system&internal=true&experimental=true failed, reason: connect ECONNREFUSED 127.0.0.1:9999"}

> curl -X POST 'http://localhost:5601/api/ingest_manager/setup' -H 'kbn-xsrf: <string>' --user elastic:changeme
{"statusCode":502,"error":"Bad Gateway","message":"Error connecting to package registry at http://localhost:9999/search?package=system&internal=true&experimental=true: request to http://localhost:9999/search?package=system&internal=true&experimental=true failed, reason: connect ECONNREFUSED 127.0.0.1:9999"}

@jfsiii jfsiii added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.9.0 Team:Fleet Team label for Observability Data Collection Fleet team labels Aug 4, 2020
@jfsiii jfsiii requested a review from a team August 4, 2020 20:30
@jfsiii jfsiii self-assigned this Aug 4, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@jfsiii jfsiii requested a review from ruflin August 4, 2020 20:41
@jfsiii jfsiii changed the title [Ingest Manager] setupIngestManager should reject if errors occur [Ingest Manager] prevent crash on unhandled rejection from setupIngestManager Aug 4, 2020
Copy link
Contributor

@neptunian neptunian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested locally and works

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

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

import { setupIngestManager } from './setup';
import { savedObjectsClientMock } from 'src/core/server/mocks';

describe('setupIngestManager', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see a test for this!

@jfsiii jfsiii merged commit beb7b82 into elastic:master Aug 5, 2020
jfsiii pushed a commit that referenced this pull request Aug 5, 2020
…tManager (#74300) (#74349)

* Add test to ensure setup rejects if errors thrown.

* Return the promise from setup so test passes
jfsiii pushed a commit that referenced this pull request Aug 5, 2020
…tManager (#74300) (#74350)

* Add test to ensure setup rejects if errors thrown.

* Return the promise from setup so test passes
gmmorris added a commit to gmmorris/kibana that referenced this pull request Aug 5, 2020
* master: (154 commits)
  [ML] Fix initial plugin's bundle size (elastic#74047)
  [Ingest Manager] prevent crash on unhandled rejection from setupIngestManager (elastic#74300)
  [Logs UI] Correct trial period duration in anomaly splash screen (elastic#74249)
  [Discover] Inline noWhiteSpace function (elastic#74331)
  [DOCS] Add Observability topic (elastic#73041)
  skip flaky suite (elastic#74327)
  [Security Solution][Detections] Fixes Severity Override not matching for Elastic Endpoint Security rule (elastic#74317)
  [Security Solution][Exceptions] - Fixes exceptions builder nested deletion issue and adds unit tests (elastic#74250)
  Fixed Alert details does not update page title and breadcrumb (elastic#74214)
  [src/dev/build] build Kibana Platform bundles from source (elastic#73591)
  [Reporting] Shorten asset path to help CLI FS Watcher (elastic#74185)
  Fix TMS not loaded in legacy maps (elastic#73570)
  [Security Solution] styling for notes' panel (elastic#74274)
  [Security Solution][Tech Debt] cleans up ts-ignore issues and some smaller linter issues  (elastic#74268)
  Make the actions plugin support generics (elastic#71439)
  [Security Solution] Keep original note creator (elastic#74203)
  [CI] Fix xpack kibana build dir in xpack visual regression script
  [CI] Fix baseline_capture job by adding parallel process number back
  [Monitoring] Ensure setup mode works on cloud but only for alerts (elastic#73127)
  [Maps] Custom color ramps should show correctly on the map for mvt layers (elastic#74169)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.9.0 v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Ingest Manager] POST /setup crashes Kibana with Unhandled Promise rejection
6 participants