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

[jfsiii]

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]

@elasticmachine merge upstream

[jfsiii]

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.

[nchaulet]

I think 400 make sense here 👍

[elasticmachine]

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

[kibanamachine]

💚 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 🚀

[ghost]

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]

@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]

@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]

@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).