From bec0581774b00a4874349f2bcf1665a706d0e76a Mon Sep 17 00:00:00 2001 From: Justin Bennett Date: Tue, 14 Nov 2023 12:26:54 -0500 Subject: [PATCH] remove concept of fleet association --- nexus/db-model/src/ip_pool.rs | 2 - nexus/db-queries/src/db/datastore/ip_pool.rs | 89 +++++++------------- nexus/db-queries/src/db/datastore/project.rs | 6 +- nexus/db-queries/src/db/datastore/rack.rs | 33 -------- nexus/src/app/ip_pool.rs | 32 ++----- nexus/src/external_api/http_entrypoints.rs | 25 ++---- nexus/types/src/external_api/params.rs | 41 +-------- nexus/types/src/external_api/shared.rs | 1 - schema/crdb/19.0.0/up1.sql | 3 +- schema/crdb/dbinit.sql | 3 +- 10 files changed, 49 insertions(+), 186 deletions(-) diff --git a/nexus/db-model/src/ip_pool.rs b/nexus/db-model/src/ip_pool.rs index a8bee67bdf..e587566713 100644 --- a/nexus/db-model/src/ip_pool.rs +++ b/nexus/db-model/src/ip_pool.rs @@ -85,7 +85,6 @@ impl_enum_type!( #[diesel(sql_type = IpPoolResourceTypeEnum)] pub enum IpPoolResourceType; - Fleet => b"fleet" Silo => b"silo" ); @@ -101,7 +100,6 @@ pub struct IpPoolResource { impl From for shared::IpPoolResourceType { fn from(typ: IpPoolResourceType) -> Self { match typ { - IpPoolResourceType::Fleet => Self::Fleet, IpPoolResourceType::Silo => Self::Silo, } } diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index 9c557c9e9b..151280d7b3 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -95,11 +95,9 @@ impl DataStore { ip_pool::table .inner_join(ip_pool_resource::table) .filter( - (ip_pool_resource::resource_type + ip_pool_resource::resource_type .eq(IpPoolResourceType::Silo) - .and(ip_pool_resource::resource_id.eq(authz_silo.id()))) - .or(ip_pool_resource::resource_type - .eq(IpPoolResourceType::Fleet)), + .and(ip_pool_resource::resource_id.eq(authz_silo.id())), ) .filter(ip_pool::id.eq(authz_pool.id())) .filter(ip_pool::time_deleted.is_null()) @@ -139,11 +137,9 @@ impl DataStore { ip_pool::table .inner_join(ip_pool_resource::table) .filter( - (ip_pool_resource::resource_type + ip_pool_resource::resource_type .eq(IpPoolResourceType::Silo) - .and(ip_pool_resource::resource_id.eq(authz_silo_id))) - .or(ip_pool_resource::resource_type - .eq(IpPoolResourceType::Fleet)), + .and(ip_pool_resource::resource_id.eq(authz_silo_id)), ) .filter(ip_pool_resource::is_default.eq(true)) .filter(ip_pool::time_deleted.is_null()) @@ -429,57 +425,35 @@ impl DataStore { // We can only delete the association if there are no IPs allocated // from this pool in the associated resource. - // most of the query is the same between silo and fleet - let base_query = |table: external_ip::table| { - table - .inner_join( - instance::table - .on(external_ip::parent_id.eq(instance::id.nullable())), - ) - .filter(external_ip::is_service.eq(false)) - .filter(external_ip::parent_id.is_not_null()) - .filter(external_ip::time_deleted.is_null()) - .filter(external_ip::ip_pool_id.eq(association.ip_pool_id)) - .filter(instance::time_deleted.is_not_null()) - .select(ExternalIp::as_select()) - .limit(1) - }; - - let existing_ips = match association.resource_type { - IpPoolResourceType::Silo => { - - // if it's a silo association, we also have to join through IPs to instances - // to projects to get the silo ID - base_query(external_ip::table) - .inner_join( - project::table.on(instance::project_id.eq(project::id)), - ) - .filter(project::silo_id.eq(association.resource_id)) - .load_async::( - &*self.pool_connection_authorized(opctx).await?, - ) - .await - }, - IpPoolResourceType::Fleet => { - // If it's a fleet association, we can't delete it if there are any IPs - // allocated from the pool anywhere - base_query(external_ip::table) - .load_async::( - &*self.pool_connection_authorized(opctx).await?, - ) - .await - } - } - .map_err(|e| { - Error::internal_error(&format!( - "error checking for outstanding IPs before deleting IP pool association to resource: {:?}", - e - )) - })?; + let existing_ips = external_ip::table + .inner_join( + instance::table + .on(external_ip::parent_id.eq(instance::id.nullable())), + ) + .filter(external_ip::is_service.eq(false)) + .filter(external_ip::parent_id.is_not_null()) + .filter(external_ip::time_deleted.is_null()) + .filter(external_ip::ip_pool_id.eq(association.ip_pool_id)) + .filter(instance::time_deleted.is_not_null()) + .select(ExternalIp::as_select()) + .limit(1) + // we have to join through IPs to instances to projects to get the silo ID + .inner_join(project::table.on(instance::project_id.eq(project::id))) + .filter(project::silo_id.eq(association.resource_id)) + .load_async::( + &*self.pool_connection_authorized(opctx).await?, + ) + .await + .map_err(|e| { + Error::internal_error(&format!( + "error checking for outstanding IPs before deleting IP pool association to resource: {:?}", + e + )) + })?; if !existing_ips.is_empty() { return Err(Error::InvalidRequest { - message: "IP addresses from this pool are in use in the associated silo/fleet".to_string() + message: "IP addresses from this pool are in use in the associated silo".to_string() }); } @@ -487,8 +461,7 @@ impl DataStore { } /// Delete IP pool assocation with resource unless there are outstanding - /// IPs allocated from the pool in the associated silo (or the fleet, if - /// it's a fleet association). + /// IPs allocated from the pool in the associated silo pub async fn ip_pool_dissociate_resource( &self, opctx: &OpContext, diff --git a/nexus/db-queries/src/db/datastore/project.rs b/nexus/db-queries/src/db/datastore/project.rs index 762fecfa43..783a3a952c 100644 --- a/nexus/db-queries/src/db/datastore/project.rs +++ b/nexus/db-queries/src/db/datastore/project.rs @@ -353,13 +353,11 @@ impl DataStore { &pagparams.map_name(|n| Name::ref_cast(n)), ), } - // TODO: make sure this join is compatible with pagination logic .inner_join(ip_pool_resource::table) .filter( - (ip_pool_resource::resource_type + ip_pool_resource::resource_type .eq(IpPoolResourceType::Silo) - .and(ip_pool_resource::resource_id.eq(silo_id))) - .or(ip_pool_resource::resource_type.eq(IpPoolResourceType::Fleet)), + .and(ip_pool_resource::resource_id.eq(silo_id)), ) .filter(ip_pool::time_deleted.is_null()) .select(db::model::IpPool::as_select()) diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index 27211c4d82..1f4a53ab12 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -19,7 +19,6 @@ use crate::db::fixed_data::silo::INTERNAL_SILO_ID; use crate::db::fixed_data::vpc_subnet::DNS_VPC_SUBNET; use crate::db::fixed_data::vpc_subnet::NEXUS_VPC_SUBNET; use crate::db::fixed_data::vpc_subnet::NTP_VPC_SUBNET; -use crate::db::fixed_data::FLEET_ID; use crate::db::identity::Asset; use crate::db::model::Dataset; use crate::db::model::IncompleteExternalIp; @@ -736,38 +735,6 @@ impl DataStore { .await?; } - let default_pool = - db::model::IpPool::new(&IdentityMetadataCreateParams { - name: "default".parse::().unwrap(), - description: String::from("default IP pool"), - }); - - let default_pool_id = default_pool.id(); - - let default_created = self - .ip_pool_create(opctx, default_pool) - .await - .map(|_| true) - .or_else(|e| match e { - Error::ObjectAlreadyExists { .. } => Ok(false), - _ => Err(e), - })?; - - // make pool default for fleet. only need to do this if the create went - // through, i.e., if it wasn't already there - if default_created { - self.ip_pool_associate_resource( - opctx, - db::model::IpPoolResource { - ip_pool_id: default_pool_id, - resource_type: db::model::IpPoolResourceType::Fleet, - resource_id: *FLEET_ID, - is_default: true, - }, - ) - .await?; - } - Ok(()) } diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs index 95c93e4b1d..8324018b37 100644 --- a/nexus/src/app/ip_pool.rs +++ b/nexus/src/app/ip_pool.rs @@ -12,11 +12,9 @@ use nexus_db_model::IpPoolResourceType; use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; use nexus_db_queries::db; -use nexus_db_queries::db::fixed_data::FLEET_ID; use nexus_db_queries::db::lookup; use nexus_db_queries::db::lookup::LookupPath; use nexus_db_queries::db::model::Name; -use nexus_types::identity::Resource; use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DataPageParams; @@ -95,7 +93,6 @@ impl super::Nexus { pool_lookup: &lookup::IpPool<'_>, assoc_create: ¶ms::IpPoolAssociationCreate, ) -> CreateResult { - // TODO: check for perms on specified resource? or unnecessary because this is an operator action? let (.., authz_pool) = pool_lookup.lookup_for(authz::Action::Modify).await?; let (resource_type, resource_id, is_default) = match assoc_create { @@ -110,14 +107,6 @@ impl super::Nexus { assoc_silo.is_default, ) } - params::IpPoolAssociationCreate::Fleet(assoc_fleet) => { - // we don't need to be assured of the fleet's existence - ( - db::model::IpPoolResourceType::Fleet, - *FLEET_ID, - assoc_fleet.is_default, - ) - } }; self.db_datastore .ip_pool_associate_resource( @@ -136,31 +125,20 @@ impl super::Nexus { &self, opctx: &OpContext, pool_lookup: &lookup::IpPool<'_>, - ip_pool_dissoc: ¶ms::IpPoolAssociationDeleteValidated, + silo_lookup: &lookup::Silo<'_>, ) -> DeleteResult { let (.., authz_pool) = pool_lookup.lookup_for(authz::Action::Modify).await?; - - let (resource_type, resource_id) = match ip_pool_dissoc { - params::IpPoolAssociationDeleteValidated::Silo(assoc) => { - let (.., silo) = self - .silo_lookup(opctx, assoc.silo.clone())? - .fetch() - .await?; - (IpPoolResourceType::Silo, silo.id()) - } - params::IpPoolAssociationDeleteValidated::Fleet => { - (IpPoolResourceType::Fleet, *FLEET_ID) - } - }; + let (.., authz_silo) = + silo_lookup.lookup_for(authz::Action::Modify).await?; self.db_datastore .ip_pool_dissociate_resource( opctx, &IpPoolResourceDelete { ip_pool_id: authz_pool.id(), - resource_id, - resource_type, + resource_id: authz_silo.id(), + resource_type: IpPoolResourceType::Silo, }, ) .await diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 11bdfcaf8b..3ba6cf8d8a 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1323,14 +1323,10 @@ async fn ip_pool_update( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -// TODO: associate just seems like the wrong word and I'd like to change it -// across the board. What I really mean is "make available to" or "make availale -// for use in" - -/// List IP pool resource associations +/// List an IP pool's associated silo configuration #[endpoint { method = GET, - path = "/v1/system/ip-pools/{pool}/associations", + path = "/v1/system/ip-pools/{pool}/silos", tags = ["system/networking"], }] async fn ip_pool_association_list( @@ -1368,10 +1364,10 @@ async fn ip_pool_association_list( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } -/// Associate an IP Pool with a silo or the fleet +/// Associate an IP Pool with a silo #[endpoint { method = POST, - path = "/v1/system/ip-pools/{pool}/associations", + path = "/v1/system/ip-pools/{pool}/silos", tags = ["system/networking"], }] async fn ip_pool_association_create( @@ -1397,7 +1393,7 @@ async fn ip_pool_association_create( /// Remove an IP pool's association with a silo or project #[endpoint { method = DELETE, - path = "/v1/system/ip-pools/{pool}/associations", + path = "/v1/system/ip-pools/{pool}/silos", tags = ["system/networking"], }] async fn ip_pool_association_delete( @@ -1412,17 +1408,10 @@ async fn ip_pool_association_delete( let path = path_params.into_inner(); let query = query_params.into_inner(); - let validated_params = - params::IpPoolAssociationDeleteValidated::try_from(query) - .map_err(|e| HttpError::for_bad_request(None, e))?; - let pool_lookup = nexus.ip_pool_lookup(&opctx, &path.pool)?; + let silo_lookup = nexus.silo_lookup(&opctx, query.silo)?; nexus - .ip_pool_dissociate_resource( - &opctx, - &pool_lookup, - &validated_params, - ) + .ip_pool_dissociate_resource(&opctx, &pool_lookup, &silo_lookup) .await?; Ok(HttpResponseUpdatedNoContent()) }; diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index c04a8cd111..d85ef52386 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -763,27 +763,16 @@ pub struct IpPoolAssociateSilo { pub is_default: bool, } -#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct IpPoolAssociateFleet { - pub is_default: bool, -} - -/// Parameters for associating an IP pool with a resource (fleet, silo) +/// Parameters for associating an IP pool with a silo #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] #[serde(tag = "resource_type", rename_all = "snake_case")] pub enum IpPoolAssociationCreate { Silo(IpPoolAssociateSilo), - Fleet(IpPoolAssociateFleet), } -// It would be cool if this was an enum like Silo(NameOrId) | Fleet, but -// OpenAPI doesn't support fancy schemas on query params. So instead we -// use this flat struct for the query params directly, and manually parse -// that into the good struct below. #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] pub struct IpPoolAssociationDelete { - pub silo: Option, - pub resource_type: shared::IpPoolResourceType, + pub silo: NameOrId, } // technically these are not params, but they are used with params @@ -792,32 +781,6 @@ pub struct IpPoolSiloAssociationDelete { pub silo: NameOrId, } -#[derive(Clone, Debug)] -pub enum IpPoolAssociationDeleteValidated { - Silo(IpPoolSiloAssociationDelete), - Fleet, -} - -impl TryFrom for IpPoolAssociationDeleteValidated { - type Error = String; - - fn try_from(value: IpPoolAssociationDelete) -> Result { - match (value.silo, value.resource_type) { - (Some(silo), shared::IpPoolResourceType::Silo) => { - Ok(Self::Silo(IpPoolSiloAssociationDelete { silo })) - } - (None, shared::IpPoolResourceType::Fleet) => Ok(Self::Fleet), - (Some(_), shared::IpPoolResourceType::Fleet) => { - Err("Silo must be null if resource_type is fleet".to_string()) - } - (None, shared::IpPoolResourceType::Silo) => { - Err("Silo must be specified if resource_type is silo" - .to_string()) - } - } - } -} - // INSTANCES /// Describes an attachment of an `InstanceNetworkInterface` to an `Instance`, diff --git a/nexus/types/src/external_api/shared.rs b/nexus/types/src/external_api/shared.rs index c7a8cda4ab..dcd7709f61 100644 --- a/nexus/types/src/external_api/shared.rs +++ b/nexus/types/src/external_api/shared.rs @@ -248,7 +248,6 @@ pub enum UpdateableComponentType { #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema, PartialEq)] #[serde(rename_all = "snake_case")] pub enum IpPoolResourceType { - Fleet, Silo, } diff --git a/schema/crdb/19.0.0/up1.sql b/schema/crdb/19.0.0/up1.sql index 5ccd8f51b9..28204a4d3b 100644 --- a/schema/crdb/19.0.0/up1.sql +++ b/schema/crdb/19.0.0/up1.sql @@ -1,4 +1,3 @@ CREATE TYPE IF NOT EXISTS omicron.public.ip_pool_resource_type AS ENUM ( - 'silo', - 'fleet' + 'silo' ); diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 79e43aaf93..45abda3665 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -1543,8 +1543,7 @@ CREATE UNIQUE INDEX IF NOT EXISTS lookup_pool_by_name ON omicron.public.ip_pool -- silo default and a fleet default. If we were to add a project type, it should -- be added before silo. CREATE TYPE IF NOT EXISTS omicron.public.ip_pool_resource_type AS ENUM ( - 'silo', - 'fleet' + 'silo' ); -- join table associating IP pools with resources like fleet or silo