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

[Fleet] use defaultFleetErrorHandler in all fleet routes #200741

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

nchaulet
Copy link
Member

@nchaulet nchaulet commented Nov 19, 2024

Summary

Resolve #197518

To handle Fleet error with have a handler defaultFleetErrorHandler that transform known error into specific response with specific statusCode.

That PR make a change to use defaultFleetErrorHandler in all fleet registered route instead of having to manually add a try/catch in every fleet handler, so we do not forget to handle some errors.

Should we backport this to 8.x ?

@nchaulet nchaulet force-pushed the feature-default-fleet-error-handler branch 3 times, most recently from 2c7400c to 96e0366 Compare November 19, 2024 15:27
@nchaulet nchaulet self-assigned this Nov 19, 2024
@nchaulet nchaulet added Team:Fleet Team label for Observability Data Collection Fleet team release_note:skip Skip the PR/issue when compiling release notes labels Nov 19, 2024
@nchaulet nchaulet force-pushed the feature-default-fleet-error-handler branch from 96e0366 to bcf1f17 Compare November 19, 2024 15:39
@nchaulet nchaulet marked this pull request as ready for review November 19, 2024 15:39
@nchaulet nchaulet requested a review from a team as a code owner November 19, 2024 15:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@nchaulet nchaulet added the backport:skip This commit does not require backporting label Nov 19, 2024
Copy link
Contributor

@juliaElastic juliaElastic left a comment

Choose a reason for hiding this comment

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

LGTM, great refactor!
Should we document the usage somewhere, at least in code comments?

@nchaulet
Copy link
Member Author

LGTM, great refactor!
Should we document the usage somewhere, at least in code comments?

Probably worth adding a section about our fleet_router in Fleet readme, I will made that change

@nchaulet
Copy link
Member Author

@juliaElastic @kpollich do you think this should be backported to 8.x?

@juliaElastic
Copy link
Contributor

It sounds like a small risk change, so I would vote to backport.

@nchaulet nchaulet added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:skip This commit does not require backporting labels Nov 19, 2024
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

cc @nchaulet

@nchaulet nchaulet enabled auto-merge (squash) November 19, 2024 18:29
@nchaulet nchaulet merged commit 523fd13 into elastic:main Nov 19, 2024
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11919725447

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 200741

Questions ?

Please refer to the Backport tool documentation

nchaulet added a commit to nchaulet/kibana that referenced this pull request Nov 19, 2024
)

(cherry picked from commit 523fd13)

# Conflicts:
#	x-pack/plugins/fleet/server/routes/agent/handlers.ts
#	x-pack/plugins/fleet/server/routes/agent_policy/handlers.ts
#	x-pack/plugins/fleet/server/routes/app/index.ts
#	x-pack/plugins/fleet/server/routes/epm/handlers.ts
#	x-pack/plugins/fleet/server/routes/health_check/handler.ts
#	x-pack/plugins/fleet/server/routes/settings/enrollment_settings_handler.ts
#	x-pack/plugins/fleet/server/routes/uninstall_token/handlers.ts
@nchaulet
Copy link
Member Author

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

nchaulet added a commit that referenced this pull request Nov 20, 2024
…) (#200803)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Fleet] use defaultFleetErrorHandler in all fleet routes
(#200741)](#200741)

<!--- Backport version: 8.9.8 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Nicolas
Chaulet","email":"nicolas.chaulet@elastic.co"},"sourceCommit":{"committedDate":"2024-11-19T18:49:44Z","message":"[Fleet]
use defaultFleetErrorHandler in all fleet routes
(#200741)","sha":"523fd13925cd0ec4f02f9ac069b09153e36fb9e0","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Fleet","v9.0.0","backport:prev-minor"],"number":200741,"url":"https://github.com/elastic/kibana/pull/200741","mergeCommit":{"message":"[Fleet]
use defaultFleetErrorHandler in all fleet routes
(#200741)","sha":"523fd13925cd0ec4f02f9ac069b09153e36fb9e0"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/200741","number":200741,"mergeCommit":{"message":"[Fleet]
use defaultFleetErrorHandler in all fleet routes
(#200741)","sha":"523fd13925cd0ec4f02f9ac069b09153e36fb9e0"}}]}]
BACKPORT-->
paulinashakirova pushed a commit to paulinashakirova/kibana that referenced this pull request Nov 26, 2024
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fleet] Missing fleet error handling in our handler
4 participants