[Ingest Manager] Ensure at least default Error handling on all routes by jfsiii · Pull Request #77975 · elastic/kibana

jfsiii · 2020-09-20T12:16:42Z

Summary

Fixes the error logging & HTTP status code portions of #66688 for all routes by using the defaultIngestErrorHandler introduced in #74409

In the request handler, if the caught error is instanceof IngestManagerError, use res.customError() to return the error to the caller. Pass the message, and get a suitable HTTP response code from the getHTTPResponseCode() helper.

In the request handler, also use the Kibana Logger to log the error message to the Kibana log.

In the request handler, if the error is not IngestManagerError, use status code 500. In that case, log an error to the console with the full error message, and also log the stack trace of the error.

/setup & /epm/* were implemented in #74409

Now the handlers look roughly like:

async function handler(context, request, response): KibanaResponse {
  try {
    const result = await something();
    return response.ok({ body: result });
  } catch (error) {
    return defaultIngestErrorHandler({ error, response });
  }
}

Checklist

Delete any items that are not applicable to this PR.

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios

Update one test a1939d3 to reflect the change in result from 500 to 404.

I'm not sure if any more are needed/desired. There are tests for defaultIngestErrorHandler which sure the advertised behavior.

    IngestManagerError
      ✓ 502: RegistryError (7ms)
      ✓ 404: PackageNotFoundError (2ms)
      ✓ 400: IngestManagerError (2ms)
    Boom
      ✓ 500: constructor - one arg (2ms)
      ✓ custom: constructor - 2 args (1ms)
      ✓ 400: Boom.badRequest (2ms)
      ✓ 404: Boom.notFound (1ms)
    all other errors
      ✓ 500 (2ms)

Any non-default logic is still run before defaultIngestErrorHandler is called.

For maintainers

This was checked for breaking API changes and was labeled appropriately

jfsiii · 2020-09-21T09:39:53Z

@elasticmachine merge upstream

jfsiii · 2020-09-21T13:39:28Z

x-pack/test/ingest_manager_api_integration/apis/agent_policy/agent_policy.ts

            description: '',
          })
-          .expect(500);
+          .expect(404);


This is consistent with what we do elsewhere, and I prefer 4xx over 500 here, but we can easily keep the 500 behavior if that's desired.

I think 400 make sense here 👍

elasticmachine · 2020-09-21T13:42:01Z

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

kibanamachine · 2020-09-21T15:05:40Z

💚 Build Succeeded

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

Build metrics

✅ unchanged

History

💔 Build #75748 failed c8004ac

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

nchaulet

Looks good to me 🚀

…routes (#77975) (#78086) * [Ingest Manager] Ensure at least default Error handling on all routes (#77975) * res.customError -> defaultIngestErrorHandler * Missed a variable rename in prior commit * copying an invalid policy will 404; not 500 Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> * Add import missed by backport script Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…routes (#77975) (#78088) * [Ingest Manager] Ensure at least default Error handling on all routes (#77975) * res.customError -> defaultIngestErrorHandler * Missed a variable rename in prior commit * copying an invalid policy will 404; not 500 Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/ingest_manager/server/routes/agent/acks_handlers.ts # x-pack/plugins/ingest_manager/server/routes/agent/handlers.ts # x-pack/plugins/ingest_manager/server/routes/agent_policy/handlers.ts # x-pack/plugins/ingest_manager/server/routes/package_policy/handlers.ts # x-pack/plugins/ingest_manager/server/routes/settings/index.ts # x-pack/test/ingest_manager_api_integration/apis/agent_policy/agent_policy.ts * Remove any 7.x references to *_policy vs *_config * 404, not 500, on invalid id

ghost · 2020-10-20T11:17:17Z

Hi @jfsiii

We have gone through the ticket and observed it required DEV_Validation to test the ticket .

Please let us know if anything is missing from our end.

jfsiii · 2020-10-20T11:23:55Z

@rahulgupta-qasource I don't know what DEV_Validation is. Are you unable to make GET and POST requests to the /api/fleet/* endpoints?

EricDavisX · 2020-10-20T15:16:47Z

@jfsiii hi. I can chime in, we're implementing process as we go - thanks for the support. :) This DEV_Validate label is a carry over from process the QA team had with other teams. It's use was intended to specify the test team believed it was beyond regular test duty (usually beyond the ability of the test team to handle it without more input and possibly without more expertise developed). It was used to indicate that and to close the loop back that the dev team would handle it or elaborate further on what we shall do.

Allow me to follow through now:
We are in a place where we are developing more automated tests routinely, and this change is best handled with our Kibana automation FTR framework. I know we have some tests in place, but with this modification I see only 1 test adjusted so I presume you've or another engineer has done representative testing enough (perhaps one manual test from each distinct api group enhanced) to feel confident in the merge and we can open a new test ticket to cover the test enhancements, if we think it warrants the test. We'd clearly be more confident if we had those, but I can't judge the value proposition versus other automation we need to develop. I'll leave that to you and you can log a ticket as you see best.

The QA validation we've done over the 7.9.3 build shows that the merge was sane and we can call the testing side done as I see it.

Very happy to discourse any topic or specific item about this pr or test need and to figure out how to assign any further test work. I know we have an OAS api file in Kibana, we can use that to do manual review if we think it is warranted. The test team understands Postman usage to make API calls, but Rahul has not specifically done much API tests until now so it will include some ramp up time and discourse about which APIs need coverage.

jfsiii · 2020-10-20T15:45:18Z

@EricDavisX Thanks for the explanation. I'm lacking context for why I was pinged about this, but I'm not asking for any different testing here. Let me know if you need anything from me, but I'm fine leaving this closed and covered by our existing approach(es).

John Schulz added 2 commits September 20, 2020 07:10

res.customError -> defaultIngestErrorHandler

c8004ac

Missed a variable rename in prior commit

89d7e5e

elasticmachine and others added 2 commits September 21, 2020 05:39

Merge branch 'master' into update-happy-path-error-handlers

d8c6ece

copying an invalid policy will 404; not 500

a1939d3

jfsiii commented Sep 21, 2020

View reviewed changes

jfsiii marked this pull request as ready for review September 21, 2020 13:40

jfsiii requested a review from a team September 21, 2020 13:40

jfsiii changed the title ~~[Ingest Manager] Add remaining default Error handlers~~ [Ingest Manager] Ensure at least default Error handling on all routes Sep 21, 2020

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Sep 21, 2020

jfsiii added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v7.9.3 v8.0.0 labels Sep 21, 2020

jfsiii self-assigned this Sep 21, 2020

jfsiii requested a review from nchaulet September 21, 2020 15:06

jfsiii mentioned this pull request Sep 21, 2020

[Ingest Manager] Agent bulk actions UI #77690

Merged

nchaulet approved these changes Sep 21, 2020

View reviewed changes

jfsiii merged commit 06d1436 into elastic:master Sep 21, 2020

This was referenced Sep 21, 2020

[7.x] [Ingest Manager] Ensure at least default Error handling on all routes (#77975) #78086

Merged

[7.9] [Ingest Manager] Ensure at least default Error handling on all routes (#77975) #78088

Merged

This was referenced Oct 1, 2020

[Ingest Manager] Error handling in server APIs #66688

Closed

Ingest Manager - unable to check permissions message seen on the screen -while logged in as elastic super user. #74457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Ingest Manager] Ensure at least default Error handling on all routes#77975

[Ingest Manager] Ensure at least default Error handling on all routes#77975
jfsiii merged 4 commits intoelastic:masterfrom
jfsiii:update-happy-path-error-handlers

jfsiii commented Sep 20, 2020 •

edited

Loading

Uh oh!

jfsiii commented Sep 21, 2020

Uh oh!

jfsiii Sep 21, 2020

Uh oh!

nchaulet Sep 21, 2020

Uh oh!

elasticmachine commented Sep 21, 2020

Uh oh!

kibanamachine commented Sep 21, 2020

Uh oh!

nchaulet left a comment

Uh oh!

ghost commented Oct 20, 2020

Uh oh!

jfsiii commented Oct 20, 2020

Uh oh!

EricDavisX commented Oct 20, 2020

Uh oh!

jfsiii commented Oct 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

jfsiii commented Sep 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

jfsiii commented Sep 21, 2020

Uh oh!

jfsiii Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

nchaulet Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 21, 2020

Uh oh!

kibanamachine commented Sep 21, 2020

💚 Build Succeeded

Build metrics

History

Uh oh!

nchaulet left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 20, 2020

Uh oh!

jfsiii commented Oct 20, 2020

Uh oh!

EricDavisX commented Oct 20, 2020

Uh oh!

jfsiii commented Oct 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jfsiii commented Sep 20, 2020 •

edited

Loading