From 047e16b88612e5dc2af09d25d7270a205866acd2 Mon Sep 17 00:00:00 2001 From: John Gallagher Date: Wed, 15 May 2024 11:35:48 -0400 Subject: [PATCH] blueprint builder: check for unknown external networkign in input --- .../planning/src/blueprint_builder/builder.rs | 61 +++++++++++++++++- .../blueprint_builder/external_networking.rs | 62 +++++++++++++++++++ nexus/reconfigurator/planning/src/planner.rs | 39 ++++++++++++ .../types/src/deployment/network_resources.rs | 24 +++++-- nexus/types/src/deployment/planning_input.rs | 6 ++ nexus/types/src/deployment/tri_map.rs | 4 ++ 6 files changed, 188 insertions(+), 8 deletions(-) diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs index a75a8fc2a5..67bbd40da3 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/builder.rs @@ -1044,10 +1044,12 @@ pub mod test { use crate::system::SledBuilder; use expectorate::assert_contents; use nexus_types::deployment::BlueprintZoneFilter; + use nexus_types::deployment::OmicronZoneNetworkResources; use nexus_types::external_api::views::SledPolicy; use omicron_common::address::IpRange; use omicron_test_utils::dev::test_setup_log; use std::collections::BTreeSet; + use std::mem; pub const DEFAULT_N_SLEDS: usize = 3; @@ -1332,6 +1334,14 @@ pub mod test { static TEST_NAME: &str = "blueprint_builder_test_add_physical_disks"; let logctx = test_setup_log(TEST_NAME); let (_, input, _) = example(&logctx.log, TEST_NAME, DEFAULT_N_SLEDS); + let input = { + // Clear out the external networking records from `input`, since + // we're building an empty blueprint. + let mut builder = input.into_builder(); + *builder.network_resources_mut() = + OmicronZoneNetworkResources::new(); + builder.build() + }; // Start with an empty blueprint (sleds with no zones). let parent = BlueprintBuilder::build_empty_with_sleds_seeded( @@ -1380,6 +1390,14 @@ pub mod test { // Discard the example blueprint and start with an empty one. let (collection, input, _) = example(&logctx.log, TEST_NAME, DEFAULT_N_SLEDS); + let input = { + // Clear out the external networking records from `input`, since + // we're building an empty blueprint. + let mut builder = input.into_builder(); + *builder.network_resources_mut() = + OmicronZoneNetworkResources::new(); + builder.build() + }; let parent = BlueprintBuilder::build_empty_with_sleds_seeded( input.all_sled_ids(SledFilter::Commissioned), "test", @@ -1421,7 +1439,7 @@ pub mod test { fn test_add_nexus_error_cases() { static TEST_NAME: &str = "blueprint_builder_test_add_nexus_error_cases"; let logctx = test_setup_log(TEST_NAME); - let (mut collection, input, mut parent) = + let (mut collection, mut input, mut parent) = example(&logctx.log, TEST_NAME, DEFAULT_N_SLEDS); // Remove the Nexus zone from one of the sleds so that @@ -1435,12 +1453,51 @@ pub mod test { if zones.zones.zones.len() < nzones_before_retain { selected_sled_id = Some(*sled_id); // Also remove this zone from the blueprint. + let mut removed_nexus = None; parent .blueprint_zones .get_mut(sled_id) .expect("missing sled") .zones - .retain(|z| !z.zone_type.is_nexus()); + .retain(|z| match &z.zone_type { + BlueprintZoneType::Nexus(z) => { + removed_nexus = Some(z.clone()); + false + } + _ => true, + }); + let removed_nexus = + removed_nexus.expect("removed Nexus from blueprint"); + + // Also remove this Nexus's external networking resources + // from `input`. + let mut builder = input.into_builder(); + let mut new_network_resources = + OmicronZoneNetworkResources::new(); + let old_network_resources = builder.network_resources_mut(); + for ip in old_network_resources.omicron_zone_external_ips() + { + if ip.ip.id() != removed_nexus.external_ip.id { + new_network_resources + .add_external_ip(ip.zone_id, ip.ip) + .expect("copied IP to new input"); + } + } + for nic in old_network_resources.omicron_zone_nics() { + if nic.nic.id.into_untyped_uuid() + != removed_nexus.nic.id + { + new_network_resources + .add_nic(nic.zone_id, nic.nic) + .expect("copied NIC to new input"); + } + } + mem::swap( + old_network_resources, + &mut new_network_resources, + ); + input = builder.build(); + break; } } diff --git a/nexus/reconfigurator/planning/src/blueprint_builder/external_networking.rs b/nexus/reconfigurator/planning/src/blueprint_builder/external_networking.rs index 8e82a023ea..73963d5171 100644 --- a/nexus/reconfigurator/planning/src/blueprint_builder/external_networking.rs +++ b/nexus/reconfigurator/planning/src/blueprint_builder/external_networking.rs @@ -10,8 +10,12 @@ use nexus_types::deployment::Blueprint; use nexus_types::deployment::BlueprintZoneFilter; use nexus_types::deployment::BlueprintZoneType; use nexus_types::deployment::PlanningInput; +use omicron_common::address::DNS_OPTE_IPV4_SUBNET; +use omicron_common::address::DNS_OPTE_IPV6_SUBNET; use omicron_common::address::NEXUS_OPTE_IPV4_SUBNET; use omicron_common::address::NEXUS_OPTE_IPV6_SUBNET; +use omicron_common::address::NTP_OPTE_IPV4_SUBNET; +use omicron_common::address::NTP_OPTE_IPV6_SUBNET; use omicron_common::api::external::IpNet; use omicron_common::api::external::MacAddr; use std::collections::HashSet; @@ -117,6 +121,64 @@ impl<'a> BuilderExternalNetworking<'a> { } } + // Check the planning input: there shouldn't be any external networking + // resources in the database (the source of `input`) that we don't know + // about from the parent blueprint. + for external_ip_entry in + input.network_resources().omicron_zone_external_ips() + { + if !used_external_ips.contains(&external_ip_entry.ip.ip()) { + bail!( + "planning input contains unexpected external IP \ + (IP not found in parent blueprint): {external_ip_entry:?}" + ); + } + } + for nic_entry in input.network_resources().omicron_zone_nics() { + if !used_macs.contains(&nic_entry.nic.mac) { + bail!( + "planning input contains unexpected NIC \ + (MAC not found in parent blueprint): {nic_entry:?}" + ); + } + match nic_entry.nic.ip { + IpAddr::V4(ip) if NEXUS_OPTE_IPV4_SUBNET.contains(ip) => { + if !existing_nexus_v4_ips.contains(&ip) { + bail!( + "planning input contains unexpected NIC \ + (IP not found in parent blueprint): {nic_entry:?}" + ); + } + } + IpAddr::V4(ip) if NTP_OPTE_IPV4_SUBNET.contains(ip) => { + // TODO check existing_ntp_v4_ips, once it exists + } + IpAddr::V4(ip) if DNS_OPTE_IPV4_SUBNET.contains(ip) => { + // TODO check existing_dns_v4_ips, once it exists + } + IpAddr::V6(ip) if NEXUS_OPTE_IPV6_SUBNET.contains(ip) => { + if !existing_nexus_v6_ips.contains(&ip) { + bail!( + "planning input contains unexpected NIC \ + (IP not found in parent blueprint): {nic_entry:?}" + ); + } + } + IpAddr::V6(ip) if NTP_OPTE_IPV6_SUBNET.contains(ip) => { + // TODO check existing_ntp_v6_ips, once it exists + } + IpAddr::V6(ip) if DNS_OPTE_IPV6_SUBNET.contains(ip) => { + // TODO check existing_dns_v6_ips, once it exists + } + _ => { + bail!( + "planning input contains unexpected NIC \ + (IP not contained in known OPTE subnet): {nic_entry:?}" + ) + } + } + } + // TODO-performance Building these iterators as "walk through the list // and skip anything we've used already" is fine as long as we're // talking about a small number of resources (e.g., single-digit number diff --git a/nexus/reconfigurator/planning/src/planner.rs b/nexus/reconfigurator/planning/src/planner.rs index 5535b28910..d40832b2b2 100644 --- a/nexus/reconfigurator/planning/src/planner.rs +++ b/nexus/reconfigurator/planning/src/planner.rs @@ -504,6 +504,7 @@ mod test { use nexus_types::deployment::BlueprintZoneFilter; use nexus_types::deployment::BlueprintZoneType; use nexus_types::deployment::DiffSledModified; + use nexus_types::deployment::OmicronZoneNetworkResources; use nexus_types::external_api::views::SledPolicy; use nexus_types::external_api::views::SledProvisionPolicy; use nexus_types::external_api::views::SledState; @@ -511,9 +512,11 @@ mod test { use omicron_common::api::external::Generation; use omicron_common::disk::DiskIdentity; use omicron_test_utils::dev::test_setup_log; + use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::PhysicalDiskUuid; use omicron_uuid_kinds::ZpoolUuid; use std::collections::HashMap; + use std::mem; /// Runs through a basic sequence of blueprints for adding a sled #[test] @@ -723,6 +726,42 @@ mod test { assert_eq!(collection.omicron_zones.len(), 1); blueprint.blueprint_zones.retain(|k, _v| keep_sled_id == *k); + // Also remove all the networking resources for the zones we just + // stripped out; i.e., only keep those for `keep_sled_id`. + let mut new_network_resources = OmicronZoneNetworkResources::new(); + let old_network_resources = builder.network_resources_mut(); + for old_ip in old_network_resources.omicron_zone_external_ips() { + if blueprint.all_omicron_zones(BlueprintZoneFilter::All).any( + |(_, zone)| { + zone.zone_type + .external_networking() + .map(|(ip, _nic)| ip.id() == old_ip.ip.id()) + .unwrap_or(false) + }, + ) { + new_network_resources + .add_external_ip(old_ip.zone_id, old_ip.ip) + .expect("copied IP to new input"); + } + } + for old_nic in old_network_resources.omicron_zone_nics() { + if blueprint.all_omicron_zones(BlueprintZoneFilter::All).any( + |(_, zone)| { + zone.zone_type + .external_networking() + .map(|(_ip, nic)| { + nic.id == old_nic.nic.id.into_untyped_uuid() + }) + .unwrap_or(false) + }, + ) { + new_network_resources + .add_nic(old_nic.zone_id, old_nic.nic) + .expect("copied NIC to new input"); + } + } + mem::swap(old_network_resources, &mut &mut new_network_resources); + (keep_sled_id, blueprint, collection, builder.build()) }; diff --git a/nexus/types/src/deployment/network_resources.rs b/nexus/types/src/deployment/network_resources.rs index 15f495d87a..c93e604af9 100644 --- a/nexus/types/src/deployment/network_resources.rs +++ b/nexus/types/src/deployment/network_resources.rs @@ -59,6 +59,18 @@ impl OmicronZoneNetworkResources { } } + pub fn omicron_zone_external_ips( + &self, + ) -> impl Iterator + '_ { + self.omicron_zone_external_ips.iter().copied() + } + + pub fn omicron_zone_nics( + &self, + ) -> impl Iterator + '_ { + self.omicron_zone_nics.iter().copied() + } + pub fn add_external_ip( &mut self, zone_id: OmicronZoneUuid, @@ -79,7 +91,7 @@ impl OmicronZoneNetworkResources { zone_id: OmicronZoneUuid, nic: OmicronZoneNic, ) -> Result<(), AddNetworkResourceError> { - let entry = OmicronZoneNicEntry { zone_id, nic: nic.clone() }; + let entry = OmicronZoneNicEntry { zone_id, nic }; self.omicron_zone_nics.insert_no_dups(entry).map_err(|err| { AddNetworkResourceError::DuplicateOmicronZoneNic { zone_id, @@ -221,7 +233,7 @@ pub struct OmicronZoneExternalSnatIp { /// /// This is a slimmer `nexus_db_model::ServiceNetworkInterface` that only stores /// the fields necessary for blueprint planning. -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, Copy, Serialize, Deserialize)] pub struct OmicronZoneNic { pub id: VnicUuid, pub mac: MacAddr, @@ -233,7 +245,7 @@ pub struct OmicronZoneNic { /// A pair of an Omicron zone ID and an external IP. /// /// Part of [`OmicronZoneNetworkResources`]. -#[derive(Clone, Debug, Deserialize, Serialize)] +#[derive(Clone, Copy, Debug, Deserialize, Serialize)] pub struct OmicronZoneExternalIpEntry { pub zone_id: OmicronZoneUuid, pub ip: OmicronZoneExternalIp, @@ -264,10 +276,10 @@ impl TriMapEntry for OmicronZoneExternalIpEntry { /// A pair of an Omicron zone ID and a network interface. /// /// Part of [`OmicronZoneNetworkResources`]. -#[derive(Clone, Debug, Deserialize, Serialize)] +#[derive(Clone, Copy, Debug, Deserialize, Serialize)] pub struct OmicronZoneNicEntry { - zone_id: OmicronZoneUuid, - nic: OmicronZoneNic, + pub zone_id: OmicronZoneUuid, + pub nic: OmicronZoneNic, } impl TriMapEntry for OmicronZoneNicEntry { diff --git a/nexus/types/src/deployment/planning_input.rs b/nexus/types/src/deployment/planning_input.rs index ccb15b858a..c8cdeec15b 100644 --- a/nexus/types/src/deployment/planning_input.rs +++ b/nexus/types/src/deployment/planning_input.rs @@ -548,6 +548,12 @@ impl PlanningInputBuilder { Ok(self.network_resources.add_nic(zone_id, nic)?) } + pub fn network_resources_mut( + &mut self, + ) -> &mut OmicronZoneNetworkResources { + &mut self.network_resources + } + pub fn policy_mut(&mut self) -> &mut Policy { &mut self.policy } diff --git a/nexus/types/src/deployment/tri_map.rs b/nexus/types/src/deployment/tri_map.rs index 52b64aec43..e4ef320b4f 100644 --- a/nexus/types/src/deployment/tri_map.rs +++ b/nexus/types/src/deployment/tri_map.rs @@ -92,6 +92,10 @@ impl TriMap { } } + pub(crate) fn iter(&self) -> impl Iterator { + self.entries.iter() + } + /// Checks general invariants of the map. /// /// The code below always upholds these invariants, but it's useful to have