From 983037d965fdcdf089b09fa90fac31501defae9e Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Fri, 13 May 2022 13:51:34 +0200 Subject: [PATCH] Possibility of different error types for different APIs --- src/api/admin/api_server.rs | 1 + src/api/error.rs | 85 +++++++++++++++++++------------------ src/api/generic_server.rs | 21 ++++++--- src/api/k2v/api_server.rs | 1 + src/api/lib.rs | 2 +- src/api/s3/api_server.rs | 1 + src/web/error.rs | 3 +- 7 files changed, 64 insertions(+), 50 deletions(-) diff --git a/src/api/admin/api_server.rs b/src/api/admin/api_server.rs index 15aa690f..bffffd72 100644 --- a/src/api/admin/api_server.rs +++ b/src/api/admin/api_server.rs @@ -96,6 +96,7 @@ impl ApiHandler for AdminApiServer { const API_NAME_DISPLAY: &'static str = "Admin"; type Endpoint = Endpoint; + type Error = Error; fn parse_endpoint(&self, req: &Request) -> Result { Endpoint::from_request(req) diff --git a/src/api/error.rs b/src/api/error.rs index f111c801..5c33d763 100644 --- a/src/api/error.rs +++ b/src/api/error.rs @@ -2,11 +2,12 @@ use std::convert::TryInto; use err_derive::Error; use hyper::header::HeaderValue; -use hyper::{HeaderMap, StatusCode}; +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; use crate::s3::xml as s3_xml; /// Errors of this crate @@ -142,28 +143,6 @@ impl From for Error { } impl Error { - /// Get the HTTP status code that best represents the meaning of the error for the client - pub fn http_status_code(&self) -> StatusCode { - match self { - Error::NoSuchKey | Error::NoSuchBucket | Error::NoSuchUpload => StatusCode::NOT_FOUND, - Error::BucketNotEmpty | Error::BucketAlreadyExists => StatusCode::CONFLICT, - Error::PreconditionFailed => StatusCode::PRECONDITION_FAILED, - Error::Forbidden(_) => StatusCode::FORBIDDEN, - Error::NotAcceptable(_) => StatusCode::NOT_ACCEPTABLE, - Error::InternalError( - GarageError::Timeout - | GarageError::RemoteError(_) - | GarageError::Quorum(_, _, _, _), - ) => StatusCode::SERVICE_UNAVAILABLE, - Error::InternalError(_) | Error::Hyper(_) | Error::Http(_) => { - StatusCode::INTERNAL_SERVER_ERROR - } - Error::InvalidRange(_) => StatusCode::RANGE_NOT_SATISFIABLE, - Error::NotImplemented(_) => StatusCode::NOT_IMPLEMENTED, - _ => StatusCode::BAD_REQUEST, - } - } - pub fn aws_code(&self) -> &'static str { match self { Error::NoSuchKey => "NoSuchKey", @@ -187,27 +166,32 @@ impl Error { _ => "InvalidRequest", } } +} - pub fn aws_xml(&self, garage_region: &str, path: &str) -> String { - let error = s3_xml::Error { - code: s3_xml::Value(self.aws_code().to_string()), - message: s3_xml::Value(format!("{}", self)), - resource: Some(s3_xml::Value(path.to_string())), - region: Some(s3_xml::Value(garage_region.to_string())), - }; - s3_xml::to_xml_with_header(&error).unwrap_or_else(|_| { - r#" - - - InternalError - XML encoding of error failed - - "# - .into() - }) +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::NoSuchKey | Error::NoSuchBucket | Error::NoSuchUpload => StatusCode::NOT_FOUND, + Error::BucketNotEmpty | Error::BucketAlreadyExists => StatusCode::CONFLICT, + Error::PreconditionFailed => StatusCode::PRECONDITION_FAILED, + Error::Forbidden(_) => StatusCode::FORBIDDEN, + Error::NotAcceptable(_) => StatusCode::NOT_ACCEPTABLE, + Error::InternalError( + GarageError::Timeout + | GarageError::RemoteError(_) + | GarageError::Quorum(_, _, _, _), + ) => StatusCode::SERVICE_UNAVAILABLE, + Error::InternalError(_) | Error::Hyper(_) | Error::Http(_) => { + StatusCode::INTERNAL_SERVER_ERROR + } + Error::InvalidRange(_) => StatusCode::RANGE_NOT_SATISFIABLE, + Error::NotImplemented(_) => StatusCode::NOT_IMPLEMENTED, + _ => StatusCode::BAD_REQUEST, + } } - pub fn add_headers(&self, header_map: &mut HeaderMap) { + fn add_http_headers(&self, header_map: &mut HeaderMap) { use hyper::header; #[allow(clippy::single_match)] match self { @@ -222,6 +206,25 @@ impl Error { _ => (), } } + + fn http_body(&self, garage_region: &str, path: &str) -> Body { + let error = s3_xml::Error { + code: s3_xml::Value(self.aws_code().to_string()), + message: s3_xml::Value(format!("{}", self)), + resource: Some(s3_xml::Value(path.to_string())), + region: Some(s3_xml::Value(garage_region.to_string())), + }; + Body::from(s3_xml::to_xml_with_header(&error).unwrap_or_else(|_| { + r#" + + + InternalError + XML encoding of error failed + + "# + .into() + })) + } } /// Trait to map error to the Bad Request error code diff --git a/src/api/generic_server.rs b/src/api/generic_server.rs index 9281e596..77278908 100644 --- a/src/api/generic_server.rs +++ b/src/api/generic_server.rs @@ -5,9 +5,11 @@ use async_trait::async_trait; use futures::future::Future; +use hyper::header::HeaderValue; use hyper::server::conn::AddrStream; use hyper::service::{make_service_fn, service_fn}; use hyper::{Body, Request, Response, Server}; +use hyper::{HeaderMap, StatusCode}; use opentelemetry::{ global, @@ -19,26 +21,31 @@ use opentelemetry::{ use garage_util::error::Error as GarageError; use garage_util::metrics::{gen_trace_id, RecordDuration}; -use crate::error::*; - pub(crate) trait ApiEndpoint: Send + Sync + 'static { fn name(&self) -> &'static str; fn add_span_attributes(&self, span: SpanRef<'_>); } +pub trait ApiError: std::error::Error + Send + Sync + 'static { + fn http_status_code(&self) -> StatusCode; + fn add_http_headers(&self, header_map: &mut HeaderMap); + fn http_body(&self, garage_region: &str, path: &str) -> Body; +} + #[async_trait] pub(crate) trait ApiHandler: Send + Sync + 'static { const API_NAME: &'static str; const API_NAME_DISPLAY: &'static str; type Endpoint: ApiEndpoint; + type Error: ApiError; - fn parse_endpoint(&self, r: &Request) -> Result; + fn parse_endpoint(&self, r: &Request) -> Result; async fn handle( &self, req: Request, endpoint: Self::Endpoint, - ) -> Result, Error>; + ) -> Result, Self::Error>; } pub(crate) struct ApiServer { @@ -142,13 +149,13 @@ impl ApiServer { Ok(x) } Err(e) => { - let body: Body = Body::from(e.aws_xml(&self.region, uri.path())); + let body: Body = e.http_body(&self.region, uri.path()); let mut http_error_builder = Response::builder() .status(e.http_status_code()) .header("Content-Type", "application/xml"); if let Some(header_map) = http_error_builder.headers_mut() { - e.add_headers(header_map) + e.add_http_headers(header_map) } let http_error = http_error_builder.body(body)?; @@ -163,7 +170,7 @@ impl ApiServer { } } - async fn handler_stage2(&self, req: Request) -> Result, Error> { + async fn handler_stage2(&self, req: Request) -> Result, A::Error> { let endpoint = self.api_handler.parse_endpoint(&req)?; debug!("Endpoint: {}", endpoint.name()); diff --git a/src/api/k2v/api_server.rs b/src/api/k2v/api_server.rs index 5f5e9030..b14bcda6 100644 --- a/src/api/k2v/api_server.rs +++ b/src/api/k2v/api_server.rs @@ -60,6 +60,7 @@ impl ApiHandler for K2VApiServer { const API_NAME_DISPLAY: &'static str = "K2V"; type Endpoint = K2VApiEndpoint; + type Error = Error; fn parse_endpoint(&self, req: &Request) -> Result { let (endpoint, bucket_name) = Endpoint::from_request(req)?; diff --git a/src/api/lib.rs b/src/api/lib.rs index 5c522799..f8165d2a 100644 --- a/src/api/lib.rs +++ b/src/api/lib.rs @@ -6,7 +6,7 @@ pub mod error; pub use error::Error; mod encoding; -mod generic_server; +pub mod generic_server; pub mod helpers; mod router_macros; /// This mode is public only to help testing. Don't expect stability here diff --git a/src/api/s3/api_server.rs b/src/api/s3/api_server.rs index 78a69d53..2bf4a1bc 100644 --- a/src/api/s3/api_server.rs +++ b/src/api/s3/api_server.rs @@ -75,6 +75,7 @@ impl ApiHandler for S3ApiServer { const API_NAME_DISPLAY: &'static str = "S3"; type Endpoint = S3ApiEndpoint; + type Error = Error; fn parse_endpoint(&self, req: &Request) -> Result { let authority = req diff --git a/src/web/error.rs b/src/web/error.rs index 55990e9d..e9ed5314 100644 --- a/src/web/error.rs +++ b/src/web/error.rs @@ -2,6 +2,7 @@ use err_derive::Error; use hyper::header::HeaderValue; use hyper::{HeaderMap, StatusCode}; +use garage_api::generic_server::ApiError; use garage_util::error::Error as GarageError; /// Errors of this crate @@ -52,7 +53,7 @@ impl Error { pub fn add_headers(&self, header_map: &mut HeaderMap) { #[allow(clippy::single_match)] match self { - Error::ApiError(e) => e.add_headers(header_map), + Error::ApiError(e) => e.add_http_headers(header_map), _ => (), } }