src/api/admin/bucket.rs

@@ -423,7 +423,6 @@ pub async fn handle_update_bucket(
 					"Please specify indexDocument when enabling website access.",
 				)?,
 				error_document: wa.error_document,
-				routing_rules: Vec::new(),
 			}));
 		} else {
 			if wa.index_document.is_some() || wa.error_document.is_some() {

src/api/s3/get.rs

@@ -163,15 +163,7 @@ pub async fn handle_head(
 	key: &str,
 	part_number: Option<u64>,
 ) -> Result<Response<ResBody>, Error> {
-	handle_head_without_ctx(
-		ctx.garage,
-		req,
-		ctx.bucket_id,
-		key,
-		StatusCode::OK,
-		part_number,
-	)
-	.await
+	handle_head_without_ctx(ctx.garage, req, ctx.bucket_id, key, part_number).await
 }
 
 /// Handle HEAD request for website
@@ -180,7 +172,6 @@ pub async fn handle_head_without_ctx(
 	req: &Request<impl Body>,
 	bucket_id: Uuid,
 	key: &str,
-	status_code: StatusCode,
 	part_number: Option<u64>,
 ) -> Result<Response<ResBody>, Error> {
 	let object = garage
@@ -281,7 +272,7 @@ pub async fn handle_head_without_ctx(
 			checksum_mode,
 		)
 		.header(CONTENT_LENGTH, format!("{}", version_meta.size))
-		.status(status_code)
+		.status(StatusCode::OK)
 		.body(empty_body())?)
 	}
 }
@@ -294,16 +285,7 @@ pub async fn handle_get(
 	part_number: Option<u64>,
 	overrides: GetObjectOverrides,
 ) -> Result<Response<ResBody>, Error> {
-	handle_get_without_ctx(
-		ctx.garage,
-		req,
-		ctx.bucket_id,
-		key,
-		StatusCode::OK,
-		part_number,
-		overrides,
-	)
-	.await
+	handle_get_without_ctx(ctx.garage, req, ctx.bucket_id, key, part_number, overrides).await
 }
 
 /// Handle GET request
@@ -312,7 +294,6 @@ pub async fn handle_get_without_ctx(
 	req: &Request<impl Body>,
 	bucket_id: Uuid,
 	key: &str,
-	status_code: StatusCode,
 	part_number: Option<u64>,
 	overrides: GetObjectOverrides,
 ) -> Result<Response<ResBody>, Error> {
@@ -348,15 +329,11 @@ pub async fn handle_get_without_ctx(
 
 	let checksum_mode = checksum_mode(&req);
 
-	match (
-		part_number,
-		parse_range_header(req, last_v_meta.size)?,
-		status_code == StatusCode::OK,
-	) {
-		(Some(_), Some(_), _) => Err(Error::bad_request(
+	match (part_number, parse_range_header(req, last_v_meta.size)?) {
+		(Some(_), Some(_)) => Err(Error::bad_request(
 			"Cannot specify both partNumber and Range header",
 		)),
-		(Some(pn), None, true) => {
+		(Some(pn), None) => {
 			handle_get_part(
 				garage,
 				last_v,
@@ -369,7 +346,7 @@ pub async fn handle_get_without_ctx(
 			)
 			.await
 		}
-		(None, Some(range), true) => {
+		(None, Some(range)) => {
 			handle_get_range(
 				garage,
 				last_v,
@@ -383,8 +360,7 @@ pub async fn handle_get_without_ctx(
 			)
 			.await
 		}
-		_ => {
-			// either not a range, or an error request: always return the full doc
+		(None, None) => {
 			handle_get_full(
 				garage,
 				last_v,
@@ -394,7 +370,6 @@ pub async fn handle_get_without_ctx(
 				&headers,
 				overrides,
 				checksum_mode,
-				status_code,
 			)
 			.await
 		}
@@ -410,7 +385,6 @@ async fn handle_get_full(
 	meta_inner: &ObjectVersionMetaInner,
 	overrides: GetObjectOverrides,
 	checksum_mode: ChecksumMode,
-	status_code: StatusCode,
 ) -> Result<Response<ResBody>, Error> {
 	let mut resp_builder = object_headers(
 		version,
@@ -420,7 +394,7 @@ async fn handle_get_full(
 		checksum_mode,
 	)
 	.header(CONTENT_LENGTH, format!("{}", version_meta.size))
-	.status(status_code);
+	.status(StatusCode::OK);
 	getobject_override_headers(overrides, &mut resp_builder)?;
 
 	let stream = full_object_byte_stream(garage, version, version_data, encryption);

src/api/s3/website.rs

@@ -10,7 +10,7 @@ use crate::s3::error::*;
 use crate::s3::xml::{to_xml_with_header, xmlns_tag, IntValue, Value};
 use crate::signature::verify_signed_content;
 
-use garage_model::bucket_table::{self, *};
+use garage_model::bucket_table::*;
 use garage_util::data::*;
 
 pub async fn handle_get_website(ctx: ReqCtx) -> Result<Response<ResBody>, Error> {
@@ -25,28 +25,7 @@ pub async fn handle_get_website(ctx: ReqCtx) -> Result<Response<ResBody>, Error>
 				suffix: Value(website.index_document.to_string()),
 			}),
 			redirect_all_requests_to: None,
-			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(),
-			},
+			routing_rules: None,
 		};
 		let xml = to_xml_with_header(&wc)?;
 		Ok(Response::builder()
@@ -122,28 +101,18 @@ pub struct WebsiteConfiguration {
 	pub index_document: Option<Suffix>,
 	#[serde(rename = "RedirectAllRequestsTo")]
 	pub redirect_all_requests_to: Option<Target>,
-	#[serde(
-		rename = "RoutingRules",
-		default,
-		skip_serializing_if = "RoutingRules::is_empty"
-	)]
-	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()
-	}
+	#[serde(rename = "RoutingRules")]
+	pub routing_rules: Option<Vec<RoutingRule>>,
 }
 
 #[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")]
@@ -197,7 +166,7 @@ impl WebsiteConfiguration {
 		if self.redirect_all_requests_to.is_some()
 			&& (self.error_document.is_some()
 				|| self.index_document.is_some()
-				|| !self.routing_rules.is_empty())
+				|| self.routing_rules.is_some())
 		{
 			return Err(Error::bad_request(
 				"Bad XML: can't have RedirectAllRequestsTo and other fields",
@@ -212,15 +181,10 @@ impl WebsiteConfiguration {
 		if let Some(ref rart) = self.redirect_all_requests_to {
 			rart.validate()?;
 		}
-		for rr in &self.routing_rules.rules {
-			rr.validate()?;
-		}
-		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(
-				"Bad XML: RoutingRules can't have more than 1000 child elements",
-			));
+		if let Some(ref rrs) = self.routing_rules {
+			for rr in rrs {
+				rr.inner.validate()?;
+			}
 		}
 
 		Ok(())
@@ -231,6 +195,10 @@ 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
@@ -238,33 +206,6 @@ impl WebsiteConfiguration {
 					.map(|x| x.suffix.0)
 					.unwrap_or_else(|| "index.html".to_string()),
 				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.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.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
-									.redirect
-									.http_redirect_code
-									.map(|c| c.0 as u16)
-									.unwrap_or(302),
-								replace_key_prefix: rule.redirect.replace_prefix.map(|k| k.0),
-								replace_key: rule.redirect.replace_full.map(|k| k.0),
-							},
-						}
-					})
-					.collect(),
 			})
 		}
 	}
@@ -305,69 +246,37 @@ impl Target {
 	}
 }
 
-impl RoutingRule {
+impl RoutingRuleInner {
 	pub fn validate(&self) -> Result<(), Error> {
-		if let Some(condition) = &self.condition {
-			condition.validate()?;
-		}
-		self.redirect.validate()
-	}
-}
-
-impl Condition {
-	pub fn validate(&self) -> Result<bool, Error> {
-		if let Some(ref error_code) = self.http_error_code {
-			// TODO do other error codes make sense? Aws only allows 4xx and 5xx
-			if error_code.0 != 404 {
-				return Err(Error::bad_request(
-					"Bad XML: HttpErrorCodeReturnedEquals must be 404 or absent",
-				));
-			}
-		}
-		Ok(self.prefix.is_some())
+		let has_prefix = self
+			.condition
+			.as_ref()
+			.and_then(|c| c.prefix.as_ref())
+			.is_some();
+		self.redirect.validate(has_prefix)
 	}
 }
 
 impl Redirect {
-	pub fn validate(&self) -> Result<(), Error> {
-		if self.replace_prefix.is_some() && self.replace_full.is_some() {
-			return Err(Error::bad_request(
-				"Bad XML: both ReplaceKeyPrefixWith and ReplaceKeyWith are set",
-			));
+	pub fn validate(&self, has_prefix: bool) -> Result<(), Error> {
+		if self.replace_prefix.is_some() {
+			if self.replace_full.is_some() {
+				return Err(Error::bad_request(
+					"Bad XML: both ReplaceKeyPrefixWith and ReplaceKeyWith are set",
+				));
+			}
+			if !has_prefix {
+				return Err(Error::bad_request(
+					"Bad XML: ReplaceKeyPrefixWith is set, but  KeyPrefixEquals isn't",
+				));
+			}
 		}
 		if let Some(ref protocol) = self.protocol {
 			if protocol.0 != "http" && protocol.0 != "https" {
 				return Err(Error::bad_request("Bad XML: invalid protocol"));
 			}
 		}
-		if let Some(ref http_redirect_code) = self.http_redirect_code {
-			match http_redirect_code.0 {
-				// aws allows all 3xx except 300, but some are non-sensical (not modified,
-				// use proxy...)
-				301 | 302 | 303 | 307 | 308 => {
-					if self.hostname.is_none() && self.protocol.is_some() {
-						return Err(Error::bad_request(
-							"Bad XML: HostName must be set if Protocol is set",
-						));
-					}
-				}
-				// aws doesn't allow these codes, but netlify does, and it seems like a
-				// cool feature (change the page seen without changing the url shown by the
-				// user agent)
-				200 | 404 => {
-					if self.hostname.is_some() || self.protocol.is_some() {
-						// hostname would mean different bucket, protocol doesn't make
-						// sense
-						return Err(Error::bad_request(
-                                        "Bad XML: an HttpRedirectCode of 200 is not acceptable alongside HostName or Protocol",
-                                ));
-					}
-				}
-				_ => {
-					return Err(Error::bad_request("Bad XML: invalid HttpRedirectCode"));
-				}
-			}
-		}
+		// TODO there are probably more invalide cases, but which ones?
 		Ok(())
 	}
 }
@@ -406,15 +315,6 @@ 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();
@@ -430,36 +330,21 @@ mod tests {
 				hostname: Value("garage.tld".to_owned()),
 				protocol: Some(Value("https".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())),
-						},
+			routing_rules: Some(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())),
 					},
-					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/admin/bucket.rs

@@ -393,7 +393,6 @@ impl AdminRpcHandler {
 			Some(WebsiteConfig {
 				index_document: query.index_document.clone(),
 				error_document: query.error_document.clone(),
-				routing_rules: Vec::new(),
 			})
 		} else {
 			None

src/garage/tests/s3/website.rs

@@ -5,10 +5,7 @@ use crate::json_body;
 use assert_json_diff::assert_json_eq;
 use aws_sdk_s3::{
 	primitives::ByteStream,
-	types::{
-		Condition, CorsConfiguration, CorsRule, ErrorDocument, IndexDocument, Protocol, Redirect,
-		RoutingRule, WebsiteConfiguration,
-	},
+	types::{CorsConfiguration, CorsRule, ErrorDocument, IndexDocument, WebsiteConfiguration},
 };
 use http::{Request, StatusCode};
 use http_body_util::BodyExt;
@@ -508,444 +505,3 @@ 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/model/bucket_table.rs

@@ -60,60 +60,6 @@ mod v08 {
 	pub struct WebsiteConfig {
 		pub index_document: String,
 		pub error_document: Option<String>,
-		#[serde(default, skip_serializing_if = "Vec::is_empty")]
-		pub routing_rules: Vec<RoutingRule>,
-	}
-
-	#[derive(PartialEq, Eq, Clone, Debug, Serialize, Deserialize)]
-	pub struct RoutingRule {
-		pub condition: Option<Condition>,
-		pub redirect: Redirect,
-	}
-
-	#[derive(PartialEq, Eq, Clone, Debug, Serialize, Deserialize)]
-	pub struct Condition {
-		pub http_error_code: Option<u16>,
-		pub prefix: Option<String>,
-	}
-
-	#[derive(PartialEq, Eq, Clone, Debug, Serialize, Deserialize)]
-	pub struct Redirect {
-		pub hostname: Option<String>,
-		pub http_redirect_code: u16,
-		pub protocol: Option<String>,
-		pub replace_key_prefix: Option<String>,
-		pub replace_key: Option<String>,
-	}
-
-	impl Redirect {
-		pub fn compute_target(&self, suffix: Option<&str>) -> String {
-			let mut res = String::new();
-			if let Some(hostname) = &self.hostname {
-				if let Some(protocol) = &self.protocol {
-					res.push_str(protocol);
-					res.push_str("://");
-				} else {
-					res.push_str("//");
-				}
-				res.push_str(hostname);
-			}
-			res.push('/');
-			if let Some(replace_key_prefix) = &self.replace_key_prefix {
-				res.push_str(replace_key_prefix);
-				if let Some(suffix) = suffix {
-					res.push_str(suffix)
-				}
-			} else if let Some(replace_key) = &self.replace_key {
-				res.push_str(replace_key)
-			}
-			res
-		}
-	}
-
-	#[derive(PartialEq, Eq, Clone, Debug, Serialize, Deserialize)]
-	pub struct RedirectAll {
-		pub hostname: String,
-		pub protoco: String,
 	}
 
 	#[derive(PartialEq, Eq, Clone, Debug, Serialize, Deserialize)]

src/web/web_server.rs

@@ -28,7 +28,6 @@ use garage_api::s3::error::{
 };
 use garage_api::s3::get::{handle_get_without_ctx, handle_head_without_ctx};
 
-use garage_model::bucket_table::RoutingRule;
 use garage_model::garage::Garage;
 
 use garage_table::*;
@@ -235,32 +234,26 @@ 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, &website_config.routing_rules)?;
+		let (key, may_redirect) = path_to_keys(&path, index)?;
 
 		debug!(
-			"Selected bucket: \"{}\" {:?}, routing to {:?}",
-			bucket_name, bucket_id, routing_result,
+			"Selected bucket: \"{}\" {:?}, target key: \"{}\", may redirect to: {:?}",
+			bucket_name, bucket_id, key, may_redirect
 		);
 
-		let ret_doc = match (req.method(), routing_result.main_target()) {
-			(&Method::OPTIONS, _) => handle_options_for_bucket(req, &bucket_params)
+		let ret_doc = match *req.method() {
+			Method::OPTIONS => handle_options_for_bucket(req, &bucket_params)
 				.map_err(ApiError::from)
 				.map(|res| res.map(|_empty_body: EmptyBody| empty_body())),
-			(_, Err((url, code))) => Ok(Response::builder()
-				.status(code)
-				.header("Location", url)
-				.body(empty_body())
-				.unwrap()),
-			(&Method::HEAD, Ok((key, code))) => {
-				handle_head_without_ctx(self.garage.clone(), req, bucket_id, key, code, None).await
+			Method::HEAD => {
+				handle_head_without_ctx(self.garage.clone(), req, bucket_id, &key, None).await
 			}
-			(&Method::GET, Ok((key, code))) => {
+			Method::GET => {
 				handle_get_without_ctx(
 					self.garage.clone(),
 					req,
 					bucket_id,
-					key,
-					code,
+					&key,
 					None,
 					Default::default(),
 				)
@@ -269,68 +262,16 @@ impl WebServer {
 			_ => Err(ApiError::bad_request("HTTP method not supported")),
 		};
 
-		// Try handling errors if bucket configuration provided fallbacks
-		let ret_doc_with_redir = match (&ret_doc, &routing_result) {
-			(
-				Err(ApiError::NoSuchKey),
-				RoutingResult::LoadOrRedirect {
-					redirect_if_exists,
-					redirect_url,
-					redirect_code,
-					..
-				},
-			) => {
-				let redirect = if let Some(redirect_key) = redirect_if_exists {
-					self.check_key_exists(bucket_id, redirect_key.as_str())
-						.await?
-				} else {
-					true
-				};
-				if redirect {
-					Ok(Response::builder()
-						.status(redirect_code)
-						.header("Location", redirect_url)
-						.body(empty_body())
-						.unwrap())
-				} else {
-					ret_doc
-				}
-			}
-			(
-				Err(ApiError::NoSuchKey),
-				RoutingResult::LoadOrAlternativeError {
-					redirect_key,
-					redirect_code,
-					..
-				},
-			) => {
-				match *req.method() {
-					Method::HEAD => {
-						handle_head_without_ctx(
-							self.garage.clone(),
-							req,
-							bucket_id,
-							redirect_key,
-							*redirect_code,
-							None,
-						)
-						.await
-					}
-					Method::GET => {
-						handle_get_without_ctx(
-							self.garage.clone(),
-							req,
-							bucket_id,
-							redirect_key,
-							*redirect_code,
-							None,
-							Default::default(),
-						)
-						.await
-					}
-					// we shouldn't ever reach here
-					_ => Err(ApiError::bad_request("HTTP method not supported")),
-				}
+		// Try implicit redirect on error
+		let ret_doc_with_redir = match (&ret_doc, may_redirect) {
+			(Err(ApiError::NoSuchKey), ImplicitRedirect::To { key, url })
+				if self.check_key_exists(bucket_id, key.as_str()).await? =>
+			{
+				Ok(Response::builder()
+					.status(StatusCode::FOUND)
+					.header("Location", url)
+					.body(empty_body())
+					.unwrap())
 			}
 			_ => ret_doc,
 		};
@@ -366,7 +307,6 @@ impl WebServer {
 					&req2,
 					bucket_id,
 					&error_document,
-					error.http_status_code(),
 					None,
 					Default::default(),
 				)
@@ -383,6 +323,8 @@ impl WebServer {
 							error
 						);
 
+						*error_doc.status_mut() = error.http_status_code();
+
 						// 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()) {
@@ -429,44 +371,9 @@ fn error_to_res(e: Error) -> Response<BoxBody<Error>> {
 }
 
 #[derive(Debug, PartialEq)]
-enum RoutingResult {
-	// Load a key and use `code` as status, or fallback to normal 404 handler if not found
-	LoadKey {
-		key: String,
-		code: StatusCode,
-	},
-	// Load a key and use `200` as status, or fallback with a redirection using `redirect_code`
-	// as status
-	LoadOrRedirect {
-		key: String,
-		redirect_if_exists: Option<String>,
-		redirect_url: String,
-		redirect_code: StatusCode,
-	},
-	// Load a key and use `200` as status, or fallback by loading a different key and use
-	// `redirect_code` as status
-	LoadOrAlternativeError {
-		key: String,
-		redirect_key: String,
-		redirect_code: StatusCode,
-	},
-	// Send an http redirect with `code` as status
-	Redirect {
-		url: String,
-		code: StatusCode,
-	},
-}
-
-impl RoutingResult {
-	// return Ok((key_to_deref, status_code)) or Err((redirect_target, status_code))
-	fn main_target(&self) -> Result<(&str, StatusCode), (&str, StatusCode)> {
-		match self {
-			RoutingResult::LoadKey { key, code } => Ok((key, *code)),
-			RoutingResult::LoadOrRedirect { key, .. } => Ok((key, StatusCode::OK)),
-			RoutingResult::LoadOrAlternativeError { key, .. } => Ok((key, StatusCode::OK)),
-			RoutingResult::Redirect { url, code } => Err((url, *code)),
-		}
-	}
+enum ImplicitRedirect {
+	No,
+	To { key: String, url: String },
 }
 
 /// Path to key
@@ -476,131 +383,35 @@ impl RoutingResult {
 /// which is also AWS S3 behavior.
 ///
 /// Check: https://docs.aws.amazon.com/AmazonS3/latest/userguide/IndexDocumentSupport.html
-fn path_to_keys(
-	path: &str,
-	index: &str,
-	routing_rules: &[RoutingRule],
-) -> Result<RoutingResult, Error> {
+fn path_to_keys<'a>(path: &'a str, index: &str) -> Result<(String, ImplicitRedirect), Error> {
 	let path_utf8 = percent_encoding::percent_decode_str(path).decode_utf8()?;
 
 	let base_key = match path_utf8.strip_prefix("/") {
 		Some(bk) => bk,
 		None => return Err(Error::BadRequest("Path must start with a / (slash)".into())),
 	};
-
-	let is_bucket_root = base_key.is_empty();
+	let is_bucket_root = base_key.len() == 0;
 	let is_trailing_slash = path_utf8.ends_with("/");
 
-	let key = if is_bucket_root || is_trailing_slash {
-		// we can't store anything at the root, so we need to query the index
-		// if the key end with a slash, we always query the index
-		format!("{base_key}{index}")
-	} else {
-		// if the key doesn't end with `/`, leave it unmodified
-		base_key.to_string()
-	};
+	match (is_bucket_root, is_trailing_slash) {
+		// It is not possible to store something at the root of the bucket (ie. empty key),
+		// the only option is to fetch the index
+		(true, _) => Ok((index.to_string(), ImplicitRedirect::No)),
 
-	let mut routing_rules_iter = routing_rules.iter();
-	let key = loop {
-		let Some(routing_rule) = routing_rules_iter.next() else {
-			break key;
-		};
+		// "If you create a folder structure in your bucket, you must have an index document at each level. In each folder, the index document must have the same name, for example, index.html. When a user specifies a URL that resembles a folder lookup, the presence or absence of a trailing slash determines the behavior of the website. For example, the following URL, with a trailing slash, returns the photos/index.html index document."
+		(false, true) => Ok((format!("{base_key}{index}"), ImplicitRedirect::No)),
 
-		let Ok(status_code) = StatusCode::from_u16(routing_rule.redirect.http_redirect_code) else {
-			continue;
-		};
-		if let Some(condition) = &routing_rule.condition {
-			let suffix = if let Some(prefix) = &condition.prefix {
-				let Some(suffix) = key.strip_prefix(prefix) else {
-					continue;
-				};
-				Some(suffix)
-			} else {
-				None
-			};
-			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 =
-				condition.http_error_code == Some(StatusCode::NOT_FOUND.as_u16());
-			match (query_alternative_key, redirect_on_error) {
-				(false, false) => {
-					return Ok(RoutingResult::Redirect {
-						url: target,
-						code: status_code,
-					})
-				}
-				(true, false) => {
-					// we need to remove the leading /
-					target.remove(0);
-					if status_code == StatusCode::OK {
-						break target;
-					} else {
-						return Ok(RoutingResult::LoadKey {
-							key: target,
-							code: status_code,
-						});
-					}
-				}
-				(false, true) => {
-					return Ok(RoutingResult::LoadOrRedirect {
-						key,
-						redirect_if_exists: None,
-						redirect_url: target,
-						redirect_code: status_code,
-					});
-				}
-				(true, true) => {
-					target.remove(0);
-					return Ok(RoutingResult::LoadOrAlternativeError {
-						key,
-						redirect_key: target,
-						redirect_code: status_code,
-					});
-				}
-			}
-		} else {
-			let target = routing_rule.redirect.compute_target(None);
-			return Ok(RoutingResult::Redirect {
-				url: target,
-				code: status_code,
-			});
-		}
-	};
-
-	if is_bucket_root || is_trailing_slash {
-		Ok(RoutingResult::LoadKey {
-			key,
-			code: StatusCode::OK,
-		})
-	} else {
-		Ok(RoutingResult::LoadOrRedirect {
-			redirect_if_exists: Some(format!("{key}/{index}")),
-			// we can't use `path` because key might have changed substentially in case of
-			// routing rules
-			redirect_url: percent_encoding::percent_encode(
-				format!("/{key}/").as_bytes(),
-				PATH_ENCODING_SET,
-			)
-			.to_string(),
-			key,
-			redirect_code: StatusCode::FOUND,
-		})
+		// "However, if you exclude the trailing slash from the preceding URL, Amazon S3 first looks for an object photos in the bucket. If the photos object is not found, it searches for an index document, photos/index.html. If that document is found, Amazon S3 returns a 302 Found message and points to the photos/ key. For subsequent requests to photos/, Amazon S3 returns photos/index.html. If the index document is not found, Amazon S3 returns an error."
+		(false, false) => Ok((
+			base_key.to_string(),
+			ImplicitRedirect::To {
+				key: format!("{base_key}/{index}"),
+				url: format!("{path}/"),
+			},
+		)),
 	}
 }
 
-// per https://url.spec.whatwg.org/#path-percent-encode-set
-const PATH_ENCODING_SET: &percent_encoding::AsciiSet = &percent_encoding::CONTROLS
-	.add(b' ')
-	.add(b'"')
-	.add(b'#')
-	.add(b'<')
-	.add(b'>')
-	.add(b'?')
-	.add(b'`')
-	.add(b'{')
-	.add(b'}');
-
 #[cfg(test)]
 mod tests {
 	use super::*;
@@ -608,39 +419,35 @@ mod tests {
 	#[test]
 	fn path_to_keys_test() -> Result<(), Error> {
 		assert_eq!(
-			path_to_keys("/file%20.jpg", "index.html", &[])?,
-			RoutingResult::LoadOrRedirect {
-				key: "file .jpg".to_string(),
-				redirect_url: "/file%20.jpg/".to_string(),
-				redirect_if_exists: Some("file .jpg/index.html".to_string()),
-				redirect_code: StatusCode::FOUND,
-			}
+			path_to_keys("/file%20.jpg", "index.html")?,
+			(
+				"file .jpg".to_string(),
+				ImplicitRedirect::To {
+					key: "file .jpg/index.html".to_string(),
+					url: "/file%20.jpg/".to_string()
+				}
+			)
 		);
 		assert_eq!(
-			path_to_keys("/%20t/", "index.html", &[])?,
-			RoutingResult::LoadKey {
-				key: " t/index.html".to_string(),
-				code: StatusCode::OK
-			}
+			path_to_keys("/%20t/", "index.html")?,
+			(" t/index.html".to_string(), ImplicitRedirect::No)
 		);
 		assert_eq!(
-			path_to_keys("/", "index.html", &[])?,
-			RoutingResult::LoadKey {
-				key: "index.html".to_string(),
-				code: StatusCode::OK
-			}
+			path_to_keys("/", "index.html")?,
+			("index.html".to_string(), ImplicitRedirect::No)
 		);
 		assert_eq!(
-			path_to_keys("/hello", "index.html", &[])?,
-			RoutingResult::LoadOrRedirect {
-				key: "hello".to_string(),
-				redirect_url: "/hello/".to_string(),
-				redirect_if_exists: Some("hello/index.html".to_string()),
-				redirect_code: StatusCode::FOUND,
-			}
+			path_to_keys("/hello", "index.html")?,
+			(
+				"hello".to_string(),
+				ImplicitRedirect::To {
+					key: "hello/index.html".to_string(),
+					url: "/hello/".to_string()
+				}
+			)
 		);
-		assert!(path_to_keys("", "index.html", &[]).is_err());
-		assert!(path_to_keys("i/am/relative", "index.html", &[]).is_err());
+		assert!(path_to_keys("", "index.html").is_err());
+		assert!(path_to_keys("i/am/relative", "index.html").is_err());
 		Ok(())
 	}
 }