From 270133ea95166b0edc8efd1585b061ba9121f55e Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 18 Jan 2024 11:44:24 -0600 Subject: [PATCH 1/6] first pass at list IP pools for silo endpoint --- nexus/db-queries/src/db/datastore/ip_pool.rs | 38 +++++- nexus/src/app/ip_pool.rs | 17 ++- nexus/src/external_api/http_entrypoints.rs | 46 ++++++- nexus/tests/output/nexus_tags.txt | 1 + nexus/types/src/external_api/views.rs | 16 +++ openapi/nexus.json | 136 ++++++++++++++++++- 6 files changed, 244 insertions(+), 10 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index c9fdb5f0ee..a7f715771d 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -80,7 +80,7 @@ impl DataStore { } /// List IP pools linked to the current silo - pub async fn silo_ip_pools_list( + pub async fn current_silo_ip_pool_list( &self, opctx: &OpContext, pagparams: &PaginatedBy<'_>, @@ -400,6 +400,34 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } + /// Returns (IpPool, IpPoolResource) so we can know in the calling code + /// whether the pool is default for the silo + pub async fn silo_ip_pool_list( + &self, + opctx: &OpContext, + authz_silo: &authz::Silo, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec<(IpPool, IpPoolResource)> { + use db::schema::ip_pool; + use db::schema::ip_pool_resource; + + paginated( + ip_pool_resource::table, + ip_pool_resource::ip_pool_id, + pagparams, + ) + .inner_join(ip_pool::table) + .filter(ip_pool_resource::resource_id.eq(authz_silo.id())) + .filter(ip_pool_resource::resource_type.eq(IpPoolResourceType::Silo)) + .filter(ip_pool::time_deleted.is_null()) + .select(<(IpPool, IpPoolResource)>::as_select()) + .load_async::<(IpPool, IpPoolResource)>( + &*self.pool_connection_authorized(opctx).await?, + ) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) + } + pub async fn ip_pool_link_silo( &self, opctx: &OpContext, @@ -868,7 +896,7 @@ mod test { .expect("Should list IP pools"); assert_eq!(all_pools.len(), 0); let silo_pools = datastore - .silo_ip_pools_list(&opctx, &pagbyid) + .current_silo_ip_pool_list(&opctx, &pagbyid) .await .expect("Should list silo IP pools"); assert_eq!(silo_pools.len(), 0); @@ -893,7 +921,7 @@ mod test { .expect("Should list IP pools"); assert_eq!(all_pools.len(), 1); let silo_pools = datastore - .silo_ip_pools_list(&opctx, &pagbyid) + .current_silo_ip_pool_list(&opctx, &pagbyid) .await .expect("Should list silo IP pools"); assert_eq!(silo_pools.len(), 0); @@ -929,7 +957,7 @@ mod test { // now it shows up in the silo list let silo_pools = datastore - .silo_ip_pools_list(&opctx, &pagbyid) + .current_silo_ip_pool_list(&opctx, &pagbyid) .await .expect("Should list silo IP pools"); assert_eq!(silo_pools.len(), 1); @@ -998,7 +1026,7 @@ mod test { // and silo pools list is empty again let silo_pools = datastore - .silo_ip_pools_list(&opctx, &pagbyid) + .current_silo_ip_pool_list(&opctx, &pagbyid) .await .expect("Should list silo IP pools"); assert_eq!(silo_pools.len(), 0); diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs index 1d9b3e515e..876728fa4c 100644 --- a/nexus/src/app/ip_pool.rs +++ b/nexus/src/app/ip_pool.rs @@ -74,12 +74,12 @@ impl super::Nexus { } /// List IP pools in current silo - pub(crate) async fn silo_ip_pools_list( + pub(crate) async fn current_silo_ip_pool_list( &self, opctx: &OpContext, pagparams: &PaginatedBy<'_>, ) -> ListResultVec { - self.db_datastore.silo_ip_pools_list(opctx, pagparams).await + self.db_datastore.current_silo_ip_pool_list(opctx, pagparams).await } // Look up pool by name or ID, but only return it if it's linked to the @@ -101,6 +101,7 @@ impl super::Nexus { Ok(pool) } + /// List silos for a given pool pub(crate) async fn ip_pool_silo_list( &self, opctx: &OpContext, @@ -112,6 +113,18 @@ impl super::Nexus { self.db_datastore.ip_pool_silo_list(opctx, &authz_pool, pagparams).await } + // List pools for a given silo + pub(crate) async fn silo_ip_pool_list( + &self, + opctx: &OpContext, + silo_lookup: &lookup::Silo<'_>, + pagparams: &DataPageParams<'_, Uuid>, + ) -> ListResultVec<(db::model::IpPool, db::model::IpPoolResource)> { + let (.., authz_silo) = + silo_lookup.lookup_for(authz::Action::Read).await?; + self.db_datastore.silo_ip_pool_list(opctx, &authz_silo, pagparams).await + } + pub(crate) async fn ip_pool_link_silo( &self, opctx: &OpContext, diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 21acb45ed3..ca569553fe 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -279,6 +279,7 @@ pub(crate) fn external_api() -> NexusApiDescription { api.register(silo_delete)?; api.register(silo_policy_view)?; api.register(silo_policy_update)?; + api.register(silo_ip_pool_list)?; api.register(silo_utilization_view)?; api.register(silo_utilization_list)?; @@ -741,7 +742,7 @@ async fn silo_create( /// Fetch a silo /// -/// Fetch a silo by name. +/// Fetch a silo by name or ID. #[endpoint { method = GET, path = "/v1/system/silos/{silo}", @@ -763,6 +764,46 @@ async fn silo_view( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +/// List IP pools available within silo +#[endpoint { + method = GET, + path = "/v1/system/silos/{silo}/ip-pools", + tags = ["system/silos"], +}] +async fn silo_ip_pool_list( + rqctx: RequestContext>, + path_params: Path, + query_params: Query, +) -> Result>, HttpError> { + let apictx = rqctx.context(); + let handler = async { + let opctx = crate::context::op_context_for_external_api(&rqctx).await?; + let nexus = &apictx.nexus; + let path = path_params.into_inner(); + + let query = query_params.into_inner(); + let pag_params = data_page_params_for(&rqctx, &query)?; + + let silo_lookup = nexus.silo_lookup(&opctx, path.silo)?; + let pools = nexus + .silo_ip_pool_list(&opctx, &silo_lookup, &pag_params) + .await? + .iter() + .map(|(pool, silo_link)| views::SiloIpPool { + identity: pool.identity(), + is_default: silo_link.is_default, + }) + .collect(); + + Ok(HttpResponseOk(ScanById::results_page( + &query, + pools, + &|_, pool: &views::SiloIpPool| pool.identity.id, + )?)) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + /// Delete a silo /// /// Delete a silo by name. @@ -1302,6 +1343,7 @@ async fn project_policy_update( async fn project_ip_pool_list( rqctx: RequestContext>, query_params: Query, + // TODO: have this return SiloIpPool so it has is_default on it ) -> Result>, HttpError> { let apictx = rqctx.context(); let handler = async { @@ -1312,7 +1354,7 @@ async fn project_ip_pool_list( let paginated_by = name_or_id_pagination(&pag_params, scan_params)?; let opctx = crate::context::op_context_for_external_api(&rqctx).await?; let pools = nexus - .silo_ip_pools_list(&opctx, &paginated_by) + .current_silo_ip_pool_list(&opctx, &paginated_by) .await? .into_iter() .map(IpPool::from) diff --git a/nexus/tests/output/nexus_tags.txt b/nexus/tests/output/nexus_tags.txt index 2d842dd930..bd79a9c3e9 100644 --- a/nexus/tests/output/nexus_tags.txt +++ b/nexus/tests/output/nexus_tags.txt @@ -185,6 +185,7 @@ saml_identity_provider_view GET /v1/system/identity-providers/ silo_create POST /v1/system/silos silo_delete DELETE /v1/system/silos/{silo} silo_identity_provider_list GET /v1/system/identity-providers +silo_ip_pool_list GET /v1/system/silos/{silo}/ip-pools silo_list GET /v1/system/silos silo_policy_update PUT /v1/system/silos/{silo}/policy silo_policy_view GET /v1/system/silos/{silo}/policy diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index cf312d3b82..cc0e8c62c1 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -303,6 +303,22 @@ pub struct IpPool { pub identity: IdentityMetadata, } +/// A collection of IP ranges. If a pool is linked to a silo, IP addresses from +/// the pool can be allocated within that silo +#[derive(ObjectIdentity, Clone, Debug, Deserialize, Serialize, JsonSchema)] +pub struct SiloIpPool { + #[serde(flatten)] + pub identity: IdentityMetadata, + + /// When a pool is the default for a silo, floating IPs and instance + /// ephemeral IPs will come from that pool when no other pool is specified. + /// There can be at most one default for a given silo. + pub is_default: bool, +} + +// TODO: rename IpPoolSilo or get rid of it somehow. we cannot have both +// IpPoolSilo and SiloIpPool. come on + /// A link between an IP pool and a silo that allows one to allocate IPs from /// the pool within the silo #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] diff --git a/openapi/nexus.json b/openapi/nexus.json index a4ba6cbb86..21d9a98f51 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -6580,7 +6580,7 @@ "system/silos" ], "summary": "Fetch a silo", - "description": "Fetch a silo by name.", + "description": "Fetch a silo by name or ID.", "operationId": "silo_view", "parameters": [ { @@ -6643,6 +6643,74 @@ } } }, + "/v1/system/silos/{silo}/ip-pools": { + "get": { + "tags": [ + "system/silos" + ], + "summary": "List IP pools available within silo", + "operationId": "silo_ip_pool_list", + "parameters": [ + { + "in": "path", + "name": "silo", + "description": "Name or ID of the silo", + "required": true, + "schema": { + "$ref": "#/components/schemas/NameOrId" + } + }, + { + "in": "query", + "name": "limit", + "description": "Maximum number of items returned by a single call", + "schema": { + "nullable": true, + "type": "integer", + "format": "uint32", + "minimum": 1 + } + }, + { + "in": "query", + "name": "page_token", + "description": "Token returned by previous call to retrieve the subsequent page", + "schema": { + "nullable": true, + "type": "string" + } + }, + { + "in": "query", + "name": "sort_by", + "schema": { + "$ref": "#/components/schemas/IdSortMode" + } + } + ], + "responses": { + "200": { + "description": "successful operation", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/SiloIpPoolResultsPage" + } + } + } + }, + "4XX": { + "$ref": "#/components/responses/Error" + }, + "5XX": { + "$ref": "#/components/responses/Error" + } + }, + "x-dropshot-pagination": { + "required": [] + } + } + }, "/v1/system/silos/{silo}/policy": { "get": { "tags": [ @@ -13802,6 +13870,72 @@ } ] }, + "SiloIpPool": { + "description": "A collection of IP ranges. If a pool is linked to a silo, IP addresses from the pool can be allocated within that silo", + "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" + }, + "is_default": { + "description": "When a pool is the default for a silo, floating IPs and instance ephemeral IPs will come from that pool when no other pool is specified. There can be at most one default for a given silo.", + "type": "boolean" + }, + "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", + "format": "date-time" + }, + "time_modified": { + "description": "timestamp when this resource was last modified", + "type": "string", + "format": "date-time" + } + }, + "required": [ + "description", + "id", + "is_default", + "name", + "time_created", + "time_modified" + ] + }, + "SiloIpPoolResultsPage": { + "description": "A single page of results", + "type": "object", + "properties": { + "items": { + "description": "list of items on this page of results", + "type": "array", + "items": { + "$ref": "#/components/schemas/SiloIpPool" + } + }, + "next_page": { + "nullable": true, + "description": "token used to fetch the next page of results (if any)", + "type": "string" + } + }, + "required": [ + "items" + ] + }, "SiloQuotas": { "description": "A collection of resource counts used to set the virtual capacity of a silo", "type": "object", From 532dd5ad0ea6da45d873f380da0eef1be5ca4c12 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 18 Jan 2024 12:50:17 -0600 Subject: [PATCH 2/6] project IP pools endpoint also includes is_default --- nexus/db-queries/src/db/datastore/ip_pool.rs | 14 ++++++-------- nexus/src/app/ip_pool.rs | 2 +- nexus/src/external_api/http_entrypoints.rs | 15 ++++++++++++--- nexus/tests/integration_tests/ip_pools.rs | 7 +++++-- 4 files changed, 24 insertions(+), 14 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index a7f715771d..a6d7b6bc20 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -84,7 +84,7 @@ impl DataStore { &self, opctx: &OpContext, pagparams: &PaginatedBy<'_>, - ) -> ListResultVec { + ) -> ListResultVec<(IpPool, IpPoolResource)> { use db::schema::ip_pool; use db::schema::ip_pool_resource; @@ -108,13 +108,10 @@ impl DataStore { ), } .inner_join(ip_pool_resource::table) - .filter( - ip_pool_resource::resource_type - .eq(IpPoolResourceType::Silo) - .and(ip_pool_resource::resource_id.eq(silo_id)), - ) + .filter(ip_pool_resource::resource_type.eq(IpPoolResourceType::Silo)) + .filter(ip_pool_resource::resource_id.eq(silo_id)) .filter(ip_pool::time_deleted.is_null()) - .select(db::model::IpPool::as_select()) + .select(<(IpPool, IpPoolResource)>::as_select()) .get_results_async(&*self.pool_connection_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) @@ -961,7 +958,8 @@ mod test { .await .expect("Should list silo IP pools"); assert_eq!(silo_pools.len(), 1); - assert_eq!(silo_pools[0].id(), pool1_for_silo.id()); + assert_eq!(silo_pools[0].0.id(), pool1_for_silo.id()); + assert_eq!(silo_pools[0].1.is_default, false); // linking an already linked silo errors due to PK conflict let err = datastore diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs index 876728fa4c..d1a9ecfbd2 100644 --- a/nexus/src/app/ip_pool.rs +++ b/nexus/src/app/ip_pool.rs @@ -78,7 +78,7 @@ impl super::Nexus { &self, opctx: &OpContext, pagparams: &PaginatedBy<'_>, - ) -> ListResultVec { + ) -> ListResultVec<(db::model::IpPool, db::model::IpPoolResource)> { self.db_datastore.current_silo_ip_pool_list(opctx, pagparams).await } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index ca569553fe..992c93c4a1 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1343,8 +1343,7 @@ async fn project_policy_update( async fn project_ip_pool_list( rqctx: RequestContext>, query_params: Query, - // TODO: have this return SiloIpPool so it has is_default on it -) -> Result>, HttpError> { +) -> Result>, HttpError> { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.nexus; @@ -1357,7 +1356,10 @@ async fn project_ip_pool_list( .current_silo_ip_pool_list(&opctx, &paginated_by) .await? .into_iter() - .map(IpPool::from) + .map(|(pool, silo_link)| views::SiloIpPool { + identity: pool.identity(), + is_default: silo_link.is_default, + }) .collect(); Ok(HttpResponseOk(ScanByNameOrId::results_page( &query, @@ -1531,6 +1533,13 @@ async fn ip_pool_silo_list( // option would be to paginate by a composite key representing the (pool, // resource_type, resource) query_params: Query, + // TODO: this could just list views::Silo -- it's not like knowing silo_id + // and nothing else is particularly useful -- except we also want to say + // whether the pool is marked default on each silo. So one option would + // be to do the same as we did with SiloIpPool -- include is_default on + // whatever the thing is. Still... all we'd have to do to make this usable + // in both places would be to make it { ...IpPool, silo_id, silo_name, + // is_default } ) -> Result>, HttpError> { let apictx = rqctx.context(); let handler = async { diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index d97eda9a0b..440376361f 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -42,6 +42,7 @@ use nexus_types::external_api::views::IpPool; use nexus_types::external_api::views::IpPoolRange; use nexus_types::external_api::views::IpPoolSilo; use nexus_types::external_api::views::Silo; +use nexus_types::external_api::views::SiloIpPool; use nexus_types::identity::Resource; use omicron_common::api::external::IdentityMetadataUpdateParams; use omicron_common::api::external::NameOrId; @@ -933,12 +934,14 @@ async fn test_ip_pool_list_in_silo(cptestctx: &ControlPlaneTestContext) { ); create_ip_pool(client, otherpool_name, Some(otherpool_range)).await; - let list = - objects_list_page_authz::(client, "/v1/ip-pools").await.items; + let list = objects_list_page_authz::(client, "/v1/ip-pools") + .await + .items; // only mypool shows up because it's linked to my silo assert_eq!(list.len(), 1); assert_eq!(list[0].identity.name.to_string(), mypool_name); + assert!(list[0].is_default); // fetch the pool directly too let url = format!("/v1/ip-pools/{}", mypool_name); From 06d1fea6a61517a190b1e256ae530d5ae947dd4c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 18 Jan 2024 15:45:23 -0600 Subject: [PATCH 3/6] /v1/ip-pools/{pool} responds with SiloIpPool too --- nexus/src/app/ip_pool.rs | 9 ++++----- nexus/src/external_api/http_entrypoints.rs | 10 +++++++--- nexus/tests/integration_tests/endpoints.rs | 10 ++++++++++ nexus/tests/integration_tests/ip_pools.rs | 3 ++- nexus/types/src/external_api/views.rs | 3 +-- openapi/nexus.json | 6 +++--- 6 files changed, 27 insertions(+), 14 deletions(-) diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs index d1a9ecfbd2..1aceb3c4a3 100644 --- a/nexus/src/app/ip_pool.rs +++ b/nexus/src/app/ip_pool.rs @@ -88,17 +88,16 @@ impl super::Nexus { &'a self, opctx: &'a OpContext, pool: &'a NameOrId, - ) -> LookupResult { + ) -> LookupResult<(db::model::IpPool, db::model::IpPoolResource)> { let (authz_pool, pool) = self.ip_pool_lookup(opctx, pool)?.fetch().await?; // 404 if no link is found in the current silo let link = self.db_datastore.ip_pool_fetch_link(opctx, pool.id()).await; - if link.is_err() { - return Err(authz_pool.not_found()); + match link { + Ok(link) => Ok((pool, link)), + Err(_) => Err(authz_pool.not_found()), } - - Ok(pool) } /// List silos for a given pool diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 992c93c4a1..d1fb5759d5 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1379,14 +1379,18 @@ async fn project_ip_pool_list( async fn project_ip_pool_view( rqctx: RequestContext>, path_params: Path, -) -> Result, HttpError> { +) -> Result, HttpError> { let apictx = rqctx.context(); let handler = async { let opctx = crate::context::op_context_for_external_api(&rqctx).await?; let nexus = &apictx.nexus; let pool_selector = path_params.into_inner().pool; - let pool = nexus.silo_ip_pool_fetch(&opctx, &pool_selector).await?; - Ok(HttpResponseOk(IpPool::from(pool))) + let (pool, silo_link) = + nexus.silo_ip_pool_fetch(&opctx, &pool_selector).await?; + Ok(HttpResponseOk(views::SiloIpPool { + identity: pool.identity(), + is_default: silo_link.is_default, + })) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index 11bfa34c5f..e8d54b876e 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -80,6 +80,8 @@ pub static DEMO_SILO_NAME: Lazy = Lazy::new(|| "demo-silo".parse().unwrap()); pub static DEMO_SILO_URL: Lazy = Lazy::new(|| format!("/v1/system/silos/{}", *DEMO_SILO_NAME)); +pub static DEMO_SILO_IP_POOLS_URL: Lazy = + Lazy::new(|| format!("{}/ip-pools", *DEMO_SILO_URL)); pub static DEMO_SILO_POLICY_URL: Lazy = Lazy::new(|| format!("/v1/system/silos/{}/policy", *DEMO_SILO_NAME)); pub static DEMO_SILO_QUOTAS_URL: Lazy = @@ -1110,6 +1112,14 @@ pub static VERIFY_ENDPOINTS: Lazy> = Lazy::new(|| { AllowedMethod::Delete, ], }, + VerifyEndpoint { + url: &DEMO_SILO_IP_POOLS_URL, + visibility: Visibility::Protected, + unprivileged_access: UnprivilegedAccess::None, + allowed_methods: vec![ + AllowedMethod::Get, + ], + }, VerifyEndpoint { url: &DEMO_SILO_POLICY_URL, visibility: Visibility::Protected, diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index 440376361f..43499eb5a9 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -945,8 +945,9 @@ async fn test_ip_pool_list_in_silo(cptestctx: &ControlPlaneTestContext) { // fetch the pool directly too let url = format!("/v1/ip-pools/{}", mypool_name); - let pool: IpPool = object_get(client, &url).await; + let pool = object_get::(client, &url).await; assert_eq!(pool.identity.name.as_str(), mypool_name); + assert!(pool.is_default); // fetching the other pool directly 404s let url = format!("/v1/ip-pools/{}", otherpool_name); diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index cc0e8c62c1..b39a7031b6 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -303,8 +303,7 @@ pub struct IpPool { pub identity: IdentityMetadata, } -/// A collection of IP ranges. If a pool is linked to a silo, IP addresses from -/// the pool can be allocated within that silo +/// An IP pool in the context of a silo #[derive(ObjectIdentity, Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct SiloIpPool { #[serde(flatten)] diff --git a/openapi/nexus.json b/openapi/nexus.json index 21d9a98f51..3551c450ce 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -2191,7 +2191,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/IpPoolResultsPage" + "$ref": "#/components/schemas/SiloIpPoolResultsPage" } } } @@ -2232,7 +2232,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/IpPool" + "$ref": "#/components/schemas/SiloIpPool" } } } @@ -13871,7 +13871,7 @@ ] }, "SiloIpPool": { - "description": "A collection of IP ranges. If a pool is linked to a silo, IP addresses from the pool can be allocated within that silo", + "description": "An IP pool in the context of a silo", "type": "object", "properties": { "description": { From b489f0957967f8a8186ff3e1a1ed71e270eb97aa Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 19 Jan 2024 10:41:23 -0600 Subject: [PATCH 4/6] test for ip pools for silo endpoint --- nexus/db-queries/src/db/datastore/ip_pool.rs | 19 ++++--- nexus/src/app/ip_pool.rs | 2 +- nexus/src/external_api/http_entrypoints.rs | 10 ++-- nexus/tests/integration_tests/ip_pools.rs | 60 +++++++++++++++++--- 4 files changed, 72 insertions(+), 19 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index a6d7b6bc20..93eb358b99 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -403,17 +403,22 @@ impl DataStore { &self, opctx: &OpContext, authz_silo: &authz::Silo, - pagparams: &DataPageParams<'_, Uuid>, + pagparams: &PaginatedBy<'_>, ) -> ListResultVec<(IpPool, IpPoolResource)> { use db::schema::ip_pool; use db::schema::ip_pool_resource; - paginated( - ip_pool_resource::table, - ip_pool_resource::ip_pool_id, - pagparams, - ) - .inner_join(ip_pool::table) + match pagparams { + PaginatedBy::Id(pagparams) => { + paginated(ip_pool::table, ip_pool::id, pagparams) + } + PaginatedBy::Name(pagparams) => paginated( + ip_pool::table, + ip_pool::name, + &pagparams.map_name(|n| Name::ref_cast(n)), + ), + } + .inner_join(ip_pool_resource::table) .filter(ip_pool_resource::resource_id.eq(authz_silo.id())) .filter(ip_pool_resource::resource_type.eq(IpPoolResourceType::Silo)) .filter(ip_pool::time_deleted.is_null()) diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs index 1aceb3c4a3..902b3c97e8 100644 --- a/nexus/src/app/ip_pool.rs +++ b/nexus/src/app/ip_pool.rs @@ -117,7 +117,7 @@ impl super::Nexus { &self, opctx: &OpContext, silo_lookup: &lookup::Silo<'_>, - pagparams: &DataPageParams<'_, Uuid>, + pagparams: &PaginatedBy<'_>, ) -> ListResultVec<(db::model::IpPool, db::model::IpPoolResource)> { let (.., authz_silo) = silo_lookup.lookup_for(authz::Action::Read).await?; diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index d1fb5759d5..81117d7429 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -773,7 +773,7 @@ async fn silo_view( async fn silo_ip_pool_list( rqctx: RequestContext>, path_params: Path, - query_params: Query, + query_params: Query, ) -> Result>, HttpError> { let apictx = rqctx.context(); let handler = async { @@ -783,10 +783,12 @@ async fn silo_ip_pool_list( let query = query_params.into_inner(); let pag_params = data_page_params_for(&rqctx, &query)?; + let scan_params = ScanByNameOrId::from_query(&query)?; + let paginated_by = name_or_id_pagination(&pag_params, scan_params)?; let silo_lookup = nexus.silo_lookup(&opctx, path.silo)?; let pools = nexus - .silo_ip_pool_list(&opctx, &silo_lookup, &pag_params) + .silo_ip_pool_list(&opctx, &silo_lookup, &paginated_by) .await? .iter() .map(|(pool, silo_link)| views::SiloIpPool { @@ -795,10 +797,10 @@ async fn silo_ip_pool_list( }) .collect(); - Ok(HttpResponseOk(ScanById::results_page( + Ok(HttpResponseOk(ScanByNameOrId::results_page( &query, pools, - &|_, pool: &views::SiloIpPool| pool.identity.id, + &marker_for_name_or_id, )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index 43499eb5a9..9a261e9e8d 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -262,6 +262,19 @@ async fn test_ip_pool_list_dedupe(cptestctx: &ControlPlaneTestContext) { assert_eq!(ip_pools.len(), 2); assert_eq!(ip_pools[0].identity.id, pool1.id()); assert_eq!(ip_pools[1].identity.id, pool2.id()); + + let silo1_pools = pools_for_silo(client, "silo1").await; + assert_eq!(silo1_pools.len(), 2); + assert_eq!(silo1_pools[0].id(), pool1.id()); + assert_eq!(silo1_pools[1].id(), pool2.id()); + + let silo2_pools = pools_for_silo(client, "silo2").await; + assert_eq!(silo2_pools.len(), 1); + assert_eq!(silo2_pools[0].identity.name, "pool1"); + + let silo3_pools = pools_for_silo(client, "silo3").await; + assert_eq!(silo3_pools.len(), 1); + assert_eq!(silo3_pools[0].identity.name, "pool1"); } /// The internal IP pool, defined by its association with the internal silo, @@ -361,6 +374,10 @@ async fn test_ip_pool_silo_link(cptestctx: &ControlPlaneTestContext) { let assocs_p0 = silos_for_pool(client, "p0").await; assert_eq!(assocs_p0.items.len(), 0); + let silo_name = cptestctx.silo_name.as_str(); + let silo_pools = pools_for_silo(client, silo_name).await; + assert_eq!(silo_pools.len(), 0); + // expect 404 on association if the specified silo doesn't exist let nonexistent_silo_id = Uuid::new_v4(); let params = params::IpPoolSiloLink { @@ -375,11 +392,14 @@ async fn test_ip_pool_silo_link(cptestctx: &ControlPlaneTestContext) { StatusCode::NOT_FOUND, ) .await; + let not_found = + format!("not found: silo with id \"{nonexistent_silo_id}\""); + assert_eq!(error.message, not_found); - assert_eq!( - error.message, - format!("not found: silo with id \"{nonexistent_silo_id}\"") - ); + // pools for silo also 404s on nonexistent silo + let url = format!("/v1/system/silos/{}/ip-pools", nonexistent_silo_id); + let error = object_get_error(client, &url, StatusCode::NOT_FOUND).await; + assert_eq!(error.message, not_found); // associate by name with silo that exists let silo = NameOrId::Name(cptestctx.silo_name.clone()); @@ -408,6 +428,11 @@ async fn test_ip_pool_silo_link(cptestctx: &ControlPlaneTestContext) { assert_eq!(assocs_p0.items.len(), 1); assert_eq!(assocs_p0.items[0], silo_link); + let silo_pools = pools_for_silo(client, silo_name).await; + assert_eq!(silo_pools.len(), 1); + assert_eq!(silo_pools[0].identity.id, p0.identity.id); + assert_eq!(silo_pools[0].is_default, false); + // associate same silo to other pool by ID instead of name let link_params = params::IpPoolSiloLink { silo: NameOrId::Id(silo_id), @@ -423,6 +448,13 @@ async fn test_ip_pool_silo_link(cptestctx: &ControlPlaneTestContext) { IpPoolSilo { ip_pool_id: p1.identity.id, is_default: true, silo_id } ); + let silo_pools = pools_for_silo(client, silo_name).await; + assert_eq!(silo_pools.len(), 2); + assert_eq!(silo_pools[0].id(), p0.id()); + assert_eq!(silo_pools[0].is_default, false); + assert_eq!(silo_pools[1].id(), p1.id()); + assert_eq!(silo_pools[1].is_default, true); + // creating a third pool and trying to link it as default: true should fail create_pool(client, "p2").await; let url = "/v1/system/ip-pools/p2/silos"; @@ -447,13 +479,19 @@ async fn test_ip_pool_silo_link(cptestctx: &ControlPlaneTestContext) { "IP Pool cannot be deleted while it is linked to a silo", ); - // unlink silo (doesn't matter that it's a default) + // unlink p1 from silo (doesn't matter that it's a default) let url = format!("/v1/system/ip-pools/p1/silos/{}", cptestctx.silo_name); object_delete(client, &url).await; let silos_p1 = silos_for_pool(client, "p1").await; assert_eq!(silos_p1.items.len(), 0); + // after unlinking p1, only p0 is left + let silo_pools = pools_for_silo(client, silo_name).await; + assert_eq!(silo_pools.len(), 1); + assert_eq!(silo_pools[0].identity.id, p0.identity.id); + assert_eq!(silo_pools[0].is_default, false); + // now we can delete the pool too object_delete(client, "/v1/system/ip-pools/p1").await; } @@ -590,12 +628,20 @@ fn get_names(pools: Vec) -> Vec { async fn silos_for_pool( client: &ClientTestContext, - id: &str, + pool: &str, ) -> ResultsPage { - let url = format!("/v1/system/ip-pools/{}/silos", id); + let url = format!("/v1/system/ip-pools/{}/silos", pool); objects_list_page_authz::(client, &url).await } +async fn pools_for_silo( + client: &ClientTestContext, + silo: &str, +) -> Vec { + let url = format!("/v1/system/silos/{}/ip-pools", silo); + objects_list_page_authz::(client, &url).await.items +} + async fn create_pool(client: &ClientTestContext, name: &str) -> IpPool { let params = IpPoolCreate { identity: IdentityMetadataCreateParams { From 7685d47de437b076260c8a630ff1e1ec504a8ada Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 19 Jan 2024 11:13:44 -0600 Subject: [PATCH 5/6] views::IpPoolSilo -> views::IpPoolSiloLink, params::IpPoolSiloLink -> params::IpPoolLinkSilo --- end-to-end-tests/src/bin/bootstrap.rs | 4 +- nexus/db-model/src/ip_pool.rs | 2 +- nexus/src/app/ip_pool.rs | 2 +- nexus/src/external_api/http_entrypoints.rs | 10 ++--- nexus/test-utils/src/resource_helpers.rs | 4 +- nexus/tests/integration_tests/endpoints.rs | 4 +- nexus/tests/integration_tests/instances.rs | 6 +-- nexus/tests/integration_tests/ip_pools.rs | 51 ++++++++++++---------- nexus/types/src/external_api/params.rs | 2 +- nexus/types/src/external_api/views.rs | 5 +-- openapi/nexus.json | 48 ++++++++++---------- 11 files changed, 71 insertions(+), 67 deletions(-) diff --git a/end-to-end-tests/src/bin/bootstrap.rs b/end-to-end-tests/src/bin/bootstrap.rs index 21e59647ae..b02bed4265 100644 --- a/end-to-end-tests/src/bin/bootstrap.rs +++ b/end-to-end-tests/src/bin/bootstrap.rs @@ -4,7 +4,7 @@ use end_to_end_tests::helpers::{generate_name, get_system_ip_pool}; use omicron_test_utils::dev::poll::{wait_for_condition, CondCheckError}; use oxide_client::types::{ ByteCount, DeviceAccessTokenRequest, DeviceAuthRequest, DeviceAuthVerify, - DiskCreate, DiskSource, IpPoolCreate, IpPoolSiloLink, IpRange, Ipv4Range, + DiskCreate, DiskSource, IpPoolCreate, IpPoolLinkSilo, IpRange, Ipv4Range, NameOrId, SiloQuotasUpdate, }; use oxide_client::{ @@ -51,7 +51,7 @@ async fn main() -> Result<()> { client .ip_pool_silo_link() .pool(pool_name) - .body(IpPoolSiloLink { + .body(IpPoolLinkSilo { silo: NameOrId::Name(params.silo_name().parse().unwrap()), is_default: true, }) diff --git a/nexus/db-model/src/ip_pool.rs b/nexus/db-model/src/ip_pool.rs index bec1113151..030d052c22 100644 --- a/nexus/db-model/src/ip_pool.rs +++ b/nexus/db-model/src/ip_pool.rs @@ -97,7 +97,7 @@ pub struct IpPoolResource { pub is_default: bool, } -impl From for views::IpPoolSilo { +impl From for views::IpPoolSiloLink { fn from(assoc: IpPoolResource) -> Self { Self { ip_pool_id: assoc.ip_pool_id, diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs index 902b3c97e8..b5648a3346 100644 --- a/nexus/src/app/ip_pool.rs +++ b/nexus/src/app/ip_pool.rs @@ -128,7 +128,7 @@ impl super::Nexus { &self, opctx: &OpContext, pool_lookup: &lookup::IpPool<'_>, - silo_link: ¶ms::IpPoolSiloLink, + silo_link: ¶ms::IpPoolLinkSilo, ) -> CreateResult { let (authz_pool,) = pool_lookup.lookup_for(authz::Action::Modify).await?; diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 81117d7429..65b03a9fdf 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1546,7 +1546,7 @@ async fn ip_pool_silo_list( // whatever the thing is. Still... all we'd have to do to make this usable // in both places would be to make it { ...IpPool, silo_id, silo_name, // is_default } -) -> Result>, HttpError> { +) -> Result>, HttpError> { let apictx = rqctx.context(); let handler = async { let opctx = crate::context::op_context_for_external_api(&rqctx).await?; @@ -1568,7 +1568,7 @@ async fn ip_pool_silo_list( Ok(HttpResponseOk(ScanById::results_page( &query, assocs, - &|_, x: &views::IpPoolSilo| x.silo_id, + &|_, x: &views::IpPoolSiloLink| x.silo_id, )?)) }; apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await @@ -1583,8 +1583,8 @@ async fn ip_pool_silo_list( async fn ip_pool_silo_link( rqctx: RequestContext>, path_params: Path, - resource_assoc: TypedBody, -) -> Result, HttpError> { + resource_assoc: TypedBody, +) -> Result, HttpError> { let apictx = rqctx.context(); let handler = async { let opctx = crate::context::op_context_for_external_api(&rqctx).await?; @@ -1638,7 +1638,7 @@ async fn ip_pool_silo_update( rqctx: RequestContext>, path_params: Path, update: TypedBody, -) -> Result, HttpError> { +) -> Result, HttpError> { let apictx = rqctx.context(); let handler = async { let opctx = crate::context::op_context_for_external_api(&rqctx).await?; diff --git a/nexus/test-utils/src/resource_helpers.rs b/nexus/test-utils/src/resource_helpers.rs index c2516a1509..4fe03f204c 100644 --- a/nexus/test-utils/src/resource_helpers.rs +++ b/nexus/test-utils/src/resource_helpers.rs @@ -246,9 +246,9 @@ pub async fn link_ip_pool( is_default: bool, ) { let link = - params::IpPoolSiloLink { silo: NameOrId::Id(*silo_id), is_default }; + params::IpPoolLinkSilo { silo: NameOrId::Id(*silo_id), is_default }; let url = format!("/v1/system/ip-pools/{pool_name}/silos"); - object_create::( + object_create::( client, &url, &link, ) .await; diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index e8d54b876e..8beffe43a5 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -629,8 +629,8 @@ pub static DEMO_IP_POOL_UPDATE: Lazy = }); pub static DEMO_IP_POOL_SILOS_URL: Lazy = Lazy::new(|| format!("{}/silos", *DEMO_IP_POOL_URL)); -pub static DEMO_IP_POOL_SILOS_BODY: Lazy = - Lazy::new(|| params::IpPoolSiloLink { +pub static DEMO_IP_POOL_SILOS_BODY: Lazy = + Lazy::new(|| params::IpPoolLinkSilo { silo: NameOrId::Id(DEFAULT_SILO.identity().id), is_default: true, // necessary for demo instance create to go through }); diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 044f87f7c1..2f4e913185 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -3657,7 +3657,7 @@ async fn test_instance_ephemeral_ip_from_correct_pool( ); // make pool2 default and create instance with default pool. check that it now it comes from pool2 - let _: views::IpPoolSilo = object_put( + let _: views::IpPoolSiloLink = object_put( client, &format!("/v1/system/ip-pools/pool2/silos/{}", DEFAULT_SILO.id()), ¶ms::IpPoolSiloUpdate { is_default: true }, @@ -3788,11 +3788,11 @@ async fn test_instance_ephemeral_ip_from_orphan_pool( // associate the pool with a different silo and we should get the same // error on instance create - let params = params::IpPoolSiloLink { + let params = params::IpPoolLinkSilo { silo: NameOrId::Name(cptestctx.silo_name.clone()), is_default: false, }; - let _: views::IpPoolSilo = + let _: views::IpPoolSiloLink = object_create(client, "/v1/system/ip-pools/orphan-pool/silos", ¶ms) .await; diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index 9a261e9e8d..7843e816fd 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -31,7 +31,7 @@ use nexus_test_utils::resource_helpers::objects_list_page_authz; use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::params; use nexus_types::external_api::params::IpPoolCreate; -use nexus_types::external_api::params::IpPoolSiloLink; +use nexus_types::external_api::params::IpPoolLinkSilo; use nexus_types::external_api::params::IpPoolSiloUpdate; use nexus_types::external_api::params::IpPoolUpdate; use nexus_types::external_api::shared::IpRange; @@ -40,7 +40,7 @@ use nexus_types::external_api::shared::Ipv6Range; use nexus_types::external_api::shared::SiloIdentityMode; use nexus_types::external_api::views::IpPool; use nexus_types::external_api::views::IpPoolRange; -use nexus_types::external_api::views::IpPoolSilo; +use nexus_types::external_api::views::IpPoolSiloLink; use nexus_types::external_api::views::Silo; use nexus_types::external_api::views::SiloIpPool; use nexus_types::identity::Resource; @@ -346,7 +346,7 @@ async fn test_ip_pool_service_no_cud(cptestctx: &ControlPlaneTestContext) { // linking not allowed - // let link_body = params::IpPoolSiloLink { + // let link_body = params::IpPoolLinkSilo { // silo: NameOrId::Name(cptestctx.silo_name.clone()), // is_default: false, // }; @@ -380,7 +380,7 @@ async fn test_ip_pool_silo_link(cptestctx: &ControlPlaneTestContext) { // expect 404 on association if the specified silo doesn't exist let nonexistent_silo_id = Uuid::new_v4(); - let params = params::IpPoolSiloLink { + let params = params::IpPoolLinkSilo { silo: NameOrId::Id(nonexistent_silo_id), is_default: false, }; @@ -404,8 +404,8 @@ async fn test_ip_pool_silo_link(cptestctx: &ControlPlaneTestContext) { // associate by name with silo that exists let silo = NameOrId::Name(cptestctx.silo_name.clone()); let params = - params::IpPoolSiloLink { silo: silo.clone(), is_default: false }; - let _: IpPoolSilo = + params::IpPoolLinkSilo { silo: silo.clone(), is_default: false }; + let _: IpPoolSiloLink = object_create(client, "/v1/system/ip-pools/p0/silos", ¶ms).await; // second attempt to create the same link errors due to conflict @@ -423,8 +423,11 @@ async fn test_ip_pool_silo_link(cptestctx: &ControlPlaneTestContext) { let silo_id = object_get::(client, &silo_url).await.identity.id; let assocs_p0 = silos_for_pool(client, "p0").await; - let silo_link = - IpPoolSilo { ip_pool_id: p0.identity.id, silo_id, is_default: false }; + let silo_link = IpPoolSiloLink { + ip_pool_id: p0.identity.id, + silo_id, + is_default: false, + }; assert_eq!(assocs_p0.items.len(), 1); assert_eq!(assocs_p0.items[0], silo_link); @@ -434,18 +437,22 @@ async fn test_ip_pool_silo_link(cptestctx: &ControlPlaneTestContext) { assert_eq!(silo_pools[0].is_default, false); // associate same silo to other pool by ID instead of name - let link_params = params::IpPoolSiloLink { + let link_params = params::IpPoolLinkSilo { silo: NameOrId::Id(silo_id), is_default: true, }; let url = "/v1/system/ip-pools/p1/silos"; - let _: IpPoolSilo = object_create(client, &url, &link_params).await; + let _: IpPoolSiloLink = object_create(client, &url, &link_params).await; let silos_p1 = silos_for_pool(client, "p1").await; assert_eq!(silos_p1.items.len(), 1); assert_eq!( silos_p1.items[0], - IpPoolSilo { ip_pool_id: p1.identity.id, is_default: true, silo_id } + IpPoolSiloLink { + ip_pool_id: p1.identity.id, + is_default: true, + silo_id + } ); let silo_pools = pools_for_silo(client, silo_name).await; @@ -525,10 +532,10 @@ async fn test_ip_pool_update_default(cptestctx: &ControlPlaneTestContext) { // associate both pools with the test silo let silo = NameOrId::Name(cptestctx.silo_name.clone()); let params = - params::IpPoolSiloLink { silo: silo.clone(), is_default: false }; - let _: IpPoolSilo = + params::IpPoolLinkSilo { silo: silo.clone(), is_default: false }; + let _: IpPoolSiloLink = object_create(client, "/v1/system/ip-pools/p0/silos", ¶ms).await; - let _: IpPoolSilo = + let _: IpPoolSiloLink = object_create(client, "/v1/system/ip-pools/p1/silos", ¶ms).await; // now both are linked to the silo, neither is marked default @@ -542,10 +549,10 @@ async fn test_ip_pool_update_default(cptestctx: &ControlPlaneTestContext) { // make p0 default let params = IpPoolSiloUpdate { is_default: true }; - let _: IpPoolSilo = object_put(client, &p0_silo_url, ¶ms).await; + let _: IpPoolSiloLink = object_put(client, &p0_silo_url, ¶ms).await; // making the same one default again is not an error - let _: IpPoolSilo = object_put(client, &p0_silo_url, ¶ms).await; + let _: IpPoolSiloLink = object_put(client, &p0_silo_url, ¶ms).await; // now p0 is default let silos_p0 = silos_for_pool(client, "p0").await; @@ -563,7 +570,7 @@ async fn test_ip_pool_update_default(cptestctx: &ControlPlaneTestContext) { let params = IpPoolSiloUpdate { is_default: true }; let p1_silo_url = format!("/v1/system/ip-pools/p1/silos/{}", cptestctx.silo_name); - let _: IpPoolSilo = object_put(client, &p1_silo_url, ¶ms).await; + let _: IpPoolSiloLink = object_put(client, &p1_silo_url, ¶ms).await; // p1 is now default let silos_p1 = silos_for_pool(client, "p1").await; @@ -577,7 +584,7 @@ async fn test_ip_pool_update_default(cptestctx: &ControlPlaneTestContext) { // we can also unset default let params = IpPoolSiloUpdate { is_default: false }; - let _: IpPoolSilo = object_put(client, &p1_silo_url, ¶ms).await; + let _: IpPoolSiloLink = object_put(client, &p1_silo_url, ¶ms).await; let silos_p1 = silos_for_pool(client, "p1").await; assert_eq!(silos_p1.items.len(), 1); @@ -629,9 +636,9 @@ fn get_names(pools: Vec) -> Vec { async fn silos_for_pool( client: &ClientTestContext, pool: &str, -) -> ResultsPage { +) -> ResultsPage { let url = format!("/v1/system/ip-pools/{}/silos", pool); - objects_list_page_authz::(client, &url).await + objects_list_page_authz::(client, &url).await } async fn pools_for_silo( @@ -1028,13 +1035,13 @@ async fn test_ip_range_delete_with_allocated_external_ip_fails( .await; // associate pool with default silo, which is the privileged user's silo - let params = IpPoolSiloLink { + let params = IpPoolLinkSilo { silo: NameOrId::Id(DEFAULT_SILO.id()), is_default: true, }; NexusRequest::objects_post(client, &ip_pool_silos_url, ¶ms) .authn_as(AuthnMode::PrivilegedUser) - .execute_and_parse_unwrap::() + .execute_and_parse_unwrap::() .await; // Add an IP range to the pool diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index a33bc0b8bb..750e83c2a2 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -855,7 +855,7 @@ pub struct IpPoolSiloPath { } #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct IpPoolSiloLink { +pub struct IpPoolLinkSilo { pub silo: NameOrId, /// When a pool is the default for a silo, floating IPs and instance /// ephemeral IPs will come from that pool when no other pool is specified. diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index b39a7031b6..314dd4ed00 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -315,13 +315,10 @@ pub struct SiloIpPool { pub is_default: bool, } -// TODO: rename IpPoolSilo or get rid of it somehow. we cannot have both -// IpPoolSilo and SiloIpPool. come on - /// A link between an IP pool and a silo that allows one to allocate IPs from /// the pool within the silo #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] -pub struct IpPoolSilo { +pub struct IpPoolSiloLink { pub ip_pool_id: Uuid, pub silo_id: Uuid, /// When a pool is the default for a silo, floating IPs and instance diff --git a/openapi/nexus.json b/openapi/nexus.json index 3551c450ce..2dd4037430 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -5039,7 +5039,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/IpPoolSiloResultsPage" + "$ref": "#/components/schemas/IpPoolSiloLinkResultsPage" } } } @@ -5076,7 +5076,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/IpPoolSiloLink" + "$ref": "#/components/schemas/IpPoolLinkSilo" } } }, @@ -5088,7 +5088,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/IpPoolSilo" + "$ref": "#/components/schemas/IpPoolSiloLink" } } } @@ -5144,7 +5144,7 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/IpPoolSilo" + "$ref": "#/components/schemas/IpPoolSiloLink" } } } @@ -6684,7 +6684,7 @@ "in": "query", "name": "sort_by", "schema": { - "$ref": "#/components/schemas/IdSortMode" + "$ref": "#/components/schemas/NameOrIdSortMode" } } ], @@ -12565,6 +12565,22 @@ "name" ] }, + "IpPoolLinkSilo": { + "type": "object", + "properties": { + "is_default": { + "description": "When a pool is the default for a silo, floating IPs and instance ephemeral IPs will come from that pool when no other pool is specified. There can be at most one default for a given silo.", + "type": "boolean" + }, + "silo": { + "$ref": "#/components/schemas/NameOrId" + } + }, + "required": [ + "is_default", + "silo" + ] + }, "IpPoolRange": { "type": "object", "properties": { @@ -12633,7 +12649,7 @@ "items" ] }, - "IpPoolSilo": { + "IpPoolSiloLink": { "description": "A link between an IP pool and a silo that allows one to allocate IPs from the pool within the silo", "type": "object", "properties": { @@ -12656,23 +12672,7 @@ "silo_id" ] }, - "IpPoolSiloLink": { - "type": "object", - "properties": { - "is_default": { - "description": "When a pool is the default for a silo, floating IPs and instance ephemeral IPs will come from that pool when no other pool is specified. There can be at most one default for a given silo.", - "type": "boolean" - }, - "silo": { - "$ref": "#/components/schemas/NameOrId" - } - }, - "required": [ - "is_default", - "silo" - ] - }, - "IpPoolSiloResultsPage": { + "IpPoolSiloLinkResultsPage": { "description": "A single page of results", "type": "object", "properties": { @@ -12680,7 +12680,7 @@ "description": "list of items on this page of results", "type": "array", "items": { - "$ref": "#/components/schemas/IpPoolSilo" + "$ref": "#/components/schemas/IpPoolSiloLink" } }, "next_page": { From 9e8500d11143c7c0601e5b08509958a07f032470 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 19 Jan 2024 14:39:44 -0600 Subject: [PATCH 6/6] share query to fetch pools for silo --- nexus/db-queries/src/db/datastore/ip_pool.rs | 53 +++----------------- nexus/src/app/ip_pool.rs | 19 ++++++- 2 files changed, 26 insertions(+), 46 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index 93eb358b99..331126ef97 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -79,44 +79,6 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } - /// List IP pools linked to the current silo - pub async fn current_silo_ip_pool_list( - &self, - opctx: &OpContext, - pagparams: &PaginatedBy<'_>, - ) -> ListResultVec<(IpPool, IpPoolResource)> { - use db::schema::ip_pool; - use db::schema::ip_pool_resource; - - // From the developer user's point of view, we treat IP pools linked to - // their silo as silo resources, so they can list them if they can list - // silo children - let authz_silo = - opctx.authn.silo_required().internal_context("listing IP pools")?; - opctx.authorize(authz::Action::ListChildren, &authz_silo).await?; - - let silo_id = authz_silo.id(); - - match pagparams { - PaginatedBy::Id(pagparams) => { - paginated(ip_pool::table, ip_pool::id, pagparams) - } - PaginatedBy::Name(pagparams) => paginated( - ip_pool::table, - ip_pool::name, - &pagparams.map_name(|n| Name::ref_cast(n)), - ), - } - .inner_join(ip_pool_resource::table) - .filter(ip_pool_resource::resource_type.eq(IpPoolResourceType::Silo)) - .filter(ip_pool_resource::resource_id.eq(silo_id)) - .filter(ip_pool::time_deleted.is_null()) - .select(<(IpPool, IpPoolResource)>::as_select()) - .get_results_async(&*self.pool_connection_authorized(opctx).await?) - .await - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) - } - /// Look up whether the given pool is available to users in the current /// silo, i.e., whether there is an entry in the association table linking /// the pool with that silo @@ -423,9 +385,7 @@ impl DataStore { .filter(ip_pool_resource::resource_type.eq(IpPoolResourceType::Silo)) .filter(ip_pool::time_deleted.is_null()) .select(<(IpPool, IpPoolResource)>::as_select()) - .load_async::<(IpPool, IpPoolResource)>( - &*self.pool_connection_authorized(opctx).await?, - ) + .load_async(&*self.pool_connection_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } @@ -897,8 +857,11 @@ mod test { .await .expect("Should list IP pools"); assert_eq!(all_pools.len(), 0); + + let authz_silo = opctx.authn.silo_required().unwrap(); + let silo_pools = datastore - .current_silo_ip_pool_list(&opctx, &pagbyid) + .silo_ip_pool_list(&opctx, &authz_silo, &pagbyid) .await .expect("Should list silo IP pools"); assert_eq!(silo_pools.len(), 0); @@ -923,7 +886,7 @@ mod test { .expect("Should list IP pools"); assert_eq!(all_pools.len(), 1); let silo_pools = datastore - .current_silo_ip_pool_list(&opctx, &pagbyid) + .silo_ip_pool_list(&opctx, &authz_silo, &pagbyid) .await .expect("Should list silo IP pools"); assert_eq!(silo_pools.len(), 0); @@ -959,7 +922,7 @@ mod test { // now it shows up in the silo list let silo_pools = datastore - .current_silo_ip_pool_list(&opctx, &pagbyid) + .silo_ip_pool_list(&opctx, &authz_silo, &pagbyid) .await .expect("Should list silo IP pools"); assert_eq!(silo_pools.len(), 1); @@ -1029,7 +992,7 @@ mod test { // and silo pools list is empty again let silo_pools = datastore - .current_silo_ip_pool_list(&opctx, &pagbyid) + .silo_ip_pool_list(&opctx, &authz_silo, &pagbyid) .await .expect("Should list silo IP pools"); assert_eq!(silo_pools.len(), 0); diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs index b5648a3346..d8d36fff4b 100644 --- a/nexus/src/app/ip_pool.rs +++ b/nexus/src/app/ip_pool.rs @@ -20,6 +20,7 @@ use omicron_common::api::external::CreateResult; use omicron_common::api::external::DataPageParams; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; +use omicron_common::api::external::InternalContext; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; use omicron_common::api::external::NameOrId; @@ -79,7 +80,15 @@ impl super::Nexus { opctx: &OpContext, pagparams: &PaginatedBy<'_>, ) -> ListResultVec<(db::model::IpPool, db::model::IpPoolResource)> { - self.db_datastore.current_silo_ip_pool_list(opctx, pagparams).await + let authz_silo = + opctx.authn.silo_required().internal_context("listing IP pools")?; + + // From the developer user's point of view, we treat IP pools linked to + // their silo as silo resources, so they can list them if they can list + // silo children + opctx.authorize(authz::Action::ListChildren, &authz_silo).await?; + + self.db_datastore.silo_ip_pool_list(opctx, &authz_silo, pagparams).await } // Look up pool by name or ID, but only return it if it's linked to the @@ -109,6 +118,10 @@ impl super::Nexus { ) -> ListResultVec { let (.., authz_pool) = pool_lookup.lookup_for(authz::Action::ListChildren).await?; + + // check ability to list silos in general + opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?; + self.db_datastore.ip_pool_silo_list(opctx, &authz_pool, pagparams).await } @@ -121,6 +134,10 @@ impl super::Nexus { ) -> ListResultVec<(db::model::IpPool, db::model::IpPoolResource)> { let (.., authz_silo) = silo_lookup.lookup_for(authz::Action::Read).await?; + // check ability to list pools in general + opctx + .authorize(authz::Action::ListChildren, &authz::IP_POOL_LIST) + .await?; self.db_datastore.silo_ip_pool_list(opctx, &authz_silo, pagparams).await }