From 9f191fec73d58240b9a1a489152b433dfb47e117 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Wed, 17 Nov 2021 15:32:41 -0800 Subject: [PATCH 01/10] want better interface for making test HTTP requests --- Cargo.lock | 2 - Cargo.toml | 4 +- nexus/tests/common/http_testing.rs | 470 +++++++++++++++++++++++++ nexus/tests/common/mod.rs | 1 + nexus/tests/common/resource_helpers.rs | 31 +- nexus/tests/test_authz.rs | 43 ++- 6 files changed, 512 insertions(+), 39 deletions(-) create mode 100644 nexus/tests/common/http_testing.rs diff --git a/Cargo.lock b/Cargo.lock index 46241bdde47..90173b4c6b2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -526,7 +526,6 @@ checksum = "4bb454f0228b18c7f4c3b0ebbee346ed9c52e7443b0999cd543ff3571205701d" [[package]] name = "dropshot" version = "0.5.2-dev" -source = "git+https://github.com/oxidecomputer/dropshot?branch=main#c193292b40e271a31b302c4b38bfddd40ba255a4" dependencies = [ "async-trait", "base64", @@ -560,7 +559,6 @@ dependencies = [ [[package]] name = "dropshot_endpoint" version = "0.5.2-dev" -source = "git+https://github.com/oxidecomputer/dropshot?branch=main#c193292b40e271a31b302c4b38bfddd40ba255a4" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index e1427ee0306..26ae904950f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,8 +40,8 @@ panic = "abort" # It's common during development to use a local copy of dropshot or steno in the # parent directory. If you want to use those, uncomment one of these blocks. # -#[patch."https://github.com/oxidecomputer/dropshot"] -#dropshot = { path = "../dropshot/dropshot" } +[patch."https://github.com/oxidecomputer/dropshot"] +dropshot = { path = "../dropshot/dropshot" } #[patch."https://github.com/oxidecomputer/steno"] #steno = { path = "../steno" } diff --git a/nexus/tests/common/http_testing.rs b/nexus/tests/common/http_testing.rs new file mode 100644 index 00000000000..ab0ea4a3dc1 --- /dev/null +++ b/nexus/tests/common/http_testing.rs @@ -0,0 +1,470 @@ +//! Facilities for testing HTTP servers + +use anyhow::anyhow; +use anyhow::ensure; +use anyhow::Context; +use dropshot::test_util::ClientTestContext; +use std::convert::TryInto; +use std::fmt::Debug; + +/// Convenient way to make an outgoing HTTP request and verify various +/// properties of the response for testing +// +// When testing an HTTP server, we make varying requests to the server and +// verify a bunch of properties about it's behavior. A lot of things can go +// wrong along the way: +// +// - failed to serialize request body +// - failed to construct a valid request +// - failed to transmit the request or receive a well-formed response +// - server did something funny, like gave back unexpected or invalid headers +// - server returned a response different from what we expected, either in +// status code or the format of the response body +// +// We want to make it easy for consumers to verify all these things by default +// while also making it possible to skip certain checks (e.g., don't check the +// status code, maybe because the caller may expect any number of several status +// codes). +// +// The design here enables consumers to set up the request (including +// expectations about the response, like the status code), execute it, get back +// an error if anything went wrong, and get back the expected body (parsed) +// otherwise. The caller can specify: +// +// - HTTP request method (required) +// - HTTP request uri (required) +// - zero or more HTTP request headers +// - an HTTP request body (optional) +// - an expected status code (optional) +// - an expected response body type (optional) +// +// On top of all this, we want failures to be easy to debug. For now, that +// means we always log the outgoing request and incoming response. With a bit +// more work, we could also format the final error to include as much of the +// request and response information as we have, including the caller's +// expectations. +// +pub struct RequestBuilder { + method: http::Method, + uri: http::Uri, + headers: http::HeaderMap, + body: hyper::Body, + error: Option, + + expected_status: Option, + allowed_headers: Option>, +} + +impl RequestBuilder { + /// Start building a request with the given `method` and `uri` + /// + /// `testctx` is only used to construct the full URL (including HTTP scheme, + /// server name, and port). + pub fn new( + testctx: &dropshot::test_util::ClientTestContext, + method: http::Method, + uri: &str, + ) -> Self { + let uri = testctx.url(uri); + RequestBuilder { + method, + uri, + headers: http::HeaderMap::new(), + body: hyper::Body::empty(), + expected_status: None, + allowed_headers: Some(vec![ + http::header::HeaderName::from_static("content-length"), + http::header::HeaderName::from_static("content-type"), + http::header::HeaderName::from_static("date"), + http::header::HeaderName::from_static("x-request-id"), + ]), + error: None, + } + } + + /// Set header `name` to `value` in the outgoing request + pub fn header(mut self, name: K, value: V) -> Self + where + K: TryInto + Debug, + V: TryInto + Debug, + KE: std::error::Error + Send + Sync + 'static, + VE: std::error::Error + Send + Sync + 'static, + { + let header_name_dbg = format!("{:?}", name); + let header_value_dbg = format!("{:?}", value); + let header_name: Result = + name.try_into().with_context(|| { + format!("converting header name {}", header_name_dbg) + }); + let header_value: Result = + value.try_into().with_context(|| { + format!( + "converting value for header {}: {}", + header_name_dbg, header_value_dbg + ) + }); + match (header_name, header_value) { + (Ok(name), Ok(value)) => { + self.headers.append(name, value); + } + (Err(error), _) => { + self.error = Some(error); + } + (_, Err(error)) => { + self.error = Some(error); + } + }; + self + } + + /// Set the outgoing request body to the result of serializing `body` + /// + /// If `body` is `None`, the request body will be empty. + pub fn body( + mut self, + body: Option, + ) -> Self { + let new_body = body.map(|b| { + serde_json::to_string(&b) + .context("failed to serialize request body") + }); + match new_body { + Some(Err(error)) => self.error = Some(error), + Some(Ok(new_body)) => self.body = hyper::Body::from(new_body), + None => self.body = hyper::Body::empty(), + }; + self + } + + /// Record that we expect to get status code `expected_status` in the + /// response + /// + /// If `expected_status` is not `None`, then [`execute()`] will check this + /// and raise an error if a different status code is found. + pub fn expect_status( + mut self, + expected_status: Option, + ) -> Self { + self.expected_status = expected_status; + self + } + + /// Record a list of header names allowed in the response + /// + /// If this function is used, then [`execute()`] will check each header in + /// the response against this list and raise an error if a header name is + /// found that's not in this list. + pub fn expect_allowed_headers< + I: IntoIterator, + >( + mut self, + allowed_headers: I, + ) -> Self { + self.allowed_headers = Some(allowed_headers.into_iter().collect()); + self + } + + /// Make the HTTP request using the given `client`, verify the returned + /// response, and make the response available to the caller + /// + /// This function checks the returned status code (if [`expect_status()`] + /// was used), allowed headers (if [`allowed_headers()`] was used), and + /// various other properties of the response. + pub async fn execute( + self, + log: &slog::Logger, + client: &hyper::Client, + ) -> Result { + if let Some(error) = self.error { + return Err(error); + } + + let mut builder = + http::Request::builder().method(self.method).uri(self.uri); + for (header_name, header_value) in &self.headers { + builder = builder.header(header_name, header_value); + } + let request = + builder.body(self.body).context("failed to construct request")?; + + let time_before = chrono::offset::Utc::now().timestamp(); + slog::info!(log, "client request"; + "method" => %request.method(), + "uri" => %request.uri(), + "body" => ?&request.body(), + ); + + let mut response = client + .request(request) + .await + .context("making request to server")?; + + // Check that we got the expected response code. + let status = response.status(); + slog::info!(log, "client received response"; "status" => ?status); + + if let Some(expected_status) = self.expected_status { + ensure!( + expected_status == status, + "expected status code {}, found {}", + expected_status, + status + ); + } + + // Check that we didn't have any unexpected headers. + let headers = response.headers(); + if let Some(allowed_headers) = self.allowed_headers { + for header_name in headers.keys() { + ensure!( + allowed_headers.contains(header_name), + "response contained unexpected header {:?}", + header_name + ); + } + } + + // Sanity check the Date header in the response. This check assumes + // that the server is running on the same system as the client, which is + // currently true because this is running in a test suite. We may need + // to relax this constraint if we want to use this code elsewhere. + // + // Even on a single system, this check will fail spuriously in the + // unlikely event that the system clock is adjusted backwards in between + // when we sent the request and when we received the response. We + // consider that case unlikely enough to be worth doing this check + // anyway. (We'll try to check for the clock reset condition, too, but + // we cannot catch all cases that would cause the Date header check to + // be incorrect.) + // + // Note that the Date header typically only has precision down to one + // second, so we don't want to try to do a more precise comparison. + let time_after = chrono::offset::Utc::now().timestamp(); + ensure!( + time_before <= time_after, + "time went backwards during the request" + ); + let date_header = headers + .get(http::header::DATE) + .ok_or_else(|| anyhow!("missing Date header in response"))? + .to_str() + .context("converting Date header to string")?; + let time_request = chrono::DateTime::parse_from_rfc2822(date_header) + .context("parsing server's Date header")?; + ensure!( + time_request.timestamp() >= time_before - 1, + "Date header was too early" + ); + ensure!( + time_request.timestamp() <= time_after + 1, + "Date header was too late" + ); + + // Validate that we have a request id header. + // TODO-coverage check that it's unique among requests we've issued + let request_id_header = headers + .get(dropshot::HEADER_REQUEST_ID) + .ok_or_else(|| anyhow!("missing request id header"))? + .to_str() + .context("parsing request-id header as string")? + .to_string(); + + // Read the response. Note that this is somewhat dangerous -- a broken + // or malicious server could do damage by sending us an enormous + // response here. Since we only use this in a test suite, we ignore + // that risk. + let response_body = hyper::body::to_bytes(response.body_mut()) + .await + .context("reading response body")?; + + // For "204 No Content" responses, validate that we got no content in + // the body. + if status == http::StatusCode::NO_CONTENT { + ensure!( + response_body.len() == 0, + "expected empty response for 204 status code" + ) + } + + // For errors of any kind, attempt to parse the body and make sure the + // request id matches what we found in the header. + let test_response = TestResponse { status, response_body }; + if status.is_client_error() || status.is_server_error() { + let error_body = test_response + .expect_response_body::() + .context("parsing error body")?; + ensure!( + error_body.request_id == request_id_header, + "expected error response body to have request id {:?} \ + (to match request-id header), bout found {:?}", + request_id_header, + error_body.request_id + ); + } + + Ok(test_response) + } +} + +/// Represents a response from an HTTP server +pub struct TestResponse { + pub status: http::StatusCode, + pub response_body: bytes::Bytes, +} + +impl TestResponse { + /// Parse the response body as an instance of `R` and returns it + /// + /// Fails if the body could not be parsed as an `R`. + pub fn expect_response_body( + &self, + ) -> Result { + serde_json::from_slice(self.response_body.as_ref()) + .context("parsing response body") + } +} + +/// Specifies what user (if any) the caller wants to use for authenticating to +/// the server +pub enum AuthnMode { + UnprivilegedUser, + PrivilegedUser, +} + +/// Helper for constructing requests to Nexus's external API +/// +/// This is a thin wrapper around [`RequestBuilder`] that exists to allow +/// callers to specify authentication details (and, in the future, any other +/// application-level considerations). +pub struct NexusRequest { + request_builder: RequestBuilder, +} + +impl NexusRequest { + /// Create a `NexusRequest` around `request_builder` + /// + /// Most callers should use [`objects_post`], [`object_get`], + /// [`object_delete`], or the other wrapper constructors. If you use this + /// function directly, you should set up your `request_builder` first with + /// whatever HTTP-level stuff you want (including any non-authentication + /// headers), then call this function to get a `NexusRequest`, then use + /// [`authn_as()`] to configure authentication. + pub fn new(request_builder: RequestBuilder) -> Self { + NexusRequest { request_builder } + } + + /// Causes the request to authenticate to Nexus as a user specified by + /// `mode` + pub fn authn_as(mut self, mode: AuthnMode) -> Self { + use omicron_nexus::authn; + let header_value = match mode { + AuthnMode::UnprivilegedUser => authn::TEST_USER_UUID_UNPRIVILEGED, + AuthnMode::PrivilegedUser => authn::TEST_USER_UUID_PRIVILEGED, + }; + + self.request_builder = self.request_builder.header( + http::header::HeaderName::from_static( + authn::external::spoof::HTTP_HEADER_OXIDE_AUTHN_SPOOF, + ), + http::header::HeaderValue::from_static(header_value), + ); + self + } + + /// See [`RequestBuilder::execute()`]. + pub async fn execute( + self, + log: &slog::Logger, + ) -> Result { + let client = hyper::Client::new(); + self.request_builder.execute(log, &client).await + } + + /// Returns a new `NexusRequest` suitable for `POST $uri` with the given + /// `body` + pub fn objects_post( + testctx: &ClientTestContext, + uri: &str, + body: BodyType, + ) -> Self { + NexusRequest::new( + RequestBuilder::new(testctx, http::Method::POST, uri) + .body(Some(body)) + .expect_status(Some(http::StatusCode::CREATED)), + ) + } + + /// Returns a new `NexusRequest` suitable for `GET $uri` + pub fn object_get(testctx: &ClientTestContext, uri: &str) -> Self { + NexusRequest::new( + RequestBuilder::new(testctx, http::Method::GET, uri) + .expect_status(Some(http::StatusCode::OK)), + ) + } + + /// Returns a new `NexusRequest` suitable for `DELETE $uri` + pub fn object_delete(testctx: &ClientTestContext, uri: &str) -> Self { + NexusRequest::new( + RequestBuilder::new(testctx, http::Method::GET, uri) + .expect_status(Some(http::StatusCode::NO_CONTENT)), + ) + } +} + +/// Functions for compatibility with corresponding `dropshot::test_util` +/// functions. +pub mod dropshot_compat { + use super::NexusRequest; + use dropshot::{test_util::ClientTestContext, ResultsPage}; + use serde::{de::DeserializeOwned, Serialize}; + + /// See [`dropshot::test_util::object_get`]. + pub async fn object_get( + testctx: &ClientTestContext, + object_url: &str, + ) -> T + where + T: DeserializeOwned, + { + NexusRequest::object_get(testctx, object_url) + .execute(&testctx.client_log) + .await + .unwrap() + .expect_response_body::() + .unwrap() + } + + /// See [`dropshot::test_util::objects_list_page`]. + pub async fn objects_list_page( + testctx: &ClientTestContext, + list_url: &str, + ) -> dropshot::ResultsPage + where + T: DeserializeOwned, + { + NexusRequest::object_get(testctx, list_url) + .execute(&testctx.client_log) + .await + .unwrap() + .expect_response_body::>() + .unwrap() + } + + /// See [`dropshot::test_util::objects_post`]. + pub async fn objects_post( + testctx: &ClientTestContext, + collection_url: &str, + input: S, + ) -> T + where + S: Serialize + std::fmt::Debug, + T: DeserializeOwned, + { + NexusRequest::objects_post(testctx, collection_url, input) + .execute(&testctx.client_log) + .await + .unwrap() + .expect_response_body::() + .unwrap() + } +} diff --git a/nexus/tests/common/mod.rs b/nexus/tests/common/mod.rs index a61e6d232d4..e766c3c6212 100644 --- a/nexus/tests/common/mod.rs +++ b/nexus/tests/common/mod.rs @@ -20,6 +20,7 @@ use std::time::Duration; use uuid::Uuid; pub mod resource_helpers; +pub mod http_testing; const SLED_AGENT_UUID: &str = "b6d65341-167c-41df-9b5c-41cded99c229"; const RACK_UUID: &str = "c19a698f-c6f9-4a17-ae30-20d711b8f7dc"; diff --git a/nexus/tests/common/resource_helpers.rs b/nexus/tests/common/resource_helpers.rs index 1611d33f69a..86c5e6c4be4 100644 --- a/nexus/tests/common/resource_helpers.rs +++ b/nexus/tests/common/resource_helpers.rs @@ -1,13 +1,10 @@ -use dropshot::test_util::objects_post; -use dropshot::test_util::read_json; +use super::http_testing::dropshot_compat::objects_post; +use super::http_testing::AuthnMode; +use super::http_testing::NexusRequest; use dropshot::test_util::ClientTestContext; use dropshot::HttpErrorResponseBody; use dropshot::Method; use http::StatusCode; -use omicron_common::api::external::VpcRouter; -use omicron_common::api::external::VpcRouterCreateParams; -use omicron_nexus::authn::external::spoof::HTTP_HEADER_OXIDE_AUTHN_SPOOF; - use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::Organization; use omicron_common::api::external::OrganizationCreateParams; @@ -15,6 +12,8 @@ use omicron_common::api::external::Project; use omicron_common::api::external::ProjectCreateParams; use omicron_common::api::external::Vpc; use omicron_common::api::external::VpcCreateParams; +use omicron_common::api::external::VpcRouter; +use omicron_common::api::external::VpcRouterCreateParams; pub async fn create_organization( client: &ClientTestContext, @@ -26,21 +25,13 @@ pub async fn create_organization( description: "an org".to_string(), }, }; - let authn_header = http::HeaderValue::from_static( - omicron_nexus::authn::TEST_USER_UUID_PRIVILEGED, - ); - let uri = client.url("/organizations"); - let request = hyper::Request::builder() - .header(HTTP_HEADER_OXIDE_AUTHN_SPOOF, authn_header) - .method(Method::POST) - .uri(uri) - .body(serde_json::to_string(&input).unwrap().into()) - .expect("attempted to construct invalid test request"); - let mut response = client - .make_request_with_request(request, StatusCode::CREATED) + NexusRequest::objects_post(client, "/organizations", input) + .authn_as(AuthnMode::PrivilegedUser) + .execute(&client.client_log) .await - .expect("failed to make request"); - read_json::(&mut response).await + .expect("failed to make request") + .expect_response_body() + .unwrap() } pub async fn create_project( diff --git a/nexus/tests/test_authz.rs b/nexus/tests/test_authz.rs index 16b741a9480..e6323b09820 100644 --- a/nexus/tests/test_authz.rs +++ b/nexus/tests/test_authz.rs @@ -1,5 +1,7 @@ //! Basic end-to-end tests for authorization +use common::http_testing::RequestBuilder; use dropshot::HttpErrorResponseBody; +use futures::Future; use omicron_common::api::external::{ IdentityMetadataCreateParams, OrganizationCreateParams, }; @@ -25,17 +27,20 @@ extern crate slog; #[tokio::test] async fn test_authz_basic() { let cptestctx = test_setup("test_authz_basic").await; + let log = &cptestctx.logctx.log; let client = &cptestctx.external_client; // With no credentials, we should get back a 401 "Unauthorized" response. let error = - try_create_organization(&client, None, StatusCode::UNAUTHORIZED).await; + try_create_organization(log, &client, None, StatusCode::UNAUTHORIZED) + .await; assert_eq!(error.error_code, Some(String::from("Unauthorized"))); assert_eq!(error.message.as_str(), "credentials missing or invalid"); // If we provide the valid credentials of an unprivileged user, we should // get back a 403 "Forbidden" response. let error = try_create_organization( + log, &client, Some(omicron_nexus::authn::TEST_USER_UUID_UNPRIVILEGED), StatusCode::FORBIDDEN, @@ -49,6 +54,7 @@ async fn test_authz_basic() { // the authentication system in general, outside the context of Nexus). // This one verifies that we've correctly integrated authn with Nexus. let error = try_create_organization( + &log, &client, Some(omicron_nexus::authn::external::spoof::SPOOF_RESERVED_BAD_ACTOR), StatusCode::UNAUTHORIZED, @@ -58,6 +64,7 @@ async fn test_authz_basic() { assert_eq!(error.message.as_str(), "credentials missing or invalid"); let error = try_create_organization( + &log, &client, Some(omicron_nexus::authn::external::spoof::SPOOF_RESERVED_BAD_CREDS), StatusCode::UNAUTHORIZED, @@ -69,29 +76,35 @@ async fn test_authz_basic() { cptestctx.teardown().await; } -async fn try_create_organization( +fn try_create_organization( + log: &slog::Logger, client: &dropshot::test_util::ClientTestContext, maybe_user_id: Option<&'static str>, expected_status: http::StatusCode, -) -> HttpErrorResponseBody { - let organization_name = "a-crime-family"; +) -> impl Future { + let log = log.new(slog::o!()); let input = OrganizationCreateParams { identity: IdentityMetadataCreateParams { - name: organization_name.parse().unwrap(), + name: "a-crime-family".parse().unwrap(), description: "an org".to_string(), }, }; - let uri = client.url("/organizations"); - let mut request = hyper::Request::builder().method(Method::POST).uri(uri); + + let mut builder = + RequestBuilder::new(client, Method::POST, "/organizations") + .body(Some(input)) + .expect_status(Some(expected_status)); if let Some(user_id) = maybe_user_id { let authn_header = http::HeaderValue::from_static(user_id); - request = request.header(HTTP_HEADER_OXIDE_AUTHN_SPOOF, authn_header); + builder = builder.header(HTTP_HEADER_OXIDE_AUTHN_SPOOF, authn_header); + } + + async move { + builder + .execute(&log, &hyper::Client::new()) + .await + .expect("failed to make request") + .expect_response_body() + .unwrap() } - let request = request - .body(serde_json::to_string(&input).unwrap().into()) - .expect("attempted to construct invalid test request"); - client - .make_request_with_request(request, expected_status) - .await - .expect_err("did not expect request to succeed") } From a2992052247ee33cffbba43ceff63a2099316ca6 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 18 Nov 2021 17:14:23 -0800 Subject: [PATCH 02/10] fix mismerge --- Cargo.lock | 6 ++---- nexus/tests/common/resource_helpers.rs | 3 --- nexus/tests/test_authz.rs | 23 +++++++++++------------ 3 files changed, 13 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 39217b4ef79..56e99398672 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -653,8 +653,7 @@ checksum = "4bb454f0228b18c7f4c3b0ebbee346ed9c52e7443b0999cd543ff3571205701d" [[package]] name = "dropshot" -version = "0.5.2-dev" -source = "git+https://github.com/oxidecomputer/dropshot?branch=main#c7fe5d2ae94150c1b2c3d0c2844474fa2fb4c8bc" +version = "0.6.1-dev" dependencies = [ "async-trait", "base64", @@ -688,8 +687,7 @@ dependencies = [ [[package]] name = "dropshot_endpoint" -version = "0.5.2-dev" -source = "git+https://github.com/oxidecomputer/dropshot?branch=main#c7fe5d2ae94150c1b2c3d0c2844474fa2fb4c8bc" +version = "0.6.1-dev" dependencies = [ "proc-macro2", "quote", diff --git a/nexus/tests/common/resource_helpers.rs b/nexus/tests/common/resource_helpers.rs index 316f1a187a0..04b81282ffd 100644 --- a/nexus/tests/common/resource_helpers.rs +++ b/nexus/tests/common/resource_helpers.rs @@ -9,10 +9,7 @@ use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::Vpc; use omicron_common::api::external::VpcCreateParams; use omicron_common::api::external::VpcRouter; -use omicron_common::api::external::VpcRouter; -use omicron_common::api::external::VpcRouterCreateParams; use omicron_common::api::external::VpcRouterCreateParams; -use omicron_nexus::authn::external::spoof::HTTP_HEADER_OXIDE_AUTHN_SPOOF; use omicron_nexus::external_api::params; use omicron_nexus::external_api::views::{Organization, Project}; diff --git a/nexus/tests/test_authz.rs b/nexus/tests/test_authz.rs index ca75982a6f8..c75a188c625 100644 --- a/nexus/tests/test_authz.rs +++ b/nexus/tests/test_authz.rs @@ -2,9 +2,6 @@ use common::http_testing::RequestBuilder; use dropshot::HttpErrorResponseBody; use futures::Future; -use omicron_common::api::external::{ - IdentityMetadataCreateParams, OrganizationCreateParams, -}; pub mod common; use common::test_setup; @@ -78,14 +75,14 @@ async fn test_authz_basic() { cptestctx.teardown().await; } -async fn try_create_organization( +fn try_create_organization( log: &slog::Logger, client: &dropshot::test_util::ClientTestContext, maybe_user_id: Option<&'static str>, expected_status: http::StatusCode, -) -> HttpErrorResponseBody { +) -> impl Future { let log = log.new(slog::o!()); - let input = params::OrganizationCreateParams { + let input = params::OrganizationCreate { identity: IdentityMetadataCreateParams { name: "a-crime-family".parse().unwrap(), description: "an org".to_string(), @@ -101,10 +98,12 @@ async fn try_create_organization( builder = builder.header(HTTP_HEADER_OXIDE_AUTHN_SPOOF, authn_header); } - builder - .execute(&log, &hyper::Client::new()) - .await - .expect("failed to make request") - .expect_response_body() - .unwrap() + async move { + builder + .execute(&log, &hyper::Client::new()) + .await + .expect("failed to make request") + .expect_response_body() + .unwrap() + } } From f57370dc5adfa491736524c7f37b519725789300 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 18 Nov 2021 18:17:16 -0800 Subject: [PATCH 03/10] the dropshot change landed --- Cargo.lock | 2 ++ Cargo.toml | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 56e99398672..73314f99344 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -654,6 +654,7 @@ checksum = "4bb454f0228b18c7f4c3b0ebbee346ed9c52e7443b0999cd543ff3571205701d" [[package]] name = "dropshot" version = "0.6.1-dev" +source = "git+https://github.com/oxidecomputer/dropshot?branch=main#a1f980db2188a9187702ff804de5f2f557a52c92" dependencies = [ "async-trait", "base64", @@ -688,6 +689,7 @@ dependencies = [ [[package]] name = "dropshot_endpoint" version = "0.6.1-dev" +source = "git+https://github.com/oxidecomputer/dropshot?branch=main#a1f980db2188a9187702ff804de5f2f557a52c92" dependencies = [ "proc-macro2", "quote", diff --git a/Cargo.toml b/Cargo.toml index 26ae904950f..e1427ee0306 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,8 +40,8 @@ panic = "abort" # It's common during development to use a local copy of dropshot or steno in the # parent directory. If you want to use those, uncomment one of these blocks. # -[patch."https://github.com/oxidecomputer/dropshot"] -dropshot = { path = "../dropshot/dropshot" } +#[patch."https://github.com/oxidecomputer/dropshot"] +#dropshot = { path = "../dropshot/dropshot" } #[patch."https://github.com/oxidecomputer/steno"] #steno = { path = "../steno" } From edc8f62de6d8cd1a8c7dcdfcacb8e00d3b47afe1 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 18 Nov 2021 18:19:01 -0800 Subject: [PATCH 04/10] fix style --- nexus/tests/common/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/tests/common/mod.rs b/nexus/tests/common/mod.rs index e766c3c6212..a8f529561eb 100644 --- a/nexus/tests/common/mod.rs +++ b/nexus/tests/common/mod.rs @@ -19,8 +19,8 @@ use std::path::Path; use std::time::Duration; use uuid::Uuid; -pub mod resource_helpers; pub mod http_testing; +pub mod resource_helpers; const SLED_AGENT_UUID: &str = "b6d65341-167c-41df-9b5c-41cded99c229"; const RACK_UUID: &str = "c19a698f-c6f9-4a17-ae30-20d711b8f7dc"; From cecf5b29b1345bde5013ab66cf1af2a2dd6c6c9e Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 19 Nov 2021 08:24:58 -0800 Subject: [PATCH 05/10] code review feedback --- nexus/tests/common/http_testing.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/nexus/tests/common/http_testing.rs b/nexus/tests/common/http_testing.rs index ab0ea4a3dc1..bb65bb040d5 100644 --- a/nexus/tests/common/http_testing.rs +++ b/nexus/tests/common/http_testing.rs @@ -73,9 +73,9 @@ impl RequestBuilder { body: hyper::Body::empty(), expected_status: None, allowed_headers: Some(vec![ - http::header::HeaderName::from_static("content-length"), - http::header::HeaderName::from_static("content-type"), - http::header::HeaderName::from_static("date"), + http::header::CONTENT_LENGTH, + http::header::CONTENT_TYPE, + http::header::DATE, http::header::HeaderName::from_static("x-request-id"), ]), error: None, @@ -288,7 +288,11 @@ impl RequestBuilder { // For errors of any kind, attempt to parse the body and make sure the // request id matches what we found in the header. - let test_response = TestResponse { status, response_body }; + let test_response = TestResponse { + status, + headers: response.headers().clone(), + body: response_body, + }; if status.is_client_error() || status.is_server_error() { let error_body = test_response .expect_response_body::() @@ -309,7 +313,8 @@ impl RequestBuilder { /// Represents a response from an HTTP server pub struct TestResponse { pub status: http::StatusCode, - pub response_body: bytes::Bytes, + pub headers: http::HeaderMap, + body: bytes::Bytes, } impl TestResponse { @@ -319,7 +324,7 @@ impl TestResponse { pub fn expect_response_body( &self, ) -> Result { - serde_json::from_slice(self.response_body.as_ref()) + serde_json::from_slice(self.body.as_ref()) .context("parsing response body") } } From 5eb6f1ca9b1dec43926d286cd20eb97c5bd70e71 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 19 Nov 2021 08:26:16 -0800 Subject: [PATCH 06/10] allow set-cookie header too --- nexus/tests/common/http_testing.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/nexus/tests/common/http_testing.rs b/nexus/tests/common/http_testing.rs index bb65bb040d5..5c4e3609e4b 100644 --- a/nexus/tests/common/http_testing.rs +++ b/nexus/tests/common/http_testing.rs @@ -76,6 +76,7 @@ impl RequestBuilder { http::header::CONTENT_LENGTH, http::header::CONTENT_TYPE, http::header::DATE, + http::header::SET_COOKIE, http::header::HeaderName::from_static("x-request-id"), ]), error: None, From 4357355b5497a98920721777b1a6ce63d282c8f7 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 19 Nov 2021 08:54:12 -0800 Subject: [PATCH 07/10] streamline use of client and log --- nexus/tests/common/http_testing.rs | 67 +++++++++++++------------- nexus/tests/common/resource_helpers.rs | 4 +- nexus/tests/test_authz.rs | 25 ++++------ 3 files changed, 45 insertions(+), 51 deletions(-) diff --git a/nexus/tests/common/http_testing.rs b/nexus/tests/common/http_testing.rs index 5c4e3609e4b..8300760eec9 100644 --- a/nexus/tests/common/http_testing.rs +++ b/nexus/tests/common/http_testing.rs @@ -44,7 +44,9 @@ use std::fmt::Debug; // request and response information as we have, including the caller's // expectations. // -pub struct RequestBuilder { +pub struct RequestBuilder<'a> { + client: &'a dropshot::test_util::ClientTestContext, + method: http::Method, uri: http::Uri, headers: http::HeaderMap, @@ -55,18 +57,19 @@ pub struct RequestBuilder { allowed_headers: Option>, } -impl RequestBuilder { +impl<'a> RequestBuilder<'a> { /// Start building a request with the given `method` and `uri` /// /// `testctx` is only used to construct the full URL (including HTTP scheme, /// server name, and port). pub fn new( - testctx: &dropshot::test_util::ClientTestContext, + client: &'a dropshot::test_util::ClientTestContext, method: http::Method, uri: &str, ) -> Self { - let uri = testctx.url(uri); + let uri = client.url(uri); RequestBuilder { + client, method, uri, headers: http::HeaderMap::new(), @@ -171,11 +174,7 @@ impl RequestBuilder { /// This function checks the returned status code (if [`expect_status()`] /// was used), allowed headers (if [`allowed_headers()`] was used), and /// various other properties of the response. - pub async fn execute( - self, - log: &slog::Logger, - client: &hyper::Client, - ) -> Result { + pub async fn execute(self) -> Result { if let Some(error) = self.error { return Err(error); } @@ -189,20 +188,26 @@ impl RequestBuilder { builder.body(self.body).context("failed to construct request")?; let time_before = chrono::offset::Utc::now().timestamp(); - slog::info!(log, "client request"; + slog::info!(self.client.client_log, + "client request"; "method" => %request.method(), "uri" => %request.uri(), "body" => ?&request.body(), ); - let mut response = client + let mut response = self + .client + .client .request(request) .await .context("making request to server")?; // Check that we got the expected response code. let status = response.status(); - slog::info!(log, "client received response"; "status" => ?status); + slog::info!(self.client.client_log, + "client received response"; + "status" => ?status + ); if let Some(expected_status) = self.expected_status { ensure!( @@ -296,7 +301,7 @@ impl RequestBuilder { }; if status.is_client_error() || status.is_server_error() { let error_body = test_response - .expect_response_body::() + .response_body::() .context("parsing error body")?; ensure!( error_body.request_id == request_id_header, @@ -322,7 +327,7 @@ impl TestResponse { /// Parse the response body as an instance of `R` and returns it /// /// Fails if the body could not be parsed as an `R`. - pub fn expect_response_body( + pub fn response_body( &self, ) -> Result { serde_json::from_slice(self.body.as_ref()) @@ -342,11 +347,11 @@ pub enum AuthnMode { /// This is a thin wrapper around [`RequestBuilder`] that exists to allow /// callers to specify authentication details (and, in the future, any other /// application-level considerations). -pub struct NexusRequest { - request_builder: RequestBuilder, +pub struct NexusRequest<'a> { + request_builder: RequestBuilder<'a>, } -impl NexusRequest { +impl<'a> NexusRequest<'a> { /// Create a `NexusRequest` around `request_builder` /// /// Most callers should use [`objects_post`], [`object_get`], @@ -355,7 +360,7 @@ impl NexusRequest { /// whatever HTTP-level stuff you want (including any non-authentication /// headers), then call this function to get a `NexusRequest`, then use /// [`authn_as()`] to configure authentication. - pub fn new(request_builder: RequestBuilder) -> Self { + pub fn new(request_builder: RequestBuilder<'a>) -> Self { NexusRequest { request_builder } } @@ -378,18 +383,14 @@ impl NexusRequest { } /// See [`RequestBuilder::execute()`]. - pub async fn execute( - self, - log: &slog::Logger, - ) -> Result { - let client = hyper::Client::new(); - self.request_builder.execute(log, &client).await + pub async fn execute(self) -> Result { + self.request_builder.execute().await } /// Returns a new `NexusRequest` suitable for `POST $uri` with the given /// `body` pub fn objects_post( - testctx: &ClientTestContext, + testctx: &'a ClientTestContext, uri: &str, body: BodyType, ) -> Self { @@ -401,7 +402,7 @@ impl NexusRequest { } /// Returns a new `NexusRequest` suitable for `GET $uri` - pub fn object_get(testctx: &ClientTestContext, uri: &str) -> Self { + pub fn object_get(testctx: &'a ClientTestContext, uri: &str) -> Self { NexusRequest::new( RequestBuilder::new(testctx, http::Method::GET, uri) .expect_status(Some(http::StatusCode::OK)), @@ -409,7 +410,7 @@ impl NexusRequest { } /// Returns a new `NexusRequest` suitable for `DELETE $uri` - pub fn object_delete(testctx: &ClientTestContext, uri: &str) -> Self { + pub fn object_delete(testctx: &'a ClientTestContext, uri: &str) -> Self { NexusRequest::new( RequestBuilder::new(testctx, http::Method::GET, uri) .expect_status(Some(http::StatusCode::NO_CONTENT)), @@ -433,10 +434,10 @@ pub mod dropshot_compat { T: DeserializeOwned, { NexusRequest::object_get(testctx, object_url) - .execute(&testctx.client_log) + .execute() .await .unwrap() - .expect_response_body::() + .response_body::() .unwrap() } @@ -449,10 +450,10 @@ pub mod dropshot_compat { T: DeserializeOwned, { NexusRequest::object_get(testctx, list_url) - .execute(&testctx.client_log) + .execute() .await .unwrap() - .expect_response_body::>() + .response_body::>() .unwrap() } @@ -467,10 +468,10 @@ pub mod dropshot_compat { T: DeserializeOwned, { NexusRequest::objects_post(testctx, collection_url, input) - .execute(&testctx.client_log) + .execute() .await .unwrap() - .expect_response_body::() + .response_body::() .unwrap() } } diff --git a/nexus/tests/common/resource_helpers.rs b/nexus/tests/common/resource_helpers.rs index 04b81282ffd..8464920fbf7 100644 --- a/nexus/tests/common/resource_helpers.rs +++ b/nexus/tests/common/resource_helpers.rs @@ -25,10 +25,10 @@ pub async fn create_organization( }; NexusRequest::objects_post(client, "/organizations", input) .authn_as(AuthnMode::PrivilegedUser) - .execute(&client.client_log) + .execute() .await .expect("failed to make request") - .expect_response_body() + .response_body() .unwrap() } diff --git a/nexus/tests/test_authz.rs b/nexus/tests/test_authz.rs index c75a188c625..302b9460958 100644 --- a/nexus/tests/test_authz.rs +++ b/nexus/tests/test_authz.rs @@ -26,21 +26,18 @@ extern crate slog; #[tokio::test] async fn test_authz_basic() { let cptestctx = test_setup("test_authz_basic").await; - let log = &cptestctx.logctx.log; let client = &cptestctx.external_client; // With no credentials, we should get back a 401 "Unauthorized" response. let error = - try_create_organization(log, &client, None, StatusCode::UNAUTHORIZED) - .await; + try_create_organization(client, None, StatusCode::UNAUTHORIZED).await; assert_eq!(error.error_code, Some(String::from("Unauthorized"))); assert_eq!(error.message.as_str(), "credentials missing or invalid"); // If we provide the valid credentials of an unprivileged user, we should // get back a 403 "Forbidden" response. let error = try_create_organization( - log, - &client, + client, Some(omicron_nexus::authn::TEST_USER_UUID_UNPRIVILEGED), StatusCode::FORBIDDEN, ) @@ -53,8 +50,7 @@ async fn test_authz_basic() { // the authentication system in general, outside the context of Nexus). // This one verifies that we've correctly integrated authn with Nexus. let error = try_create_organization( - &log, - &client, + client, Some(omicron_nexus::authn::external::spoof::SPOOF_RESERVED_BAD_ACTOR), StatusCode::UNAUTHORIZED, ) @@ -63,8 +59,7 @@ async fn test_authz_basic() { assert_eq!(error.message.as_str(), "credentials missing or invalid"); let error = try_create_organization( - &log, - &client, + client, Some(omicron_nexus::authn::external::spoof::SPOOF_RESERVED_BAD_CREDS), StatusCode::UNAUTHORIZED, ) @@ -75,13 +70,11 @@ async fn test_authz_basic() { cptestctx.teardown().await; } -fn try_create_organization( - log: &slog::Logger, - client: &dropshot::test_util::ClientTestContext, +fn try_create_organization<'a>( + client: &'a dropshot::test_util::ClientTestContext, maybe_user_id: Option<&'static str>, expected_status: http::StatusCode, -) -> impl Future { - let log = log.new(slog::o!()); +) -> impl Future + 'a { let input = params::OrganizationCreate { identity: IdentityMetadataCreateParams { name: "a-crime-family".parse().unwrap(), @@ -100,10 +93,10 @@ fn try_create_organization( async move { builder - .execute(&log, &hyper::Client::new()) + .execute() .await .expect("failed to make request") - .expect_response_body() + .response_body() .unwrap() } } From 236af020814997645e99162b93c8617abee9490f Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 19 Nov 2021 08:57:14 -0800 Subject: [PATCH 08/10] fix comment --- nexus/tests/common/http_testing.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/nexus/tests/common/http_testing.rs b/nexus/tests/common/http_testing.rs index 8300760eec9..8a3a378b291 100644 --- a/nexus/tests/common/http_testing.rs +++ b/nexus/tests/common/http_testing.rs @@ -59,9 +59,6 @@ pub struct RequestBuilder<'a> { impl<'a> RequestBuilder<'a> { /// Start building a request with the given `method` and `uri` - /// - /// `testctx` is only used to construct the full URL (including HTTP scheme, - /// server name, and port). pub fn new( client: &'a dropshot::test_util::ClientTestContext, method: http::Method, From 20120b506a1bee0462453aa928aa90f1825065ce Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 19 Nov 2021 08:58:48 -0800 Subject: [PATCH 09/10] more precise naming for testctx --- nexus/tests/common/http_testing.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/nexus/tests/common/http_testing.rs b/nexus/tests/common/http_testing.rs index 8a3a378b291..7770d9c0da9 100644 --- a/nexus/tests/common/http_testing.rs +++ b/nexus/tests/common/http_testing.rs @@ -45,7 +45,7 @@ use std::fmt::Debug; // expectations. // pub struct RequestBuilder<'a> { - client: &'a dropshot::test_util::ClientTestContext, + testctx: &'a dropshot::test_util::ClientTestContext, method: http::Method, uri: http::Uri, @@ -60,13 +60,13 @@ pub struct RequestBuilder<'a> { impl<'a> RequestBuilder<'a> { /// Start building a request with the given `method` and `uri` pub fn new( - client: &'a dropshot::test_util::ClientTestContext, + testctx: &'a dropshot::test_util::ClientTestContext, method: http::Method, uri: &str, ) -> Self { - let uri = client.url(uri); + let uri = testctx.url(uri); RequestBuilder { - client, + testctx, method, uri, headers: http::HeaderMap::new(), @@ -185,7 +185,7 @@ impl<'a> RequestBuilder<'a> { builder.body(self.body).context("failed to construct request")?; let time_before = chrono::offset::Utc::now().timestamp(); - slog::info!(self.client.client_log, + slog::info!(self.testctx.client_log, "client request"; "method" => %request.method(), "uri" => %request.uri(), @@ -193,7 +193,7 @@ impl<'a> RequestBuilder<'a> { ); let mut response = self - .client + .testctx .client .request(request) .await @@ -201,7 +201,7 @@ impl<'a> RequestBuilder<'a> { // Check that we got the expected response code. let status = response.status(); - slog::info!(self.client.client_log, + slog::info!(self.testctx.client_log, "client received response"; "status" => ?status ); From 167207e4b8645d089e0b04d4faa5576d1ca98576 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 19 Nov 2021 09:09:14 -0800 Subject: [PATCH 10/10] no longer need to desugar `try_create_organization` --- nexus/tests/test_authz.rs | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/nexus/tests/test_authz.rs b/nexus/tests/test_authz.rs index 302b9460958..fe071b5b80e 100644 --- a/nexus/tests/test_authz.rs +++ b/nexus/tests/test_authz.rs @@ -1,7 +1,6 @@ //! Basic end-to-end tests for authorization use common::http_testing::RequestBuilder; use dropshot::HttpErrorResponseBody; -use futures::Future; pub mod common; use common::test_setup; @@ -70,11 +69,11 @@ async fn test_authz_basic() { cptestctx.teardown().await; } -fn try_create_organization<'a>( - client: &'a dropshot::test_util::ClientTestContext, +async fn try_create_organization( + client: &dropshot::test_util::ClientTestContext, maybe_user_id: Option<&'static str>, expected_status: http::StatusCode, -) -> impl Future + 'a { +) -> HttpErrorResponseBody { let input = params::OrganizationCreate { identity: IdentityMetadataCreateParams { name: "a-crime-family".parse().unwrap(), @@ -91,12 +90,10 @@ fn try_create_organization<'a>( builder = builder.header(HTTP_HEADER_OXIDE_AUTHN_SPOOF, authn_header); } - async move { - builder - .execute() - .await - .expect("failed to make request") - .response_body() - .unwrap() - } + builder + .execute() + .await + .expect("failed to make request") + .response_body() + .unwrap() }