diff --git a/Cargo.lock b/Cargo.lock index f9fc092188d..fcf081ea019 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6741,6 +6741,7 @@ dependencies = [ "hex", "iddqd", "ipnetwork", + "itertools 0.14.0", "macaddr", "newtype_derive", "nexus-config", diff --git a/nexus/db-model/Cargo.toml b/nexus/db-model/Cargo.toml index 58292ceffca..6769eb15f19 100644 --- a/nexus/db-model/Cargo.toml +++ b/nexus/db-model/Cargo.toml @@ -22,6 +22,7 @@ diesel = { workspace = true, features = ["postgres", "r2d2", "chrono", "serde_js hex.workspace = true iddqd.workspace = true ipnetwork.workspace = true +itertools.workspace = true macaddr.workspace = true newtype_derive.workspace = true omicron-cockroach-metrics.workspace = true diff --git a/nexus/db-model/src/network_interface.rs b/nexus/db-model/src/network_interface.rs index 68eb8ec5ca7..598b78b1681 100644 --- a/nexus/db-model/src/network_interface.rs +++ b/nexus/db-model/src/network_interface.rs @@ -12,7 +12,8 @@ use chrono::DateTime; use chrono::Utc; use db_macros::Resource; use diesel::AsChangeset; -use ipnetwork::IpNetwork; +use itertools::Either; +use itertools::Itertools; use nexus_db_schema::schema::instance_network_interface; use nexus_db_schema::schema::network_interface; use nexus_db_schema::schema::service_network_interface; @@ -98,9 +99,13 @@ pub struct NetworkInterface { /// True if this is the instance's primary interface. #[diesel(column_name = is_primary)] pub primary: bool, - /// List of additional networks on which the instance is allowed to send / - /// receive traffic. - pub transit_ips: Vec, + /// List of additional IPv4 networks on which the instance is allowed to + /// send / receive traffic. + #[diesel(column_name = transit_ips)] + pub transit_ips_v4: Vec, + /// List of additional IPv6 networks on which the instance is allowed to + /// send / receive traffic. + pub transit_ips_v6: Vec, } impl NetworkInterface { @@ -119,21 +124,12 @@ impl NetworkInterface { ))); } (None, Some(ip)) => { - // Check that all transit IPs are IPv6. let transit_ips = self - .transit_ips + .transit_ips_v6 .iter() - .map(|net| { - let IpNetwork::V6(net) = net else { - return Err(Error::internal_error(&format!( - "NIC with ID '{}' is IPv6-only, but has \ - IPv4 transit IPs", - self.id(), - ))); - }; - Ok(Ipv6Net::from(*net)) - }) - .collect::>()?; + .copied() + .map(Into::into) + .collect(); PrivateIpConfig::V6(PrivateIpv6Config::new_with_transit_ips( *ip, ipv6_subnet, @@ -141,21 +137,12 @@ impl NetworkInterface { )?) } (Some(ip), None) => { - // Check that all transit IPs are IPv4. let transit_ips = self - .transit_ips + .transit_ips_v4 .iter() - .map(|net| { - let IpNetwork::V4(net) = net else { - return Err(Error::internal_error(&format!( - "NIC with ID '{}' is IPv4-only, but has \ - IPv6 transit IPs", - self.id(), - ))); - }; - Ok(Ipv4Net::from(*net)) - }) - .collect::>()?; + .copied() + .map(Into::into) + .collect(); PrivateIpConfig::V4(PrivateIpv4Config::new_with_transit_ips( *ip, ipv4_subnet, @@ -164,20 +151,16 @@ impl NetworkInterface { } (Some(ipv4), Some(ipv6)) => { let ipv4_transit_ips = self - .transit_ips + .transit_ips_v4 .iter() - .filter_map(|net| match net { - IpNetwork::V4(net) => Some(Ipv4Net::from(*net)), - IpNetwork::V6(_) => None, - }) + .copied() + .map(Into::into) .collect(); let ipv6_transit_ips = self - .transit_ips + .transit_ips_v6 .iter() - .filter_map(|net| match net { - IpNetwork::V6(net) => Some(Ipv6Net::from(*net)), - IpNetwork::V4(_) => None, - }) + .copied() + .map(Into::into) .collect(); let v4 = PrivateIpv4Config::new_with_transit_ips( *ipv4, @@ -241,7 +224,8 @@ pub struct InstanceNetworkInterface { pub slot: SqlU8, #[diesel(column_name = is_primary)] pub primary: bool, - pub transit_ips: Vec, + pub transit_ips_v4: Vec, + pub transit_ips_v6: Vec, } /// Service Network Interface DB model. @@ -353,7 +337,8 @@ impl NetworkInterface { ipv6: self.ipv6, slot: self.slot, primary: self.primary, - transit_ips: self.transit_ips, + transit_ips_v4: self.transit_ips_v4, + transit_ips_v6: self.transit_ips_v6, } } @@ -404,7 +389,8 @@ impl From for NetworkInterface { ipv6: iface.ipv6, slot: iface.slot, primary: iface.primary, - transit_ips: iface.transit_ips, + transit_ips_v4: iface.transit_ips_v4, + transit_ips_v6: iface.transit_ips_v6, } } } @@ -429,7 +415,8 @@ impl From for NetworkInterface { ipv6: iface.ipv6, slot: iface.slot, primary: iface.primary, - transit_ips: vec![], + transit_ips_v4: vec![], + transit_ips_v6: vec![], } } } @@ -513,15 +500,19 @@ impl IpConfig { IpConfig::V4(Ipv4Config::default()) } - /// Return the IPv4 address assignment. - pub fn ipv4_assignment(&self) -> Option<&Ipv4Assignment> { + /// Return the IPv4 configuration, if it exists. + pub fn ipv4_config(&self) -> Option<&Ipv4Config> { match self { - IpConfig::V4(Ipv4Config { ip, .. }) => Some(ip), + IpConfig::V4(v4) | IpConfig::DualStack { v4, .. } => Some(v4), IpConfig::V6(_) => None, - IpConfig::DualStack { v4: Ipv4Config { ip, .. }, .. } => Some(ip), } } + /// Return the IPv4 address assignment. + pub fn ipv4_assignment(&self) -> Option<&Ipv4Assignment> { + self.ipv4_config().map(|v4| &v4.ip) + } + /// Return the IPv4 address explicitly requested, if one exists. pub fn ipv4_addr(&self) -> Option<&std::net::Ipv4Addr> { self.ipv4_assignment().and_then(|assignment| match assignment { @@ -530,6 +521,11 @@ impl IpConfig { }) } + /// Return the IPv4 transit IPs, if any. + pub fn ipv4_transit_ips(&self) -> Option> { + self.ipv4_config().map(|v4| v4.transit_ips.clone()) + } + /// Construct an IPv6 configuration with no transit IPs. pub fn from_ipv6(addr: std::net::Ipv6Addr) -> Self { IpConfig::V6(Ipv6Config { @@ -543,15 +539,19 @@ impl IpConfig { IpConfig::V6(Ipv6Config::default()) } - /// Return the IPv6 address assignment. - pub fn ipv6_assignment(&self) -> Option<&Ipv6Assignment> { + /// Return the IPv6 configuration, if it exists. + pub fn ipv6_config(&self) -> Option<&Ipv6Config> { match self { - IpConfig::V6(Ipv6Config { ip, .. }) => Some(ip), + IpConfig::V6(v6) | IpConfig::DualStack { v6, .. } => Some(v6), IpConfig::V4(_) => None, - IpConfig::DualStack { v6: Ipv6Config { ip, .. }, .. } => Some(ip), } } + /// Return the IPv6 address assignment. + pub fn ipv6_assignment(&self) -> Option<&Ipv6Assignment> { + self.ipv6_config().map(|v6| &v6.ip) + } + /// Return the IPv6 address explicitly requested, if one exists. pub fn ipv6_addr(&self) -> Option<&std::net::Ipv6Addr> { self.ipv6_assignment().and_then(|assignment| match assignment { @@ -560,25 +560,24 @@ impl IpConfig { }) } + /// Return the IPv6 transit IPs, if any. + pub fn ipv6_transit_ips(&self) -> Option> { + self.ipv6_config().map(|v6| v6.transit_ips.clone()) + } + /// Return the transit IPs requested in this configuration. pub fn transit_ips(&self) -> Vec { - match self { - IpConfig::V4(Ipv4Config { transit_ips, .. }) => { - transit_ips.iter().copied().map(Into::into).collect() - } - IpConfig::V6(Ipv6Config { transit_ips, .. }) => { - transit_ips.iter().copied().map(Into::into).collect() - } - IpConfig::DualStack { - v4: Ipv4Config { transit_ips: ipv4_addrs, .. }, - v6: Ipv6Config { transit_ips: ipv6_addrs, .. }, - } => ipv4_addrs - .iter() - .copied() - .map(Into::into) - .chain(ipv6_addrs.iter().copied().map(Into::into)) - .collect(), - } + self.ipv4_transit_ips() + .unwrap_or_default() + .into_iter() + .map(Into::into) + .chain( + self.ipv6_transit_ips() + .unwrap_or_default() + .into_iter() + .map(Into::into), + ) + .collect() } /// Construct a dual-stack IP configuration with explicit IP addresses. @@ -775,7 +774,9 @@ pub struct NetworkInterfaceUpdate { pub time_modified: DateTime, #[diesel(column_name = is_primary)] pub primary: Option, - pub transit_ips: Vec, + #[diesel(column_name = transit_ips)] + pub transit_ips_v4: Vec, + pub transit_ips_v6: Vec, } impl From for external::InstanceNetworkInterface { @@ -791,10 +792,13 @@ impl From for external::InstanceNetworkInterface { ip, mac: *iface.mac, primary: iface.primary, + // https://github.com/oxidecomputer/omicron/issues/9248 Separate + // these into IP versions. transit_ips: iface - .transit_ips + .transit_ips_v4 .into_iter() - .map(Into::into) + .map(|v4| v4.0.into()) + .chain(iface.transit_ips_v6.into_iter().map(|v6| v6.0.into())) .collect(), } } @@ -803,16 +807,18 @@ impl From for external::InstanceNetworkInterface { impl From for NetworkInterfaceUpdate { fn from(params: params::InstanceNetworkInterfaceUpdate) -> Self { let primary = if params.primary { Some(true) } else { None }; + let (transit_ips_v4, transit_ips_v6): (Vec<_>, Vec<_>) = + params.transit_ips.into_iter().partition_map(|net| match net { + IpNet::V4(v4) => Either::Left(crate::Ipv4Net::from(v4)), + IpNet::V6(v6) => Either::Right(crate::Ipv6Net::from(v6)), + }); Self { name: params.identity.name.map(|n| n.into()), description: params.identity.description, time_modified: Utc::now(), primary, - transit_ips: params - .transit_ips - .into_iter() - .map(Into::into) - .collect(), + transit_ips_v4, + transit_ips_v6, } } } diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index a9f9ef99b8e..da17bf04dfc 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock}; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: Version = Version::new(213, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(214, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock> = LazyLock::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(214, "separate-transit-ips-by-version"), KnownVersion::new(213, "fm-cases"), KnownVersion::new(212, "local-storage-disk-type"), KnownVersion::new(211, "blueprint-sled-config-subnet"), diff --git a/nexus/db-queries/src/db/datastore/network_interface.rs b/nexus/db-queries/src/db/datastore/network_interface.rs index 1c9df4e490f..a38d6ef9a53 100644 --- a/nexus/db-queries/src/db/datastore/network_interface.rs +++ b/nexus/db-queries/src/db/datastore/network_interface.rs @@ -51,8 +51,6 @@ use omicron_common::api::external::http_pagination::PaginatedBy; use omicron_common::api::internal::shared::PrivateIpConfig; use omicron_common::api::internal::shared::PrivateIpv4Config; use omicron_common::api::internal::shared::PrivateIpv6Config; -use oxnet::Ipv4Net; -use oxnet::Ipv6Net; use ref_cast::RefCast; use uuid::Uuid; @@ -85,7 +83,9 @@ struct NicInfo { #[diesel(select_expression = nexus_db_schema::schema::network_interface::slot)] slot: SqlU8, #[diesel(select_expression = nexus_db_schema::schema::network_interface::transit_ips)] - transit_ips: Vec, + transit_ips_v4: Vec, + #[diesel(select_expression = nexus_db_schema::schema::network_interface::transit_ips_v6)] + transit_ips_v6: Vec, } impl TryFrom @@ -99,77 +99,47 @@ impl TryFrom omicron_common::api::internal::shared::NetworkInterface, Self::Error, > { - let ip = match (nic.ipv4, nic.ipv6) { - (None, None) => { - return Err(Error::internal_error( - "Found NIC with no VPC-private IP addresses at all", - )); - } - (Some(ipv4), None) => { + let maybe_ipv4_config = match nic.ipv4 { + None => None, + Some(ipv4) => { let transit_ips = nic - .transit_ips + .transit_ips_v4 .iter() - .map(|net| { - let ipnetwork::IpNetwork::V4(net) = net else { - return Err(Error::internal_error( - "Found NIC with IPv4 address only, but \ - which has IPv6 transit IPs", - )); - }; - Ok(Ipv4Net::from(*net)) - }) - .collect::>()?; - PrivateIpConfig::V4(PrivateIpv4Config::new_with_transit_ips( + .copied() + .map(Into::into) + .collect(); + Some(PrivateIpv4Config::new_with_transit_ips( *ipv4, *nic.ipv4_block, transit_ips, )?) } - (None, Some(ipv6)) => { + }; + let maybe_ipv6_config = match nic.ipv6 { + None => None, + Some(ipv6) => { let transit_ips = nic - .transit_ips + .transit_ips_v6 .iter() - .map(|net| { - let ipnetwork::IpNetwork::V6(net) = net else { - return Err(Error::internal_error( - "Found NIC with IPv6 address only, but \ - which has IPv4 transit IPs", - )); - }; - Ok(Ipv6Net::from(*net)) - }) - .collect::>()?; - PrivateIpConfig::V6(PrivateIpv6Config::new_with_transit_ips( + .copied() + .map(Into::into) + .collect(); + Some(PrivateIpv6Config::new_with_transit_ips( *ipv6, *nic.ipv6_block, transit_ips, )?) } - (Some(ipv4), Some(ipv6)) => { - let mut ipv4_transit_ips = Vec::new(); - let mut ipv6_transit_ips = Vec::new(); - for net in nic.transit_ips.iter() { - match net { - ipnetwork::IpNetwork::V4(net) => { - ipv4_transit_ips.push(Ipv4Net::from(*net)) - } - ipnetwork::IpNetwork::V6(net) => { - ipv6_transit_ips.push(Ipv6Net::from(*net)) - } - } - } - let v4 = PrivateIpv4Config::new_with_transit_ips( - *ipv4, - *nic.ipv4_block, - ipv4_transit_ips, - )?; - let v6 = PrivateIpv6Config::new_with_transit_ips( - *ipv6, - *nic.ipv6_block, - ipv6_transit_ips, - )?; - PrivateIpConfig::DualStack { v4, v6 } + }; + let ip = match (maybe_ipv4_config, maybe_ipv6_config) { + (None, None) => { + return Err(Error::internal_error( + "Found NIC with no VPC-private IP addresses at all", + )); } + (Some(v4), None) => PrivateIpConfig::V4(v4), + (None, Some(v6)) => PrivateIpConfig::V6(v6), + (Some(v4), Some(v6)) => PrivateIpConfig::DualStack { v4, v6 }, }; let kind = match nic.kind { NetworkInterfaceKind::Instance => { diff --git a/nexus/db-queries/src/db/queries/network_interface.rs b/nexus/db-queries/src/db/queries/network_interface.rs index 927e7700748..a5b656010b6 100644 --- a/nexus/db-queries/src/db/queries/network_interface.rs +++ b/nexus/db-queries/src/db/queries/network_interface.rs @@ -3070,13 +3070,22 @@ mod tests { inserted.mac, kind, ); assert_eq!( - inserted.transit_ips, - incomplete - .ip_config - .transit_ips() - .into_iter() - .map(ipnetwork::IpNetwork::from) - .collect::>() + incomplete.ip_config.ipv4_transit_ips().unwrap_or_default(), + inserted + .transit_ips_v4 + .iter() + .copied() + .map(|v4| v4.0) + .collect::>(), + ); + assert_eq!( + incomplete.ip_config.ipv6_transit_ips().unwrap_or_default(), + inserted + .transit_ips_v6 + .iter() + .copied() + .map(|v6| v6.0) + .collect::>(), ); } diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index 7686c478104..3ef6efb4b0a 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -601,7 +601,9 @@ table! { ipv6 -> Nullable, slot -> Int2, is_primary -> Bool, + // NOTE: These are the IPv4 transit IPs specifically. transit_ips -> Array, + transit_ips_v6 -> Array, } } @@ -621,7 +623,8 @@ table! { ipv6 -> Nullable, slot -> Int2, is_primary -> Bool, - transit_ips -> Array, + transit_ips_v4 -> Array, + transit_ips_v6 -> Array, } } joinable!(instance_network_interface -> instance (instance_id)); diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 879fe5b3309..d0ef5cb7cf7 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -1883,9 +1883,13 @@ CREATE TABLE IF NOT EXISTS omicron.public.network_interface ( is_primary BOOL NOT NULL, /* - * A supplementary list of addresses/CIDR blocks which a NIC is + * A supplementary list of IPv4 addresses/CIDR blocks which a NIC is * *allowed* to send/receive traffic on, in addition to its * assigned address. + * + * NOTE: Despite the name, these are always IPv4 networks or addresses. + * We've kept the original name since renaming columns idempotently is difficult + * in CRDB right now. */ transit_ips INET[] NOT NULL DEFAULT ARRAY[], @@ -1895,11 +1899,26 @@ CREATE TABLE IF NOT EXISTS omicron.public.network_interface ( */ ipv6 INET, + /* + * A supplementary list of IPv6 addresses/CIDR blocks which a NIC is + * *allowed* to send/receive traffic on, in addition to its + * assigned address. + */ + transit_ips_v6 INET[] NOT NULL DEFAULT ARRAY[], + /* Constraint ensuring we have at least one IP address from either family. * Both may be specified. */ CONSTRAINT at_least_one_ip_address CHECK ( ip IS NOT NULL OR ipv6 IS NOT NULL + ), + + /* Constraint ensuring that if we have transit IPs of a specific version, we + * also have a corresponding IP address. + */ + CONSTRAINT transit_ips_require_ip_address CHECK ( + (array_length(transit_ips, 1) = 0 OR ip IS NOT NULL) AND + (array_length(transit_ips_v6, 1) = 0 OR ipv6 IS NOT NULL) ) ); @@ -1923,7 +1942,8 @@ SELECT ipv6, slot, is_primary, - transit_ips + transit_ips as transit_ips_v4, + transit_ips_v6 FROM omicron.public.network_interface WHERE @@ -7458,7 +7478,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '213.0.0', NULL) + (TRUE, NOW(), NOW(), '214.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/separate-transit-ips-by-version/up01.sql b/schema/crdb/separate-transit-ips-by-version/up01.sql new file mode 100644 index 00000000000..a131ff17c3d --- /dev/null +++ b/schema/crdb/separate-transit-ips-by-version/up01.sql @@ -0,0 +1,5 @@ +/* Add the new column for IPv6 transit IPs. */ +ALTER TABLE +omicron.public.network_interface +ADD COLUMN IF NOT EXISTS transit_ips_v6 +INET[] NOT NULL DEFAULT ARRAY[]; diff --git a/schema/crdb/separate-transit-ips-by-version/up02.sql b/schema/crdb/separate-transit-ips-by-version/up02.sql new file mode 100644 index 00000000000..abd537b34e2 --- /dev/null +++ b/schema/crdb/separate-transit-ips-by-version/up02.sql @@ -0,0 +1,18 @@ +/* + * Update the transit_ips and transit_ips_v6 columns to be specific to each IP + * version. + * + * CRDB doesn't have methods for operating on arrays, like filter / map / etc, + * so we unnest them into a sequence of rows and insert back. + */ +SET LOCAL disallow_full_table_scans = 'off'; +UPDATE omicron.public.network_interface +SET +transit_ips = ARRAY( + SELECT ip FROM UNNEST(transit_ips) AS ip + WHERE family(ip) = 4 +), +transit_ips_v6 = ARRAY( + SELECT ip FROM UNNEST(transit_ips) AS ip + WHERE family(ip) = 6 +); diff --git a/schema/crdb/separate-transit-ips-by-version/up03.sql b/schema/crdb/separate-transit-ips-by-version/up03.sql new file mode 100644 index 00000000000..93b744a064d --- /dev/null +++ b/schema/crdb/separate-transit-ips-by-version/up03.sql @@ -0,0 +1,8 @@ +/* Now add the CHECK constraint ensuring we have a matching IP */ +ALTER TABLE +omicron.public.network_interface +ADD CONSTRAINT IF NOT EXISTS transit_ips_require_ip_address +CHECK ( + (array_length(transit_ips, 1) = 0 OR ip IS NOT NULL) AND + (array_length(transit_ips_v6, 1) = 0 OR ipv6 IS NOT NULL) +); diff --git a/schema/crdb/separate-transit-ips-by-version/up04.sql b/schema/crdb/separate-transit-ips-by-version/up04.sql new file mode 100644 index 00000000000..82109618856 --- /dev/null +++ b/schema/crdb/separate-transit-ips-by-version/up04.sql @@ -0,0 +1,4 @@ +/* Drop the view showing _instance_ network interfaces, since we cnanot alter + * those at this time. + */ +DROP VIEW IF EXISTS omicron.public.instance_network_interface; diff --git a/schema/crdb/separate-transit-ips-by-version/up05.sql b/schema/crdb/separate-transit-ips-by-version/up05.sql new file mode 100644 index 00000000000..729711d74a5 --- /dev/null +++ b/schema/crdb/separate-transit-ips-by-version/up05.sql @@ -0,0 +1,23 @@ +/* And recreate it with the transit IPs separated by version */ +CREATE VIEW IF NOT EXISTS omicron.public.instance_network_interface AS +SELECT + id, + name, + description, + time_created, + time_modified, + time_deleted, + parent_id AS instance_id, + vpc_id, + subnet_id, + mac, + ip AS ipv4, + ipv6, + slot, + is_primary, + transit_ips AS transit_ips_v4, + transit_ips_v6 +FROM + omicron.public.network_interface +WHERE + kind = 'instance';