Fix some error codes

2022-01-05 17:07:36 +01:00 · 2022-01-05 17:07:36 +01:00 · 168a90dfb5
commit 168a90dfb5
parent fb1e31add0
8 changed files with 53 additions and 36 deletions
--- a/src/api/api_server.rs
+++ b/src/api/api_server.rs
@ -107,6 +107,7 @@ async fn handler_inner(garage: Arc<Garage>, req: Request<Body>) -> Result<Respon
 		.and_then(|root_domain| host_to_bucket(&host, root_domain));

 	let endpoint = Endpoint::from_request(&req, bucket.map(ToOwned::to_owned))?;
+	debug!("Endpoint: {:?}", endpoint);

 	// Special code path for CreateBucket API endpoint
 	if let Endpoint::CreateBucket { bucket } = endpoint {
@ -306,7 +307,7 @@ async fn resolve_bucket(
 			.bucket_helper()
 			.resolve_global_bucket_name(bucket_name)
 			.await?
-			.ok_or(Error::NotFound)?)
+			.ok_or(Error::NoSuchBucket)?)
 	}
 }

--- a/src/api/error.rs
+++ b/src/api/error.rs
@ -35,8 +35,24 @@ pub enum Error {
 	AuthorizationHeaderMalformed(String),

 	/// The object requested don't exists
-	#[error(display = "Not found")]
-	NotFound,
+	#[error(display = "Key not found")]
+	NoSuchKey,
+
+	/// The bucket requested don't exists
+	#[error(display = "Bucket not found")]
+	NoSuchBucket,
+
+	/// The multipart upload requested don't exists
+	#[error(display = "Upload not found")]
+	NoSuchUpload,
+
+	/// 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
 	/// The request contained an invalid UTF-8 sequence in its path or in other parameters
@ -97,7 +113,8 @@ 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::NotFound => StatusCode::NOT_FOUND,
+			Error::NoSuchKey | Error::NoSuchBucket | Error::NoSuchUpload => StatusCode::NOT_FOUND,
+			Error::BucketNotEmpty | Error::BucketAlreadyExists => StatusCode::CONFLICT,
 			Error::Forbidden(_) => StatusCode::FORBIDDEN,
 			Error::InternalError(
 				GarageError::Timeout
@ -115,9 +132,14 @@ impl Error {

 	pub fn aws_code(&self) -> &'static str {
 		match self {
-			Error::NotFound => "NoSuchKey",
+			Error::NoSuchKey => "NoSuchKey",
+			Error::NoSuchBucket => "NoSuchBucket",
+			Error::NoSuchUpload => "NoSuchUpload",
+			Error::BucketAlreadyExists => "BucketAlreadyExists",
+			Error::BucketNotEmpty => "BucketNotEmpty",
 			Error::Forbidden(_) => "AccessDenied",
 			Error::AuthorizationHeaderMalformed(_) => "AuthorizationHeaderMalformed",
+			Error::NotImplemented(_) => "NotImplemented",
 			Error::InternalError(
 				GarageError::Timeout
 				| GarageError::RemoteError(_)
--- a/src/api/s3_bucket.rs
+++ b/src/api/s3_bucket.rs
@ -153,10 +153,7 @@ pub async fn handle_create_bucket(
 		// otherwise return a forbidden error.
 		let kp = api_key.bucket_permissions(&bucket_id);
 		if !(kp.allow_write || kp.allow_owner) {
-			return Err(Error::Forbidden(format!(
-				"Key {} does not have write or owner permissions on bucket {}",
-				api_key.key_id, bucket_name
-			)));
+			return Err(Error::BucketAlreadyExists);
 		}
 	} else {
 		// Create the bucket!
@ -231,10 +228,7 @@ pub async fn handle_delete_bucket(
 			.get_range(&bucket_id, None, Some(DeletedFilter::NotDeleted), 10)
 			.await?;
 		if !objects.is_empty() {
-			return Err(Error::BadRequest(format!(
-				"Bucket {} is not empty",
-				bucket_name
-			)));
+			return Err(Error::BucketNotEmpty);
 		}

 		// --- done checking, now commit ---
--- a/src/api/s3_copy.rs
+++ b/src/api/s3_copy.rs
@ -27,14 +27,14 @@ pub async fn handle_copy(
 		.object_table
 		.get(&source_bucket_id, &source_key.to_string())
 		.await?
-		.ok_or(Error::NotFound)?;
+		.ok_or(Error::NoSuchKey)?;

 	let source_last_v = source_object
 		.versions()
 		.iter()
 		.rev()
 		.find(|v| v.is_complete())
-		.ok_or(Error::NotFound)?;
+		.ok_or(Error::NoSuchKey)?;

 	let source_last_state = match &source_last_v.state {
 		ObjectVersionState::Complete(x) => x,
@ -47,7 +47,7 @@ pub async fn handle_copy(
 	// Implement x-amz-metadata-directive: REPLACE
 	let old_meta = match source_last_state {
 		ObjectVersionData::DeleteMarker => {
-			return Err(Error::NotFound);
+			return Err(Error::NoSuchKey);
 		}
 		ObjectVersionData::Inline(meta, _bytes) => meta,
 		ObjectVersionData::FirstBlock(meta, _fbh) => meta,
@ -88,7 +88,7 @@ pub async fn handle_copy(
 				.version_table
 				.get(&source_last_v.uuid, &EmptyKey)
 				.await?;
-			let source_version = source_version.ok_or(Error::NotFound)?;
+			let source_version = source_version.ok_or(Error::NoSuchKey)?;

 			// Write an "uploading" marker in Object table
 			// This holds a reference to the object in the Version table
--- a/src/api/s3_delete.rs
+++ b/src/api/s3_delete.rs
@ -21,7 +21,7 @@ async fn handle_delete_internal(
 		.object_table
 		.get(&bucket_id, &key.to_string())
 		.await?
-		.ok_or(Error::NotFound)?; // No need to delete
+		.ok_or(Error::NoSuchKey)?; // No need to delete

 	let interesting_versions = object.versions().iter().filter(|v| {
 		!matches!(
@ -40,7 +40,7 @@ async fn handle_delete_internal(
 		timestamp = std::cmp::max(timestamp, v.timestamp + 1);
 	}

-	let deleted_version = version_to_delete.ok_or(Error::NotFound)?;
+	let deleted_version = version_to_delete.ok_or(Error::NoSuchKey)?;

 	let version_uuid = gen_uuid();

--- a/src/api/s3_get.rs
+++ b/src/api/s3_get.rs
@ -92,14 +92,14 @@ pub async fn handle_head(
 		.object_table
 		.get(&bucket_id, &key.to_string())
 		.await?
-		.ok_or(Error::NotFound)?;
+		.ok_or(Error::NoSuchKey)?;

 	let version = object
 		.versions()
 		.iter()
 		.rev()
 		.find(|v| v.is_data())
-		.ok_or(Error::NotFound)?;
+		.ok_or(Error::NoSuchKey)?;

 	let version_meta = match &version.state {
 		ObjectVersionState::Complete(ObjectVersionData::Inline(meta, _)) => meta,
@ -131,21 +131,21 @@ pub async fn handle_get(
 		.object_table
 		.get(&bucket_id, &key.to_string())
 		.await?
-		.ok_or(Error::NotFound)?;
+		.ok_or(Error::NoSuchKey)?;

 	let last_v = object
 		.versions()
 		.iter()
 		.rev()
 		.find(|v| v.is_complete())
-		.ok_or(Error::NotFound)?;
+		.ok_or(Error::NoSuchKey)?;

 	let last_v_data = match &last_v.state {
 		ObjectVersionState::Complete(x) => x,
 		_ => unreachable!(),
 	};
 	let last_v_meta = match last_v_data {
-		ObjectVersionData::DeleteMarker => return Err(Error::NotFound),
+		ObjectVersionData::DeleteMarker => return Err(Error::NoSuchKey),
 		ObjectVersionData::Inline(meta, _) => meta,
 		ObjectVersionData::FirstBlock(meta, _) => meta,
 	};
@ -196,7 +196,7 @@ pub async fn handle_get(
 			let get_next_blocks = garage.version_table.get(&last_v.uuid, &EmptyKey);

 			let (first_block, version) = futures::try_join!(read_first_block, get_next_blocks)?;
-			let version = version.ok_or(Error::NotFound)?;
+			let version = version.ok_or(Error::NoSuchKey)?;

 			let mut blocks = version
 				.blocks
@ -261,7 +261,7 @@ async fn handle_get_range(
 			let version = garage.version_table.get(&version.uuid, &EmptyKey).await?;
 			let version = match version {
 				Some(v) => v,
-				None => return Err(Error::NotFound),
+				None => return Err(Error::NoSuchKey),
 			};

 			// We will store here the list of blocks that have an intersection with the requested
--- a/src/api/s3_put.rs
+++ b/src/api/s3_put.rs
@ -382,7 +382,7 @@ pub async fn handle_put_part(
 		.iter()
 		.any(|v| v.uuid == version_uuid && v.is_uploading())
 	{
-		return Err(Error::NotFound);
+		return Err(Error::NoSuchUpload);
 	}

 	// Copy block to store
@ -449,15 +449,15 @@ pub async fn handle_complete_multipart_upload(
 		garage.version_table.get(&version_uuid, &EmptyKey),
 	)?;

-	let object = object.ok_or_else(|| Error::BadRequest("Object not found".to_string()))?;
+	let object = object.ok_or(Error::NoSuchKey)?;
 	let mut object_version = object
 		.versions()
 		.iter()
 		.find(|v| v.uuid == version_uuid && v.is_uploading())
 		.cloned()
-		.ok_or_else(|| Error::BadRequest("Version not found".to_string()))?;
+		.ok_or(Error::NoSuchUpload)?;

-	let version = version.ok_or_else(|| Error::BadRequest("Version not found".to_string()))?;
+	let version = version.ok_or(Error::NoSuchKey)?;
 	if version.blocks.is_empty() {
 		return Err(Error::BadRequest("No data was uploaded".to_string()));
 	}
@ -538,14 +538,14 @@ pub async fn handle_abort_multipart_upload(
 		.object_table
 		.get(&bucket_id, &key.to_string())
 		.await?;
-	let object = object.ok_or_else(|| Error::BadRequest("Object not found".to_string()))?;
+	let object = object.ok_or(Error::NoSuchKey)?;

 	let object_version = object
 		.versions()
 		.iter()
 		.find(|v| v.uuid == version_uuid && v.is_uploading());
 	let mut object_version = match object_version {
-		None => return Err(Error::NotFound),
+		None => return Err(Error::NoSuchUpload),
 		Some(x) => x.clone(),
 	};

@ -611,9 +611,9 @@ pub(crate) fn get_headers(req: &Request<Body>) -> Result<ObjectVersionHeaders, E
 }

 fn decode_upload_id(id: &str) -> Result<Uuid, Error> {
-	let id_bin = hex::decode(id).ok_or_bad_request("Invalid upload ID")?;
+	let id_bin = hex::decode(id).map_err(|_| Error::NoSuchUpload)?;
 	if id_bin.len() != 32 {
-		return None.ok_or_bad_request("Invalid upload ID");
+		return Err(Error::NoSuchUpload);
 	}
 	let mut uuid = [0u8; 32];
 	uuid.copy_from_slice(&id_bin[..]);
--- a/src/api/s3_website.rs
+++ b/src/api/s3_website.rs
@ -22,7 +22,7 @@ pub async fn handle_delete_website(
 		.bucket_table
 		.get(&EmptyKey, &bucket_id)
 		.await?
-		.ok_or(Error::NotFound)?;
+		.ok_or(Error::NoSuchBucket)?;

 	if let crdt::Deletable::Present(param) = &mut bucket.state {
 		param.website_config.update(None);
@ -50,7 +50,7 @@ pub async fn handle_put_website(
 		.bucket_table
 		.get(&EmptyKey, &bucket_id)
 		.await?
-		.ok_or(Error::NotFound)?;
+		.ok_or(Error::NoSuchBucket)?;

 	let conf: WebsiteConfiguration = from_reader(&body as &[u8])?;
 	conf.validate()?;