From 7a5d329e49cc7018cbfa14d37589f51860f66cf0 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Fri, 13 May 2022 15:21:32 +0200 Subject: [PATCH] More error refactoring --- src/api/admin/bucket.rs | 4 ++-- src/api/admin/cluster.rs | 2 +- src/api/admin/error.rs | 5 ----- src/api/admin/key.rs | 2 +- src/api/admin/mod.rs | 11 ----------- src/api/common_error.rs | 5 +++++ src/api/helpers.rs | 29 +---------------------------- src/api/k2v/api_server.rs | 2 +- src/api/s3/api_server.rs | 2 +- src/api/s3/copy.rs | 4 ++-- src/api/s3/post_object.rs | 3 +-- src/model/helper/bucket.rs | 22 ++++++++++++++++++++++ 12 files changed, 37 insertions(+), 54 deletions(-) diff --git a/src/api/admin/bucket.rs b/src/api/admin/bucket.rs index db1fda0f..b226c015 100644 --- a/src/api/admin/bucket.rs +++ b/src/api/admin/bucket.rs @@ -18,7 +18,7 @@ use garage_model::s3::object_table::ObjectFilter; use crate::admin::error::*; use crate::admin::key::ApiBucketKeyPerm; -use crate::admin::parse_json_body; +use crate::helpers::parse_json_body; pub async fn handle_list_buckets(garage: &Arc) -> Result, Error> { let buckets = garage @@ -333,7 +333,7 @@ pub async fn handle_delete_bucket( ) .await?; if !objects.is_empty() { - return Err(Error::bad_request("Bucket is not empty")); + return Err(Error::BucketNotEmpty); } // --- done checking, now commit --- diff --git a/src/api/admin/cluster.rs b/src/api/admin/cluster.rs index db4d968d..91d99d8a 100644 --- a/src/api/admin/cluster.rs +++ b/src/api/admin/cluster.rs @@ -14,7 +14,7 @@ use garage_rpc::layout::*; use garage_model::garage::Garage; use crate::admin::error::*; -use crate::admin::parse_json_body; +use crate::helpers::parse_json_body; pub async fn handle_get_cluster_status(garage: &Arc) -> Result, Error> { let res = GetClusterStatusResponse { diff --git a/src/api/admin/error.rs b/src/api/admin/error.rs index 1f49fed5..1d68dc69 100644 --- a/src/api/admin/error.rs +++ b/src/api/admin/error.rs @@ -40,10 +40,6 @@ pub enum Error { /// Bucket name is not valid according to AWS S3 specs #[error(display = "Invalid bucket name")] InvalidBucketName, - - /// The client sent a request for an action not supported by garage - #[error(display = "Unimplemented action: {}", _0)] - NotImplemented(String), } impl From for Error @@ -75,7 +71,6 @@ impl ApiError for Error { Error::NoSuchAccessKey | Error::NoSuchBucket => StatusCode::NOT_FOUND, Error::BucketNotEmpty | Error::BucketAlreadyExists => StatusCode::CONFLICT, Error::Forbidden(_) => StatusCode::FORBIDDEN, - Error::NotImplemented(_) => StatusCode::NOT_IMPLEMENTED, Error::InvalidBucketName => StatusCode::BAD_REQUEST, } } diff --git a/src/api/admin/key.rs b/src/api/admin/key.rs index e5f25601..8060bf1a 100644 --- a/src/api/admin/key.rs +++ b/src/api/admin/key.rs @@ -12,7 +12,7 @@ use garage_model::garage::Garage; use garage_model::key_table::*; use crate::admin::error::*; -use crate::admin::parse_json_body; +use crate::helpers::parse_json_body; pub async fn handle_list_keys(garage: &Arc) -> Result, Error> { let res = garage diff --git a/src/api/admin/mod.rs b/src/api/admin/mod.rs index 73700e6e..c4857c10 100644 --- a/src/api/admin/mod.rs +++ b/src/api/admin/mod.rs @@ -5,14 +5,3 @@ mod router; mod bucket; mod cluster; mod key; - -use hyper::{Body, Request}; -use serde::Deserialize; - -use error::*; - -pub async fn parse_json_body Deserialize<'de>>(req: Request) -> Result { - let body = hyper::body::to_bytes(req.into_body()).await?; - let resp: T = serde_json::from_slice(&body).ok_or_bad_request("Invalid JSON")?; - Ok(resp) -} diff --git a/src/api/common_error.rs b/src/api/common_error.rs index eca27e6b..48106e03 100644 --- a/src/api/common_error.rs +++ b/src/api/common_error.rs @@ -38,6 +38,11 @@ impl CommonError { CommonError::BadRequest(_) => StatusCode::BAD_REQUEST, } } + + + pub fn bad_request(msg: M) -> Self { + CommonError::BadRequest(msg.to_string()) + } } /// Trait to map error to the Bad Request error code diff --git a/src/api/helpers.rs b/src/api/helpers.rs index 121fbd5a..599be3f7 100644 --- a/src/api/helpers.rs +++ b/src/api/helpers.rs @@ -2,12 +2,7 @@ use hyper::{Body, Request}; use idna::domain_to_unicode; use serde::Deserialize; -use garage_util::data::*; - -use garage_model::garage::Garage; -use garage_model::key_table::Key; - -use crate::s3::error::*; +use crate::common_error::{*, CommonError as Error}; /// What kind of authorization is required to perform a given action #[derive(Debug, Clone, PartialEq, Eq)] @@ -81,28 +76,6 @@ pub fn authority_to_host(authority: &str) -> Result { authority.map(|h| domain_to_unicode(h).0) } -#[allow(clippy::ptr_arg)] -pub async fn resolve_bucket( - garage: &Garage, - bucket_name: &String, - api_key: &Key, -) -> Result { - let api_key_params = api_key - .state - .as_option() - .ok_or_internal_error("Key should not be deleted at this point")?; - - if let Some(Some(bucket_id)) = api_key_params.local_aliases.get(bucket_name) { - Ok(*bucket_id) - } else { - Ok(garage - .bucket_helper() - .resolve_global_bucket_name(bucket_name) - .await? - .ok_or(Error::NoSuchBucket)?) - } -} - /// Extract the bucket name and the key name from an HTTP path and possibly a bucket provided in /// the host header of the request /// diff --git a/src/api/k2v/api_server.rs b/src/api/k2v/api_server.rs index 9b4ad882..38ef8d45 100644 --- a/src/api/k2v/api_server.rs +++ b/src/api/k2v/api_server.rs @@ -100,7 +100,7 @@ impl ApiHandler for K2VApiServer { "k2v", )?; - let bucket_id = resolve_bucket(&garage, &bucket_name, &api_key).await?; + let bucket_id = garage.bucket_helper().resolve_bucket(&bucket_name, &api_key).await?; let bucket = garage .bucket_table .get(&EmptyKey, &bucket_id) diff --git a/src/api/s3/api_server.rs b/src/api/s3/api_server.rs index 77ac3879..6b565fd0 100644 --- a/src/api/s3/api_server.rs +++ b/src/api/s3/api_server.rs @@ -149,7 +149,7 @@ impl ApiHandler for S3ApiServer { return handle_create_bucket(&garage, req, content_sha256, api_key, bucket_name).await; } - let bucket_id = resolve_bucket(&garage, &bucket_name, &api_key).await?; + let bucket_id = garage.bucket_helper().resolve_bucket(&bucket_name, &api_key).await?; let bucket = garage .bucket_table .get(&EmptyKey, &bucket_id) diff --git a/src/api/s3/copy.rs b/src/api/s3/copy.rs index abd90f4a..2468678e 100644 --- a/src/api/s3/copy.rs +++ b/src/api/s3/copy.rs @@ -19,7 +19,7 @@ use garage_model::s3::object_table::*; use garage_model::s3::version_table::*; use crate::s3::error::*; -use crate::helpers::{parse_bucket_key, resolve_bucket}; +use crate::helpers::{parse_bucket_key}; use crate::s3::put::{decode_upload_id, get_headers}; use crate::s3::xml::{self as s3_xml, xmlns_tag}; @@ -413,7 +413,7 @@ async fn get_copy_source( let copy_source = percent_encoding::percent_decode_str(copy_source).decode_utf8()?; let (source_bucket, source_key) = parse_bucket_key(©_source, None)?; - let source_bucket_id = resolve_bucket(garage, &source_bucket.to_string(), api_key).await?; + let source_bucket_id = garage.bucket_helper().resolve_bucket(&source_bucket.to_string(), api_key).await?; if !api_key.allow_read(&source_bucket_id) { return Err(Error::Forbidden(format!( diff --git a/src/api/s3/post_object.rs b/src/api/s3/post_object.rs index 343aa366..c4b63452 100644 --- a/src/api/s3/post_object.rs +++ b/src/api/s3/post_object.rs @@ -15,7 +15,6 @@ use serde::Deserialize; use garage_model::garage::Garage; use crate::s3::error::*; -use crate::helpers::resolve_bucket; use crate::s3::put::{get_headers, save_stream}; use crate::s3::xml as s3_xml; use crate::signature::payload::{parse_date, verify_v4}; @@ -129,7 +128,7 @@ pub async fn handle_post_object( ) .await?; - let bucket_id = resolve_bucket(&garage, &bucket, &api_key).await?; + let bucket_id = garage.bucket_helper().resolve_bucket(&bucket, &api_key).await?; if !api_key.allow_write(&bucket_id) { return Err(Error::Forbidden( diff --git a/src/model/helper/bucket.rs b/src/model/helper/bucket.rs index 788bf3a6..2f1c6ae9 100644 --- a/src/model/helper/bucket.rs +++ b/src/model/helper/bucket.rs @@ -6,6 +6,7 @@ use garage_util::time::*; use crate::bucket_alias_table::*; use crate::bucket_table::*; +use crate::key_table::*; use crate::garage::Garage; use crate::helper::error::*; use crate::helper::key::KeyHelper; @@ -49,6 +50,27 @@ impl<'a> BucketHelper<'a> { } } + #[allow(clippy::ptr_arg)] + pub async fn resolve_bucket( + &self, + bucket_name: &String, + api_key: &Key, + ) -> Result { + let api_key_params = api_key + .state + .as_option() + .ok_or_message("Key should not be deleted at this point")?; + + if let Some(Some(bucket_id)) = api_key_params.local_aliases.get(bucket_name) { + Ok(*bucket_id) + } else { + Ok(self. + resolve_global_bucket_name(bucket_name) + .await? + .ok_or_else(|| Error::NoSuchBucket(bucket_name.to_string()))?) + } + } + /// Returns a Bucket if it is present in bucket table, /// even if it is in deleted state. Querying a non-existing /// bucket ID returns an internal error.