From 47467df83e7be107d92ec9fefb23542f760e9039 Mon Sep 17 00:00:00 2001 From: Alex Auvolat Date: Sat, 4 Jan 2025 19:50:57 +0100 Subject: [PATCH] avoid handling status_code-related logic in api/s3/get.rs --- src/api/s3/get.rs | 44 +++++------------------ src/web/web_server.rs | 83 +++++++++++++++++++++++++++++++++---------- 2 files changed, 73 insertions(+), 54 deletions(-) diff --git a/src/api/s3/get.rs b/src/api/s3/get.rs index eea3434e..f5d3cf11 100644 --- a/src/api/s3/get.rs +++ b/src/api/s3/get.rs @@ -163,15 +163,7 @@ pub async fn handle_head( key: &str, part_number: Option, ) -> Result, 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, bucket_id: Uuid, key: &str, - status_code: StatusCode, part_number: Option, ) -> Result, 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, overrides: GetObjectOverrides, ) -> Result, 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, bucket_id: Uuid, key: &str, - status_code: StatusCode, part_number: Option, overrides: GetObjectOverrides, ) -> Result, 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, 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); diff --git a/src/web/web_server.rs b/src/web/web_server.rs index bd543bb0..23a21614 100644 --- a/src/web/web_server.rs +++ b/src/web/web_server.rs @@ -6,6 +6,7 @@ use tokio::net::{TcpListener, UnixListener}; use tokio::sync::watch; use hyper::{ + body::Body, body::Incoming as IncomingBody, header::{HeaderValue, HOST}, Method, Request, Response, StatusCode, @@ -22,6 +23,7 @@ use crate::error::*; use garage_api::generic_server::{server_loop, UnixListenerOn}; use garage_api::helpers::*; +use garage_api::s3::api_server::ResBody; use garage_api::s3::cors::{add_cors_headers, find_matching_cors_rule, handle_options_for_bucket}; use garage_api::s3::error::{ CommonErrorDerivative, Error as ApiError, OkOrBadRequest, OkOrInternalError, @@ -252,19 +254,10 @@ impl WebServer { .body(empty_body()) .unwrap()), (&Method::HEAD, Ok((key, code))) => { - handle_head_without_ctx(self.garage.clone(), req, bucket_id, key, code, None).await + handle_head(self.garage.clone(), req, bucket_id, key, code).await } (&Method::GET, Ok((key, code))) => { - handle_get_without_ctx( - self.garage.clone(), - req, - bucket_id, - key, - code, - None, - Default::default(), - ) - .await + handle_get(self.garage.clone(), req, bucket_id, key, code).await } _ => Err(ApiError::bad_request("HTTP method not supported")), }; @@ -306,25 +299,22 @@ impl WebServer { ) => { match *req.method() { Method::HEAD => { - handle_head_without_ctx( + handle_head( self.garage.clone(), req, bucket_id, redirect_key, *redirect_code, - None, ) .await } Method::GET => { - handle_get_without_ctx( + handle_get( self.garage.clone(), req, bucket_id, redirect_key, *redirect_code, - None, - Default::default(), ) .await } @@ -361,14 +351,12 @@ impl WebServer { .body(empty_body::()) .unwrap(); - match handle_get_without_ctx( + match handle_get( self.garage.clone(), &req2, bucket_id, &error_document, error.http_status_code(), - None, - Default::default(), ) .await { @@ -413,6 +401,63 @@ impl WebServer { } } +async fn handle_head( + garage: Arc, + req: &Request, + bucket_id: Uuid, + key: &str, + status_code: StatusCode, +) -> Result, ApiError> { + if status_code != StatusCode::OK { + // See comment in handle_get + let cleaned_req = Request::builder() + .uri(req.uri()) + .body(empty_body::()) + .unwrap(); + + let mut ret = handle_head_without_ctx(garage, &cleaned_req, bucket_id, key, None).await?; + *ret.status_mut() = status_code; + Ok(ret) + } else { + handle_head_without_ctx(garage, req, bucket_id, key, None).await + } +} + +pub async fn handle_get( + garage: Arc, + req: &Request, + bucket_id: Uuid, + key: &str, + status_code: StatusCode, +) -> Result, ApiError> { + if status_code != StatusCode::OK { + // If we are returning an error document, discard all headers from + // the original GET request that would have influenced the result: + // - Range header, we don't want to return a subrange of the error document + // - Caching directives such as If-None-Match, etc, which are not relevant + let cleaned_req = Request::builder() + .uri(req.uri()) + .body(empty_body::()) + .unwrap(); + + let mut ret = handle_get_without_ctx( + garage, + &cleaned_req, + bucket_id, + key, + None, + Default::default(), + ) + .await?; + + *ret.status_mut() = status_code; + + Ok(ret) + } else { + handle_get_without_ctx(garage, req, bucket_id, key, None, Default::default()).await + } +} + fn error_to_res(e: Error) -> Response> { // If we are here, it is either that: // - there was an error before trying to get the requested URL