Deuxfleurs/garage
41
56
Fork 73
Code Issues 102 Pull requests 14 Releases 30 Wiki Activity

add proper request router for s3 api #163

Merged
lx merged 5 commits from trinity-1686a/garage:s3-router into main 2021-12-06 14:17:47 +00:00
Conversation 2 Commits 5 Files changed 7 +82 -129
3 changed files with 82 additions and 129 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 3043cd1a86 - Show all commits

11
src/api/api_server.rs
View file

@ -228,10 +228,11 @@ async fn handler_inner(garage: Arc<Garage>, req: Request<Body>) -> Result<Respon
list_type,
..
} => {
if list_type == "2" {
handle_list(
garage,
&ListObjectsQuery {
lx commented 2021-11-30 14:01:23 +00:00
Outdated
Review
Copy link

At this point we should probably return 400 bad request if list_type is not equal to "v2". It doesn't make much sense calling handle_list with is_v2 = false here because the arguments we are giving are those of ListObjectsV2: continuation_token, start_after, etc.

(question: what happens on AWS S3 if we give a list_type value that is not "v2" ?)

At this point we should probably return 400 bad request if `list_type` is not equal to `"v2"`. It doesn't make much sense calling `handle_list` with `is_v2 = false` here because the arguments we are giving are those of ListObjectsV2: continuation_token, start_after, etc. (question: what happens on AWS S3 if we give a list_type value that is not "v2" ?)
is_v2: list_type == "v2",
is_v2: true,
bucket,
delimiter: delimiter.map(|d| d.to_string()),
max_keys: max_keys.unwrap_or(1000),
@ -243,11 +244,17 @@ async fn handler_inner(garage: Arc<Garage>, req: Request<Body>) -> Result<Respon
},
)
.await
} else {
Err(Error::BadRequest(format!(
"Invalid endpoint: list-type={}",
list_type
lx commented 2021-11-30 13:56:13 +00:00
Outdated
Review
Copy link

TODO (not necessarily in this PR): add a new error type so that we return HTTP 501

TODO (not necessarily in this PR): add a new error type so that we return HTTP 501
👍 1
)))
}
}
Endpoint::DeleteObjects { bucket } => {
handle_delete_objects(garage, &bucket, req, content_sha256).await
}
_ => Err(Error::BadRequest("Unsupported call".to_string())),
endpoint => Err(Error::NotImplemented(endpoint.name().to_owned())),
}
}

5
src/api/error.rs
View file

@ -65,6 +65,10 @@ pub enum Error {
/// The client sent an invalid request
#[error(display = "Bad request: {}", _0)]
BadRequest(String),
/// The client sent a request for an action not supported by garage
#[error(display = "Unimplemented action: {}", _0)]
NotImplemented(String),
}
impl From<roxmltree::Error> for Error {
@ -94,6 +98,7 @@ impl Error {
StatusCode::INTERNAL_SERVER_ERROR
}
Error::InvalidRange(_) => StatusCode::RANGE_NOT_SATISFIABLE,
Error::NotImplemented(_) => StatusCode::NOT_IMPLEMENTED,
_ => StatusCode::BAD_REQUEST,
}
}

167
src/api/s3_router.rs
View file

@ -81,7 +81,53 @@ macro_rules! s3_match {
.parse()
.map_err(|_| Error::BadRequest("Failed to parse query parameter".to_owned()))?
}};
(@func
$(#[$doc:meta])*
pub enum Endpoint {
$(
$(#[$outer:meta])*
$variant:ident $({
bucket: String,
$($name:ident: $ty:ty,)*
})?,
)*
}) => {
$(#[$doc])*
pub enum Endpoint {
$(
$(#[$outer])*
$variant $({
bucket: String,
$($name: $ty, )*
})?,
)*
}
impl Endpoint {
pub fn name(&self) -> &'static str {
match self {
$(Endpoint::$variant $({ $($name: _,)* .. })? => stringify!($variant),)*
}
}
/// Get the bucket the request target. Returns None for requests not related to a bucket.
pub fn get_bucket(&self) -> Option<&str> {
match self {
$(
Endpoint::$variant $({ bucket, $($name: _,)* .. })? => s3_match!{@if ($(bucket $($name)*)?) then (Some(bucket)) else (None)},
)*
}
}
}
};
(@if ($($cond:tt)+) then ($($then:tt)*) else ($($else:tt)*)) => {
$($then)*
};
(@if () then ($($then:tt)*) else ($($else:tt)*)) => {
$($else)*
};
}
s3_match! {@func
/// List of all S3 API endpoints.
///
@ -318,7 +364,7 @@ pub enum Endpoint {
},
ListObjectsV2 {
bucket: String,
/// This value should always be 2. It is not checked when constructing the struct
// This value should always be 2. It is not checked when constructing the struct
list_type: String,
continuation_token: Option<String>,
delimiter: Option<char>,
@ -440,7 +486,7 @@ pub enum Endpoint {
SelectObjectContent {
bucket: String,
key: String,
/// This value should always be 2. It is not checked when constructing the struct
// This value should always be 2. It is not checked when constructing the struct
select_type: String,
},
UploadPart {
@ -455,7 +501,7 @@ pub enum Endpoint {
part_number: u64,
upload_id: String,
},
}
}}
impl Endpoint {
/// Determine which S3 endpoint a request is for using the request, and a bucket which was
@ -496,11 +542,9 @@ impl Endpoint {
};
if let Some(message) = query.nonempty_message() {
// maybe this should just be a warn! ?
Err(Error::BadRequest(message.to_owned()))
} else {
Ok(res)
debug!("Unused query parameter: {}", message)
}
Ok(res)
}
/// Determine which endpoint a request is for, knowing it is a GET.
@ -683,107 +727,6 @@ impl Endpoint {
}
}
/// Get the bucket the request target. Returns None for requests not related to a bucket.
pub fn get_bucket(&self) -> Option<&str> {
s3_match! {
@extract
self,
bucket,
[
AbortMultipartUpload,
CompleteMultipartUpload,
CopyObject,
CreateBucket,
CreateMultipartUpload,
DeleteBucket,
DeleteBucketAnalyticsConfiguration,
DeleteBucketCors,
DeleteBucketEncryption,
DeleteBucketIntelligentTieringConfiguration,
DeleteBucketInventoryConfiguration,
DeleteBucketLifecycle,
DeleteBucketMetricsConfiguration,
DeleteBucketOwnershipControls,
DeleteBucketPolicy,
DeleteBucketReplication,
DeleteBucketTagging,
DeleteBucketWebsite,
DeleteObject,
DeleteObjects,
DeleteObjectTagging,
DeletePublicAccessBlock,
GetBucketAccelerateConfiguration,
GetBucketAcl,
GetBucketAnalyticsConfiguration,
GetBucketCors,
GetBucketEncryption,
GetBucketIntelligentTieringConfiguration,
GetBucketInventoryConfiguration,
GetBucketLifecycleConfiguration,
GetBucketLocation,
GetBucketLogging,
GetBucketMetricsConfiguration,
GetBucketNotificationConfiguration,
GetBucketOwnershipControls,
GetBucketPolicy,
GetBucketPolicyStatus,
GetBucketReplication,
GetBucketRequestPayment,
GetBucketTagging,
GetBucketVersioning,
GetBucketWebsite,
GetObject,
GetObjectAcl,
GetObjectLegalHold,
GetObjectLockConfiguration,
GetObjectRetention,
GetObjectTagging,
GetObjectTorrent,
GetPublicAccessBlock,
HeadBucket,
HeadObject,
ListBucketAnalyticsConfigurations,
ListBucketIntelligentTieringConfigurations,
ListBucketInventoryConfigurations,
ListBucketMetricsConfigurations,
ListMultipartUploads,
ListObjects,
ListObjectsV2,
ListObjectVersions,
ListParts,
PutBucketAccelerateConfiguration,
PutBucketAcl,
PutBucketAnalyticsConfiguration,
PutBucketCors,
PutBucketEncryption,
PutBucketIntelligentTieringConfiguration,
PutBucketInventoryConfiguration,
PutBucketLifecycleConfiguration,
PutBucketLogging,
PutBucketMetricsConfiguration,
PutBucketNotificationConfiguration,
PutBucketOwnershipControls,
PutBucketPolicy,
PutBucketReplication,
PutBucketRequestPayment,
PutBucketTagging,
PutBucketVersioning,
PutBucketWebsite,
PutObject,
PutObjectAcl,
PutObjectLegalHold,
PutObjectLockConfiguration,
PutObjectRetention,
PutObjectTagging,
PutPublicAccessBlock,
RestoreObject,
SelectObjectContent,
UploadPart,
UploadPartCopy,
]
}
}
/// Get the key the request target. Returns None for requests which don't use a key.
#[allow(dead_code)]
pub fn get_key(&self) -> Option<&str> {
@ -929,10 +872,8 @@ macro_rules! generateQueryParameters {
}
continue;
} else {
return Err(Error::BadRequest(format!(
"Unknown query parameter '{}'",
k
)));
debug!("Received an unknown query parameter: '{}'", k);
false
}
}
};
@ -953,7 +894,7 @@ macro_rules! generateQueryParameters {
Some("Keyword not used")
} $(
else if self.$name.is_some() {
Some(concat!("Query parameter not needed: '", $rest, "'" ))
Some(concat!("'", $rest, "'"))
}
)* else {
None