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

Reviewed-on: Deuxfleurs/garage#942

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 {

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 {
+impl TryFrom<HelperError> for CommonError {
-	fn from(err: HelperError) -> Self {
+	type Error = HelperError;
+
+	fn try_from(err: HelperError) -> Result<Self, HelperError> {
 		match err {
-			HelperError::Internal(i) => Self::InternalError(i),
+			HelperError::Internal(i) => Ok(Self::InternalError(i)),
-			HelperError::BadRequest(b) => Self::BadRequest(b),
+			HelperError::BadRequest(b) => Ok(Self::BadRequest(b)),
-			HelperError::InvalidBucketName(n) => Self::InvalidBucketName(n),
+			HelperError::InvalidBucketName(n) => Ok(Self::InvalidBucketName(n)),
-			HelperError::NoSuchBucket(n) => Self::NoSuchBucket(n),
+			HelperError::NoSuchBucket(n) => Ok(Self::NoSuchBucket(n)),
-			e => Self::bad_request(format!("{}", e)),
+			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(

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() {

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 {

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,
 		}
 	}
 

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;

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)

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!(

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 {

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 {

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."));

src/model/k2v/causality.rs

@@ -16,8 +16,6 @@ use serde::{Deserialize, Serialize};
 
 use garage_util::data::*;
 
-use crate::helper::error::{Error as HelperError, OkOrBadRequest};
-
 /// Node IDs used in K2V are u64 integers that are the abbreviation
 /// of full Garage node IDs which are 256-bit UUIDs.
 pub type K2VNodeId = u64;
@@ -99,10 +97,6 @@ impl CausalContext {
 		Some(ret)
 	}
 
-	pub fn parse_helper(s: &str) -> Result<Self, HelperError> {
-		Self::parse(s).ok_or_bad_request("Invalid causality token")
-	}
-
 	/// Check if this causal context contains newer items than another one
 	pub fn is_newer_than(&self, other: &Self) -> bool {
 		vclock_gt(&self.vector_clock, &other.vector_clock)