From e7ddba53e38195744fd3bac29eda35fca87ab095 Mon Sep 17 00:00:00 2001
From: Alex Auvolat <alex@adnab.me>
Date: Thu, 12 May 2022 17:10:25 +0200
Subject: [PATCH] Slightly more detailed error reporting from helper

---
 src/api/error.rs           |  5 +++++
 src/garage/main.rs         |  1 +
 src/model/helper/bucket.rs | 19 +++++--------------
 src/model/helper/error.rs  | 10 ++++++++++
 src/model/helper/key.rs    |  2 +-
 5 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/src/api/error.rs b/src/api/error.rs
index 4b7254d2..f111c801 100644
--- a/src/api/error.rs
+++ b/src/api/error.rs
@@ -84,6 +84,10 @@ pub enum Error {
 	#[error(display = "Invalid base64: {}", _0)]
 	InvalidBase64(#[error(source)] base64::DecodeError),
 
+	/// Bucket name is not valid according to AWS S3 specs
+	#[error(display = "Invalid bucket name")]
+	InvalidBucketName,
+
 	/// The client sent invalid XML data
 	#[error(display = "Invalid XML: {}", _0)]
 	InvalidXml(String),
@@ -126,6 +130,7 @@ impl From<HelperError> for Error {
 		match err {
 			HelperError::Internal(i) => Self::InternalError(i),
 			HelperError::BadRequest(b) => Self::BadRequest(b),
+			e => Self::BadRequest(format!("{}", e)),
 		}
 	}
 }
diff --git a/src/garage/main.rs b/src/garage/main.rs
index 69ab1147..bd09b6ea 100644
--- a/src/garage/main.rs
+++ b/src/garage/main.rs
@@ -142,6 +142,7 @@ async fn cli_command(opt: Opt) -> Result<(), Error> {
 	match cli_command_dispatch(opt.cmd, &system_rpc_endpoint, &admin_rpc_endpoint, id).await {
 		Err(HelperError::Internal(i)) => Err(Error::Message(format!("Internal error: {}", i))),
 		Err(HelperError::BadRequest(b)) => Err(Error::Message(b)),
+		Err(e) => Err(Error::Message(format!("{}", e))),
 		Ok(x) => Ok(x),
 	}
 }
diff --git a/src/model/helper/bucket.rs b/src/model/helper/bucket.rs
index 7e81b946..788bf3a6 100644
--- a/src/model/helper/bucket.rs
+++ b/src/model/helper/bucket.rs
@@ -71,10 +71,7 @@ impl<'a> BucketHelper<'a> {
 			.get(&EmptyKey, &bucket_id)
 			.await?
 			.filter(|b| !b.is_deleted())
-			.ok_or_bad_request(format!(
-				"Bucket {:?} does not exist or has been deleted",
-				bucket_id
-			))
+			.ok_or_else(|| Error::NoSuchBucket(hex::encode(bucket_id)))
 	}
 
 	/// Sets a new alias for a bucket in global namespace.
@@ -88,10 +85,7 @@ impl<'a> BucketHelper<'a> {
 		alias_name: &String,
 	) -> Result<(), Error> {
 		if !is_valid_bucket_name(alias_name) {
-			return Err(Error::BadRequest(format!(
-				"{}: {}",
-				alias_name, INVALID_BUCKET_NAME_MESSAGE
-			)));
+			return Err(Error::InvalidBucketName(alias_name.to_string()));
 		}
 
 		let mut bucket = self.get_existing_bucket(bucket_id).await?;
@@ -122,7 +116,7 @@ impl<'a> BucketHelper<'a> {
 
 		let alias = match alias {
 			None => BucketAlias::new(alias_name.clone(), alias_ts, Some(bucket_id))
-				.ok_or_bad_request(format!("{}: {}", alias_name, INVALID_BUCKET_NAME_MESSAGE))?,
+				.ok_or_else(|| Error::InvalidBucketName(alias_name.clone()))?,
 			Some(mut a) => {
 				a.state = Lww::raw(alias_ts, Some(bucket_id));
 				a
@@ -210,7 +204,7 @@ impl<'a> BucketHelper<'a> {
 			.bucket_alias_table
 			.get(&EmptyKey, alias_name)
 			.await?
-			.ok_or_message(format!("Alias {} not found", alias_name))?;
+			.ok_or_else(|| Error::NoSuchBucket(alias_name.to_string()))?;
 
 		// Checks ok, remove alias
 		let alias_ts = match bucket.state.as_option() {
@@ -252,10 +246,7 @@ impl<'a> BucketHelper<'a> {
 		let key_helper = KeyHelper(self.0);
 
 		if !is_valid_bucket_name(alias_name) {
-			return Err(Error::BadRequest(format!(
-				"{}: {}",
-				alias_name, INVALID_BUCKET_NAME_MESSAGE
-			)));
+			return Err(Error::InvalidBucketName(alias_name.to_string()));
 		}
 
 		let mut bucket = self.get_existing_bucket(bucket_id).await?;
diff --git a/src/model/helper/error.rs b/src/model/helper/error.rs
index 30b2ba32..3ca8f55c 100644
--- a/src/model/helper/error.rs
+++ b/src/model/helper/error.rs
@@ -10,6 +10,16 @@ pub enum Error {
 
 	#[error(display = "Bad request: {}", _0)]
 	BadRequest(String),
+
+	/// Bucket name is not valid according to AWS S3 specs
+	#[error(display = "Invalid bucket name: {}", _0)]
+	InvalidBucketName(String),
+
+	#[error(display = "Access key not found: {}", _0)]
+	NoSuchAccessKey(String),
+
+	#[error(display = "Bucket not found: {}", _0)]
+	NoSuchBucket(String),
 }
 
 impl From<netapp::error::Error> for Error {
diff --git a/src/model/helper/key.rs b/src/model/helper/key.rs
index eea37f79..c1a8e974 100644
--- a/src/model/helper/key.rs
+++ b/src/model/helper/key.rs
@@ -34,7 +34,7 @@ impl<'a> KeyHelper<'a> {
 			.get(&EmptyKey, key_id)
 			.await?
 			.filter(|b| !b.state.is_deleted())
-			.ok_or_bad_request(format!("Key {} does not exist or has been deleted", key_id))
+			.ok_or_else(|| Error::NoSuchAccessKey(key_id.to_string()))
 	}
 
 	/// Returns a Key if it is present in key table,