From a8cfa4d65719fd7a614181eafa294522815fe5d5 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 12 Nov 2024 10:31:22 +0000 Subject: [PATCH 1/4] [spr] changes to main this commit is based on Created using spr 1.3.6-beta.1 [skip ci] --- .../api_trait.rs} | 0 .../mod.rs => integration-tests/common.rs} | 0 .../config.rs} | 3 +-- .../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} | 5 +--- 14 files changed, 39 insertions(+), 23 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} (99%) 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/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 99% rename from dropshot/tests/test_config.rs rename to dropshot/tests/integration-tests/config.rs index f2b71d7b9..de2221bb8 100644 --- a/dropshot/tests/test_config.rs +++ b/dropshot/tests/integration-tests/config.rs @@ -16,8 +16,7 @@ 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}; // Bad values for "bind_address" 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..d6c27bfc4 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 From d33aff71cf4257e2b1e21e92491fd6b6d474e653 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 12 Nov 2024 10:57:21 +0000 Subject: [PATCH 2/4] [spr] changes introduced through rebase Created using spr 1.3.6-beta.1 [skip ci] --- dropshot/tests/integration-tests/config.rs | 52 ++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/dropshot/tests/integration-tests/config.rs b/dropshot/tests/integration-tests/config.rs index de2221bb8..b555e5efc 100644 --- a/dropshot/tests/integration-tests/config.rs +++ b/dropshot/tests/integration-tests/config.rs @@ -18,6 +18,58 @@ use tokio::sync::mpsc; 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" + 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(), + request_body_max_bytes: 1048576, + default_handler_task_mode: HandlerTaskMode::CancelOnDisconnect, + log_headers: vec!["X-Forwarded-For".to_string()], + }, + ); +} + // Bad values for "bind_address" #[test] From 9b02b18c94f6028ccd25dac2414a7575383d4698 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 12 Nov 2024 23:23:02 +0000 Subject: [PATCH 3/4] [spr] changes introduced through rebase 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 | 13 +-- dropshot/src/extractor/path.rs | 3 +- dropshot/src/handler.rs | 29 ++++-- dropshot/src/lib.rs | 1 + dropshot/src/router.rs | 69 +++++++------ dropshot/src/server.rs | 9 +- dropshot/src/websocket.rs | 14 +-- dropshot/tests/integration-tests/config.rs | 26 ++++- dropshot/tests/integration-tests/tls.rs | 4 +- 12 files changed, 217 insertions(+), 73 deletions(-) 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..a6c29c873 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?; @@ -154,7 +153,8 @@ 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.body_content_type.clone(); + let expected_content_type = + rqctx.endpoint_metadata.body_content_type.clone(); use ApiEndpointBodyContentType::*; @@ -262,10 +262,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 +424,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/extractor/path.rs b/dropshot/src/extractor/path.rs index 24fb25044..c7521b531 100644 --- a/dropshot/src/extractor/path.rs +++ b/dropshot/src/extractor/path.rs @@ -43,7 +43,8 @@ where async fn from_request( rqctx: &RequestContext, ) -> Result, HttpError> { - let params: PathType = http_extract_path_params(&rqctx.path_variables)?; + let params: PathType = + http_extract_path_params(&rqctx.endpoint_metadata.variables)?; Ok(Path { inner: params }) } diff --git a/dropshot/src/handler.rs b/dropshot/src/handler.rs index 7e17f74aa..242a959b4 100644 --- a/dropshot/src/handler.rs +++ b/dropshot/src/handler.rs @@ -73,19 +73,16 @@ pub type HttpHandlerResult = Result, HttpError>; /// Handle for various interfaces useful during request processing. #[derive(Debug)] +#[non_exhaustive] pub struct RequestContext { /// shared server state pub server: Arc>, - /// HTTP request routing variables - pub path_variables: VariableSet, - /// expected request body mime type - pub body_content_type: ApiEndpointBodyContentType, + /// Endpoint-specific information. + pub endpoint_metadata: RequestEndpointMetadata, /// unique id assigned to this request pub request_id: String, /// logger for this specific request pub log: Logger, - /// The operation ID for the endpoint handler method - pub operation_id: String, /// basic request information (method, URI, etc.) pub request: RequestInfo, } @@ -176,6 +173,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 @@ -203,6 +205,21 @@ impl RequestContext { } } +/// Endpoint-specific information for a request. +/// +/// This is part of [`RequestContext`] and [`RouterLookupResult`]. +#[derive(Debug)] +pub struct RequestEndpointMetadata { + /// The operation ID for the endpoint handler method. + pub operation_id: String, + + /// HTTP request routing variables. + pub variables: VariableSet, + + /// The expected request body MIME type. + pub body_content_type: ApiEndpointBodyContentType, +} + /// Helper trait for extracting the underlying Context type from the /// first argument to an endpoint. This trait exists to help the /// endpoint macro parse this argument. diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 96aa20d9f..360948a58 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -811,6 +811,7 @@ pub use handler::HttpResponseTemporaryRedirect; pub use handler::HttpResponseUpdatedNoContent; pub use handler::NoHeaders; pub use handler::RequestContext; +pub use handler::RequestEndpointMetadata; pub use handler::RequestInfo; pub use http_util::CONTENT_TYPE_JSON; pub use http_util::CONTENT_TYPE_MULTIPART_FORM_DATA; diff --git a/dropshot/src/router.rs b/dropshot/src/router.rs index ed3af536d..98ba5e51a 100644 --- a/dropshot/src/router.rs +++ b/dropshot/src/router.rs @@ -8,7 +8,7 @@ use crate::from_map::MapError; use crate::from_map::MapValue; use crate::server::ServerContext; use crate::ApiEndpoint; -use crate::ApiEndpointBodyContentType; +use crate::RequestEndpointMetadata; use http::Method; use http::StatusCode; use percent_encoding::percent_decode_str; @@ -202,17 +202,13 @@ impl MapValue for VariableValue { } } -/// `RouterLookupResult` represents the result of invoking -/// `HttpRouter::lookup_route()`. A successful route lookup includes -/// the handler, a mapping of variables in the configured path to the -/// corresponding values in the actual path, and the expected body -/// content type. +/// The result of invoking `HttpRouter::lookup_route()`. +/// +/// A successful route lookup includes the handler and endpoint-related metadata. #[derive(Debug)] pub struct RouterLookupResult { pub handler: Arc>, - pub operation_id: String, - pub variables: VariableSet, - pub body_content_type: ApiEndpointBodyContentType, + pub metadata: RequestEndpointMetadata, } impl HttpRouterNode { @@ -483,9 +479,11 @@ impl HttpRouter { .get(&methodname) .map(|handler| RouterLookupResult { handler: Arc::clone(&handler.handler), - operation_id: handler.operation_id.clone(), - variables, - body_content_type: handler.body_content_type.clone(), + metadata: RequestEndpointMetadata { + operation_id: handler.operation_id.clone(), + variables, + body_content_type: handler.body_content_type.clone(), + }, }) .ok_or_else(|| { HttpError::for_status(None, StatusCode::METHOD_NOT_ALLOWED) @@ -1033,13 +1031,13 @@ mod test { router.insert(new_endpoint(new_handler_named("h1"), Method::GET, "/")); let result = router.lookup_route(&Method::GET, "/".into()).unwrap(); assert_eq!(result.handler.label(), "h1"); - assert!(result.variables.is_empty()); + assert!(result.metadata.variables.is_empty()); let result = router.lookup_route(&Method::GET, "//".into()).unwrap(); assert_eq!(result.handler.label(), "h1"); - assert!(result.variables.is_empty()); + assert!(result.metadata.variables.is_empty()); let result = router.lookup_route(&Method::GET, "///".into()).unwrap(); assert_eq!(result.handler.label(), "h1"); - assert!(result.variables.is_empty()); + assert!(result.metadata.variables.is_empty()); // Now insert a handler for a different method at the root. Verify that // we get both this handler and the previous one if we ask for the @@ -1049,11 +1047,11 @@ mod test { router.insert(new_endpoint(new_handler_named("h2"), Method::PUT, "/")); let result = router.lookup_route(&Method::PUT, "/".into()).unwrap(); assert_eq!(result.handler.label(), "h2"); - assert!(result.variables.is_empty()); + assert!(result.metadata.variables.is_empty()); let result = router.lookup_route(&Method::GET, "/".into()).unwrap(); assert_eq!(result.handler.label(), "h1"); assert!(router.lookup_route(&Method::DELETE, "/".into()).is_err()); - assert!(result.variables.is_empty()); + assert!(result.metadata.variables.is_empty()); // Now insert a handler one level deeper. Verify that all the previous // handlers behave as we expect, and that we have one handler at the new @@ -1066,24 +1064,24 @@ mod test { )); let result = router.lookup_route(&Method::PUT, "/".into()).unwrap(); assert_eq!(result.handler.label(), "h2"); - assert!(result.variables.is_empty()); + assert!(result.metadata.variables.is_empty()); let result = router.lookup_route(&Method::GET, "/".into()).unwrap(); assert_eq!(result.handler.label(), "h1"); - assert!(result.variables.is_empty()); + assert!(result.metadata.variables.is_empty()); let result = router.lookup_route(&Method::GET, "/foo".into()).unwrap(); assert_eq!(result.handler.label(), "h3"); - assert!(result.variables.is_empty()); + assert!(result.metadata.variables.is_empty()); let result = router.lookup_route(&Method::GET, "/foo/".into()).unwrap(); assert_eq!(result.handler.label(), "h3"); - assert!(result.variables.is_empty()); + assert!(result.metadata.variables.is_empty()); let result = router.lookup_route(&Method::GET, "//foo//".into()).unwrap(); assert_eq!(result.handler.label(), "h3"); - assert!(result.variables.is_empty()); + assert!(result.metadata.variables.is_empty()); let result = router.lookup_route(&Method::GET, "/foo//".into()).unwrap(); assert_eq!(result.handler.label(), "h3"); - assert!(result.variables.is_empty()); + assert!(result.metadata.variables.is_empty()); assert!(router.lookup_route(&Method::PUT, "/foo".into()).is_err()); assert!(router.lookup_route(&Method::PUT, "/foo/".into()).is_err()); assert!(router.lookup_route(&Method::PUT, "//foo//".into()).is_err()); @@ -1107,7 +1105,7 @@ mod test { .lookup_route(&Method::GET, "/not{a}variable".into()) .unwrap(); assert_eq!(result.handler.label(), "h4"); - assert!(result.variables.is_empty()); + assert!(result.metadata.variables.is_empty()); assert!(router .lookup_route(&Method::GET, "/not{b}variable".into()) .is_err()); @@ -1134,11 +1132,11 @@ mod test { .unwrap(); assert_eq!(result.handler.label(), "h5"); assert_eq!( - result.variables.keys().collect::>(), + result.metadata.variables.keys().collect::>(), vec!["project_id"] ); assert_eq!( - *result.variables.get("project_id").unwrap(), + *result.metadata.variables.get("project_id").unwrap(), VariableValue::String("p12345".to_string()) ); assert!(router @@ -1149,7 +1147,7 @@ mod test { .unwrap(); assert_eq!(result.handler.label(), "h5"); assert_eq!( - *result.variables.get("project_id").unwrap(), + *result.metadata.variables.get("project_id").unwrap(), VariableValue::String("p12345".to_string()) ); let result = router @@ -1157,7 +1155,7 @@ mod test { .unwrap(); assert_eq!(result.handler.label(), "h5"); assert_eq!( - *result.variables.get("project_id").unwrap(), + *result.metadata.variables.get("project_id").unwrap(), VariableValue::String("p12345".to_string()) ); // Trick question! @@ -1166,7 +1164,7 @@ mod test { .unwrap(); assert_eq!(result.handler.label(), "h5"); assert_eq!( - *result.variables.get("project_id").unwrap(), + *result.metadata.variables.get("project_id").unwrap(), VariableValue::String("{project_id}".to_string()) ); } @@ -1189,19 +1187,19 @@ mod test { .unwrap(); assert_eq!(result.handler.label(), "h6"); assert_eq!( - result.variables.keys().collect::>(), + result.metadata.variables.keys().collect::>(), vec!["fwrule_id", "instance_id", "project_id"] ); assert_eq!( - *result.variables.get("project_id").unwrap(), + *result.metadata.variables.get("project_id").unwrap(), VariableValue::String("p1".to_string()) ); assert_eq!( - *result.variables.get("instance_id").unwrap(), + *result.metadata.variables.get("instance_id").unwrap(), VariableValue::String("i2".to_string()) ); assert_eq!( - *result.variables.get("fwrule_id").unwrap(), + *result.metadata.variables.get("fwrule_id").unwrap(), VariableValue::String("fw3".to_string()) ); } @@ -1245,7 +1243,7 @@ mod test { .unwrap(); assert_eq!( - result.variables.get("path"), + result.metadata.variables.get("path"), Some(&VariableValue::Components(vec![ "missiles".to_string(), "launch".to_string() @@ -1278,7 +1276,8 @@ mod test { .unwrap(); let path = - from_map::(&result.variables).unwrap(); + from_map::(&result.metadata.variables) + .unwrap(); assert_eq!(path.t, "console"); assert_eq!(path.r, "missiles"); diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index ab6135bbf..4b94bab94 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, @@ -890,9 +891,7 @@ async fn http_request_handle( let rqctx = RequestContext { server: Arc::clone(&server), request: RequestInfo::new(&request, remote_addr), - path_variables: lookup_result.variables, - body_content_type: lookup_result.body_content_type, - operation_id: lookup_result.operation_id, + endpoint_metadata: lookup_result.metadata, request_id: request_id.to_string(), log: request_log, }; diff --git a/dropshot/src/websocket.rs b/dropshot/src/websocket.rs index d184e4120..41e5030a8 100644 --- a/dropshot/src/websocket.rs +++ b/dropshot/src/websocket.rs @@ -353,8 +353,8 @@ mod tests { use crate::router::HttpRouter; use crate::server::{DropshotState, ServerConfig}; use crate::{ - ExclusiveExtractor, HttpError, RequestContext, RequestInfo, - WebsocketUpgrade, + ExclusiveExtractor, HttpError, RequestContext, RequestEndpointMetadata, + RequestInfo, WebsocketUpgrade, }; use debug_ignore::DebugIgnore; use http::Request; @@ -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: @@ -398,9 +398,11 @@ mod tests { ), }), request: RequestInfo::new(&request, remote_addr), - path_variables: Default::default(), - body_content_type: Default::default(), - operation_id: "".to_string(), + endpoint_metadata: RequestEndpointMetadata { + variables: Default::default(), + body_content_type: Default::default(), + operation_id: "".to_string(), + }, request_id: "".to_string(), log: log.clone(), }; diff --git a/dropshot/tests/integration-tests/config.rs b/dropshot/tests/integration-tests/config.rs index b555e5efc..092653ba6 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(), }; From 45e00a56108f13d52be7af4585e3b9a00af83623 Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 12 Nov 2024 23:24:50 +0000 Subject: [PATCH 4/4] update copyright Created using spr 1.3.6-beta.1 --- dropshot/tests/fail/bad_trait_channel28.rs | 2 +- dropshot/tests/fail/bad_trait_endpoint28.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dropshot/tests/fail/bad_trait_channel28.rs b/dropshot/tests/fail/bad_trait_channel28.rs index ca1ffc8ae..c76234730 100644 --- a/dropshot/tests/fail/bad_trait_channel28.rs +++ b/dropshot/tests/fail/bad_trait_channel28.rs @@ -1,4 +1,4 @@ -// Copyright 2020 Oxide Computer Company +// Copyright 2024 Oxide Computer Company #![allow(unused_imports)] diff --git a/dropshot/tests/fail/bad_trait_endpoint28.rs b/dropshot/tests/fail/bad_trait_endpoint28.rs index 2178ac887..a96a22e8b 100644 --- a/dropshot/tests/fail/bad_trait_endpoint28.rs +++ b/dropshot/tests/fail/bad_trait_endpoint28.rs @@ -1,4 +1,4 @@ -// Copyright 2020 Oxide Computer Company +// Copyright 2024 Oxide Computer Company #![allow(unused_imports)]