From b81d6911f7fd46969ef73720e4489d0bf536fe47 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 12 Nov 2024 23:21:33 +0000 Subject: [PATCH 1/2] [spr] changes to main this commit is based on Created using spr 1.3.6-beta.1 [skip ci] --- CHANGELOG.adoc | 8 ++ README.adoc | 4 +- dropshot/src/config.rs | 110 +++++++++++++++++- dropshot/src/extractor/body.rs | 10 +- dropshot/src/handler.rs | 5 + dropshot/src/server.rs | 5 +- dropshot/src/websocket.rs | 2 +- .../api_trait.rs} | 0 .../mod.rs => integration-tests/common.rs} | 0 .../config.rs} | 77 +++++++++++- .../demo.rs} | 4 +- .../detached_shutdown.rs} | 2 +- dropshot/tests/integration-tests/main.rs | 25 ++++ .../multipart.rs} | 4 +- .../openapi.rs} | 0 .../pagination.rs} | 9 +- .../pagination_schema.rs} | 0 .../path_names.rs} | 0 .../starter.rs} | 6 +- .../streaming.rs} | 4 +- .../{test_tls.rs => integration-tests/tls.rs} | 9 +- 21 files changed, 241 insertions(+), 43 deletions(-) rename dropshot/tests/{test_api_trait.rs => integration-tests/api_trait.rs} (100%) rename dropshot/tests/{common/mod.rs => integration-tests/common.rs} (100%) rename dropshot/tests/{test_config.rs => integration-tests/config.rs} (90%) rename dropshot/tests/{test_demo.rs => integration-tests/demo.rs} (99%) rename dropshot/tests/{test_detached_shutdown.rs => integration-tests/detached_shutdown.rs} (99%) create mode 100644 dropshot/tests/integration-tests/main.rs rename dropshot/tests/{test_multipart.rs => integration-tests/multipart.rs} (99%) rename dropshot/tests/{test_openapi.rs => integration-tests/openapi.rs} (100%) rename dropshot/tests/{test_pagination.rs => integration-tests/pagination.rs} (99%) rename dropshot/tests/{test_pagination_schema.rs => integration-tests/pagination_schema.rs} (100%) rename dropshot/tests/{test_path_names.rs => integration-tests/path_names.rs} (100%) rename dropshot/tests/{test_starter.rs => integration-tests/starter.rs} (98%) rename dropshot/tests/{test_streaming.rs => integration-tests/streaming.rs} (99%) rename dropshot/tests/{test_tls.rs => integration-tests/tls.rs} (99%) diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index 1091346d9..97204cdf0 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -15,6 +15,14 @@ https://github.com/oxidecomputer/dropshot/compare/v0.12.0\...HEAD[Full list of commits] +=== Breaking changes + +* The `request_body_max_bytes` config has been renamed to `default_request_body_max_bytes`. This is to make its semantics clear with respect to per-endpoint request limits. ++ +Defining the old config option will produce an error, guiding you to perform the rename. + +=== Other notable changes + * https://github.com/oxidecomputer/dropshot/pull/1122[#1122] Adds a new `ServerBuilder` as the primary way of constructing a Dropshot server. This replaces `HttpServerStarter::new()` and `HttpServerStarter::new_with_tls()`. These older functions still exist for compatibility. They may be removed in an upcoming release, along with the `HttpServerStarter`. + In this release, using the builder interface is not very different from using these older functions. But as we look at adding new construction-time options (e.g., for API versioning), those will only be added to the builder. diff --git a/README.adoc b/README.adoc index e05d10c51..cb9e59245 100644 --- a/README.adoc +++ b/README.adoc @@ -45,11 +45,13 @@ include: |No |Specifies that the server should bind to the given IP address and TCP port. In general, servers can bind to more than one IP address and port, but this is not (yet?) supported. Defaults to "127.0.0.1:0". -|`request_body_max_bytes` +|`default_request_body_max_bytes` |`4096` |No |Specifies the maximum number of bytes allowed in a request body. Larger requests will receive a 400 error. Defaults to 1024. +Can be overridden per-endpoint via the `request_body_max_bytes` parameter to `#[endpoint { ... }]`. + |`tls.type` |`"AsFile"` |No diff --git a/dropshot/src/config.rs b/dropshot/src/config.rs index 40414ece9..0266714b0 100644 --- a/dropshot/src/config.rs +++ b/dropshot/src/config.rs @@ -32,7 +32,7 @@ pub type RawTlsConfig = rustls::ServerConfig; /// r##" /// [http_api_server] /// bind_address = "127.0.0.1:12345" -/// request_body_max_bytes = 1024 +/// default_request_body_max_bytes = 1024 /// ## ... (other app-specific config) /// "## /// ).map_err(|error| format!("parsing config: {}", error))?; @@ -43,12 +43,15 @@ pub type RawTlsConfig = rustls::ServerConfig; /// } /// ``` #[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] -#[serde(default)] +#[serde( + from = "DeserializedConfigDropshot", + into = "DeserializedConfigDropshot" +)] pub struct ConfigDropshot { /// IP address and TCP port to which to bind for accepting connections pub bind_address: SocketAddr, /// maximum allowed size of a request body, defaults to 1024 - pub request_body_max_bytes: usize, + pub default_request_body_max_bytes: usize, /// Default behavior for HTTP handler functions with respect to clients /// disconnecting early. pub default_handler_task_mode: HandlerTaskMode, @@ -113,9 +116,108 @@ impl Default for ConfigDropshot { fn default() -> Self { ConfigDropshot { bind_address: "127.0.0.1:0".parse().unwrap(), - request_body_max_bytes: 1024, + default_request_body_max_bytes: 1024, default_handler_task_mode: HandlerTaskMode::Detached, log_headers: Default::default(), } } } + +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +#[serde(default)] +struct DeserializedConfigDropshot { + bind_address: SocketAddr, + default_request_body_max_bytes: usize, + // Previous name for default_request_body_max_bytes. Present only to guide + // users to the new name. + #[serde( + deserialize_with = "deserialize_invalid_request_body_max_bytes", + skip_serializing_if = "Option::is_none" + )] + request_body_max_bytes: Option, + default_handler_task_mode: HandlerTaskMode, + log_headers: Vec, +} + +impl From for ConfigDropshot { + fn from(v: DeserializedConfigDropshot) -> Self { + ConfigDropshot { + bind_address: v.bind_address, + default_request_body_max_bytes: v.default_request_body_max_bytes, + default_handler_task_mode: v.default_handler_task_mode, + log_headers: v.log_headers, + } + } +} + +impl From for DeserializedConfigDropshot { + fn from(v: ConfigDropshot) -> Self { + DeserializedConfigDropshot { + bind_address: v.bind_address, + default_request_body_max_bytes: v.default_request_body_max_bytes, + request_body_max_bytes: None, + default_handler_task_mode: v.default_handler_task_mode, + log_headers: v.log_headers, + } + } +} + +impl Default for DeserializedConfigDropshot { + fn default() -> Self { + ConfigDropshot::default().into() + } +} + +/// A marker type to indicate that the configuration is invalid. +/// +/// This type can never be constructed, which means that for any valid config, +/// `Option` is always none. +#[derive(Clone, Debug, PartialEq, Serialize)] +pub enum InvalidConfig {} + +fn deserialize_invalid_request_body_max_bytes<'de, D>( + deserializer: D, +) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + deserialize_invalid( + deserializer, + "request_body_max_bytes has been renamed to \ + default_request_body_max_bytes", + ) +} + +fn deserialize_invalid<'de, D>( + deserializer: D, + msg: &'static str, +) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + use serde::de::Error; + + struct V { + msg: &'static str, + } + + impl<'de> serde::de::Visitor<'de> for V { + type Value = Option; + + fn expecting( + &self, + formatter: &mut std::fmt::Formatter, + ) -> std::fmt::Result { + write!(formatter, "the field to be absent ({})", self.msg) + } + + fn visit_some(self, _: D) -> Result + where + D: serde::Deserializer<'de>, + { + Err(D::Error::custom(self.msg)) + } + } + + deserializer.deserialize_any(V { msg }) +} diff --git a/dropshot/src/extractor/body.rs b/dropshot/src/extractor/body.rs index db474f097..07f902d89 100644 --- a/dropshot/src/extractor/body.rs +++ b/dropshot/src/extractor/body.rs @@ -128,9 +128,8 @@ async fn http_request_load_body( where BodyType: JsonSchema + DeserializeOwned + Send + Sync, { - let server = &rqctx.server; let (parts, body) = request.into_parts(); - let body = StreamingBody::new(body, server.config.request_body_max_bytes) + let body = StreamingBody::new(body, rqctx.request_body_max_bytes()) .into_bytes_mut() .await?; @@ -262,10 +261,9 @@ impl ExclusiveExtractor for UntypedBody { rqctx: &RequestContext, request: hyper::Request, ) -> Result { - let server = &rqctx.server; let body = request.into_body(); let body_bytes = - StreamingBody::new(body, server.config.request_body_max_bytes) + StreamingBody::new(body, rqctx.request_body_max_bytes()) .into_bytes_mut() .await?; Ok(UntypedBody { content: body_bytes.freeze() }) @@ -425,11 +423,9 @@ impl ExclusiveExtractor for StreamingBody { rqctx: &RequestContext, request: hyper::Request, ) -> Result { - let server = &rqctx.server; - Ok(Self { body: request.into_body(), - cap: server.config.request_body_max_bytes, + cap: rqctx.request_body_max_bytes(), }) } diff --git a/dropshot/src/handler.rs b/dropshot/src/handler.rs index 7e17f74aa..c04056e34 100644 --- a/dropshot/src/handler.rs +++ b/dropshot/src/handler.rs @@ -176,6 +176,11 @@ impl RequestContext { &self.server.private } + /// Returns the maximum request body size. + pub fn request_body_max_bytes(&self) -> usize { + self.server.config.default_request_body_max_bytes + } + /// Returns the appropriate count of items to return for a paginated request /// /// This first looks at any client-requested limit and clamps it based on the diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index ab6135bbf..3137b8b38 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -86,7 +86,7 @@ impl DropshotState { #[derive(Debug)] pub struct ServerConfig { /// maximum allowed size of a request body - pub request_body_max_bytes: usize, + pub default_request_body_max_bytes: usize, /// maximum size of any page of results pub page_max_nitems: NonZeroU32, /// default size for a page of results @@ -178,7 +178,8 @@ impl HttpServerStarter { let server_config = ServerConfig { // We start aggressively to ensure test coverage. - request_body_max_bytes: config.request_body_max_bytes, + default_request_body_max_bytes: config + .default_request_body_max_bytes, page_max_nitems: NonZeroU32::new(10000).unwrap(), page_default_nitems: NonZeroU32::new(100).unwrap(), default_handler_task_mode: config.default_handler_task_mode, diff --git a/dropshot/src/websocket.rs b/dropshot/src/websocket.rs index d184e4120..84c6c9296 100644 --- a/dropshot/src/websocket.rs +++ b/dropshot/src/websocket.rs @@ -379,7 +379,7 @@ mod tests { server: Arc::new(DropshotState { private: (), config: ServerConfig { - request_body_max_bytes: 0, + default_request_body_max_bytes: 0, page_max_nitems: NonZeroU32::new(1).unwrap(), page_default_nitems: NonZeroU32::new(1).unwrap(), default_handler_task_mode: diff --git a/dropshot/tests/test_api_trait.rs b/dropshot/tests/integration-tests/api_trait.rs similarity index 100% rename from dropshot/tests/test_api_trait.rs rename to dropshot/tests/integration-tests/api_trait.rs diff --git a/dropshot/tests/common/mod.rs b/dropshot/tests/integration-tests/common.rs similarity index 100% rename from dropshot/tests/common/mod.rs rename to dropshot/tests/integration-tests/common.rs diff --git a/dropshot/tests/test_config.rs b/dropshot/tests/integration-tests/config.rs similarity index 90% rename from dropshot/tests/test_config.rs rename to dropshot/tests/integration-tests/config.rs index f2b71d7b9..092653ba6 100644 --- a/dropshot/tests/test_config.rs +++ b/dropshot/tests/integration-tests/config.rs @@ -16,8 +16,59 @@ use std::sync::atomic::{AtomicU16, Ordering}; use tempfile::NamedTempFile; use tokio::sync::mpsc; -pub mod common; -use common::create_log_context; +use crate::common::{self, create_log_context}; + +#[test] +fn test_valid_config_empty() { + let parsed = + read_config::("valid_config_empty", "").unwrap(); + assert_eq!(parsed, ConfigDropshot::default()); +} + +#[test] +fn test_valid_config_basic() { + // Ensure that a basic config which leaves optional fields unset is + // correctly parsed. + let parsed = read_config::( + "valid_config_basic", + r#" + bind_address = "127.0.0.1:12345" + "#, + ) + .unwrap(); + + assert_eq!( + parsed, + ConfigDropshot { + bind_address: "127.0.0.1:12345".parse().unwrap(), + ..ConfigDropshot::default() + } + ); +} + +#[test] +fn test_valid_config_all_settings() { + let parsed = read_config::( + "valid_config_basic", + r#" + bind_address = "127.0.0.1:12345" + default_request_body_max_bytes = 1048576 + default_handler_task_mode = "cancel-on-disconnect" + log_headers = ["X-Forwarded-For"] + "#, + ) + .unwrap(); + + assert_eq!( + parsed, + ConfigDropshot { + bind_address: "127.0.0.1:12345".parse().unwrap(), + default_request_body_max_bytes: 1048576, + default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, + log_headers: vec!["X-Forwarded-For".to_string()], + }, + ); +} // Bad values for "bind_address" @@ -63,7 +114,7 @@ fn test_config_bad_bind_address_garbage() { fn test_config_bad_request_body_max_bytes_negative() { let error = read_config::( "bad_request_body_max_bytes_negative", - "request_body_max_bytes = -1024", + "default_request_body_max_bytes = -1024", ) .unwrap_err() .to_string(); @@ -75,7 +126,7 @@ fn test_config_bad_request_body_max_bytes_negative() { fn test_config_bad_request_body_max_bytes_too_large() { let error = read_config::( "bad_request_body_max_bytes_too_large", - "request_body_max_bytes = 999999999999999999999999999999", + "default_request_body_max_bytes = 999999999999999999999999999999", ) .unwrap_err() .to_string(); @@ -83,6 +134,22 @@ fn test_config_bad_request_body_max_bytes_too_large() { assert!(error.starts_with("")); } +#[test] +fn test_config_deprecated_request_body_max_bytes() { + let error = read_config::( + "deprecated_request_body_max_bytes", + "request_body_max_bytes = 1024", + ) + .unwrap_err(); + assert_eq!( + error.message(), + "invalid type: integer `1024`, \ + expected the field to be absent \ + (request_body_max_bytes has been renamed to \ + default_request_body_max_bytes)", + ); +} + fn make_server( context: T, config: &ConfigDropshot, @@ -111,7 +178,7 @@ fn make_config( std::net::IpAddr::from_str(bind_ip_str).unwrap(), bind_port, ), - request_body_max_bytes: 1024, + default_request_body_max_bytes: 1024, default_handler_task_mode, log_headers: Default::default(), } diff --git a/dropshot/tests/test_demo.rs b/dropshot/tests/integration-tests/demo.rs similarity index 99% rename from dropshot/tests/test_demo.rs rename to dropshot/tests/integration-tests/demo.rs index 6d26d047e..4c692c1c7 100644 --- a/dropshot/tests/test_demo.rs +++ b/dropshot/tests/integration-tests/demo.rs @@ -58,9 +58,9 @@ use tokio_tungstenite::tungstenite::Message; use tokio_tungstenite::WebSocketStream; use uuid::Uuid; -extern crate slog; +use crate::common; -pub mod common; +extern crate slog; fn demo_api() -> ApiDescription { let mut api = ApiDescription::::new(); diff --git a/dropshot/tests/test_detached_shutdown.rs b/dropshot/tests/integration-tests/detached_shutdown.rs similarity index 99% rename from dropshot/tests/test_detached_shutdown.rs rename to dropshot/tests/integration-tests/detached_shutdown.rs index 2c11f7fbe..edfa66825 100644 --- a/dropshot/tests/test_detached_shutdown.rs +++ b/dropshot/tests/integration-tests/detached_shutdown.rs @@ -11,7 +11,7 @@ use http::{Method, Response, StatusCode}; use std::time::Duration; use tokio::sync::mpsc; -pub mod common; +use crate::common; struct Context { endpoint_started_tx: mpsc::UnboundedSender<()>, diff --git a/dropshot/tests/integration-tests/main.rs b/dropshot/tests/integration-tests/main.rs new file mode 100644 index 000000000..b9f6468c5 --- /dev/null +++ b/dropshot/tests/integration-tests/main.rs @@ -0,0 +1,25 @@ +// Copyright 2024 Oxide Computer Company + +//! Integration tests for Dropshot. +//! +//! These are all combined into the same file to ensure that a single binary is +//! generated, speeding up link times. + +#[macro_use] +extern crate slog; +#[macro_use] +extern crate lazy_static; + +mod api_trait; +mod common; +mod config; +mod demo; +mod detached_shutdown; +mod multipart; +mod openapi; +mod pagination; +mod pagination_schema; +mod path_names; +mod starter; +mod streaming; +mod tls; diff --git a/dropshot/tests/test_multipart.rs b/dropshot/tests/integration-tests/multipart.rs similarity index 99% rename from dropshot/tests/test_multipart.rs rename to dropshot/tests/integration-tests/multipart.rs index 3728ce501..2ec40734e 100644 --- a/dropshot/tests/test_multipart.rs +++ b/dropshot/tests/integration-tests/multipart.rs @@ -8,9 +8,9 @@ use dropshot::{ }; use http::{Method, Response, StatusCode}; -extern crate slog; +use crate::common; -pub mod common; +extern crate slog; fn api() -> ApiDescription { let mut api = ApiDescription::new(); diff --git a/dropshot/tests/test_openapi.rs b/dropshot/tests/integration-tests/openapi.rs similarity index 100% rename from dropshot/tests/test_openapi.rs rename to dropshot/tests/integration-tests/openapi.rs diff --git a/dropshot/tests/test_pagination.rs b/dropshot/tests/integration-tests/pagination.rs similarity index 99% rename from dropshot/tests/test_pagination.rs rename to dropshot/tests/integration-tests/pagination.rs index 51d3d3ab0..3e443fa70 100644 --- a/dropshot/tests/test_pagination.rs +++ b/dropshot/tests/integration-tests/pagination.rs @@ -46,12 +46,7 @@ use subprocess::NullFile; use subprocess::Popen; use uuid::Uuid; -#[macro_use] -extern crate slog; -#[macro_use] -extern crate lazy_static; - -pub mod common; +use crate::common; // Common helpers @@ -571,7 +566,7 @@ lazy_static! { } fn make_word_list() -> BTreeSet { - let word_list = include_str!("wordlist.txt"); + let word_list = include_str!("../wordlist.txt"); word_list.lines().map(|s| s.to_string()).collect() } diff --git a/dropshot/tests/test_pagination_schema.rs b/dropshot/tests/integration-tests/pagination_schema.rs similarity index 100% rename from dropshot/tests/test_pagination_schema.rs rename to dropshot/tests/integration-tests/pagination_schema.rs diff --git a/dropshot/tests/test_path_names.rs b/dropshot/tests/integration-tests/path_names.rs similarity index 100% rename from dropshot/tests/test_path_names.rs rename to dropshot/tests/integration-tests/path_names.rs diff --git a/dropshot/tests/test_starter.rs b/dropshot/tests/integration-tests/starter.rs similarity index 98% rename from dropshot/tests/test_starter.rs rename to dropshot/tests/integration-tests/starter.rs index bb90ef522..a1fa4b0c9 100644 --- a/dropshot/tests/test_starter.rs +++ b/dropshot/tests/integration-tests/starter.rs @@ -3,9 +3,6 @@ //! Quick check that the "legacy" HttpServerStarter::new() and //! HttpServerStarter::new_with_tls() interfaces work. -pub mod common; - -use common::create_log_context; use dropshot::endpoint; use dropshot::test_util::read_json; use dropshot::test_util::ClientTestContext; @@ -17,6 +14,9 @@ use dropshot::HttpResponseOk; use dropshot::HttpServerStarter; use dropshot::RequestContext; +use crate::common; +use crate::common::create_log_context; + extern crate slog; /// Test starting a server with `HttpServerStarter::new()`. diff --git a/dropshot/tests/test_streaming.rs b/dropshot/tests/integration-tests/streaming.rs similarity index 99% rename from dropshot/tests/test_streaming.rs rename to dropshot/tests/integration-tests/streaming.rs index fffa51bef..718fd8f2e 100644 --- a/dropshot/tests/test_streaming.rs +++ b/dropshot/tests/integration-tests/streaming.rs @@ -8,9 +8,9 @@ use http::{Method, Response, StatusCode}; use http_body_util::BodyExt; use tokio::io::{AsyncSeekExt, AsyncWriteExt}; -extern crate slog; +use crate::common; -pub mod common; +extern crate slog; fn api() -> ApiDescription { let mut api = ApiDescription::new(); diff --git a/dropshot/tests/test_tls.rs b/dropshot/tests/integration-tests/tls.rs similarity index 99% rename from dropshot/tests/test_tls.rs rename to dropshot/tests/integration-tests/tls.rs index 9ac700ca9..ad63e760c 100644 --- a/dropshot/tests/test_tls.rs +++ b/dropshot/tests/integration-tests/tls.rs @@ -13,10 +13,7 @@ use std::path::Path; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; -pub mod common; -use common::create_log_context; - -use crate::common::generate_tls_key; +use crate::common::{self, create_log_context, generate_tls_key}; /// See rustls::client::ServerCertVerifier::verify_server_cert for argument /// meanings @@ -118,7 +115,7 @@ fn make_server( ) -> HttpServer { let config = ConfigDropshot { bind_address: "127.0.0.1:0".parse().unwrap(), - request_body_max_bytes: 1024, + default_request_body_max_bytes: 1024, default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, log_headers: Default::default(), }; @@ -432,7 +429,7 @@ async fn test_server_is_https() { let config = ConfigDropshot { bind_address: "127.0.0.1:0".parse().unwrap(), - request_body_max_bytes: 1024, + default_request_body_max_bytes: 1024, default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, log_headers: Default::default(), }; From 0e62868affa1daede4c2f76d2b788f53ee05d95c Mon Sep 17 00:00:00 2001 From: Rain Date: Sat, 16 Nov 2024 20:29:19 +0000 Subject: [PATCH 2/2] fmt Created using spr 1.3.6-beta.1 --- dropshot/src/extractor/body.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dropshot/src/extractor/body.rs b/dropshot/src/extractor/body.rs index 3279a3609..144e7415b 100644 --- a/dropshot/src/extractor/body.rs +++ b/dropshot/src/extractor/body.rs @@ -153,8 +153,7 @@ where let body_content_type = ApiEndpointBodyContentType::from_mime_type(&mime_type) .map_err(|e| HttpError::for_bad_request(None, e))?; - let expected_content_type = - rqctx.endpoint.body_content_type.clone(); + let expected_content_type = rqctx.endpoint.body_content_type.clone(); use ApiEndpointBodyContentType::*;