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

[Upgrade Assistant] Remove "boom" from reindex service #58715

Merged

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Feb 27, 2020

Summary

This contribution replaces the error handling inside reindex service with custom error handling (which was prone to errors could either be Boom HTTP errors or actual Errors which led to complications in handling). Custom Errors are also platform prescribed: #57458. The mapping to HTTP specific codes is moved to just the handlers (see mapAnyErrorToKibanaHttpResponse inside reindex_indices).

How to test

The existing Jest test coverage tests that all throws are still occurring when expected.

This was integration tested locally using this setup:

  1. Paste the code snippet into cluster_checkup.ts over the handler
  2. Inside the reindex_service, remove the if statement around the throw statement on line 287. This will demonstrate an integration test.
Hack cluster_checkup.ts, ln 32
versionCheckHandlerWrapper(async (
        {
          core: {
            elasticsearch: { dataClient },
          },
        },
        request,
        response
      )
 => {
      // post migration
      const body = await getUpgradeAssistantStatus(
        dataClient,
        isCloudEnabled,
      );

      body.indices = body.indices.concat([{
        level: 'none',
        message: 'none level test',
        url: 'test/url',
        details: 'test details',
        index: 'test1',
        reindex: true,
        needsDefaultFields: false,
      }, {
        level: 'info',
        message: 'info level test',
        url: 'test/url',
        details: 'test details',
        index: 'test2',
        reindex: true,
        needsDefaultFields: false,
      }, {
        level: 'warning',
        message: 'warning level test2',
        url: 'test/url',
        details: 'test details',
        index: 'test3',
        reindex: true,
        needsDefaultFields: false,
      }, {
        level: 'critical',
        message: 'metricbeat-1 successful fix',
        url: 'test/url',
        details: 'test details',
        index: 'metricbeat-1',
        reindex: false,
        needsDefaultFields: true,
      }, {
        level: 'critical',
        message: 'metricbeat-2 failed fix',
        url: 'test/url',
        details: 'test details',
        index: 'metricbeat-2',
        reindex: false,
        needsDefaultFields: true,
      }]);


      body.cluster = body.cluster.concat([{
        level: 'none',
        message: 'none level test message',
        url: 'test/url',
        details: 'test details',
      }, {
        level: 'info',
        message: 'info level test message',
        url: 'test/url',
        details: 'test details',
      }, {
        level: 'warning',
        message: 'warning level test message',
        url: 'test/url',
        details: 'test details',
      }, {
        level: 'critical',
        message: 'critical level test message',
        url: 'test/url',
        details: 'test details',
      }]);

      try {
        return response.ok({
          body,
        });
      } catch (e) {
        if (e.status === 403) {
          return response.forbidden(e.message);
        }

        return response.internalError({ body: e });
      }
    })

To reviewers

  1. Currently, reindex service is only used in the reindex routes and inside of reindex worker. Specifically, the processNextStep function is used inside of reindex worker. These cases are all covered and tested, flag it here if you can find any instance of reindex service being used (and errors being caught) that I may have missed.
  2. I am using MyError extends Error with symbol to uniquely identify errors. Open to discussing this approach here too.

Additional

  • Currently there is only one place that a boom check is in server side UA - reindex actions. This is an isBoom check against an error that could be emitted from Saved Objects client.

For maintainers

The reindex service had logic inside it for mapping errors
to HTTP. This has been moved to the route handlers and also
removed Boom.

There is one more instance of Boom use inside of reindex-actions
but that comes from Saved Objects which should probably be removed
at a later stage.
Specify the full relative import path to the kibana's core
server folder
@jloleysens jloleysens added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes Feature:Upgrade Assistant v7.7.0 labels Feb 27, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Code changes LGTM. Tested locally and all error handling working as expected.

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@jloleysens jloleysens merged commit ac5e7aa into elastic:master Mar 2, 2020
@jloleysens jloleysens deleted the ua/fix/controller-error-handling branch March 2, 2020 10:08
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 2, 2020
* Removed Boom from reindex-service

The reindex service had logic inside it for mapping errors
to HTTP. This has been moved to the route handlers and also
removed Boom.

There is one more instance of Boom use inside of reindex-actions
but that comes from Saved Objects which should probably be removed
at a later stage.

* Fix import path

Specify the full relative import path to the kibana's core
server folder

* Remove unnecessary if statement

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

# Conflicts:
#	x-pack/plugins/upgrade_assistant/server/routes/reindex_indices.ts
jloleysens added a commit to jloleysens/kibana that referenced this pull request Mar 2, 2020
…dex-server-side

* 'master' of github.com:elastic/kibana: (34 commits)
  [Upgrade Assistant] Remove "boom" from reindex service (elastic#58715)
  [data] Clean up QueryStringInput unit tests (elastic#58704)
  [SIEM] Detection Fix typo in Adobe Hijack Persistence rule (elastic#58804)
  Restores [SIEM][CASE] Init Configure Case Page (elastic#58121) (elastic#58924)
  Skips additional failing Ingest Manager integration tests
  Skips failing Ingest Manager integration tests
  Move dev tools styles to NP (elastic#58855)
  change to have kibana --ssl cli option use more recent certs (elastic#57933)
  disable failing suite (elastic#58942)
  Don't start pollEsNodesVersion unless someone subscribes (elastic#56923)
  Do not write UUID file during optimize process (elastic#58899)
  [Endpoint] Task/add nav bar (elastic#58604)
  [Metric Alerts] Add backend support for multiple expressions per alert  (elastic#58672)
  [Metrics Alerts] Fix alerting on a rate aggregation (elastic#58789)
  disable flaky suite (elastic#55953)
  Revert "[SIEM] apollo@3 (elastic#51926)" and "[SIEM][CASE] Init Confi… (elastic#58806)
  [resubmit] Prep agg types for new platform (elastic#58893)
  [Lens] Allow number formatting within Lens (elastic#56253)
  [Autocomplete] Use settings from config rather than UI settings (elastic#58784)
  Improve action and trigger types (elastic#58657)
  ...

# Conflicts:
#	x-pack/plugins/upgrade_assistant/server/routes/reindex_indices/reindex_indices.ts
jloleysens added a commit that referenced this pull request Mar 2, 2020
…#59010)

* [Upgrade Assistant] Remove "boom" from reindex service (#58715)

* Removed Boom from reindex-service

The reindex service had logic inside it for mapping errors
to HTTP. This has been moved to the route handlers and also
removed Boom.

There is one more instance of Boom use inside of reindex-actions
but that comes from Saved Objects which should probably be removed
at a later stage.

* Fix import path

Specify the full relative import path to the kibana's core
server folder

* Remove unnecessary if statement

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

# Conflicts:
#	x-pack/plugins/upgrade_assistant/server/routes/reindex_indices.ts

* Added support for 7.x specific error
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Upgrade Assistant release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants