From 951fb6c4efb5ea71638c572ff55f2b90417eb5fc Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 12 Jan 2023 13:01:40 -0800 Subject: [PATCH] rename the new pub type (requires moving the internal DTrace type with the same name) add demo test fix up docs a bit --- dropshot/src/dtrace.rs | 42 ++++++++++++++ dropshot/src/extractor/mod.rs | 4 +- dropshot/src/handler.rs | 11 ++-- dropshot/src/lib.rs | 67 ++++++----------------- dropshot/src/server.rs | 14 ++--- dropshot/src/websocket.rs | 4 +- dropshot/tests/fail/bad_endpoint1.stderr | 1 + dropshot/tests/fail/bad_endpoint11.stderr | 1 + dropshot/tests/fail/bad_endpoint13.stderr | 1 + dropshot/tests/fail/bad_endpoint2.stderr | 1 + dropshot/tests/fail/bad_endpoint8.stderr | 1 + dropshot/tests/test_demo.rs | 60 +++++++++++++++++++- dropshot_endpoint/src/lib.rs | 1 + 13 files changed, 139 insertions(+), 69 deletions(-) create mode 100644 dropshot/src/dtrace.rs diff --git a/dropshot/src/dtrace.rs b/dropshot/src/dtrace.rs new file mode 100644 index 00000000..3f6a9d41 --- /dev/null +++ b/dropshot/src/dtrace.rs @@ -0,0 +1,42 @@ +// Copyright 2023 Oxide Computer Company +//! DTrace probes and support + +#[derive(Debug, Clone, serde::Serialize)] +pub(crate) struct RequestInfo { + id: String, + local_addr: std::net::SocketAddr, + remote_addr: std::net::SocketAddr, + method: String, + path: String, + query: Option, +} + +#[derive(Debug, Clone, serde::Serialize)] +pub(crate) struct ResponseInfo { + id: String, + local_addr: std::net::SocketAddr, + remote_addr: std::net::SocketAddr, + status_code: u16, + message: String, +} + +#[cfg(feature = "usdt-probes")] +#[usdt::provider(provider = "dropshot")] +mod probes { + use super::{RequestInfo, ResponseInfo}; + fn request__start(_: &RequestInfo) {} + fn request__done(_: &ResponseInfo) {} +} + +/// The result of registering a server's DTrace USDT probes. +#[derive(Debug, Clone, PartialEq)] +pub enum ProbeRegistration { + /// The probes are explicitly disabled at compile time. + Disabled, + + /// Probes were successfully registered. + Succeeded, + + /// Registration failed, with an error message explaining the cause. + Failed(String), +} diff --git a/dropshot/src/extractor/mod.rs b/dropshot/src/extractor/mod.rs index d659f683..a580c28a 100644 --- a/dropshot/src/extractor/mod.rs +++ b/dropshot/src/extractor/mod.rs @@ -31,7 +31,7 @@ use std::fmt::Debug; mod common; -use crate::RequestHeader; +use crate::RequestInfo; pub use common::ExclusiveExtractor; pub use common::ExtractorMetadata; pub use common::RequestExtractor; @@ -58,7 +58,7 @@ impl Query { /// Given an HTTP request, pull out the query string and attempt to deserialize /// it as an instance of `QueryType`. fn http_request_load_query( - request: &RequestHeader, + request: &RequestInfo, ) -> Result, HttpError> where QueryType: DeserializeOwned + JsonSchema + Send + Sync, diff --git a/dropshot/src/handler.rs b/dropshot/src/handler.rs index 4f4fc317..36056e50 100644 --- a/dropshot/src/handler.rs +++ b/dropshot/src/handler.rs @@ -85,23 +85,22 @@ pub struct RequestContext { pub log: Logger, /// basic request information (method, URI, etc.) - pub request: RequestHeader, + pub request: RequestInfo, } // This is deliberately as close to compatible with `hyper::Request` as // reasonable. -// XXX-dap TODO This could use a better name. #[derive(Debug)] -pub struct RequestHeader { +pub struct RequestInfo { method: http::Method, uri: http::Uri, version: http::Version, headers: http::HeaderMap, } -impl From<&hyper::Request> for RequestHeader { +impl From<&hyper::Request> for RequestInfo { fn from(request: &hyper::Request) -> Self { - RequestHeader { + RequestInfo { method: request.method().clone(), uri: request.uri().clone(), version: request.version().clone(), @@ -110,7 +109,7 @@ impl From<&hyper::Request> for RequestHeader { } } -impl RequestHeader { +impl RequestInfo { pub fn method(&self) -> &http::Method { &self.method } diff --git a/dropshot/src/lib.rs b/dropshot/src/lib.rs index 83dc475c..df01ca63 100644 --- a/dropshot/src/lib.rs +++ b/dropshot/src/lib.rs @@ -213,15 +213,16 @@ //! [path_params: Path

,] //! [body_param: TypedBody,] //! [body_param: UntypedBody,] +//! [raw_request: RawRequest,] //! ) -> Result //! ``` //! //! The `RequestContext` must appear first. The `Context` type is //! caller-provided context which is provided when the server is created. //! -//! The types `Query`, `Path`, `TypedBody`, and `UntypedBody` are called -//! **Extractors** because they cause information to be pulled out of the request -//! and made available to the handler function. +//! The types `Query`, `Path`, `TypedBody`, `UntypedBody`, and `RawRequest` are +//! called **Extractors** because they cause information to be pulled out of the +//! request and made available to the handler function. //! //! * [`Query`]`` extracts parameters from a query string, deserializing them //! into an instance of type `Q`. `Q` must implement `serde::Deserialize` and @@ -233,11 +234,14 @@ //! body as JSON (or form/url-encoded) and deserializing it into an instance //! of type `J`. `J` must implement `serde::Deserialize` and `schemars::JsonSchema`. //! * [`UntypedBody`] extracts the raw bytes of the request body. +//! * [`RawRequest`] provides access to the underlying [`hyper::Request`]. The +//! hope is that this would generally not be needed. It can be useful to +//! implement functionality not provided by Dropshot. //! -//! `Query` and `Path` impl `SharedExtractor`. `TypedBody` and `UntypedBody` -//! impl `ExclusiveExtractor`. Your function may accept 0-3 extractors, but -//! only one can be `ExclusiveExtractor`, and it must be the last one. -//! Otherwise, the order of extractor arguments does not matter. +//! `Query` and `Path` impl `SharedExtractor`. `TypedBody`, `UntypedBody`, and +//! `RawRequest` impl `ExclusiveExtractor`. Your function may accept 0-3 +//! extractors, but only one can be `ExclusiveExtractor`, and it must be the +//! last one. Otherwise, the order of extractor arguments does not matter. //! //! If the handler accepts any extractors and the corresponding extraction //! cannot be completed, the request fails with status code 400 and an error @@ -504,8 +508,8 @@ //! Dropshot optionally exposes two DTrace probes, `request_start` and //! `request_finish`. These provide detailed information about each request, //! such as their ID, the local and remote IPs, and the response information. -//! See the [`RequestInfo`] and [`ResponseInfo`] types for a complete listing -//! of what's available. +//! See the dropshot::dtrace::RequestInfo` and `dropshot::dtrae::ResponseInfo` +//! types for a complete listing of what's available. //! //! These probes are implemented via the [`usdt`] crate. They may require a //! nightly toolchain if built on macOS prior to Rust version 1.66. Otherwise a @@ -551,45 +555,9 @@ feature(asm_sym) )] -#[derive(Debug, Clone, serde::Serialize)] -pub(crate) struct RequestInfo { - id: String, - local_addr: std::net::SocketAddr, - remote_addr: std::net::SocketAddr, - method: String, - path: String, - query: Option, -} - -#[derive(Debug, Clone, serde::Serialize)] -pub(crate) struct ResponseInfo { - id: String, - local_addr: std::net::SocketAddr, - remote_addr: std::net::SocketAddr, - status_code: u16, - message: String, -} - -#[cfg(feature = "usdt-probes")] -#[usdt::provider(provider = "dropshot")] -mod probes { - use crate::{RequestInfo, ResponseInfo}; - fn request__start(_: &RequestInfo) {} - fn request__done(_: &ResponseInfo) {} -} - -/// The result of registering a server's DTrace USDT probes. -#[derive(Debug, Clone, PartialEq)] -pub enum ProbeRegistration { - /// The probes are explicitly disabled at compile time. - Disabled, - - /// Probes were successfully registered. - Succeeded, - - /// Registration failed, with an error message explaining the cause. - Failed(String), -} +// The macro used to define DTrace probes needs to be defined before anything +// that might use it. +mod dtrace; mod api_description; mod config; @@ -626,6 +594,7 @@ pub use api_description::TagDetails; pub use api_description::TagExternalDocs; pub use config::ConfigDropshot; pub use config::ConfigTls; +pub use dtrace::ProbeRegistration; pub use error::HttpError; pub use error::HttpErrorResponseBody; pub use extractor::ExclusiveExtractor; @@ -653,7 +622,7 @@ pub use handler::HttpResponseTemporaryRedirect; pub use handler::HttpResponseUpdatedNoContent; pub use handler::NoHeaders; pub use handler::RequestContext; -pub use handler::RequestHeader; +pub use handler::RequestInfo; pub use http_util::CONTENT_TYPE_JSON; pub use http_util::CONTENT_TYPE_NDJSON; pub use http_util::CONTENT_TYPE_OCTET_STREAM; diff --git a/dropshot/src/server.rs b/dropshot/src/server.rs index 0a250b93..380de0c8 100644 --- a/dropshot/src/server.rs +++ b/dropshot/src/server.rs @@ -1,13 +1,13 @@ -// Copyright 2020 Oxide Computer Company +// Copyright 2023 Oxide Computer Company //! Generic server-wide state and facilities use super::api_description::ApiDescription; use super::config::{ConfigDropshot, ConfigTls}; +#[cfg(feature = "usdt-probes")] +use super::dtrace::probes; use super::error::HttpError; use super::handler::RequestContext; use super::http_util::HEADER_REQUEST_ID; -#[cfg(feature = "usdt-probes")] -use super::probes; use super::router::HttpRouter; use super::ProbeRegistration; @@ -38,7 +38,7 @@ use tokio::net::{TcpListener, TcpStream}; use tokio_rustls::{server::TlsStream, TlsAcceptor}; use uuid::Uuid; -use crate::RequestHeader; +use crate::RequestInfo; use slog::Logger; // TODO Replace this with something else? @@ -679,7 +679,7 @@ async fn http_request_handle_wrap( #[cfg(feature = "usdt-probes")] probes::request__start!(|| { let uri = request.uri(); - crate::RequestInfo { + crate::dtrace::RequestInfo { id: request_id.clone(), local_addr: server.local_addr, remote_addr, @@ -710,7 +710,7 @@ async fn http_request_handle_wrap( #[cfg(feature = "usdt-probes")] probes::request__done!(|| { - crate::ResponseInfo { + crate::dtrace::ResponseInfo { id: request_id.clone(), local_addr, remote_addr, @@ -771,7 +771,7 @@ async fn http_request_handle( server.router.lookup_route(&method, uri.path().into())?; let rqctx = RequestContext { server: Arc::clone(&server), - request: RequestHeader::from(&request), + request: RequestInfo::from(&request), path_variables: lookup_result.variables, body_content_type: lookup_result.body_content_type, request_id: request_id.to_string(), diff --git a/dropshot/src/websocket.rs b/dropshot/src/websocket.rs index df63d2c2..262dd9ca 100644 --- a/dropshot/src/websocket.rs +++ b/dropshot/src/websocket.rs @@ -297,7 +297,7 @@ mod tests { use crate::router::HttpRouter; use crate::server::{DropshotState, ServerConfig}; use crate::{ - ExclusiveExtractor, HttpError, RequestContext, RequestHeader, + ExclusiveExtractor, HttpError, RequestContext, RequestInfo, WebsocketUpgrade, }; use http::Request; @@ -332,7 +332,7 @@ mod tests { ), tls_acceptor: None, }), - request: RequestHeader::from(&request), + request: RequestInfo::from(&request), path_variables: Default::default(), body_content_type: Default::default(), request_id: "".to_string(), diff --git a/dropshot/tests/fail/bad_endpoint1.stderr b/dropshot/tests/fail/bad_endpoint1.stderr index 7d1989a1..729ca36c 100644 --- a/dropshot/tests/fail/bad_endpoint1.stderr +++ b/dropshot/tests/fail/bad_endpoint1.stderr @@ -5,6 +5,7 @@ error: Endpoint handlers must have the following signature: [path_params: Path

,] [body_param: TypedBody,] [body_param: UntypedBody,] + [raw_request: RawRequest,] ) -> Result --> tests/fail/bad_endpoint1.rs:20:1 | diff --git a/dropshot/tests/fail/bad_endpoint11.stderr b/dropshot/tests/fail/bad_endpoint11.stderr index 1d4f19d8..7d39b1b7 100644 --- a/dropshot/tests/fail/bad_endpoint11.stderr +++ b/dropshot/tests/fail/bad_endpoint11.stderr @@ -5,6 +5,7 @@ error: Endpoint handlers must have the following signature: [path_params: Path

,] [body_param: TypedBody,] [body_param: UntypedBody,] + [raw_request: RawRequest,] ) -> Result --> tests/fail/bad_endpoint11.rs:13:1 | diff --git a/dropshot/tests/fail/bad_endpoint13.stderr b/dropshot/tests/fail/bad_endpoint13.stderr index 1559b41d..d209cd2b 100644 --- a/dropshot/tests/fail/bad_endpoint13.stderr +++ b/dropshot/tests/fail/bad_endpoint13.stderr @@ -5,6 +5,7 @@ error: Endpoint handlers must have the following signature: [path_params: Path

,] [body_param: TypedBody,] [body_param: UntypedBody,] + [raw_request: RawRequest,] ) -> Result --> tests/fail/bad_endpoint13.rs:19:1 | diff --git a/dropshot/tests/fail/bad_endpoint2.stderr b/dropshot/tests/fail/bad_endpoint2.stderr index c7120761..42e88eb5 100644 --- a/dropshot/tests/fail/bad_endpoint2.stderr +++ b/dropshot/tests/fail/bad_endpoint2.stderr @@ -5,6 +5,7 @@ error: Endpoint handlers must have the following signature: [path_params: Path

,] [body_param: TypedBody,] [body_param: UntypedBody,] + [raw_request: RawRequest,] ) -> Result --> tests/fail/bad_endpoint2.rs:13:1 | diff --git a/dropshot/tests/fail/bad_endpoint8.stderr b/dropshot/tests/fail/bad_endpoint8.stderr index dc606708..f1fede5a 100644 --- a/dropshot/tests/fail/bad_endpoint8.stderr +++ b/dropshot/tests/fail/bad_endpoint8.stderr @@ -5,6 +5,7 @@ error: Endpoint handlers must have the following signature: [path_params: Path

,] [body_param: TypedBody,] [body_param: UntypedBody,] + [raw_request: RawRequest,] ) -> Result --> tests/fail/bad_endpoint8.rs:20:1 | diff --git a/dropshot/tests/test_demo.rs b/dropshot/tests/test_demo.rs index b28c7856..9155d130 100644 --- a/dropshot/tests/test_demo.rs +++ b/dropshot/tests/test_demo.rs @@ -6,9 +6,9 @@ //! //! Note that the purpose is mainly to exercise the various possible function //! signatures that can be used to implement handler functions. We don't need to -//! exercises very many cases (or error cases) of each one because the handlers -//! themselves are not important, but we need to exercise enough to validate that -//! the generic JSON and query parsing handles error cases. +//! exercise very many cases (or error cases) of each one because the handlers +//! themselves are not important, but we need to exercise enough to validate +//! that the generic JSON and query parsing handles error cases. //! //! TODO-hardening: add test cases that exceed limits (e.g., query string length, //! JSON body length) @@ -34,6 +34,7 @@ use dropshot::HttpResponseTemporaryRedirect; use dropshot::HttpResponseUpdatedNoContent; use dropshot::Path; use dropshot::Query; +use dropshot::RawRequest; use dropshot::RequestContext; use dropshot::TypedBody; use dropshot::UntypedBody; @@ -70,6 +71,7 @@ fn demo_api() -> ApiDescription { api.register(demo_handler_path_param_uuid).unwrap(); api.register(demo_handler_path_param_u32).unwrap(); api.register(demo_handler_untyped_body).unwrap(); + api.register(demo_handler_raw_request).unwrap(); api.register(demo_handler_delete).unwrap(); api.register(demo_handler_headers).unwrap(); api.register(demo_handler_302_bogus).unwrap(); @@ -670,6 +672,32 @@ async fn test_untyped_body() { testctx.teardown().await; } +// Test `RawRequest`. +#[tokio::test] +async fn test_raw_request() { + let api = demo_api(); + let testctx = common::test_setup("tet_raw_request", api); + let client = &testctx.client_testctx; + + // Success case + let body = "you may know what you need but to get what you want \ + better see that you keep what you have"; + let mut response = client + .make_request_with_body( + Method::PUT, + "/testing/raw_request", + body.into(), + StatusCode::OK, + ) + .await + .unwrap(); + let json: DemoRaw = read_json(&mut response).await; + assert_eq!(json.nbytes, 90); + assert_eq!(json.method, "PUT"); + + testctx.teardown().await; +} + // Test delete request #[tokio::test] async fn test_delete_request() { @@ -979,6 +1007,32 @@ async fn demo_handler_untyped_body( Ok(HttpResponseOk(DemoUntyped { nbytes, as_utf8 })) } +#[derive(Deserialize, Serialize, JsonSchema)] +pub struct DemoRaw { + pub nbytes: usize, + pub method: String, +} + +#[endpoint { + method = PUT, + path = "/testing/raw_request" +}] +async fn demo_handler_raw_request( + _rqctx: Arc>, + raw_request: RawRequest, +) -> Result, HttpError> { + let request = raw_request.into_inner(); + + let (parts, body) = request.into_parts(); + // This is not generally a good pattern because it allows untrusted + // consumers to use up all memory. This is just a narrow test. + let whole_body = hyper::body::to_bytes(body).await.unwrap(); + Ok(HttpResponseOk(DemoRaw { + nbytes: whole_body.len(), + method: parts.method.to_string(), + })) +} + #[derive(Deserialize, Serialize, JsonSchema)] pub struct DemoPathImpossible { pub test1: String, diff --git a/dropshot_endpoint/src/lib.rs b/dropshot_endpoint/src/lib.rs index faeb929f..429011a5 100644 --- a/dropshot_endpoint/src/lib.rs +++ b/dropshot_endpoint/src/lib.rs @@ -86,6 +86,7 @@ const USAGE: &str = "Endpoint handlers must have the following signature: [path_params: Path

,] [body_param: TypedBody,] [body_param: UntypedBody,] + [raw_request: RawRequest,] ) -> Result"; /// This attribute transforms a handler function into a Dropshot endpoint