From 6a55e8966c0cd4708674b40e46fe253032fa5e28 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 6 Dec 2023 13:11:07 -0600 Subject: [PATCH] add make_default endpoint and give it one lil test --- nexus/db-queries/src/db/datastore/ip_pool.rs | 84 ++++++++++++++++++- .../db-queries/src/db/queries/external_ip.rs | 4 +- nexus/src/app/ip_pool.rs | 15 ++++ nexus/src/external_api/http_entrypoints.rs | 33 +++++++- nexus/tests/integration_tests/instances.rs | 44 ++++++---- nexus/tests/integration_tests/ip_pools.rs | 58 ++++++++----- nexus/types/src/external_api/params.rs | 5 -- 7 files changed, 196 insertions(+), 47 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index 4f221d86f3..4696ed2070 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -21,9 +21,11 @@ use crate::db::model::{ use crate::db::pagination::paginated; use crate::db::pool::DbConnection; use crate::db::queries::ip_pool::FilterOverlappingIpRanges; -use async_bb8_diesel::AsyncRunQueryDsl; +use crate::db::TransactionError; +use async_bb8_diesel::{AsyncConnection, AsyncRunQueryDsl}; use chrono::Utc; use diesel::prelude::*; +use diesel::result::Error as DieselError; use ipnetwork::IpNetwork; use nexus_db_model::ExternalIp; use nexus_db_model::IpPoolResourceType; @@ -362,6 +364,8 @@ impl DataStore { .map_err(|e| public_error_from_diesel(e, ErrorHandler::Server)) } + // TODO: separate this operation from update so that we can have /link 409 + // or whatever when the association already exists? pub async fn ip_pool_associate_resource( &self, opctx: &OpContext, @@ -408,6 +412,84 @@ impl DataStore { }) } + // TODO: make default should fail when the association doesn't exist. + // should it also fail when it's already default? probably not? + pub async fn ip_pool_make_default( + &self, + opctx: &OpContext, + authz_ip_pool: &authz::IpPool, + authz_silo: &authz::Silo, + ) -> UpdateResult { + use db::schema::ip_pool_resource::dsl; + + // TODO: correct auth check + opctx + .authorize(authz::Action::CreateChild, &authz::IP_POOL_LIST) + .await?; + + let ip_pool_id = authz_ip_pool.id(); + let silo_id = authz_silo.id(); + + // Errors returned from the below transactions. + #[derive(Debug)] + enum IpPoolResourceUpdateError { + FailedToUnsetDefault(DieselError), + } + type TxnError = TransactionError; + + let conn = self.pool_connection_authorized(opctx).await?; + conn.transaction_async(|conn| async move { + // note this is matching the specified silo, but could be any pool + let existing_default_for_silo = dsl::ip_pool_resource + .filter(dsl::resource_type.eq(IpPoolResourceType::Silo)) + .filter(dsl::resource_id.eq(silo_id)) + .filter(dsl::is_default.eq(true)) + .select(IpPoolResource::as_select()) + .get_result_async(&conn) + .await; + + // if there is an existing default, we need to unset it before we can + // set the new default + if let Ok(existing_default) = existing_default_for_silo { + // if the pool we're making default is already default for this + // silo, don't error: just noop + if existing_default.ip_pool_id == ip_pool_id { + return Ok(existing_default); + } + + let unset_default = diesel::update(dsl::ip_pool_resource) + .filter(dsl::resource_id.eq(existing_default.resource_id)) + .filter(dsl::ip_pool_id.eq(existing_default.ip_pool_id)) + .filter( + dsl::resource_type.eq(existing_default.resource_type), + ) + .set(dsl::is_default.eq(false)) + .execute_async(&conn) + .await; + if let Err(e) = unset_default { + return Err(TxnError::CustomError( + IpPoolResourceUpdateError::FailedToUnsetDefault(e), + )); + } + } + + // TODO: test that this errors if the link doesn't exist already + let updated_link = diesel::update(dsl::ip_pool_resource) + .filter(dsl::resource_id.eq(silo_id)) + .filter(dsl::ip_pool_id.eq(ip_pool_id)) + .filter(dsl::resource_type.eq(IpPoolResourceType::Silo)) + .set(dsl::is_default.eq(true)) + .returning(IpPoolResource::as_returning()) + .get_result_async(&conn) + .await?; + Ok(updated_link) + }) + .await + .map_err(|e| { + Error::internal_error(&format!("Transaction error: {:?}", e)) + }) + } + // TODO: write a test for this async fn ensure_no_ips_outstanding( &self, diff --git a/nexus/db-queries/src/db/queries/external_ip.rs b/nexus/db-queries/src/db/queries/external_ip.rs index cb05a910a2..baf27a8b1b 100644 --- a/nexus/db-queries/src/db/queries/external_ip.rs +++ b/nexus/db-queries/src/db/queries/external_ip.rs @@ -886,7 +886,7 @@ mod tests { self.db_datastore .ip_pool_associate_resource(&self.opctx, association) .await - .expect("Failed to associate IP dool with silo"); + .expect("Failed to associate IP pool with silo"); self.initialize_ip_pool(name, range).await; } @@ -1712,6 +1712,8 @@ mod tests { Ipv4Addr::new(10, 0, 0, 6), )) .unwrap(); + // TODO: failing because I changed create_ip_pool to make it + // default for the silo, and there is already a default context.create_ip_pool("p1", second_range).await; // Allocating an address on an instance in the second pool should be diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs index 7284facf72..e97fd2b9df 100644 --- a/nexus/src/app/ip_pool.rs +++ b/nexus/src/app/ip_pool.rs @@ -135,6 +135,21 @@ impl super::Nexus { .await } + pub(crate) async fn ip_pool_make_default( + &self, + opctx: &OpContext, + pool_lookup: &lookup::IpPool<'_>, + silo_lookup: &lookup::Silo<'_>, + ) -> CreateResult { + let (.., authz_pool) = + pool_lookup.lookup_for(authz::Action::Modify).await?; + let (.., authz_silo) = + silo_lookup.lookup_for(authz::Action::Read).await?; + self.db_datastore + .ip_pool_make_default(opctx, &authz_pool, &authz_silo) + .await + } + pub(crate) async fn ip_pools_list( &self, opctx: &OpContext, diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 25e4039d24..b842c81c7d 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -125,6 +125,7 @@ pub(crate) fn external_api() -> NexusApiDescription { api.register(ip_pool_silo_list)?; api.register(ip_pool_silo_link)?; api.register(ip_pool_silo_unlink)?; + api.register(ip_pool_make_default)?; api.register(ip_pool_view)?; api.register(ip_pool_delete)?; api.register(ip_pool_update)?; @@ -1399,7 +1400,7 @@ async fn ip_pool_silo_link( async fn ip_pool_silo_unlink( rqctx: RequestContext>, path_params: Path, - query_params: Query, + query_params: Query, ) -> Result { let apictx = rqctx.context(); let handler = async { @@ -1418,6 +1419,36 @@ async fn ip_pool_silo_unlink( apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await } +// TODO: change this to PUT /ip-pools/{pool}/silos/{silo} so +// it can be used for both set default true and false + +/// Make an IP pool the default for a silo +#[endpoint { + method = POST, + path = "/v1/system/ip-pools/{pool}/make_default", + tags = ["system/networking"], +}] +async fn ip_pool_make_default( + rqctx: RequestContext>, + path_params: Path, + silo_selector: TypedBody, +) -> 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 silo_selector = silo_selector.into_inner(); + let pool_lookup = nexus.ip_pool_lookup(&opctx, &path.pool)?; + let silo_lookup = nexus.silo_lookup(&opctx, silo_selector.silo)?; + let assoc = nexus + .ip_pool_make_default(&opctx, &pool_lookup, &silo_lookup) + .await?; + Ok(HttpResponseCreated(assoc.into())) + }; + apictx.external_latencies.instrument_dropshot_handler(&rqctx, handler).await +} + /// Fetch the IP pool used for Oxide services #[endpoint { method = GET, diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 85b6f16317..c5d4abc23b 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -3583,35 +3583,45 @@ async fn test_instance_ephemeral_ip_from_correct_pool( ); // make first pool the default for the priv user's silo - let link = params::IpPoolSiloLink { - silo: NameOrId::Id(DEFAULT_SILO.id()), - is_default: true, - }; - create_ip_pool(&client, "default", Some(range1), Some(link)).await; + dbg!(DEFAULT_SILO.id()); + let silo = NameOrId::Id(DEFAULT_SILO.id()); + let link = params::IpPoolSiloLink { silo: silo.clone(), is_default: true }; + create_ip_pool(&client, "pool1", Some(range1), Some(link)).await; // second pool is associated with the silo but not default - let link = params::IpPoolSiloLink { - silo: NameOrId::Id(DEFAULT_SILO.id()), - is_default: false, - }; - create_ip_pool(&client, "other-pool", Some(range2), Some(link)).await; + let link = params::IpPoolSiloLink { silo: silo.clone(), is_default: false }; + create_ip_pool(&client, "pool2", Some(range2), Some(link)).await; // Create an instance with pool name blank, expect IP from default pool - create_instance_with_pool(client, "default-pool-inst", None).await; + create_instance_with_pool(client, "pool1-inst", None).await; - let ip = fetch_instance_ephemeral_ip(client, "default-pool-inst").await; + let ip = fetch_instance_ephemeral_ip(client, "pool1-inst").await; assert!( ip.ip >= range1.first_address() && ip.ip <= range1.last_address(), - "Expected ephemeral IP to come from default pool" + "Expected ephemeral IP to come from pool1" ); // Create an instance explicitly using the non-default "other-pool". - create_instance_with_pool(client, "other-pool-inst", Some("other-pool")) - .await; - let ip = fetch_instance_ephemeral_ip(client, "other-pool-inst").await; + create_instance_with_pool(client, "pool2-inst", Some("pool2")).await; + let ip = fetch_instance_ephemeral_ip(client, "pool2-inst").await; + assert!( + ip.ip >= range2.first_address() && ip.ip <= range2.last_address(), + "Expected ephemeral IP to come from pool2" + ); + + // make pool2 default and create instance with default pool. check that it now it comes from pool2 + let _: views::IpPoolSilo = object_create( + client, + "/v1/system/ip-pools/pool2/make_default", + ¶ms::SiloSelector { silo: silo.clone() }, + ) + .await; + + create_instance_with_pool(client, "pool2-inst2", None).await; + let ip = fetch_instance_ephemeral_ip(client, "pool2-inst2").await; assert!( ip.ip >= range2.first_address() && ip.ip <= range2.last_address(), - "Expected ephemeral IP to come from other pool" + "Expected ephemeral IP to come from pool2" ); } diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index 515184dbb9..ca8308a441 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -30,6 +30,7 @@ use nexus_types::external_api::params::InstanceNetworkInterfaceAttachment; use nexus_types::external_api::params::IpPoolCreate; use nexus_types::external_api::params::IpPoolSiloLink; use nexus_types::external_api::params::IpPoolUpdate; +use nexus_types::external_api::params::SiloSelector; use nexus_types::external_api::shared::IpRange; use nexus_types::external_api::shared::Ipv4Range; use nexus_types::external_api::shared::Ipv6Range; @@ -351,7 +352,7 @@ async fn test_ip_pool_with_silo(cptestctx: &ControlPlaneTestContext) { let p1 = create_pool(client, "p1").await; // there should be no associations - let assocs_p0 = get_associations(client, "p0").await; + let assocs_p0 = silos_for_pool(client, "p0").await; assert_eq!(assocs_p0.items.len(), 0); // expect 404 on association if the specified silo doesn't exist @@ -383,46 +384,62 @@ async fn test_ip_pool_with_silo(cptestctx: &ControlPlaneTestContext) { ); // associate by name with silo that exists - let params = params::IpPoolSiloLink { - silo: NameOrId::Name(cptestctx.silo_name.clone()), - is_default: false, - }; + let silo = NameOrId::Name(cptestctx.silo_name.clone()); + let params = + params::IpPoolSiloLink { silo: silo.clone(), is_default: false }; let _: IpPoolSilo = object_create(client, "/v1/system/ip-pools/p0/silos", ¶ms).await; // get silo ID so we can test association by ID as well let silo_url = format!("/v1/system/silos/{}", cptestctx.silo_name); - let silo = NexusRequest::object_get(client, &silo_url) + let silo_id = NexusRequest::object_get(client, &silo_url) .authn_as(AuthnMode::PrivilegedUser) .execute_and_parse_unwrap::() - .await; - let silo_id = silo.identity.id; + .await + .identity + .id; - let assocs_p0 = get_associations(client, "p0").await; - let silo_assoc = + let assocs_p0 = silos_for_pool(client, "p0").await; + let silo_link = IpPoolSilo { 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_assoc); + assert_eq!(assocs_p0.items[0], silo_link); // TODO: dissociate silo // TODO: confirm dissociation // associate same silo to other pool by ID let params = params::IpPoolSiloLink { - silo: NameOrId::Id(silo.identity.id), + silo: NameOrId::Id(silo_id), is_default: false, }; let _: IpPoolSilo = object_create(client, "/v1/system/ip-pools/p1/silos", ¶ms).await; // association should look the same as the other one, except different pool ID - let assocs_p1 = get_associations(client, "p1").await; - assert_eq!(assocs_p1.items.len(), 1); + let silos_p1 = silos_for_pool(client, "p1").await; + assert_eq!(silos_p1.items.len(), 1); assert_eq!( - assocs_p1.items[0], - IpPoolSilo { ip_pool_id: p1.identity.id, ..silo_assoc } + silos_p1.items[0], + IpPoolSilo { ip_pool_id: p1.identity.id, ..silo_link } ); + // make p0's pool default and show that it changes + let params = SiloSelector { silo: silo.clone() }; + let _: IpPoolSilo = + object_create(client, "/v1/system/ip-pools/p0/make_default", ¶ms) + .await; + // making the same one default again is not an error + let _: IpPoolSilo = + object_create(client, "/v1/system/ip-pools/p0/make_default", ¶ms) + .await; + + let silos_p0 = silos_for_pool(client, "p0").await; + assert_eq!(silos_p0.items.len(), 1); + assert_eq!(silos_p0.items[0].is_default, true); + + // TODO: unset default + // TODO: associating a resource that is already associated should be a noop // and return a success message @@ -471,15 +488,12 @@ fn get_names(pools: Vec) -> Vec { pools.iter().map(|p| p.identity.name.to_string()).collect() } -async fn get_associations( +async fn silos_for_pool( client: &ClientTestContext, id: &str, ) -> ResultsPage { - objects_list_page_authz::( - client, - &format!("/v1/system/ip-pools/{}/silos", id), - ) - .await + let url = format!("/v1/system/ip-pools/{}/silos", id); + objects_list_page_authz::(client, &url).await } async fn create_pool(client: &ClientTestContext, name: &str) -> IpPool { diff --git a/nexus/types/src/external_api/params.rs b/nexus/types/src/external_api/params.rs index 9c91cf3e82..ebd8fe86b1 100644 --- a/nexus/types/src/external_api/params.rs +++ b/nexus/types/src/external_api/params.rs @@ -763,11 +763,6 @@ pub struct IpPoolSiloLink { pub is_default: bool, } -#[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)] -pub struct IpPoolSiloUnlink { - pub silo: NameOrId, -} - // INSTANCES /// Describes an attachment of an `InstanceNetworkInterface` to an `Instance`,