diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc index c81de2a43..3cac092f7 100644 --- a/CHANGELOG.adoc +++ b/CHANGELOG.adoc @@ -15,6 +15,12 @@ https://github.com/oxidecomputer/dropshot/compare/v0.13.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. + == 0.13.0 (released 2024-11-13) https://github.com/oxidecomputer/dropshot/compare/v0.12.0\...v0.13.0[Full list of commits] diff --git a/README.adoc b/README.adoc index e05d10c51..c46c27cc7 100644 --- a/README.adoc +++ b/README.adoc @@ -45,7 +45,7 @@ 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. diff --git a/dropshot/src/config.rs b/dropshot/src/config.rs index 40414ece9..b8a15db65 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,113 @@ 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, in Dropshot < 0.14. + // Present only to guide users to the new name. + #[serde( + deserialize_with = "deserialize_invalid_request_body_max_bytes", + skip_serializing + )] + 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)] +pub enum InvalidConfig {} + +// We prefer having a deserialize function over `impl Deserialize for +// InvalidConfig` for two reasons: +// +// 1. This returns an `Option`, not an `InvalidConfig`. +// 2. This way, the deserializer has a custom message associated with it. +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 c47e8a8f8..461e12160 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -89,7 +89,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 @@ -182,7 +182,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 1fb4cfec3..67cb07460 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/integration-tests/config.rs b/dropshot/tests/integration-tests/config.rs index dc3a4f5b4..e62660e70 100644 --- a/dropshot/tests/integration-tests/config.rs +++ b/dropshot/tests/integration-tests/config.rs @@ -52,7 +52,7 @@ fn test_valid_config_all_settings() { "valid_config_basic", r#" bind_address = "127.0.0.1:12345" - request_body_max_bytes = 1048576 + default_request_body_max_bytes = 1048576 default_handler_task_mode = "cancel-on-disconnect" log_headers = ["X-Forwarded-For"] "#, @@ -63,7 +63,7 @@ fn test_valid_config_all_settings() { parsed, ConfigDropshot { bind_address: "127.0.0.1:12345".parse().unwrap(), - request_body_max_bytes: 1048576, + default_request_body_max_bytes: 1048576, default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, log_headers: vec!["X-Forwarded-For".to_string()], }, @@ -114,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(); @@ -126,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(); @@ -134,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, @@ -162,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/integration-tests/tls.rs b/dropshot/tests/integration-tests/tls.rs index d6c27bfc4..ad63e760c 100644 --- a/dropshot/tests/integration-tests/tls.rs +++ b/dropshot/tests/integration-tests/tls.rs @@ -115,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(), }; @@ -429,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(), };