From c0fb9fd0fe553e5eda39dcb1a09f059bcd631b6c Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Fri, 13 May 2022 14:30:30 +0200 Subject: [PATCH] Common error type and admin error type that uses it --- src/api/admin/api_server.rs | 2 +- src/api/admin/bucket.rs | 22 ++++---- src/api/admin/cluster.rs | 4 +- src/api/admin/error.rs | 94 +++++++++++++++++++++++++++++++ src/api/admin/key.rs | 16 +++--- src/api/admin/mod.rs | 13 +++++ src/api/admin/router.rs | 2 +- src/api/common_error.rs | 108 ++++++++++++++++++++++++++++++++++++ src/api/error.rs | 4 ++ src/api/lib.rs | 1 + src/api/router_macros.rs | 12 ++-- 11 files changed, 249 insertions(+), 29 deletions(-) create mode 100644 src/api/admin/error.rs create mode 100644 src/api/common_error.rs diff --git a/src/api/admin/api_server.rs b/src/api/admin/api_server.rs index bffffd72..b344a51b 100644 --- a/src/api/admin/api_server.rs +++ b/src/api/admin/api_server.rs @@ -15,9 +15,9 @@ use prometheus::{Encoder, TextEncoder}; use garage_model::garage::Garage; use garage_util::error::Error as GarageError; -use crate::error::*; use crate::generic_server::*; +use crate::admin::error::*; use crate::admin::bucket::*; use crate::admin::cluster::*; use crate::admin::key::*; diff --git a/src/api/admin/bucket.rs b/src/api/admin/bucket.rs index 2a25bb18..1ecb66ab 100644 --- a/src/api/admin/bucket.rs +++ b/src/api/admin/bucket.rs @@ -17,8 +17,8 @@ use garage_model::permission::*; use garage_model::s3::object_table::ObjectFilter; use crate::admin::key::ApiBucketKeyPerm; -use crate::error::*; -use crate::helpers::*; +use crate::admin::error::*; +use crate::admin::parse_json_body; pub async fn handle_list_buckets(garage: &Arc) -> Result, Error> { let buckets = garage @@ -97,9 +97,9 @@ pub async fn handle_get_bucket_info( .await? .ok_or_bad_request("Bucket not found")?, _ => { - return Err(Error::BadRequest( - "Either id or globalAlias must be provided (but not both)".into(), - )) + return Err(Error::bad_request( + "Either id or globalAlias must be provided (but not both)" + )); } }; @@ -225,7 +225,7 @@ pub async fn handle_create_bucket( if let Some(ga) = &req.global_alias { if !is_valid_bucket_name(ga) { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "{}: {}", ga, INVALID_BUCKET_NAME_MESSAGE ))); @@ -240,7 +240,7 @@ pub async fn handle_create_bucket( if let Some(la) = &req.local_alias { if !is_valid_bucket_name(&la.alias) { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "{}: {}", la.alias, INVALID_BUCKET_NAME_MESSAGE ))); @@ -250,10 +250,10 @@ pub async fn handle_create_bucket( .key_table .get(&EmptyKey, &la.access_key_id) .await? - .ok_or(Error::NoSuchKey)?; - let state = key.state.as_option().ok_or(Error::NoSuchKey)?; + .ok_or(Error::NoSuchAccessKey)?; + let state = key.state.as_option().ok_or(Error::NoSuchAccessKey)?; if matches!(state.local_aliases.get(&la.alias), Some(_)) { - return Err(Error::BadRequest("Local alias already exists".into())); + return Err(Error::bad_request("Local alias already exists")); } } @@ -333,7 +333,7 @@ pub async fn handle_delete_bucket( ) .await?; if !objects.is_empty() { - return Err(Error::BadRequest("Bucket is not empty".into())); + return Err(Error::bad_request("Bucket is not empty")); } // --- done checking, now commit --- diff --git a/src/api/admin/cluster.rs b/src/api/admin/cluster.rs index b8e9d96c..db4d968d 100644 --- a/src/api/admin/cluster.rs +++ b/src/api/admin/cluster.rs @@ -13,8 +13,8 @@ use garage_rpc::layout::*; use garage_model::garage::Garage; -use crate::error::*; -use crate::helpers::*; +use crate::admin::error::*; +use crate::admin::parse_json_body; pub async fn handle_get_cluster_status(garage: &Arc) -> Result, Error> { let res = GetClusterStatusResponse { diff --git a/src/api/admin/error.rs b/src/api/admin/error.rs new file mode 100644 index 00000000..3e488d8d --- /dev/null +++ b/src/api/admin/error.rs @@ -0,0 +1,94 @@ +use err_derive::Error; +use hyper::header::HeaderValue; +use hyper::{Body, HeaderMap, StatusCode}; + +use garage_model::helper::error::Error as HelperError; +use garage_util::error::Error as GarageError; + +use crate::generic_server::ApiError; +pub use crate::common_error::*; + +/// Errors of this crate +#[derive(Debug, Error)] +pub enum Error { + #[error(display = "{}", _0)] + /// Error from common error + CommonError(CommonError), + + // Category: cannot process + /// No proper api key was used, or the signature was invalid + #[error(display = "Forbidden: {}", _0)] + Forbidden(String), + + /// The API access key does not exist + #[error(display = "Access key not found")] + NoSuchAccessKey, + + /// The bucket requested don't exists + #[error(display = "Bucket not found")] + NoSuchBucket, + + /// Tried to create a bucket that already exist + #[error(display = "Bucket already exists")] + BucketAlreadyExists, + + /// Tried to delete a non-empty bucket + #[error(display = "Tried to delete a non-empty bucket")] + BucketNotEmpty, + + // Category: bad request + /// Bucket name is not valid according to AWS S3 specs + #[error(display = "Invalid bucket name")] + InvalidBucketName, + + /// The client sent a request for an action not supported by garage + #[error(display = "Unimplemented action: {}", _0)] + NotImplemented(String), +} + +impl From for Error +where CommonError: From { + fn from(err: T) -> Self { + Error::CommonError(CommonError::from(err)) + } +} + +impl From for Error { + fn from(err: HelperError) -> Self { + match err { + HelperError::Internal(i) => Self::CommonError(CommonError::InternalError(i)), + HelperError::BadRequest(b) => Self::CommonError(CommonError::BadRequest(b)), + HelperError::InvalidBucketName(_) => Self::InvalidBucketName, + HelperError::NoSuchAccessKey(_) => Self::NoSuchAccessKey, + HelperError::NoSuchBucket(_) => Self::NoSuchBucket, + } + } +} + +impl ApiError for Error { + /// Get the HTTP status code that best represents the meaning of the error for the client + fn http_status_code(&self) -> StatusCode { + match self { + Error::CommonError(c) => c.http_status_code(), + Error::NoSuchAccessKey | Error::NoSuchBucket => StatusCode::NOT_FOUND, + Error::BucketNotEmpty | Error::BucketAlreadyExists => StatusCode::CONFLICT, + Error::Forbidden(_) => StatusCode::FORBIDDEN, + Error::NotImplemented(_) => StatusCode::NOT_IMPLEMENTED, + Error::InvalidBucketName => StatusCode::BAD_REQUEST, + } + } + + fn add_http_headers(&self, _header_map: &mut HeaderMap) { + // nothing + } + + fn http_body(&self, garage_region: &str, path: &str) -> Body { + Body::from(format!("ERROR: {}\n\ngarage region: {}\npath: {}", self, garage_region, path)) + } +} + +impl Error { + pub fn bad_request(msg: M) -> Self { + Self::CommonError(CommonError::BadRequest(msg.to_string())) + } +} diff --git a/src/api/admin/key.rs b/src/api/admin/key.rs index 19ad5160..e5f25601 100644 --- a/src/api/admin/key.rs +++ b/src/api/admin/key.rs @@ -11,8 +11,8 @@ use garage_table::*; use garage_model::garage::Garage; use garage_model::key_table::*; -use crate::error::*; -use crate::helpers::*; +use crate::admin::error::*; +use crate::admin::parse_json_body; pub async fn handle_list_keys(garage: &Arc) -> Result, Error> { let res = garage @@ -54,13 +54,13 @@ pub async fn handle_get_key_info( .key_table .get(&EmptyKey, &id) .await? - .ok_or(Error::NoSuchKey)? + .ok_or(Error::NoSuchAccessKey)? } else if let Some(search) = search { garage .key_helper() .get_existing_matching_key(&search) .await - .map_err(|_| Error::NoSuchKey)? + .map_err(|_| Error::NoSuchAccessKey)? } else { unreachable!(); }; @@ -96,9 +96,9 @@ pub async fn handle_update_key( .key_table .get(&EmptyKey, &id) .await? - .ok_or(Error::NoSuchKey)?; + .ok_or(Error::NoSuchAccessKey)?; - let key_state = key.state.as_option_mut().ok_or(Error::NoSuchKey)?; + let key_state = key.state.as_option_mut().ok_or(Error::NoSuchAccessKey)?; if let Some(new_name) = req.name { key_state.name.update(new_name); @@ -131,9 +131,9 @@ pub async fn handle_delete_key(garage: &Arc, id: String) -> Result Deserialize<'de>>(req: Request) -> Result { + let body = hyper::body::to_bytes(req.into_body()).await?; + let resp: T = serde_json::from_slice(&body).ok_or_bad_request("Invalid JSON")?; + Ok(resp) +} diff --git a/src/api/admin/router.rs b/src/api/admin/router.rs index 6f787fe9..2a5098bf 100644 --- a/src/api/admin/router.rs +++ b/src/api/admin/router.rs @@ -2,7 +2,7 @@ use std::borrow::Cow; use hyper::{Method, Request}; -use crate::error::*; +use crate::admin::error::*; use crate::router_macros::*; pub enum Authorization { diff --git a/src/api/common_error.rs b/src/api/common_error.rs new file mode 100644 index 00000000..8be85f97 --- /dev/null +++ b/src/api/common_error.rs @@ -0,0 +1,108 @@ +use err_derive::Error; +use hyper::header::HeaderValue; +use hyper::{Body, HeaderMap, StatusCode}; + +use garage_model::helper::error::Error as HelperError; +use garage_util::error::Error as GarageError; + +use crate::generic_server::ApiError; + +/// Errors of this crate +#[derive(Debug, Error)] +pub enum CommonError { + // Category: internal error + /// Error related to deeper parts of Garage + #[error(display = "Internal error: {}", _0)] + InternalError(#[error(source)] GarageError), + + /// Error related to Hyper + #[error(display = "Internal error (Hyper error): {}", _0)] + Hyper(#[error(source)] hyper::Error), + + /// Error related to HTTP + #[error(display = "Internal error (HTTP error): {}", _0)] + Http(#[error(source)] http::Error), + + /// The client sent an invalid request + #[error(display = "Bad request: {}", _0)] + BadRequest(String), +} + +impl CommonError { + pub fn http_status_code(&self) -> StatusCode { + match self { + CommonError::InternalError( + GarageError::Timeout + | GarageError::RemoteError(_) + | GarageError::Quorum(_, _, _, _), + ) => StatusCode::SERVICE_UNAVAILABLE, + CommonError::InternalError(_) | CommonError::Hyper(_) | CommonError::Http(_) => + StatusCode::INTERNAL_SERVER_ERROR, + CommonError::BadRequest(_) => StatusCode::BAD_REQUEST, + } + } +} + +/// Trait to map error to the Bad Request error code +pub trait OkOrBadRequest { + type S; + fn ok_or_bad_request>(self, reason: M) -> Result; +} + +impl OkOrBadRequest for Result +where + E: std::fmt::Display, +{ + type S = T; + fn ok_or_bad_request>(self, reason: M) -> Result { + match self { + Ok(x) => Ok(x), + Err(e) => Err(CommonError::BadRequest(format!("{}: {}", reason.as_ref(), e))), + } + } +} + +impl OkOrBadRequest for Option { + type S = T; + fn ok_or_bad_request>(self, reason: M) -> Result { + match self { + Some(x) => Ok(x), + None => Err(CommonError::BadRequest(reason.as_ref().to_string())), + } + } +} + +/// Trait to map an error to an Internal Error code +pub trait OkOrInternalError { + type S; + fn ok_or_internal_error>(self, reason: M) -> Result; +} + +impl OkOrInternalError for Result +where + E: std::fmt::Display, +{ + type S = T; + fn ok_or_internal_error>(self, reason: M) -> Result { + match self { + Ok(x) => Ok(x), + Err(e) => Err(CommonError::InternalError(GarageError::Message(format!( + "{}: {}", + reason.as_ref(), + e + )))), + } + } +} + +impl OkOrInternalError for Option { + type S = T; + fn ok_or_internal_error>(self, reason: M) -> Result { + match self { + Some(x) => Ok(x), + None => Err(CommonError::InternalError(GarageError::Message( + reason.as_ref().to_string(), + ))), + } + } +} diff --git a/src/api/error.rs b/src/api/error.rs index 5c33d763..90bfccef 100644 --- a/src/api/error.rs +++ b/src/api/error.rs @@ -166,6 +166,10 @@ impl Error { _ => "InvalidRequest", } } + + pub fn bad_request(msg: M) -> Self { + Self::BadRequest(msg.to_string()) + } } impl ApiError for Error { diff --git a/src/api/lib.rs b/src/api/lib.rs index f8165d2a..5bc2a18e 100644 --- a/src/api/lib.rs +++ b/src/api/lib.rs @@ -2,6 +2,7 @@ #[macro_use] extern crate tracing; +pub mod common_error; pub mod error; pub use error::Error; diff --git a/src/api/router_macros.rs b/src/api/router_macros.rs index a3e885e6..4c593300 100644 --- a/src/api/router_macros.rs +++ b/src/api/router_macros.rs @@ -38,7 +38,7 @@ macro_rules! router_match { }, )* (m, p) => { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "Unknown API endpoint: {} {}", m, p ))) @@ -78,7 +78,7 @@ macro_rules! router_match { )*)? }), )* - (kw, _) => Err(Error::BadRequest(format!("Invalid endpoint: {}", kw))) + (kw, _) => Err(Error::bad_request(format!("Invalid endpoint: {}", kw))) } }}; @@ -97,14 +97,14 @@ macro_rules! router_match { .take() .map(|param| param.parse()) .transpose() - .map_err(|_| Error::BadRequest("Failed to parse query parameter".to_owned()))? + .map_err(|_| Error::bad_request("Failed to parse query parameter"))? }}; (@@parse_param $query:expr, parse, $param:ident) => {{ // extract and parse mandatory query parameter // both missing and un-parseable parameters are reported as errors $query.$param.take().ok_or_bad_request("Missing argument for endpoint")? .parse() - .map_err(|_| Error::BadRequest("Failed to parse query parameter".to_owned()))? + .map_err(|_| Error::bad_request("Failed to parse query parameter"))? }}; (@func $(#[$doc:meta])* @@ -173,7 +173,7 @@ macro_rules! generateQueryParameters { false } else if v.as_ref().is_empty() { if res.keyword.replace(k).is_some() { - return Err(Error::BadRequest("Multiple keywords".to_owned())); + return Err(Error::bad_request("Multiple keywords")); } continue; } else { @@ -183,7 +183,7 @@ macro_rules! generateQueryParameters { } }; if repeated { - return Err(Error::BadRequest(format!( + return Err(Error::bad_request(format!( "Query parameter repeated: '{}'", k )));