Deuxfleurs/garage
47
56
Fork 73
Code Issues 100 Pull requests 14 Releases 30 Wiki Activity

Support for PostObject #222

Merged
lx merged 14 commits from trinity-1686a/garage:post-object into main 2022-02-21 22:02:31 +00:00
Conversation 1 Commits 14 Files changed 12 +164 -4
5 changed files with 164 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 1e9d7dc087 - Show all commits

42
Cargo.lock generated
View file

@ -469,6 +469,15 @@ version = "1.6.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "e78d4f1cc4ae33bbfc157ed5d5a5ef3bc29227303d595861deb238fcec4e9457"
[[package]]
name = "encoding_rs"
version = "0.8.30"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "7896dc8abb250ffdda33912550faa54c88ec8b998dec0b2c55ab224921ce11df"
dependencies = [
"cfg-if",
]
[[package]]
name = "env_logger"
version = "0.7.1"
@ -704,6 +713,7 @@ dependencies = [
"idna",
"log",
"md-5",
"multer",
"nom",
"percent-encoding",
"pin-project",
@ -1314,6 +1324,12 @@ dependencies = [
"autocfg",
]
[[package]]
name = "mime"
version = "0.3.16"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "2a60c7ce501c71e03a9c9c0d35b861413ae925bd979cc7a4e30d060069aaac8d"
[[package]]
name = "minimal-lexical"
version = "0.2.1"
@ -1342,6 +1358,24 @@ dependencies = [
"winapi",
]
[[package]]
name = "multer"
version = "2.0.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "5f8f35e687561d5c1667590911e6698a8cb714a134a7505718a182e7bc9d3836"
dependencies = [
"bytes 1.1.0",
"encoding_rs",
"futures-util",
"http",
"httparse",
"log",
"memchr",
"mime",
"spin 0.9.2",
"version_check",
]
[[package]]
name = "netapp"
version = "0.3.0"
@ -1670,7 +1704,7 @@ dependencies = [
"cc",
"libc",
"once_cell",
"spin",
"spin 0.5.2",
"untrusted",
"web-sys",
"winapi",
@ -1933,6 +1967,12 @@ version = "0.5.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "6e63cff320ae2c57904679ba7cb63280a3dc4613885beafb148ee7bf9aa9042d"
[[package]]
name = "spin"
version = "0.9.2"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "511254be0c5bcf062b019a6c89c01a664aa359ded62f78aa72c6fc137c0590e5"
[[package]]
name = "static_init"
version = "1.0.2"

1
src/api/Cargo.toml
View file

@ -40,6 +40,7 @@ http = "0.2"
httpdate = "0.3"
http-range = "0.1"
hyper = { version = "0.14", features = ["server", "http1", "runtime", "tcp", "stream"] }
multer = "2.0"
percent-encoding = "2.1.0"
roxmltree = "0.14"
serde = { version = "1.0", features = ["derive"] }

5
src/api/api_server.rs
View file

@ -25,6 +25,7 @@ use crate::s3_cors::*;
use crate::s3_delete::*;
use crate::s3_get::*;
use crate::s3_list::*;
use crate::s3_post_object::handle_post_object;
use crate::s3_put::*;
use crate::s3_router::{Authorization, Endpoint};
use crate::s3_website::*;
@ -111,9 +112,7 @@ async fn handler_inner(garage: Arc<Garage>, req: Request<Body>) -> Result<Respon
debug!("Endpoint: {:?}", endpoint);
if let Endpoint::PostObject {} = endpoint {
return Err(Error::NotImplemented(
"POST object is not supported yet".to_owned(),
));
return handle_post_object(garage, req, bucket_name.unwrap()).await;
}
let (api_key, content_sha256) = check_payload_signature(&garage, &req).await?;

1
src/api/lib.rs
View file

@ -19,6 +19,7 @@ pub mod s3_cors;
mod s3_delete;
pub mod s3_get;
mod s3_list;
mod s3_post_object;
mod s3_put;
mod s3_router;
mod s3_website;

119
src/api/s3_post_object.rs Normal file
View file

@ -0,0 +1,119 @@
use std::collections::HashMap;
use std::sync::Arc;
use hyper::{header, Body, Request, Response, StatusCode};
use garage_model::garage::Garage;
use crate::error::Error;
use multer::{Constraints, Multipart, SizeLimit};
pub async fn handle_post_object(
garage: Arc<Garage>,
req: Request<Body>,
bucket: String,
) -> Result<Response<Body>, Error> {
let boundary = req
.headers()
.get(header::CONTENT_TYPE)
.and_then(|ct| ct.to_str().ok())
.and_then(|ct| multer::parse_boundary(ct).ok())
.ok_or_else(|| Error::BadRequest("Counld not get multipart boundary".to_owned()))?;
// these limits are rather arbitrary
let constraints = Constraints::new().size_limit(
SizeLimit::new()
.per_field(32 * 1024)
.for_field("file", 5 * 1024 * 1024 * 1024),
);
let mut multipart = Multipart::with_constraints(req.into_body(), boundary, constraints);
let mut headers = HashMap::new();
let mut key_id = None;
let mut key = None;
let mut policy = None;
let mut redirect = Err(204);
while let Some(mut field) = multipart.next_field().await.unwrap() {
let name = if let Some(name) = field.name() {
name.to_owned()
} else {
continue;
};
if name != "file" {
let content = field.text().await.unwrap();
trinity-1686a marked this conversation as resolved Outdated
lx commented 2022-02-08 22:05:42 +00:00
Outdated
Review
Copy link

I think we can avoid putting almost the entire code of this function in the while loop (and remove 1 indentation level almost everywhere) by doing something like this:

let file = loop {
    let field = match multipart.next_field().await? {
        None => return Err(no file field found),
        Some(x) => x,
    };
    let name = ...;
    if name == "file" {
         break field;
    }
    // here handle header field adding it to the headermap
};
// here do all of the rest of the processing once we have all headers and are now reading the file body

This looks much nicer to me, especially as in the current version we have a for inside the while, which looks a bit like a nested loop but is in fact not at all.

I think we can avoid putting almost the entire code of this function in the `while` loop (and remove 1 indentation level almost everywhere) by doing something like this: ```rust let file = loop { let field = match multipart.next_field().await? { None => return Err(no file field found), Some(x) => x, }; let name = ...; if name == "file" { break field; } // here handle header field adding it to the headermap }; // here do all of the rest of the processing once we have all headers and are now reading the file body ``` This looks much nicer to me, especially as in the current version we have a `for` inside the `while`, which looks a bit like a nested loop but is in fact not at all.
match name.as_str() {
// main fields
"AWSAccessKeyId" => {
key_id = Some(content);
}
"key" => {
key = Some(content);
}
"policy" => {
policy = Some(content);
}
// special handling
"success_action_redirect" | "redirect" => {
// TODO should verify it's a valid looking URI
redirect = Ok(content);
}
"success_action_status" => {
let code = name.parse::<u16>().unwrap_or(204);
redirect = Err(code);
}
"tagging" => {
// TODO Garage does not support tagging so this can be left empty. It's essentially
// a header except it must be parsed from xml to x-www-form-urlencoded
continue;
}
// headers to PutObject
"acl" | "Cache-Control" | "Content-Type" | "Content-Encoding" | "Expires" => {
headers.insert(name, content);
}
_ if name.starts_with("x-amz-") => {
headers.insert(name, content);
}
_ => {
// TODO should we ignore or error?
}
}
continue;
}
let _file_name = field.file_name();
let _content_type = field.content_type();
while let Some(_chunk) = field.chunk().await.unwrap() {}
let resp = match redirect {
Err(200) => Response::builder()
.status(StatusCode::OK)
.body(Body::empty())?,
Err(201) => {
// body should be an XML document, not sure which yet
Response::builder()
.status(StatusCode::CREATED)
.body(todo!())?
}
// invalid codes are handled as 204
Err(_) => Response::builder()
.status(StatusCode::NO_CONTENT)
.body(Body::empty())?,
Ok(uri) => {
// TODO maybe body should contain a link to the ressource?
Response::builder()
.status(StatusCode::SEE_OTHER)
.header(header::LOCATION, uri)
.body(Body::empty())?
}
};
return Ok(resp);
}
trinity-1686a marked this conversation as resolved Outdated
lx commented 2022-02-14 16:24:43 +00:00
Outdated
Review
Copy link

This looks like it should rather be an error case: if the application builder put ${filename} in the key field but the browser for some reason didn't include the name of the uploaded file, we probably want to reject the request.

This looks like it should rather be an error case: if the application builder put `${filename}` in the key field but the browser for some reason didn't include the name of the uploaded file, we probably want to reject the request.
trinity-1686a commented 2022-02-15 17:14:31 +00:00
Outdated
Review
Copy link

that was actually how a previous iteration worked. This behavior was added because it's exactly how AWS behave (not that I can say I agree with that behavior)

that was actually how a previous iteration worked. This behavior was added because it's exactly how AWS behave (not that I can say I agree with that behavior)
lx commented 2022-02-15 17:33:52 +00:00
Outdated
Review
Copy link

:/

:/
return Err(Error::BadRequest(
"Request did not contain a file".to_owned(),
));
}