diff --git a/common/src/api/external/http_pagination.rs b/common/src/api/external/http_pagination.rs index 1ae6706e101..601534db431 100644 --- a/common/src/api/external/http_pagination.rs +++ b/common/src/api/external/http_pagination.rs @@ -42,6 +42,7 @@ use crate::api::external::DataPageParams; use crate::api::external::Name; +use crate::api::external::NameOrId; use crate::api::external::ObjectIdentity; use crate::api::external::PaginationOrder; use dropshot::HttpError; @@ -147,14 +148,15 @@ pub fn marker_for_id(_: &S, t: &T) -> Uuid { /// /// This is intended for use with [`ScanByNameOrId::results_page`] with objects /// that impl [`ObjectIdentity`]. -pub fn marker_for_name_or_id( - scan: &ScanByNameOrId, +pub fn marker_for_name_or_id( + scan: &ScanByNameOrId, item: &T, -) -> NameOrIdMarker { +) -> NameOrId { let identity = item.identity(); - match pagination_field_for_scan_params(scan) { - PagField::Name => NameOrIdMarker::Name(identity.name.clone()), - PagField::Id => NameOrIdMarker::Id(identity.id), + match scan.sort_by { + NameOrIdSortMode::NameAscending => identity.name.clone().into(), + NameOrIdSortMode::NameDescending => identity.name.clone().into(), + NameOrIdSortMode::IdAscending => identity.id.into(), } } @@ -177,10 +179,6 @@ where /// Given a request and pagination parameters, return a [`DataPageParams`] /// describing the current page of results to return -/// -/// This implementation is used for `ScanByName` and `ScanById`. See -/// [`data_page_params_nameid_name`] and [`data_page_params_nameid_id`] for -/// variants that can be used for `ScanByNameOrId`. pub fn data_page_params_for<'a, S, C>( rqctx: &'a Arc>, pag_params: &'a PaginationParams>, @@ -298,15 +296,45 @@ impl ScanParams for ScanById { // We include this now primarily to exercise the interface for doing so. /// Query parameters for pagination by name or id -pub type PaginatedByNameOrId = - PaginationParams; +pub type PaginatedByNameOrId = PaginationParams< + ScanByNameOrId, + PageSelectorByNameOrId, +>; /// Page selector for pagination by name or id -pub type PageSelectorByNameOrId = PageSelector; +pub type PageSelectorByNameOrId = + PageSelector, NameOrId>; + +pub fn name_or_id_pagination< + 'a, + Selector: Clone + Debug + DeserializeOwned + JsonSchema + PartialEq + Serialize, +>( + params: &PaginatedByNameOrId, + pagparams: &'a DataPageParams, +) -> Result, HttpError> { + let params = ScanByNameOrId::::from_query(params)?; + match params.sort_by { + NameOrIdSortMode::NameAscending => Ok(PaginatedBy::Name( + pagparams.try_into()?, + params.selector.clone(), + )), + NameOrIdSortMode::NameDescending => Ok(PaginatedBy::Name( + pagparams.try_into()?, + params.selector.clone(), + )), + NameOrIdSortMode::IdAscending => { + Ok(PaginatedBy::Id(pagparams.try_into()?, params.selector.clone())) + } + } +} + /// Scan parameters for resources that support scanning by name or id #[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] -pub struct ScanByNameOrId { +pub struct ScanByNameOrId { #[serde(default = "default_nameid_sort_mode")] sort_by: NameOrIdSortMode, + + #[serde(flatten)] + selector: Selector, } /// Supported set of sort modes for scanning by name or id #[derive(Copy, Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] @@ -324,42 +352,21 @@ fn default_nameid_sort_mode() -> NameOrIdSortMode { NameOrIdSortMode::NameAscending } -// TODO-correctness It's tempting to make this a serde(untagged) enum, which -// would clean up the format of the page selector parameter. However, it would -// have the side effect that if the name happened to be a valid uuid, then we'd -// parse it as a uuid here, even if the corresponding scan parameters indicated -// that we were doing a scan by name. Then we'd fail later on an invalid -// combination. We could infer the correct variant here from the "sort_by" -// field of the adjacent scan params, but we'd have to write our own -// `Deserialize` to do this. This might be worth revisiting before we commit to -// any particular version of the API. -#[derive(Clone, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] -#[serde(rename_all = "camelCase")] -pub enum NameOrIdMarker { - Id(Uuid), - Name(Name), -} - fn bad_token_error() -> HttpError { HttpError::for_bad_request(None, String::from("invalid page token")) } -#[derive(Debug, PartialEq)] -pub enum PagField { - Id, - Name, +#[derive(Debug)] +pub enum PaginatedBy<'a, Selector> { + Id(DataPageParams<'a, Uuid>, Selector), + Name(DataPageParams<'a, Name>, Selector), } -pub fn pagination_field_for_scan_params(p: &ScanByNameOrId) -> PagField { - match p.sort_by { - NameOrIdSortMode::NameAscending => PagField::Name, - NameOrIdSortMode::NameDescending => PagField::Name, - NameOrIdSortMode::IdAscending => PagField::Id, - } -} - -impl ScanParams for ScanByNameOrId { - type MarkerValue = NameOrIdMarker; +impl< + T: Clone + Debug + DeserializeOwned + JsonSchema + PartialEq + Serialize, + > ScanParams for ScanByNameOrId +{ + type MarkerValue = NameOrId; fn direction(&self) -> PaginationOrder { match self.sort_by { @@ -377,7 +384,7 @@ impl ScanParams for ScanByNameOrId { WhichPage::Next(PageSelectorByNameOrId { scan, - last_seen: NameOrIdMarker::Name(_), + last_seen: NameOrId::Name(_), }) => match scan.sort_by { NameOrIdSortMode::NameAscending => Ok(scan), NameOrIdSortMode::NameDescending => Ok(scan), @@ -386,7 +393,7 @@ impl ScanParams for ScanByNameOrId { WhichPage::Next(PageSelectorByNameOrId { scan, - last_seen: NameOrIdMarker::Id(_), + last_seen: NameOrId::Id(_), }) => match scan.sort_by { NameOrIdSortMode::NameAscending => Err(()), NameOrIdSortMode::NameDescending => Err(()), @@ -397,94 +404,23 @@ impl ScanParams for ScanByNameOrId { } } -/// Serves the same purpose as [`data_page_params_for`] for the specific case of -/// `ScanByNameOrId` when scanning by `name` -/// -/// Why do we need a separate function here? Because `data_page_params_for` only -/// knows how to return the (statically-defined) marker value from the page -/// selector. For `ScanByNameOrId`, this would return the enum -/// `NameOrIdMarker`. But at some point our caller needs the specific type -/// (e.g., `Name` for a scan by name or `Uuid` for a scan by Uuid). They get -/// that from this function and its partner, [`data_page_params_nameid_id`]. -/// These functions are where we look at the enum variant and extract the -/// specific marker value out. -pub fn data_page_params_nameid_name<'a, C>( - rqctx: &'a Arc>, - pag_params: &'a PaginatedByNameOrId, -) -> Result, HttpError> -where - C: dropshot::ServerContext, -{ - let limit = rqctx.page_limit(pag_params)?; - data_page_params_nameid_name_limit(limit, pag_params) -} - -fn data_page_params_nameid_name_limit( - limit: NonZeroU32, - pag_params: &PaginatedByNameOrId, -) -> Result, HttpError> { - let data_page = data_page_params_with_limit(limit, pag_params)?; - let direction = data_page.direction; - let marker = match data_page.marker { - None => None, - Some(NameOrIdMarker::Name(name)) => Some(name), - // This should arguably be a panic or a 500 error, since the caller - // should not have invoked this version of the function if they didn't - // know they were looking at a name-based marker. - Some(NameOrIdMarker::Id(_)) => return Err(bad_token_error()), - }; - Ok(DataPageParams { limit, direction, marker }) -} - -/// See [`data_page_params_nameid_name`]. -pub fn data_page_params_nameid_id<'a, C>( - rqctx: &'a Arc>, - pag_params: &'a PaginatedByNameOrId, -) -> Result, HttpError> -where - C: dropshot::ServerContext, -{ - let limit = rqctx.page_limit(pag_params)?; - data_page_params_nameid_id_limit(limit, pag_params) -} - -fn data_page_params_nameid_id_limit( - limit: NonZeroU32, - pag_params: &PaginatedByNameOrId, -) -> Result, HttpError> { - let data_page = data_page_params_with_limit(limit, pag_params)?; - let direction = data_page.direction; - let marker = match data_page.marker { - None => None, - Some(NameOrIdMarker::Id(id)) => Some(id), - // This should arguably be a panic or a 500 error, since the caller - // should not have invoked this version of the function if they didn't - // know they were looking at an id-based marker. - Some(NameOrIdMarker::Name(_)) => return Err(bad_token_error()), - }; - Ok(DataPageParams { limit, direction, marker }) -} - #[cfg(test)] mod test { - use super::data_page_params_nameid_id_limit; - use super::data_page_params_nameid_name_limit; use super::data_page_params_with_limit; use super::marker_for_id; use super::marker_for_name; use super::marker_for_name_or_id; use super::page_selector_for; - use super::pagination_field_for_scan_params; use super::IdSortMode; use super::Name; - use super::NameOrIdMarker; + use super::NameOrId; use super::NameOrIdSortMode; use super::NameSortMode; - use super::PagField; use super::PageSelector; use super::PageSelectorById; use super::PageSelectorByName; use super::PageSelectorByNameOrId; + use super::PaginatedBy; use super::PaginatedById; use super::PaginatedByName; use super::PaginatedByNameOrId; @@ -492,6 +428,7 @@ mod test { use super::ScanByName; use super::ScanByNameOrId; use super::ScanParams; + use crate::api::external::http_pagination::name_or_id_pagination; use crate::api::external::IdentityMetadata; use crate::api::external::ObjectIdentity; use chrono::Utc; @@ -499,7 +436,6 @@ mod test { use dropshot::PaginationParams; use dropshot::WhichPage; use expectorate::assert_contents; - use http::StatusCode; use schemars::schema_for; use serde::Serialize; use serde_json::to_string_pretty; @@ -522,7 +458,7 @@ mod test { ("scan parameters, scan by id only", schema_for!(ScanById)), ( "scan parameters, scan by name or id", - schema_for!(ScanByNameOrId), + schema_for!(ScanByNameOrId<()>), ), ( "page selector, scan by name only", @@ -553,10 +489,14 @@ mod test { fn test_pagination_examples() { let scan_by_id = ScanById { sort_by: IdSortMode::IdAscending }; let scan_by_name = ScanByName { sort_by: NameSortMode::NameAscending }; - let scan_by_nameid_name = - ScanByNameOrId { sort_by: NameOrIdSortMode::NameAscending }; - let scan_by_nameid_id = - ScanByNameOrId { sort_by: NameOrIdSortMode::IdAscending }; + let scan_by_nameid_name = ScanByNameOrId::<()> { + sort_by: NameOrIdSortMode::NameAscending, + selector: (), + }; + let scan_by_nameid_id = ScanByNameOrId::<()> { + sort_by: NameOrIdSortMode::IdAscending, + selector: (), + }; let id: Uuid = "61a78113-d3c6-4b35-a410-23e9eae64328".parse().unwrap(); let name: Name = "bort".parse().unwrap(); let examples = vec![ @@ -595,7 +535,7 @@ mod test { "page selector: by name or id, using id ascending", to_string_pretty(&PageSelectorByNameOrId { scan: scan_by_nameid_id, - last_seen: NameOrIdMarker::Id(id), + last_seen: NameOrId::Id(id), }) .unwrap(), ), @@ -603,7 +543,7 @@ mod test { "page selector: by name or id, using id ascending", to_string_pretty(&PageSelectorByNameOrId { scan: scan_by_nameid_name, - last_seen: NameOrIdMarker::Name(name), + last_seen: NameOrId::Name(name), }) .unwrap(), ), @@ -814,77 +754,102 @@ mod test { #[test] fn test_scan_by_nameid_name() { // Start with the common battery of tests. - let scan = ScanByNameOrId { sort_by: NameOrIdSortMode::NameDescending }; - assert_eq!(pagination_field_for_scan_params(&scan), PagField::Name); + let scan = ScanByNameOrId { + sort_by: NameOrIdSortMode::NameDescending, + selector: (), + }; assert_eq!(scan.direction(), PaginationOrder::Descending); let list = list_of_things(); - let thing0_marker = NameOrIdMarker::Name("thing0".parse().unwrap()); + let thing0_marker = NameOrId::Name("thing0".parse().unwrap()); let thinglast_name: Name = "thing19".parse().unwrap(); - let thinglast_marker = NameOrIdMarker::Name(thinglast_name.clone()); + let thinglast_marker = NameOrId::Name(thinglast_name.clone()); let (p0, p1) = test_scan_param_common( &list, &scan, "sort_by=name_descending", &thing0_marker, &thinglast_marker, - &ScanByNameOrId { sort_by: NameOrIdSortMode::NameAscending }, + &ScanByNameOrId { + sort_by: NameOrIdSortMode::NameAscending, + selector: (), + }, &marker_for_name_or_id, ); // Verify data pages based on the query params. let limit = NonZeroU32::new(123).unwrap(); - let data_page = data_page_params_nameid_name_limit(limit, &p0).unwrap(); + let data_page = data_page_params_with_limit(limit, &p0).unwrap(); + let data_page = match name_or_id_pagination(&p0, &data_page) { + Ok(PaginatedBy::Name(params, ..)) => params, + _ => { + panic!("Expected Name pagination, got Id pagination") + } + }; assert_eq!(data_page.marker, None); assert_eq!(data_page.direction, PaginationOrder::Descending); assert_eq!(data_page.limit, limit); - let data_page = data_page_params_nameid_name_limit(limit, &p1).unwrap(); - assert_eq!(data_page.marker, Some(&thinglast_name)); + let data_page = data_page_params_with_limit(limit, &p1).unwrap(); + let data_page = match name_or_id_pagination(&p1, &data_page) { + Ok(PaginatedBy::Name(params, ..)) => params, + _ => { + panic!("Expected Name pagination, got Id pagination") + } + }; + assert_eq!(data_page.marker, Some(&thinglast_name.into())); assert_eq!(data_page.direction, PaginationOrder::Descending); assert_eq!(data_page.limit, limit); - - let error = data_page_params_nameid_id_limit(limit, &p1).unwrap_err(); - assert_eq!(error.status_code, StatusCode::BAD_REQUEST); - assert_eq!(error.external_message, "invalid page token"); } #[test] fn test_scan_by_nameid_id() { // Start with the common battery of tests. - let scan = ScanByNameOrId { sort_by: NameOrIdSortMode::IdAscending }; - assert_eq!(pagination_field_for_scan_params(&scan), PagField::Id); + let scan = ScanByNameOrId { + sort_by: NameOrIdSortMode::IdAscending, + selector: (), + }; assert_eq!(scan.direction(), PaginationOrder::Ascending); let list = list_of_things(); - let thing0_marker = NameOrIdMarker::Id(list[0].identity.id); + let thing0_marker = NameOrId::Id(list[0].identity.id); let thinglast_id = list[list.len() - 1].identity.id; - let thinglast_marker = - NameOrIdMarker::Id(list[list.len() - 1].identity.id); + let thinglast_marker = NameOrId::Id(list[list.len() - 1].identity.id); let (p0, p1) = test_scan_param_common( &list, &scan, "sort_by=id_ascending", &thing0_marker, &thinglast_marker, - &ScanByNameOrId { sort_by: NameOrIdSortMode::NameAscending }, + &ScanByNameOrId { + sort_by: NameOrIdSortMode::NameAscending, + selector: (), + }, &marker_for_name_or_id, ); // Verify data pages based on the query params. let limit = NonZeroU32::new(123).unwrap(); - let data_page = data_page_params_nameid_id_limit(limit, &p0).unwrap(); + let data_page = data_page_params_with_limit(limit, &p0).unwrap(); + let data_page = match name_or_id_pagination(&p0, &data_page) { + Ok(PaginatedBy::Id(params, ..)) => params, + _ => { + panic!("Expected id pagination, got name pagination") + } + }; assert_eq!(data_page.marker, None); assert_eq!(data_page.direction, PaginationOrder::Ascending); assert_eq!(data_page.limit, limit); - let data_page = data_page_params_nameid_id_limit(limit, &p1).unwrap(); - assert_eq!(data_page.marker, Some(&thinglast_id)); + let data_page = data_page_params_with_limit(limit, &p1).unwrap(); + let data_page = match name_or_id_pagination(&p1, &data_page) { + Ok(PaginatedBy::Id(params, ..)) => params, + _ => { + panic!("Expected id pagination, got name pagination") + } + }; + assert_eq!(data_page.marker, Some(&thinglast_id.into())); assert_eq!(data_page.direction, PaginationOrder::Ascending); assert_eq!(data_page.limit, limit); - - let error = data_page_params_nameid_name_limit(limit, &p1).unwrap_err(); - assert_eq!(error.status_code, StatusCode::BAD_REQUEST); - assert_eq!(error.external_message, "invalid page token"); } } diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 6d988c61397..0ea4ffe6023 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -9,6 +9,7 @@ mod error; pub mod http_pagination; +use dropshot::HttpError; pub use error::*; use anyhow::anyhow; @@ -111,6 +112,56 @@ impl<'a, NameType> DataPageParams<'a, NameType> { } } +impl<'a> TryFrom<&DataPageParams<'a, NameOrId>> for DataPageParams<'a, Name> { + type Error = HttpError; + + fn try_from( + value: &DataPageParams<'a, NameOrId>, + ) -> Result { + match value.marker { + Some(NameOrId::Name(name)) => Ok(DataPageParams { + marker: Some(name), + direction: value.direction, + limit: value.limit, + }), + None => Ok(DataPageParams { + marker: None, + direction: value.direction, + limit: value.limit, + }), + _ => Err(HttpError::for_bad_request( + None, + String::from("invalid pagination marker"), + )), + } + } +} + +impl<'a> TryFrom<&DataPageParams<'a, NameOrId>> for DataPageParams<'a, Uuid> { + type Error = HttpError; + + fn try_from( + value: &DataPageParams<'a, NameOrId>, + ) -> Result { + match value.marker { + Some(NameOrId::Id(id)) => Ok(DataPageParams { + marker: Some(id), + direction: value.direction, + limit: value.limit, + }), + None => Ok(DataPageParams { + marker: None, + direction: value.direction, + limit: value.limit, + }), + _ => Err(HttpError::for_bad_request( + None, + String::from("invalid pagination marker"), + )), + } + } +} + /// A name used in the API /// /// Names are generally user-provided unique identifiers, highly constrained as @@ -268,7 +319,7 @@ impl Name { } } -#[derive(Serialize, Deserialize, Display, Clone)] +#[derive(Debug, Serialize, Deserialize, Display, Clone, PartialEq)] #[display("{0}")] #[serde(untagged)] pub enum NameOrId { diff --git a/common/tests/output/pagination-examples.txt b/common/tests/output/pagination-examples.txt index f9bfefbe21d..304509448a5 100644 --- a/common/tests/output/pagination-examples.txt +++ b/common/tests/output/pagination-examples.txt @@ -27,14 +27,10 @@ example pagination parameters: page selector: by name ascending example pagination parameters: page selector: by name or id, using id ascending { "sort_by": "id_ascending", - "last_seen": { - "id": "61a78113-d3c6-4b35-a410-23e9eae64328" - } + "last_seen": "61a78113-d3c6-4b35-a410-23e9eae64328" } example pagination parameters: page selector: by name or id, using id ascending { "sort_by": "name_ascending", - "last_seen": { - "name": "bort" - } + "last_seen": "bort" } diff --git a/common/tests/output/pagination-schema.txt b/common/tests/output/pagination-schema.txt index 2fdc227cdaa..732f853fd88 100644 --- a/common/tests/output/pagination-schema.txt +++ b/common/tests/output/pagination-schema.txt @@ -63,7 +63,7 @@ schema for pagination parameters: scan parameters, scan by id only schema for pagination parameters: scan parameters, scan by name or id { "$schema": "http://json-schema.org/draft-07/schema#", - "title": "ScanByNameOrId", + "title": "ScanByNameOrId_for_Null", "description": "Scan parameters for resources that support scanning by name or id", "type": "object", "properties": { @@ -196,7 +196,7 @@ schema for pagination parameters: page selector, scan by id only schema for pagination parameters: page selector, scan by name or id { "$schema": "http://json-schema.org/draft-07/schema#", - "title": "PageSelector_for_ScanByNameOrId_and_NameOrIdMarker", + "title": "PageSelector_for_ScanByNameOrId_for_Null_and_NameOrId", "description": "Specifies which page of results we're on\n\nThis type is generic over the different scan modes that we support.", "type": "object", "required": [ @@ -207,7 +207,7 @@ schema for pagination parameters: page selector, scan by name or id "description": "value of the marker field last seen by the client", "allOf": [ { - "$ref": "#/definitions/NameOrIdMarker" + "$ref": "#/definitions/NameOrId" } ] }, @@ -228,32 +228,24 @@ schema for pagination parameters: page selector, scan by name or id "maxLength": 63, "pattern": "^(?![0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$)^[a-z][a-z0-9-]*[a-zA-Z0-9]$" }, - "NameOrIdMarker": { + "NameOrId": { "oneOf": [ { - "type": "object", - "required": [ - "id" - ], - "properties": { - "id": { + "title": "id", + "allOf": [ + { "type": "string", "format": "uuid" } - }, - "additionalProperties": false + ] }, { - "type": "object", - "required": [ - "name" - ], - "properties": { - "name": { + "title": "name", + "allOf": [ + { "$ref": "#/definitions/Name" } - }, - "additionalProperties": false + ] } ] }, diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 0794f19c33c..44bb4b9f9be 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -228,7 +228,20 @@ impl super::Nexus { Ok(db_instance) } - pub async fn project_list_instances( + pub async fn project_list_instances_by_id( + &self, + opctx: &OpContext, + project_lookup: &lookup::Project<'_>, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + let (.., authz_project) = + project_lookup.lookup_for(authz::Action::ListChildren).await?; + self.db_datastore + .project_list_instances_by_id(opctx, &authz_project, pagparams) + .await + } + + pub async fn project_list_instances_by_name( &self, opctx: &OpContext, project_lookup: &lookup::Project<'_>, @@ -237,7 +250,7 @@ impl super::Nexus { let (.., authz_project) = project_lookup.lookup_for(authz::Action::ListChildren).await?; self.db_datastore - .project_list_instances(opctx, &authz_project, pagparams) + .project_list_instances_by_name(opctx, &authz_project, pagparams) .await } diff --git a/nexus/src/db/datastore/instance.rs b/nexus/src/db/datastore/instance.rs index 2b9cef3c299..917e2902294 100644 --- a/nexus/src/db/datastore/instance.rs +++ b/nexus/src/db/datastore/instance.rs @@ -112,7 +112,25 @@ impl DataStore { Ok(instance) } - pub async fn project_list_instances( + pub async fn project_list_instances_by_id( + &self, + opctx: &OpContext, + authz_project: &authz::Project, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec { + opctx.authorize(authz::Action::ListChildren, authz_project).await?; + + use db::schema::instance::dsl; + paginated(dsl::instance, dsl::id, &pagparams) + .filter(dsl::project_id.eq(authz_project.id())) + .filter(dsl::time_deleted.is_null()) + .select(Instance::as_select()) + .load_async::(self.pool_authorized(opctx).await?) + .await + .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) + } + + pub async fn project_list_instances_by_name( &self, opctx: &OpContext, authz_project: &authz::Project, @@ -122,8 +140,8 @@ impl DataStore { use db::schema::instance::dsl; paginated(dsl::instance, dsl::name, &pagparams) - .filter(dsl::time_deleted.is_null()) .filter(dsl::project_id.eq(authz_project.id())) + .filter(dsl::time_deleted.is_null()) .select(Instance::as_select()) .load_async::(self.pool_authorized(opctx).await?) .await diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 32e7c723eff..0337d6c8ac8 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -41,12 +41,10 @@ use dropshot::{ }; use ipnetwork::IpNetwork; use omicron_common::api::external::http_pagination::data_page_params_for; -use omicron_common::api::external::http_pagination::data_page_params_nameid_id; -use omicron_common::api::external::http_pagination::data_page_params_nameid_name; use omicron_common::api::external::http_pagination::marker_for_name; use omicron_common::api::external::http_pagination::marker_for_name_or_id; -use omicron_common::api::external::http_pagination::pagination_field_for_scan_params; -use omicron_common::api::external::http_pagination::PagField; +use omicron_common::api::external::http_pagination::name_or_id_pagination; +use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::http_pagination::PaginatedById; use omicron_common::api::external::http_pagination::PaginatedByName; use omicron_common::api::external::http_pagination::PaginatedByNameOrId; @@ -481,20 +479,18 @@ async fn silo_list( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let query = query_params.into_inner(); - let params = ScanByNameOrId::from_query(&query)?; - let field = pagination_field_for_scan_params(params); - - let silos = match field { - PagField::Id => { - let page_selector = data_page_params_nameid_id(&rqctx, &query)?; + let pag_params = data_page_params_for(&rqctx, &query)?; + let silos = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(page_selector, ..) => { nexus.silos_list_by_id(&opctx, &page_selector).await? } - - PagField::Name => { - let page_selector = - data_page_params_nameid_name(&rqctx, &query)? - .map_name(|n| Name::ref_cast(n)); - nexus.silos_list_by_name(&opctx, &page_selector).await? + PaginatedBy::Name(page_selector, ..) => { + nexus + .silos_list_by_name( + &opctx, + &page_selector.map_name(|n| Name::ref_cast(n)), + ) + .await? } } .into_iter() @@ -948,20 +944,18 @@ async fn organization_list_v1( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let query = query_params.into_inner(); - let params = ScanByNameOrId::from_query(&query)?; - let field = pagination_field_for_scan_params(params); - - let organizations = match field { - PagField::Id => { - let page_selector = data_page_params_nameid_id(&rqctx, &query)?; + let pag_params = data_page_params_for(&rqctx, &query)?; + let organizations = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(page_selector, ..) => { nexus.organizations_list_by_id(&opctx, &page_selector).await? } - - PagField::Name => { - let page_selector = - data_page_params_nameid_name(&rqctx, &query)? - .map_name(|n| Name::ref_cast(n)); - nexus.organizations_list_by_name(&opctx, &page_selector).await? + PaginatedBy::Name(page_selector, ..) => { + nexus + .organizations_list_by_name( + &opctx, + &page_selector.map_name(|n| Name::ref_cast(n)), + ) + .await? } } .into_iter() @@ -993,20 +987,18 @@ async fn organization_list( let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let query = query_params.into_inner(); - let params = ScanByNameOrId::from_query(&query)?; - let field = pagination_field_for_scan_params(params); - - let organizations = match field { - PagField::Id => { - let page_selector = data_page_params_nameid_id(&rqctx, &query)?; + let pag_params = data_page_params_for(&rqctx, &query)?; + let organizations = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(page_selector, ..) => { nexus.organizations_list_by_id(&opctx, &page_selector).await? } - - PagField::Name => { - let page_selector = - data_page_params_nameid_name(&rqctx, &query)? - .map_name(|n| Name::ref_cast(n)); - nexus.organizations_list_by_name(&opctx, &page_selector).await? + PaginatedBy::Name(page_selector, ..) => { + nexus + .organizations_list_by_name( + &opctx, + &page_selector.map_name(|n| Name::ref_cast(n)), + ) + .await? } } .into_iter() @@ -1433,39 +1425,34 @@ async fn organization_policy_update( }] async fn project_list_v1( rqctx: Arc>>, - query_params: Query, + query_params: Query>, ) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let query = query_params.into_inner(); + let pag_params = data_page_params_for(&rqctx, &query)?; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - let organization_lookup = - nexus.organization_lookup(&opctx, &query.organization)?; - let params = ScanByNameOrId::from_query(&query.pagination)?; - let field = pagination_field_for_scan_params(params); - let projects = match field { - PagField::Id => { - let page_selector = - data_page_params_nameid_id(&rqctx, &query.pagination)?; + let projects = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(pagparams, selector) => { + let organization_lookup = + nexus.organization_lookup(&opctx, &selector)?; nexus .projects_list_by_id( &opctx, &organization_lookup, - &page_selector, + &pagparams, ) .await? } - - PagField::Name => { - let page_selector = - data_page_params_nameid_name(&rqctx, &query.pagination)? - .map_name(|n| Name::ref_cast(n)); + PaginatedBy::Name(pagparams, selector) => { + let organization_lookup = + nexus.organization_lookup(&opctx, &selector)?; nexus .projects_list_by_name( &opctx, &organization_lookup, - &page_selector, + &pagparams.map_name(|n| Name::ref_cast(n)), ) .await? } @@ -1474,7 +1461,7 @@ async fn project_list_v1( .map(|p| p.into()) .collect(); Ok(HttpResponseOk(ScanByNameOrId::results_page( - &query.pagination, + &query, projects, &marker_for_name_or_id, )?)) @@ -1499,7 +1486,7 @@ async fn project_list( let nexus = &apictx.nexus; let query = query_params.into_inner(); let path = path_params.into_inner(); - + let pag_params = data_page_params_for(&rqctx, &query)?; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; let organization_selector = ¶ms::OrganizationSelector { @@ -1507,11 +1494,8 @@ async fn project_list( }; let organization_lookup = nexus.organization_lookup(&opctx, &organization_selector)?; - let params = ScanByNameOrId::from_query(&query)?; - let field = pagination_field_for_scan_params(params); - let projects = match field { - PagField::Id => { - let page_selector = data_page_params_nameid_id(&rqctx, &query)?; + let projects = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(page_selector, ..) => { nexus .projects_list_by_id( &opctx, @@ -1520,16 +1504,12 @@ async fn project_list( ) .await? } - - PagField::Name => { - let page_selector = - data_page_params_nameid_name(&rqctx, &query)? - .map_name(|n| Name::ref_cast(n)); + PaginatedBy::Name(page_selector, ..) => { nexus .projects_list_by_name( &opctx, &organization_lookup, - &page_selector, + &page_selector.map_name(|n| Name::ref_cast(n)), ) .await? } @@ -1975,20 +1955,20 @@ async fn ip_pool_list( let apictx = rqctx.context(); let nexus = &apictx.nexus; let query = query_params.into_inner(); + let pag_params = data_page_params_for(&rqctx, &query)?; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - let params = ScanByNameOrId::from_query(&query)?; - let field = pagination_field_for_scan_params(params); - let pools = match field { - PagField::Id => { - let page_selector = data_page_params_nameid_id(&rqctx, &query)?; + let pools = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(page_selector, ..) => { nexus.ip_pools_list_by_id(&opctx, &page_selector).await? } - PagField::Name => { - let page_selector = - data_page_params_nameid_name(&rqctx, &query)? - .map_name(|n| Name::ref_cast(n)); - nexus.ip_pools_list_by_name(&opctx, &page_selector).await? + PaginatedBy::Name(page_selector, ..) => { + nexus + .ip_pools_list_by_name( + &opctx, + &page_selector.map_name(|n| Name::ref_cast(n)), + ) + .await? } } .into_iter() @@ -2545,30 +2525,43 @@ async fn disk_metrics_list( }] async fn instance_list_v1( rqctx: Arc>>, - query_params: Query, + query_params: Query>, ) -> Result>, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let query = query_params.into_inner(); + let pag_params = data_page_params_for(&rqctx, &query)?; let handler = async { let opctx = OpContext::for_external_api(&rqctx).await?; - let project_lookup = - nexus.project_lookup(&opctx, &query.project_selector)?; - let instances = nexus - .project_list_instances( - &opctx, - &project_lookup, - &data_page_params_for(&rqctx, &query.pagination)? - .map_name(|n| Name::ref_cast(n)), - ) - .await? - .into_iter() - .map(|i| i.into()) - .collect(); - Ok(HttpResponseOk(ScanByName::results_page( - &query.pagination, + let instances = match name_or_id_pagination(&query, &pag_params)? { + PaginatedBy::Id(pag_params, selector) => { + let project_lookup = nexus.project_lookup(&opctx, &selector)?; + nexus + .project_list_instances_by_id( + &opctx, + &project_lookup, + &pag_params, + ) + .await? + } + PaginatedBy::Name(pag_params, selector) => { + let project_lookup = nexus.project_lookup(&opctx, &selector)?; + nexus + .project_list_instances_by_name( + &opctx, + &project_lookup, + &pag_params.map_name(|n| Name::ref_cast(n)), + ) + .await? + } + } + .into_iter() + .map(|i| i.into()) + .collect(); + Ok(HttpResponseOk(ScanByNameOrId::results_page( + &query, instances, - &marker_for_name, + &marker_for_name_or_id, )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await @@ -2597,7 +2590,7 @@ async fn instance_list( let opctx = OpContext::for_external_api(&rqctx).await?; let project_lookup = nexus.project_lookup(&opctx, &project_selector)?; let instances = nexus - .project_list_instances( + .project_list_instances_by_name( &opctx, &project_lookup, &data_page_params_for(&rqctx, &query)? @@ -2928,7 +2921,7 @@ async fn instance_reboot_v1( rqctx: Arc>>, query_params: Query, path_params: Path, -) -> Result, HttpError> { +) -> Result, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let path = path_params.into_inner(); @@ -2942,7 +2935,7 @@ async fn instance_reboot_v1( let instance_lookup = nexus.instance_lookup(&opctx, &instance_selector)?; let instance = nexus.instance_reboot(&opctx, &instance_lookup).await?; - Ok(HttpResponseOk(instance.into())) + Ok(HttpResponseAccepted(instance.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -2987,7 +2980,7 @@ async fn instance_start_v1( rqctx: Arc>>, query_params: Query, path_params: Path, -) -> Result, HttpError> { +) -> Result, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let path = path_params.into_inner(); @@ -3001,7 +2994,7 @@ async fn instance_start_v1( let instance_lookup = nexus.instance_lookup(&opctx, &instance_selector)?; let instance = nexus.instance_start(&opctx, &instance_lookup).await?; - Ok(HttpResponseOk(instance.into())) + Ok(HttpResponseAccepted(instance.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -3046,7 +3039,7 @@ async fn instance_stop_v1( rqctx: Arc>>, query_params: Query, path_params: Path, -) -> Result, HttpError> { +) -> Result, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let path = path_params.into_inner(); @@ -3060,7 +3053,7 @@ async fn instance_stop_v1( let instance_lookup = nexus.instance_lookup(&opctx, &instance_selector)?; let instance = nexus.instance_stop(&opctx, &instance_lookup).await?; - Ok(HttpResponseOk(instance.into())) + Ok(HttpResponseAccepted(instance.into())) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -3104,14 +3097,16 @@ async fn instance_stop( async fn instance_serial_console_v1( rqctx: Arc>>, path_params: Path, - query_params: Query, + query_params: Query, + selector_params: Query, ) -> Result, HttpError> { let apictx = rqctx.context(); let nexus = &apictx.nexus; let path = path_params.into_inner(); let query = query_params.into_inner(); + let selector = selector_params.into_inner(); let instance_selector = params::InstanceSelector { - project_selector: query.project_selector, + project_selector: selector.project_selector, instance: path.instance, }; let handler = async { @@ -3119,10 +3114,7 @@ async fn instance_serial_console_v1( let instance_lookup = nexus.instance_lookup(&opctx, &instance_selector)?; let data = nexus - .instance_serial_console_data( - &instance_lookup, - &query.console_params, - ) + .instance_serial_console_data(&instance_lookup, &query) .await?; Ok(HttpResponseOk(data)) }; diff --git a/nexus/test-utils/src/http_testing.rs b/nexus/test-utils/src/http_testing.rs index 73e66846042..02232345509 100644 --- a/nexus/test-utils/src/http_testing.rs +++ b/nexus/test-utils/src/http_testing.rs @@ -653,13 +653,19 @@ impl<'a> NexusRequest<'a> { let mut next_token: Option = None; const DEFAULT_PAGE_SIZE: usize = 10; let limit = limit.unwrap_or(DEFAULT_PAGE_SIZE); - let url_base = format!("{}?limit={}&", collection_url, limit); + let url_base = if collection_url.contains("?") { + format!("{}&limit={}", collection_url, limit) + } else { + format!("{}?limit={}", collection_url, limit) + }; loop { let url = if let Some(next_token) = &next_token { - format!("{}page_token={}", url_base, next_token) + format!("{}&page_token={}", url_base, next_token) + } else if !initial_params.is_empty() { + format!("{}&{}", url_base, initial_params) } else { - format!("{}{}", url_base, initial_params) + url_base.clone() }; let page = NexusRequest::object_get(testctx, &url) diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index 5efc14c2ec7..b429a744574 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -168,7 +168,7 @@ pub async fn create_organization( ) -> Organization { object_create( client, - "/organizations", + "/v1/organizations", ¶ms::OrganizationCreate { identity: IdentityMetadataCreateParams { name: organization_name.parse().unwrap(), @@ -184,7 +184,7 @@ pub async fn create_project( organization_name: &str, project_name: &str, ) -> Project { - let url = format!("/organizations/{}/projects", &organization_name); + let url = format!("/v1/projects?organization={}", &organization_name); object_create( client, &url, @@ -269,7 +269,7 @@ pub async fn create_instance_with( disks: Vec, ) -> Instance { let url = format!( - "/organizations/{}/projects/{}/instances", + "/v1/instances?organization={}&project={}", organization_name, project_name ); object_create( diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index f5ff9987f5c..7f3ccf8e8df 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -57,12 +57,18 @@ type ControlPlaneTestContext = static ORGANIZATION_NAME: &str = "test-org"; static PROJECT_NAME: &str = "springfield-squidport"; -fn get_project_url() -> String { - format!("/organizations/{}/projects/{}", ORGANIZATION_NAME, PROJECT_NAME) +fn get_instances_url() -> String { + format!( + "/v1/instances?organization={}&project={}", + ORGANIZATION_NAME, PROJECT_NAME + ) } -fn get_instances_url() -> String { - format!("{}/instances", get_project_url()) +fn get_instance_url(instance_name: &str) -> String { + format!( + "/v1/instances/{}?organization={}&project={}", + instance_name, ORGANIZATION_NAME, PROJECT_NAME + ) } async fn create_org_and_project(client: &ClientTestContext) -> Uuid { @@ -80,10 +86,7 @@ async fn test_instances_access_before_create_returns_not_found( // Create a project that we'll use for testing. create_organization(&client, ORGANIZATION_NAME).await; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); + let url_instances = get_instances_url(); let _ = create_project(&client, ORGANIZATION_NAME, PROJECT_NAME).await; // List instances. There aren't any yet. @@ -91,7 +94,7 @@ async fn test_instances_access_before_create_returns_not_found( assert_eq!(instances.len(), 0); // Make sure we get a 404 if we fetch one. - let instance_url = format!("{}/just-rainsticks", url_instances); + let instance_url = get_instance_url("just-rainsticks"); let error: HttpErrorResponseBody = NexusRequest::expect_failure( client, StatusCode::NOT_FOUND, @@ -194,24 +197,21 @@ async fn test_instances_create_reboot_halt( let client = &cptestctx.external_client; let apictx = &cptestctx.server.apictx; let nexus = &apictx.nexus; + let instance_name = "just-rainsticks"; create_org_and_project(&client).await; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); + let url_instances = get_instances_url(); // Create an instance. - let instance_url = format!("{}/just-rainsticks", url_instances); - let instance = create_instance( - client, - ORGANIZATION_NAME, - PROJECT_NAME, - "just-rainsticks", - ) - .await; - assert_eq!(instance.identity.name, "just-rainsticks"); - assert_eq!(instance.identity.description, "instance \"just-rainsticks\""); + let instance_url = get_instance_url(instance_name); + let instance = + create_instance(client, ORGANIZATION_NAME, PROJECT_NAME, instance_name) + .await; + assert_eq!(instance.identity.name, instance_name); + assert_eq!( + instance.identity.description, + format!("instance \"{}\"", instance_name).as_str() + ); let InstanceCpuCount(nfoundcpus) = instance.ncpus; // These particulars are hardcoded in create_instance(). assert_eq!(nfoundcpus, 4); @@ -248,7 +248,10 @@ async fn test_instances_create_reboot_halt( .unwrap() .parsed_body() .unwrap(); - assert_eq!(error.message, "already exists: instance \"just-rainsticks\""); + assert_eq!( + error.message, + format!("already exists: instance \"{}\"", instance_name).as_str() + ); // List instances again and expect to find the one we just created. let instances = instances_list(&client, &url_instances).await; @@ -290,7 +293,7 @@ async fn test_instances_create_reboot_halt( // not even the state timestamp. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Start).await; + instance_post(&client, instance_name, InstanceOp::Start).await; instances_eq(&instance, &instance_next); let instance_next = instance_get(&client, &instance_url).await; instances_eq(&instance, &instance_next); @@ -298,7 +301,7 @@ async fn test_instances_create_reboot_halt( // Reboot the instance. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Reboot).await; + instance_post(&client, instance_name, InstanceOp::Reboot).await; assert_eq!(instance_next.runtime.run_state, InstanceState::Rebooting); assert!( instance_next.runtime.time_run_state_updated @@ -326,7 +329,7 @@ async fn test_instances_create_reboot_halt( // Request a halt and verify both the immediate state and the finished state. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Stop).await; + instance_post(&client, instance_name, InstanceOp::Stop).await; assert_eq!(instance_next.runtime.run_state, InstanceState::Stopping); assert!( instance_next.runtime.time_run_state_updated @@ -346,7 +349,7 @@ async fn test_instances_create_reboot_halt( // not even the state timestamp. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Stop).await; + instance_post(&client, instance_name, InstanceOp::Stop).await; instances_eq(&instance, &instance_next); let instance_next = instance_get(&client, &instance_url).await; instances_eq(&instance, &instance_next); @@ -357,7 +360,7 @@ async fn test_instances_create_reboot_halt( client, StatusCode::BAD_REQUEST, Method::POST, - &format!("{}/reboot", &instance_url), + get_instance_url(format!("{}/reboot", instance_name).as_str()).as_str(), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -378,7 +381,7 @@ async fn test_instances_create_reboot_halt( // succeed, having stopped in between. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Start).await; + instance_post(&client, instance_name, InstanceOp::Start).await; assert_eq!(instance_next.runtime.run_state, InstanceState::Starting); assert!( instance_next.runtime.time_run_state_updated @@ -387,7 +390,7 @@ async fn test_instances_create_reboot_halt( let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Reboot).await; + instance_post(&client, instance_name, InstanceOp::Reboot).await; assert_eq!(instance_next.runtime.run_state, InstanceState::Rebooting); assert!( instance_next.runtime.time_run_state_updated @@ -417,7 +420,7 @@ async fn test_instances_create_reboot_halt( // state. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Stop).await; + instance_post(&client, instance_name, InstanceOp::Stop).await; assert_eq!(instance_next.runtime.run_state, InstanceState::Stopping); assert!( instance_next.runtime.time_run_state_updated @@ -428,7 +431,7 @@ async fn test_instances_create_reboot_halt( client, StatusCode::BAD_REQUEST, Method::POST, - &format!("{}/reboot", instance_url), + get_instance_url(format!("{}/reboot", instance_name).as_str()).as_str(), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -480,7 +483,7 @@ async fn test_instances_create_reboot_halt( client, StatusCode::NOT_FOUND, Method::POST, - &format!("{}/reboot", instance_url), + get_instance_url(format!("{}/reboot", instance_name).as_str()).as_str(), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -492,7 +495,7 @@ async fn test_instances_create_reboot_halt( client, StatusCode::NOT_FOUND, Method::POST, - &format!("{}/start", instance_url), + get_instance_url(format!("{}/start", instance_name).as_str()).as_str(), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -502,7 +505,7 @@ async fn test_instances_create_reboot_halt( client, StatusCode::NOT_FOUND, Method::POST, - &format!("{}/stop", instance_url), + get_instance_url(format!("{}/stop", instance_name).as_str()).as_str(), ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -517,6 +520,7 @@ async fn test_instances_create_stopped_start( let client = &cptestctx.external_client; let apictx = &cptestctx.server.apictx; let nexus = &apictx.nexus; + let instance_name = "just-rainsticks"; create_org_and_project(&client).await; let url_instances = format!( @@ -530,8 +534,8 @@ async fn test_instances_create_stopped_start( &url_instances, ¶ms::InstanceCreate { identity: IdentityMetadataCreateParams { - name: "just-rainsticks".parse().unwrap(), - description: "instance just-rainsticks".to_string(), + name: instance_name.parse().unwrap(), + description: format!("instance {}", instance_name), }, ncpus: InstanceCpuCount(4), memory: ByteCount::from_gibibytes_u32(1), @@ -548,9 +552,9 @@ async fn test_instances_create_stopped_start( assert_eq!(instance.runtime.run_state, InstanceState::Stopped); // Start the instance. - let instance_url = format!("{}/just-rainsticks", url_instances); + let instance_url = get_instance_url(instance_name); let instance = - instance_post(&client, &instance_url, InstanceOp::Start).await; + instance_post(&client, instance_name, InstanceOp::Start).await; // Now, simulate completion of instance boot and check the state reported. instance_simulate(nexus, &instance.identity.id).await; @@ -570,15 +574,12 @@ async fn test_instances_delete_fails_when_running_succeeds_when_stopped( let client = &cptestctx.external_client; let apictx = &cptestctx.server.apictx; let nexus = &apictx.nexus; + let instance_name = "just-rainsticks"; create_org_and_project(&client).await; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); // Create an instance. - let instance_url = format!("{}/just-rainsticks", url_instances); + let instance_url = get_instance_url(instance_name); let instance = create_instance( client, ORGANIZATION_NAME, @@ -613,7 +614,7 @@ async fn test_instances_delete_fails_when_running_succeeds_when_stopped( // Stop the instance let instance = - instance_post(&client, &instance_url, InstanceOp::Stop).await; + instance_post(&client, instance_name, InstanceOp::Stop).await; instance_simulate(nexus, &instance.identity.id).await; let instance = instance_get(&client, &instance_url).await; assert_eq!(instance.runtime.run_state, InstanceState::Stopped); @@ -1031,6 +1032,7 @@ async fn test_instance_create_delete_network_interface( ) { let client = &cptestctx.external_client; let nexus = &cptestctx.server.apictx.nexus; + let instance_name = "nic-attach-test-inst"; create_org_and_project(&client).await; let url_instances = format!( @@ -1061,7 +1063,7 @@ async fn test_instance_create_delete_network_interface( // Create an instance with no network interfaces let instance_params = params::InstanceCreate { identity: IdentityMetadataCreateParams { - name: Name::try_from(String::from("nic-attach-test-inst")).unwrap(), + name: instance_name.parse().unwrap(), description: String::from("instance to test attaching new nic"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), @@ -1080,8 +1082,6 @@ async fn test_instance_create_delete_network_interface( .await .expect("Failed to create instance with two network interfaces"); let instance = response.parsed_body::().unwrap(); - let url_instance = - format!("{}/{}", url_instances, instance.identity.name.as_str()); // Verify there are no interfaces let url_interfaces = format!( @@ -1148,8 +1148,7 @@ async fn test_instance_create_delete_network_interface( ); // Stop the instance - let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + let instance = instance_post(client, instance_name, InstanceOp::Stop).await; instance_simulate(nexus, &instance.identity.id).await; // Verify we can now make the requests again @@ -1177,7 +1176,7 @@ async fn test_instance_create_delete_network_interface( // Restart the instance, verify the interfaces are still correct. let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Start).await; + instance_post(client, instance_name, InstanceOp::Start).await; instance_simulate(nexus, &instance.identity.id).await; // Get all interfaces in one request. @@ -1219,8 +1218,7 @@ async fn test_instance_create_delete_network_interface( } // Stop the instance and verify we can delete the interface - let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + let instance = instance_post(client, instance_name, InstanceOp::Stop).await; instance_simulate(nexus, &instance.identity.id).await; // We should not be able to delete the primary interface, while the @@ -1279,12 +1277,10 @@ async fn test_instance_update_network_interfaces( ) { let client = &cptestctx.external_client; let nexus = &cptestctx.server.apictx.nexus; + let instance_name = "nic-update-test-inst"; create_org_and_project(&client).await; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); + let url_instances = get_instances_url(); // Create the VPC Subnet for the secondary interface let secondary_subnet = params::VpcSubnetCreate { @@ -1309,7 +1305,7 @@ async fn test_instance_update_network_interfaces( // Create an instance with no network interfaces let instance_params = params::InstanceCreate { identity: IdentityMetadataCreateParams { - name: Name::try_from(String::from("nic-update-test-inst")).unwrap(), + name: instance_name.parse().unwrap(), description: String::from("instance to test updatin nics"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), @@ -1328,8 +1324,6 @@ async fn test_instance_update_network_interfaces( .await .expect("Failed to create instance with two network interfaces"); let instance = response.parsed_body::().unwrap(); - let url_instance = - format!("{}/{}", url_instances, instance.identity.name.as_str()); let url_interfaces = format!( "/organizations/{}/projects/{}/instances/{}/network-interfaces", ORGANIZATION_NAME, PROJECT_NAME, instance.identity.name, @@ -1358,8 +1352,7 @@ async fn test_instance_update_network_interfaces( ]; // Stop the instance - let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + let instance = instance_post(client, instance_name, InstanceOp::Stop).await; instance_simulate(nexus, &instance.identity.id).await; // Create the first interface on the instance. @@ -1381,7 +1374,7 @@ async fn test_instance_update_network_interfaces( // Restart the instance, to ensure we can only modify things when it's // stopped. let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Start).await; + instance_post(client, instance_name, InstanceOp::Start).await; instance_simulate(nexus, &instance.identity.id).await; // We'll change the interface's name and description @@ -1419,8 +1412,7 @@ async fn test_instance_update_network_interfaces( ); // Stop the instance again, and now verify that the update works. - let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + let instance = instance_post(client, instance_name, InstanceOp::Stop).await; instance_simulate(nexus, &instance.identity.id).await; let updated_primary_iface = NexusRequest::object_put( client, @@ -1526,7 +1518,7 @@ async fn test_instance_update_network_interfaces( // Restart the instance, and verify that we can't update either interface. let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Start).await; + instance_post(client, instance_name, InstanceOp::Start).await; instance_simulate(nexus, &instance.identity.id).await; for if_name in @@ -1555,8 +1547,7 @@ async fn test_instance_update_network_interfaces( } // Stop the instance again. - let instance = - instance_post(client, url_instance.as_str(), InstanceOp::Stop).await; + let instance = instance_post(client, instance_name, InstanceOp::Stop).await; instance_simulate(nexus, &instance.identity.id).await; // Verify that we can set the secondary as the new primary, and that nothing @@ -1759,6 +1750,7 @@ async fn test_instance_with_multiple_nics_unwinds_completely( #[nexus_test] async fn test_attach_one_disk_to_instance(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; + let instance_name = "nifs"; // Test pre-reqs DiskTest::new(&cptestctx).await; @@ -1787,7 +1779,7 @@ async fn test_attach_one_disk_to_instance(cptestctx: &ControlPlaneTestContext) { // Create the instance let instance_params = params::InstanceCreate { identity: IdentityMetadataCreateParams { - name: Name::try_from(String::from("nfs")).unwrap(), + name: instance_name.parse().unwrap(), description: String::from("probably serving data"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), @@ -1804,10 +1796,7 @@ async fn test_attach_one_disk_to_instance(cptestctx: &ControlPlaneTestContext) { start: true, }; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); + let url_instances = get_instances_url(); let builder = RequestBuilder::new(client, http::Method::POST, &url_instances) .body(Some(&instance_params)) @@ -2482,6 +2471,7 @@ async fn test_disks_detached_when_instance_destroyed( cptestctx: &ControlPlaneTestContext, ) { let client = &cptestctx.external_client; + let instance_name = "nfs"; // Test pre-reqs DiskTest::new(&cptestctx).await; @@ -2521,7 +2511,7 @@ async fn test_disks_detached_when_instance_destroyed( // Boot the instance let instance_params = params::InstanceCreate { identity: IdentityMetadataCreateParams { - name: Name::try_from(String::from("nfs")).unwrap(), + name: instance_name.parse().unwrap(), description: String::from("probably serving data"), }, ncpus: InstanceCpuCount::try_from(2).unwrap(), @@ -2545,10 +2535,7 @@ async fn test_disks_detached_when_instance_destroyed( start: true, }; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); + let url_instances = get_instances_url(); let builder = RequestBuilder::new(client, http::Method::POST, &url_instances) @@ -2588,7 +2575,7 @@ async fn test_disks_detached_when_instance_destroyed( ); let instance = - instance_post(&client, &instance_url, InstanceOp::Stop).await; + instance_post(&client, instance_name, InstanceOp::Stop).await; let apictx = &cptestctx.server.apictx; let nexus = &apictx.nexus; instance_simulate(nexus, &instance.identity.id).await; @@ -2727,16 +2714,17 @@ async fn test_instance_serial(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; let apictx = &cptestctx.server.apictx; let nexus = &apictx.nexus; + let instance_name = "kris-picks"; create_org_and_project(&client).await; - let url_instances = format!( - "/organizations/{}/projects/{}/instances", - ORGANIZATION_NAME, PROJECT_NAME - ); + let instance_url = get_instance_url(instance_name); // Make sure we get a 404 if we try to access the serial console before creation. - let instance_serial_url = - format!("{}/kris-picks/serial-console?from_start=0", url_instances); + let instance_serial_url = format!( + "{}&{}", + get_instance_url(format!("{}/serial-console", instance_name).as_str()), + "from_start=0" + ); let error: HttpErrorResponseBody = NexusRequest::expect_failure( client, StatusCode::NOT_FOUND, @@ -2749,12 +2737,14 @@ async fn test_instance_serial(cptestctx: &ControlPlaneTestContext) { .unwrap() .parsed_body() .unwrap(); - assert_eq!(error.message, "not found: instance with name \"kris-picks\""); + assert_eq!( + error.message, + format!("not found: instance with name \"{}\"", instance_name).as_str() + ); // Create an instance. - let instance_url = format!("{}/kris-picks", url_instances); let instance = - create_instance(client, ORGANIZATION_NAME, PROJECT_NAME, "kris-picks") + create_instance(client, ORGANIZATION_NAME, PROJECT_NAME, instance_name) .await; // Now, simulate completion of instance boot and check the state reported. @@ -2785,7 +2775,7 @@ async fn test_instance_serial(cptestctx: &ControlPlaneTestContext) { // Request a halt and verify both the immediate state and the finished state. let instance = instance_next; let instance_next = - instance_post(&client, &instance_url, InstanceOp::Stop).await; + instance_post(&client, instance_name, InstanceOp::Stop).await; assert_eq!(instance_next.runtime.run_state, InstanceState::Stopping); assert!( instance_next.runtime.time_run_state_updated @@ -3018,17 +3008,20 @@ pub enum InstanceOp { pub async fn instance_post( client: &ClientTestContext, - instance_url: &str, + instance_name: &str, which: InstanceOp, ) -> Instance { - let url = format!( - "{}/{}", - instance_url, - match which { - InstanceOp::Start => "start", - InstanceOp::Stop => "stop", - InstanceOp::Reboot => "reboot", - } + let url = get_instance_url( + format!( + "{}/{}", + instance_name, + match which { + InstanceOp::Start => "start", + InstanceOp::Stop => "stop", + InstanceOp::Reboot => "reboot", + } + ) + .as_str(), ); NexusRequest::new( RequestBuilder::new(client, Method::POST, &url) diff --git a/nexus/tests/integration_tests/organizations.rs b/nexus/tests/integration_tests/organizations.rs index b6fff263ea8..c45cc3732f4 100644 --- a/nexus/tests/integration_tests/organizations.rs +++ b/nexus/tests/integration_tests/organizations.rs @@ -26,7 +26,7 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { create_organization(&client, &o2_name).await; // Verify GET /organizations/{org} works - let o1_url = format!("/organizations/{}", o1_name); + let o1_url = format!("/v1/organizations/{}", o1_name); let organization: Organization = NexusRequest::object_get(&client, &o1_url) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -36,7 +36,7 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { .unwrap(); assert_eq!(organization.identity.name, o1_name); - let o2_url = format!("/organizations/{}", o2_name); + let o2_url = format!("/v1/organizations/{}", o2_name); let organization: Organization = NexusRequest::object_get(&client, &o2_url) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -51,7 +51,7 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { &client, StatusCode::NOT_FOUND, Method::GET, - &"/organizations/fake-org", + &"/v1/organizations/fake-org", ) .authn_as(AuthnMode::PrivilegedUser) .execute() @@ -60,7 +60,7 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { // Verify GET /organizations works let organizations = - objects_list_page_authz::(client, "/organizations") + objects_list_page_authz::(client, "/v1/organizations") .await .items; assert_eq!(organizations.len(), 2); @@ -90,7 +90,7 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { // Verify the org is gone from the organizations list let organizations = - objects_list_page_authz::(client, "/organizations") + objects_list_page_authz::(client, "/v1/organizations") .await .items; assert_eq!(organizations.len(), 1); @@ -111,7 +111,8 @@ async fn test_organizations(cptestctx: &ControlPlaneTestContext) { // Attempt to delete a non-empty organization let project_name = "p1"; - let project_url = format!("{}/projects/{}", o2_url, project_name); + let project_url = + format!("/organizations/{}/projects/{}", o2_name, project_name); create_project(&client, &o2_name, &project_name).await; NexusRequest::expect_failure( &client, diff --git a/nexus/tests/integration_tests/projects.rs b/nexus/tests/integration_tests/projects.rs index 2ebe24b9ed3..627187ba329 100644 --- a/nexus/tests/integration_tests/projects.rs +++ b/nexus/tests/integration_tests/projects.rs @@ -40,11 +40,11 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) { create_project(&client, &org_name, &p2_name).await; - let p1_url = format!("/organizations/{}/projects/{}", org_name, p1_name); + let p1_url = format!("/v1/projects/{}?organization={}", p1_name, org_name); let project: Project = project_get(&client, &p1_url).await; assert_eq!(project.identity.name, p1_name); - let p2_url = format!("/organizations/{}/projects/{}", org_name, p2_name); + let p2_url = format!("/v1/projects/{}?organization={}", p2_name, org_name); let project: Project = project_get(&client, &p2_url).await; assert_eq!(project.identity.name, p2_name); @@ -75,7 +75,7 @@ async fn test_projects(cptestctx: &ControlPlaneTestContext) { // Make sure the list projects results for the new org make sense let projects = NexusRequest::iter_collection_authn::( &client, - &format!("/organizations/{}/projects", org2_name), + &format!("/v1/projects?organization={}", org2_name), "", None, ) diff --git a/nexus/tests/integration_tests/vpc_subnets.rs b/nexus/tests/integration_tests/vpc_subnets.rs index 67f6ed77d0b..17a968df5b8 100644 --- a/nexus/tests/integration_tests/vpc_subnets.rs +++ b/nexus/tests/integration_tests/vpc_subnets.rs @@ -38,6 +38,7 @@ async fn test_delete_vpc_subnet_with_interfaces_fails( // Create a project that we'll use for testing. let org_name = "test-org"; let project_name = "springfield-squidport"; + let instance_name = "inst"; create_organization(&client, &org_name).await; let _ = create_project(&client, org_name, project_name).await; populate_ip_pool(client, "default", None).await; @@ -56,10 +57,10 @@ async fn test_delete_vpc_subnet_with_interfaces_fails( // Create an instance in the default VPC and VPC Subnet. Verify that we // cannot delete the subnet until the instance is gone. let instance_url = format!( - "/organizations/{org_name}/projects/{project_name}/instances/inst" + "/organizations/{org_name}/projects/{project_name}/instances/{instance_name}" ); let instance = - create_instance(client, &org_name, project_name, "inst").await; + create_instance(client, &org_name, project_name, instance_name).await; instance_simulate(nexus, &instance.identity.id).await; let err: HttpErrorResponseBody = NexusRequest::expect_failure( &client, @@ -80,7 +81,7 @@ async fn test_delete_vpc_subnet_with_interfaces_fails( ); // Stop and then delete the instance - instance_post(client, &instance_url, InstanceOp::Stop).await; + instance_post(client, instance_name, InstanceOp::Stop).await; instance_simulate(&nexus, &instance.identity.id).await; NexusRequest::object_delete(&client, &instance_url) .authn_as(AuthnMode::PrivilegedUser) diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 913154d748e..efcd31f2473 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -7,7 +7,6 @@ use crate::external_api::shared; use chrono::{DateTime, Utc}; use omicron_common::api::external::{ - http_pagination::{PaginatedByName, PaginatedByNameOrId}, ByteCount, IdentityMetadataCreateParams, IdentityMetadataUpdateParams, InstanceCpuCount, Ipv4Net, Ipv6Net, Name, NameOrId, }; @@ -34,7 +33,7 @@ pub struct InstancePath { pub instance: NameOrId, } -#[derive(Deserialize, JsonSchema)] +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] pub struct OrganizationSelector { pub organization: NameOrId, } @@ -51,7 +50,7 @@ pub struct OptionalOrganizationSelector { pub organization_selector: Option, } -#[derive(Deserialize, JsonSchema)] +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema, PartialEq)] pub struct ProjectSelector { #[serde(flatten)] pub organization_selector: Option, @@ -69,14 +68,6 @@ impl ProjectSelector { } } -#[derive(Deserialize, JsonSchema)] -pub struct ProjectList { - #[serde(flatten)] - pub pagination: PaginatedByNameOrId, - #[serde(flatten)] - pub organization: OrganizationSelector, -} - #[derive(Deserialize, JsonSchema)] pub struct OptionalProjectSelector { #[serde(flatten)] @@ -105,23 +96,6 @@ impl InstanceSelector { } } -#[derive(Deserialize, JsonSchema)] -pub struct InstanceList { - #[serde(flatten)] - pub pagination: PaginatedByName, - #[serde(flatten)] - pub project_selector: ProjectSelector, -} - -#[derive(Deserialize, JsonSchema)] -pub struct InstanceSerialConsole { - #[serde(flatten)] - pub project_selector: Option, - - #[serde(flatten)] - pub console_params: InstanceSerialConsoleRequest, -} - // Silos /// Create-time parameters for a [`Silo`](crate::external_api::views::Silo) diff --git a/openapi/nexus.json b/openapi/nexus.json index 9144a3c2d04..a1c546824e2 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -7361,7 +7361,6 @@ { "in": "query", "name": "project", - "required": true, "schema": { "$ref": "#/components/schemas/NameOrId" } @@ -7370,7 +7369,7 @@ "in": "query", "name": "sort_by", "schema": { - "$ref": "#/components/schemas/NameSortMode" + "$ref": "#/components/schemas/NameOrIdSortMode" } } ], @@ -7633,8 +7632,8 @@ } ], "responses": { - "200": { - "description": "successful operation", + "202": { + "description": "successfully enqueued operation", "content": { "application/json": { "schema": { @@ -7812,8 +7811,8 @@ } ], "responses": { - "200": { - "description": "successful operation", + "202": { + "description": "successfully enqueued operation", "content": { "application/json": { "schema": { @@ -7863,8 +7862,8 @@ } ], "responses": { - "200": { - "description": "successful operation", + "202": { + "description": "successfully enqueued operation", "content": { "application/json": { "schema": { @@ -8188,7 +8187,6 @@ { "in": "query", "name": "organization", - "required": true, "schema": { "$ref": "#/components/schemas/NameOrId" }