From d720313289003560892ca877f4093adf8969d214 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Wed, 5 Nov 2025 16:34:27 +0000 Subject: [PATCH 1/3] Search all ranges in a pool for the next external IP - Add a where-clause to ensure we skip IP Pool Ranges which have been exhausted when searching for the next external IP address. - Add regressions - Fixes #9342 --- .../db-queries/src/db/queries/external_ip.rs | 172 +++++++++++++++++- .../output/next_automatic_floating_ip.sql | 2 + 2 files changed, 164 insertions(+), 10 deletions(-) diff --git a/nexus/db-queries/src/db/queries/external_ip.rs b/nexus/db-queries/src/db/queries/external_ip.rs index d096a125be1..91aacefc5b1 100644 --- a/nexus/db-queries/src/db/queries/external_ip.rs +++ b/nexus/db-queries/src/db/queries/external_ip.rs @@ -458,7 +458,10 @@ impl NextExternalIp { UNION ALL ", ); self.push_automatic_full_ip_subquery_body(out.reborrow())?; - out.push_sql(") AS all_candidates ORDER BY candidate_ip LIMIT 1 "); + out.push_sql(") AS all_candidates \ + WHERE candidate_ip IS NOT NULL \ + ORDER BY candidate_ip \ + LIMIT 1 "); Ok(()) } @@ -857,7 +860,7 @@ mod tests { name: &str, range: IpRange, is_default: bool, - ) -> authz::IpPool { + ) -> (authz::IpPool, IpPool) { let pool = IpPool::new( &IdentityMetadataCreateParams { name: name.parse().unwrap(), @@ -867,7 +870,7 @@ mod tests { IpPoolReservationType::ExternalSilos, ); - self.db + let db_pool = self.db .datastore() .ip_pool_create(self.db.opctx(), pool.clone()) .await @@ -888,12 +891,13 @@ mod tests { self.initialize_ip_pool(name, range).await; - LookupPath::new(self.db.opctx(), self.db.datastore()) + let authz_pool = LookupPath::new(self.db.opctx(), self.db.datastore()) .ip_pool_id(pool.id()) .lookup_for(authz::Action::Read) .await .unwrap() - .0 + .0; + (authz_pool, db_pool) } async fn initialize_ip_pool(&self, name: &str, range: IpRange) { @@ -1603,7 +1607,7 @@ mod tests { Ipv4Addr::new(10, 0, 0, 6), )) .unwrap(); - let p1 = context.create_ip_pool("p1", second_range, false).await; + let (p1, ..) = context.create_ip_pool("p1", second_range, false).await; // Allocating an address on an instance in the second pool should be // respected, even though there are IPs available in the first. @@ -1648,7 +1652,7 @@ mod tests { let last_address = Ipv4Addr::new(10, 0, 0, 6); let second_range = IpRange::try_from((first_address, last_address)).unwrap(); - let p1 = context.create_ip_pool("p1", second_range, false).await; + let (p1, ..) = context.create_ip_pool("p1", second_range, false).await; // Allocate all available addresses in the second pool. let first_octet = first_address.octets()[3]; @@ -1810,7 +1814,7 @@ mod tests { let first_address = Ipv4Addr::new(10, 0, 0, 1); let last_address = Ipv4Addr::new(10, 0, 0, 3); let range = IpRange::try_from((first_address, last_address)).unwrap(); - let p1 = context.create_ip_pool("default", range, true).await; + let (p1, ..) = context.create_ip_pool("default", range, true).await; let mut ips = Vec::with_capacity(range.len() as _); let mut instance_id = None; @@ -1870,7 +1874,7 @@ mod tests { let first_address = Ipv4Addr::new(10, 0, 0, 1); let last_address = Ipv4Addr::new(10, 0, 0, 3); let range = IpRange::try_from((first_address, last_address)).unwrap(); - let p1 = context.create_ip_pool("default", range, true).await; + let (p1, ..) = context.create_ip_pool("default", range, true).await; let mut ips = Vec::with_capacity(range.len() as usize * 4); let mut instance_id = None; @@ -1937,7 +1941,7 @@ mod tests { "fd00::ffff".parse::().unwrap(), )) .unwrap(); - let pool = context.create_ip_pool("default", range, true).await; + let (pool, ..) = context.create_ip_pool("default", range, true).await; let start = std::time::Instant::now(); for (i, expected_addr) in range.iter().enumerate() { @@ -2076,4 +2080,152 @@ mod tests { context.success().await; } + + #[tokio::test] + async fn can_allocate_ephemeral_ips_from_all_ranges_in_a_pool() { + let context = TestContext::new( + "can_allocate_ephemeral_ips_from_all_ranges_in_a_pool", + ) + .await; + + // Create two ranges in the same pool. Each range will have one address + // for simplicity. + let addrs = [ + Ipv4Addr::new(10, 0, 0, 1), + Ipv4Addr::new(10, 0, 0, 2), + ]; + let range1 = IpRange::try_from((addrs[0], addrs[0])).unwrap(); + let range2 = IpRange::try_from((addrs[1], addrs[1])).unwrap(); + let (authz_pool, db_pool) = context.create_ip_pool("default", range1, true).await; + let _ = context + .db + .datastore() + .ip_pool_add_range( + context.db.opctx(), + &authz_pool, + &db_pool, + &range2 + ) + .await + .expect("able to add a second range to the pool"); + + // Allocate an instance and address, which should take the first address + // (which is the whole range). + let iid = context.create_instance("inst1").await; + let ip = context + .db + .datastore() + .allocate_instance_ephemeral_ip( + context.db.opctx(), + Uuid::new_v4(), + iid, + Some(authz_pool.clone()), + true, + ) + .await + .expect("Failed to allocate instance ephemeral IP address") + .0; + if let IpAddr::V4(addr) = ip.ip.ip() { + assert_eq!(addr, addrs[0]); + } else { + panic!("Expected an IPv4 address"); + } + + // Allocate another one, which should take the second address, which is + // "all" of the second range. + let iid = context.create_instance("inst2").await; + let ip = context + .db + .datastore() + .allocate_instance_ephemeral_ip( + context.db.opctx(), + Uuid::new_v4(), + iid, + Some(authz_pool.clone()), + true, + ) + .await + .expect("Failed to allocate instance ephemeral IP address") + .0; + if let IpAddr::V4(addr) = ip.ip.ip() { + assert_eq!(addr, addrs[1]); + } else { + panic!("Expected an IPv4 address"); + } + + context.success().await; + } + + #[tokio::test] + async fn can_allocate_snat_ips_from_all_ranges_in_a_pool() { + let context = TestContext::new( + "can_allocate_snat_ips_from_all_ranges_in_a_pool", + ) + .await; + + // Create two ranges in the same pool. Each range will have one address + // for simplicity. + let addrs = [ + Ipv4Addr::new(10, 0, 0, 1), + Ipv4Addr::new(10, 0, 0, 2), + ]; + let range1 = IpRange::try_from((addrs[0], addrs[0])).unwrap(); + let range2 = IpRange::try_from((addrs[1], addrs[1])).unwrap(); + let (authz_pool, db_pool) = context.create_ip_pool("default", range1, true).await; + let _ = context + .db + .datastore() + .ip_pool_add_range( + context.db.opctx(), + &authz_pool, + &db_pool, + &range2 + ) + .await + .expect("able to add a second range to the pool"); + + // Allocate 4 instances, to take the whole address that constitutes the + // first range. + for i in 0..4 { + let iid = context.create_instance(&format!("inst{i}")).await; + let ip = context + .db + .datastore() + .allocate_instance_snat_ip( + context.db.opctx(), + Uuid::new_v4(), + iid, + db_pool.id(), + ) + .await + .expect("Failed to allocate instance SNAT IP address"); + if let IpAddr::V4(addr) = ip.ip.ip() { + assert_eq!(addr, addrs[0]); + } else { + panic!("Expected an IPv4 address"); + } + } + + // Allocate another one, which should take the second address, the first + // port block in the second range. + let iid = context.create_instance("last").await; + let ip = context + .db + .datastore() + .allocate_instance_snat_ip( + context.db.opctx(), + Uuid::new_v4(), + iid, + db_pool.id(), + ) + .await + .expect("Failed to allocate instance SNAT IP address"); + if let IpAddr::V4(addr) = ip.ip.ip() { + assert_eq!(addr, addrs[1]); + } else { + panic!("Expected an IPv4 address"); + } + + context.success().await; + } } diff --git a/nexus/db-queries/tests/output/next_automatic_floating_ip.sql b/nexus/db-queries/tests/output/next_automatic_floating_ip.sql index b7bb6a0ba5f..70461b2c23a 100644 --- a/nexus/db-queries/tests/output/next_automatic_floating_ip.sql +++ b/nexus/db-queries/tests/output/next_automatic_floating_ip.sql @@ -99,6 +99,8 @@ WITH r.ip_pool_id = $19 AND r.time_deleted IS NULL ) AS all_candidates + WHERE + candidate_ip IS NOT NULL ORDER BY candidate_ip LIMIT From 35119941966636e620940cf0906f340e4d14e441 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Wed, 5 Nov 2025 16:36:31 +0000 Subject: [PATCH 2/3] fmt --- .../db-queries/src/db/queries/external_ip.rs | 49 +++++++++---------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/nexus/db-queries/src/db/queries/external_ip.rs b/nexus/db-queries/src/db/queries/external_ip.rs index 91aacefc5b1..4005c0d56f1 100644 --- a/nexus/db-queries/src/db/queries/external_ip.rs +++ b/nexus/db-queries/src/db/queries/external_ip.rs @@ -458,10 +458,12 @@ impl NextExternalIp { UNION ALL ", ); self.push_automatic_full_ip_subquery_body(out.reborrow())?; - out.push_sql(") AS all_candidates \ + out.push_sql( + ") AS all_candidates \ WHERE candidate_ip IS NOT NULL \ ORDER BY candidate_ip \ - LIMIT 1 "); + LIMIT 1 ", + ); Ok(()) } @@ -870,7 +872,8 @@ mod tests { IpPoolReservationType::ExternalSilos, ); - let db_pool = self.db + let db_pool = self + .db .datastore() .ip_pool_create(self.db.opctx(), pool.clone()) .await @@ -891,12 +894,13 @@ mod tests { self.initialize_ip_pool(name, range).await; - let authz_pool = LookupPath::new(self.db.opctx(), self.db.datastore()) - .ip_pool_id(pool.id()) - .lookup_for(authz::Action::Read) - .await - .unwrap() - .0; + let authz_pool = + LookupPath::new(self.db.opctx(), self.db.datastore()) + .ip_pool_id(pool.id()) + .lookup_for(authz::Action::Read) + .await + .unwrap() + .0; (authz_pool, db_pool) } @@ -2090,13 +2094,11 @@ mod tests { // Create two ranges in the same pool. Each range will have one address // for simplicity. - let addrs = [ - Ipv4Addr::new(10, 0, 0, 1), - Ipv4Addr::new(10, 0, 0, 2), - ]; + let addrs = [Ipv4Addr::new(10, 0, 0, 1), Ipv4Addr::new(10, 0, 0, 2)]; let range1 = IpRange::try_from((addrs[0], addrs[0])).unwrap(); let range2 = IpRange::try_from((addrs[1], addrs[1])).unwrap(); - let (authz_pool, db_pool) = context.create_ip_pool("default", range1, true).await; + let (authz_pool, db_pool) = + context.create_ip_pool("default", range1, true).await; let _ = context .db .datastore() @@ -2104,7 +2106,7 @@ mod tests { context.db.opctx(), &authz_pool, &db_pool, - &range2 + &range2, ) .await .expect("able to add a second range to the pool"); @@ -2158,20 +2160,17 @@ mod tests { #[tokio::test] async fn can_allocate_snat_ips_from_all_ranges_in_a_pool() { - let context = TestContext::new( - "can_allocate_snat_ips_from_all_ranges_in_a_pool", - ) - .await; + let context = + TestContext::new("can_allocate_snat_ips_from_all_ranges_in_a_pool") + .await; // Create two ranges in the same pool. Each range will have one address // for simplicity. - let addrs = [ - Ipv4Addr::new(10, 0, 0, 1), - Ipv4Addr::new(10, 0, 0, 2), - ]; + let addrs = [Ipv4Addr::new(10, 0, 0, 1), Ipv4Addr::new(10, 0, 0, 2)]; let range1 = IpRange::try_from((addrs[0], addrs[0])).unwrap(); let range2 = IpRange::try_from((addrs[1], addrs[1])).unwrap(); - let (authz_pool, db_pool) = context.create_ip_pool("default", range1, true).await; + let (authz_pool, db_pool) = + context.create_ip_pool("default", range1, true).await; let _ = context .db .datastore() @@ -2179,7 +2178,7 @@ mod tests { context.db.opctx(), &authz_pool, &db_pool, - &range2 + &range2, ) .await .expect("able to add a second range to the pool"); From f1056e3c67e80f6da02d8fa6820e194d145e64f1 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Wed, 5 Nov 2025 18:56:29 +0000 Subject: [PATCH 3/3] Fixup error message --- nexus/db-queries/src/db/queries/external_ip.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/queries/external_ip.rs b/nexus/db-queries/src/db/queries/external_ip.rs index 4005c0d56f1..f8c07314a42 100644 --- a/nexus/db-queries/src/db/queries/external_ip.rs +++ b/nexus/db-queries/src/db/queries/external_ip.rs @@ -1130,7 +1130,7 @@ mod tests { res.unwrap_err(), Error::insufficient_capacity( "No external IP addresses available", - "NextExternalIp::new tried to insert NULL ip", + "NextExternalIp::new returned NotFound", ), ); context.success().await;