add proper request router for s3 api #163

trinity-1686a · 2021-11-28T20:54:01Z

trinity-1686a commented

2021-11-28 20:54:01 +00:00

Current request router was organically grown, and is getting messier and messier with each addition.
This router cover exaustively existing API endpoints (with exceptions listed in #161(comment) either because new and old api endpoint can't feasabily be differentied, or it's more lambda than s3).

fix #161 Current request router was organically grown, and is getting messier and messier with each addition. This router cover exaustively existing API endpoints (with exceptions listed in [#161(comment)](https://git.deuxfleurs.fr/Deuxfleurs/garage/issues/161#issuecomment-1773) either because new and old api endpoint can't feasabily be differentied, or it's more lambda than s3).

❤️ 3

trinity-1686a added the

scope

s3-api

label 2021-11-28 20:54:01 +00:00

trinity-1686a force-pushed s3-router from b8f214876b to edac39409a

2021-11-28 21:02:03 +00:00

Compare

trinity-1686a force-pushed s3-router from edac39409a to dff2977561

2021-11-28 21:13:18 +00:00

Compare

trinity-1686a force-pushed s3-router from dff2977561 to 5d7b6c41b5

2021-11-28 21:48:34 +00:00

Compare

trinity-1686a force-pushed s3-router from 1b3203fdc2 to 41f83cff67

2021-11-29 23:02:49 +00:00

Compare

trinity-1686a changed title from ~~WIP: s3-router~~ to s3-router

2021-11-29 23:03:12 +00:00

trinity-1686a changed title from ~~s3-router~~ to add proper request router for s3 api

2021-11-29 23:03:39 +00:00

trinity-1686a commented

2021-11-29 23:07:46 +00:00

api_server::parse_bucket_key should probably be reverted to before it supported vhost-style bucket, as it's now used only to parse bucket name and key from headers in CopyObject.

`api_server::parse_bucket_key` should probably be reverted to before it supported vhost-style bucket, as it's now used only to parse bucket name and key from headers in CopyObject.

lx approved these changes 2021-11-30 14:08:19 +00:00

lx left a comment

Thanks trinity for this impressive PR. I'm very enthousiastic about merging it, my only worry is that we haven't checked that all of the endpoints we previously had implemented still work correctly (i.e. there are no bugs in the parser that affect the routes Garage implements). We'll talk about a testing strategy for this but we can still merge into main in the meantime.

Just a small comment, regarding s3_router.rs line 500: how does AWS S3 behave if we give additionnal parameters that are not useful for the query? If it throws an error we should keep the current code that throws an error, but if it silently ignores them we should just do that.

Thanks trinity for this impressive PR. I'm very enthousiastic about merging it, my only worry is that we haven't checked that all of the endpoints we previously had implemented still work correctly (i.e. there are no bugs in the parser that affect the routes Garage implements). We'll talk about a testing strategy for this but we can still merge into `main` in the meantime. Just a small comment, regarding `s3_router.rs` line 500: how does AWS S3 behave if we give additionnal parameters that are not useful for the query? If it throws an error we should keep the current code that throws an error, but if it silently ignores them we should just do that.

src/api/api_server.rs Outdated

					
				@ -267,0 +231,4 @@

							handle_list(

								garage,

								&ListObjectsQuery {

									is_v2: list_type == "v2",

lx commented

2021-11-30 14:01:23 +00:00

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" ?)

src/api/api_server.rs Outdated

					
				@ -267,1 +247,4 @@

						Endpoint::DeleteObjects { bucket } => {

							handle_delete_objects(garage, &bucket, req, content_sha256).await

						}

						_ => Err(Error::BadRequest("Unsupported call".to_string())),