[Fleet] Fix epm endpoints return errors by criamico · Pull Request #171722 · elastic/kibana

criamico · 2023-11-22T11:01:07Z

Summary

[Fleet] Improve error handling on epm endpoints.

Currently most errors occurring when doing any operation with packages will throw and result in a 500 in the correspondent endpoint.
This PR is an attempts to handle those errors in a more comprehensive way and to return meaningful responses.

Where I can I'm replacing the generic Error with FleetError; it calls Logger.error and checks if the error belongs to a specific type, if not defaults to 500.

The error described in elastic/integrations#8268 will now return a 400: https://github.com/elastic/kibana/pull/171722/files#diff-952b3c1842d5d24d9e70833cae1683e2d78df7b489dc99665dab723cc10927c1R349-R352

Checklist

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

ghost · 2023-11-22T11:01:21Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

criamico · 2023-11-23T11:37:11Z

@elasticmachine merge upstream

criamico · 2023-11-23T11:38:52Z

x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts

-    throw new Error(`Error template for ${templateIndexPattern} contains a final_pipeline`);
+    throw new PackageESError(
+      `Error template for ${templateIndexPattern} contains a final_pipeline`
+    );


I'm just not sure if these errors should be a 500. I found a few that happen when dealing with ES but I'm not totally sure what the correct response should be.

The template here refers to the template obtained from the package? If this depends only on the content of the package this should probably be a PackageInvalidArchiveError, or some other error indicating that the package is invalid, and produce a 4xx error.

Btw, since elastic/package-spec#587, we are more strict on what can be defined in the index template settings and we don't allow final_pipeline, so a package with final_pipeline in the template settings is indeed invalid.

I think you're right, that is indeed the template obtained from the package. I'll change it to PackageInvalidArchiveError

elasticmachine · 2023-11-23T11:39:10Z

Pinging @elastic/fleet (Team:Fleet)

…-fix'

sphilipse

approving as code owner for CI auto-commit

jsoriano

Great to see more specific errors for each case 👍

Do we have tests covering these cases? Should we take the opportunity to add more tests around package installation?

x-pack/plugins/fleet/server/errors/handlers.ts

jsoriano · 2023-11-23T13:30:19Z

x-pack/plugins/fleet/server/services/epm/agent/agent.ts

-      throw new Error('Invalid key');
+      throw new PackageInvalidArchiveError(
+        `Error while compiling agent template: Invalid key ${lastKeyPart}`
+      );


👍 nice to use typed errors on these cases.

x-pack/plugins/fleet/server/services/epm/elasticsearch/ilm/install.ts

jsoriano · 2023-11-23T13:33:45Z

x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts

-    throw new Error(`Error template for ${templateIndexPattern} contains a final_pipeline`);
+    throw new PackageESError(
+      `Error template for ${templateIndexPattern} contains a final_pipeline`
+    );


The template here refers to the template obtained from the package? If this depends only on the content of the package this should probably be a PackageInvalidArchiveError, or some other error indicating that the package is invalid, and produce a 4xx error.

Btw, since elastic/package-spec#587, we are more strict on what can be defined in the index template settings and we don't allow final_pipeline, so a package with final_pipeline in the template settings is indeed invalid.

x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts

jsoriano · 2023-11-23T13:47:09Z

x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts

+    logger.warn(`Cannot rollover data stream [${dataStreamName}] due to error: ${error}`);
+    throw new PackageESError(
+      `Cannot rollover data stream [${dataStreamName}] due to error: ${error}`
+    );


👍 this actually looks like a case where we should return a 500 error.

jsoriano · 2023-11-23T13:48:43Z

x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts

-      throw new Error(`could not update lifecycle settings for ${dataStreamName}: ${err.message}`);
+      logger.warn(`Could not update lifecycle settings for ${dataStreamName}: ${err.message}`);
+      throw new PackageESError(
+        `Could not update lifecycle settings for ${dataStreamName}: ${err.message}`


I wonder if this can happen because the new settings are invalid, in this case we should produce a 400 error. But I am fine with producing a 500 error by now.

I'll add a comment to it so we can reconsider it in the future.

jsoriano · 2023-11-23T13:49:50Z

x-pack/plugins/fleet/server/services/epm/elasticsearch/template/template.ts

  } catch (err) {
-    throw new Error(`could not update index template settings for ${dataStreamName}`);
+    logger.warn(`Could not update index template settings for ${dataStreamName}`);
+    throw new PackageESError(`Could not update index template settings for ${dataStreamName}`);


Same thing here. If the new index template settings are invalid this should produce a 400 error. But I am fine with leaving this as a 500 error if we don't have a way to discern the cases.

criamico · 2023-11-23T14:06:47Z

@jsoriano

Do we have tests covering these cases? Should we take the opportunity to add more tests around package installation?

Great question, we don't have coverage for all these cases. I'm adding some of them but I'm not even sure how to repro some edge cases.

nchaulet · 2023-11-24T13:12:09Z

x-pack/plugins/serverless_search/public/application/components/connectors_ingestion.tsx

        <EuiFlexItem grow={false}>
-          <EuiLink onClick={() => createConnector()}>
+          <EuiLink
+            data-test-subj="serverlessSearchConnectorIngestionPanelSetUpAConnectorLink"


nit: why that change in that PR?

This was an auto commit. Probably was already fixed by some other PR, I'll see if I get any conflicts when I merge

nchaulet

Thanks for that PR, code LGTM 🚀 (after typecheck are fixed)

criamico · 2023-11-27T09:32:59Z

@elasticmachine merge upstream

kibana-ci · 2023-11-27T10:46:43Z

💚 Build Succeeded

Buildkite Build
Commit: 1ad3582

Metrics [docs]

✅ unchanged

History

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

cc @criamico

[Fleet] Fix epm endpoints return errors

337da71

criamico self-assigned this Nov 22, 2023

criamico added the Team:Fleet Team label for Observability Data Collection Fleet team label Nov 22, 2023

criamico added v8.12.0 release_note:fix labels Nov 22, 2023

criamico added 5 commits November 22, 2023 17:05

Better handling of epm errors

37a4f5f

fix linter

1cf3702

Handle addtional errors

8fb6039

More errors

ea6c281

Add some more loggers

9fe511c

Merge branch 'main' into 2606_fix_epm_return_errors

d4e1f65

criamico commented Nov 23, 2023

View reviewed changes

criamico marked this pull request as ready for review November 23, 2023 11:39

criamico requested a review from a team as a code owner November 23, 2023 11:39

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

2af8450

…-fix'

kibanamachine requested a review from a team November 23, 2023 12:13

sphilipse approved these changes Nov 23, 2023

View reviewed changes

nchaulet self-requested a review November 23, 2023 13:45

jsoriano reviewed Nov 23, 2023

View reviewed changes

criamico added 2 commits November 23, 2023 15:39

Address code review and remove duplicate logger

595cef9

more fix

3500e85

nchaulet reviewed Nov 24, 2023

View reviewed changes

nchaulet approved these changes Nov 24, 2023

View reviewed changes

Fix type check errors

d98c300

Merge branch 'main' into 2606_fix_epm_return_errors

1ad3582

criamico merged commit 8d7816c into elastic:main Nov 27, 2023

kibanamachine added the backport:skip This PR does not require backporting label Nov 27, 2023

criamico deleted the 2606_fix_epm_return_errors branch November 27, 2023 13:26

Conversation

criamico commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

ghost commented Nov 22, 2023

🤖 GitHub comments

Uh oh!

criamico commented Nov 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Nov 23, 2023

Uh oh!

sphilipse left a comment

Choose a reason for hiding this comment

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

criamico commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nchaulet left a comment

Choose a reason for hiding this comment

Uh oh!

criamico commented Nov 27, 2023

Uh oh!

kibana-ci commented Nov 27, 2023

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

criamico commented Nov 22, 2023 •

edited

Loading

criamico commented Nov 23, 2023 •

edited

Loading