Merge pull request 'api: better handling of helper errors to distinguish error codes' (#942) from fix-getkeyinfo-404 into mainHEADmain

Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/942

11 files changed, 97 insertions, 23 deletions

diff --git a/src/api/admin/error.rs b/src/api/admin/error.rs
index 2668b42d..40d686e3 100644
--- a/src/api/admin/error.rs
+++ b/src/api/admin/error.rs
@@ -1,3 +1,5 @@
+use std::convert::TryFrom;
+
 use err_derive::Error;
 use hyper::header::HeaderValue;
 use hyper::{HeaderMap, StatusCode};
@@ -38,6 +40,19 @@ where
 	}
 }
 
+/// FIXME: helper errors are transformed into their corresponding variants
+/// in the Error struct, but in many case a helper error should be considered
+/// an internal error.
+impl From<HelperError> for Error {
+	fn from(err: HelperError) -> Error {
+		match CommonError::try_from(err) {
+			Ok(ce) => Self::Common(ce),
+			Err(HelperError::NoSuchAccessKey(k)) => Self::NoSuchAccessKey(k),
+			Err(_) => unreachable!(),
+		}
+	}
+}
+
 impl CommonErrorDerivative for Error {}
 
 impl Error {
diff --git a/src/api/common_error.rs b/src/api/common_error.rs
index c47555d4..0c8006dc 100644
--- a/src/api/common_error.rs
+++ b/src/api/common_error.rs
@@ -1,3 +1,5 @@
+use std::convert::TryFrom;
+
 use err_derive::Error;
 use hyper::StatusCode;
 
@@ -97,18 +99,39 @@ impl CommonError {
 	}
 }
 
-impl From<HelperError> for CommonError {
-	fn from(err: HelperError) -> Self {
+impl TryFrom<HelperError> for CommonError {
+	type Error = HelperError;
+
+	fn try_from(err: HelperError) -> Result<Self, HelperError> {
 		match err {
-			HelperError::Internal(i) => Self::InternalError(i),
-			HelperError::BadRequest(b) => Self::BadRequest(b),
-			HelperError::InvalidBucketName(n) => Self::InvalidBucketName(n),
-			HelperError::NoSuchBucket(n) => Self::NoSuchBucket(n),
-			e => Self::bad_request(format!("{}", e)),
+			HelperError::Internal(i) => Ok(Self::InternalError(i)),
+			HelperError::BadRequest(b) => Ok(Self::BadRequest(b)),
+			HelperError::InvalidBucketName(n) => Ok(Self::InvalidBucketName(n)),
+			HelperError::NoSuchBucket(n) => Ok(Self::NoSuchBucket(n)),
+			e => Err(e),
 		}
 	}
 }
 
+/// This function converts HelperErrors into CommonErrors,
+/// for variants that exist in CommonError.
+/// This is used for helper functions that might return InvalidBucketName
+/// or NoSuchBucket for instance, and we want to pass that error
+/// up to our caller.
+pub(crate) fn pass_helper_error(err: HelperError) -> CommonError {
+	match CommonError::try_from(err) {
+		Ok(e) => e,
+		Err(e) => panic!("Helper error `{}` should hot have happenned here", e),
+	}
+}
+
+pub(crate) fn helper_error_as_internal(err: HelperError) -> CommonError {
+	match err {
+		HelperError::Internal(e) => CommonError::InternalError(e),
+		e => CommonError::InternalError(GarageError::Message(e.to_string())),
+	}
+}
+
 pub trait CommonErrorDerivative: From<CommonError> {
 	fn internal_error<M: ToString>(msg: M) -> Self {
 		Self::from(CommonError::InternalError(GarageError::Message(
diff --git a/src/api/k2v/api_server.rs b/src/api/k2v/api_server.rs
index f2a3942e..de6e5f06 100644
--- a/src/api/k2v/api_server.rs
+++ b/src/api/k2v/api_server.rs
@@ -90,11 +90,13 @@ impl ApiHandler for K2VApiServer {
 		let bucket_id = garage
 			.bucket_helper()
 			.resolve_bucket(&bucket_name, &api_key)
-			.await?;
+			.await
+			.map_err(pass_helper_error)?;
 		let bucket = garage
 			.bucket_helper()
 			.get_existing_bucket(bucket_id)
-			.await?;
+			.await
+			.map_err(helper_error_as_internal)?;
 		let bucket_params = bucket.state.into_option().unwrap();
 
 		let allowed = match endpoint.authorization_type() {
diff --git a/src/api/k2v/batch.rs b/src/api/k2v/batch.rs
index 02b7ae8b..e4d0b0e5 100644
--- a/src/api/k2v/batch.rs
+++ b/src/api/k2v/batch.rs
@@ -4,12 +4,12 @@ use serde::{Deserialize, Serialize};
 
 use garage_table::{EnumerationOrder, TableSchema};
 
-use garage_model::k2v::causality::*;
 use garage_model::k2v::item_table::*;
 
 use crate::helpers::*;
 use crate::k2v::api_server::{ReqBody, ResBody};
 use crate::k2v::error::*;
+use crate::k2v::item::parse_causality_token;
 use crate::k2v::range::read_range;
 
 pub async fn handle_insert_batch(
@@ -23,7 +23,7 @@ pub async fn handle_insert_batch(
 
 	let mut items2 = vec![];
 	for it in items {
-		let ct = it.ct.map(|s| CausalContext::parse_helper(&s)).transpose()?;
+		let ct = it.ct.map(|s| parse_causality_token(&s)).transpose()?;
 		let v = match it.v {
 			Some(vs) => DvvsValue::Value(
 				BASE64_STANDARD
@@ -281,7 +281,8 @@ pub(crate) async fn handle_poll_range(
 			query.seen_marker,
 			timeout_msec,
 		)
-		.await?;
+		.await
+		.map_err(pass_helper_error)?;
 
 	if let Some((items, seen_marker)) = resp {
 		let resp = PollRangeResponse {
diff --git a/src/api/k2v/error.rs b/src/api/k2v/error.rs
index 16479227..dbe4be2c 100644
--- a/src/api/k2v/error.rs
+++ b/src/api/k2v/error.rs
@@ -3,6 +3,7 @@ use hyper::header::HeaderValue;
 use hyper::{HeaderMap, StatusCode};
 
 use crate::common_error::CommonError;
+pub(crate) use crate::common_error::{helper_error_as_internal, pass_helper_error};
 pub use crate::common_error::{CommonErrorDerivative, OkOrBadRequest, OkOrInternalError};
 use crate::generic_server::ApiError;
 use crate::helpers::*;
@@ -28,6 +29,10 @@ pub enum Error {
 	#[error(display = "Invalid base64: {}", _0)]
 	InvalidBase64(#[error(source)] base64::DecodeError),
 
+	/// Invalid causality token
+	#[error(display = "Invalid causality token")]
+	InvalidCausalityToken,
+
 	/// The client asked for an invalid return format (invalid Accept header)
 	#[error(display = "Not acceptable: {}", _0)]
 	NotAcceptable(String),
@@ -72,6 +77,7 @@ impl Error {
 			Error::AuthorizationHeaderMalformed(_) => "AuthorizationHeaderMalformed",
 			Error::InvalidBase64(_) => "InvalidBase64",
 			Error::InvalidUtf8Str(_) => "InvalidUtf8String",
+			Error::InvalidCausalityToken => "CausalityToken",
 		}
 	}
 }
@@ -85,7 +91,8 @@ impl ApiError for Error {
 			Error::NotAcceptable(_) => StatusCode::NOT_ACCEPTABLE,
 			Error::AuthorizationHeaderMalformed(_)
 			| Error::InvalidBase64(_)
-			| Error::InvalidUtf8Str(_) => StatusCode::BAD_REQUEST,
+			| Error::InvalidUtf8Str(_)
+			| Error::InvalidCausalityToken => StatusCode::BAD_REQUEST,
 		}
 	}
 
diff --git a/src/api/k2v/item.rs b/src/api/k2v/item.rs
index af3af4e4..87371727 100644
--- a/src/api/k2v/item.rs
+++ b/src/api/k2v/item.rs
@@ -18,6 +18,10 @@ pub enum ReturnFormat {
 	Either,
 }
 
+pub(crate) fn parse_causality_token(s: &str) -> Result<CausalContext, Error> {
+	CausalContext::parse(s).ok_or(Error::InvalidCausalityToken)
+}
+
 impl ReturnFormat {
 	pub fn from(req: &Request<ReqBody>) -> Result<Self, Error> {
 		let accept = match req.headers().get(header::ACCEPT) {
@@ -136,7 +140,7 @@ pub async fn handle_insert_item(
 		.get(X_GARAGE_CAUSALITY_TOKEN)
 		.map(|s| s.to_str())
 		.transpose()?
-		.map(CausalContext::parse_helper)
+		.map(parse_causality_token)
 		.transpose()?;
 
 	let body = http_body_util::BodyExt::collect(req.into_body())
@@ -176,7 +180,7 @@ pub async fn handle_delete_item(
 		.get(X_GARAGE_CAUSALITY_TOKEN)
 		.map(|s| s.to_str())
 		.transpose()?
-		.map(CausalContext::parse_helper)
+		.map(parse_causality_token)
 		.transpose()?;
 
 	let value = DvvsValue::Deleted;
diff --git a/src/api/s3/api_server.rs b/src/api/s3/api_server.rs
index 1737af33..f9dafa10 100644
--- a/src/api/s3/api_server.rs
+++ b/src/api/s3/api_server.rs
@@ -150,7 +150,8 @@ impl ApiHandler for S3ApiServer {
 		let bucket_id = garage
 			.bucket_helper()
 			.resolve_bucket(&bucket_name, &api_key)
-			.await?;
+			.await
+			.map_err(pass_helper_error)?;
 		let bucket = garage
 			.bucket_helper()
 			.get_existing_bucket(bucket_id)
diff --git a/src/api/s3/copy.rs b/src/api/s3/copy.rs
index e375a714..b67ace88 100644
--- a/src/api/s3/copy.rs
+++ b/src/api/s3/copy.rs
@@ -655,7 +655,8 @@ async fn get_copy_source(ctx: &ReqCtx, req: &Request<ReqBody>) -> Result<Object,
 	let source_bucket_id = garage
 		.bucket_helper()
 		.resolve_bucket(&source_bucket.to_string(), api_key)
-		.await?;
+		.await
+		.map_err(pass_helper_error)?;
 
 	if !api_key.allow_read(&source_bucket_id) {
 		return Err(Error::forbidden(format!(
diff --git a/src/api/s3/cors.rs b/src/api/s3/cors.rs
index 173b7ffe..32dcc0d5 100644
--- a/src/api/s3/cors.rs
+++ b/src/api/s3/cors.rs
@@ -1,6 +1,7 @@
-use quick_xml::de::from_reader;
 use std::sync::Arc;
 
+use quick_xml::de::from_reader;
+
 use http::header::{
 	ACCESS_CONTROL_ALLOW_HEADERS, ACCESS_CONTROL_ALLOW_METHODS, ACCESS_CONTROL_ALLOW_ORIGIN,
 	ACCESS_CONTROL_EXPOSE_HEADERS, ACCESS_CONTROL_REQUEST_HEADERS, ACCESS_CONTROL_REQUEST_METHOD,
@@ -14,7 +15,7 @@ use http_body_util::BodyExt;
 
 use serde::{Deserialize, Serialize};
 
-use crate::common_error::CommonError;
+use crate::common_error::{helper_error_as_internal, CommonError};
 use crate::helpers::*;
 use crate::s3::api_server::{ReqBody, ResBody};
 use crate::s3::error::*;
@@ -116,9 +117,16 @@ pub async fn handle_options_api(
 	// OPTIONS calls are not auhtenticated).
 	if let Some(bn) = bucket_name {
 		let helper = garage.bucket_helper();
-		let bucket_id = helper.resolve_global_bucket_name(&bn).await?;
+		let bucket_id = helper
+			.resolve_global_bucket_name(&bn)
+			.await
+			.map_err(helper_error_as_internal)?;
 		if let Some(id) = bucket_id {
-			let bucket = garage.bucket_helper().get_existing_bucket(id).await?;
+			let bucket = garage
+				.bucket_helper()
+				.get_existing_bucket(id)
+				.await
+				.map_err(helper_error_as_internal)?;
 			let bucket_params = bucket.state.into_option().unwrap();
 			handle_options_for_bucket(req, &bucket_params)
 		} else {
diff --git a/src/api/s3/error.rs b/src/api/s3/error.rs
index 2855e0b3..22d2fe14 100644
--- a/src/api/s3/error.rs
+++ b/src/api/s3/error.rs
@@ -4,7 +4,10 @@ use err_derive::Error;
 use hyper::header::HeaderValue;
 use hyper::{HeaderMap, StatusCode};
 
-use crate::common_error::CommonError;
+use garage_model::helper::error::Error as HelperError;
+
+pub(crate) use crate::common_error::pass_helper_error;
+use crate::common_error::{helper_error_as_internal, CommonError};
 pub use crate::common_error::{CommonErrorDerivative, OkOrBadRequest, OkOrInternalError};
 use crate::generic_server::ApiError;
 use crate::helpers::*;
@@ -87,6 +90,14 @@ where
 	}
 }
 
+// Helper errors are always passed as internal errors by default.
+// To pass the specific error code back to the client, use `pass_helper_error`.
+impl From<HelperError> for Error {
+	fn from(err: HelperError) -> Error {
+		Error::Common(helper_error_as_internal(err))
+	}
+}
+
 impl CommonErrorDerivative for Error {}
 
 impl From<roxmltree::Error> for Error {
diff --git a/src/api/s3/post_object.rs b/src/api/s3/post_object.rs
index 725f3847..5279ec6a 100644
--- a/src/api/s3/post_object.rs
+++ b/src/api/s3/post_object.rs
@@ -107,7 +107,8 @@ pub async fn handle_post_object(
 	let bucket_id = garage
 		.bucket_helper()
 		.resolve_bucket(&bucket_name, &api_key)
-		.await?;
+		.await
+		.map_err(pass_helper_error)?;
 
 	if !api_key.allow_write(&bucket_id) {
 		return Err(Error::forbidden("Operation is not allowed for this key."));