From 5b0191fe44d826fc53f905be9d83f066966ea5bd Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 17 Oct 2025 11:48:45 -0400 Subject: [PATCH 01/16] BlueprintBuilder: require caller to provide internal DNS subnet This is part of #9238: we'd like to pull `BlueprintResourceAllocator`'s weird construction out of `BlueprintBuilder`, and this removes _one_ use of it. When adding an internal DNS zone, the caller must now specify the `DnsSubet` instead of `BlueprintBuilder` choosing internally. To assist with this, adds `BlueprintBuilder::available_internal_dns_subnets()`, which returns an iterator of available subnets. I'm not sure this is the right interface; will leave a comment inline with some other ideas. --- .../planning/src/blueprint_builder/builder.rs | 47 ++++- .../planning/src/blueprint_editor.rs | 1 - .../src/blueprint_editor/allocators.rs | 37 +--- .../allocators/internal_dns.rs | 195 ------------------ nexus/reconfigurator/planning/src/example.rs | 5 + nexus/reconfigurator/planning/src/planner.rs | 21 +- 6 files changed, 66 insertions(+), 240 deletions(-) delete mode 100644 nexus/reconfigurator/planning/src/blueprint_editor/allocators/internal_dns.rs diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index aa0f1ae99ab..27be01663af 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -11,7 +11,6 @@ use crate::blueprint_editor::EditedSled; use crate::blueprint_editor::ExternalNetworkingChoice; use crate::blueprint_editor::ExternalNetworkingError; use crate::blueprint_editor::ExternalSnatNetworkingChoice; -use crate::blueprint_editor::NoAvailableDnsSubnets; use crate::blueprint_editor::SledEditError; use crate::blueprint_editor::SledEditor; use crate::mgs_updates::PendingHostPhase2Changes; @@ -65,6 +64,7 @@ use nexus_types::inventory::Collection; use omicron_common::address::CLICKHOUSE_HTTP_PORT; use omicron_common::address::DNS_HTTP_PORT; use omicron_common::address::DNS_PORT; +use omicron_common::address::DnsSubnet; use omicron_common::address::NTP_PORT; use omicron_common::address::ReservedRackSubnet; use omicron_common::api::external::Generation; @@ -138,8 +138,10 @@ pub enum Error { }, #[error("error constructing resource allocator")] AllocatorInput(#[from] BlueprintResourceAllocatorInputError), - #[error("error allocating internal DNS subnet")] - AllocateInternalDnsSubnet(#[from] NoAvailableDnsSubnets), + #[error("no commissioned sleds - rack subnet is unknown")] + RackSubnetUnknownNoSleds, + #[error("no reserved subnets available for internal DNS")] + NoAvailableDnsSubnets, #[error("error allocating external networking resources")] AllocateExternalNetworking(#[from] ExternalNetworkingError), #[error("can only have {INTERNAL_DNS_REDUNDANCY} internal DNS servers")] @@ -696,6 +698,40 @@ impl<'a> BlueprintBuilder<'a> { self.new_blueprint_id } + pub fn available_internal_dns_subnets( + &self, + ) -> Result, Error> { + // TODO-multirack We need the rack subnet to know what the reserved + // internal DNS subnets are. Pick any sled; this isn't right in + // multirack (either DNS will be on a wider subnet or we need to pick a + // particular rack subnet here?). + let any_sled_subnet = self + .input + .all_sled_resources(SledFilter::Commissioned) + .map(|(_sled_id, resources)| resources.subnet) + .next() + .ok_or(Error::RackSubnetUnknownNoSleds)?; + let rack_subnet = ReservedRackSubnet::from_subnet(any_sled_subnet); + + // Compute the "in use" subnets; this includes all in-service internal + // DNS zones _and_ any "expunged but not yet confirmed to be gone" + // zones, so we use the somewhat unusual `could_be_running` filter + // instead of the more typical `is_in_service`. + let internal_dns_subnets_in_use = self + .current_zones(BlueprintZoneDisposition::could_be_running) + .filter_map(|(_sled_id, zone)| match &zone.zone_type { + BlueprintZoneType::InternalDns(internal_dns) => { + Some(DnsSubnet::from_addr(*internal_dns.dns_address.ip())) + } + _ => None, + }) + .collect::>(); + + Ok(rack_subnet.get_dns_subnets().into_iter().filter(move |subnet| { + !internal_dns_subnets_in_use.contains(&subnet) + })) + } + fn resource_allocator( &mut self, ) -> Result<&mut BlueprintResourceAllocator, Error> { @@ -1380,12 +1416,9 @@ impl<'a> BlueprintBuilder<'a> { &mut self, sled_id: SledUuid, image_source: BlueprintZoneImageSource, + dns_subnet: DnsSubnet, ) -> Result<(), Error> { let gz_address_index = self.next_internal_dns_gz_address_index(sled_id); - let sled_subnet = self.sled_resources(sled_id)?.subnet; - let rack_subnet = ReservedRackSubnet::from_subnet(sled_subnet); - let dns_subnet = - self.resource_allocator()?.next_internal_dns_subnet(rack_subnet)?; let address = dns_subnet.dns_address(); let zpool = self.sled_select_zpool(sled_id, ZoneKind::InternalDns)?; let zone_type = diff --git a/nexus/reconfigurator/planning/src/blueprint_editor.rs b/nexus/reconfigurator/planning/src/blueprint_editor.rs index 001b51ec3b1..bb1e2dbaec1 100644 --- a/nexus/reconfigurator/planning/src/blueprint_editor.rs +++ b/nexus/reconfigurator/planning/src/blueprint_editor.rs @@ -11,7 +11,6 @@ mod sled_editor; pub use allocators::BlueprintResourceAllocatorInputError; pub use allocators::ExternalNetworkingError; -pub use allocators::NoAvailableDnsSubnets; pub use sled_editor::DatasetsEditError; pub use sled_editor::DisksEditError; pub use sled_editor::MultipleDatasetsOfKind; diff --git a/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs b/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs index 7ad4a20e398..5258fa0164a 100644 --- a/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs +++ b/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs @@ -8,23 +8,16 @@ use std::net::IpAddr; use super::SledEditor; use nexus_types::deployment::BlueprintZoneDisposition; -use nexus_types::deployment::BlueprintZoneType; -use nexus_types::deployment::blueprint_zone_type::InternalDns; -use omicron_common::address::DnsSubnet; use omicron_common::address::IpRange; -use omicron_common::address::ReservedRackSubnet; mod external_networking; -mod internal_dns; pub use self::external_networking::ExternalNetworkingError; -pub use self::internal_dns::NoAvailableDnsSubnets; pub(crate) use self::external_networking::ExternalNetworkingChoice; pub(crate) use self::external_networking::ExternalSnatNetworkingChoice; use self::external_networking::ExternalNetworkingAllocator; -use self::internal_dns::InternalDnsSubnetAllocator; #[derive(Debug, thiserror::Error)] pub enum BlueprintResourceAllocatorInputError { @@ -35,7 +28,6 @@ pub enum BlueprintResourceAllocatorInputError { #[derive(Debug)] pub(crate) struct BlueprintResourceAllocator { external_networking: ExternalNetworkingAllocator, - internal_dns: InternalDnsSubnetAllocator, } impl BlueprintResourceAllocator { @@ -46,26 +38,6 @@ impl BlueprintResourceAllocator { where I: Iterator + Clone, { - let internal_dns_subnets_in_use = all_sleds - .clone() - .flat_map(|editor| { - editor - // We use `could_be_running` here instead of `in_service` to - // avoid reusing an internal DNS subnet from an - // expunged-but-possibly-still-running zone. - .zones(BlueprintZoneDisposition::could_be_running) - .filter_map(|z| match z.zone_type { - BlueprintZoneType::InternalDns(InternalDns { - dns_address, - .. - }) => Some(DnsSubnet::from_addr(*dns_address.ip())), - _ => None, - }) - }) - .collect(); - let internal_dns = - InternalDnsSubnetAllocator::new(internal_dns_subnets_in_use); - let external_networking = ExternalNetworkingAllocator::new( all_sleds.clone().flat_map(|editor| { editor.zones(BlueprintZoneDisposition::is_in_service) @@ -77,14 +49,7 @@ impl BlueprintResourceAllocator { ) .map_err(BlueprintResourceAllocatorInputError::ExternalNetworking)?; - Ok(Self { external_networking, internal_dns }) - } - - pub fn next_internal_dns_subnet( - &mut self, - rack_subnet: ReservedRackSubnet, - ) -> Result { - self.internal_dns.alloc(rack_subnet) + Ok(Self { external_networking }) } pub(crate) fn next_external_ip_nexus( diff --git a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/internal_dns.rs b/nexus/reconfigurator/planning/src/blueprint_editor/allocators/internal_dns.rs deleted file mode 100644 index 9f2787f09a4..00000000000 --- a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/internal_dns.rs +++ /dev/null @@ -1,195 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -use omicron_common::address::DnsSubnet; -use omicron_common::address::ReservedRackSubnet; -use std::collections::BTreeSet; - -#[derive(Debug, thiserror::Error)] -#[error("no reserved subnets available for internal DNS")] -pub struct NoAvailableDnsSubnets; - -/// Internal DNS zones are not allocated an address in the sled's subnet. -/// Instead, they get a /64 subnet of the "reserved" rack subnet (so that -/// it's routable with IPv6), and use the first address in that. There may -/// be at most `INTERNAL_DNS_REDUNDANCY` subnets (and so servers) -/// allocated. This structure tracks which subnets are currently allocated. -#[derive(Debug)] -pub struct InternalDnsSubnetAllocator { - in_use: BTreeSet, -} - -impl InternalDnsSubnetAllocator { - pub fn new(in_use: BTreeSet) -> Self { - Self { in_use } - } - - /// Allocate the first available DNS subnet, or call a function to generate - /// a default. The default is needed because we can't necessarily guess the - /// correct reserved rack subnet (e.g., there might not be any internal DNS - /// zones in the parent blueprint, though that would itself be odd), but we - /// can derive it at runtime from the sled address. - pub fn alloc( - &mut self, - rack_subnet: ReservedRackSubnet, - ) -> Result { - let new = if let Some(first) = self.in_use.first() { - // Take the first available DNS subnet. We currently generate - // all `INTERNAL_DNS_REDUNDANCY` subnets and subtract any - // that are in use; this is fine as long as that constant is small. - let subnets = BTreeSet::from_iter( - ReservedRackSubnet::from_subnet(first.subnet()) - .get_dns_subnets(), - ); - let mut avail = subnets.difference(&self.in_use); - if let Some(first) = avail.next() { - *first - } else { - return Err(NoAvailableDnsSubnets); - } - } else { - rack_subnet.get_dns_subnet(1) - }; - self.in_use.insert(new); - Ok(new) - } - - #[cfg(test)] - fn first(&self) -> Option { - self.in_use.first().copied() - } - - #[cfg(test)] - fn pop_first(&mut self) -> Option { - self.in_use.pop_first() - } - - #[cfg(test)] - fn last(&self) -> Option { - self.in_use.last().cloned() - } - - #[cfg(test)] - fn len(&self) -> usize { - self.in_use.len() - } -} - -#[cfg(test)] -pub mod test { - use super::*; - use crate::blueprint_builder::test::verify_blueprint; - use crate::example::ExampleSystemBuilder; - use nexus_types::deployment::BlueprintZoneDisposition; - use nexus_types::deployment::BlueprintZoneType; - use nexus_types::deployment::blueprint_zone_type::InternalDns; - use omicron_common::disk::DatasetKind; - use omicron_common::policy::INTERNAL_DNS_REDUNDANCY; - use omicron_test_utils::dev::test_setup_log; - - #[test] - fn test_dns_subnet_allocator() { - static TEST_NAME: &str = "test_dns_subnet_allocator"; - let logctx = test_setup_log(TEST_NAME); - - // Our test below assumes `INTERNAL_DNS_REDUNDANCY` is greater than 1 - // (so we can test adding more); fail fast if that changes. - assert!(INTERNAL_DNS_REDUNDANCY > 1); - - // Use our example system to create a blueprint and input. - let (_, mut blueprint1) = - ExampleSystemBuilder::new(&logctx.log, TEST_NAME) - .nsleds(INTERNAL_DNS_REDUNDANCY) - .build(); - - // `ExampleSystem` adds an internal DNS server to every sled. Manually - // prune out all but the first of them to give us space to add more. - for (_, sled_config) in blueprint1.sleds.iter_mut().skip(1) { - sled_config.zones.retain(|z| !z.zone_type.is_internal_dns()); - } - let npruned = blueprint1.sleds.len() - 1; - assert!(npruned > 0); - - // Also prune out the zones' datasets, or we're left with an invalid - // blueprint. - for (_, sled_config) in blueprint1.sleds.iter_mut().skip(1) { - sled_config.datasets.retain(|dataset| { - // This is gross; once zone configs know explicit dataset IDs, - // we should retain by ID instead. - match &dataset.kind { - DatasetKind::InternalDns => false, - DatasetKind::TransientZone { name } => { - !name.starts_with("oxz_internal_dns") - } - _ => true, - } - }); - } - - verify_blueprint(&blueprint1); - - // Create an allocator. - let mut allocator = InternalDnsSubnetAllocator::new( - blueprint1 - .all_omicron_zones(BlueprintZoneDisposition::is_in_service) - .filter_map(|(_sled_id, zone_config)| { - match zone_config.zone_type { - BlueprintZoneType::InternalDns(InternalDns { - dns_address, - .. - }) => Some(DnsSubnet::from_addr(*dns_address.ip())), - _ => None, - } - }) - .collect(), - ); - - // There should be exactly one subnet allocated. - assert_eq!(allocator.len(), 1, "should be 1 subnets allocated"); - let first = allocator.first().expect("should be a first subnet"); - let mut last = allocator.last().expect("should be a last subnet"); - assert_eq!(first, last); - let rack_subnet = first.rack_subnet(); - - // Allocate `npruned` new subnets. - for _ in 0..npruned { - let new = allocator.alloc(rack_subnet).expect("allocated a subnet"); - assert!( - new > last, - "newly allocated subnets should be after prior ones" - ); - assert_eq!( - new.rack_subnet(), - last.rack_subnet(), - "newly allocated subnets should be in the same rack subnet" - ); - last = new; - } - assert_eq!( - allocator.len(), - INTERNAL_DNS_REDUNDANCY, - "should be {INTERNAL_DNS_REDUNDANCY} subnets allocated" - ); - allocator.alloc(rack_subnet).expect_err("no subnets available"); - - // Test packing. - let first = allocator.pop_first().expect("should be a first subnet"); - let second = allocator.pop_first().expect("should be a second subnet"); - assert!(first < second, "first should be before second"); - assert_eq!( - allocator.alloc(rack_subnet).expect("allocation failed"), - first, - "should get first subnet" - ); - assert_eq!( - allocator.alloc(rack_subnet).expect("allocation failed"), - second, - "should get second subnet" - ); - allocator.alloc(rack_subnet).expect_err("no subnets available"); - - // Done! - logctx.cleanup_successful(); - } -} diff --git a/nexus/reconfigurator/planning/src/example.rs b/nexus/reconfigurator/planning/src/example.rs index 31e4d1d61e8..45517c54611 100644 --- a/nexus/reconfigurator/planning/src/example.rs +++ b/nexus/reconfigurator/planning/src/example.rs @@ -596,6 +596,8 @@ impl ExampleSystemBuilder { ) .unwrap(); } + let mut internal_dns_subnets = + builder.available_internal_dns_subnets().unwrap(); for _ in 0..self .internal_dns_count .on(discretionary_ix, discretionary_sled_count) @@ -608,6 +610,9 @@ impl ExampleSystemBuilder { .expect( "obtained InternalDNS image source", ), + internal_dns_subnets.next().expect( + "sufficient available internal DNS subnets", + ), ) .unwrap(); } diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index da6ec986921..99ea102373f 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -22,6 +22,7 @@ use crate::planner::image_source::NoopConvertHostPhase2Contents; use crate::planner::image_source::NoopConvertZoneStatus; use crate::planner::omicron_zone_placement::PlacementError; use iddqd::IdOrdMap; +use itertools::Either; use itertools::Itertools; use nexus_sled_agent_shared::inventory::ConfigReconcilerInventoryResult; use nexus_sled_agent_shared::inventory::OmicronZoneImageSource; @@ -66,6 +67,7 @@ use slog::{Logger, info, o, warn}; use slog_error_chain::InlineErrorChain; use std::collections::BTreeMap; use std::collections::BTreeSet; +use std::iter; use std::str::FromStr; use std::sync::Arc; use swrite::SWrite; @@ -1256,6 +1258,18 @@ impl<'a> Planner<'a> { image_source: BlueprintZoneImageSource, report: &mut PlanningAddStepReport, ) -> Result<(), Error> { + // If `kind` is "internal DNS", we'll need to pick subnets, but + // computing the available subnets isn't free. We could do something + // fancy with lazy construction, but that gets a little messy. Instead, + // always construct an iterator, and create an empty iterator for any + // `kind` that isn't "internal DNS". + let mut available_internal_dns_subnets = match kind { + DiscretionaryOmicronZone::InternalDns => { + Either::Left(self.blueprint.available_internal_dns_subnets()?) + } + _ => Either::Right(iter::empty()), + }; + for i in 0..num_zones_to_add { let sled_id = match zone_placement.place_zone(kind) { Ok(sled_id) => sled_id, @@ -1296,7 +1310,12 @@ impl<'a> Planner<'a> { .blueprint .sled_add_zone_crucible_pantry(sled_id, image)?, DiscretionaryOmicronZone::InternalDns => { - self.blueprint.sled_add_zone_internal_dns(sled_id, image)? + let dns_subnet = available_internal_dns_subnets + .next() + .ok_or(Error::NoAvailableDnsSubnets)?; + self.blueprint.sled_add_zone_internal_dns( + sled_id, image, dns_subnet, + )? } DiscretionaryOmicronZone::ExternalDns => { self.blueprint.sled_add_zone_external_dns(sled_id, image)? From aafec76caf693c1dbc878d9776a826c0853185b8 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 17 Oct 2025 17:17:07 -0400 Subject: [PATCH 02/16] first draft of ExternalIpPolicy type --- nexus/db-queries/src/db/datastore/rack.rs | 39 ++++--- .../tests/integration/blueprint_edit.rs | 8 +- nexus/reconfigurator/execution/src/dns.rs | 2 +- .../planning/src/blueprint_builder/builder.rs | 6 +- .../src/blueprint_editor/allocators.rs | 6 +- nexus/reconfigurator/planning/src/planner.rs | 7 +- nexus/reconfigurator/planning/src/system.rs | 17 +-- nexus/reconfigurator/preparation/src/lib.rs | 4 +- nexus/reconfigurator/simulation/src/system.rs | 27 +++-- nexus/types/src/deployment.rs | 2 + nexus/types/src/deployment/planning_input.rs | 103 +++++++++++++++++- 11 files changed, 169 insertions(+), 52 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index 35cf9cd8e9b..34d4fe9a867 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -1075,6 +1075,7 @@ mod test { use nexus_types::deployment::BlueprintSledConfig; use nexus_types::deployment::BlueprintSource; use nexus_types::deployment::CockroachDbPreserveDowngrade; + use nexus_types::deployment::ExternalIpPolicy; use nexus_types::deployment::PendingMgsUpdates; use nexus_types::deployment::{ BlueprintZoneConfig, OmicronZoneExternalFloatingAddr, @@ -1411,17 +1412,17 @@ mod test { let sled2 = create_test_sled(&datastore, SledUuid::new_v4()).await; let sled3 = create_test_sled(&datastore, SledUuid::new_v4()).await; - let service_ip_pool_ranges = vec![ + let external_ip_policy = ExternalIpPolicy::new(vec![ IpRange::try_from(( Ipv4Addr::new(1, 2, 3, 4), Ipv4Addr::new(1, 2, 3, 6), )) .unwrap(), - ]; + ]); let mut system = SystemDescription::new(); system - .service_ip_pool_ranges(service_ip_pool_ranges.clone()) + .external_ip_policy(external_ip_policy.clone()) .sled(SledBuilder::new().id(sled1.id())) .expect("failed to add sled1") .sled(SledBuilder::new().id(sled2.id())) @@ -1645,7 +1646,9 @@ mod test { &opctx, RackInit { blueprint: blueprint.clone(), - service_ip_pool_ranges, + service_ip_pool_ranges: external_ip_policy + .service_ip_pool_ranges() + .to_vec(), ..Default::default() }, ) @@ -1764,14 +1767,14 @@ mod test { // Ask for two Nexus services, with different external IPs. let nexus_ip_start = Ipv4Addr::new(1, 2, 3, 4); let nexus_ip_end = Ipv4Addr::new(1, 2, 3, 5); - let service_ip_pool_ranges = vec![ + let external_ip_policy = ExternalIpPolicy::new(vec![ IpRange::try_from((nexus_ip_start, nexus_ip_end)) .expect("Cannot create IP Range"), - ]; + ]); let mut system = SystemDescription::new(); system - .service_ip_pool_ranges(service_ip_pool_ranges.clone()) + .external_ip_policy(external_ip_policy.clone()) .sled(SledBuilder::new().id(sled.id())) .expect("failed to add sled"); @@ -1916,7 +1919,9 @@ mod test { RackInit { blueprint: blueprint.clone(), datasets: datasets.clone(), - service_ip_pool_ranges, + service_ip_pool_ranges: external_ip_policy + .service_ip_pool_ranges() + .to_vec(), internal_dns, external_dns, ..Default::default() @@ -2057,14 +2062,14 @@ mod test { Ipv6Addr::new(0xfd00, 0x1122, 0x3344, 0, 0, 0, 0, 1); let nexus_ip_end = Ipv6Addr::new(0xfd00, 0x1122, 0x3344, 0, 0, 0, 0, 10); - let service_ip_pool_ranges = vec![ + let external_ip_policy = ExternalIpPolicy::new(vec![ IpRange::try_from((nexus_ip_start, nexus_ip_end)) .expect("Cannot create IP Range"), - ]; + ]); let mut system = SystemDescription::new(); system - .service_ip_pool_ranges(service_ip_pool_ranges.clone()) + .external_ip_policy(external_ip_policy.clone()) .sled(SledBuilder::new().id(sled.id())) .expect("failed to add sled"); @@ -2167,7 +2172,9 @@ mod test { RackInit { blueprint: blueprint.clone(), datasets: datasets.clone(), - service_ip_pool_ranges, + service_ip_pool_ranges: external_ip_policy + .service_ip_pool_ranges() + .to_vec(), internal_dns, external_dns, ..Default::default() @@ -2410,11 +2417,11 @@ mod test { let sled = create_test_sled(&datastore, SledUuid::new_v4()).await; let ip = IpAddr::V4(Ipv4Addr::new(1, 2, 3, 4)); - let service_ip_pool_ranges = vec![IpRange::from(ip)]; + let external_ip_policy = ExternalIpPolicy::new(vec![IpRange::from(ip)]); let mut system = SystemDescription::new(); system - .service_ip_pool_ranges(service_ip_pool_ranges.clone()) + .external_ip_policy(external_ip_policy.clone()) .sled(SledBuilder::new().id(sled.id())) .expect("failed to add sled"); @@ -2531,7 +2538,9 @@ mod test { RackInit { rack_id: rack_id(), blueprint: blueprint.clone(), - service_ip_pool_ranges, + service_ip_pool_ranges: external_ip_policy + .service_ip_pool_ranges() + .to_vec(), ..Default::default() }, ) diff --git a/nexus/reconfigurator/cli-integration-tests/tests/integration/blueprint_edit.rs b/nexus/reconfigurator/cli-integration-tests/tests/integration/blueprint_edit.rs index b905818652d..a938930603a 100644 --- a/nexus/reconfigurator/cli-integration-tests/tests/integration/blueprint_edit.rs +++ b/nexus/reconfigurator/cli-integration-tests/tests/integration/blueprint_edit.rs @@ -127,7 +127,13 @@ async fn test_blueprint_edit(cptestctx: &ControlPlaneTestContext) { .planning_input .sled_lookup(SledFilter::Commissioned, sled_id) .expect("state1 has initial sled"); - assert!(!state1.planning_input.service_ip_pool_ranges().is_empty()); + assert!( + !state1 + .planning_input + .external_ip_policy() + .service_ip_pool_ranges() + .is_empty() + ); assert!(!state1.silo_names.is_empty()); assert!(!state1.external_dns_zone_names.is_empty()); // We waited for the first inventory collection already. diff --git a/nexus/reconfigurator/execution/src/dns.rs b/nexus/reconfigurator/execution/src/dns.rs index 69bb198ff3d..f5426ee193f 100644 --- a/nexus/reconfigurator/execution/src/dns.rs +++ b/nexus/reconfigurator/execution/src/dns.rs @@ -1568,7 +1568,7 @@ mod test { // We'll need another (fake) external IP for this new Nexus. builder .policy_mut() - .service_ip_pool_ranges + .external_ips .push(IpRange::from(IpAddr::V4(Ipv4Addr::LOCALHOST))); builder.build() diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index 6d9c36cbdf6..bae7ca302b2 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -751,7 +751,7 @@ impl<'a> BlueprintBuilder<'a> { let allocator = BlueprintResourceAllocator::new( self.sled_editors.values(), - self.input.service_ip_pool_ranges().to_vec(), + self.input.external_ip_policy(), )?; Ok::<_, Error>(allocator) @@ -2803,6 +2803,7 @@ pub mod test { use nexus_reconfigurator_blippy::BlippyReportSortKey; use nexus_types::deployment::BlueprintArtifactVersion; use nexus_types::deployment::BlueprintDatasetDisposition; + use nexus_types::deployment::ExternalIpPolicy; use nexus_types::deployment::OmicronZoneNetworkResources; use nexus_types::external_api::views::SledPolicy; use omicron_common::address::IpRange; @@ -3541,7 +3542,8 @@ pub mod test { assert!(!used_ip_ranges.is_empty()); let input = { let mut builder = input.into_builder(); - builder.policy_mut().service_ip_pool_ranges = used_ip_ranges; + builder.policy_mut().external_ips = + ExternalIpPolicy::new(used_ip_ranges); builder.build() }; diff --git a/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs b/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs index 5258fa0164a..7dc428825db 100644 --- a/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs +++ b/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs @@ -8,7 +8,7 @@ use std::net::IpAddr; use super::SledEditor; use nexus_types::deployment::BlueprintZoneDisposition; -use omicron_common::address::IpRange; +use nexus_types::deployment::ExternalIpPolicy; mod external_networking; @@ -33,7 +33,7 @@ pub(crate) struct BlueprintResourceAllocator { impl BlueprintResourceAllocator { pub fn new<'a, I>( all_sleds: I, - service_ip_pool_ranges: Vec, + external_ip_policy: &ExternalIpPolicy, ) -> Result where I: Iterator + Clone, @@ -45,7 +45,7 @@ impl BlueprintResourceAllocator { all_sleds.flat_map(|editor| { editor.zones(BlueprintZoneDisposition::is_expunged) }), - service_ip_pool_ranges, + external_ip_policy.service_ip_pool_ranges().to_vec(), ) .map_err(BlueprintResourceAllocatorInputError::ExternalNetworking)?; diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index df04c8e1fc3..cb1daef46e5 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -3172,9 +3172,12 @@ pub(crate) mod test { // the service IP pool. This will force reuse of the IP that was // allocated to the expunged Nexus zone. let mut builder = input.into_builder(); - assert_eq!(builder.policy_mut().service_ip_pool_ranges.len(), 1); + assert_eq!( + builder.policy_mut().external_ips.service_ip_pool_ranges().len(), + 1 + ); builder.policy_mut().target_nexus_zone_count = - builder.policy_mut().service_ip_pool_ranges[0] + builder.policy_mut().external_ips.service_ip_pool_ranges()[0] .len() .try_into() .unwrap(); diff --git a/nexus/reconfigurator/planning/src/system.rs b/nexus/reconfigurator/planning/src/system.rs index 16ba4266ef0..0e0398f6007 100644 --- a/nexus/reconfigurator/planning/src/system.rs +++ b/nexus/reconfigurator/planning/src/system.rs @@ -33,6 +33,7 @@ use nexus_types::deployment::ClickhousePolicy; use nexus_types::deployment::CockroachDbClusterVersion; use nexus_types::deployment::CockroachDbSettings; use nexus_types::deployment::ExpectedVersion; +use nexus_types::deployment::ExternalIpPolicy; use nexus_types::deployment::OximeterReadPolicy; use nexus_types::deployment::PlannerConfig; use nexus_types::deployment::PlanningInputBuilder; @@ -117,7 +118,7 @@ pub struct SystemDescription { target_cockroachdb_zone_count: usize, target_cockroachdb_cluster_version: CockroachDbClusterVersion, target_crucible_pantry_zone_count: usize, - service_ip_pool_ranges: Vec, + external_ip_policy: ExternalIpPolicy, internal_dns_version: Generation, external_dns_version: Generation, clickhouse_policy: Option, @@ -181,13 +182,13 @@ impl SystemDescription { CockroachDbClusterVersion::POLICY; // IPs from TEST-NET-1 (RFC 5737) - let service_ip_pool_ranges = vec![ + let external_ip_policy = ExternalIpPolicy::new(vec![ IpRange::try_from(( "192.0.2.2".parse::().unwrap(), "192.0.2.20".parse::().unwrap(), )) .unwrap(), - ]; + ]); SystemDescription { sleds: IndexMap::new(), @@ -202,7 +203,7 @@ impl SystemDescription { target_cockroachdb_zone_count, target_cockroachdb_cluster_version, target_crucible_pantry_zone_count, - service_ip_pool_ranges, + external_ip_policy, internal_dns_version: Generation::new(), external_dns_version: Generation::new(), clickhouse_policy: None, @@ -302,11 +303,11 @@ impl SystemDescription { self.target_internal_dns_zone_count } - pub fn service_ip_pool_ranges( + pub fn external_ip_policy( &mut self, - ranges: Vec, + policy: ExternalIpPolicy, ) -> &mut Self { - self.service_ip_pool_ranges = ranges; + self.external_ip_policy = policy; self } @@ -1075,7 +1076,7 @@ impl SystemDescription { &self, ) -> anyhow::Result { let policy = Policy { - service_ip_pool_ranges: self.service_ip_pool_ranges.clone(), + external_ips: self.external_ip_policy.clone(), target_boundary_ntp_zone_count: self.target_boundary_ntp_zone_count, target_nexus_zone_count: self.target_nexus_zone_count, target_internal_dns_zone_count: self.target_internal_dns_zone_count, diff --git a/nexus/reconfigurator/preparation/src/lib.rs b/nexus/reconfigurator/preparation/src/lib.rs index 81b96e9c952..d36c3fca69c 100644 --- a/nexus/reconfigurator/preparation/src/lib.rs +++ b/nexus/reconfigurator/preparation/src/lib.rs @@ -20,6 +20,7 @@ use nexus_types::deployment::BlueprintMetadata; use nexus_types::deployment::ClickhousePolicy; use nexus_types::deployment::CockroachDbClusterVersion; use nexus_types::deployment::CockroachDbSettings; +use nexus_types::deployment::ExternalIpPolicy; use nexus_types::deployment::OmicronZoneExternalIp; use nexus_types::deployment::OmicronZoneNic; use nexus_types::deployment::OximeterReadPolicy; @@ -268,8 +269,9 @@ impl PlanningInputFromDb<'_> { }) }) .collect::, _>>()?; + let external_ips = ExternalIpPolicy::new(service_ip_pool_ranges); let policy = Policy { - service_ip_pool_ranges, + external_ips, target_boundary_ntp_zone_count: self.target_boundary_ntp_zone_count, target_nexus_zone_count: self.target_nexus_zone_count, target_internal_dns_zone_count: self.target_internal_dns_zone_count, diff --git a/nexus/reconfigurator/simulation/src/system.rs b/nexus/reconfigurator/simulation/src/system.rs index 7318e66d86b..448cf5aa3d2 100644 --- a/nexus/reconfigurator/simulation/src/system.rs +++ b/nexus/reconfigurator/simulation/src/system.rs @@ -14,14 +14,13 @@ use nexus_reconfigurator_planning::{ }; use nexus_types::{ deployment::{ - Blueprint, BlueprintTarget, SledFilter, UnstableReconfiguratorState, + Blueprint, BlueprintTarget, ExternalIpPolicy, SledFilter, + UnstableReconfiguratorState, }, internal_api::params::{DnsConfigParams, DnsConfigZone}, inventory::{CabooseWhich, Collection}, }; -use omicron_common::{ - address::IpRange, api::external::Generation, disk::M2Slot, -}; +use omicron_common::{api::external::Generation, disk::M2Slot}; use omicron_uuid_kinds::{BlueprintUuid, CollectionUuid, SledUuid}; use strum::IntoEnumIterator as _; @@ -599,8 +598,8 @@ pub struct LoadSerializedSystemResult { /// The blueprint IDs successfully loaded. pub blueprint_ids: Vec, - /// The service IP pool ranges. - pub service_ip_pool_ranges: Vec, + /// The external IP policy. + pub external_ip_policy: ExternalIpPolicy, /// Internal DNS generations. pub internal_dns_generations: Vec, @@ -617,7 +616,7 @@ impl LoadSerializedSystemResult { sled_ids: Vec::new(), collection_ids: Vec::new(), blueprint_ids: Vec::new(), - service_ip_pool_ranges: Vec::new(), + external_ip_policy: ExternalIpPolicy::empty(), internal_dns_generations: Vec::new(), external_dns_generations: Vec::new(), } @@ -643,10 +642,10 @@ impl fmt::Display for LoadSerializedSystemResult { join_comma_or_none(&self.blueprint_ids) )?; writeln!( - // TODO: output format for IP ranges that's not just Debug? + // TODO: output format for IP policy that's not just Debug? f, - "loaded service IP pool ranges: {:?}", - self.service_ip_pool_ranges, + "loaded external IP policy: {:?}", + self.external_ip_policy, )?; writeln!( f, @@ -882,11 +881,11 @@ impl SimSystemBuilderInner { } } - self.system.description.service_ip_pool_ranges( - state.planning_input.service_ip_pool_ranges().to_vec(), + self.system.description.external_ip_policy( + state.planning_input.external_ip_policy().clone(), ); - system_res.service_ip_pool_ranges = - state.planning_input.service_ip_pool_ranges().to_vec(); + system_res.external_ip_policy = + state.planning_input.external_ip_policy().clone(); self.set_internal_dns(state.internal_dns); self.set_external_dns(state.external_dns); diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index 11b09f7e5d3..52a9d92c3ff 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -102,6 +102,8 @@ pub use planning_input::CockroachDbClusterVersion; pub use planning_input::CockroachDbPreserveDowngrade; pub use planning_input::CockroachDbSettings; pub use planning_input::DiskFilter; +pub use planning_input::ExternalIpPolicy; +pub use planning_input::ExternalIpPolicyError; pub use planning_input::OximeterReadMode; pub use planning_input::OximeterReadPolicy; pub use planning_input::PlanningInput; diff --git a/nexus/types/src/deployment/planning_input.rs b/nexus/types/src/deployment/planning_input.rs index e216b60cf61..977b3331339 100644 --- a/nexus/types/src/deployment/planning_input.rs +++ b/nexus/types/src/deployment/planning_input.rs @@ -45,6 +45,7 @@ use std::collections::BTreeSet; use std::collections::btree_map::Entry; use std::error; use std::fmt; +use std::net::IpAddr; use strum::Display; use strum::IntoEnumIterator; @@ -223,8 +224,8 @@ impl PlanningInput { &self.policy.planner_config } - pub fn service_ip_pool_ranges(&self) -> &[IpRange] { - &self.policy.service_ip_pool_ranges + pub fn external_ip_policy(&self) -> &ExternalIpPolicy { + &self.policy.external_ips } pub fn clickhouse_cluster_enabled(&self) -> bool { @@ -974,9 +975,9 @@ impl SledState { /// sled additionally has non-policy [`SledResources`] needed for planning. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Policy { - /// ranges specified by the IP pool for externally-visible control plane + /// description of IP addresses for externally-visible control plane /// services (e.g., external DNS, Nexus, boundary NTP) - pub service_ip_pool_ranges: Vec, + pub external_ips: ExternalIpPolicy, /// desired total number of deployed Boundary NTP zones pub target_boundary_ntp_zone_count: usize, @@ -1043,6 +1044,98 @@ pub struct Policy { pub planner_config: PlannerConfig, } +/// Subset of [`Policy`] specific to external IP addresses. +// NOTE: The fields of the struct are private and we manually implement +// `Deserialize` to maintain invariants (e.g., that every `external_dns_ip` is +// contained in one of the `service_ip_pool_ranges`). +#[derive(Debug, Clone, Serialize)] +pub struct ExternalIpPolicy { + /// ranges specified by the IP pool for externally-visible control plane + /// services (e.g., external DNS, Nexus, boundary NTP) + service_ip_pool_ranges: Vec, + + /// specific IP addresses contained in `service_ip_pool_ranges` that are + /// earmarked for external DNS + external_dns_ips: BTreeSet, +} + +impl<'de> Deserialize<'de> for ExternalIpPolicy { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + use serde::de::Error; + + // The fields of `ExternalIpPolicyShadow` should exactly match the + // fields of `ExternalIpPolicy`. We're not really using serde's remote + // derive, but by adding the attribute we get compile-time checking that + // all the field names and types match. (It doesn't check the _order_, + // but that should be fine as long as we're using JSON or similar + // formats.) + #[derive(Deserialize)] + #[serde(remote = "ExternalIpPolicy")] + struct ExternalIpPolicyShadow { + service_ip_pool_ranges: Vec, + external_dns_ips: BTreeSet, + } + + // Deserialize without validation... + let ExternalIpPolicy { service_ip_pool_ranges, external_dns_ips } = + ExternalIpPolicyShadow::deserialize(deserializer)?; + + // ...then validate. + let mut policy = ExternalIpPolicy::new(service_ip_pool_ranges); + for ip in external_dns_ips { + policy.set_ip_for_external_dns(ip).map_err(D::Error::custom)?; + } + + Ok(policy) + } +} + +impl ExternalIpPolicy { + pub fn empty() -> Self { + Self { + service_ip_pool_ranges: Vec::new(), + external_dns_ips: BTreeSet::new(), + } + } + + // TODO-john comment + pub fn new(service_ip_pool_ranges: Vec) -> Self { + Self { service_ip_pool_ranges, external_dns_ips: BTreeSet::new() } + } + + // TODO-john comment + pub fn set_ip_for_external_dns( + &mut self, + ip: IpAddr, + ) -> Result<(), ExternalIpPolicyError> { + if self.service_ip_pool_ranges.iter().any(|pool| pool.contains(ip)) { + self.external_dns_ips.insert(ip); + Ok(()) + } else { + Err(ExternalIpPolicyError::ExternalDnsOutsideServiceIpPools(ip)) + } + } + + // TODO-john comment / remove / rename? + pub fn push(&mut self, pool_range: IpRange) { + self.service_ip_pool_ranges.push(pool_range); + } + + // TODO-john comment / remove? + pub fn service_ip_pool_ranges(&self) -> &[IpRange] { + &self.service_ip_pool_ranges + } +} + +#[derive(Debug, thiserror::Error)] +pub enum ExternalIpPolicyError { + #[error("external DNS IP {0} is not a member of any service IP pool")] + ExternalDnsOutsideServiceIpPools(IpAddr), +} + #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] pub struct OximeterReadPolicy { // We set the version as `u32` instead of `Generation` because we later need @@ -1297,7 +1390,7 @@ impl PlanningInputBuilder { // This empty input is known to be valid. PlanningInput { policy: Policy { - service_ip_pool_ranges: Vec::new(), + external_ips: ExternalIpPolicy::empty(), target_boundary_ntp_zone_count: 0, target_nexus_zone_count: 0, target_internal_dns_zone_count: 0, From 2dceb1c10475ce92018d7de1da0743d61c8e4cd7 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 22 Oct 2025 17:27:25 -0400 Subject: [PATCH 03/16] read external DNS IPs from policy instead of the parent blueprint --- .../tests/output/cmds-stdout | 2 +- .../tests/output/cmds-target-release-stdout | 2 +- nexus/db-queries/src/db/datastore/rack.rs | 8 +- .../planning/src/blueprint_builder/builder.rs | 14 -- .../src/blueprint_editor/allocators.rs | 25 +-- .../allocators/external_networking.rs | 185 ++++++------------ nexus/reconfigurator/planning/src/example.rs | 48 +++-- nexus/reconfigurator/planning/src/planner.rs | 41 ++-- nexus/reconfigurator/planning/src/system.rs | 10 +- nexus/reconfigurator/simulation/src/system.rs | 2 +- nexus/types/src/deployment/planning_input.rs | 4 + 11 files changed, 140 insertions(+), 201 deletions(-) diff --git a/dev-tools/reconfigurator-cli/tests/output/cmds-stdout b/dev-tools/reconfigurator-cli/tests/output/cmds-stdout index d928df99716..c69ad975e58 100644 --- a/dev-tools/reconfigurator-cli/tests/output/cmds-stdout +++ b/dev-tools/reconfigurator-cli/tests/output/cmds-stdout @@ -871,7 +871,7 @@ result: loaded sleds: 04ef3330-c682-4a08-8def-fcc4bef31bcd, 90c1102a-b9f5-4d88-92a2-60d54a2d98cc, dde1c0e2-b10d-4621-b420-f179f7a7a00a loaded collections: 6e066695-94bc-4250-bd63-fd799c166cc1 loaded blueprints: (none) - loaded service IP pool ranges: [V4(Ipv4Range { first: 192.0.2.2, last: 192.0.2.20 })] + loaded external IP policy: ExternalIpPolicy { service_ip_pool_ranges: [V4(Ipv4Range { first: 192.0.2.2, last: 192.0.2.20 })], external_dns_ips: {} } loaded internal DNS generations: (none) loaded external DNS generations: (none) config: diff --git a/dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout b/dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout index 3ed126b11d4..621bf8c2bcd 100644 --- a/dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout +++ b/dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout @@ -1038,7 +1038,7 @@ result: loaded sleds: 2b8f0cb3-0295-4b3c-bc58-4fe88b57112c, 98e6b7c2-2efa-41ca-b20a-0a4d61102fe6, d81c6a84-79b8-4958-ae41-ea46c9b19763 loaded collections: f45ba181-4b56-42cc-a762-874d90184a43, eb0796d5-ab8a-4f7b-a884-b4aeacb8ab51, 61f451b3-2121-4ed6-91c7-a550054f6c21 loaded blueprints: 184f10b3-61cb-41ef-9b93-3489b2bac559, dbcbd3d6-41ff-48ae-ac0b-1becc9b2fd21, 8da82a8e-bf97-4fbd-8ddd-9f6462732cf1, 58d5e830-0884-47d8-a7cd-b2b3751adeb4 - loaded service IP pool ranges: [V4(Ipv4Range { first: 192.0.2.2, last: 192.0.2.20 })] + loaded external IP policy: ExternalIpPolicy { service_ip_pool_ranges: [V4(Ipv4Range { first: 192.0.2.2, last: 192.0.2.20 }), V4(Ipv4Range { first: 198.51.100.1, last: 198.51.100.30 })], external_dns_ips: {198.51.100.1, 198.51.100.2, 198.51.100.3} } loaded internal DNS generations: (none) loaded external DNS generations: (none) config: diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index 34d4fe9a867..e4080b142b3 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -1422,7 +1422,7 @@ mod test { let mut system = SystemDescription::new(); system - .external_ip_policy(external_ip_policy.clone()) + .set_external_ip_policy(external_ip_policy.clone()) .sled(SledBuilder::new().id(sled1.id())) .expect("failed to add sled1") .sled(SledBuilder::new().id(sled2.id())) @@ -1774,7 +1774,7 @@ mod test { let mut system = SystemDescription::new(); system - .external_ip_policy(external_ip_policy.clone()) + .set_external_ip_policy(external_ip_policy.clone()) .sled(SledBuilder::new().id(sled.id())) .expect("failed to add sled"); @@ -2069,7 +2069,7 @@ mod test { let mut system = SystemDescription::new(); system - .external_ip_policy(external_ip_policy.clone()) + .set_external_ip_policy(external_ip_policy.clone()) .sled(SledBuilder::new().id(sled.id())) .expect("failed to add sled"); @@ -2421,7 +2421,7 @@ mod test { let mut system = SystemDescription::new(); system - .external_ip_policy(external_ip_policy.clone()) + .set_external_ip_policy(external_ip_policy.clone()) .sled(SledBuilder::new().id(sled.id())) .expect("failed to add sled"); diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index bae7ca302b2..c472dd02657 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -2273,20 +2273,6 @@ impl<'a> BlueprintBuilder<'a> { }); } - /// Allow a test to manually add an external DNS address, which could - /// ordinarily only come from RSS. - /// - /// TODO-cleanup: Remove when external DNS addresses are in the policy. - // This can't be `#[cfg(test)]` because it's used by the `ExampleSystem` - // helper (which itself is used by reconfigurator-cli and friends). We give - // it a scary name instead. - pub(crate) fn inject_untracked_external_dns_ip( - &mut self, - addr: IpAddr, - ) -> Result<(), Error> { - Ok(self.resource_allocator()?.inject_untracked_external_dns_ip(addr)?) - } - pub fn pending_mgs_updates_replace_all( &mut self, updates: nexus_types::deployment::PendingMgsUpdates, diff --git a/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs b/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs index 7dc428825db..c4246fd2908 100644 --- a/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs +++ b/nexus/reconfigurator/planning/src/blueprint_editor/allocators.rs @@ -4,8 +4,6 @@ //! Blueprint planner resource allocation -use std::net::IpAddr; - use super::SledEditor; use nexus_types::deployment::BlueprintZoneDisposition; use nexus_types::deployment::ExternalIpPolicy; @@ -36,16 +34,13 @@ impl BlueprintResourceAllocator { external_ip_policy: &ExternalIpPolicy, ) -> Result where - I: Iterator + Clone, + I: Iterator, { let external_networking = ExternalNetworkingAllocator::new( - all_sleds.clone().flat_map(|editor| { - editor.zones(BlueprintZoneDisposition::is_in_service) - }), all_sleds.flat_map(|editor| { - editor.zones(BlueprintZoneDisposition::is_expunged) + editor.zones(BlueprintZoneDisposition::is_in_service) }), - external_ip_policy.service_ip_pool_ranges().to_vec(), + external_ip_policy, ) .map_err(BlueprintResourceAllocatorInputError::ExternalNetworking)?; @@ -69,18 +64,4 @@ impl BlueprintResourceAllocator { ) -> Result { self.external_networking.for_new_boundary_ntp() } - - /// Allow a test to manually add an external DNS address, which could - /// ordinarily only come from RSS. - /// - /// TODO-cleanup: Remove when external DNS addresses are in the policy. - // This can't be `#[cfg(test)]` because it's used by the `ExampleSystem` - // helper (which itself is used by reconfigurator-cli and friends). We give - // it a scary name instead. - pub(crate) fn inject_untracked_external_dns_ip( - &mut self, - ip: IpAddr, - ) -> Result<(), ExternalNetworkingError> { - self.external_networking.add_external_dns_ip(ip) - } } diff --git a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs b/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs index a1d5339cc92..51123729478 100644 --- a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs +++ b/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs @@ -8,11 +8,11 @@ use nexus_config::NUM_INITIAL_RESERVED_IP_ADDRESSES; use nexus_sled_agent_shared::inventory::ZoneKind; use nexus_types::deployment::BlueprintZoneConfig; use nexus_types::deployment::BlueprintZoneType; +use nexus_types::deployment::ExternalIpPolicy; use nexus_types::deployment::OmicronZoneExternalIp; use nexus_types::inventory::SourceNatConfig; use omicron_common::address::DNS_OPTE_IPV4_SUBNET; use omicron_common::address::DNS_OPTE_IPV6_SUBNET; -use omicron_common::address::IpRange; use omicron_common::address::NEXUS_OPTE_IPV4_SUBNET; use omicron_common::address::NEXUS_OPTE_IPV6_SUBNET; use omicron_common::address::NTP_OPTE_IPV4_SUBNET; @@ -59,7 +59,7 @@ pub(super) struct ExternalNetworkingAllocator { // see https://github.com/oxidecomputer/omicron/issues/3732 available_external_dns_ips: BTreeSet, - // Allocator for external IPs for service zones + // Allocator for external IPs for Nexus and Boundary NTP zones. external_ip_alloc: ExternalIpAllocator, // Iterator of available MAC addresses in the system address range @@ -69,8 +69,7 @@ pub(super) struct ExternalNetworkingAllocator { impl ExternalNetworkingAllocator { pub(super) fn new<'b>( running_omicron_zones: impl Iterator, - expunged_omicron_zones: impl Iterator, - service_ip_pool_ranges: Vec, + external_ip_policy: &ExternalIpPolicy, ) -> anyhow::Result { // Scan through the running zones and build several sets of "used // resources". When adding new control plane zones to a sled, we may @@ -112,9 +111,9 @@ impl ExternalNetworkingAllocator { let mut existing_external_dns_v6_ips: HashSet = HashSet::new(); let mut external_ip_alloc = - ExternalIpAllocator::new(service_ip_pool_ranges); + ExternalIpAllocator::new(external_ip_policy); let mut used_macs: HashSet = HashSet::new(); - let mut used_external_dns_ips: HashSet = HashSet::new(); + let mut used_external_dns_ips: BTreeSet = BTreeSet::new(); for z in running_omicron_zones { let zone_type = &z.zone_type; @@ -181,36 +180,35 @@ impl ExternalNetworkingAllocator { } } - // Recycle the IP addresses of expunged external DNS zones, - // ensuring that those addresses aren't currently in use. - // TODO: Remove when external DNS addresses come from policy. - let mut available_external_dns_ips = BTreeSet::new(); - for zone in expunged_omicron_zones { - if let BlueprintZoneType::ExternalDns(dns) = &zone.zone_type { - let ip = dns.dns_address.addr.ip(); - if !used_external_dns_ips.contains(&ip) { - available_external_dns_ips.insert(ip); - - // We also need to partition the external DNS IPs off from - // the set of standard service external IPs. For any - // available external DNS IP, mark it as used as far as the - // normal IP allocator is concerned. - let omicron_ip = OmicronZoneExternalIp::Floating( - dns.dns_address.into_ip(), - ); - match external_ip_alloc.mark_ip_used(&omicron_ip) { - // Because we're checking _expunged_ zones, we might - // have duplicate IPs. Ignore those; that means we've - // already marked this IP as used, which is all we need. - Ok(()) - | Err(ExternalIpAllocatorError::DuplicateExternalIp( - _, - )) => (), - Err(err) => return Err(err.into()), - } - } - } + // External DNS IPs are special: + // + // 1. Check that we don't have any in-use external DNS IPs that we don't + // expect. Tere should be no way for this to happen at the time of + // this writing, because there's no way to change the set of external + // DNS IPs after RSS. Once we add a way to change it, presumably our + // caller should handle this somehow (e.g., the planner should + // expunge any external DNS zones on IPs that are no longer part of + // the policy's set?). + // 2. The set of available IPs is the difference between the policy set + // and the in use set. + let mut unexpected_external_dns_ips = used_external_dns_ips + .difference(external_ip_policy.external_dns_ips()) + .filter(|ip| { + // As above, ignore localhost for the test suite. + !ip.is_loopback() + }) + .peekable(); + if unexpected_external_dns_ips.peek().is_some() { + bail!( + "unexpected in-service external DNS IPs: {:?}", + unexpected_external_dns_ips.collect::>() + ); } + let available_external_dns_ips = external_ip_policy + .external_dns_ips() + .difference(&used_external_dns_ips) + .copied() + .collect::>(); // TODO-performance Building these iterators as "walk through the list // and skip anything we've used already" is fine as long as we're @@ -380,23 +378,6 @@ impl ExternalNetworkingAllocator { nic_mac, }) } - - /// Allow a test to manually add an external DNS address, - /// which could otherwise only be added via RSS. - /// - /// TODO-cleanup: Remove when external DNS addresses are in the policy. - pub(crate) fn add_external_dns_ip( - &mut self, - addr: IpAddr, - ) -> Result<(), ExternalNetworkingError> { - if self.available_external_dns_ips.insert(addr) { - Ok(()) - } else { - return Err(ExternalNetworkingError::AddDuplicateExternalDnsIp { - ip: addr, - }); - } - } } #[derive(Debug, Clone, Copy)] @@ -470,9 +451,13 @@ enum ExternalIpAllocatorError { } impl ExternalIpAllocator { - fn new(service_pool_ranges: Vec) -> Self { - let service_ip_pool_ips = - service_pool_ranges.into_iter().flat_map(|r| r.iter()); + fn new(policy: &ExternalIpPolicy) -> Self { + let service_ip_pool_ranges = policy.service_ip_pool_ranges().to_vec(); + let external_dns_ips = policy.external_dns_ips().clone(); + let service_ip_pool_ips = service_ip_pool_ranges + .into_iter() + .flat_map(|r| r.iter()) + .filter(move |ip| !external_dns_ips.contains(ip)); Self { service_ip_pool_ips: DebugIgnore(Box::new(service_ip_pool_ips)), used_exclusive_ips: BTreeSet::new(), @@ -671,8 +656,6 @@ impl TryFrom<(u16, u16)> for SnatPortRange { #[cfg(test)] pub mod test { - use std::net::SocketAddr; - use super::*; use illumos_utils::zpool::ZpoolName; use nexus_sled_agent_shared::inventory::OmicronZoneDataset; @@ -684,13 +667,14 @@ pub mod test { use nexus_types::deployment::blueprint_zone_type; use nexus_types::inventory::NetworkInterface; use nexus_types::inventory::NetworkInterfaceKind; - use omicron_common::api::external::Generation; + use omicron_common::address::IpRange; use omicron_common::api::external::Vni; use omicron_uuid_kinds::ExternalIpUuid; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::OmicronZoneUuid; use omicron_uuid_kinds::ZpoolUuid; use slog_error_chain::InlineErrorChain; + use std::net::SocketAddr; use test_strategy::proptest; use uuid::Uuid; @@ -772,7 +756,8 @@ pub mod test { }; // Build up the allocator and mark all used IPs. - let mut allocator = ExternalIpAllocator::new(ip_pool_ranges); + let mut allocator = + ExternalIpAllocator::new(&ExternalIpPolicy::new(ip_pool_ranges)); for &ip in &used_exclusive { allocator .mark_ip_used(&as_floating(ip)) @@ -872,7 +857,8 @@ pub mod test { #[test] fn external_dns_ips_are_partitioned_separately() { - // Construct a service IP range with 3 IPs. + // Construct a service IP range with 3 IPs. The first two are for + // external DNS, and the third is for other services. let service_ip_pool = IpRange::try_from(( "192.0.2.1".parse::().unwrap(), "192.0.2.3".parse::().unwrap(), @@ -880,6 +866,17 @@ pub mod test { .unwrap(); assert_eq!(service_ip_pool.len(), 3); + let external_ip_policy = { + let mut policy = ExternalIpPolicy::new(vec![service_ip_pool]); + policy + .set_ip_for_external_dns("192.0.2.1".parse().unwrap()) + .unwrap(); + policy + .set_ip_for_external_dns("192.0.2.2".parse().unwrap()) + .unwrap(); + policy + }; + let make_external_dns = |index, disposition| { let id = OmicronZoneUuid::new_v4(); let pool_name = ZpoolName::new_external(ZpoolUuid::new_v4()); @@ -922,27 +919,18 @@ pub mod test { } }; - // Construct a set of inputs with 1 in-service external DNS zone (at IP - // 1) and 1 expunged external DNS zone (at IP 2). That should result in - // IP 2 being set aside for new external DNS zones, and one IP (IP 3) - // being set aside for other services (e.g., Nexus). + // Pass only a single running zone (external DNS at IP *.1). That should + // result in IP *.2 being set aside for a new external DNS zone and IP + // *.3 being set aside for other services (e.g., Nexus). let running_external_dns = make_external_dns(0, BlueprintZoneDisposition::InService); - let expunged_external_dns = make_external_dns( - 1, - BlueprintZoneDisposition::Expunged { - as_of_generation: Generation::new(), - ready_for_cleanup: false, - }, - ); // Construct a builder; ask for external DNS IPs first (we should get IP // 1 then "none available") then Nexus IPs (we should get IP 2 then // "none available"). let mut builder = ExternalNetworkingAllocator::new( [&running_external_dns].iter().copied(), - [&expunged_external_dns].iter().copied(), - vec![service_ip_pool], + &external_ip_policy, ) .expect("constructed builder"); @@ -981,56 +969,7 @@ pub mod test { // partitioned off and do not depend on request ordering. let mut builder = ExternalNetworkingAllocator::new( [&running_external_dns].iter().copied(), - [&expunged_external_dns].iter().copied(), - vec![service_ip_pool], - ) - .expect("constructed builder"); - - // Test Nexus - assert_eq!( - builder.for_new_nexus().expect("got Nexus IP").external_ip, - service_ip_pool.iter().nth(2).unwrap() - ); - let err = builder.for_new_nexus().expect_err("no Nexus IPs left"); - assert!( - matches!( - err, - ExternalNetworkingError::NoExternalServiceIpAvailable - ), - "unexpected error: {}", - InlineErrorChain::new(&err), - ); - - // Text external DNS - assert_eq!( - builder - .for_new_external_dns() - .expect("got external DNS IP") - .external_ip, - service_ip_pool.iter().nth(1).unwrap() - ); - let err = builder.for_new_external_dns().expect_err("no DNS IPs left"); - assert!( - matches!(err, ExternalNetworkingError::NoExternalDnsIpAvailable), - "unexpected error: {}", - InlineErrorChain::new(&err), - ); - - // Repeat the above test, but this time with two expunged DNS zones - // (different IDs, but both assigned the same IP, which is perfectly - // reasonable for expunged zones if they were never in service at the - // same time). - let expunged_external_dns2 = make_external_dns( - 1, - BlueprintZoneDisposition::Expunged { - as_of_generation: Generation::new(), - ready_for_cleanup: false, - }, - ); - let mut builder = ExternalNetworkingAllocator::new( - [&running_external_dns].iter().copied(), - [&expunged_external_dns, &expunged_external_dns2].iter().copied(), - vec![service_ip_pool], + &external_ip_policy, ) .expect("constructed builder"); diff --git a/nexus/reconfigurator/planning/src/example.rs b/nexus/reconfigurator/planning/src/example.rs index 5940e433c27..c0d5ce5cfd9 100644 --- a/nexus/reconfigurator/planning/src/example.rs +++ b/nexus/reconfigurator/planning/src/example.rs @@ -36,6 +36,7 @@ use nexus_types::deployment::SledFilter; use nexus_types::deployment::TargetReleaseDescription; use nexus_types::external_api::views::SledPolicy; use nexus_types::inventory::Collection; +use omicron_common::address::IpRange; use omicron_common::api::external::TufRepoDescription; use omicron_common::policy::CRUCIBLE_PANTRY_REDUNDANCY; use omicron_common::policy::INTERNAL_DNS_REDUNDANCY; @@ -309,7 +310,7 @@ impl ExampleSystemBuilder { /// anywhere between 0 and 30, inclusive, is permitted. (The limit of 30 is /// primarily to simplify the implementation.) /// - /// Each DNS server is assigned an address in the 10.x.x.x range. + /// Each DNS server is assigned an address in the 198.51.100.x range. pub fn external_dns_count( mut self, external_dns_count: usize, @@ -500,9 +501,36 @@ impl ExampleSystemBuilder { .unwrap(); } + // Add as many external IPs as is necessary for external DNS zones. We + // pick addresses in the TEST-NET-2 (RFC 5737) range. + let mut external_ip_policy = system.external_ip_policy().clone(); + if self.external_dns_count.0 > 0 { + external_ip_policy.push( + IpRange::try_from(( + "198.51.100.1".parse::().unwrap(), + "198.51.100.30".parse::().unwrap(), + )) + .unwrap(), + ); + } + for i in 0..self.external_dns_count.0 { + external_ip_policy + .set_ip_for_external_dns(IpAddr::V4(Ipv4Addr::new( + 198, + 51, + 100, + (i + 1) + .try_into() + .expect("external_dns_count is always <= 30"), + ))) + .expect("test IPs are valid service IPs"); + } + system.set_external_ip_policy(external_ip_policy); + let mut input_builder = system .to_planning_input_builder() .expect("failed to make planning input builder"); + let base_input = input_builder.clone().build(); // Start with an empty blueprint containing only our sleds, no zones. @@ -529,24 +557,6 @@ impl ExampleSystemBuilder { ) .unwrap(); - // Add as many external IPs as is necessary for external DNS zones. We - // pick addresses in the TEST-NET-2 (RFC 5737) range. - for i in 0..self.external_dns_count.0 { - builder - .inject_untracked_external_dns_ip(IpAddr::V4(Ipv4Addr::new( - 198, - 51, - 100, - (i + 1) - .try_into() - .expect("external_dns_count is always <= 30"), - ))) - .expect( - "this shouldn't error because provided external IPs \ - are all unique", - ); - } - let discretionary_sled_count = base_input.all_sled_ids(SledFilter::Discretionary).count(); diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index cb1daef46e5..c626dbee536 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -2482,6 +2482,7 @@ pub(crate) mod test { use nexus_types::inventory::CockroachStatus; use nexus_types::inventory::InternalDnsGenerationStatus; use nexus_types::inventory::TimeSync; + use omicron_common::address::IpRange; use omicron_common::api::external::Generation; use omicron_common::api::external::MacAddr; use omicron_common::api::external::TufArtifactMeta; @@ -3250,6 +3251,7 @@ pub(crate) mod test { // We should not be able to add any external DNS zones yet, // because we haven't give it any addresses (which currently // come only from RSS). This is not an error, though. + assert!(input.external_ip_policy().external_dns_ips().is_empty()); let mut builder = BlueprintBuilder::new_based_on( &logctx.log, &blueprint1, @@ -3267,8 +3269,31 @@ pub(crate) mod test { ) .expect_err("can't add external DNS zones"); - // Build a builder for a modfied blueprint that will include - // some external DNS addresses. + // Change the policy: add some external DNS IPs. + let external_dns_ips = + ["10.0.0.1", "10.0.0.2", "10.0.0.3"].map(|addr| { + addr.parse::() + .expect("can't parse external DNS IP address") + }); + let input = { + let mut builder = input.into_builder(); + // Add a "service IP pool" covering our external DNS IP range. + builder.policy_mut().external_ips.push( + IpRange::try_from((external_dns_ips[0], external_dns_ips[2])) + .unwrap(), + ); + // Set these IPs as "for external DNS". + for ip in external_dns_ips { + builder + .policy_mut() + .external_ips + .set_ip_for_external_dns(ip) + .unwrap(); + } + builder.build() + }; + + // Build a builder for a modfied blueprint based on the new policy. let mut blueprint_builder = BlueprintBuilder::new_based_on( &logctx.log, &blueprint1, @@ -3279,18 +3304,6 @@ pub(crate) mod test { ) .expect("failed to build blueprint builder"); - // Manually reach into the external networking allocator and "find" - // some external IP addresses (maybe they fell off a truck). - // TODO-cleanup: Remove when external DNS addresses are in the policy. - let external_dns_ips = - ["10.0.0.1", "10.0.0.2", "10.0.0.3"].map(|addr| { - addr.parse::() - .expect("can't parse external DNS IP address") - }); - for addr in external_dns_ips { - blueprint_builder.inject_untracked_external_dns_ip(addr).unwrap(); - } - // Now we can add external DNS zones. We'll add two to the first // sled and one to the second. let (sled_1, sled_2) = { diff --git a/nexus/reconfigurator/planning/src/system.rs b/nexus/reconfigurator/planning/src/system.rs index 0e0398f6007..4a095740fb5 100644 --- a/nexus/reconfigurator/planning/src/system.rs +++ b/nexus/reconfigurator/planning/src/system.rs @@ -181,7 +181,9 @@ impl SystemDescription { let target_cockroachdb_cluster_version = CockroachDbClusterVersion::POLICY; - // IPs from TEST-NET-1 (RFC 5737) + // Nexus / Boundary NTPs IPs from TEST-NET-1 (RFC 5737). + // + // This policy doesn't configure any external DNS IPs. let external_ip_policy = ExternalIpPolicy::new(vec![ IpRange::try_from(( "192.0.2.2".parse::().unwrap(), @@ -303,7 +305,11 @@ impl SystemDescription { self.target_internal_dns_zone_count } - pub fn external_ip_policy( + pub fn external_ip_policy(&self) -> &ExternalIpPolicy { + &self.external_ip_policy + } + + pub fn set_external_ip_policy( &mut self, policy: ExternalIpPolicy, ) -> &mut Self { diff --git a/nexus/reconfigurator/simulation/src/system.rs b/nexus/reconfigurator/simulation/src/system.rs index 448cf5aa3d2..72954971c07 100644 --- a/nexus/reconfigurator/simulation/src/system.rs +++ b/nexus/reconfigurator/simulation/src/system.rs @@ -881,7 +881,7 @@ impl SimSystemBuilderInner { } } - self.system.description.external_ip_policy( + self.system.description.set_external_ip_policy( state.planning_input.external_ip_policy().clone(), ); system_res.external_ip_policy = diff --git a/nexus/types/src/deployment/planning_input.rs b/nexus/types/src/deployment/planning_input.rs index 977b3331339..16ec0ca703c 100644 --- a/nexus/types/src/deployment/planning_input.rs +++ b/nexus/types/src/deployment/planning_input.rs @@ -1128,6 +1128,10 @@ impl ExternalIpPolicy { pub fn service_ip_pool_ranges(&self) -> &[IpRange] { &self.service_ip_pool_ranges } + + pub fn external_dns_ips(&self) -> &BTreeSet { + &self.external_dns_ips + } } #[derive(Debug, thiserror::Error)] From 2ba1048f23b8db2c7616c647938621569f7e255e Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 24 Oct 2025 11:02:07 -0400 Subject: [PATCH 04/16] add IpRange::overlaps() --- common/src/address.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/common/src/address.rs b/common/src/address.rs index 0a5d63ff5ba..bba7548fe59 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -524,6 +524,17 @@ impl IpRange { IpRange::V6(_) => IpVersion::V6, } } + + /// Returns `true` if `self` has any IPs in common with `other`; false + /// otherwise. + pub fn overlaps(&self, other: &IpRange) -> bool { + match (self, other) { + (IpRange::V4(r0), IpRange::V4(r1)) => r0.overlaps(r1), + (IpRange::V6(r0), IpRange::V6(r1)) => r0.overlaps(r1), + (IpRange::V4(_), IpRange::V6(_)) + | (IpRange::V6(_), IpRange::V4(_)) => false, + } + } } impl From for IpRange { @@ -628,6 +639,16 @@ impl Ipv4Range { let end_num = u32::from(self.last); end_num - start_num + 1 } + + /// Returns `true` if `self` has any IPs in common with `other`; false + /// otherwise. + pub fn overlaps(&self, other: &Ipv4Range) -> bool { + // We're disjoint if we either end before other or begin after it; any + // other combination means we have some IP(s) in common. + let is_disjoint = self.last_address() < other.first_address() + || self.first_address() > other.last_address(); + !is_disjoint + } } impl From for Ipv4Range { @@ -701,6 +722,16 @@ impl Ipv6Range { let end_num = u128::from(self.last); end_num - start_num + 1 } + + /// Returns `true` if `self` has any IPs in common with `other`; false + /// otherwise. + pub fn overlaps(&self, other: &Ipv6Range) -> bool { + // We're disjoint if we either end before other or begin after it; any + // other combination means we have some IP(s) in common. + let is_disjoint = self.last_address() < other.first_address() + || self.first_address() > other.last_address(); + !is_disjoint + } } impl From for Ipv6Range { From 8e6e014f3f63dcdc985c332dd784d63f64513fa1 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Fri, 24 Oct 2025 17:58:43 -0400 Subject: [PATCH 05/16] add DataStore::policy_external_dns_ips() --- .../deployment/external_networking.rs | 200 +++++++++++++++++- 1 file changed, 198 insertions(+), 2 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment/external_networking.rs b/nexus/db-queries/src/db/datastore/deployment/external_networking.rs index 2c979954a5f..f0c80082293 100644 --- a/nexus/db-queries/src/db/datastore/deployment/external_networking.rs +++ b/nexus/db-queries/src/db/datastore/deployment/external_networking.rs @@ -16,6 +16,8 @@ use nexus_db_model::IpConfig; use nexus_db_model::IpPool; use nexus_sled_agent_shared::inventory::ZoneKind; use nexus_types::deployment::BlueprintZoneConfig; +use nexus_types::deployment::BlueprintZoneDisposition; +use nexus_types::deployment::BlueprintZoneType; use nexus_types::deployment::OmicronZoneExternalIp; use omicron_common::api::external::Error; use omicron_common::api::external::IdentityMetadataCreateParams; @@ -30,8 +32,54 @@ use slog::error; use slog::info; use slog::warn; use slog_error_chain::InlineErrorChain; +use std::collections::BTreeSet; +use std::net::IpAddr; impl DataStore { + pub async fn policy_external_dns_ips( + &self, + opctx: &OpContext, + ) -> Result, Error> { + // We should have explicit storage for external DNS IPs that an operator + // can update. Today, we do not: whatever external DNS IPs are provided + // at rack setup time are the IPs we use forever. (Fixing this is + // tracked by https://github.com/oxidecomputer/omicron/issues/3732.) + // + // We can _implicitly_ determine the current set of external DNS IPs by + // examining the current target blueprint and looking at the IPs of all + // of its external DNS zones. We _must_ include expunged zones as well + // as in-service zones: during an update, we'll create a blueprint that + // expunges an external DNS zones, waits for it to go away, then wants + // to reassign that zone's external IP to a new external DNS zones. But + // because we are scanning expunged zones, we also have to allow for + // duplicates - this isn't an error and is expected if we've performed + // more than one update, at least until we start pruning old expunged + // zones out of the blueprint (tracked by + // https://github.com/oxidecomputer/omicron/issues/5552). + // + // Because we can't (yet) change external DNS IPs, we don't have to + // worry about the current blueprint changing between when we read it + // and when we calculate the set of external DNS IPs: the set will be + // identical for all blueprints back to the original one created by RSS. + // + // We don't really need to load the entire blueprint here, but it's easy + // and ideally this code will be deleted in relatively short order. + let (_target, blueprint) = + self.blueprint_target_get_current_full(opctx).await?; + + let external_dns_ips = blueprint + .all_omicron_zones(BlueprintZoneDisposition::any) + .filter_map(|(_sled_id, z)| match &z.zone_type { + BlueprintZoneType::ExternalDns(external_dns) => { + Some(external_dns.dns_address.addr.ip()) + } + _ => None, + }) + .collect(); + + Ok(external_dns_ips) + } + pub(super) async fn ensure_zone_external_networking_allocated_on_connection( &self, conn: &async_bb8_diesel::Connection, @@ -461,9 +509,10 @@ mod tests { use chrono::Utc; use nexus_config::NUM_INITIAL_RESERVED_IP_ADDRESSES; use nexus_db_model::SqlU16; + use nexus_reconfigurator_planning::blueprint_builder::BlueprintBuilder; use nexus_sled_agent_shared::inventory::OmicronZoneDataset; + use nexus_types::deployment::BlueprintTarget; use nexus_types::deployment::BlueprintZoneConfig; - use nexus_types::deployment::BlueprintZoneDisposition; use nexus_types::deployment::BlueprintZoneImageSource; use nexus_types::deployment::BlueprintZoneType; use nexus_types::deployment::OmicronZoneExternalFloatingAddr; @@ -483,10 +532,12 @@ mod tests { use omicron_common::api::external::Vni; use omicron_common::zpool_name::ZpoolName; use omicron_test_utils::dev; + use omicron_uuid_kinds::BlueprintUuid; use omicron_uuid_kinds::ExternalIpUuid; + use omicron_uuid_kinds::ExternalZpoolUuid; + use omicron_uuid_kinds::SledUuid; use omicron_uuid_kinds::ZpoolUuid; use oxnet::IpNet; - use std::collections::BTreeSet; use std::net::IpAddr; use std::net::SocketAddr; use uuid::Uuid; @@ -1292,4 +1343,149 @@ mod tests { db.terminate().await; logctx.cleanup_successful(); } + + #[tokio::test] + async fn test_policy_external_dns_ips() { + const TEST_NAME: &str = "test_policy_external_dns_ips"; + + // Helper closures to reduce boilerplate below. + let make_bp_target = |blueprint_id| BlueprintTarget { + target_id: blueprint_id, + enabled: false, + time_made_target: Utc::now(), + }; + let mut opte_ip_iter = DNS_OPTE_IPV4_SUBNET.addr_iter(); + let mut mac_iter = MacAddr::iter_system(); + let mut make_external_dns_zone = |ip, disposition| { + let zone_id = OmicronZoneUuid::new_v4(); + let pool = ZpoolName::External(ExternalZpoolUuid::new_v4()); + BlueprintZoneConfig { + disposition, + id: zone_id, + filesystem_pool: pool, + zone_type: BlueprintZoneType::ExternalDns( + blueprint_zone_type::ExternalDns { + dataset: OmicronZoneDataset { pool_name: pool }, + http_address: "[::1]:0".parse().unwrap(), + dns_address: OmicronZoneExternalFloatingAddr { + id: ExternalIpUuid::new_v4(), + addr: SocketAddr::new(ip, 0), + }, + nic: NetworkInterface { + id: Uuid::new_v4(), + kind: NetworkInterfaceKind::Service { + id: zone_id.into_untyped_uuid(), + }, + name: "test-external-dns".parse().unwrap(), + ip: opte_ip_iter.next().unwrap().into(), + mac: mac_iter.next().unwrap(), + subnet: IpNet::from(*DNS_OPTE_IPV4_SUBNET), + vni: Vni::SERVICES_VNI, + primary: true, + slot: 0, + transit_ips: vec![], + }, + }, + ), + image_source: BlueprintZoneImageSource::InstallDataset, + } + }; + + // Set up. + let logctx = dev::test_setup_log(TEST_NAME); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + + // Create a blueprint with one sled and no zones. Insert it and make it + // the target. + let sled_id = SledUuid::new_v4(); + let bp0 = BlueprintBuilder::build_empty_with_sleds( + std::iter::once(sled_id), + TEST_NAME, + ); + datastore.blueprint_insert(opctx, &bp0).await.expect("inserted bp0"); + datastore + .blueprint_target_set_current(opctx, make_bp_target(bp0.id)) + .await + .expect("made bp0 the target"); + + // No external DNS zones => no external DNS IPs. + let external_dns_ips = datastore + .policy_external_dns_ips(opctx) + .await + .expect("got external DNS IPs"); + assert_eq!(external_dns_ips, BTreeSet::new()); + + // Create a blueprint with three in-service external DNS zones. We + // should get their IPs back. + let expected_ips = ["192.168.1.1", "192.168.1.2", "192.168.1.3"] + .into_iter() + .map(|ip| ip.parse::().unwrap()) + .collect::>(); + let mut bp1 = bp0.clone(); + bp1.id = BlueprintUuid::new_v4(); + bp1.parent_blueprint_id = Some(bp0.id); + for &ip in &expected_ips { + bp1.sleds.get_mut(&sled_id).unwrap().zones.insert( + make_external_dns_zone(ip, BlueprintZoneDisposition::InService), + ); + } + + // Insert bp1 and make it the target. Confirm we get back the expected + // external DNS IPs. + datastore.blueprint_insert(opctx, &bp1).await.expect("inserted bp1"); + datastore + .blueprint_target_set_current(opctx, make_bp_target(bp1.id)) + .await + .expect("made bp1 the target"); + let external_dns_ips = datastore + .policy_external_dns_ips(opctx) + .await + .expect("got external DNS IPs"); + assert_eq!(external_dns_ips, expected_ips); + + // Create a third blueprint with multiple expunged external DNS zones + // covering a couple additional IPs. Those should also be returned. + let extra_ips = ["192.168.1.4", "192.168.1.5"] + .into_iter() + .map(|ip| ip.parse::().unwrap()) + .collect::>(); + assert_eq!(expected_ips.intersection(&extra_ips).count(), 0); + + let mut bp2 = bp1.clone(); + bp2.id = BlueprintUuid::new_v4(); + bp2.parent_blueprint_id = Some(bp1.id); + for &ip in &extra_ips { + for i in 0..4 { + bp2.sleds.get_mut(&sled_id).unwrap().zones.insert( + make_external_dns_zone( + ip, + BlueprintZoneDisposition::Expunged { + as_of_generation: Generation::new(), + ready_for_cleanup: i % 2 == 0, + }, + ), + ); + } + } + + // Insert bp1 and make it the target. Confirm we get back the expected + // external DNS IPs. + datastore.blueprint_insert(opctx, &bp2).await.expect("inserted bp2"); + datastore + .blueprint_target_set_current(opctx, make_bp_target(bp2.id)) + .await + .expect("made bp2 the target"); + let external_dns_ips = datastore + .policy_external_dns_ips(opctx) + .await + .expect("got external DNS IPs"); + let expected_ips = + expected_ips.union(&extra_ips).copied().collect::>(); + assert_eq!(external_dns_ips, expected_ips); + + // Clean up. + db.terminate().await; + logctx.cleanup_successful(); + } } From 42eaa55680459ac12f8a4d216fd0007062223ce9 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 27 Oct 2025 10:10:32 -0400 Subject: [PATCH 06/16] separate ExternalIpPolicyBuilder --- .../deployment/external_networking.rs | 46 ++++--- nexus/db-queries/src/db/datastore/rack.rs | 15 ++- nexus/reconfigurator/execution/src/dns.rs | 15 ++- .../planning/src/blueprint_builder/builder.rs | 9 +- .../allocators/external_networking.rs | 23 ++-- nexus/reconfigurator/planning/src/example.rs | 43 +++--- nexus/reconfigurator/planning/src/planner.rs | 20 +-- nexus/reconfigurator/planning/src/system.rs | 20 ++- nexus/reconfigurator/preparation/src/lib.rs | 52 ++++++-- nexus/types/src/deployment.rs | 1 + nexus/types/src/deployment/planning_input.rs | 122 +++++++++++++++--- 11 files changed, 252 insertions(+), 114 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment/external_networking.rs b/nexus/db-queries/src/db/datastore/deployment/external_networking.rs index f0c80082293..33ecd0d9200 100644 --- a/nexus/db-queries/src/db/datastore/deployment/external_networking.rs +++ b/nexus/db-queries/src/db/datastore/deployment/external_networking.rs @@ -36,25 +36,28 @@ use std::collections::BTreeSet; use std::net::IpAddr; impl DataStore { - pub async fn policy_external_dns_ips( + /// Return the set of external IPs configured for our external DNS servers + /// when the rack was set up. + /// + /// We should have explicit storage for the external IPs on which we run + /// external DNS that an operator can update. Today, we do not: whatever + /// external DNS IPs are provided at rack setup time are the IPs we use + /// forever. (Fixing this is tracked by + /// https://github.com/oxidecomputer/omicron/issues/8255.) + pub async fn external_dns_external_ips_specified_by_rack_setup( &self, opctx: &OpContext, ) -> Result, Error> { - // We should have explicit storage for external DNS IPs that an operator - // can update. Today, we do not: whatever external DNS IPs are provided - // at rack setup time are the IPs we use forever. (Fixing this is - // tracked by https://github.com/oxidecomputer/omicron/issues/3732.) - // - // We can _implicitly_ determine the current set of external DNS IPs by - // examining the current target blueprint and looking at the IPs of all - // of its external DNS zones. We _must_ include expunged zones as well - // as in-service zones: during an update, we'll create a blueprint that - // expunges an external DNS zones, waits for it to go away, then wants - // to reassign that zone's external IP to a new external DNS zones. But - // because we are scanning expunged zones, we also have to allow for - // duplicates - this isn't an error and is expected if we've performed - // more than one update, at least until we start pruning old expunged - // zones out of the blueprint (tracked by + // We can _implicitly_ determine the set of external DNS IPs provied + // during rack setup by examining the current target blueprint and + // looking at the IPs of all of its external DNS zones. We _must_ + // include expunged zones as well as in-service zones: during an update, + // we'll create a blueprint that expunges an external DNS zones, waits + // for it to go away, then wants to reassign that zone's external IP to + // a new external DNS zones. But because we are scanning expunged zones, + // we also have to allow for duplicates - this isn't an error and is + // expected if we've performed more than one update, at least until we + // start pruning old expunged zones out of the blueprint (tracked by // https://github.com/oxidecomputer/omicron/issues/5552). // // Because we can't (yet) change external DNS IPs, we don't have to @@ -1345,8 +1348,9 @@ mod tests { } #[tokio::test] - async fn test_policy_external_dns_ips() { - const TEST_NAME: &str = "test_policy_external_dns_ips"; + async fn test_external_dns_external_ips_specified_by_rack_setup() { + const TEST_NAME: &str = + "test_external_dns_external_ips_specified_by_rack_setup"; // Helper closures to reduce boilerplate below. let make_bp_target = |blueprint_id| BlueprintTarget { @@ -1411,7 +1415,7 @@ mod tests { // No external DNS zones => no external DNS IPs. let external_dns_ips = datastore - .policy_external_dns_ips(opctx) + .external_dns_external_ips_specified_by_rack_setup(opctx) .await .expect("got external DNS IPs"); assert_eq!(external_dns_ips, BTreeSet::new()); @@ -1439,7 +1443,7 @@ mod tests { .await .expect("made bp1 the target"); let external_dns_ips = datastore - .policy_external_dns_ips(opctx) + .external_dns_external_ips_specified_by_rack_setup(opctx) .await .expect("got external DNS IPs"); assert_eq!(external_dns_ips, expected_ips); @@ -1477,7 +1481,7 @@ mod tests { .await .expect("made bp2 the target"); let external_dns_ips = datastore - .policy_external_dns_ips(opctx) + .external_dns_external_ips_specified_by_rack_setup(opctx) .await .expect("got external DNS IPs"); let expected_ips = diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index e4080b142b3..605d4fafab0 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -1412,13 +1412,13 @@ mod test { let sled2 = create_test_sled(&datastore, SledUuid::new_v4()).await; let sled3 = create_test_sled(&datastore, SledUuid::new_v4()).await; - let external_ip_policy = ExternalIpPolicy::new(vec![ + let external_ip_policy = ExternalIpPolicy::single_pool_no_external_dns( IpRange::try_from(( Ipv4Addr::new(1, 2, 3, 4), Ipv4Addr::new(1, 2, 3, 6), )) .unwrap(), - ]); + ); let mut system = SystemDescription::new(); system @@ -1767,10 +1767,10 @@ mod test { // Ask for two Nexus services, with different external IPs. let nexus_ip_start = Ipv4Addr::new(1, 2, 3, 4); let nexus_ip_end = Ipv4Addr::new(1, 2, 3, 5); - let external_ip_policy = ExternalIpPolicy::new(vec![ + let external_ip_policy = ExternalIpPolicy::single_pool_no_external_dns( IpRange::try_from((nexus_ip_start, nexus_ip_end)) .expect("Cannot create IP Range"), - ]); + ); let mut system = SystemDescription::new(); system @@ -2062,10 +2062,10 @@ mod test { Ipv6Addr::new(0xfd00, 0x1122, 0x3344, 0, 0, 0, 0, 1); let nexus_ip_end = Ipv6Addr::new(0xfd00, 0x1122, 0x3344, 0, 0, 0, 0, 10); - let external_ip_policy = ExternalIpPolicy::new(vec![ + let external_ip_policy = ExternalIpPolicy::single_pool_no_external_dns( IpRange::try_from((nexus_ip_start, nexus_ip_end)) .expect("Cannot create IP Range"), - ]); + ); let mut system = SystemDescription::new(); system @@ -2417,7 +2417,8 @@ mod test { let sled = create_test_sled(&datastore, SledUuid::new_v4()).await; let ip = IpAddr::V4(Ipv4Addr::new(1, 2, 3, 4)); - let external_ip_policy = ExternalIpPolicy::new(vec![IpRange::from(ip)]); + let external_ip_policy = + ExternalIpPolicy::single_pool_no_external_dns(IpRange::from(ip)); let mut system = SystemDescription::new(); system diff --git a/nexus/reconfigurator/execution/src/dns.rs b/nexus/reconfigurator/execution/src/dns.rs index f5426ee193f..32f67ecb1bf 100644 --- a/nexus/reconfigurator/execution/src/dns.rs +++ b/nexus/reconfigurator/execution/src/dns.rs @@ -1537,6 +1537,7 @@ mod test { sled_rows: &sled_rows, zpool_rows: &zpool_rows, ip_pool_range_rows: &ip_pool_range_rows, + external_dns_external_ips: BTreeSet::new(), internal_dns_version: dns_initial_internal.generation.into(), external_dns_version: dns_latest_external.generation.into(), // These are not used because we're not actually going through @@ -1566,10 +1567,16 @@ mod test { .into_builder(); // We'll need another (fake) external IP for this new Nexus. - builder - .policy_mut() - .external_ips - .push(IpRange::from(IpAddr::V4(Ipv4Addr::LOCALHOST))); + builder.policy_mut().external_ips = { + let mut ip_policy = + builder.policy_mut().external_ips.clone().into_builder(); + ip_policy + .push_service_ip_pool(IpRange::from(IpAddr::V4( + Ipv4Addr::LOCALHOST, + ))) + .unwrap(); + ip_policy.build() + }; builder.build() }; diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index c472dd02657..9bfe95cfd89 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -3528,8 +3528,13 @@ pub mod test { assert!(!used_ip_ranges.is_empty()); let input = { let mut builder = input.into_builder(); - builder.policy_mut().external_ips = - ExternalIpPolicy::new(used_ip_ranges); + builder.policy_mut().external_ips = { + let mut ip_policy = ExternalIpPolicy::builder(); + for r in used_ip_ranges { + ip_policy.push_service_ip_pool(r).unwrap(); + } + ip_policy.build() + }; builder.build() }; diff --git a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs b/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs index 51123729478..7e2bd7ce27f 100644 --- a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs +++ b/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs @@ -756,8 +756,14 @@ pub mod test { }; // Build up the allocator and mark all used IPs. - let mut allocator = - ExternalIpAllocator::new(&ExternalIpPolicy::new(ip_pool_ranges)); + let policy = { + let mut builder = ExternalIpPolicy::builder(); + for r in ip_pool_ranges { + builder.push_service_ip_pool(r).unwrap(); + } + builder.build() + }; + let mut allocator = ExternalIpAllocator::new(&policy); for &ip in &used_exclusive { allocator .mark_ip_used(&as_floating(ip)) @@ -867,14 +873,11 @@ pub mod test { assert_eq!(service_ip_pool.len(), 3); let external_ip_policy = { - let mut policy = ExternalIpPolicy::new(vec![service_ip_pool]); - policy - .set_ip_for_external_dns("192.0.2.1".parse().unwrap()) - .unwrap(); - policy - .set_ip_for_external_dns("192.0.2.2".parse().unwrap()) - .unwrap(); - policy + let mut builder = ExternalIpPolicy::builder(); + builder.push_service_ip_pool(service_ip_pool).unwrap(); + builder.add_external_dns_ip("192.0.2.1".parse().unwrap()).unwrap(); + builder.add_external_dns_ip("192.0.2.2".parse().unwrap()).unwrap(); + builder.build() }; let make_external_dns = |index, disposition| { diff --git a/nexus/reconfigurator/planning/src/example.rs b/nexus/reconfigurator/planning/src/example.rs index c0d5ce5cfd9..9eed1317b23 100644 --- a/nexus/reconfigurator/planning/src/example.rs +++ b/nexus/reconfigurator/planning/src/example.rs @@ -503,29 +503,30 @@ impl ExampleSystemBuilder { // Add as many external IPs as is necessary for external DNS zones. We // pick addresses in the TEST-NET-2 (RFC 5737) range. - let mut external_ip_policy = system.external_ip_policy().clone(); if self.external_dns_count.0 > 0 { - external_ip_policy.push( - IpRange::try_from(( - "198.51.100.1".parse::().unwrap(), - "198.51.100.30".parse::().unwrap(), - )) - .unwrap(), - ); - } - for i in 0..self.external_dns_count.0 { - external_ip_policy - .set_ip_for_external_dns(IpAddr::V4(Ipv4Addr::new( - 198, - 51, - 100, - (i + 1) - .try_into() - .expect("external_dns_count is always <= 30"), - ))) - .expect("test IPs are valid service IPs"); + let mut builder = + system.external_ip_policy().clone().into_builder(); + builder + .push_service_ip_pool( + IpRange::try_from(( + "198.51.100.1".parse::().unwrap(), + "198.51.100.30".parse::().unwrap(), + )) + .unwrap(), + ) + .unwrap(); + for i in 0..self.external_dns_count.0 { + let lo = (i + 1) + .try_into() + .expect("external_dns_count is always <= 30"); + builder + .add_external_dns_ip(IpAddr::V4(Ipv4Addr::new( + 198, 51, 100, lo, + ))) + .expect("test IPs are valid service IPs"); + } + system.set_external_ip_policy(builder.build()); } - system.set_external_ip_policy(external_ip_policy); let mut input_builder = system .to_planning_input_builder() diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index c626dbee536..b79aeba535c 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -3277,19 +3277,23 @@ pub(crate) mod test { }); let input = { let mut builder = input.into_builder(); + let mut ip_policy = + builder.policy_mut().external_ips.clone().into_builder(); // Add a "service IP pool" covering our external DNS IP range. - builder.policy_mut().external_ips.push( - IpRange::try_from((external_dns_ips[0], external_dns_ips[2])) + ip_policy + .push_service_ip_pool( + IpRange::try_from(( + external_dns_ips[0], + external_dns_ips[2], + )) .unwrap(), - ); + ) + .unwrap(); // Set these IPs as "for external DNS". for ip in external_dns_ips { - builder - .policy_mut() - .external_ips - .set_ip_for_external_dns(ip) - .unwrap(); + ip_policy.add_external_dns_ip(ip).unwrap(); } + builder.policy_mut().external_ips = ip_policy.build(); builder.build() }; diff --git a/nexus/reconfigurator/planning/src/system.rs b/nexus/reconfigurator/planning/src/system.rs index 4a095740fb5..2852c009cbe 100644 --- a/nexus/reconfigurator/planning/src/system.rs +++ b/nexus/reconfigurator/planning/src/system.rs @@ -184,13 +184,19 @@ impl SystemDescription { // Nexus / Boundary NTPs IPs from TEST-NET-1 (RFC 5737). // // This policy doesn't configure any external DNS IPs. - let external_ip_policy = ExternalIpPolicy::new(vec![ - IpRange::try_from(( - "192.0.2.2".parse::().unwrap(), - "192.0.2.20".parse::().unwrap(), - )) - .unwrap(), - ]); + let external_ip_policy = { + let mut builder = ExternalIpPolicy::builder(); + builder + .push_service_ip_pool( + IpRange::try_from(( + "192.0.2.2".parse::().unwrap(), + "192.0.2.20".parse::().unwrap(), + )) + .unwrap(), + ) + .unwrap(); + builder.build() + }; SystemDescription { sleds: IndexMap::new(), diff --git a/nexus/reconfigurator/preparation/src/lib.rs b/nexus/reconfigurator/preparation/src/lib.rs index d36c3fca69c..31914daebdf 100644 --- a/nexus/reconfigurator/preparation/src/lib.rs +++ b/nexus/reconfigurator/preparation/src/lib.rs @@ -60,6 +60,7 @@ use slog_error_chain::InlineErrorChain; use std::cmp::Reverse; use std::collections::BTreeMap; use std::collections::BTreeSet; +use std::net::IpAddr; /// Given various pieces of database state that go into the blueprint planning /// process, produce a `PlanningInput` object encapsulating what the planner @@ -69,6 +70,7 @@ pub struct PlanningInputFromDb<'a> { pub zpool_rows: &'a [(nexus_db_model::Zpool, nexus_db_model::PhysicalDisk)], pub ip_pool_range_rows: &'a [nexus_db_model::IpPoolRange], + pub external_dns_external_ips: BTreeSet, pub external_ip_rows: &'a [nexus_db_model::ExternalIp], pub service_nic_rows: &'a [nexus_db_model::ServiceNetworkInterface], pub target_boundary_ntp_zone_count: usize, @@ -121,6 +123,14 @@ impl PlanningInputFromDb<'_> { .internal_context("fetching all external zpool rows")?; let ip_pool_range_rows = fetch_all_service_ip_pool_ranges(opctx, datastore).await?; + // TODO-correctness We ought to allow the IPs on which we run external + // DNS servers to change, but we don't: instead, we always reuse the IPs + // that were specified when the rack was set up. + // + // https://github.com/oxidecomputer/omicron/issues/8255 + let external_dns_external_ips = datastore + .external_dns_external_ips_specified_by_rack_setup(opctx) + .await?; let external_ip_rows = datastore .external_ip_list_service_all_batched(opctx) .await @@ -229,6 +239,7 @@ impl PlanningInputFromDb<'_> { sled_rows: &sled_rows, zpool_rows: &zpool_rows, ip_pool_range_rows: &ip_pool_range_rows, + external_dns_external_ips, target_boundary_ntp_zone_count: BOUNDARY_NTP_REDUNDANCY, target_nexus_zone_count: NEXUS_REDUNDANCY, target_internal_dns_zone_count: INTERNAL_DNS_REDUNDANCY, @@ -257,21 +268,36 @@ impl PlanningInputFromDb<'_> { Ok(planning_input) } + fn build_external_ip_policy(&self) -> Result { + let mut builder = ExternalIpPolicy::builder(); + for range in self.ip_pool_range_rows { + let range = IpRange::try_from(range).map_err(|e| { + Error::internal_error(&format!( + "invalid IP pool range in database: {}", + InlineErrorChain::new(&e), + )) + })?; + builder.push_service_ip_pool(range).map_err(|e| { + Error::internal_error(&format!( + "cannot construct external IP policy: {}", + InlineErrorChain::new(&e), + )) + })?; + } + for &ip in &self.external_dns_external_ips { + builder.add_external_dns_ip(ip).map_err(|e| { + Error::internal_error(&format!( + "cannot construct external IP policy: {}", + InlineErrorChain::new(&e), + )) + })?; + } + Ok(builder.build()) + } + pub fn build(&self) -> Result { - let service_ip_pool_ranges = self - .ip_pool_range_rows - .iter() - .map(|range| { - IpRange::try_from(range).map_err(|e| { - Error::internal_error(&format!( - "invalid IP pool range in database: {e:#}" - )) - }) - }) - .collect::, _>>()?; - let external_ips = ExternalIpPolicy::new(service_ip_pool_ranges); let policy = Policy { - external_ips, + external_ips: self.build_external_ip_policy()?, target_boundary_ntp_zone_count: self.target_boundary_ntp_zone_count, target_nexus_zone_count: self.target_nexus_zone_count, target_internal_dns_zone_count: self.target_internal_dns_zone_count, diff --git a/nexus/types/src/deployment.rs b/nexus/types/src/deployment.rs index 52a9d92c3ff..ab74ebc057e 100644 --- a/nexus/types/src/deployment.rs +++ b/nexus/types/src/deployment.rs @@ -103,6 +103,7 @@ pub use planning_input::CockroachDbPreserveDowngrade; pub use planning_input::CockroachDbSettings; pub use planning_input::DiskFilter; pub use planning_input::ExternalIpPolicy; +pub use planning_input::ExternalIpPolicyBuilder; pub use planning_input::ExternalIpPolicyError; pub use planning_input::OximeterReadMode; pub use planning_input::OximeterReadPolicy; diff --git a/nexus/types/src/deployment/planning_input.rs b/nexus/types/src/deployment/planning_input.rs index 16ec0ca703c..0dd2ddf29c3 100644 --- a/nexus/types/src/deployment/planning_input.rs +++ b/nexus/types/src/deployment/planning_input.rs @@ -1083,13 +1083,16 @@ impl<'de> Deserialize<'de> for ExternalIpPolicy { let ExternalIpPolicy { service_ip_pool_ranges, external_dns_ips } = ExternalIpPolicyShadow::deserialize(deserializer)?; - // ...then validate. - let mut policy = ExternalIpPolicy::new(service_ip_pool_ranges); + // ...then validate by going through the builder. + let mut builder = ExternalIpPolicy::builder(); + for pool in service_ip_pool_ranges { + builder.push_service_ip_pool(pool).map_err(D::Error::custom)?; + } for ip in external_dns_ips { - policy.set_ip_for_external_dns(ip).map_err(D::Error::custom)?; + builder.add_external_dns_ip(ip).map_err(D::Error::custom)?; } - Ok(policy) + Ok(builder.build()) } } @@ -1101,27 +1104,28 @@ impl ExternalIpPolicy { } } - // TODO-john comment - pub fn new(service_ip_pool_ranges: Vec) -> Self { - Self { service_ip_pool_ranges, external_dns_ips: BTreeSet::new() } + /// Construct an `ExternalIpPolicy` containing a single service IP pool and + /// no external DNS IPs. + /// + /// This is used primarily by tests. A single pool with no external DNS IPs + /// can be constructed infallibly. + pub fn single_pool_no_external_dns(range: IpRange) -> Self { + Self { + service_ip_pool_ranges: vec![range], + external_dns_ips: BTreeSet::new(), + } } - // TODO-john comment - pub fn set_ip_for_external_dns( - &mut self, - ip: IpAddr, - ) -> Result<(), ExternalIpPolicyError> { - if self.service_ip_pool_ranges.iter().any(|pool| pool.contains(ip)) { - self.external_dns_ips.insert(ip); - Ok(()) - } else { - Err(ExternalIpPolicyError::ExternalDnsOutsideServiceIpPools(ip)) - } + /// Construct an empty [`ExternalIpPolicyBuilder`]. + pub fn builder() -> ExternalIpPolicyBuilder { + ExternalIpPolicyBuilder::new() } - // TODO-john comment / remove / rename? - pub fn push(&mut self, pool_range: IpRange) { - self.service_ip_pool_ranges.push(pool_range); + /// Construct an [`ExternalIpPolicyBuilder`] that contains all of this + /// policy's IP pools and external DNS IPs. + pub fn into_builder(self) -> ExternalIpPolicyBuilder { + let Self { service_ip_pool_ranges, external_dns_ips } = self; + ExternalIpPolicyBuilder { service_ip_pool_ranges, external_dns_ips } } // TODO-john comment / remove? @@ -1134,8 +1138,84 @@ impl ExternalIpPolicy { } } +#[derive(Debug, Clone, Default)] +pub struct ExternalIpPolicyBuilder { + service_ip_pool_ranges: Vec, + external_dns_ips: BTreeSet, +} + +impl ExternalIpPolicyBuilder { + /// Create a new, empty `ExternalIpPolicyBuilder`. + pub fn new() -> Self { + Self::default() + } + + /// Add an external IP pool for services. + /// + /// # Errors + /// + /// Fails if `pool_range` overlaps an existing pool within this builder. + // TODO-correctness Eventually this may be broken up into different pools + // for different kinds of services. For now, we group all services into a + // single set of pools and then separately carve out external DNS from + // within those pools. + pub fn push_service_ip_pool( + &mut self, + new_pool: IpRange, + ) -> Result<(), ExternalIpPolicyError> { + // Linear scan should be good enough: we don't expect many pools, and + // the check we do for each pool is cheap. + for &existing_pool in &self.service_ip_pool_ranges { + if existing_pool.overlaps(&new_pool) { + return Err(ExternalIpPolicyError::OverlappingPools { + new_pool, + existing_pool, + }); + } + } + self.service_ip_pool_ranges.push(new_pool); + Ok(()) + } + + /// Mark an IP as exclusively for the use of external DNS. + /// + /// # Errors + /// + /// Fails if the requested IP is _not_ covered by an existing service IP + /// pool range. + // TODO-correctness See the note above on `push_service_ip_pool()`; the fact + // that external DNS IPs are not special or distinct from the general + // service IP pools is a little confusing, and it would be nice to clean + // that up eventually. + pub fn add_external_dns_ip( + &mut self, + ip: IpAddr, + ) -> Result<(), ExternalIpPolicyError> { + if self.service_ip_pool_ranges.iter().any(|pool| pool.contains(ip)) { + self.external_dns_ips.insert(ip); + Ok(()) + } else { + Err(ExternalIpPolicyError::ExternalDnsOutsideServiceIpPools(ip)) + } + } + + pub fn build(self) -> ExternalIpPolicy { + let Self { service_ip_pool_ranges, external_dns_ips } = self; + ExternalIpPolicy { service_ip_pool_ranges, external_dns_ips } + } +} + #[derive(Debug, thiserror::Error)] pub enum ExternalIpPolicyError { + #[error( + "external IP policy cannot have overlapping pools: \ + [{}, {}] overlaps [{}, {}]", + new_pool.first_address(), + new_pool.last_address(), + existing_pool.first_address(), + existing_pool.last_address(), + )] + OverlappingPools { new_pool: IpRange, existing_pool: IpRange }, #[error("external DNS IP {0} is not a member of any service IP pool")] ExternalDnsOutsideServiceIpPools(IpAddr), } From c2fe90ac8dfbf044502ac48aaac0918dda5e8c66 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 27 Oct 2025 14:41:22 -0400 Subject: [PATCH 07/16] test fix --- nexus/reconfigurator/execution/src/dns.rs | 93 +++++++++++------------ 1 file changed, 44 insertions(+), 49 deletions(-) diff --git a/nexus/reconfigurator/execution/src/dns.rs b/nexus/reconfigurator/execution/src/dns.rs index 32f67ecb1bf..1f622f62752 100644 --- a/nexus/reconfigurator/execution/src/dns.rs +++ b/nexus/reconfigurator/execution/src/dns.rs @@ -377,7 +377,6 @@ mod test { use nexus_types::internal_api::params::DnsRecord; use nexus_types::internal_api::params::Srv; use nexus_types::silo::silo_dns_name; - use omicron_common::address::IpRange; use omicron_common::address::Ipv6Subnet; use omicron_common::address::RACK_PREFIX; use omicron_common::address::REPO_DEPOT_PORT; @@ -1532,54 +1531,37 @@ mod test { .into_iter() .map(|z| z.nexus_id()) .collect(); - let planning_input = { - let mut builder = PlanningInputFromDb { - sled_rows: &sled_rows, - zpool_rows: &zpool_rows, - ip_pool_range_rows: &ip_pool_range_rows, - external_dns_external_ips: BTreeSet::new(), - internal_dns_version: dns_initial_internal.generation.into(), - external_dns_version: dns_latest_external.generation.into(), - // These are not used because we're not actually going through - // the planner. - cockroachdb_settings: &CockroachDbSettings::empty(), - external_ip_rows: &[], - service_nic_rows: &[], - target_boundary_ntp_zone_count: BOUNDARY_NTP_REDUNDANCY, - target_nexus_zone_count: NEXUS_REDUNDANCY, - target_internal_dns_zone_count: INTERNAL_DNS_REDUNDANCY, - target_oximeter_zone_count: OXIMETER_REDUNDANCY, - target_cockroachdb_zone_count: COCKROACHDB_REDUNDANCY, - target_cockroachdb_cluster_version: - CockroachDbClusterVersion::POLICY, - target_crucible_pantry_zone_count: CRUCIBLE_PANTRY_REDUNDANCY, - clickhouse_policy: None, - oximeter_read_policy: OximeterReadPolicy::new(1), - tuf_repo: TufRepoPolicy::initial(), - old_repo: TufRepoPolicy::initial(), - planner_config: PlannerConfig::default(), - active_nexus_zones, - not_yet_nexus_zones: BTreeSet::new(), - log, - } - .build() - .unwrap() - .into_builder(); - - // We'll need another (fake) external IP for this new Nexus. - builder.policy_mut().external_ips = { - let mut ip_policy = - builder.policy_mut().external_ips.clone().into_builder(); - ip_policy - .push_service_ip_pool(IpRange::from(IpAddr::V4( - Ipv4Addr::LOCALHOST, - ))) - .unwrap(); - ip_policy.build() - }; - - builder.build() - }; + let planning_input = PlanningInputFromDb { + sled_rows: &sled_rows, + zpool_rows: &zpool_rows, + ip_pool_range_rows: &ip_pool_range_rows, + external_dns_external_ips: BTreeSet::new(), + internal_dns_version: dns_initial_internal.generation.into(), + external_dns_version: dns_latest_external.generation.into(), + // These are not used because we're not actually going through + // the planner. + cockroachdb_settings: &CockroachDbSettings::empty(), + external_ip_rows: &[], + service_nic_rows: &[], + target_boundary_ntp_zone_count: BOUNDARY_NTP_REDUNDANCY, + target_nexus_zone_count: NEXUS_REDUNDANCY, + target_internal_dns_zone_count: INTERNAL_DNS_REDUNDANCY, + target_oximeter_zone_count: OXIMETER_REDUNDANCY, + target_cockroachdb_zone_count: COCKROACHDB_REDUNDANCY, + target_cockroachdb_cluster_version: + CockroachDbClusterVersion::POLICY, + target_crucible_pantry_zone_count: CRUCIBLE_PANTRY_REDUNDANCY, + clickhouse_policy: None, + oximeter_read_policy: OximeterReadPolicy::new(1), + tuf_repo: TufRepoPolicy::initial(), + old_repo: TufRepoPolicy::initial(), + planner_config: PlannerConfig::default(), + active_nexus_zones, + not_yet_nexus_zones: BTreeSet::new(), + log, + } + .build() + .unwrap(); let collection = CollectionBuilder::new("test").build(); let mut builder = BlueprintBuilder::new_based_on( &log, @@ -1592,6 +1574,19 @@ mod test { .unwrap(); let sled_id = blueprint.sleds().next().expect("expected at least one sled"); + + // Adding a Nexus zone requires an available external IP address. If we + // were to look at the current blueprint and the external IP policy, + // we'd see three IP pools, each containing a single IP address used by + // one service: + // + // * 1.2.3.4 (boundary NTP) + // * 127.0.0.1 (Nexus) + // * ::1 (external DNS) + // + // However, when the builder compiles its list of "IPs already in use", + // it _ignores_ loopback addresses, meaning we still have two external + // IPs available for new zones (127.0.0.1 and ::1). builder .sled_add_zone_nexus( sled_id, From 82474cdf209e2fa868a347be0b1f6f30da4518d6 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 27 Oct 2025 15:26:02 -0400 Subject: [PATCH 08/16] don't expose raw IP pools slice --- nexus/db-queries/src/db/datastore/rack.rs | 53 +++++++++---------- .../tests/integration/blueprint_edit.rs | 10 ++-- .../allocators/external_networking.rs | 7 +-- nexus/reconfigurator/planning/src/planner.rs | 15 +++--- nexus/types/src/deployment/planning_input.rs | 14 +++-- 5 files changed, 50 insertions(+), 49 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index 605d4fafab0..e761832f5ee 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -1412,17 +1412,17 @@ mod test { let sled2 = create_test_sled(&datastore, SledUuid::new_v4()).await; let sled3 = create_test_sled(&datastore, SledUuid::new_v4()).await; - let external_ip_policy = ExternalIpPolicy::single_pool_no_external_dns( - IpRange::try_from(( - Ipv4Addr::new(1, 2, 3, 4), - Ipv4Addr::new(1, 2, 3, 6), - )) - .unwrap(), - ); + let service_ip_pool = IpRange::try_from(( + Ipv4Addr::new(1, 2, 3, 4), + Ipv4Addr::new(1, 2, 3, 6), + )) + .unwrap(); + let external_ip_policy = + ExternalIpPolicy::single_pool_no_external_dns(service_ip_pool); let mut system = SystemDescription::new(); system - .set_external_ip_policy(external_ip_policy.clone()) + .set_external_ip_policy(external_ip_policy) .sled(SledBuilder::new().id(sled1.id())) .expect("failed to add sled1") .sled(SledBuilder::new().id(sled2.id())) @@ -1646,9 +1646,7 @@ mod test { &opctx, RackInit { blueprint: blueprint.clone(), - service_ip_pool_ranges: external_ip_policy - .service_ip_pool_ranges() - .to_vec(), + service_ip_pool_ranges: vec![service_ip_pool], ..Default::default() }, ) @@ -1767,14 +1765,16 @@ mod test { // Ask for two Nexus services, with different external IPs. let nexus_ip_start = Ipv4Addr::new(1, 2, 3, 4); let nexus_ip_end = Ipv4Addr::new(1, 2, 3, 5); - let external_ip_policy = ExternalIpPolicy::single_pool_no_external_dns( + let service_ip_pool = IpRange::try_from((nexus_ip_start, nexus_ip_end)) - .expect("Cannot create IP Range"), + .expect("Cannot create IP Range"); + let external_ip_policy = ExternalIpPolicy::single_pool_no_external_dns( + service_ip_pool ); let mut system = SystemDescription::new(); system - .set_external_ip_policy(external_ip_policy.clone()) + .set_external_ip_policy(external_ip_policy) .sled(SledBuilder::new().id(sled.id())) .expect("failed to add sled"); @@ -1919,9 +1919,7 @@ mod test { RackInit { blueprint: blueprint.clone(), datasets: datasets.clone(), - service_ip_pool_ranges: external_ip_policy - .service_ip_pool_ranges() - .to_vec(), + service_ip_pool_ranges: vec![service_ip_pool], internal_dns, external_dns, ..Default::default() @@ -2062,14 +2060,16 @@ mod test { Ipv6Addr::new(0xfd00, 0x1122, 0x3344, 0, 0, 0, 0, 1); let nexus_ip_end = Ipv6Addr::new(0xfd00, 0x1122, 0x3344, 0, 0, 0, 0, 10); - let external_ip_policy = ExternalIpPolicy::single_pool_no_external_dns( + let service_ip_pool = IpRange::try_from((nexus_ip_start, nexus_ip_end)) - .expect("Cannot create IP Range"), + .expect("Cannot create IP Range"); + let external_ip_policy = ExternalIpPolicy::single_pool_no_external_dns( + service_ip_pool ); let mut system = SystemDescription::new(); system - .set_external_ip_policy(external_ip_policy.clone()) + .set_external_ip_policy(external_ip_policy) .sled(SledBuilder::new().id(sled.id())) .expect("failed to add sled"); @@ -2172,9 +2172,7 @@ mod test { RackInit { blueprint: blueprint.clone(), datasets: datasets.clone(), - service_ip_pool_ranges: external_ip_policy - .service_ip_pool_ranges() - .to_vec(), + service_ip_pool_ranges: vec![service_ip_pool], internal_dns, external_dns, ..Default::default() @@ -2417,12 +2415,13 @@ mod test { let sled = create_test_sled(&datastore, SledUuid::new_v4()).await; let ip = IpAddr::V4(Ipv4Addr::new(1, 2, 3, 4)); + let service_ip_pool = IpRange::from(ip); let external_ip_policy = - ExternalIpPolicy::single_pool_no_external_dns(IpRange::from(ip)); + ExternalIpPolicy::single_pool_no_external_dns(service_ip_pool); let mut system = SystemDescription::new(); system - .set_external_ip_policy(external_ip_policy.clone()) + .set_external_ip_policy(external_ip_policy) .sled(SledBuilder::new().id(sled.id())) .expect("failed to add sled"); @@ -2539,9 +2538,7 @@ mod test { RackInit { rack_id: rack_id(), blueprint: blueprint.clone(), - service_ip_pool_ranges: external_ip_policy - .service_ip_pool_ranges() - .to_vec(), + service_ip_pool_ranges: vec![service_ip_pool], ..Default::default() }, ) diff --git a/nexus/reconfigurator/cli-integration-tests/tests/integration/blueprint_edit.rs b/nexus/reconfigurator/cli-integration-tests/tests/integration/blueprint_edit.rs index a938930603a..0ffeee4a304 100644 --- a/nexus/reconfigurator/cli-integration-tests/tests/integration/blueprint_edit.rs +++ b/nexus/reconfigurator/cli-integration-tests/tests/integration/blueprint_edit.rs @@ -127,12 +127,14 @@ async fn test_blueprint_edit(cptestctx: &ControlPlaneTestContext) { .planning_input .sled_lookup(SledFilter::Commissioned, sled_id) .expect("state1 has initial sled"); - assert!( - !state1 + assert_ne!( + state1 .planning_input .external_ip_policy() - .service_ip_pool_ranges() - .is_empty() + .clone() + .into_non_external_dns_ips() + .next(), + None, ); assert!(!state1.silo_names.is_empty()); assert!(!state1.external_dns_zone_names.is_empty()); diff --git a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs b/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs index 7e2bd7ce27f..78f2a99443e 100644 --- a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs +++ b/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs @@ -452,12 +452,7 @@ enum ExternalIpAllocatorError { impl ExternalIpAllocator { fn new(policy: &ExternalIpPolicy) -> Self { - let service_ip_pool_ranges = policy.service_ip_pool_ranges().to_vec(); - let external_dns_ips = policy.external_dns_ips().clone(); - let service_ip_pool_ips = service_ip_pool_ranges - .into_iter() - .flat_map(|r| r.iter()) - .filter(move |ip| !external_dns_ips.contains(ip)); + let service_ip_pool_ips = policy.clone().into_non_external_dns_ips(); Self { service_ip_pool_ips: DebugIgnore(Box::new(service_ip_pool_ips)), used_exclusive_ips: BTreeSet::new(), diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index b79aeba535c..7ce59207e58 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -3173,15 +3173,14 @@ pub(crate) mod test { // the service IP pool. This will force reuse of the IP that was // allocated to the expunged Nexus zone. let mut builder = input.into_builder(); - assert_eq!( - builder.policy_mut().external_ips.service_ip_pool_ranges().len(), - 1 - ); + let num_available_external_ips = builder + .policy_mut() + .external_ips + .clone() + .into_non_external_dns_ips() + .count(); builder.policy_mut().target_nexus_zone_count = - builder.policy_mut().external_ips.service_ip_pool_ranges()[0] - .len() - .try_into() - .unwrap(); + num_available_external_ips; let input = builder.build(); let blueprint3 = Planner::new_based_on( logctx.log.clone(), diff --git a/nexus/types/src/deployment/planning_input.rs b/nexus/types/src/deployment/planning_input.rs index 0dd2ddf29c3..618edf2299b 100644 --- a/nexus/types/src/deployment/planning_input.rs +++ b/nexus/types/src/deployment/planning_input.rs @@ -1128,11 +1128,19 @@ impl ExternalIpPolicy { ExternalIpPolicyBuilder { service_ip_pool_ranges, external_dns_ips } } - // TODO-john comment / remove? - pub fn service_ip_pool_ranges(&self) -> &[IpRange] { - &self.service_ip_pool_ranges + /// Consume this `ExternalIpPolicy`, returning an iterator over all IPs that + /// should be used for services other than external DNS (i.e., Nexus and + /// boundary NTP). + pub fn into_non_external_dns_ips(self) -> impl Iterator { + let Self { service_ip_pool_ranges, external_dns_ips } = self; + service_ip_pool_ranges + .into_iter() + .flat_map(|r| r.iter()) + .filter(move |ip| !external_dns_ips.contains(ip)) } + /// The set of external IP addresses on which we should run external DNS + /// servers. pub fn external_dns_ips(&self) -> &BTreeSet { &self.external_dns_ips } From b92fe086568b98da556889824c235bafc29eaf3e Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 27 Oct 2025 15:37:28 -0400 Subject: [PATCH 09/16] cargo fmt --- nexus/db-queries/src/db/datastore/rack.rs | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index e761832f5ee..be616bbd96f 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -1765,12 +1765,10 @@ mod test { // Ask for two Nexus services, with different external IPs. let nexus_ip_start = Ipv4Addr::new(1, 2, 3, 4); let nexus_ip_end = Ipv4Addr::new(1, 2, 3, 5); - let service_ip_pool = - IpRange::try_from((nexus_ip_start, nexus_ip_end)) - .expect("Cannot create IP Range"); - let external_ip_policy = ExternalIpPolicy::single_pool_no_external_dns( - service_ip_pool - ); + let service_ip_pool = IpRange::try_from((nexus_ip_start, nexus_ip_end)) + .expect("Cannot create IP Range"); + let external_ip_policy = + ExternalIpPolicy::single_pool_no_external_dns(service_ip_pool); let mut system = SystemDescription::new(); system @@ -2060,12 +2058,10 @@ mod test { Ipv6Addr::new(0xfd00, 0x1122, 0x3344, 0, 0, 0, 0, 1); let nexus_ip_end = Ipv6Addr::new(0xfd00, 0x1122, 0x3344, 0, 0, 0, 0, 10); - let service_ip_pool = - IpRange::try_from((nexus_ip_start, nexus_ip_end)) - .expect("Cannot create IP Range"); - let external_ip_policy = ExternalIpPolicy::single_pool_no_external_dns( - service_ip_pool - ); + let service_ip_pool = IpRange::try_from((nexus_ip_start, nexus_ip_end)) + .expect("Cannot create IP Range"); + let external_ip_policy = + ExternalIpPolicy::single_pool_no_external_dns(service_ip_pool); let mut system = SystemDescription::new(); system From 34a8be20bfce84c012a361b5dd9fc312a78b3722 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 27 Oct 2025 15:48:01 -0400 Subject: [PATCH 10/16] typo --- .../src/blueprint_editor/allocators/external_networking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs b/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs index 78f2a99443e..19d8efdb441 100644 --- a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs +++ b/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs @@ -183,7 +183,7 @@ impl ExternalNetworkingAllocator { // External DNS IPs are special: // // 1. Check that we don't have any in-use external DNS IPs that we don't - // expect. Tere should be no way for this to happen at the time of + // expect. There should be no way for this to happen at the time of // this writing, because there's no way to change the set of external // DNS IPs after RSS. Once we add a way to change it, presumably our // caller should handle this somehow (e.g., the planner should From 3f3f370a450238da5290a4364ae72d31076d91bf Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Mon, 27 Oct 2025 16:20:45 -0400 Subject: [PATCH 11/16] rustdoc linkify --- .../src/db/datastore/deployment/external_networking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/datastore/deployment/external_networking.rs b/nexus/db-queries/src/db/datastore/deployment/external_networking.rs index 33ecd0d9200..c0eb63be21d 100644 --- a/nexus/db-queries/src/db/datastore/deployment/external_networking.rs +++ b/nexus/db-queries/src/db/datastore/deployment/external_networking.rs @@ -43,7 +43,7 @@ impl DataStore { /// external DNS that an operator can update. Today, we do not: whatever /// external DNS IPs are provided at rack setup time are the IPs we use /// forever. (Fixing this is tracked by - /// https://github.com/oxidecomputer/omicron/issues/8255.) + /// .) pub async fn external_dns_external_ips_specified_by_rack_setup( &self, opctx: &OpContext, From a7edfe5e01165958c11b07905cba52b6c5ace45a Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Tue, 28 Oct 2025 10:42:52 -0400 Subject: [PATCH 12/16] test fix --- nexus/reconfigurator/planning/src/planner.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index df04c8e1fc3..16e14a5355c 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -3022,7 +3022,9 @@ pub(crate) mod test { Err(err) => { let err = InlineErrorChain::new(&err).to_string(); assert!( - err.contains("error allocating internal DNS"), + err.contains( + "no reserved subnets available for internal DNS" + ), "unexpected error: {err}" ); } From c385f6d85a893fbc50b1da4bc9153860a6a30f86 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Tue, 28 Oct 2025 11:58:51 -0400 Subject: [PATCH 13/16] clean up ExternalIpPolicy confusion about pools and ranges --- common/src/address.rs | 11 - .../planning/src/blueprint_builder/builder.rs | 2 +- .../allocators/external_networking.rs | 19 +- nexus/reconfigurator/planning/src/example.rs | 8 +- nexus/reconfigurator/planning/src/planner.rs | 16 +- nexus/reconfigurator/planning/src/system.rs | 8 +- nexus/reconfigurator/preparation/src/lib.rs | 2 +- nexus/types/src/deployment/planning_input.rs | 203 +++++++++++++----- 8 files changed, 179 insertions(+), 90 deletions(-) diff --git a/common/src/address.rs b/common/src/address.rs index bba7548fe59..2cc89deee30 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -524,17 +524,6 @@ impl IpRange { IpRange::V6(_) => IpVersion::V6, } } - - /// Returns `true` if `self` has any IPs in common with `other`; false - /// otherwise. - pub fn overlaps(&self, other: &IpRange) -> bool { - match (self, other) { - (IpRange::V4(r0), IpRange::V4(r1)) => r0.overlaps(r1), - (IpRange::V6(r0), IpRange::V6(r1)) => r0.overlaps(r1), - (IpRange::V4(_), IpRange::V6(_)) - | (IpRange::V6(_), IpRange::V4(_)) => false, - } - } } impl From for IpRange { diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index 9bfe95cfd89..5a31605b2e8 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -3531,7 +3531,7 @@ pub mod test { builder.policy_mut().external_ips = { let mut ip_policy = ExternalIpPolicy::builder(); for r in used_ip_ranges { - ip_policy.push_service_ip_pool(r).unwrap(); + ip_policy.push_service_pool_range(r).unwrap(); } ip_policy.build() }; diff --git a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs b/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs index 19d8efdb441..769dc1f44ff 100644 --- a/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs +++ b/nexus/reconfigurator/planning/src/blueprint_editor/allocators/external_networking.rs @@ -663,6 +663,7 @@ pub mod test { use nexus_types::inventory::NetworkInterface; use nexus_types::inventory::NetworkInterfaceKind; use omicron_common::address::IpRange; + use omicron_common::address::Ipv4Range; use omicron_common::api::external::Vni; use omicron_uuid_kinds::ExternalIpUuid; use omicron_uuid_kinds::GenericUuid; @@ -754,7 +755,7 @@ pub mod test { let policy = { let mut builder = ExternalIpPolicy::builder(); for r in ip_pool_ranges { - builder.push_service_ip_pool(r).unwrap(); + builder.push_service_pool_range(r).unwrap(); } builder.build() }; @@ -860,16 +861,16 @@ pub mod test { fn external_dns_ips_are_partitioned_separately() { // Construct a service IP range with 3 IPs. The first two are for // external DNS, and the third is for other services. - let service_ip_pool = IpRange::try_from(( - "192.0.2.1".parse::().unwrap(), - "192.0.2.3".parse::().unwrap(), - )) + let service_ip_pool = Ipv4Range::new( + "192.0.2.1".parse::().unwrap(), + "192.0.2.3".parse::().unwrap(), + ) .unwrap(); assert_eq!(service_ip_pool.len(), 3); let external_ip_policy = { let mut builder = ExternalIpPolicy::builder(); - builder.push_service_ip_pool(service_ip_pool).unwrap(); + builder.push_service_pool_ipv4_range(service_ip_pool).unwrap(); builder.add_external_dns_ip("192.0.2.1".parse().unwrap()).unwrap(); builder.add_external_dns_ip("192.0.2.2".parse().unwrap()).unwrap(); builder.build() @@ -889,7 +890,11 @@ pub mod test { dns_address: OmicronZoneExternalFloatingAddr { id: ExternalIpUuid::new_v4(), addr: SocketAddr::new( - service_ip_pool.iter().nth(index).unwrap(), + service_ip_pool + .iter() + .nth(index) + .unwrap() + .into(), 0, ), }, diff --git a/nexus/reconfigurator/planning/src/example.rs b/nexus/reconfigurator/planning/src/example.rs index 9eed1317b23..62456b7b4af 100644 --- a/nexus/reconfigurator/planning/src/example.rs +++ b/nexus/reconfigurator/planning/src/example.rs @@ -36,7 +36,7 @@ use nexus_types::deployment::SledFilter; use nexus_types::deployment::TargetReleaseDescription; use nexus_types::external_api::views::SledPolicy; use nexus_types::inventory::Collection; -use omicron_common::address::IpRange; +use omicron_common::address::Ipv4Range; use omicron_common::api::external::TufRepoDescription; use omicron_common::policy::CRUCIBLE_PANTRY_REDUNDANCY; use omicron_common::policy::INTERNAL_DNS_REDUNDANCY; @@ -507,11 +507,11 @@ impl ExampleSystemBuilder { let mut builder = system.external_ip_policy().clone().into_builder(); builder - .push_service_ip_pool( - IpRange::try_from(( + .push_service_pool_ipv4_range( + Ipv4Range::new( "198.51.100.1".parse::().unwrap(), "198.51.100.30".parse::().unwrap(), - )) + ) .unwrap(), ) .unwrap(); diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index ee87ef2ad27..6c59470b112 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -2482,7 +2482,7 @@ pub(crate) mod test { use nexus_types::inventory::CockroachStatus; use nexus_types::inventory::InternalDnsGenerationStatus; use nexus_types::inventory::TimeSync; - use omicron_common::address::IpRange; + use omicron_common::address::Ipv4Range; use omicron_common::api::external::Generation; use omicron_common::api::external::MacAddr; use omicron_common::api::external::TufArtifactMeta; @@ -2509,6 +2509,7 @@ pub(crate) mod test { use std::collections::BTreeMap; use std::collections::HashMap; use std::net::IpAddr; + use std::net::Ipv4Addr; use std::net::Ipv6Addr; use tufaceous_artifact::ArtifactHash; use tufaceous_artifact::ArtifactKind; @@ -3273,7 +3274,7 @@ pub(crate) mod test { // Change the policy: add some external DNS IPs. let external_dns_ips = ["10.0.0.1", "10.0.0.2", "10.0.0.3"].map(|addr| { - addr.parse::() + addr.parse::() .expect("can't parse external DNS IP address") }); let input = { @@ -3282,17 +3283,14 @@ pub(crate) mod test { builder.policy_mut().external_ips.clone().into_builder(); // Add a "service IP pool" covering our external DNS IP range. ip_policy - .push_service_ip_pool( - IpRange::try_from(( - external_dns_ips[0], - external_dns_ips[2], - )) - .unwrap(), + .push_service_pool_ipv4_range( + Ipv4Range::new(external_dns_ips[0], external_dns_ips[2]) + .unwrap(), ) .unwrap(); // Set these IPs as "for external DNS". for ip in external_dns_ips { - ip_policy.add_external_dns_ip(ip).unwrap(); + ip_policy.add_external_dns_ip(ip.into()).unwrap(); } builder.policy_mut().external_ips = ip_policy.build(); builder.build() diff --git a/nexus/reconfigurator/planning/src/system.rs b/nexus/reconfigurator/planning/src/system.rs index 2852c009cbe..d9fc473940a 100644 --- a/nexus/reconfigurator/planning/src/system.rs +++ b/nexus/reconfigurator/planning/src/system.rs @@ -54,7 +54,7 @@ use nexus_types::inventory::CabooseWhich; use nexus_types::inventory::PowerState; use nexus_types::inventory::RotSlot; use nexus_types::inventory::SpType; -use omicron_common::address::IpRange; +use omicron_common::address::Ipv4Range; use omicron_common::address::Ipv6Subnet; use omicron_common::address::RACK_PREFIX; use omicron_common::address::SLED_PREFIX; @@ -187,11 +187,11 @@ impl SystemDescription { let external_ip_policy = { let mut builder = ExternalIpPolicy::builder(); builder - .push_service_ip_pool( - IpRange::try_from(( + .push_service_pool_ipv4_range( + Ipv4Range::new( "192.0.2.2".parse::().unwrap(), "192.0.2.20".parse::().unwrap(), - )) + ) .unwrap(), ) .unwrap(); diff --git a/nexus/reconfigurator/preparation/src/lib.rs b/nexus/reconfigurator/preparation/src/lib.rs index 31914daebdf..3330832b3b8 100644 --- a/nexus/reconfigurator/preparation/src/lib.rs +++ b/nexus/reconfigurator/preparation/src/lib.rs @@ -277,7 +277,7 @@ impl PlanningInputFromDb<'_> { InlineErrorChain::new(&e), )) })?; - builder.push_service_ip_pool(range).map_err(|e| { + builder.push_service_pool_range(range).map_err(|e| { Error::internal_error(&format!( "cannot construct external IP policy: {}", InlineErrorChain::new(&e), diff --git a/nexus/types/src/deployment/planning_input.rs b/nexus/types/src/deployment/planning_input.rs index 618edf2299b..75970e81b1f 100644 --- a/nexus/types/src/deployment/planning_input.rs +++ b/nexus/types/src/deployment/planning_input.rs @@ -25,6 +25,8 @@ use daft::Diffable; use ipnetwork::IpNetwork; use nexus_sled_agent_shared::inventory::ZoneKind; use omicron_common::address::IpRange; +use omicron_common::address::Ipv4Range; +use omicron_common::address::Ipv6Range; use omicron_common::address::Ipv6Subnet; use omicron_common::address::SLED_PREFIX; use omicron_common::api::external::Generation; @@ -1045,17 +1047,26 @@ pub struct Policy { } /// Subset of [`Policy`] specific to external IP addresses. +/// +/// Today, this type encompasses three logical parts of a policy: +/// +/// * A single IPv4 service IP pool, made up of 0 or more ranges. +/// * A single IPv6 service IP pool, made up of 0 or more ranges. +/// * A set of external DNS IP addresses, each of which must be present in one +/// of the two service IP pools. +/// +/// This is slightly more general than what deployed racks actually have: they +/// all only populate the IPv4 service pool and external DNS IP addresses. But +/// `#[nexus_test]` uses all of the above, and this a step in the direction +/// we're moving where we will have more than one service IP pool (see +/// ). // NOTE: The fields of the struct are private and we manually implement // `Deserialize` to maintain invariants (e.g., that every `external_dns_ip` is -// contained in one of the `service_ip_pool_ranges`). +// contained in one of the service pool ranges). #[derive(Debug, Clone, Serialize)] pub struct ExternalIpPolicy { - /// ranges specified by the IP pool for externally-visible control plane - /// services (e.g., external DNS, Nexus, boundary NTP) - service_ip_pool_ranges: Vec, - - /// specific IP addresses contained in `service_ip_pool_ranges` that are - /// earmarked for external DNS + service_pool_ipv4_ranges: Vec, + service_pool_ipv6_ranges: Vec, external_dns_ips: BTreeSet, } @@ -1075,18 +1086,29 @@ impl<'de> Deserialize<'de> for ExternalIpPolicy { #[derive(Deserialize)] #[serde(remote = "ExternalIpPolicy")] struct ExternalIpPolicyShadow { - service_ip_pool_ranges: Vec, + service_pool_ipv4_ranges: Vec, + service_pool_ipv6_ranges: Vec, external_dns_ips: BTreeSet, } // Deserialize without validation... - let ExternalIpPolicy { service_ip_pool_ranges, external_dns_ips } = - ExternalIpPolicyShadow::deserialize(deserializer)?; + let ExternalIpPolicy { + service_pool_ipv4_ranges, + service_pool_ipv6_ranges, + external_dns_ips, + } = ExternalIpPolicyShadow::deserialize(deserializer)?; // ...then validate by going through the builder. let mut builder = ExternalIpPolicy::builder(); - for pool in service_ip_pool_ranges { - builder.push_service_ip_pool(pool).map_err(D::Error::custom)?; + for r in service_pool_ipv4_ranges { + builder + .push_service_pool_ipv4_range(r) + .map_err(D::Error::custom)?; + } + for r in service_pool_ipv6_ranges { + builder + .push_service_pool_ipv6_range(r) + .map_err(D::Error::custom)?; } for ip in external_dns_ips { builder.add_external_dns_ip(ip).map_err(D::Error::custom)?; @@ -1099,19 +1121,25 @@ impl<'de> Deserialize<'de> for ExternalIpPolicy { impl ExternalIpPolicy { pub fn empty() -> Self { Self { - service_ip_pool_ranges: Vec::new(), + service_pool_ipv4_ranges: Vec::new(), + service_pool_ipv6_ranges: Vec::new(), external_dns_ips: BTreeSet::new(), } } - /// Construct an `ExternalIpPolicy` containing a single service IP pool and - /// no external DNS IPs. + /// Construct an `ExternalIpPolicy` containing a single service IP pool + /// range and no external DNS IPs. /// /// This is used primarily by tests. A single pool with no external DNS IPs /// can be constructed infallibly. pub fn single_pool_no_external_dns(range: IpRange) -> Self { + let (service_pool_ipv4_ranges, service_pool_ipv6_ranges) = match range { + IpRange::V4(r) => (vec![r], vec![]), + IpRange::V6(r) => (vec![], vec![r]), + }; Self { - service_ip_pool_ranges: vec![range], + service_pool_ipv4_ranges, + service_pool_ipv6_ranges, external_dns_ips: BTreeSet::new(), } } @@ -1124,19 +1152,36 @@ impl ExternalIpPolicy { /// Construct an [`ExternalIpPolicyBuilder`] that contains all of this /// policy's IP pools and external DNS IPs. pub fn into_builder(self) -> ExternalIpPolicyBuilder { - let Self { service_ip_pool_ranges, external_dns_ips } = self; - ExternalIpPolicyBuilder { service_ip_pool_ranges, external_dns_ips } + let Self { + service_pool_ipv4_ranges, + service_pool_ipv6_ranges, + external_dns_ips, + } = self; + ExternalIpPolicyBuilder { + service_pool_ipv4_ranges, + service_pool_ipv6_ranges, + external_dns_ips, + } } /// Consume this `ExternalIpPolicy`, returning an iterator over all IPs that /// should be used for services other than external DNS (i.e., Nexus and /// boundary NTP). pub fn into_non_external_dns_ips(self) -> impl Iterator { - let Self { service_ip_pool_ranges, external_dns_ips } = self; - service_ip_pool_ranges + let Self { + service_pool_ipv4_ranges, + service_pool_ipv6_ranges, + external_dns_ips, + } = self; + + let v4_ips = service_pool_ipv4_ranges .into_iter() - .flat_map(|r| r.iter()) - .filter(move |ip| !external_dns_ips.contains(ip)) + .flat_map(|r| r.iter().map(IpAddr::V4)); + let v6_ips = service_pool_ipv6_ranges + .into_iter() + .flat_map(|r| r.iter().map(IpAddr::V6)); + + v4_ips.chain(v6_ips).filter(move |ip| !external_dns_ips.contains(ip)) } /// The set of external IP addresses on which we should run external DNS @@ -1148,7 +1193,8 @@ impl ExternalIpPolicy { #[derive(Debug, Clone, Default)] pub struct ExternalIpPolicyBuilder { - service_ip_pool_ranges: Vec, + service_pool_ipv4_ranges: Vec, + service_pool_ipv6_ranges: Vec, external_dns_ips: BTreeSet, } @@ -1158,30 +1204,65 @@ impl ExternalIpPolicyBuilder { Self::default() } - /// Add an external IP pool for services. + /// Add a range to either the IPv4 or the IPv6 service pool, depending on + /// the variant of `new_range`. + /// + /// # Errors + /// + /// Fails if `new_range` overlaps an existing range within this builder. + pub fn push_service_pool_range( + &mut self, + new_range: IpRange, + ) -> Result<(), ExternalIpPolicyError> { + match new_range { + IpRange::V4(r) => self.push_service_pool_ipv4_range(r), + IpRange::V6(r) => self.push_service_pool_ipv6_range(r), + } + } + + /// Add a range to the IPv4 service pool. /// /// # Errors /// - /// Fails if `pool_range` overlaps an existing pool within this builder. - // TODO-correctness Eventually this may be broken up into different pools - // for different kinds of services. For now, we group all services into a - // single set of pools and then separately carve out external DNS from - // within those pools. - pub fn push_service_ip_pool( + /// Fails if `new_range` overlaps an existing range within this builder. + pub fn push_service_pool_ipv4_range( &mut self, - new_pool: IpRange, + new_range: Ipv4Range, ) -> Result<(), ExternalIpPolicyError> { - // Linear scan should be good enough: we don't expect many pools, and - // the check we do for each pool is cheap. - for &existing_pool in &self.service_ip_pool_ranges { - if existing_pool.overlaps(&new_pool) { - return Err(ExternalIpPolicyError::OverlappingPools { - new_pool, - existing_pool, + // Linear scan should be good enough: we don't expect many ranges, and + // the check we do for each range is cheap. + for &existing_range in &self.service_pool_ipv4_ranges { + if existing_range.overlaps(&new_range) { + return Err(ExternalIpPolicyError::OverlappingRanges { + new_range: new_range.into(), + existing_range: new_range.into(), }); } } - self.service_ip_pool_ranges.push(new_pool); + self.service_pool_ipv4_ranges.push(new_range); + Ok(()) + } + + /// Add a range to the IPv6 service pool. + /// + /// # Errors + /// + /// Fails if `new_range` overlaps an existing range within this builder. + pub fn push_service_pool_ipv6_range( + &mut self, + new_range: Ipv6Range, + ) -> Result<(), ExternalIpPolicyError> { + // Linear scan should be good enough: we don't expect many ranges, and + // the check we do for each range is cheap. + for &existing_range in &self.service_pool_ipv6_ranges { + if existing_range.overlaps(&new_range) { + return Err(ExternalIpPolicyError::OverlappingRanges { + new_range: new_range.into(), + existing_range: new_range.into(), + }); + } + } + self.service_pool_ipv6_ranges.push(new_range); Ok(()) } @@ -1191,15 +1272,23 @@ impl ExternalIpPolicyBuilder { /// /// Fails if the requested IP is _not_ covered by an existing service IP /// pool range. - // TODO-correctness See the note above on `push_service_ip_pool()`; the fact - // that external DNS IPs are not special or distinct from the general - // service IP pools is a little confusing, and it would be nice to clean - // that up eventually. + // TODO-correctness The fact that external DNS IPs are not special or + // distinct from the general service IP pools is a little confusing, and it + // would be nice to clean that up eventually. We may need to do that as a + // part of the work to move to multiple service pools. pub fn add_external_dns_ip( &mut self, ip: IpAddr, ) -> Result<(), ExternalIpPolicyError> { - if self.service_ip_pool_ranges.iter().any(|pool| pool.contains(ip)) { + let contained_in_pool = match ip { + IpAddr::V4(ip) => { + self.service_pool_ipv4_ranges.iter().any(|r| r.contains(ip)) + } + IpAddr::V6(ip) => { + self.service_pool_ipv6_ranges.iter().any(|r| r.contains(ip)) + } + }; + if contained_in_pool { self.external_dns_ips.insert(ip); Ok(()) } else { @@ -1208,22 +1297,30 @@ impl ExternalIpPolicyBuilder { } pub fn build(self) -> ExternalIpPolicy { - let Self { service_ip_pool_ranges, external_dns_ips } = self; - ExternalIpPolicy { service_ip_pool_ranges, external_dns_ips } + let Self { + service_pool_ipv4_ranges, + service_pool_ipv6_ranges, + external_dns_ips, + } = self; + ExternalIpPolicy { + service_pool_ipv4_ranges, + service_pool_ipv6_ranges, + external_dns_ips, + } } } #[derive(Debug, thiserror::Error)] pub enum ExternalIpPolicyError { #[error( - "external IP policy cannot have overlapping pools: \ - [{}, {}] overlaps [{}, {}]", - new_pool.first_address(), - new_pool.last_address(), - existing_pool.first_address(), - existing_pool.last_address(), + "external IP policy cannot have overlapping IP ranges: \ + new range [{}, {}] overlaps existing range [{}, {}]", + new_range.first_address(), + new_range.last_address(), + existing_range.first_address(), + existing_range.last_address(), )] - OverlappingPools { new_pool: IpRange, existing_pool: IpRange }, + OverlappingRanges { new_range: IpRange, existing_range: IpRange }, #[error("external DNS IP {0} is not a member of any service IP pool")] ExternalDnsOutsideServiceIpPools(IpAddr), } From 9aea90e453611ed39ff7c71c3a45fdf23feb6a78 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Tue, 28 Oct 2025 12:00:00 -0400 Subject: [PATCH 14/16] typo --- .../src/db/datastore/deployment/external_networking.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/deployment/external_networking.rs b/nexus/db-queries/src/db/datastore/deployment/external_networking.rs index c0eb63be21d..fd157bccc51 100644 --- a/nexus/db-queries/src/db/datastore/deployment/external_networking.rs +++ b/nexus/db-queries/src/db/datastore/deployment/external_networking.rs @@ -52,9 +52,9 @@ impl DataStore { // during rack setup by examining the current target blueprint and // looking at the IPs of all of its external DNS zones. We _must_ // include expunged zones as well as in-service zones: during an update, - // we'll create a blueprint that expunges an external DNS zones, waits + // we'll create a blueprint that expunges an external DNS zone, waits // for it to go away, then wants to reassign that zone's external IP to - // a new external DNS zones. But because we are scanning expunged zones, + // a new external DNS zone. But because we are scanning expunged zones, // we also have to allow for duplicates - this isn't an error and is // expected if we've performed more than one update, at least until we // start pruning old expunged zones out of the blueprint (tracked by From 60ff5b35187539d3595481a3197e9247fc1e0bbc Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Tue, 28 Oct 2025 14:15:01 -0400 Subject: [PATCH 15/16] expectorate --- dev-tools/reconfigurator-cli/tests/output/cmds-stdout | 2 +- .../reconfigurator-cli/tests/output/cmds-target-release-stdout | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dev-tools/reconfigurator-cli/tests/output/cmds-stdout b/dev-tools/reconfigurator-cli/tests/output/cmds-stdout index c69ad975e58..e8b77360734 100644 --- a/dev-tools/reconfigurator-cli/tests/output/cmds-stdout +++ b/dev-tools/reconfigurator-cli/tests/output/cmds-stdout @@ -871,7 +871,7 @@ result: loaded sleds: 04ef3330-c682-4a08-8def-fcc4bef31bcd, 90c1102a-b9f5-4d88-92a2-60d54a2d98cc, dde1c0e2-b10d-4621-b420-f179f7a7a00a loaded collections: 6e066695-94bc-4250-bd63-fd799c166cc1 loaded blueprints: (none) - loaded external IP policy: ExternalIpPolicy { service_ip_pool_ranges: [V4(Ipv4Range { first: 192.0.2.2, last: 192.0.2.20 })], external_dns_ips: {} } + loaded external IP policy: ExternalIpPolicy { service_pool_ipv4_ranges: [Ipv4Range { first: 192.0.2.2, last: 192.0.2.20 }], service_pool_ipv6_ranges: [], external_dns_ips: {} } loaded internal DNS generations: (none) loaded external DNS generations: (none) config: diff --git a/dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout b/dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout index 621bf8c2bcd..8fc2c633275 100644 --- a/dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout +++ b/dev-tools/reconfigurator-cli/tests/output/cmds-target-release-stdout @@ -1038,7 +1038,7 @@ result: loaded sleds: 2b8f0cb3-0295-4b3c-bc58-4fe88b57112c, 98e6b7c2-2efa-41ca-b20a-0a4d61102fe6, d81c6a84-79b8-4958-ae41-ea46c9b19763 loaded collections: f45ba181-4b56-42cc-a762-874d90184a43, eb0796d5-ab8a-4f7b-a884-b4aeacb8ab51, 61f451b3-2121-4ed6-91c7-a550054f6c21 loaded blueprints: 184f10b3-61cb-41ef-9b93-3489b2bac559, dbcbd3d6-41ff-48ae-ac0b-1becc9b2fd21, 8da82a8e-bf97-4fbd-8ddd-9f6462732cf1, 58d5e830-0884-47d8-a7cd-b2b3751adeb4 - loaded external IP policy: ExternalIpPolicy { service_ip_pool_ranges: [V4(Ipv4Range { first: 192.0.2.2, last: 192.0.2.20 }), V4(Ipv4Range { first: 198.51.100.1, last: 198.51.100.30 })], external_dns_ips: {198.51.100.1, 198.51.100.2, 198.51.100.3} } + loaded external IP policy: ExternalIpPolicy { service_pool_ipv4_ranges: [Ipv4Range { first: 192.0.2.2, last: 192.0.2.20 }, Ipv4Range { first: 198.51.100.1, last: 198.51.100.30 }], service_pool_ipv6_ranges: [], external_dns_ips: {198.51.100.1, 198.51.100.2, 198.51.100.3} } loaded internal DNS generations: (none) loaded external DNS generations: (none) config: From 31572a54fb474a644bc1d9deb44da890a647916f Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 5 Nov 2025 10:07:43 -0500 Subject: [PATCH 16/16] typo --- .../src/db/datastore/deployment/external_networking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/db-queries/src/db/datastore/deployment/external_networking.rs b/nexus/db-queries/src/db/datastore/deployment/external_networking.rs index fd157bccc51..567e5fcfc57 100644 --- a/nexus/db-queries/src/db/datastore/deployment/external_networking.rs +++ b/nexus/db-queries/src/db/datastore/deployment/external_networking.rs @@ -48,7 +48,7 @@ impl DataStore { &self, opctx: &OpContext, ) -> Result, Error> { - // We can _implicitly_ determine the set of external DNS IPs provied + // We can _implicitly_ determine the set of external DNS IPs provided // during rack setup by examining the current target blueprint and // looking at the IPs of all of its external DNS zones. We _must_ // include expunged zones as well as in-service zones: during an update,