From 3ea8ca1b9e8d0eb09d4d47869da1aca4c8dadade Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Thu, 6 Jan 2022 11:36:17 +0100 Subject: [PATCH 1/4] Implement GetBucketWebsite --- src/api/api_server.rs | 1 + src/api/s3_website.rs | 73 ++++++++++++++++++++++++++++++++----------- 2 files changed, 55 insertions(+), 19 deletions(-) diff --git a/src/api/api_server.rs b/src/api/api_server.rs index a38a3c5b..ea1990d9 100644 --- a/src/api/api_server.rs +++ b/src/api/api_server.rs @@ -305,6 +305,7 @@ async fn handler_inner(garage: Arc, req: Request) -> Result { handle_delete_objects(garage, bucket_id, req, content_sha256).await } + Endpoint::GetBucketWebsite { .. } => handle_get_website(garage, bucket_id).await, Endpoint::PutBucketWebsite { .. } => { handle_put_website(garage, bucket_id, req, content_sha256).await } diff --git a/src/api/s3_website.rs b/src/api/s3_website.rs index 85d7c261..fcf8cba3 100644 --- a/src/api/s3_website.rs +++ b/src/api/s3_website.rs @@ -11,9 +11,46 @@ use crate::signature::verify_signed_content; use garage_model::bucket_table::*; use garage_model::garage::Garage; use garage_table::*; -use garage_util::crdt; use garage_util::data::*; +pub async fn handle_get_website( + garage: Arc, + bucket_id: Uuid, +) -> Result, Error> { + let bucket = garage + .bucket_table + .get(&EmptyKey, &bucket_id) + .await? + .ok_or(Error::NoSuchBucket)?; + + let param = bucket + .params() + .ok_or_internal_error("Bucket should not be deleted at this point")?; + + if let Some(website) = param.website_config.get() { + let wc = WebsiteConfiguration { + xmlns: (), + error_document: website.error_document.as_ref().map(|v| Key { + key: Value(v.to_string()), + }), + index_document: Some(Suffix { + suffix: Value(website.index_document.to_string()), + }), + redirect_all_requests_to: None, + routing_rules: None, + }; + let xml = quick_xml::se::to_string(&wc)?; + Ok(Response::builder() + .status(StatusCode::OK) + .header(http::header::CONTENT_TYPE, "application/xml") + .body(Body::from(xml))?) + } else { + Ok(Response::builder() + .status(StatusCode::NO_CONTENT) + .body(Body::empty())?) + } +} + pub async fn handle_delete_website( garage: Arc, bucket_id: Uuid, @@ -24,17 +61,16 @@ pub async fn handle_delete_website( .await? .ok_or(Error::NoSuchBucket)?; - if let crdt::Deletable::Present(param) = &mut bucket.state { - param.website_config.update(None); - garage.bucket_table.insert(&bucket).await?; - } else { - unreachable!(); - } + let param = bucket + .params_mut() + .ok_or_internal_error("Bucket should not be deleted at this point")?; + + param.website_config.update(None); + garage.bucket_table.insert(&bucket).await?; Ok(Response::builder() .status(StatusCode::NO_CONTENT) - .body(Body::from(vec![])) - .unwrap()) + .body(Body::empty())?) } pub async fn handle_put_website( @@ -52,22 +88,21 @@ pub async fn handle_put_website( .await? .ok_or(Error::NoSuchBucket)?; + let param = bucket + .params_mut() + .ok_or_internal_error("Bucket should not be deleted at this point")?; + let conf: WebsiteConfiguration = from_reader(&body as &[u8])?; conf.validate()?; - if let crdt::Deletable::Present(param) = &mut bucket.state { - param - .website_config - .update(Some(conf.into_garage_website_config()?)); - garage.bucket_table.insert(&bucket).await?; - } else { - unreachable!(); - } + param + .website_config + .update(Some(conf.into_garage_website_config()?)); + garage.bucket_table.insert(&bucket).await?; Ok(Response::builder() .status(StatusCode::OK) - .body(Body::from(vec![])) - .unwrap()) + .body(Body::empty())?) } #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)] -- 2.45.2 From 9eb211948ec54430170e981201fe33b3997e2da5 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Thu, 6 Jan 2022 12:58:21 +0100 Subject: [PATCH 2/4] Allow setting index document and error document on the CLI --- src/garage/admin.rs | 4 ++-- src/garage/cli/structs.rs | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/garage/admin.rs b/src/garage/admin.rs index 4a53792d..f9ec1593 100644 --- a/src/garage/admin.rs +++ b/src/garage/admin.rs @@ -404,8 +404,8 @@ impl AdminRpcHandler { let website = if query.allow { Some(WebsiteConfig { - index_document: "index.html".into(), - error_document: None, + index_document: query.index_document.clone(), + error_document: query.error_document.clone(), }) } else { None diff --git a/src/garage/cli/structs.rs b/src/garage/cli/structs.rs index a544d6a1..30cbb2da 100644 --- a/src/garage/cli/structs.rs +++ b/src/garage/cli/structs.rs @@ -188,6 +188,14 @@ pub struct WebsiteOpt { /// Bucket name pub bucket: String, + + /// Index document: the suffix appended to request paths ending by / + #[structopt(short = "i", long = "index-document", default_value = "index.html")] + pub index_document: String, + + /// Error document: the optionnal document returned when an error occurs + #[structopt(short = "e", long = "error-document")] + pub error_document: Option, } #[derive(Serialize, Deserialize, StructOpt, Debug)] -- 2.45.2 From d4dd2e2640af9290717b669c923d1ca75dba1bde Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Thu, 6 Jan 2022 12:55:49 +0100 Subject: [PATCH 3/4] Make use of website config, return error document on error --- src/api/lib.rs | 2 +- src/util/config.rs | 2 - src/web/web_server.rs | 128 +++++++++++++++++++++++++++++++++--------- 3 files changed, 103 insertions(+), 29 deletions(-) diff --git a/src/api/lib.rs b/src/api/lib.rs index 589ffe9f..725cd9d1 100644 --- a/src/api/lib.rs +++ b/src/api/lib.rs @@ -2,7 +2,7 @@ #[macro_use] extern crate log; -mod error; +pub mod error; pub use error::Error; mod encoding; diff --git a/src/util/config.rs b/src/util/config.rs index 61a749e3..f1f4b06a 100644 --- a/src/util/config.rs +++ b/src/util/config.rs @@ -87,8 +87,6 @@ pub struct WebConfig { pub bind_addr: SocketAddr, /// Suffix to remove from domain name to find bucket pub root_domain: String, - /// Suffix to add when user-agent request path end with "/" - pub index: String, } fn default_sled_cache_capacity() -> u64 { diff --git a/src/web/web_server.rs b/src/web/web_server.rs index 49e5f21b..834f31e8 100644 --- a/src/web/web_server.rs +++ b/src/web/web_server.rs @@ -3,7 +3,7 @@ use std::{borrow::Cow, convert::Infallible, net::SocketAddr, sync::Arc}; use futures::future::Future; use hyper::{ - header::HOST, + header::{HeaderValue, HOST}, server::conn::AddrStream, service::{make_service_fn, service_fn}, Body, Method, Request, Response, Server, @@ -11,10 +11,10 @@ use hyper::{ use crate::error::*; +use garage_api::error::{Error as ApiError, OkOrBadRequest}; use garage_api::helpers::{authority_to_host, host_to_bucket}; use garage_api::s3_get::{handle_get, handle_head}; -use garage_model::bucket_table::Bucket; use garage_model::garage::Garage; use garage_table::*; @@ -52,29 +52,45 @@ async fn handle_request( addr: SocketAddr, ) -> Result, Infallible> { info!("{} {} {}", addr, req.method(), req.uri()); - let res = serve_file(garage, req).await; - match &res { - Ok(r) => debug!("{} {:?}", r.status(), r.headers()), - Err(e) => warn!("Response: error {}, {}", e.http_status_code(), e), + match serve_file(garage, &req).await { + Ok(res) => { + debug!("{} {} {}", req.method(), req.uri(), res.status()); + Ok(res) + } + Err(error) => { + info!( + "{} {} {} {}", + req.method(), + req.uri(), + error.http_status_code(), + error + ); + Ok(error_to_res(error)) + } } - - Ok(res.unwrap_or_else(error_to_res)) } fn error_to_res(e: Error) -> Response { - let body: Body = Body::from(format!("{}\n", e)); + // If we are here, it is either that: + // - there was an error before trying to get the requested URL + // from the bucket (e.g. bucket not found) + // - there was an error processing the request and (the request + // was a HEAD request or we couldn't get the error document) + // We do NOT enter this code path when returning the bucket's + // error document (this is handled in serve_file) + let body = Body::from(format!("{}\n", e)); let mut http_error = Response::new(body); *http_error.status_mut() = e.http_status_code(); e.add_headers(http_error.headers_mut()); http_error } -async fn serve_file(garage: Arc, req: Request) -> Result, Error> { +async fn serve_file(garage: Arc, req: &Request) -> Result, Error> { // Get http authority string (eg. [::1]:3902 or garage.tld:80) let authority = req .headers() .get(HOST) - .ok_or_else(|| Error::BadRequest("HOST header required".to_owned()))? + .ok_or_bad_request("HOST header required")? .to_str()?; // Get bucket @@ -91,35 +107,95 @@ async fn serve_file(garage: Arc, req: Request) -> Result handle_head(garage, &req, bucket_id, &key).await?, - Method::GET => handle_get(garage, &req, bucket_id, &key).await?, - _ => return Err(Error::BadRequest("HTTP method not supported".to_string())), - }; + let ret_doc = match *req.method() { + Method::HEAD => handle_head(garage.clone(), req, bucket_id, &key).await, + Method::GET => handle_get(garage.clone(), req, bucket_id, &key).await, + _ => Err(ApiError::BadRequest("HTTP method not supported".into())), + } + .map_err(Error::from); - Ok(res) + if let Err(error) = ret_doc { + if *req.method() == Method::HEAD || !error.http_status_code().is_client_error() { + // Do not return the error document in the following cases: + // - the error is not a 4xx error code + // - the request is a HEAD method + // In this case we just return the error code and the error message in the body, + // by relying on err_to_res that is called above when we return an Err. + return Err(error); + } + + // Same if no error document is set: just return the error directly + let error_document = match &website_config.error_document { + Some(ed) => ed.trim_start_matches('/').to_owned(), + None => return Err(error), + }; + + // We want to return the error document + // Create a fake HTTP request with path = the error document + let req2 = Request::builder() + .uri(format!("http://{}/{}", host, &error_document)) + .body(Body::empty()) + .unwrap(); + + match handle_get(garage, &req2, bucket_id, &error_document).await { + Ok(mut error_doc) => { + // The error won't be logged back in handle_request, + // so log it here + info!( + "{} {} {} {}", + req.method(), + req.uri(), + error.http_status_code(), + error + ); + + *error_doc.status_mut() = error.http_status_code(); + error.add_headers(error_doc.headers_mut()); + + // Preserve error message in a special header + for error_line in error.to_string().split('\n') { + if let Ok(v) = HeaderValue::from_bytes(error_line.as_bytes()) { + error_doc.headers_mut().append("X-Garage-Error", v); + } + } + + Ok(error_doc) + } + Err(error_doc_error) => { + warn!( + "Couldn't get error document {} for bucket {:?}: {}", + error_document, bucket_id, error_doc_error + ); + Err(error) + } + } + } else { + ret_doc + } } /// Path to key -- 2.45.2 From 60c0033c8bbd3dbc18f8c91f15674a60fd8acdc0 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Fri, 7 Jan 2022 17:13:16 +0100 Subject: [PATCH 4/4] Update documentation --- doc/book/src/reference_manual/s3_compatibility.md | 5 ++--- doc/book/src/working_documents/compatibility_target.md | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/doc/book/src/reference_manual/s3_compatibility.md b/doc/book/src/reference_manual/s3_compatibility.md index 96bccde0..5c2e6315 100644 --- a/doc/book/src/reference_manual/s3_compatibility.md +++ b/doc/book/src/reference_manual/s3_compatibility.md @@ -36,7 +36,7 @@ All APIs that are not mentionned are not implemented and will return a 501 Not I | DeleteObjects | Implemented | | GetBucketLocation | Implemented | | GetBucketVersioning | Stub (see below) | -| GetBucketWebsite | Unsupported | +| GetBucketWebsite | Implemented | | GetObject | Implemented | | HeadBucket | Implemented | | HeadObject | Implemented | @@ -56,6 +56,5 @@ All APIs that are not mentionned are not implemented and will return a 501 Not I - **ListObjects:** Implemented, but there isn't a very good specification of what `encoding-type=url` covers so there might be some encoding bugs. In our implementation the url-encoded fields are in the same in ListObjects as they are in ListObjectsV2. -- **PutBucketWebsite:** Implemented, but only store if website is enabled, not more complexe informations. +- **PutBucketWebsite:** Implemented, but only stores the index document suffix and the error document path. Redirects are not supported. -- **GetBucketWebsite:** Not implemented yet, will be when PubBucketWebsite store more informations. diff --git a/doc/book/src/working_documents/compatibility_target.md b/doc/book/src/working_documents/compatibility_target.md index e6ff9ca8..8830101f 100644 --- a/doc/book/src/working_documents/compatibility_target.md +++ b/doc/book/src/working_documents/compatibility_target.md @@ -31,8 +31,8 @@ your motivations for doing so in the PR message. | | [*PutBucketCors*](https://git.deuxfleurs.fr/Deuxfleurs/garage/issues/138) | | | [*DeleteBucketCors*](https://git.deuxfleurs.fr/Deuxfleurs/garage/issues/138) | | | UploadPartCopy | -| | [*GetBucketWebsite*](https://git.deuxfleurs.fr/Deuxfleurs/garage/issues/77) | -| | [*PutBucketWebsite*](https://git.deuxfleurs.fr/Deuxfleurs/garage/issues/77) | +| | GetBucketWebsite | +| | PutBucketWebsite | | | DeleteBucketWebsite | | ~~~~~~~~~~~~~~~~~~~~~~~~~~ | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | **B-tier** | | -- 2.45.2