diff --git a/nexus/db-queries/src/db/datastore/external_ip.rs b/nexus/db-queries/src/db/datastore/external_ip.rs index c3cd45669f..83eb6c9d94 100644 --- a/nexus/db-queries/src/db/datastore/external_ip.rs +++ b/nexus/db-queries/src/db/datastore/external_ip.rs @@ -67,12 +67,13 @@ impl DataStore { opctx: &OpContext, ip_id: Uuid, instance_id: Uuid, - pool_id: Uuid, + pool: Option, ) -> CreateResult { + let authz_pool = self.resolve_pool_for_allocation(&opctx, pool).await?; let data = IncompleteExternalIp::for_instance_source_nat( ip_id, instance_id, - pool_id, + authz_pool.id(), ); self.allocate_external_ip(opctx, data).await } diff --git a/nexus/db-queries/src/db/queries/external_ip.rs b/nexus/db-queries/src/db/queries/external_ip.rs index 7d5e254aac..61bd9a549a 100644 --- a/nexus/db-queries/src/db/queries/external_ip.rs +++ b/nexus/db-queries/src/db/queries/external_ip.rs @@ -1038,15 +1038,6 @@ mod tests { instance_id } - async fn default_pool_id(&self) -> Uuid { - let (.., pool) = self - .db_datastore - .ip_pools_fetch_default(&self.opctx) - .await - .expect("Failed to lookup default ip pool"); - pool.identity.id - } - async fn success(mut self) { self.db.cleanup().await.unwrap(); self.logctx.cleanup_successful(); @@ -1075,7 +1066,7 @@ mod tests { &context.opctx, id, instance_id, - context.default_pool_id().await, + None, ) .await .expect("Failed to allocate instance external IP address"); @@ -1092,7 +1083,7 @@ mod tests { &context.opctx, Uuid::new_v4(), instance_id, - context.default_pool_id().await, + None, ) .await .expect_err( @@ -1148,7 +1139,7 @@ mod tests { &context.opctx, Uuid::new_v4(), instance_id, - context.default_pool_id().await, + None, ) .await; assert!( @@ -1225,7 +1216,7 @@ mod tests { &context.opctx, Uuid::new_v4(), instance_id, - context.default_pool_id().await, + None, ) .await .expect("Failed to allocate instance external IP address"); @@ -1253,7 +1244,7 @@ mod tests { &context.opctx, Uuid::new_v4(), instance_id, - context.default_pool_id().await, + None, ) .await .expect("Failed to allocate instance external IP address"); @@ -1280,7 +1271,7 @@ mod tests { &context.opctx, Uuid::new_v4(), instance_id, - context.default_pool_id().await, + None, ) .await .expect("Failed to allocate instance external IP address"); @@ -1589,12 +1580,7 @@ mod tests { let id = Uuid::new_v4(); let ip = context .db_datastore - .allocate_instance_snat_ip( - &context.opctx, - id, - instance_id, - context.default_pool_id().await, - ) + .allocate_instance_snat_ip(&context.opctx, id, instance_id, None) .await .expect("Failed to allocate instance SNAT IP address"); assert_eq!(ip.kind, IpKind::SNat); @@ -1606,12 +1592,7 @@ mod tests { // value. let new_ip = context .db_datastore - .allocate_instance_snat_ip( - &context.opctx, - id, - instance_id, - context.default_pool_id().await, - ) + .allocate_instance_snat_ip(&context.opctx, id, instance_id, None) .await .expect("Failed to allocate instance SNAT IP address"); diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index 73fe910c76..f5d137879f 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -647,14 +647,35 @@ async fn sic_allocate_instance_snat_ip( let instance_id = sagactx.lookup::("instance_id")?; let ip_id = sagactx.lookup::("snat_ip_id")?; - let (.., pool) = datastore - .ip_pools_fetch_default(&opctx) - .await - .map_err(ActionError::action_failed)?; - let pool_id = pool.identity.id; + // If an ephemeral IP was asked for and it specified a pool, we + // want to use that pool for the SNAT IP. Otherwise we will use + // the default pool. + let maybe_pool = saga_params.create_params.external_ips.iter().find_map( + |eip| match eip { + params::ExternalIpCreate::Ephemeral { pool: Some(pool) } => { + Some(pool) + } + params::ExternalIpCreate::Ephemeral { pool: None } => None, + params::ExternalIpCreate::Floating { .. } => None, + }, + ); + + let maybe_pool = match maybe_pool { + Some(pool) => Some( + osagactx + .nexus() + .ip_pool_lookup(&opctx, pool) + .map_err(ActionError::action_failed)? + .lookup_for(authz::Action::CreateChild) + .await + .map_err(ActionError::action_failed)? + .0, + ), + None => None, + }; datastore - .allocate_instance_snat_ip(&opctx, ip_id, instance_id, pool_id) + .allocate_instance_snat_ip(&opctx, ip_id, instance_id, maybe_pool) .await .map_err(ActionError::action_failed)?; Ok(()) diff --git a/nexus/tests/integration_tests/instances.rs b/nexus/tests/integration_tests/instances.rs index 08cf195384..9287439058 100644 --- a/nexus/tests/integration_tests/instances.rs +++ b/nexus/tests/integration_tests/instances.rs @@ -3958,13 +3958,10 @@ async fn test_instance_ephemeral_ip_from_correct_pool( "Expected ephemeral IP to come from pool2" ); - // SNAT comes from default pool, but count does not change because - // SNAT IPs can be shared. https://github.com/oxidecomputer/omicron/issues/5043 - // is about getting SNAT IP from specified pool instead of default. assert_ip_pool_utilization(client, "pool1", 2, 5, 0, 0).await; - // ephemeral IP comes from specified pool - assert_ip_pool_utilization(client, "pool2", 1, 5, 0, 0).await; + // ephemeral IP and SNAT IP come from specified pool, so +2 + assert_ip_pool_utilization(client, "pool2", 2, 5, 0, 0).await; // make pool2 default and create instance with default pool. check that it now it comes from pool2 let _: views::IpPoolSiloLink = object_put( @@ -3983,7 +3980,8 @@ async fn test_instance_ephemeral_ip_from_correct_pool( // pool1 unchanged assert_ip_pool_utilization(client, "pool1", 2, 5, 0, 0).await; - // +1 snat (now that pool2 is default) and +1 ephemeral + // +1 ephemeral makes 3. SNAT comes from pool2 again, but count does not + // change because SNAT IPs can be shared. assert_ip_pool_utilization(client, "pool2", 3, 5, 0, 0).await; // try to delete association with pool1, but it fails because there is an @@ -4005,9 +4003,9 @@ async fn test_instance_ephemeral_ip_from_correct_pool( stop_and_delete_instance(&cptestctx, "pool2-inst").await; // pool1 is down to 0 because it had 1 snat + 1 ephemeral from pool1-inst - // and 1 snat from pool2-inst assert_ip_pool_utilization(client, "pool1", 0, 5, 0, 0).await; - // pool2 drops one because it had 1 ephemeral from pool2-inst + // pool2 drops one because it had 1 snat + 1 ephemeral from pool2-inst, but + // the snat is still in use by pool2-inst2 assert_ip_pool_utilization(client, "pool2", 2, 5, 0, 0).await; // now unlink works @@ -4163,8 +4161,7 @@ async fn test_instance_ephemeral_ip_no_default_pool_error( let url = format!("/v1/instances?project={}", PROJECT_NAME); let error = object_create_error(client, &url, &body, StatusCode::NOT_FOUND).await; - let msg = "not found: default IP pool for current silo".to_string(); - assert_eq!(error.message, msg); + assert_eq!(error.message, "not found: default IP pool for current silo"); // same deal if you specify a pool that doesn't exist let body = params::InstanceCreate { @@ -4175,7 +4172,10 @@ async fn test_instance_ephemeral_ip_no_default_pool_error( }; let error = object_create_error(client, &url, &body, StatusCode::NOT_FOUND).await; - assert_eq!(error.message, msg); + assert_eq!( + error.message, + "not found: ip-pool with name \"nonexistent-pool\"" + ); } #[nexus_test]