diff --git a/common/src/api/external/http_pagination.rs b/common/src/api/external/http_pagination.rs index c2ab7f120ef..1ae6706e101 100644 --- a/common/src/api/external/http_pagination.rs +++ b/common/src/api/external/http_pagination.rs @@ -92,12 +92,6 @@ pub trait ScanParams: /// Return the direction of the scan fn direction(&self) -> PaginationOrder; - /// Given an item, return the appropriate marker value - /// - /// For example, when scanning by name, this returns the "name" field of the - /// item. - fn marker_for_item(&self, t: &T) -> Self::MarkerValue; - /// Given pagination parameters, return the current scan parameters /// /// This can fail if the pagination parameters are not self-consistent (e.g., @@ -112,28 +106,72 @@ pub trait ScanParams: /// /// `list` contains the items that should appear on the page. It's not /// expected that consumers would override this implementation. - fn results_page( + /// + /// `marker_for_item` is a function that returns the appropriate marker + /// value for a given item. For example, when scanning by name, this + /// returns the "name" field of the item. + fn results_page( query: &PaginationParams>, list: Vec, + marker_for_item: &F, ) -> Result, dropshot::HttpError> where - T: ObjectIdentity + Serialize, + F: Fn(&Self, &T) -> Self::MarkerValue, + T: Serialize, { let scan_params = Self::from_query(query)?; - ResultsPage::new(list, scan_params, page_selector_for) + let page_selector = + |item: &T, s: &Self| page_selector_for(item, s, marker_for_item); + ResultsPage::new(list, scan_params, page_selector) + } +} + +/// Marker function that extracts the "name" from an object +/// +/// This is intended for use with [`ScanByName::results_page`] with objects that +/// impl [`ObjectIdentity`]. +pub fn marker_for_name(_: &S, t: &T) -> Name { + t.identity().name.clone() +} + +/// Marker function that extracts the "id" from an object +/// +/// This is intended for use with [`ScanById::results_page`] with objects that +/// impl [`ObjectIdentity`]. +pub fn marker_for_id(_: &S, t: &T) -> Uuid { + t.identity().id +} + +/// Marker function that extracts the "name" or "id" from an object, depending +/// on the scan in use +/// +/// This is intended for use with [`ScanByNameOrId::results_page`] with objects +/// that impl [`ObjectIdentity`]. +pub fn marker_for_name_or_id( + scan: &ScanByNameOrId, + item: &T, +) -> NameOrIdMarker { + let identity = item.identity(); + match pagination_field_for_scan_params(scan) { + PagField::Name => NameOrIdMarker::Name(identity.name.clone()), + PagField::Id => NameOrIdMarker::Id(identity.id), } } /// See `dropshot::ResultsPage::new` -fn page_selector_for(item: &T, scan_params: &S) -> PageSelector +fn page_selector_for( + item: &T, + scan_params: &S, + marker_for_item: &F, +) -> PageSelector where - T: ObjectIdentity, + F: Fn(&S, &T) -> M, S: ScanParams, M: Clone + Debug + DeserializeOwned + PartialEq + Serialize, { PageSelector { scan: scan_params.clone(), - last_seen: scan_params.marker_for_item(item), + last_seen: marker_for_item(scan_params, item), } } @@ -206,9 +244,6 @@ impl ScanParams for ScanByName { fn direction(&self) -> PaginationOrder { PaginationOrder::Ascending } - fn marker_for_item(&self, item: &T) -> Name { - item.identity().name.clone() - } fn from_query( p: &PaginationParams>, ) -> Result<&Self, HttpError> { @@ -251,9 +286,6 @@ impl ScanParams for ScanById { fn direction(&self) -> PaginationOrder { PaginationOrder::Ascending } - fn marker_for_item(&self, item: &T) -> Uuid { - item.identity().id - } fn from_query(p: &PaginatedById) -> Result<&Self, HttpError> { Ok(match p.page { WhichPage::First(ref scan_params) => scan_params, @@ -337,14 +369,6 @@ impl ScanParams for ScanByNameOrId { } } - fn marker_for_item(&self, item: &T) -> NameOrIdMarker { - let identity = item.identity(); - match pagination_field_for_scan_params(self) { - PagField::Name => NameOrIdMarker::Name(identity.name.clone()), - PagField::Id => NameOrIdMarker::Id(identity.id), - } - } - fn from_query( p: &PaginationParams>, ) -> Result<&Self, HttpError> { @@ -446,6 +470,9 @@ 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; @@ -617,32 +644,34 @@ mod test { } /// Function for running a bunch of tests on a ScanParams type. - fn test_scan_param_common( + fn test_scan_param_common( list: &Vec, scan: &S, querystring: &str, item0_marker: &S::MarkerValue, itemlast_marker: &S::MarkerValue, scan_default: &S, + marker_for_item: &F, ) -> ( PaginationParams>, PaginationParams>, ) where S: ScanParams, + F: Fn(&S, &MyThing) -> S::MarkerValue, { let li = list.len() - 1; // Test basic parts of ScanParams interface. - assert_eq!(&scan.marker_for_item(&list[0]), item0_marker); - assert_eq!(&scan.marker_for_item(&list[li]), itemlast_marker); + assert_eq!(&marker_for_item(scan, &list[0]), item0_marker); + assert_eq!(&marker_for_item(scan, &list[li]), itemlast_marker); // Test page_selector_for(). - let page_selector = page_selector_for(&list[0], scan); + let page_selector = page_selector_for(&list[0], scan, marker_for_item); assert_eq!(&page_selector.scan, scan); assert_eq!(&page_selector.last_seen, item0_marker); - let page_selector = page_selector_for(&list[li], scan); + let page_selector = page_selector_for(&list[li], scan, marker_for_item); assert_eq!(&page_selector.scan, scan); assert_eq!(&page_selector.last_seen, itemlast_marker); @@ -659,7 +688,7 @@ mod test { // Generate a results page from that, verify it, pull the token out, and // use it to generate pagination parameters for a NextPage request. - let page = S::results_page(&p0, list.clone()).unwrap(); + let page = S::results_page(&p0, list.clone(), marker_for_item).unwrap(); assert_eq!(&page.items, list); assert!(page.next_page.is_some()); let q = format!("page_token={}", page.next_page.unwrap()); @@ -694,6 +723,7 @@ mod test { &"thing0".parse().unwrap(), &"thing19".parse().unwrap(), &scan, + &marker_for_name, ); assert_eq!(scan.direction(), PaginationOrder::Ascending); @@ -733,6 +763,7 @@ mod test { &list[0].identity.id, &list[list.len() - 1].identity.id, &scan, + &marker_for_id, ); assert_eq!(scan.direction(), PaginationOrder::Ascending); @@ -798,6 +829,7 @@ mod test { &thing0_marker, &thinglast_marker, &ScanByNameOrId { sort_by: NameOrIdSortMode::NameAscending }, + &marker_for_name_or_id, ); // Verify data pages based on the query params. @@ -836,6 +868,7 @@ mod test { &thing0_marker, &thinglast_marker, &ScanByNameOrId { sort_by: NameOrIdSortMode::NameAscending }, + &marker_for_name_or_id, ); // Verify data pages based on the query params. diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index dbd99f21377..7db5cd43d17 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -867,44 +867,15 @@ impl DiskState { // These are currently only intended for observability by developers. We will // eventually want to flesh this out into something more observable for end // users. -#[derive(ObjectIdentity, Clone, Debug, Serialize, JsonSchema)] +#[derive(Clone, Debug, Serialize, JsonSchema)] pub struct Saga { pub id: Uuid, pub state: SagaState, - // TODO-cleanup This object contains a fake `IdentityMetadata`. Why? We - // want to paginate these objects. http_pagination.rs provides a bunch of - // useful facilities -- notably `PaginatedById`. `PaginatedById` - // requires being able to take an arbitrary object in the result set and get - // its id. To do that, it uses the `ObjectIdentity` trait, which expects - // to be able to return an `IdentityMetadata` reference from an object. - // Finally, the pagination facilities just pull the `id` out of that. - // - // In this case (as well as others, like sleds and racks), we have ids, and - // we want to be able to paginate by id, but we don't have full identity - // metadata. (Or we do, but it's similarly faked up.) What we should - // probably do is create a new trait, say `ObjectId`, that returns _just_ - // an id. We can provide a blanket impl for anything that impls - // IdentityMetadata. We can define one-off impls for structs like this - // one. Then the id-only pagination interfaces can require just - // `ObjectId`. - #[serde(skip)] - pub identity: IdentityMetadata, } impl From for Saga { fn from(s: steno::SagaView) -> Self { - Saga { - id: Uuid::from(s.id), - state: SagaState::from(s.state), - identity: IdentityMetadata { - // TODO-cleanup See the note in Saga above. - id: Uuid::from(s.id), - name: format!("saga-{}", s.id).parse().unwrap(), - description: format!("saga {}", s.id), - time_created: Utc::now(), - time_modified: Utc::now(), - }, - } + Saga { id: Uuid::from(s.id), state: SagaState::from(s.state) } } } diff --git a/nexus/src/authn/mod.rs b/nexus/src/authn/mod.rs index 59e5bc7a889..375ae70e583 100644 --- a/nexus/src/authn/mod.rs +++ b/nexus/src/authn/mod.rs @@ -186,7 +186,7 @@ impl Context { Context { kind: Kind::Authenticated(Details { actor: Actor::SiloUser { - silo_user_id: USER_TEST_PRIVILEGED.identity().id, + silo_user_id: USER_TEST_PRIVILEGED.id(), silo_id: USER_TEST_PRIVILEGED.silo_id, }, }), @@ -201,7 +201,7 @@ impl Context { Context { kind: Kind::Authenticated(Details { actor: Actor::SiloUser { - silo_user_id: USER_TEST_UNPRIVILEGED.identity().id, + silo_user_id: USER_TEST_UNPRIVILEGED.id(), silo_id: USER_TEST_UNPRIVILEGED.silo_id, }, }), diff --git a/nexus/src/db/identity.rs b/nexus/src/db/identity.rs index 65e4002e4e6..ca7b1817dfd 100644 --- a/nexus/src/db/identity.rs +++ b/nexus/src/db/identity.rs @@ -49,14 +49,4 @@ pub trait Asset { fn id(&self) -> Uuid; fn time_created(&self) -> DateTime; fn time_modified(&self) -> DateTime; - - fn identity(&self) -> external::IdentityMetadata { - external::IdentityMetadata { - id: self.id(), - name: "no-name".parse().unwrap(), - description: "no description".to_string(), - time_created: self.time_created(), - time_modified: self.time_modified(), - } - } } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index b47e6ecf12c..7dc4af51015 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -36,6 +36,8 @@ use dropshot::TypedBody; use dropshot::WhichPage; 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::PaginatedById; @@ -335,7 +337,11 @@ async fn silos_get( .into_iter() .map(|p| p.into()) .collect(); - Ok(HttpResponseOk(ScanByNameOrId::results_page(&query, silos)?)) + Ok(HttpResponseOk(ScanByNameOrId::results_page( + &query, + silos, + &marker_for_name_or_id, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -495,6 +501,7 @@ async fn silos_get_identity_providers( Ok(HttpResponseOk(ScanByName::results_page( &query, identity_providers, + &marker_for_name, )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await @@ -603,7 +610,11 @@ async fn organizations_get( .into_iter() .map(|p| p.into()) .collect(); - Ok(HttpResponseOk(ScanByNameOrId::results_page(&query, organizations)?)) + Ok(HttpResponseOk(ScanByNameOrId::results_page( + &query, + organizations, + &marker_for_name_or_id, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -821,7 +832,11 @@ async fn organization_projects_get( .into_iter() .map(|p| p.into()) .collect(); - Ok(HttpResponseOk(ScanByNameOrId::results_page(&query, projects)?)) + Ok(HttpResponseOk(ScanByNameOrId::results_page( + &query, + projects, + &marker_for_name_or_id, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -1043,7 +1058,11 @@ async fn project_disks_get( .into_iter() .map(|d| d.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, disks)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + disks, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -1179,7 +1198,11 @@ async fn project_instances_get( .into_iter() .map(|i| i.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, instances)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + instances, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -1488,7 +1511,11 @@ async fn instance_disks_get( .into_iter() .map(|d| d.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, disks)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + disks, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -1587,7 +1614,11 @@ async fn images_get( .into_iter() .map(|d| d.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, images)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + images, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -1706,7 +1737,11 @@ async fn project_images_get( .into_iter() .map(|d| d.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, images)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + images, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -1858,7 +1893,11 @@ async fn instance_network_interfaces_get( .into_iter() .map(|d| d.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, interfaces)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + interfaces, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -2044,7 +2083,11 @@ async fn project_snapshots_get( .into_iter() .map(|d| d.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, snapshots)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + snapshots, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -2185,7 +2228,11 @@ async fn project_vpcs_get( .map(|p| p.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, vpcs)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + vpcs, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -2347,7 +2394,11 @@ async fn vpc_subnets_get( .into_iter() .map(|vpc| vpc.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, vpcs)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + vpcs, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -2511,7 +2562,11 @@ async fn subnet_network_interfaces_get( .into_iter() .map(|interfaces| interfaces.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, interfaces)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + interfaces, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -2617,7 +2672,11 @@ async fn vpc_routers_get( .into_iter() .map(|s| s.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, routers)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + routers, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -2784,7 +2843,11 @@ async fn routers_routes_get( .into_iter() .map(|route| route.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, routes)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + routes, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -2944,7 +3007,11 @@ async fn hardware_racks_get( .racks_list(&opctx, &data_page_params_for(&rqctx, &query)?) .await?; let view_list = to_list::(rack_stream).await; - Ok(HttpResponseOk(ScanById::results_page(&query, view_list)?)) + Ok(HttpResponseOk(ScanById::results_page( + &query, + view_list, + &|_, rack: &Rack| rack.identity.id, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -3000,7 +3067,11 @@ async fn hardware_sleds_get( .into_iter() .map(|s| s.into()) .collect(); - Ok(HttpResponseOk(ScanById::results_page(&query, sleds)?)) + Ok(HttpResponseOk(ScanById::results_page( + &query, + sleds, + &|_, sled: &Sled| sled.identity.id, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -3074,7 +3145,11 @@ async fn sagas_get( let opctx = OpContext::for_external_api(&rqctx).await?; let saga_stream = nexus.sagas_list(&opctx, &pagparams).await?; let view_list = to_list(saga_stream).await; - Ok(HttpResponseOk(ScanById::results_page(&query, view_list)?)) + Ok(HttpResponseOk(ScanById::results_page( + &query, + view_list, + &|_, saga: &Saga| saga.id, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -3131,7 +3206,11 @@ async fn users_get( .into_iter() .map(|i| i.into()) .collect(); - Ok(HttpResponseOk(ScanByName::results_page(&query, users)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + users, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } @@ -3299,7 +3378,11 @@ async fn sshkeys_get( .into_iter() .map(SshKey::from) .collect::>(); - Ok(HttpResponseOk(ScanByName::results_page(&query, ssh_keys)?)) + Ok(HttpResponseOk(ScanByName::results_page( + &query, + ssh_keys, + &marker_for_name, + )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } diff --git a/nexus/src/external_api/views.rs b/nexus/src/external_api/views.rs index f758e8f4e16..2693fb485de 100644 --- a/nexus/src/external_api/views.rs +++ b/nexus/src/external_api/views.rs @@ -17,6 +17,33 @@ use serde::{Deserialize, Serialize}; use std::net::SocketAddrV6; use uuid::Uuid; +// IDENTITY METADATA + +/// Identity-related metadata that's included in "asset" public API objects +/// (which generally have no name or description) +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize, JsonSchema)] +pub struct AssetIdentityMetadata { + /// unique, immutable, system-controlled identifier for each resource + pub id: Uuid, + /// timestamp when this resource was created + pub time_created: chrono::DateTime, + /// timestamp when this resource was last modified + pub time_modified: chrono::DateTime, +} + +impl From<&T> for AssetIdentityMetadata +where + T: Asset, +{ + fn from(t: &T) -> Self { + AssetIdentityMetadata { + id: t.id(), + time_created: t.time_created(), + time_modified: t.time_modified(), + } + } +} + // SILOS /// Client view of a ['Silo'] @@ -318,31 +345,34 @@ impl From for VpcRouter { // RACKS /// Client view of an [`Rack`] -#[derive(ObjectIdentity, Clone, Debug, Deserialize, Serialize, JsonSchema)] +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct Rack { #[serde(flatten)] - pub identity: IdentityMetadata, + pub identity: AssetIdentityMetadata, } impl From for Rack { fn from(rack: model::Rack) -> Self { - Self { identity: rack.identity() } + Self { identity: AssetIdentityMetadata::from(&rack) } } } // SLEDS /// Client view of an [`Sled`] -#[derive(ObjectIdentity, Clone, Debug, Deserialize, Serialize, JsonSchema)] +#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct Sled { #[serde(flatten)] - pub identity: IdentityMetadata, + pub identity: AssetIdentityMetadata, pub service_address: SocketAddrV6, } impl From for Sled { fn from(sled: model::Sled) -> Self { - Self { identity: sled.identity(), service_address: sled.address() } + Self { + identity: AssetIdentityMetadata::from(&sled), + service_address: sled.address(), + } } } diff --git a/openapi/nexus.json b/openapi/nexus.json index 3a9c4e71060..b34b429cb00 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -7523,23 +7523,11 @@ "description": "Client view of an [`Rack`]", "type": "object", "properties": { - "description": { - "description": "human-readable free-form text about a resource", - "type": "string" - }, "id": { "description": "unique, immutable, system-controlled identifier for each resource", "type": "string", "format": "uuid" }, - "name": { - "description": "unique, mutable, user-controlled identifier for each resource", - "allOf": [ - { - "$ref": "#/components/schemas/Name" - } - ] - }, "time_created": { "description": "timestamp when this resource was created", "type": "string", @@ -7552,9 +7540,7 @@ } }, "required": [ - "description", "id", - "name", "time_created", "time_modified" ] @@ -8409,23 +8395,11 @@ "description": "Client view of an [`Sled`]", "type": "object", "properties": { - "description": { - "description": "human-readable free-form text about a resource", - "type": "string" - }, "id": { "description": "unique, immutable, system-controlled identifier for each resource", "type": "string", "format": "uuid" }, - "name": { - "description": "unique, mutable, user-controlled identifier for each resource", - "allOf": [ - { - "$ref": "#/components/schemas/Name" - } - ] - }, "service_address": { "type": "string" }, @@ -8441,9 +8415,7 @@ } }, "required": [ - "description", "id", - "name", "service_address", "time_created", "time_modified"