add tests

src/api/s3/website.rs

@@ -25,8 +25,28 @@ pub async fn handle_get_website(ctx: ReqCtx) -> Result<Response<ResBody>, 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<RoutingRule>,
+	pub routing_rules: RoutingRules,
+}
+
+#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord, Default)]
+pub struct RoutingRules {
+	#[serde(rename = "RoutingRule")]
+	pub rules: Vec<RoutingRule>,
+}
+
+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<Condition>,
 	#[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 {
+							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 {
             <ReplaceKeyWith>fullkey</ReplaceKeyWith>
          </Redirect>
       </RoutingRule>
+      <RoutingRule>
+         <Condition>
+            <KeyPrefixEquals></KeyPrefixEquals>
+         </Condition>
+         <Redirect>
+            <HttpRedirectCode>404</HttpRedirectCode>
+            <ReplaceKeyWith>missing</ReplaceKeyWith>
+         </Redirect>
+      </RoutingRule>
    </RoutingRules>
 </WebsiteConfiguration>"#;
 		let conf: WebsiteConfiguration = from_str(message).unwrap();
@@ -405,8 +432,9 @@ mod tests {
 				hostname: Value("garage.tld".to_owned()),
 				protocol: Some(Value("https".to_owned())),
 			}),
-			routing_rules: vec![RoutingRule {
-				inner: RoutingRuleInner {
+			routing_rules: RoutingRules {
+				rules: vec![
+					RoutingRule {
 						condition: Some(Condition {
 							http_error_code: Some(IntValue(404)),
 							prefix: Some(Value("prefix1".to_owned())),
@@ -419,7 +447,21 @@ mod tests {
 							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,

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<http_body_util::Full<Bytes>>,
+) {
+	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();
+}

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,
 		})
 	}