From 5673e2ad2f24db6e57fcabe139de2320dce96edf Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 20 Dec 2021 15:58:28 -0500 Subject: [PATCH 1/5] Update file serving example to stream files --- Cargo.lock | 125 +++++++++++++++++++++++++++++++ dropshot/Cargo.toml | 1 + dropshot/examples/file_server.rs | 5 +- dropshot/src/test_util.rs | 4 +- 4 files changed, 131 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e3d6d4e6d..9edc49216 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -181,6 +181,7 @@ dependencies = [ "hostname", "http", "hyper", + "hyper-staticfile", "indexmap", "lazy_static", "libc", @@ -449,6 +450,12 @@ dependencies = [ "pin-project-lite", ] +[[package]] +name = "http-range" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eee9694f83d9b7c09682fdb32213682939507884e5bcf227be9aff5d644b90dc" + [[package]] name = "httparse" version = "1.5.1" @@ -485,6 +492,36 @@ dependencies = [ "want", ] +[[package]] +name = "hyper-staticfile" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0d5ff45ea721e295c400e4c65b1c855d43d329b8ae8ec12520ab1860a240debf" +dependencies = [ + "futures-util", + "http", + "http-range", + "httpdate", + "hyper", + "mime_guess", + "percent-encoding", + "rand", + "tokio", + "url", + "winapi", +] + +[[package]] +name = "idna" +version = "0.2.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "418a0a6fab821475f634efe3ccc45c013f742efe03d853e8d3355d5cb850ecf8" +dependencies = [ + "matches", + "unicode-bidi", + "unicode-normalization", +] + [[package]] name = "indexmap" version = "1.7.0" @@ -762,6 +799,12 @@ version = "0.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8b870d8c151b6f2fb93e84a13146138f05d02ed11c7e7c54f8826aaaf7c9f184" +[[package]] +name = "ppv-lite86" +version = "0.2.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ed0cfbc8191465bed66e1718596ee0b0b35d5ee1f41c5df2189d0fe8bde535ba" + [[package]] name = "proc-macro2" version = "1.0.33" @@ -780,6 +823,46 @@ dependencies = [ "proc-macro2", ] +[[package]] +name = "rand" +version = "0.8.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2e7573632e6454cf6b99d7aac4ccca54be06da05aca2ef7423d22d27d4d4bcd8" +dependencies = [ + "libc", + "rand_chacha", + "rand_core", + "rand_hc", +] + +[[package]] +name = "rand_chacha" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e6c10a63a0fa32252be49d21e7709d4d4baf8d231c2dbce1eaa8141b9b127d88" +dependencies = [ + "ppv-lite86", + "rand_core", +] + +[[package]] +name = "rand_core" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d34f1408f55294453790c48b2f1ebbb1c5b4b7563eb1f418bcfcfdbb06ebb4e7" +dependencies = [ + "getrandom", +] + +[[package]] +name = "rand_hc" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d51e9f596de227fda2ea6c84607f5558e196eeaf43c986b724ba4fb8fdf497e7" +dependencies = [ + "rand_core", +] + [[package]] name = "redox_syscall" version = "0.2.10" @@ -1115,6 +1198,21 @@ dependencies = [ "winapi", ] +[[package]] +name = "tinyvec" +version = "1.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2c1c1d5a42b6245520c249549ec267180beaffcc0615401ac8e31853d4b6d8d2" +dependencies = [ + "tinyvec_macros", +] + +[[package]] +name = "tinyvec_macros" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cda74da7e1a664f795bb1f8a87ec406fb89a02522cf6e50620d016add6dbbf5c" + [[package]] name = "tokio" version = "1.14.0" @@ -1236,12 +1334,39 @@ dependencies = [ "version_check", ] +[[package]] +name = "unicode-bidi" +version = "0.3.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1a01404663e3db436ed2746d9fefef640d868edae3cceb81c3b8d5732fda678f" + +[[package]] +name = "unicode-normalization" +version = "0.1.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d54590932941a9e9266f0832deed84ebe1bf2e4c9e4a3554d393d18f5e854bf9" +dependencies = [ + "tinyvec", +] + [[package]] name = "unicode-xid" version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "8ccb82d61f80a663efe1f787a51b16b5a51e3314d6ac365b08639f52387b33f3" +[[package]] +name = "url" +version = "2.2.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a507c383b2d33b5fc35d1861e77e6b383d158b2da5e14fe51b83dfedf6fd578c" +dependencies = [ + "form_urlencoded", + "idna", + "matches", + "percent-encoding", +] + [[package]] name = "usdt" version = "0.3.1" diff --git a/dropshot/Cargo.toml b/dropshot/Cargo.toml index 1a0ef7250..d29032469 100644 --- a/dropshot/Cargo.toml +++ b/dropshot/Cargo.toml @@ -69,6 +69,7 @@ features = [ "uuid" ] [dev-dependencies] expectorate = "1.0.4" +hyper-staticfile = "0.8" lazy_static = "1.4.0" libc = "0.2.111" mime_guess = "2.0.3" diff --git a/dropshot/examples/file_server.rs b/dropshot/examples/file_server.rs index 6a3cf2626..c1ef23472 100644 --- a/dropshot/examples/file_server.rs +++ b/dropshot/examples/file_server.rs @@ -139,9 +139,10 @@ async fn static_content( .header(http::header::CONTENT_TYPE, "text/html") .body(body.into())?) } else { - let body = tokio::fs::read(&entry).await.map_err(|_| { + let file = tokio::fs::File::open(&entry).await.map_err(|_| { HttpError::for_bad_request(None, "EBADF".to_string()) })?; + let file_stream = hyper_staticfile::FileBytesStream::new(file); /* Derive the MIME type from the file name */ let content_type = mime_guess::from_path(&entry) @@ -151,7 +152,7 @@ async fn static_content( Ok(Response::builder() .status(StatusCode::OK) .header(http::header::CONTENT_TYPE, content_type) - .body(body.into())?) + .body(file_stream.into_body())?) } } diff --git a/dropshot/src/test_util.rs b/dropshot/src/test_util.rs index e7ad4c108..491c9eaf0 100644 --- a/dropshot/src/test_util.rs +++ b/dropshot/src/test_util.rs @@ -39,8 +39,8 @@ use crate::server::{HttpServer, HttpServerStarter, ServerContext}; * List of allowed HTTP headers in responses. This is used to make sure we * don't leak headers unexpectedly. */ -const ALLOWED_HEADER_NAMES: [&str; 4] = - ["content-length", "content-type", "date", "x-request-id"]; +const ALLOWED_HEADER_NAMES: [&str; 5] = + ["content-length", "content-type", "date", "x-request-id", "transfer-encoding"]; /** * ClientTestContext encapsulates several facilities associated with using an From 59165e4802b0a47a1a7219f30a5a15bbe5543e56 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Mon, 20 Dec 2021 15:59:10 -0500 Subject: [PATCH 2/5] fmt --- dropshot/src/test_util.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/dropshot/src/test_util.rs b/dropshot/src/test_util.rs index 491c9eaf0..f78bb6d99 100644 --- a/dropshot/src/test_util.rs +++ b/dropshot/src/test_util.rs @@ -39,8 +39,13 @@ use crate::server::{HttpServer, HttpServerStarter, ServerContext}; * List of allowed HTTP headers in responses. This is used to make sure we * don't leak headers unexpectedly. */ -const ALLOWED_HEADER_NAMES: [&str; 5] = - ["content-length", "content-type", "date", "x-request-id", "transfer-encoding"]; +const ALLOWED_HEADER_NAMES: [&str; 5] = [ + "content-length", + "content-type", + "date", + "x-request-id", + "transfer-encoding", +]; /** * ClientTestContext encapsulates several facilities associated with using an From 355489731144498715fed481fe2becbcb411f5ec Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 21 Dec 2021 10:14:46 -0500 Subject: [PATCH 3/5] Streaming test --- Cargo.lock | 24 +++++++++++ dropshot/Cargo.toml | 1 + dropshot/tests/test_streaming.rs | 72 ++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+) create mode 100644 dropshot/tests/test_streaming.rs diff --git a/Cargo.lock b/Cargo.lock index 9edc49216..a25fe01d8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -201,6 +201,7 @@ dependencies = [ "slog-term", "subprocess", "syn", + "tempfile", "tokio", "toml", "trybuild", @@ -882,6 +883,15 @@ dependencies = [ "redox_syscall", ] +[[package]] +name = "remove_dir_all" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3acd125665422973a33ac9d3dd2df85edad0f4ae9b00dafb1a05e43a9f5ef8e7" +dependencies = [ + "winapi", +] + [[package]] name = "rustversion" version = "1.0.5" @@ -1128,6 +1138,20 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f764005d11ee5f36500a149ace24e00e3da98b0158b3e2d53a7495660d3f4d60" +[[package]] +name = "tempfile" +version = "3.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dac1c663cfc93810f88aed9b8941d48cabf856a1b111c29a40439018d870eb22" +dependencies = [ + "cfg-if", + "libc", + "rand", + "redox_syscall", + "remove_dir_all", + "winapi", +] + [[package]] name = "term" version = "0.7.0" diff --git a/dropshot/Cargo.toml b/dropshot/Cargo.toml index d29032469..72805d279 100644 --- a/dropshot/Cargo.toml +++ b/dropshot/Cargo.toml @@ -74,6 +74,7 @@ lazy_static = "1.4.0" libc = "0.2.111" mime_guess = "2.0.3" subprocess = "0.2.8" +tempfile = "3.2" trybuild = "1.0.53" [dev-dependencies.schemars] diff --git a/dropshot/tests/test_streaming.rs b/dropshot/tests/test_streaming.rs new file mode 100644 index 000000000..d77308971 --- /dev/null +++ b/dropshot/tests/test_streaming.rs @@ -0,0 +1,72 @@ +// Copyright 2021 Oxide Computer Company + +//! Test cases for streaming reqeusts. + +use dropshot::{ + endpoint, ApiDescription, HttpError, RequestContext +}; +use http::{Method, Response, StatusCode}; +use hyper::Body; +use hyper_staticfile::FileBytesStream; +use std::sync::Arc; +use tokio::io::AsyncWriteExt; + +#[macro_use] +extern crate slog; + +mod common; + +fn api() -> ApiDescription { + let mut api = ApiDescription::new(); + api.register(api_streaming).unwrap(); + api +} + +#[endpoint { + method = GET, + path = "/streaming", +}] +async fn api_streaming( + _rqctx: Arc>, +) -> Result, HttpError> { + let mut file = tempfile::tempfile().map_err(|_ | { + HttpError::for_bad_request(None, "EBADF".to_string()) + }).map(|f| { + tokio::fs::File::from_std(f) + })?; + + let mut buf = [0; 8192]; + for i in 0..255 { + file.write_all(&buf).await.unwrap(); + buf.fill(i); + } + let file_stream = FileBytesStream::new(file); + Ok(Response::builder() + .status(StatusCode::OK) + .body(file_stream.into_body())?) +} + +#[tokio::test] +async fn test_streaming_basic() { + let api = api(); + let testctx = common::test_setup("streaming_basic", api); + let client = &testctx.client_testctx; + + let response = client + .make_request_no_body( + Method::GET, + "/streaming", + StatusCode::OK, + ) + .await + .expect("Expected success"); + + let transfer_encoding_header = response + .headers() + .get("transfer-encoding") + .expect("Expected 'transfer-encoding' header to be set on streaming response"); + + assert_eq!("foo", transfer_encoding_header); + + testctx.teardown().await; +} From 3a9de4373e230bc6bbee87664eca0caf2460a5c1 Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Tue, 21 Dec 2021 11:29:19 -0500 Subject: [PATCH 4/5] Fix and expand streaming tests --- dropshot/tests/test_streaming.rs | 100 ++++++++++++++++++++++--------- 1 file changed, 72 insertions(+), 28 deletions(-) diff --git a/dropshot/tests/test_streaming.rs b/dropshot/tests/test_streaming.rs index d77308971..0f9a56c40 100644 --- a/dropshot/tests/test_streaming.rs +++ b/dropshot/tests/test_streaming.rs @@ -2,14 +2,12 @@ //! Test cases for streaming reqeusts. -use dropshot::{ - endpoint, ApiDescription, HttpError, RequestContext -}; +use dropshot::{endpoint, ApiDescription, HttpError, RequestContext}; use http::{Method, Response, StatusCode}; -use hyper::Body; +use hyper::{body::HttpBody, Body}; use hyper_staticfile::FileBytesStream; use std::sync::Arc; -use tokio::io::AsyncWriteExt; +use tokio::io::{AsyncSeekExt, AsyncWriteExt}; #[macro_use] extern crate slog; @@ -22,6 +20,9 @@ fn api() -> ApiDescription { api } +const BUF_SIZE: usize = 8192; +const BUF_COUNT: usize = 128; + #[endpoint { method = GET, path = "/streaming", @@ -29,44 +30,87 @@ fn api() -> ApiDescription { async fn api_streaming( _rqctx: Arc>, ) -> Result, HttpError> { - let mut file = tempfile::tempfile().map_err(|_ | { - HttpError::for_bad_request(None, "EBADF".to_string()) - }).map(|f| { - tokio::fs::File::from_std(f) - })?; - - let mut buf = [0; 8192]; - for i in 0..255 { + let mut file = tempfile::tempfile() + .map_err(|_| HttpError::for_bad_request(None, "EBADF".to_string())) + .map(|f| tokio::fs::File::from_std(f))?; + + // Fill the file with some arbitrary contents. + let mut buf = [0; BUF_SIZE]; + for i in 0..BUF_COUNT { file.write_all(&buf).await.unwrap(); - buf.fill(i); + buf.fill((i & 255) as u8); } + file.seek(std::io::SeekFrom::Start(0)).await.unwrap(); + let file_stream = FileBytesStream::new(file); Ok(Response::builder() .status(StatusCode::OK) .body(file_stream.into_body())?) } +fn check_has_chunked_headers(response: &Response) { + let transfer_encoding_header = + response.headers().get("transfer-encoding").expect( + "Expected 'transfer-encoding' header to be set on streaming \ + response", + ); + assert_eq!("chunked", transfer_encoding_header); +} + #[tokio::test] -async fn test_streaming_basic() { +async fn test_streaming_server_streaming_client() { let api = api(); - let testctx = common::test_setup("streaming_basic", api); + let testctx = common::test_setup("streaming_server_streaming_client", api); let client = &testctx.client_testctx; - let response = client - .make_request_no_body( - Method::GET, - "/streaming", - StatusCode::OK, - ) + let mut response = client + .make_request_no_body(Method::GET, "/streaming", StatusCode::OK) .await - .expect("Expected success"); + .expect("Expected GET request to succeed"); + check_has_chunked_headers(&response); - let transfer_encoding_header = response - .headers() - .get("transfer-encoding") - .expect("Expected 'transfer-encoding' header to be set on streaming response"); + let mut chunk_count = 0; + let mut byte_count = 0; + while let Some(chunk) = response.body_mut().data().await { + let chunk = chunk.expect("Should have received chunk without error"); + byte_count += chunk.len(); + chunk_count += 1; + } + + assert!( + chunk_count >= 2, + "Expected 2+ chunks for streaming, saw: {}", + chunk_count + ); + assert_eq!( + BUF_SIZE * BUF_COUNT, + byte_count, + "Mismatch of sent vs received byte count" + ); - assert_eq!("foo", transfer_encoding_header); + testctx.teardown().await; +} + +#[tokio::test] +async fn test_streaming_server_buffered_client() { + let api = api(); + let testctx = common::test_setup("streaming_server_buffered_client", api); + let client = &testctx.client_testctx; + + let mut response = client + .make_request_no_body(Method::GET, "/streaming", StatusCode::OK) + .await + .expect("Expected GET request to succeed"); + check_has_chunked_headers(&response); + + let body_bytes = hyper::body::to_bytes(response.body_mut()) + .await + .expect("Error reading body"); + assert_eq!( + BUF_SIZE * BUF_COUNT, + body_bytes.len(), + "Mismatch of sent vs received byte count" + ); testctx.teardown().await; } From 21522d6b844d7e11ce6b8debb85e602eeb613a8d Mon Sep 17 00:00:00 2001 From: Sean Klein Date: Fri, 7 Jan 2022 18:29:57 -0500 Subject: [PATCH 5/5] Respond to dap's comments --- dropshot/src/test_util.rs | 58 +++++++++++++++++++++------- dropshot/tests/test_streaming.rs | 66 ++++++++++++++++++++++++++------ 2 files changed, 99 insertions(+), 25 deletions(-) diff --git a/dropshot/src/test_util.rs b/dropshot/src/test_util.rs index f78bb6d99..afe1947d8 100644 --- a/dropshot/src/test_util.rs +++ b/dropshot/src/test_util.rs @@ -35,16 +35,36 @@ use crate::logging::ConfigLogging; use crate::pagination::ResultsPage; use crate::server::{HttpServer, HttpServerStarter, ServerContext}; -/** - * List of allowed HTTP headers in responses. This is used to make sure we - * don't leak headers unexpectedly. - */ -const ALLOWED_HEADER_NAMES: [&str; 5] = [ - "content-length", - "content-type", - "date", - "x-request-id", - "transfer-encoding", +enum AllowedValue<'a> { + Any, + OneOf(&'a [&'a str]), +} + +struct AllowedHeader<'a> { + name: &'a str, + value: AllowedValue<'a>, +} + +impl<'a> AllowedHeader<'a> { + const fn new(name: &'a str) -> Self { + Self { + name, + value: AllowedValue::Any, + } + } +} + +// List of allowed HTTP headers in responsees. +// Used to make sure we don't leak headers unexpectedly. +const ALLOWED_HEADERS: [AllowedHeader<'static>; 5] = [ + AllowedHeader::new("content-length"), + AllowedHeader::new("content-type"), + AllowedHeader::new("date"), + AllowedHeader::new("x-request-id"), + AllowedHeader { + name: "transfer-encoding", + value: AllowedValue::OneOf(&["chunked"]), + }, ]; /** @@ -204,11 +224,21 @@ impl ClientTestContext { * statically-defined above. */ let headers = response.headers(); - for header_name in headers.keys() { + for (header_name, header_value) in headers { let mut okay = false; - for allowed_name in ALLOWED_HEADER_NAMES.iter() { - if header_name == allowed_name { - okay = true; + for allowed_header in ALLOWED_HEADERS.iter() { + if header_name == allowed_header.name { + match allowed_header.value { + AllowedValue::Any => { + okay = true; + } + AllowedValue::OneOf(allowed_values) => { + let header = header_value + .to_str() + .expect("Cannot turn header value to string"); + okay = allowed_values.contains(&header); + } + } break; } } diff --git a/dropshot/tests/test_streaming.rs b/dropshot/tests/test_streaming.rs index 0f9a56c40..4578af57d 100644 --- a/dropshot/tests/test_streaming.rs +++ b/dropshot/tests/test_streaming.rs @@ -1,6 +1,6 @@ // Copyright 2021 Oxide Computer Company -//! Test cases for streaming reqeusts. +//! Test cases for streaming requests. use dropshot::{endpoint, ApiDescription, HttpError, RequestContext}; use http::{Method, Response, StatusCode}; @@ -17,6 +17,7 @@ mod common; fn api() -> ApiDescription { let mut api = ApiDescription::new(); api.register(api_streaming).unwrap(); + api.register(api_not_streaming).unwrap(); api } @@ -31,7 +32,12 @@ async fn api_streaming( _rqctx: Arc>, ) -> Result, HttpError> { let mut file = tempfile::tempfile() - .map_err(|_| HttpError::for_bad_request(None, "EBADF".to_string())) + .map_err(|_| { + HttpError::for_bad_request( + None, + "Cannot create tempfile".to_string(), + ) + }) .map(|f| tokio::fs::File::from_std(f))?; // Fill the file with some arbitrary contents. @@ -48,13 +54,34 @@ async fn api_streaming( .body(file_stream.into_body())?) } -fn check_has_chunked_headers(response: &Response) { - let transfer_encoding_header = - response.headers().get("transfer-encoding").expect( - "Expected 'transfer-encoding' header to be set on streaming \ - response", - ); - assert_eq!("chunked", transfer_encoding_header); +#[endpoint { + method = GET, + path = "/not-streaming", +}] +async fn api_not_streaming( + _rqctx: Arc>, +) -> Result, HttpError> { + Ok(Response::builder() + .status(StatusCode::OK) + .body(serde_json::to_string("not-streaming").unwrap().into())?) +} + +fn check_has_transfer_encoding( + response: &Response, + expected_value: Option<&str>, +) { + let transfer_encoding_header = response.headers().get("transfer-encoding"); + match expected_value { + Some(expected_value) => { + assert_eq!( + expected_value, + transfer_encoding_header.expect("expected value") + ); + } + None => { + assert!(transfer_encoding_header.is_none()) + } + } } #[tokio::test] @@ -67,7 +94,7 @@ async fn test_streaming_server_streaming_client() { .make_request_no_body(Method::GET, "/streaming", StatusCode::OK) .await .expect("Expected GET request to succeed"); - check_has_chunked_headers(&response); + check_has_transfer_encoding(&response, Some("chunked")); let mut chunk_count = 0; let mut byte_count = 0; @@ -101,7 +128,7 @@ async fn test_streaming_server_buffered_client() { .make_request_no_body(Method::GET, "/streaming", StatusCode::OK) .await .expect("Expected GET request to succeed"); - check_has_chunked_headers(&response); + check_has_transfer_encoding(&response, Some("chunked")); let body_bytes = hyper::body::to_bytes(response.body_mut()) .await @@ -114,3 +141,20 @@ async fn test_streaming_server_buffered_client() { testctx.teardown().await; } + +#[tokio::test] +async fn test_non_streaming_servers_do_not_use_transfer_encoding() { + let api = api(); + let testctx = common::test_setup( + "non_streaming_servers_do_not_use_transfer_encoding", + api, + ); + let client = &testctx.client_testctx; + + let response = client + .make_request_no_body(Method::GET, "/not-streaming", StatusCode::OK) + .await + .expect("Expected GET request to succeed"); + check_has_transfer_encoding(&response, None); + testctx.teardown().await; +}