diff --git a/nexus/db-model/src/schema.rs b/nexus/db-model/src/schema.rs index 68991f1d75..2e7493716e 100644 --- a/nexus/db-model/src/schema.rs +++ b/nexus/db-model/src/schema.rs @@ -13,7 +13,7 @@ use omicron_common::api::external::SemverVersion; /// /// This should be updated whenever the schema is changed. For more details, /// refer to: schema/crdb/README.adoc -pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(24, 0, 0); +pub const SCHEMA_VERSION: SemverVersion = SemverVersion::new(25, 0, 0); table! { disk (id) { diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index 331126ef97..6d3a95af7d 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -224,15 +224,15 @@ impl DataStore { use db::schema::ip_pool_resource; opctx.authorize(authz::Action::Delete, authz_pool).await?; + let conn = self.pool_connection_authorized(opctx).await?; + // Verify there are no IP ranges still in this pool let range = ip_pool_range::dsl::ip_pool_range .filter(ip_pool_range::dsl::ip_pool_id.eq(authz_pool.id())) .filter(ip_pool_range::dsl::time_deleted.is_null()) .select(ip_pool_range::dsl::id) .limit(1) - .first_async::( - &*self.pool_connection_authorized(opctx).await?, - ) + .first_async::(&*conn) .await .optional() .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; @@ -242,23 +242,6 @@ impl DataStore { )); } - // Verify there are no linked silos - let silo_link = ip_pool_resource::table - .filter(ip_pool_resource::ip_pool_id.eq(authz_pool.id())) - .select(ip_pool_resource::resource_id) - .limit(1) - .first_async::( - &*self.pool_connection_authorized(opctx).await?, - ) - .await - .optional() - .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; - if silo_link.is_some() { - return Err(Error::invalid_request( - "IP Pool cannot be deleted while it is linked to a silo", - )); - } - // Delete the pool, conditional on the rcgen not having changed. This // protects the delete from occuring if clients created a new IP range // in between the above check for children and this query. @@ -268,7 +251,7 @@ impl DataStore { .filter(dsl::id.eq(authz_pool.id())) .filter(dsl::rcgen.eq(db_pool.rcgen)) .set(dsl::time_deleted.eq(now)) - .execute_async(&*self.pool_connection_authorized(opctx).await?) + .execute_async(&*conn) .await .map_err(|e| { public_error_from_diesel( @@ -282,6 +265,18 @@ impl DataStore { "deletion failed due to concurrent modification", )); } + + // Rather than treating outstanding links as a blocker for pool delete, + // just delete them. If we've gotten this far, we know there are no + // ranges in the pool, which means it can't be in use. + + // delete any links from this pool to any other resources (silos) + diesel::delete(ip_pool_resource::table) + .filter(ip_pool_resource::ip_pool_id.eq(authz_pool.id())) + .execute_async(&*conn) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + Ok(()) } diff --git a/nexus/db-queries/src/db/datastore/silo.rs b/nexus/db-queries/src/db/datastore/silo.rs index 2c0c5f3c47..a88a27872f 100644 --- a/nexus/db-queries/src/db/datastore/silo.rs +++ b/nexus/db-queries/src/db/datastore/silo.rs @@ -17,6 +17,7 @@ use crate::db::error::TransactionError; use crate::db::fixed_data::silo::{DEFAULT_SILO, INTERNAL_SILO}; use crate::db::identity::Resource; use crate::db::model::CollectionTypeProvisioned; +use crate::db::model::IpPoolResourceType; use crate::db::model::Name; use crate::db::model::Silo; use crate::db::model::VirtualProvisioningCollection; @@ -547,6 +548,23 @@ impl DataStore { debug!(opctx.log, "deleted {} silo IdPs for silo {}", updated_rows, id); + // delete IP pool links (not IP pools, just the links) + use db::schema::ip_pool_resource; + + let updated_rows = diesel::delete(ip_pool_resource::table) + .filter(ip_pool_resource::resource_id.eq(id)) + .filter( + ip_pool_resource::resource_type.eq(IpPoolResourceType::Silo), + ) + .execute_async(&*conn) + .await + .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server))?; + + debug!( + opctx.log, + "deleted {} IP pool links for silo {}", updated_rows, id + ); + Ok(()) } } diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index 7843e816fd..77a5cd5c8a 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -275,6 +275,32 @@ async fn test_ip_pool_list_dedupe(cptestctx: &ControlPlaneTestContext) { let silo3_pools = pools_for_silo(client, "silo3").await; assert_eq!(silo3_pools.len(), 1); assert_eq!(silo3_pools[0].identity.name, "pool1"); + + // this is a great spot to check that deleting a pool cleans up the links! + + // first we have to delete the range, otherwise delete will fail + let url = "/v1/system/ip-pools/pool1/ranges/remove"; + NexusRequest::new( + RequestBuilder::new(client, Method::POST, url) + .body(Some(&range1)) + .expect_status(Some(StatusCode::NO_CONTENT)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to delete IP range from a pool"); + + object_delete(client, "/v1/system/ip-pools/pool1").await; + + let silo1_pools = pools_for_silo(client, "silo1").await; + assert_eq!(silo1_pools.len(), 1); + assert_eq!(silo1_pools[0].id(), pool2.id()); + + let silo2_pools = pools_for_silo(client, "silo2").await; + assert_eq!(silo2_pools.len(), 0); + + let silo3_pools = pools_for_silo(client, "silo3").await; + assert_eq!(silo3_pools.len(), 0); } /// The internal IP pool, defined by its association with the internal silo, @@ -474,18 +500,6 @@ async fn test_ip_pool_silo_link(cptestctx: &ControlPlaneTestContext) { .await; assert_eq!(error.error_code.unwrap(), "ObjectAlreadyExists"); - // pool delete fails because it is linked to a silo - let error = object_delete_error( - client, - "/v1/system/ip-pools/p1", - StatusCode::BAD_REQUEST, - ) - .await; - assert_eq!( - error.message, - "IP Pool cannot be deleted while it is linked to a silo", - ); - // 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; diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index a5d4b47eaa..86bf01062f 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -16,8 +16,9 @@ use nexus_db_queries::db::identity::Asset; use nexus_db_queries::db::lookup::LookupPath; use nexus_test_utils::http_testing::{AuthnMode, NexusRequest, RequestBuilder}; use nexus_test_utils::resource_helpers::{ - create_local_user, create_project, create_silo, grant_iam, object_create, - objects_list_page_authz, projects_list, + create_ip_pool, create_local_user, create_project, create_silo, grant_iam, + link_ip_pool, object_create, object_delete, objects_list_page_authz, + projects_list, }; use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::views::Certificate; @@ -25,6 +26,7 @@ use nexus_types::external_api::views::{ self, IdentityProvider, Project, SamlIdentityProvider, Silo, }; use nexus_types::external_api::{params, shared}; +use omicron_common::address::{IpRange, Ipv4Range}; use omicron_common::api::external::ObjectIdentity; use omicron_common::api::external::{ IdentityMetadataCreateParams, LookupType, Name, @@ -2526,3 +2528,102 @@ async fn test_silo_admin_can_create_certs(cptestctx: &ControlPlaneTestContext) { assert_eq!(silo_certs.len(), 1); assert_eq!(silo_certs[0].identity.id, cert.identity.id); } + +// Test that silo delete cleans up associated groups +#[nexus_test] +async fn test_silo_delete_cleans_up_ip_pool_links( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; + + // Create a silo + let silo1 = + create_silo(&client, "silo1", true, shared::SiloIdentityMode::SamlJit) + .await; + let silo2 = + create_silo(&client, "silo2", true, shared::SiloIdentityMode::SamlJit) + .await; + + // link pool1 to both, link pool2 to silo1 only + let range1 = IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 51), + std::net::Ipv4Addr::new(10, 0, 0, 52), + ) + .unwrap(), + ); + create_ip_pool(client, "pool1", Some(range1)).await; + link_ip_pool(client, "pool1", &silo1.identity.id, true).await; + link_ip_pool(client, "pool1", &silo2.identity.id, true).await; + + let range2 = IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 53), + std::net::Ipv4Addr::new(10, 0, 0, 54), + ) + .unwrap(), + ); + create_ip_pool(client, "pool2", Some(range2)).await; + link_ip_pool(client, "pool2", &silo1.identity.id, false).await; + + // we want to make sure the links are there before we make sure they're gone + let url = "/v1/system/ip-pools/pool1/silos"; + let links = + objects_list_page_authz::(client, &url).await; + assert_eq!(links.items.len(), 2); + + let url = "/v1/system/ip-pools/pool2/silos"; + let links = + objects_list_page_authz::(client, &url).await; + assert_eq!(links.items.len(), 1); + + // Delete the silo + let url = format!("/v1/system/silos/{}", silo1.identity.id); + object_delete(client, &url).await; + + // Now make sure the links are gone + let url = "/v1/system/ip-pools/pool1/silos"; + let links = + objects_list_page_authz::(client, &url).await; + assert_eq!(links.items.len(), 1); + + let url = "/v1/system/ip-pools/pool2/silos"; + let links = + objects_list_page_authz::(client, &url).await; + assert_eq!(links.items.len(), 0); + + // but the pools are of course still there + let url = "/v1/system/ip-pools"; + let pools = objects_list_page_authz::(client, &url).await; + assert_eq!(pools.items.len(), 2); + assert_eq!(pools.items[0].identity.name, "pool1"); + assert_eq!(pools.items[1].identity.name, "pool2"); + + // nothing prevents us from deleting the pools (except the child ranges -- + // we do have to remove those) + + let url = "/v1/system/ip-pools/pool1/ranges/remove"; + NexusRequest::new( + RequestBuilder::new(client, Method::POST, url) + .body(Some(&range1)) + .expect_status(Some(StatusCode::NO_CONTENT)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to delete IP range from a pool"); + + let url = "/v1/system/ip-pools/pool2/ranges/remove"; + NexusRequest::new( + RequestBuilder::new(client, Method::POST, url) + .body(Some(&range2)) + .expect_status(Some(StatusCode::NO_CONTENT)), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("Failed to delete IP range from a pool"); + + object_delete(client, "/v1/system/ip-pools/pool1").await; + object_delete(client, "/v1/system/ip-pools/pool2").await; +} diff --git a/schema/crdb/25.0.0/up.sql b/schema/crdb/25.0.0/up.sql new file mode 100644 index 0000000000..3c963b9bc6 --- /dev/null +++ b/schema/crdb/25.0.0/up.sql @@ -0,0 +1,7 @@ +-- created solely to prevent a table scan when we delete links on silo delete +CREATE INDEX IF NOT EXISTS ip_pool_resource_id ON omicron.public.ip_pool_resource ( + resource_id +); +CREATE INDEX IF NOT EXISTS ip_pool_resource_ip_pool_id ON omicron.public.ip_pool_resource ( + ip_pool_id +); diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 2105caabef..f3ca5c4b85 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -1604,6 +1604,14 @@ CREATE UNIQUE INDEX IF NOT EXISTS one_default_ip_pool_per_resource ON omicron.pu ) where is_default = true; +-- created solely to prevent a table scan when we delete links on silo delete +CREATE INDEX IF NOT EXISTS ip_pool_resource_id ON omicron.public.ip_pool_resource ( + resource_id +); +CREATE INDEX IF NOT EXISTS ip_pool_resource_ip_pool_id ON omicron.public.ip_pool_resource ( + ip_pool_id +); + /* * IP Pools are made up of a set of IP ranges, which are start/stop addresses. * Note that these need not be CIDR blocks or well-behaved subnets with a @@ -3258,7 +3266,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - ( TRUE, NOW(), NOW(), '24.0.0', NULL) + ( TRUE, NOW(), NOW(), '25.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT;