From 65e9dde8c99a4c1406e58da0741c9e566d18b1ff Mon Sep 17 00:00:00 2001 From: trinity-1686a Date: Sun, 22 Dec 2024 15:20:09 +0100 Subject: [PATCH] add tests --- src/api/s3/website.rs | 130 ++++++---- src/garage/tests/s3/website.rs | 446 ++++++++++++++++++++++++++++++++- src/web/web_server.rs | 11 +- 3 files changed, 538 insertions(+), 49 deletions(-) diff --git a/src/api/s3/website.rs b/src/api/s3/website.rs index 934a20ff..ad592260 100644 --- a/src/api/s3/website.rs +++ b/src/api/s3/website.rs @@ -25,8 +25,28 @@ pub async fn handle_get_website(ctx: ReqCtx) -> Result, Error> suffix: Value(website.index_document.to_string()), }), redirect_all_requests_to: None, - // TODO put the correct config here - routing_rules: Vec::new(), + routing_rules: RoutingRules { + rules: website + .routing_rules + .clone() + .into_iter() + .map(|rule| RoutingRule { + condition: rule.condition.map(|cond| Condition { + http_error_code: cond.http_error_code.map(|c| IntValue(c as i64)), + prefix: cond.prefix.map(Value), + }), + redirect: Redirect { + hostname: rule.redirect.hostname.map(Value), + http_redirect_code: Some(IntValue( + rule.redirect.http_redirect_code as i64, + )), + protocol: rule.redirect.protocol.map(Value), + replace_full: rule.redirect.replace_key.map(Value), + replace_prefix: rule.redirect.replace_key_prefix.map(Value), + }, + }) + .collect(), + }, }; let xml = to_xml_with_header(&wc)?; Ok(Response::builder() @@ -105,19 +125,25 @@ pub struct WebsiteConfiguration { #[serde( rename = "RoutingRules", default, - skip_serializing_if = "Vec::is_empty" + skip_serializing_if = "RoutingRules::is_empty" )] - pub routing_rules: Vec, + pub routing_rules: RoutingRules, +} + +#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord, Default)] +pub struct RoutingRules { + #[serde(rename = "RoutingRule")] + pub rules: Vec, +} + +impl RoutingRules { + fn is_empty(&self) -> bool { + self.rules.is_empty() + } } #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)] pub struct RoutingRule { - #[serde(rename = "RoutingRule")] - pub inner: RoutingRuleInner, -} - -#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)] -pub struct RoutingRuleInner { #[serde(rename = "Condition")] pub condition: Option, #[serde(rename = "Redirect")] @@ -186,10 +212,10 @@ impl WebsiteConfiguration { if let Some(ref rart) = self.redirect_all_requests_to { rart.validate()?; } - for rr in &self.routing_rules { - rr.inner.validate()?; + for rr in &self.routing_rules.rules { + rr.validate()?; } - if self.routing_rules.len() > 1000 { + if self.routing_rules.rules.len() > 1000 { // we will do linear scans, best to avoid overly long configuration. The // limit was choosen arbitrarily return Err(Error::bad_request( @@ -205,12 +231,6 @@ impl WebsiteConfiguration { Err(Error::NotImplemented( "S3 website redirects are not currently implemented in Garage.".into(), )) - /* - } else if self.routing_rules.map(|x| !x.is_empty()).unwrap_or(false) { - Err(Error::NotImplemented( - "S3 routing rules are not currently implemented in Garage.".into(), - )) - */ } else { Ok(WebsiteConfig { index_document: self @@ -220,29 +240,27 @@ impl WebsiteConfiguration { error_document: self.error_document.map(|x| x.key.0), routing_rules: self .routing_rules + .rules .into_iter() .map(|rule| { bucket_table::RoutingRule { - condition: rule.inner.condition.map(|condition| { - bucket_table::Condition { - http_error_code: condition.http_error_code.map(|c| c.0 as u16), - prefix: condition.prefix.map(|p| p.0), - } + condition: rule.condition.map(|condition| bucket_table::Condition { + http_error_code: condition.http_error_code.map(|c| c.0 as u16), + prefix: condition.prefix.map(|p| p.0), }), redirect: bucket_table::Redirect { - hostname: rule.inner.redirect.hostname.map(|h| h.0), - protocol: rule.inner.redirect.protocol.map(|p| p.0), + hostname: rule.redirect.hostname.map(|h| h.0), + protocol: rule.redirect.protocol.map(|p| p.0), // aws default to 301, which i find punitive in case of // missconfiguration (can be permanently cached on the // user agent) http_redirect_code: rule - .inner .redirect .http_redirect_code .map(|c| c.0 as u16) .unwrap_or(302), - replace_key_prefix: rule.inner.redirect.replace_prefix.map(|k| k.0), - replace_key: rule.inner.redirect.replace_full.map(|k| k.0), + replace_key_prefix: rule.redirect.replace_prefix.map(|k| k.0), + replace_key: rule.redirect.replace_full.map(|k| k.0), }, } }) @@ -287,7 +305,7 @@ impl Target { } } -impl RoutingRuleInner { +impl RoutingRule { pub fn validate(&self) -> Result<(), Error> { if let Some(condition) = &self.condition { condition.validate()?; @@ -390,6 +408,15 @@ mod tests { fullkey + + + + + + 404 + missing + + "#; let conf: WebsiteConfiguration = from_str(message).unwrap(); @@ -405,21 +432,36 @@ mod tests { hostname: Value("garage.tld".to_owned()), protocol: Some(Value("https".to_owned())), }), - routing_rules: vec![RoutingRule { - inner: RoutingRuleInner { - condition: Some(Condition { - http_error_code: Some(IntValue(404)), - prefix: Some(Value("prefix1".to_owned())), - }), - redirect: Redirect { - hostname: Some(Value("gara.ge".to_owned())), - protocol: Some(Value("http".to_owned())), - http_redirect_code: Some(IntValue(303)), - replace_prefix: Some(Value("prefix2".to_owned())), - replace_full: Some(Value("fullkey".to_owned())), + routing_rules: RoutingRules { + rules: vec![ + RoutingRule { + condition: Some(Condition { + http_error_code: Some(IntValue(404)), + prefix: Some(Value("prefix1".to_owned())), + }), + redirect: Redirect { + hostname: Some(Value("gara.ge".to_owned())), + protocol: Some(Value("http".to_owned())), + http_redirect_code: Some(IntValue(303)), + replace_prefix: Some(Value("prefix2".to_owned())), + replace_full: Some(Value("fullkey".to_owned())), + }, }, - }, - }], + RoutingRule { + condition: Some(Condition { + http_error_code: None, + prefix: Some(Value("".to_owned())), + }), + redirect: Redirect { + hostname: None, + protocol: None, + http_redirect_code: Some(IntValue(404)), + replace_prefix: None, + replace_full: Some(Value("missing".to_owned())), + }, + }, + ], + }, }; assert_eq! { ref_value, diff --git a/src/garage/tests/s3/website.rs b/src/garage/tests/s3/website.rs index 0cadc388..12d4973b 100644 --- a/src/garage/tests/s3/website.rs +++ b/src/garage/tests/s3/website.rs @@ -5,7 +5,10 @@ use crate::json_body; use assert_json_diff::assert_json_eq; use aws_sdk_s3::{ primitives::ByteStream, - types::{CorsConfiguration, CorsRule, ErrorDocument, IndexDocument, WebsiteConfiguration}, + types::{ + Condition, CorsConfiguration, CorsRule, ErrorDocument, IndexDocument, Protocol, Redirect, + RoutingRule, WebsiteConfiguration, + }, }; use http::{Request, StatusCode}; use http_body_util::BodyExt; @@ -505,3 +508,444 @@ async fn test_website_check_domain() { }) ); } + +#[tokio::test] +async fn test_website_redirect_full_bucket() { + const BCKT_NAME: &str = "my-redirect-full"; + let ctx = common::context(); + let bucket = ctx.create_bucket(BCKT_NAME); + + let conf = WebsiteConfiguration::builder() + .routing_rules( + RoutingRule::builder() + .condition(Condition::builder().key_prefix_equals("").build()) + .redirect( + Redirect::builder() + .protocol(Protocol::Https) + .host_name("other.tld") + .replace_key_prefix_with("") + .build(), + ) + .build(), + ) + .build(); + + ctx.client + .put_bucket_website() + .bucket(&bucket) + .website_configuration(conf) + .send() + .await + .unwrap(); + + let req = Request::builder() + .method("GET") + .uri(format!("http://127.0.0.1:{}/my-path", ctx.garage.web_port)) + .header("Host", format!("{}.web.garage", BCKT_NAME)) + .body(Body::new(Bytes::new())) + .unwrap(); + + let client = Client::builder(TokioExecutor::new()).build_http(); + let resp = client.request(req).await.unwrap(); + assert_eq!(resp.status(), StatusCode::FOUND); + assert_eq!( + resp.headers() + .get(hyper::header::LOCATION) + .unwrap() + .to_str() + .unwrap(), + "https://other.tld/my-path" + ); +} + +#[tokio::test] +async fn test_website_redirect() { + const BCKT_NAME: &str = "my-redirect"; + let ctx = common::context(); + let bucket = ctx.create_bucket(BCKT_NAME); + + ctx.client + .put_object() + .bucket(&bucket) + .key("index.html") + .body(ByteStream::from_static(b"index")) + .send() + .await + .unwrap(); + ctx.client + .put_object() + .bucket(&bucket) + .key("404.html") + .body(ByteStream::from_static(b"main 404")) + .send() + .await + .unwrap(); + ctx.client + .put_object() + .bucket(&bucket) + .key("static-file") + .body(ByteStream::from_static(b"static file")) + .send() + .await + .unwrap(); + + let mut conf = WebsiteConfiguration::builder() + .index_document( + IndexDocument::builder() + .suffix("home.html") + .build() + .unwrap(), + ) + .error_document(ErrorDocument::builder().key("404.html").build().unwrap()); + + for (prefix, condition) in [("unconditional", false), ("conditional", true)] { + let code = condition.then(|| "404".to_string()); + conf = conf + // simple redirect + .routing_rules( + RoutingRule::builder() + .condition( + Condition::builder() + .set_http_error_code_returned_equals(code.clone()) + .key_prefix_equals(format!("{prefix}/redirect-prefix/")) + .build(), + ) + .redirect( + Redirect::builder() + .http_redirect_code("302") + .replace_key_prefix_with("other-prefix/") + .build(), + ) + .build(), + ) + .routing_rules( + RoutingRule::builder() + .condition( + Condition::builder() + .set_http_error_code_returned_equals(code.clone()) + .key_prefix_equals(format!("{prefix}/redirect-prefix-307/")) + .build(), + ) + .redirect( + Redirect::builder() + .http_redirect_code("307") + .replace_key_prefix_with("other-prefix/") + .build(), + ) + .build(), + ) + // simple redirect + .routing_rules( + RoutingRule::builder() + .condition( + Condition::builder() + .set_http_error_code_returned_equals(code.clone()) + .key_prefix_equals(format!("{prefix}/redirect-fixed/")) + .build(), + ) + .redirect( + Redirect::builder() + .http_redirect_code("302") + .replace_key_with("fixed_key") + .build(), + ) + .build(), + ) + // stream other file + .routing_rules( + RoutingRule::builder() + .condition( + Condition::builder() + .set_http_error_code_returned_equals(code.clone()) + .key_prefix_equals(format!("{prefix}/stream-fixed/")) + .build(), + ) + .redirect( + Redirect::builder() + .http_redirect_code("200") + .replace_key_with("static-file") + .build(), + ) + .build(), + ) + // stream other file as error + .routing_rules( + RoutingRule::builder() + .condition( + Condition::builder() + .set_http_error_code_returned_equals(code.clone()) + .key_prefix_equals(format!("{prefix}/stream-404/")) + .build(), + ) + .redirect( + Redirect::builder() + .http_redirect_code("404") + .replace_key_with("static-file") + .build(), + ) + .build(), + ) + // fail to stream other file + .routing_rules( + RoutingRule::builder() + .condition( + Condition::builder() + .set_http_error_code_returned_equals(code.clone()) + .key_prefix_equals(format!("{prefix}/stream-missing/")) + .build(), + ) + .redirect( + Redirect::builder() + .http_redirect_code("200") + .replace_key_with("missing-file") + .build(), + ) + .build(), + ); + } + let conf = conf.build(); + + ctx.client + .put_bucket_website() + .bucket(&bucket) + .website_configuration(conf.clone()) + .send() + .await + .unwrap(); + + let stored_cfg = ctx + .client + .get_bucket_website() + .bucket(&bucket) + .send() + .await + .unwrap(); + assert_eq!(stored_cfg.index_document, conf.index_document); + assert_eq!(stored_cfg.error_document, conf.error_document); + assert_eq!(stored_cfg.routing_rules, conf.routing_rules); + + let req = |path| { + Request::builder() + .method("GET") + .uri(format!( + "http://127.0.0.1:{}/{}/path", + ctx.garage.web_port, path + )) + .header("Host", format!("{}.web.garage", BCKT_NAME)) + .body(Body::new(Bytes::new())) + .unwrap() + }; + + test_redirect_helper("unconditional", true, &req).await; + test_redirect_helper("conditional", true, &req).await; + for prefix in ["unconditional", "conditional"] { + for rule_path in [ + "redirect-prefix", + "redirect-prefix-307", + "redirect-fixed", + "stream-fixed", + "stream-404", + "stream-missing", + ] { + ctx.client + .put_object() + .bucket(&bucket) + .key(format!("{prefix}/{rule_path}/path")) + .body(ByteStream::from_static(b"i exist")) + .send() + .await + .unwrap(); + } + } + test_redirect_helper("unconditional", true, &req).await; + test_redirect_helper("conditional", false, &req).await; +} + +async fn test_redirect_helper( + prefix: &str, + should_see_redirect: bool, + req: impl Fn(String) -> Request>, +) { + use http::header; + let client = Client::builder(TokioExecutor::new()).build_http(); + let expected_body = b"i exist".as_ref(); + + let resp = client + .request(req(format!("{prefix}/redirect-prefix"))) + .await + .unwrap(); + if should_see_redirect { + assert_eq!(resp.status(), StatusCode::FOUND); + assert_eq!( + resp.headers() + .get(header::LOCATION) + .unwrap() + .to_str() + .unwrap(), + "/other-prefix/path" + ); + assert!(resp + .into_body() + .collect() + .await + .unwrap() + .to_bytes() + .is_empty()); + } else { + assert_eq!(resp.status(), StatusCode::OK); + assert!(resp.headers().get(header::LOCATION).is_none()); + assert_eq!( + resp.into_body().collect().await.unwrap().to_bytes(), + expected_body, + ); + } + + let resp = client + .request(req(format!("{prefix}/redirect-prefix-307"))) + .await + .unwrap(); + if should_see_redirect { + assert_eq!(resp.status(), StatusCode::TEMPORARY_REDIRECT); + assert_eq!( + resp.headers() + .get(header::LOCATION) + .unwrap() + .to_str() + .unwrap(), + "/other-prefix/path" + ); + assert!(resp + .into_body() + .collect() + .await + .unwrap() + .to_bytes() + .is_empty()); + } else { + assert_eq!(resp.status(), StatusCode::OK); + assert!(resp.headers().get(header::LOCATION).is_none()); + assert_eq!( + resp.into_body().collect().await.unwrap().to_bytes(), + expected_body, + ); + } + + let resp = client + .request(req(format!("{prefix}/redirect-fixed"))) + .await + .unwrap(); + if should_see_redirect { + assert_eq!(resp.status(), StatusCode::FOUND); + assert_eq!( + resp.headers() + .get(header::LOCATION) + .unwrap() + .to_str() + .unwrap(), + "/fixed_key" + ); + assert!(resp + .into_body() + .collect() + .await + .unwrap() + .to_bytes() + .is_empty()); + } else { + assert_eq!(resp.status(), StatusCode::OK); + assert!(resp.headers().get(header::LOCATION).is_none()); + assert_eq!( + resp.into_body().collect().await.unwrap().to_bytes(), + expected_body, + ); + } + let resp = client + .request(req(format!("{prefix}/stream-fixed"))) + .await + .unwrap(); + if should_see_redirect { + assert_eq!(resp.status(), StatusCode::OK); + assert!(resp.headers().get(header::LOCATION).is_none()); + assert_eq!( + resp.into_body().collect().await.unwrap().to_bytes(), + b"static file".as_ref(), + ); + } else { + assert_eq!(resp.status(), StatusCode::OK); + assert!(resp.headers().get(header::LOCATION).is_none()); + assert_eq!( + resp.into_body().collect().await.unwrap().to_bytes(), + expected_body, + ); + } + let resp = client + .request(req(format!("{prefix}/stream-404"))) + .await + .unwrap(); + if should_see_redirect { + assert_eq!(resp.status(), StatusCode::NOT_FOUND); + assert!(resp.headers().get(header::LOCATION).is_none()); + assert_eq!( + resp.into_body().collect().await.unwrap().to_bytes(), + b"static file".as_ref(), + ); + } else { + assert_eq!(resp.status(), StatusCode::OK); + assert!(resp.headers().get(header::LOCATION).is_none()); + assert_eq!( + resp.into_body().collect().await.unwrap().to_bytes(), + expected_body, + ); + } + let resp = client + .request(req(format!("{prefix}/stream-404"))) + .await + .unwrap(); + if should_see_redirect { + assert_eq!(resp.status(), StatusCode::NOT_FOUND); + assert!(resp.headers().get(header::LOCATION).is_none()); + assert_eq!( + resp.into_body().collect().await.unwrap().to_bytes(), + b"static file".as_ref(), + ); + } else { + assert_eq!(resp.status(), StatusCode::OK); + assert!(resp.headers().get(header::LOCATION).is_none()); + assert_eq!( + resp.into_body().collect().await.unwrap().to_bytes(), + expected_body, + ); + } +} + +#[tokio::test] +async fn test_website_invalid_redirect() { + const BCKT_NAME: &str = "my-invalid-redirect"; + let ctx = common::context(); + let bucket = ctx.create_bucket(BCKT_NAME); + + let conf = WebsiteConfiguration::builder() + .routing_rules( + RoutingRule::builder() + .condition(Condition::builder().key_prefix_equals("").build()) + .redirect( + Redirect::builder() + .protocol(Protocol::Https) + .host_name("other.tld") + .replace_key_prefix_with("") + // we don't allow 200 with hostname + .http_redirect_code("200") + .build(), + ) + .build(), + ) + .build(); + + ctx.client + .put_bucket_website() + .bucket(&bucket) + .website_configuration(conf) + .send() + .await + .unwrap_err(); +} diff --git a/src/web/web_server.rs b/src/web/web_server.rs index d8ce0460..dfdbf8a2 100644 --- a/src/web/web_server.rs +++ b/src/web/web_server.rs @@ -235,7 +235,7 @@ impl WebServer { // Get path let path = req.uri().path().to_string(); let index = &website_config.index_document; - let routing_result = path_to_keys(&path, index, &[])?; + let routing_result = path_to_keys(&path, index, &website_config.routing_rules)?; debug!( "Selected bucket: \"{}\" {:?}, routing to {:?}", @@ -518,7 +518,7 @@ fn path_to_keys<'a>( } else { None }; - let target = routing_rule.redirect.compute_target(suffix); + let mut target = routing_rule.redirect.compute_target(suffix); let query_alternative_key = status_code == StatusCode::OK || status_code == StatusCode::NOT_FOUND; let redirect_on_error = @@ -531,6 +531,8 @@ fn path_to_keys<'a>( }) } (true, false) => { + // we need to remove the leading / + target.remove(0); if status_code == StatusCode::OK { break target; } else { @@ -549,6 +551,7 @@ fn path_to_keys<'a>( }); } (true, true) => { + target.remove(0); return Ok(RoutingResult::LoadOrAlternativeError { key, redirect_key: target, @@ -573,14 +576,14 @@ fn path_to_keys<'a>( } else { Ok(RoutingResult::LoadOrRedirect { redirect_if_exists: Some(format!("{key}/{index}")), - key, // we can't use `path` because key might have changed substentially in case of // routing rules redirect_url: percent_encoding::percent_encode( - format!("{path}/").as_bytes(), + format!("/{key}/").as_bytes(), PATH_ENCODING_SET, ) .to_string(), + key, redirect_code: StatusCode::FOUND, }) }