From 3d4a48411f276ba002d7dc6d476749a61dc1681d Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Tue, 5 Aug 2025 00:22:24 +0000 Subject: [PATCH 1/2] Add IP version to IP Pool database objects - Add an `IpVersion` type and attach to all IP Pool objects - Schema and data migration to add the version to IP Pools in the database - Add a services IP Pool for IPv6 addresses - Ensure we can only add ranges to pools of the same version - Fixes a bunch of knock-on effects of splitting out the pools by IP version - Closes #8880 --- common/src/address.rs | 41 ++ common/src/api/external/mod.rs | 1 + nexus/db-model/src/ip_pool.rs | 58 ++- nexus/db-model/src/schema_versions.rs | 3 +- .../db-queries/src/db/datastore/deployment.rs | 17 +- .../deployment/external_networking.rs | 65 +++- .../src/db/datastore/external_ip.rs | 18 +- nexus/db-queries/src/db/datastore/ip_pool.rs | 368 +++++++++++++++--- nexus/db-queries/src/db/datastore/mod.rs | 7 +- .../src/db/datastore/network_interface.rs | 28 +- nexus/db-queries/src/db/datastore/rack.rs | 353 ++++++++++++++--- .../db-queries/src/db/queries/external_ip.rs | 18 +- nexus/db-schema/src/enums.rs | 1 + nexus/db-schema/src/schema.rs | 1 + nexus/reconfigurator/execution/src/dns.rs | 34 +- nexus/reconfigurator/preparation/src/lib.rs | 32 +- nexus/src/app/ip_pool.rs | 49 ++- nexus/src/app/rack.rs | 2 +- nexus/tests/integration_tests/ip_pools.rs | 72 +++- .../types/src/deployment/network_resources.rs | 9 + nexus/types/src/external_api/shared.rs | 2 +- nexus/types/src/external_api/views.rs | 1 + schema/crdb/add-ip-version-to-pools/up01.sql | 4 + schema/crdb/add-ip-version-to-pools/up02.sql | 7 + schema/crdb/add-ip-version-to-pools/up03.sql | 3 + schema/crdb/add-ip-version-to-pools/up04.sql | 8 + schema/crdb/add-ip-version-to-pools/up05.sql | 5 + schema/crdb/dbinit.sql | 13 +- 28 files changed, 1026 insertions(+), 194 deletions(-) create mode 100644 schema/crdb/add-ip-version-to-pools/up01.sql create mode 100644 schema/crdb/add-ip-version-to-pools/up02.sql create mode 100644 schema/crdb/add-ip-version-to-pools/up03.sql create mode 100644 schema/crdb/add-ip-version-to-pools/up04.sql create mode 100644 schema/crdb/add-ip-version-to-pools/up05.sql diff --git a/common/src/address.rs b/common/src/address.rs index 92863c44a42..9e8dd798293 100644 --- a/common/src/address.rs +++ b/common/src/address.rs @@ -368,6 +368,29 @@ pub fn get_64_subnet( Ipv6Subnet::::new(Ipv6Addr::from(rack_network)) } +/// The IP address version. +#[derive(Clone, Copy, Debug, Deserialize, JsonSchema, PartialEq, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum IpVersion { + V4, + V6, +} + +impl IpVersion { + pub const fn v4() -> IpVersion { + IpVersion::V4 + } +} + +impl std::fmt::Display for IpVersion { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + match self { + Self::V4 => write!(f, "v4"), + Self::V6 => write!(f, "v6"), + } + } +} + /// An IP Range is a contiguous range of IP addresses, usually within an IP /// Pool. /// @@ -450,6 +473,24 @@ impl IpRange { IpRange::V6(ip6) => ip6.len(), } } + + /// Return true if this is an IPv4 range, and false for IPv6. + pub fn is_ipv4(&self) -> bool { + matches!(self, IpRange::V4(_)) + } + + /// Return true if this is an IPv6 range, and false for IPv4. + pub fn is_ipv6(&self) -> bool { + matches!(self, IpRange::V6(_)) + } + + /// Return the IP version of this range. + pub fn version(&self) -> IpVersion { + match self { + IpRange::V4(_) => IpVersion::V4, + IpRange::V6(_) => IpVersion::V6, + } + } } impl From for IpRange { diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index cdd208f1da7..1f9268400f2 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -9,6 +9,7 @@ mod error; pub mod http_pagination; +pub use crate::address::IpVersion; pub use crate::api::internal::shared::AllowedSourceIps; pub use crate::api::internal::shared::SwitchLocation; use crate::update::ArtifactId; diff --git a/nexus/db-model/src/ip_pool.rs b/nexus/db-model/src/ip_pool.rs index 86b9265cd48..e05498b7f3a 100644 --- a/nexus/db-model/src/ip_pool.rs +++ b/nexus/db-model/src/ip_pool.rs @@ -16,6 +16,7 @@ use nexus_db_schema::schema::ip_pool; use nexus_db_schema::schema::ip_pool_range; use nexus_db_schema::schema::ip_pool_resource; use nexus_types::external_api::params; +use nexus_types::external_api::shared; use nexus_types::external_api::shared::IpRange; use nexus_types::external_api::views; use nexus_types::identity::Resource; @@ -23,6 +24,54 @@ use omicron_common::api::external; use std::net::IpAddr; use uuid::Uuid; +impl_enum_type!( + IpVersionEnum: + + #[derive( + AsExpression, + Clone, + Copy, + Debug, + serde::Deserialize, + Eq, + FromSqlRow, + schemars::JsonSchema, + PartialEq, + serde::Serialize, + )] + pub enum IpVersion; + + V4 => b"v4" + V6 => b"v6" +); + +impl ::std::fmt::Display for IpVersion { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + IpVersion::V4 => "v4", + IpVersion::V6 => "v6", + }) + } +} + +impl From for IpVersion { + fn from(value: shared::IpVersion) -> Self { + match value { + shared::IpVersion::V4 => Self::V4, + shared::IpVersion::V6 => Self::V6, + } + } +} + +impl From for shared::IpVersion { + fn from(value: IpVersion) -> Self { + match value { + IpVersion::V4 => Self::V4, + IpVersion::V6 => Self::V6, + } + } +} + /// An IP Pool is a collection of IP addresses external to the rack. /// /// IP pools can be external or internal. External IP pools can be associated @@ -34,18 +83,25 @@ pub struct IpPool { #[diesel(embed)] pub identity: IpPoolIdentity, + /// The IP version of the pool. + pub ip_version: IpVersion, + /// Child resource generation number, for optimistic concurrency control of /// the contained ranges. pub rcgen: i64, } impl IpPool { - pub fn new(pool_identity: &external::IdentityMetadataCreateParams) -> Self { + pub fn new( + pool_identity: &external::IdentityMetadataCreateParams, + ip_version: IpVersion, + ) -> Self { Self { identity: IpPoolIdentity::new( Uuid::new_v4(), pool_identity.clone(), ), + ip_version, rcgen: 0, } } diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 1a09c13ed49..fd8fc89b3bb 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(182, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(183, 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(183, "add-ip-version-to-pools"), KnownVersion::new(182, "add-tuf-artifact-board"), KnownVersion::new(181, "rename-nat-table"), KnownVersion::new(180, "sled-cpu-family"), diff --git a/nexus/db-queries/src/db/datastore/deployment.rs b/nexus/db-queries/src/db/datastore/deployment.rs index 1f9c022ce4d..0e46092de95 100644 --- a/nexus/db-queries/src/db/datastore/deployment.rs +++ b/nexus/db-queries/src/db/datastore/deployment.rs @@ -2954,6 +2954,7 @@ mod tests { use crate::db::pub_test_utils::TestDatabase; use crate::db::raw_query_builder::QueryBuilder; use gateway_types::rot::RotSlot; + use nexus_db_model::IpVersion; use nexus_inventory::CollectionBuilder; use nexus_inventory::now_db_precision; use nexus_reconfigurator_planning::blueprint_builder::BlueprintBuilder; @@ -4221,12 +4222,17 @@ mod tests { Ipv4Addr::new(10, 0, 0, 10), )) .unwrap(); - let (service_ip_pool, _) = datastore - .ip_pools_service_lookup(&opctx) + let (service_authz_ip_pool, service_ip_pool) = datastore + .ip_pools_service_lookup(&opctx, IpVersion::V4) .await .expect("lookup service ip pool"); datastore - .ip_pool_add_range(&opctx, &service_ip_pool, &ip_range) + .ip_pool_add_range( + &opctx, + &service_authz_ip_pool, + &service_ip_pool, + &ip_range, + ) .await .expect("add range to service ip pool"); let zone_id = OmicronZoneUuid::new_v4(); @@ -4353,13 +4359,14 @@ mod tests { .map(|(ip, _nic)| ip.ip()) }) .expect("found external IP"); - let (service_ip_pool, _) = datastore - .ip_pools_service_lookup(&opctx) + let (service_authz_ip_pool, service_ip_pool) = datastore + .ip_pools_service_lookup(&opctx, IpVersion::V4) .await .expect("lookup service ip pool"); datastore .ip_pool_add_range( &opctx, + &service_authz_ip_pool, &service_ip_pool, &IpRange::try_from((nexus_ip, nexus_ip)) .expect("valid IP range"), 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 3364a15380d..85c9656f174 100644 --- a/nexus/db-queries/src/db/datastore/deployment/external_networking.rs +++ b/nexus/db-queries/src/db/datastore/deployment/external_networking.rs @@ -18,6 +18,7 @@ use nexus_types::deployment::BlueprintZoneConfig; use nexus_types::deployment::OmicronZoneExternalIp; use omicron_common::api::external::Error; use omicron_common::api::external::IdentityMetadataCreateParams; +use omicron_common::api::external::IpVersion; use omicron_common::api::internal::shared::NetworkInterface; use omicron_common::api::internal::shared::NetworkInterfaceKind; use omicron_uuid_kinds::GenericUuid; @@ -29,6 +30,47 @@ use slog::info; use slog::warn; use slog_error_chain::InlineErrorChain; +// Helper type to lookup service IP Pools only when needed, at most once. +// +// In `ensure_zone_external_networking_deallocated_on_connection`, we lookup +// pools only when we're sure we need them, based on the versions of the IP +// addresses we need to provide for each zone. +struct ServiceIpPools<'a> { + maybe_ipv4_pool: Option, + maybe_ipv6_pool: Option, + datastore: &'a DataStore, + opctx: &'a OpContext, +} + +impl<'a> ServiceIpPools<'a> { + fn new(datastore: &'a DataStore, opctx: &'a OpContext) -> Self { + Self { maybe_ipv4_pool: None, maybe_ipv6_pool: None, datastore, opctx } + } + + /// Get the service IP Pool for the given address. + /// + /// This may need to call out to the database to fetch the pool. + async fn pool_for( + &mut self, + external_ip: &OmicronZoneExternalIp, + ) -> Result<&IpPool, Error> { + let version = external_ip.ip_version(); + let pool = match version { + IpVersion::V4 => &mut self.maybe_ipv4_pool, + IpVersion::V6 => &mut self.maybe_ipv6_pool, + }; + if let Some(pool) = pool { + return Ok(&*pool); + } + let new_pool = self + .datastore + .ip_pools_service_lookup(self.opctx, version.into()) + .await? + .1; + Ok(pool.insert(new_pool)) + } +} + impl DataStore { pub(super) async fn ensure_zone_external_networking_allocated_on_connection( &self, @@ -36,10 +78,10 @@ impl DataStore { opctx: &OpContext, zones_to_allocate: impl Iterator, ) -> Result<(), Error> { - // Looking up the service pool ID requires an opctx; we'll do this once - // up front and reuse the pool ID (which never changes) in the loop - // below. - let (_, pool) = self.ip_pools_service_lookup(opctx).await?; + // Looking up the service pool IDs requires an opctx; we'll do this at + // most once inside the loop below, when we first encounter an address + // of the same IP version. + let mut ip_pools = ServiceIpPools::new(self, opctx); for z in zones_to_allocate { let Some((external_ip, nic)) = z.zone_type.external_networking() @@ -55,10 +97,12 @@ impl DataStore { "nic" => format!("{nic:?}"), )); + // Get existing pool or look it up and cache it. + let pool = ip_pools.pool_for(&external_ip).await?; let kind = z.zone_type.kind(); self.ensure_external_service_ip( conn, - &pool, + pool, kind, z.id, external_ip, @@ -592,12 +636,17 @@ mod tests { opctx: &OpContext, datastore: &DataStore, ) { - let (ip_pool, _) = datastore - .ip_pools_service_lookup(&opctx) + let (ip_pool, db_pool) = datastore + .ip_pools_service_lookup(&opctx, IpVersion::V4.into()) .await .expect("failed to find service IP pool"); datastore - .ip_pool_add_range(&opctx, &ip_pool, &self.external_ips_range) + .ip_pool_add_range( + &opctx, + &ip_pool, + &db_pool, + &self.external_ips_range, + ) .await .expect("failed to expand service IP pool"); } diff --git a/nexus/db-queries/src/db/datastore/external_ip.rs b/nexus/db-queries/src/db/datastore/external_ip.rs index d525edf9900..4aa91b46664 100644 --- a/nexus/db-queries/src/db/datastore/external_ip.rs +++ b/nexus/db-queries/src/db/datastore/external_ip.rs @@ -39,6 +39,7 @@ use nexus_db_lookup::LookupPath; use nexus_db_model::FloatingIpUpdate; use nexus_db_model::Instance; use nexus_db_model::IpAttachState; +use nexus_db_model::IpVersion; use nexus_sled_agent_shared::inventory::ZoneKind; use nexus_types::deployment::OmicronZoneExternalIp; use nexus_types::identity::Resource; @@ -318,7 +319,9 @@ impl DataStore { zone_kind: ZoneKind, external_ip: OmicronZoneExternalIp, ) -> CreateResult { - let (authz_pool, pool) = self.ip_pools_service_lookup(opctx).await?; + let version = IpVersion::from(external_ip.ip_version()); + let (authz_pool, pool) = + self.ip_pools_service_lookup(opctx, version).await?; opctx.authorize(authz::Action::CreateChild, &authz_pool).await?; let data = IncompleteExternalIp::for_omicron_zone( pool.id(), @@ -356,7 +359,12 @@ impl DataStore { ) -> ListResultVec { use nexus_db_schema::schema::external_ip::dsl; - let (authz_pool, _pool) = self.ip_pools_service_lookup(opctx).await?; + // Note the IP version used here isn't important. It's just for the + // authz check to list children, and not used for the actual database + // query below, which filters on is_service to get external IPs from + // either pool. + let (authz_pool, _pool) = + self.ip_pools_service_lookup(opctx, IpVersion::V4).await?; opctx.authorize(authz::Action::ListChildren, &authz_pool).await?; paginated(dsl::external_ip, dsl::id, pagparams) @@ -1183,12 +1191,12 @@ mod tests { Ipv4Addr::new(10, 0, 0, 10), )) .unwrap(); - let (service_ip_pool, _) = datastore - .ip_pools_service_lookup(opctx) + let (service_ip_pool, db_pool) = datastore + .ip_pools_service_lookup(opctx, IpVersion::V4) .await .expect("lookup service ip pool"); datastore - .ip_pool_add_range(opctx, &service_ip_pool, &ip_range) + .ip_pool_add_range(opctx, &service_ip_pool, &db_pool, &ip_range) .await .expect("add range to service ip pool"); diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index c080f4322f2..48b9f28d250 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -10,7 +10,8 @@ use crate::authz; use crate::context::OpContext; use crate::db::collection_insert::AsyncInsertError; use crate::db::collection_insert::DatastoreCollection; -use crate::db::datastore::SERVICE_IP_POOL_NAME; +use crate::db::datastore::SERVICE_IPV4_POOL_NAME; +use crate::db::datastore::SERVICE_IPV6_POOL_NAME; use crate::db::identity::Resource; use crate::db::model::ExternalIp; use crate::db::model::IpKind; @@ -37,6 +38,7 @@ use nexus_db_lookup::DbConnection; use nexus_db_lookup::LookupPath; use nexus_db_model::InternetGateway; use nexus_db_model::InternetGatewayIpPool; +use nexus_db_model::IpVersion; use nexus_db_model::Project; use nexus_db_model::Vpc; use nexus_types::external_api::shared::IpRange; @@ -65,6 +67,35 @@ pub struct IpsCapacity { pub ipv6: u128, } +/// Helper type with both an authz IP Pool and the actual DB record. +#[derive(Debug, Clone)] +pub struct ServiceIpPool { + pub authz_pool: authz::IpPool, + pub db_pool: IpPool, +} + +/// Helper type with service IP Pool information for both IP versions. +#[derive(Debug, Clone)] +pub struct ServiceIpPools { + pub ipv4: ServiceIpPool, + pub ipv6: ServiceIpPool, +} + +impl ServiceIpPools { + /// Return the IP Pool appropriate for a range, based on its version. + pub fn pool_for_range(&self, range: &IpRange) -> &ServiceIpPool { + if range.first_address().is_ipv4() { &self.ipv4 } else { &self.ipv6 } + } + + /// Return the IP Pool appropriate for an IP version. + pub fn pool_for_version(&self, version: IpVersion) -> &IpPool { + match version { + IpVersion::V4 => &self.ipv4.db_pool, + IpVersion::V6 => &self.ipv6.db_pool, + } + } +} + impl DataStore { /// List IP Pools pub async fn ip_pools_list( @@ -87,7 +118,8 @@ impl DataStore { &pagparams.map_name(|n| Name::ref_cast(n)), ), } - .filter(ip_pool::name.ne(SERVICE_IP_POOL_NAME)) + .filter(ip_pool::name.ne(SERVICE_IPV4_POOL_NAME)) + .filter(ip_pool::name.ne(SERVICE_IPV6_POOL_NAME)) .filter(ip_pool::time_deleted.is_null()) .select(IpPool::as_select()) .get_results_async(&*self.pool_connection_authorized(opctx).await?) @@ -186,15 +218,42 @@ impl DataStore { }) } - /// Look up IP pool intended for internal services by its well-known name. + /// Look up internal service IP Pools for both IP versions. + /// + /// This is useful when you need to handle resources like external IPs where + /// the actual address might be from either IP version. + // + // NOTE: It'd be better to do one roundtrip to the DB, but this is + // rarely-used right now. We also want to return the authz and database + // objects, so we need the lookup-path mechanism. + pub async fn ip_pools_service_lookup_both_versions( + &self, + opctx: &OpContext, + ) -> LookupResult { + let ipv4 = self.ip_pools_service_lookup(opctx, IpVersion::V4).await?; + let ipv6 = self.ip_pools_service_lookup(opctx, IpVersion::V6).await?; + Ok(ServiceIpPools { + ipv4: ServiceIpPool { authz_pool: ipv4.0, db_pool: ipv4.1 }, + ipv6: ServiceIpPool { authz_pool: ipv6.0, db_pool: ipv6.1 }, + }) + } + + /// Look up IP pool intended for internal services by their well-known + /// names. There are separate IP Pools for IPv4 and IPv6 address ranges. /// /// This method may require an index by Availability Zone in the future. pub async fn ip_pools_service_lookup( &self, opctx: &OpContext, + ip_version: IpVersion, ) -> LookupResult<(authz::IpPool, IpPool)> { - let name = SERVICE_IP_POOL_NAME.parse().unwrap(); - LookupPath::new(&opctx, self).ip_pool_name(&Name(name)).fetch().await + let name = match ip_version { + IpVersion::V4 => SERVICE_IPV4_POOL_NAME, + IpVersion::V6 => SERVICE_IPV6_POOL_NAME, + }; + let name = + Name(name.parse().expect("should be able to parse builtin names")); + LookupPath::new(&opctx, self).ip_pool_name(&name).fetch().await } /// Creates a new IP pool. @@ -300,7 +359,11 @@ impl DataStore { ip_pool::table .filter(ip_pool::id.eq(authz_pool.id())) - .filter(ip_pool::name.eq(SERVICE_IP_POOL_NAME)) + .filter( + ip_pool::name + .eq(SERVICE_IPV4_POOL_NAME) + .or(ip_pool::name.eq(SERVICE_IPV6_POOL_NAME)), + ) .filter(ip_pool::time_deleted.is_null()) .select(ip_pool::id) .first_async::( @@ -1003,11 +1066,14 @@ impl DataStore { &self, opctx: &OpContext, authz_pool: &authz::IpPool, + pool: &IpPool, range: &IpRange, ) -> CreateResult { let conn = self.pool_connection_authorized(opctx).await?; - Self::ip_pool_add_range_on_connection(&conn, opctx, authz_pool, range) - .await + Self::ip_pool_add_range_on_connection( + &conn, opctx, authz_pool, pool, range, + ) + .await } /// Variant of [Self::ip_pool_add_range] which may be called from a @@ -1016,11 +1082,24 @@ impl DataStore { conn: &async_bb8_diesel::Connection, opctx: &OpContext, authz_pool: &authz::IpPool, + pool: &IpPool, range: &IpRange, ) -> CreateResult { use nexus_db_schema::schema::ip_pool_range::dsl; opctx.authorize(authz::Action::CreateChild, authz_pool).await?; let pool_id = authz_pool.id(); + + // First ensure the IP range matches the IP version of the pool. + if pool.ip_version != range.version().into() { + return Err(Error::invalid_request(format!( + "Cannot add IP{} address range to \ + IP{} pool with ID \"{}\"", + range.version(), + pool.ip_version, + pool.id(), + ))); + } + let new_range = IpPoolRange::new(range, pool_id); let filter_subquery = FilterOverlappingIpRanges { range: new_range }; let insert_query = @@ -1151,6 +1230,7 @@ mod test { }; use crate::db::pub_test_utils::TestDatabase; use assert_matches::assert_matches; + use nexus_db_model::IpVersion; use nexus_types::external_api::params; use nexus_types::identity::Resource; use omicron_common::address::{IpRange, Ipv4Range, Ipv6Range}; @@ -1200,7 +1280,7 @@ mod test { description: "".to_string(), }; let pool1_for_silo = datastore - .ip_pool_create(&opctx, IpPool::new(&identity)) + .ip_pool_create(&opctx, IpPool::new(&identity, IpVersion::V4)) .await .expect("Failed to create IP pool"); @@ -1286,7 +1366,7 @@ mod test { description: "".to_string(), }; let second_silo_default = datastore - .ip_pool_create(&opctx, IpPool::new(&identity)) + .ip_pool_create(&opctx, IpPool::new(&identity, IpVersion::V4)) .await .expect("Failed to create pool"); let err = datastore @@ -1327,62 +1407,76 @@ mod test { } #[tokio::test] - async fn test_internal_ip_pool() { - let logctx = dev::test_setup_log("test_internal_ip_pool"); + async fn test_internal_ip_pools() { + let logctx = dev::test_setup_log("test_internal_ip_pools"); let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); - // confirm internal pool appears as internal - let (authz_pool, _pool) = - datastore.ip_pools_service_lookup(&opctx).await.unwrap(); - - let is_internal = - datastore.ip_pool_is_internal(&opctx, &authz_pool).await; - assert_eq!(is_internal, Ok(true)); - - // another random pool should not be considered internal - let identity = IdentityMetadataCreateParams { - name: "other-pool".parse().unwrap(), - description: "".to_string(), - }; - let other_pool = datastore - .ip_pool_create(&opctx, IpPool::new(&identity)) - .await - .expect("Failed to create IP pool"); - - let authz_other_pool = authz::IpPool::new( - authz::FLEET, - other_pool.id(), - LookupType::ById(other_pool.id()), - ); - let is_internal = - datastore.ip_pool_is_internal(&opctx, &authz_other_pool).await; - assert_eq!(is_internal, Ok(false)); - - // now link it to the current silo, and it is still not internal - let silo_id = opctx.authn.silo_required().unwrap().id(); - let link = IpPoolResource { - ip_pool_id: other_pool.id(), - resource_type: IpPoolResourceType::Silo, - resource_id: silo_id, - is_default: true, - }; - datastore - .ip_pool_link_silo(&opctx, link) - .await - .expect("Failed to make IP pool default for silo"); + for version in [IpVersion::V4, IpVersion::V6] { + // confirm internal pools appear as internal + let (authz_pool, pool) = datastore + .ip_pools_service_lookup(&opctx, version) + .await + .unwrap(); + assert_eq!(pool.ip_version, version); + + let is_internal = + datastore.ip_pool_is_internal(&opctx, &authz_pool).await; + assert_eq!(is_internal, Ok(true)); + + // another random pool should not be considered internal + let identity = IdentityMetadataCreateParams { + name: format!("other-{version}-pool").parse().unwrap(), + description: "".to_string(), + }; + let other_pool = datastore + .ip_pool_create(&opctx, IpPool::new(&identity, version)) + .await + .expect("Failed to create IP pool"); + assert_eq!(other_pool.ip_version, version); + + let authz_other_pool = authz::IpPool::new( + authz::FLEET, + other_pool.id(), + LookupType::ById(other_pool.id()), + ); + let is_internal = + datastore.ip_pool_is_internal(&opctx, &authz_other_pool).await; + assert_eq!(is_internal, Ok(false)); + + // now link it to the current silo, and it is still not internal. + // + // We're only making the IPv4 pool the default right now. See + // https://github.com/oxidecomputer/omicron/issues/8884 for more. + let silo_id = opctx.authn.silo_required().unwrap().id(); + let is_default = matches!(version, IpVersion::V4); + let link = IpPoolResource { + ip_pool_id: other_pool.id(), + resource_type: IpPoolResourceType::Silo, + resource_id: silo_id, + is_default, + }; + datastore + .ip_pool_link_silo(&opctx, link) + .await + .expect("Failed to link IP pool to silo"); - let is_internal = - datastore.ip_pool_is_internal(&opctx, &authz_other_pool).await; - assert_eq!(is_internal, Ok(false)); + let is_internal = + datastore.ip_pool_is_internal(&opctx, &authz_other_pool).await; + assert_eq!(is_internal, Ok(false)); + } db.terminate().await; logctx.cleanup_successful(); } + // We're breaking out the utilization tests for IPv4 and IPv6 pools, since + // pools only contain one version now. + // + // See https://github.com/oxidecomputer/omicron/issues/8888. #[tokio::test] - async fn test_ip_pool_utilization() { - let logctx = dev::test_setup_log("test_ip_utilization"); + async fn test_ipv4_ip_pool_utilization() { + let logctx = dev::test_setup_log("test_ipv4_ip_pool_utilization"); let db = TestDatabase::new_with_datastore(&logctx.log).await; let (opctx, datastore) = (db.opctx(), db.datastore()); @@ -1405,7 +1499,7 @@ mod test { description: "".to_string(), }; let pool = datastore - .ip_pool_create(&opctx, IpPool::new(&identity)) + .ip_pool_create(&opctx, IpPool::new(&identity, IpVersion::V4)) .await .expect("Failed to create IP pool"); let authz_pool = authz::IpPool::new( @@ -1430,7 +1524,7 @@ mod test { .unwrap(), ); datastore - .ip_pool_add_range(&opctx, &authz_pool, &range) + .ip_pool_add_range(&opctx, &authz_pool, &pool, &range) .await .expect("Could not add range"); @@ -1485,6 +1579,63 @@ mod test { assert_eq!(max_ips.ipv4, 5); assert_eq!(max_ips.ipv6, 0); + db.terminate().await; + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn test_ipv6_ip_pool_utilization() { + let logctx = dev::test_setup_log("test_ipv6_ip_pool_utilization"); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + + let authz_silo = opctx.authn.silo_required().unwrap(); + let project = Project::new( + authz_silo.id(), + params::ProjectCreate { + identity: IdentityMetadataCreateParams { + name: "my-project".parse().unwrap(), + description: "".to_string(), + }, + }, + ); + let (.., project) = + datastore.project_create(&opctx, project).await.unwrap(); + + // create an IP pool for the silo, add a range to it, and link it to the silo + let identity = IdentityMetadataCreateParams { + name: "my-pool".parse().unwrap(), + description: "".to_string(), + }; + let pool = datastore + .ip_pool_create(&opctx, IpPool::new(&identity, IpVersion::V6)) + .await + .expect("Failed to create IP pool"); + let authz_pool = authz::IpPool::new( + authz::FLEET, + pool.id(), + LookupType::ById(pool.id()), + ); + let link = IpPoolResource { + ip_pool_id: pool.id(), + resource_type: IpPoolResourceType::Silo, + resource_id: authz_silo.id(), + is_default: true, + }; + datastore + .ip_pool_link_silo(&opctx, link) + .await + .expect("Could not link pool to silo"); + + // capacity of zero because there are no ranges + let max_ips = datastore + .ip_pool_total_capacity(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(max_ips.ipv4, 0); + assert_eq!(max_ips.ipv6, 0); + + // Add an IPv6 range let ipv6_range = IpRange::V6( Ipv6Range::new( std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 10), @@ -1493,16 +1644,46 @@ mod test { .unwrap(), ); datastore - .ip_pool_add_range(&opctx, &authz_pool, &ipv6_range) + .ip_pool_add_range(&opctx, &authz_pool, &pool, &ipv6_range) .await .expect("Could not add range"); + let max_ips = datastore + .ip_pool_total_capacity(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(max_ips.ipv4, 0); + assert_eq!(max_ips.ipv6, 11 + 65536); + + let ip_count = datastore + .ip_pool_allocated_count(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(ip_count.ipv4, 0); + assert_eq!(ip_count.ipv6, 0); - // now test with additional v6 range + let identity = IdentityMetadataCreateParams { + name: "my-ip".parse().unwrap(), + description: "".to_string(), + }; + let ip = datastore + .allocate_floating_ip(&opctx, project.id(), identity, None, None) + .await + .expect("Could not allocate floating IP"); + assert_eq!(ip.ip.to_string(), "fd00::a/128"); + + let ip_count = datastore + .ip_pool_allocated_count(&opctx, &authz_pool) + .await + .unwrap(); + assert_eq!(ip_count.ipv4, 0); + assert_eq!(ip_count.ipv6, 1); + + // allocating one has nothing to do with total capacity let max_ips = datastore .ip_pool_total_capacity(&opctx, &authz_pool) .await .unwrap(); - assert_eq!(max_ips.ipv4, 5); + assert_eq!(max_ips.ipv4, 0); assert_eq!(max_ips.ipv6, 11 + 65536); // add a giant range for fun @@ -1516,7 +1697,7 @@ mod test { .unwrap(), ); datastore - .ip_pool_add_range(&opctx, &authz_pool, &ipv6_range) + .ip_pool_add_range(&opctx, &authz_pool, &pool, &ipv6_range) .await .expect("Could not add range"); @@ -1524,10 +1705,71 @@ mod test { .ip_pool_total_capacity(&opctx, &authz_pool) .await .unwrap(); - assert_eq!(max_ips.ipv4, 5); + assert_eq!(max_ips.ipv4, 0); assert_eq!(max_ips.ipv6, 1208925819614629174706166); db.terminate().await; logctx.cleanup_successful(); } + + #[tokio::test] + async fn cannot_insert_range_in_pool_with_different_ip_version() { + let logctx = dev::test_setup_log( + "cannot_insert_range_in_pool_with_different_ip_version", + ); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + + // IP pool versions, and ranges of the opposite version. + let versions = [IpVersion::V4, IpVersion::V6]; + let ranges = [ + IpRange::V6( + Ipv6Range::new( + std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 1, 21), + std::net::Ipv6Addr::new( + 0xfd00, 0, 0, 0xffff, 0xffff, 0xffff, 0xffff, 0xffff, + ), + ) + .unwrap(), + ), + IpRange::V4( + Ipv4Range::new( + std::net::Ipv4Addr::new(10, 0, 0, 1), + std::net::Ipv4Addr::new(10, 0, 0, 5), + ) + .unwrap(), + ), + ]; + + for (version, range) in versions.into_iter().zip(ranges) { + // Create the pool + let identity = IdentityMetadataCreateParams { + name: format!("ip{version}-pool-for-silo").parse().unwrap(), + description: "".to_string(), + }; + let pool = datastore + .ip_pool_create(&opctx, IpPool::new(&identity, version)) + .await + .expect("Failed to create IP pool"); + let authz_pool = authz::IpPool::new( + authz::FLEET, + pool.id(), + LookupType::ById(pool.id()), + ); + + // Ensure we cannot insert a range of the other version. + let res = datastore + .ip_pool_add_range(&opctx, &authz_pool, &pool, &range) + .await; + assert!( + res.is_err(), + "Should have failed to insert an IP{} range in a IP{} pool", + range.version(), + version, + ); + } + + db.terminate().await; + logctx.cleanup_successful(); + } } diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 1d148c74200..317680cbf7f 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -151,8 +151,11 @@ pub use volume::*; // TODO: This should likely turn into a configuration option. pub const REGION_REDUNDANCY_THRESHOLD: usize = 3; -/// The name of the built-in IP pool for Oxide services. -pub const SERVICE_IP_POOL_NAME: &str = "oxide-service-pool"; +/// The name of the built-in IPv4 IP pool for Oxide services. +pub const SERVICE_IPV4_POOL_NAME: &str = "oxide-service-pool-v4"; + +/// The name of the built-in IPv6 IP pool for Oxide services. +pub const SERVICE_IPV6_POOL_NAME: &str = "oxide-service-pool-v6"; /// "limit" to be used in SQL queries that paginate through large result sets /// diff --git a/nexus/db-queries/src/db/datastore/network_interface.rs b/nexus/db-queries/src/db/datastore/network_interface.rs index 0bb376d4884..6c46fd4074f 100644 --- a/nexus/db-queries/src/db/datastore/network_interface.rs +++ b/nexus/db-queries/src/db/datastore/network_interface.rs @@ -31,6 +31,7 @@ use nexus_db_errors::ErrorHandler; use nexus_db_errors::OptionalError; use nexus_db_errors::public_error_from_diesel; use nexus_db_lookup::DbConnection; +use nexus_db_model::IpVersion; use nexus_db_model::ServiceNetworkInterface; use nexus_types::identity::Resource; use omicron_common::api::external::DataPageParams; @@ -190,7 +191,11 @@ impl DataStore { // instance network interfaces, which require ListChildren on the // instance to list). As a logical proxy, we check for listing children // of the service IP pool. - let (authz_pool, _pool) = self.ip_pools_service_lookup(opctx).await?; + // + // Note that the IP version doesn't matter here, both pools have the + // same permissions. + let (authz_pool, _pool) = + self.ip_pools_service_lookup(opctx, IpVersion::V4).await?; opctx.authorize(authz::Action::ListChildren, &authz_pool).await?; paginated(dsl::service_network_interface, dsl::id, pagparams) @@ -250,8 +255,11 @@ impl DataStore { // pool) and a network interface (in the relevant VpcSubnet). Putting // this check here ensures that the caller can't proceed if they also // couldn't proceed with creating the corresponding external IP. + // + // Note that the IP version here doesn't matter, both IPv4 and IPv6 + // service pools have the same permissions. let (authz_service_ip_pool, _) = self - .ip_pools_service_lookup(opctx) + .ip_pools_service_lookup(opctx, IpVersion::V4) .await .map_err(network_interface::InsertError::External)?; opctx @@ -431,8 +439,11 @@ impl DataStore { // instance network interfaces, which require permissions on the // instance). As a logical proxy, we check for listing children of the // service IP pool. + // + // Note that the IP version here doesn't matter, both pools have the + // same permissions. let (authz_service_ip_pool, _) = self - .ip_pools_service_lookup(opctx) + .ip_pools_service_lookup(opctx, IpVersion::V4) .await .map_err(network_interface::DeleteError::External)?; opctx @@ -890,7 +901,16 @@ impl DataStore { // instance network interfaces, which require ListChildren on the // instance to list). As a logical proxy, we check for listing children // of the service IP pool. - let (authz_pool, _pool) = self.ip_pools_service_lookup(opctx).await?; + // + // TODO-cleanup: https://github.com/oxidecomputer/omicron/issues/8873. + // This authz check looks at the builtin services IP Pool, but we're + // listing _instance_ NICs. That seems to be authorizing against the + // wrong resource. + // + // But assuming this check is correct, both service pools have the same + // permissions, so the actual IP version here doesn't matter. + let (authz_pool, _pool) = + self.ip_pools_service_lookup(opctx, IpVersion::V4).await?; opctx.authorize(authz::Action::ListChildren, &authz_pool).await?; paginated(dsl::instance_network_interface, dsl::id, pagparams) diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index ce0e4f72244..3f4ccf52612 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -5,8 +5,10 @@ //! [`DataStore`] methods on [`Rack`]s. use super::DataStore; -use super::SERVICE_IP_POOL_NAME; +use super::SERVICE_IPV4_POOL_NAME; +use super::SERVICE_IPV6_POOL_NAME; use super::dns::DnsVersionUpdateBuilder; +use super::ip_pool::ServiceIpPools; use crate::authz; use crate::context::OpContext; use crate::db; @@ -37,6 +39,7 @@ use nexus_db_lookup::DbConnection; use nexus_db_lookup::LookupPath; use nexus_db_model::IncompleteNetworkInterface; use nexus_db_model::InitialDnsGroup; +use nexus_db_model::IpVersion; use nexus_db_model::PasswordHashString; use nexus_db_model::SiloUser; use nexus_db_model::SiloUserPasswordHash; @@ -513,7 +516,7 @@ impl DataStore { &self, conn: &async_bb8_diesel::Connection, log: &slog::Logger, - service_pool: &db::model::IpPool, + service_pools: &ServiceIpPools, zone_config: &BlueprintZoneConfig, ) -> Result<(), RackInitError> { // For services with external connectivity, we record their @@ -607,6 +610,8 @@ impl DataStore { ); return Ok(()); }; + let service_pool = + service_pools.pool_for_version(external_ip.ip_version().into()); let db_ip = IncompleteExternalIp::for_omicron_zone( service_pool.id(), external_ip, @@ -664,8 +669,10 @@ impl DataStore { opctx.authorize(authz::Action::CreateChild, &authz::FLEET).await?; - let (authz_service_pool, service_pool) = - self.ip_pools_service_lookup(&opctx).await?; + // We may need to populate external IP records for both IPv4 and IPv6 + // service pools, so fetch both now. + let service_ip_pools = + self.ip_pools_service_lookup_both_versions(&opctx).await?; // NOTE: This operation could likely be optimized with a CTE, but given // the low-frequency of calls, this optimization has been deferred. @@ -679,9 +686,7 @@ impl DataStore { .transaction(&conn, |conn| { let err = err.clone(); let log = log.clone(); - let authz_service_pool = authz_service_pool.clone(); let rack_init = rack_init.clone(); - let service_pool = service_pool.clone(); async move { let rack_id = rack_init.rack_id; let blueprint = rack_init.blueprint; @@ -728,10 +733,12 @@ impl DataStore { // Set up the IP pool for internal services. for range in service_ip_pool_ranges { + let service_pool = service_ip_pools.pool_for_range(&range); Self::ip_pool_add_range_on_connection( &conn, opctx, - &authz_service_pool, + &service_pool.authz_pool, + &service_pool.db_pool, &range, ) .await @@ -792,7 +799,7 @@ impl DataStore { self.rack_populate_service_networking_records( &conn, &log, - &service_pool, + &service_ip_pools, zone_config, ) .await @@ -954,36 +961,54 @@ impl DataStore { self.rack_insert(opctx, &db::model::Rack::new(rack_id)).await?; - let internal_pool = - db::model::IpPool::new(&IdentityMetadataCreateParams { - name: SERVICE_IP_POOL_NAME.parse::().unwrap(), - description: String::from("IP Pool for Oxide Services"), - }); + // Insert and link the services IP Pool for both IP versions. + for (version, name) in [ + (IpVersion::V4, SERVICE_IPV4_POOL_NAME), + (IpVersion::V6, SERVICE_IPV6_POOL_NAME), + ] { + let internal_pool = db::model::IpPool::new( + &IdentityMetadataCreateParams { + name: name.parse::().unwrap(), + description: format!( + "IP{version} IP Pool for Oxide Services" + ), + }, + version, + ); - let internal_pool_id = internal_pool.id(); + let internal_pool_id = internal_pool.id(); - let internal_created = self - .ip_pool_create(opctx, internal_pool) - .await - .map(|_| true) - .or_else(|e| match e { - Error::ObjectAlreadyExists { .. } => Ok(false), - _ => Err(e), - })?; - - // make default for the internal silo. only need to do this if - // the create went through, i.e., if it wasn't already there - if internal_created { - self.ip_pool_link_silo( - opctx, - db::model::IpPoolResource { - ip_pool_id: internal_pool_id, - resource_type: db::model::IpPoolResourceType::Silo, - resource_id: INTERNAL_SILO_ID, - is_default: true, - }, - ) - .await?; + let internal_created = self + .ip_pool_create(opctx, internal_pool) + .await + .map(|_| true) + .or_else(|e| match e { + Error::ObjectAlreadyExists { .. } => Ok(false), + _ => Err(e), + })?; + + // make default for the internal silo. only need to do this if + // the create went through, i.e., if it wasn't already there + // + // TODO-completeness: We're linking the IPv4 pool only here, but we + // need a way for the operator to control this, either at RSS or + // through the API. An alternative is to not set a default at all, + // even though both are linked. + // + // See https://github.com/oxidecomputer/omicron/issues/8884 + if internal_created { + let is_default = matches!(version, IpVersion::V4); + self.ip_pool_link_silo( + opctx, + db::model::IpPoolResource { + ip_pool_id: internal_pool_id, + resource_type: db::model::IpPoolResourceType::Silo, + resource_id: INTERNAL_SILO_ID, + is_default, + }, + ) + .await?; + } } Ok(()) @@ -1028,6 +1053,7 @@ mod test { use nexus_types::internal_api::params::DnsRecord; use nexus_types::inventory::NetworkInterface; use nexus_types::inventory::NetworkInterfaceKind; + use omicron_common::address::NEXUS_OPTE_IPV6_SUBNET; use omicron_common::address::{ DNS_OPTE_IPV4_SUBNET, NEXUS_OPTE_IPV4_SUBNET, NTP_OPTE_IPV4_SUBNET, }; @@ -1043,6 +1069,7 @@ mod test { use omicron_uuid_kinds::{SledUuid, TypedUuid}; use oxnet::IpNet; use std::collections::{BTreeMap, HashMap}; + use std::net::Ipv6Addr; use std::net::{IpAddr, Ipv4Addr, SocketAddr}; use std::num::NonZeroU32; @@ -1633,10 +1660,12 @@ mod test { assert_eq!(ntp2_external_ip.last_port.0, 16383); // Furthermore, we should be able to see that these IP addresses have - // been allocated as a part of the service IP pool. - let (.., svc_pool) = - datastore.ip_pools_service_lookup(&opctx).await.unwrap(); - assert_eq!(svc_pool.name().as_str(), "oxide-service-pool"); + // been allocated as a part of a service IP pool. + let (.., svc_pool) = datastore + .ip_pools_service_lookup(&opctx, IpVersion::V4) + .await + .unwrap(); + assert_eq!(svc_pool.name().as_str(), SERVICE_IPV4_POOL_NAME); let observed_ip_pool_ranges = get_all_ip_pool_ranges(&datastore).await; assert_eq!(observed_ip_pool_ranges.len(), 1); @@ -1918,14 +1947,250 @@ mod test { ); // Furthermore, we should be able to see that this IP addresses have been - // allocated as a part of the service IP pool. - let (.., svc_pool) = - datastore.ip_pools_service_lookup(&opctx).await.unwrap(); - assert_eq!(svc_pool.name().as_str(), "oxide-service-pool"); + // allocated as a part of a service IP pool. + let (.., svc_pool) = datastore + .ip_pools_service_lookup(&opctx, IpVersion::V4) + .await + .unwrap(); + assert_eq!(svc_pool.name().as_str(), SERVICE_IPV4_POOL_NAME); + + let observed_ip_pool_ranges = get_all_ip_pool_ranges(&datastore).await; + assert_eq!(observed_ip_pool_ranges.len(), 1); + assert_eq!(observed_ip_pool_ranges[0].ip_pool_id, svc_pool.id()); + + let observed_datasets = get_all_crucible_datasets(&datastore).await; + assert!(observed_datasets.is_empty()); + + // Verify the internal and external DNS configurations. + let dns_config_internal = datastore + .dns_config_read(&opctx, DnsGroup::Internal) + .await + .unwrap(); + assert_eq!(u64::from(dns_config_internal.generation), 1); + assert_eq!(dns_config_internal.zones.len(), 1); + assert_eq!(dns_config_internal.zones[0].zone_name, DNS_ZONE); + assert_eq!( + dns_config_internal.zones[0].records, + HashMap::from([("nexus".to_string(), internal_records)]), + ); + + let dns_config_external = datastore + .dns_config_read(&opctx, DnsGroup::External) + .await + .unwrap(); + assert_eq!(u64::from(dns_config_external.generation), 2); + assert_eq!(dns_config_external.zones.len(), 1); + assert_eq!( + dns_config_external.zones[0].zone_name, + "test-suite.oxide.test", + ); + assert_eq!( + dns_config_external.zones[0].records.get("api.sys"), + Some(&external_records) + ); + + db.terminate().await; + logctx.cleanup_successful(); + } + + #[tokio::test] + async fn rack_set_initialized_with_ipv6_public_addresses() { + let test_name = "rack_set_initialized_with_ipv6_public_addresses"; + let logctx = dev::test_setup_log(test_name); + let db = TestDatabase::new_with_datastore(&logctx.log).await; + let (opctx, datastore) = (db.opctx(), db.datastore()); + + let sled = create_test_sled(&datastore, Uuid::new_v4()).await; + + // Ask for a Nexus service with an IPv6 address. + let nexus_ip_start = + 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![ + 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()) + .sled( + SledBuilder::new().id(TypedUuid::from_untyped_uuid(sled.id())), + ) + .expect("failed to add sled"); + + let nexus_id = OmicronZoneUuid::new_v4(); + let nexus_pip = NEXUS_OPTE_IPV6_SUBNET + .nth(NUM_INITIAL_RESERVED_IP_ADDRESSES as u128 + 1) + .unwrap(); + let mut macs = MacAddr::iter_system(); + + let mut blueprint_zones = BTreeMap::new(); + blueprint_zones.insert( + SledUuid::from_untyped_uuid(sled.id()), + [BlueprintZoneConfig { + disposition: BlueprintZoneDisposition::InService, + id: nexus_id, + filesystem_pool: random_zpool(), + zone_type: BlueprintZoneType::Nexus( + blueprint_zone_type::Nexus { + internal_address: "[::1]:80".parse().unwrap(), + external_ip: OmicronZoneExternalFloatingIp { + id: ExternalIpUuid::new_v4(), + ip: nexus_ip_start.into(), + }, + external_tls: false, + external_dns_servers: vec![], + nic: NetworkInterface { + id: Uuid::new_v4(), + kind: NetworkInterfaceKind::Service { + id: nexus_id.into_untyped_uuid(), + }, + name: "nexus1".parse().unwrap(), + ip: nexus_pip.into(), + mac: macs.next().unwrap(), + subnet: IpNet::from(*NEXUS_OPTE_IPV6_SUBNET), + vni: Vni::SERVICES_VNI, + primary: true, + slot: 0, + transit_ips: vec![], + }, + }, + ), + image_source: BlueprintZoneImageSource::InstallDataset, + }] + .into_iter() + .collect::>(), + ); + + let datasets = vec![]; + + let internal_records = vec![ + DnsRecord::Aaaa("fe80::1:2:3:4".parse().unwrap()), + DnsRecord::Aaaa("fe80::1:2:3:5".parse().unwrap()), + ]; + let internal_dns = InitialDnsGroup::new( + DnsGroup::Internal, + DNS_ZONE, + "test suite", + "initial test suite internal rev", + HashMap::from([("nexus".to_string(), internal_records.clone())]), + ); + + let external_records = + vec![DnsRecord::Aaaa("fe80::5:6:7:8".parse().unwrap())]; + let external_dns = InitialDnsGroup::new( + DnsGroup::External, + "test-suite.oxide.test", + "test suite", + "initial test suite external rev", + HashMap::from([("api.sys".to_string(), external_records.clone())]), + ); + + let blueprint_id = BlueprintUuid::new_v4(); + let blueprint = Blueprint { + id: blueprint_id, + sleds: make_sled_config_only_zones(blueprint_zones), + pending_mgs_updates: PendingMgsUpdates::new(), + cockroachdb_setting_preserve_downgrade: + CockroachDbPreserveDowngrade::DoNotModify, + parent_blueprint_id: None, + internal_dns_version: *Generation::new(), + external_dns_version: *Generation::new(), + target_release_minimum_generation: *Generation::new(), + cockroachdb_fingerprint: String::new(), + clickhouse_cluster_config: None, + oximeter_read_version: *Generation::new(), + oximeter_read_mode: OximeterReadMode::SingleNode, + time_created: now_db_precision(), + creator: "test suite".to_string(), + comment: "test blueprint".to_string(), + report: PlanningReport::new(blueprint_id), + }; + + let rack = datastore + .rack_set_initialized( + &opctx, + RackInit { + blueprint: blueprint.clone(), + datasets: datasets.clone(), + service_ip_pool_ranges, + internal_dns, + external_dns, + ..Default::default() + }, + ) + .await + .expect("Failed to initialize rack"); + + assert_eq!(rack.id(), rack_id()); + assert!(rack.initialized); + + // We should see the blueprint we passed in. + let (_blueprint_target, observed_blueprint) = datastore + .blueprint_target_get_current_full(&opctx) + .await + .expect("failed to read blueprint"); + assert_eq!(observed_blueprint, blueprint); + + // We should see the Nexus service we provisioned. + let mut observed_zones: Vec<_> = observed_blueprint + .all_omicron_zones(BlueprintZoneDisposition::any) + .map(|(_, z)| z) + .collect(); + observed_zones.sort_by_key(|z| z.id); + assert_eq!(observed_zones.len(), 1); + + // We should see the IP allocated for this service. + let observed_external_ips = get_all_external_ips(&datastore).await; + for external_ip in &observed_external_ips { + assert!(external_ip.is_service); + assert!(external_ip.parent_id.is_some()); + assert_eq!(external_ip.kind, IpKind::Floating); + } + let observed_external_ips: HashMap<_, _> = observed_external_ips + .into_iter() + .map(|ip| (ip.parent_id.unwrap(), ip)) + .collect(); + assert_eq!(observed_external_ips.len(), 1); + + // The address allocated for the service should match the input. + let actual_ip = observed_external_ips + [observed_zones[0].id.as_untyped_uuid()] + .ip + .ip(); + assert_eq!( + actual_ip, + if let BlueprintZoneType::Nexus(blueprint_zone_type::Nexus { + external_ip, + .. + }) = &blueprint + .all_omicron_zones(BlueprintZoneDisposition::any) + .next() + .unwrap() + .1 + .zone_type + { + external_ip.ip + } else { + panic!("Unexpected zone type") + } + ); + assert_eq!(actual_ip, nexus_ip_start); + + // Furthermore, we should be able to see that this IP address has been + // allocated as a part of a service IPv6 IP pool. + let (.., svc_pool) = datastore + .ip_pools_service_lookup(&opctx, IpVersion::V6) + .await + .unwrap(); + assert_eq!(svc_pool.name().as_str(), SERVICE_IPV6_POOL_NAME); let observed_ip_pool_ranges = get_all_ip_pool_ranges(&datastore).await; assert_eq!(observed_ip_pool_ranges.len(), 1); assert_eq!(observed_ip_pool_ranges[0].ip_pool_id, svc_pool.id()); + assert_eq!(observed_ip_pool_ranges[0].first_address.ip(), actual_ip); let observed_datasets = get_all_crucible_datasets(&datastore).await; assert!(observed_datasets.is_empty()); diff --git a/nexus/db-queries/src/db/queries/external_ip.rs b/nexus/db-queries/src/db/queries/external_ip.rs index 5887dc620c0..5dca7772b2f 100644 --- a/nexus/db-queries/src/db/queries/external_ip.rs +++ b/nexus/db-queries/src/db/queries/external_ip.rs @@ -866,7 +866,7 @@ impl RunQueryDsl for NextExternalIp {} #[cfg(test)] mod tests { use crate::authz; - use crate::db::datastore::SERVICE_IP_POOL_NAME; + use crate::db::datastore::SERVICE_IPV4_POOL_NAME; use crate::db::identity::Resource; use crate::db::model::IpKind; use crate::db::model::IpPool; @@ -881,6 +881,7 @@ mod tests { use nexus_db_model::InstanceCpuCount; use nexus_db_model::IpPoolResource; use nexus_db_model::IpPoolResourceType; + use nexus_db_model::IpVersion; use nexus_sled_agent_shared::inventory::ZoneKind; use nexus_types::deployment::OmicronZoneExternalFloatingIp; use nexus_types::deployment::OmicronZoneExternalIp; @@ -920,10 +921,13 @@ mod tests { range: IpRange, is_default: bool, ) -> authz::IpPool { - let pool = IpPool::new(&IdentityMetadataCreateParams { - name: String::from(name).parse().unwrap(), - description: format!("ip pool {}", name), - }); + let pool = IpPool::new( + &IdentityMetadataCreateParams { + name: name.parse().unwrap(), + description: format!("ip pool {}", name), + }, + IpVersion::V4, + ); self.db .datastore() @@ -1341,7 +1345,7 @@ mod tests { Ipv4Addr::new(10, 0, 0, 4), )) .unwrap(); - context.initialize_ip_pool(SERVICE_IP_POOL_NAME, ip_range).await; + context.initialize_ip_pool(SERVICE_IPV4_POOL_NAME, ip_range).await; let ip_10_0_0_2 = OmicronZoneExternalIp::Floating(OmicronZoneExternalFloatingIp { @@ -1546,7 +1550,7 @@ mod tests { Ipv4Addr::new(10, 0, 0, 4), )) .unwrap(); - context.initialize_ip_pool(SERVICE_IP_POOL_NAME, ip_range).await; + context.initialize_ip_pool(SERVICE_IPV4_POOL_NAME, ip_range).await; let ip_10_0_0_5 = OmicronZoneExternalIp::Floating(OmicronZoneExternalFloatingIp { diff --git a/nexus/db-schema/src/enums.rs b/nexus/db-schema/src/enums.rs index 39210a32d6f..1766054c9ad 100644 --- a/nexus/db-schema/src/enums.rs +++ b/nexus/db-schema/src/enums.rs @@ -55,6 +55,7 @@ define_enums! { IpAttachStateEnum => "ip_attach_state", IpKindEnum => "ip_kind", IpPoolResourceTypeEnum => "ip_pool_resource_type", + IpVersionEnum => "ip_version", MigrationStateEnum => "migration_state", NetworkInterfaceKindEnum => "network_interface_kind", OximeterReadModeEnum => "oximeter_read_mode", diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index 29336b8e1a9..7d4daff9d2d 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -626,6 +626,7 @@ table! { time_created -> Timestamptz, time_modified -> Timestamptz, time_deleted -> Nullable, + ip_version -> crate::enums::IpVersionEnum, rcgen -> Int8, } } diff --git a/nexus/reconfigurator/execution/src/dns.rs b/nexus/reconfigurator/execution/src/dns.rs index 85bc01a30ac..2ce03d66421 100644 --- a/nexus/reconfigurator/execution/src/dns.rs +++ b/nexus/reconfigurator/execution/src/dns.rs @@ -1375,6 +1375,30 @@ mod test { DnsDiff::new(left_zone, right_zone).unwrap() } + async fn fetch_all_service_ip_pool_ranges( + datastore: &DataStore, + opctx: &OpContext, + ) -> Vec { + let service_pools = datastore + .ip_pools_service_lookup_both_versions(&opctx) + .await + .expect("success looking up both versions of the service IP Pools"); + let mut ranges = datastore + .ip_pool_list_ranges_batched(&opctx, &service_pools.ipv4.authz_pool) + .await + .expect("success listing IPv4 pool ranges"); + ranges.append( + &mut datastore + .ip_pool_list_ranges_batched( + &opctx, + &service_pools.ipv6.authz_pool, + ) + .await + .expect("success listing IPv6 pool ranges"), + ); + ranges + } + // Tests end-to-end DNS behavior: // // - If we create a blueprint matching the current system, and then apply @@ -1474,14 +1498,8 @@ mod test { .unwrap(); let zpool_rows = datastore.zpool_list_all_external_batched(&opctx).await.unwrap(); - let ip_pool_range_rows = { - let (authz_service_ip_pool, _) = - datastore.ip_pools_service_lookup(&opctx).await.unwrap(); - datastore - .ip_pool_list_ranges_batched(&opctx, &authz_service_ip_pool) - .await - .unwrap() - }; + let ip_pool_range_rows = + fetch_all_service_ip_pool_ranges(&datastore, &opctx).await; let planning_input = { let mut builder = PlanningInputFromDb { sled_rows: &sled_rows, diff --git a/nexus/reconfigurator/preparation/src/lib.rs b/nexus/reconfigurator/preparation/src/lib.rs index 49751223fc1..20c8cb63690 100644 --- a/nexus/reconfigurator/preparation/src/lib.rs +++ b/nexus/reconfigurator/preparation/src/lib.rs @@ -117,16 +117,8 @@ impl PlanningInputFromDb<'_> { .zpool_list_all_external_batched(opctx) .await .internal_context("fetching all external zpool rows")?; - let ip_pool_range_rows = { - let (authz_service_ip_pool, _) = datastore - .ip_pools_service_lookup(opctx) - .await - .internal_context("fetching IP services pool")?; - datastore - .ip_pool_list_ranges_batched(opctx, &authz_service_ip_pool) - .await - .internal_context("listing services IP pool ranges")? - }; + let ip_pool_range_rows = + fetch_all_service_ip_pool_ranges(opctx, datastore).await?; let external_ip_rows = datastore .external_ip_list_service_all_batched(opctx) .await @@ -367,6 +359,26 @@ impl PlanningInputFromDb<'_> { } } +async fn fetch_all_service_ip_pool_ranges( + opctx: &OpContext, + datastore: &DataStore, +) -> Result, Error> { + let service_pools = datastore + .ip_pools_service_lookup_both_versions(opctx) + .await + .internal_context("fetching IP services pools")?; + let mut ranges = datastore + .ip_pool_list_ranges_batched(opctx, &service_pools.ipv4.authz_pool) + .await + .internal_context("listing services IPv4 pool ranges")?; + let mut v6_ranges = datastore + .ip_pool_list_ranges_batched(opctx, &service_pools.ipv6.authz_pool) + .await + .internal_context("listing services IPv6 pool ranges")?; + ranges.append(&mut v6_ranges); + Ok(ranges) +} + /// Loads state for import into `reconfigurator-cli` /// /// This is only to be used in omdb or tests. diff --git a/nexus/src/app/ip_pool.rs b/nexus/src/app/ip_pool.rs index 07b3ea8b7d9..893bb0b3e2a 100644 --- a/nexus/src/app/ip_pool.rs +++ b/nexus/src/app/ip_pool.rs @@ -9,6 +9,7 @@ use crate::external_api::shared::IpRange; use ipnetwork::IpNetwork; use nexus_db_lookup::LookupPath; use nexus_db_lookup::lookup; +use nexus_db_model::IpVersion; use nexus_db_queries::authz; use nexus_db_queries::authz::ApiResource; use nexus_db_queries::context::OpContext; @@ -71,7 +72,11 @@ impl super::Nexus { opctx: &OpContext, pool_params: ¶ms::IpPoolCreate, ) -> CreateResult { - let pool = db::model::IpPool::new(&pool_params.identity); + let pool = db::model::IpPool::new( + &pool_params.identity, + // https://github.com/oxidecomputer/omicron/issues/8881 + IpVersion::V4, + ); self.db_datastore.ip_pool_create(opctx, pool).await } @@ -302,7 +307,7 @@ impl super::Nexus { pool_lookup: &lookup::IpPool<'_>, range: &IpRange, ) -> UpdateResult { - let (.., authz_pool, _db_pool) = + let (.., authz_pool, db_pool) = pool_lookup.fetch_for(authz::Action::Modify).await?; if self.db_datastore.ip_pool_is_internal(opctx, &authz_pool).await? { @@ -316,13 +321,17 @@ impl super::Nexus { // would be nice if we could do it in the datastore layer, but we'd // have no way of creating IPv6 ranges for the purpose of testing IP // pool utilization. + // + // See https://github.com/oxidecomputer/omicron/issues/8761. if matches!(range, IpRange::V6(_)) { return Err(Error::invalid_request( "IPv6 ranges are not allowed yet", )); } - self.db_datastore.ip_pool_add_range(opctx, &authz_pool, range).await + self.db_datastore + .ip_pool_add_range(opctx, &authz_pool, &db_pool, range) + .await } pub(crate) async fn ip_pool_delete_range( @@ -351,8 +360,11 @@ impl super::Nexus { &self, opctx: &OpContext, ) -> LookupResult { - let (authz_pool, db_pool) = - self.db_datastore.ip_pools_service_lookup(opctx).await?; + // TODO: https://github.com/oxidecomputer/omicron/issues/8881 + let (authz_pool, db_pool) = self + .db_datastore + .ip_pools_service_lookup(opctx, IpVersion::V4) + .await?; opctx.authorize(authz::Action::Read, &authz_pool).await?; Ok(db_pool) } @@ -362,8 +374,11 @@ impl super::Nexus { opctx: &OpContext, pagparams: &DataPageParams<'_, IpNetwork>, ) -> ListResultVec { - let (authz_pool, ..) = - self.db_datastore.ip_pools_service_lookup(opctx).await?; + // TODO: https://github.com/oxidecomputer/omicron/issues/8881 + let (authz_pool, ..) = self + .db_datastore + .ip_pools_service_lookup(opctx, IpVersion::V4) + .await?; opctx.authorize(authz::Action::Read, &authz_pool).await?; self.db_datastore .ip_pool_list_ranges(opctx, &authz_pool, pagparams) @@ -375,9 +390,6 @@ impl super::Nexus { opctx: &OpContext, range: &IpRange, ) -> UpdateResult { - let (authz_pool, ..) = - self.db_datastore.ip_pools_service_lookup(opctx).await?; - opctx.authorize(authz::Action::Modify, &authz_pool).await?; // Disallow V6 ranges until IPv6 is fully supported by the networking // subsystem. Instead of changing the API to reflect that (making this // endpoint inconsistent with the rest) and changing it back when we @@ -385,12 +397,21 @@ impl super::Nexus { // would be nice if we could do it in the datastore layer, but we'd // have no way of creating IPv6 ranges for the purpose of testing IP // pool utilization. + // + // See https://github.com/oxidecomputer/omicron/issues/8761. if matches!(range, IpRange::V6(_)) { return Err(Error::invalid_request( "IPv6 ranges are not allowed yet", )); } - self.db_datastore.ip_pool_add_range(opctx, &authz_pool, range).await + let (authz_pool, db_pool) = self + .db_datastore + .ip_pools_service_lookup(opctx, range.version().into()) + .await?; + opctx.authorize(authz::Action::Modify, &authz_pool).await?; + self.db_datastore + .ip_pool_add_range(opctx, &authz_pool, &db_pool, range) + .await } pub(crate) async fn ip_pool_service_delete_range( @@ -398,8 +419,10 @@ impl super::Nexus { opctx: &OpContext, range: &IpRange, ) -> DeleteResult { - let (authz_pool, ..) = - self.db_datastore.ip_pools_service_lookup(opctx).await?; + let (authz_pool, ..) = self + .db_datastore + .ip_pools_service_lookup(opctx, range.version().into()) + .await?; opctx.authorize(authz::Action::Modify, &authz_pool).await?; self.db_datastore.ip_pool_delete_range(opctx, &authz_pool, range).await } diff --git a/nexus/src/app/rack.rs b/nexus/src/app/rack.rs index 6f71055e8dc..0c2e25bd0c6 100644 --- a/nexus/src/app/rack.rs +++ b/nexus/src/app/rack.rs @@ -150,7 +150,6 @@ impl super::Nexus { }) .collect(); - let service_ip_pool_ranges = request.internal_services_ip_pool_ranges; let tls_certificates: Vec<_> = request .certs .into_iter() @@ -708,6 +707,7 @@ impl super::Nexus { } // TODO - https://github.com/oxidecomputer/omicron/issues/3277 // record port speed + let service_ip_pool_ranges = request.internal_services_ip_pool_ranges; self.db_datastore .rack_set_initialized( opctx, diff --git a/nexus/tests/integration_tests/ip_pools.rs b/nexus/tests/integration_tests/ip_pools.rs index 58a3d8b249a..52be987643d 100644 --- a/nexus/tests/integration_tests/ip_pools.rs +++ b/nexus/tests/integration_tests/ip_pools.rs @@ -14,7 +14,7 @@ use http::StatusCode; use http::method::Method; use nexus_db_queries::authz; use nexus_db_queries::context::OpContext; -use nexus_db_queries::db::datastore::SERVICE_IP_POOL_NAME; +use nexus_db_queries::db::datastore::SERVICE_IPV4_POOL_NAME; use nexus_db_queries::db::fixed_data::silo::DEFAULT_SILO; use nexus_test_utils::http_testing::AuthnMode; use nexus_test_utils::http_testing::NexusRequest; @@ -57,7 +57,6 @@ use nexus_types::silo::INTERNAL_SILO_ID; use omicron_common::address::Ipv6Range; use omicron_common::api::external::IdentityMetadataUpdateParams; use omicron_common::api::external::InstanceState; -use omicron_common::api::external::LookupType; use omicron_common::api::external::NameOrId; use omicron_common::api::external::SimpleIdentityOrName; use omicron_common::api::external::{IdentityMetadataCreateParams, Name}; @@ -326,7 +325,7 @@ async fn test_ip_pool_service_no_cud(cptestctx: &ControlPlaneTestContext) { let client = &cptestctx.external_client; let internal_pool_name_url = - format!("/v1/system/ip-pools/{}", SERVICE_IP_POOL_NAME); + format!("/v1/system/ip-pools/{}", SERVICE_IPV4_POOL_NAME); // we can fetch the service pool by name or ID let pool = NexusRequest::object_get(client, &internal_pool_name_url) @@ -354,7 +353,8 @@ async fn test_ip_pool_service_no_cud(cptestctx: &ControlPlaneTestContext) { StatusCode::NOT_FOUND, ) .await; - let not_found_name = "not found: ip-pool with name \"oxide-service-pool\""; + let not_found_name = + "not found: ip-pool with name \"oxide-service-pool-v4\""; assert_eq!(error.message, not_found_name); let not_found_id = @@ -823,10 +823,12 @@ async fn create_pool(client: &ClientTestContext, name: &str) -> IpPool { // testing allocated is done in a bunch of other places. look for // assert_ip_pool_utilization calls #[nexus_test] -async fn test_ip_pool_utilization_total(cptestctx: &ControlPlaneTestContext) { +async fn test_ipv4_ip_pool_utilization_total( + cptestctx: &ControlPlaneTestContext, +) { let client = &cptestctx.external_client; - let pool = create_pool(client, "p0").await; + let _pool = create_pool(client, "p0").await; assert_ip_pool_utilization(client, "p0", 0, 0, 0, 0).await; @@ -843,22 +845,51 @@ async fn test_ip_pool_utilization_total(cptestctx: &ControlPlaneTestContext) { object_create::(client, &add_url, &range).await; assert_ip_pool_utilization(client, "p0", 0, 5, 0, 0).await; +} - // Now let's add a gigantic range. This requires direct datastore - // shenanigans because adding IPv6 ranges through the API is currently not - // allowed. It's worth doing because we want this code to correctly handle - // IPv6 ranges when they are allowed again. +// We're going to test adding an IPv6 pool and collecting its utilization, even +// though that requires operating directly on the datastore. When we resolve +// https://github.com/oxidecomputer/omicron/issues/8881, we can switch this to +// use the API to create the IPv6 pool and ranges instead. +#[nexus_test] +async fn test_ipv6_ip_pool_utilization_total( + cptestctx: &ControlPlaneTestContext, +) { + let client = &cptestctx.external_client; let nexus = &cptestctx.server.server_context().nexus; let datastore = nexus.datastore(); let log = cptestctx.logctx.log.new(o!()); let opctx = OpContext::for_tests(log, datastore.clone()); - let authz_pool = authz::IpPool::new( - authz::FLEET, - pool.identity.id, - LookupType::ByName("p0".to_string()), - ); + let identity = IdentityMetadataCreateParams { + name: "p0".parse().unwrap(), + description: String::new(), + }; + let pool = datastore + .ip_pool_create( + &opctx, + nexus_db_model::IpPool::new( + &identity, + nexus_db_model::IpVersion::V6, + ), + ) + .await + .expect("should be able to create IPv6 pool"); + // Check the utilization is zero. + assert_ip_pool_utilization(client, "p0", 0, 0, 0, 0).await; + + // Now let's add a gigantic range. This requires direct datastore + // shenanigans because adding IPv6 ranges through the API is currently not + // allowed. It's worth doing because we want this code to correctly handle + // IPv6 ranges when they are allowed again. + let by_id = NameOrId::Id(pool.id()); + let (authz_pool, db_pool) = nexus + .ip_pool_lookup(&opctx, &by_id) + .expect("should be able to lookup pool we just created") + .fetch_for(authz::Action::CreateChild) + .await + .expect("should be able to fetch pool we just created"); let big_range = IpRange::V6( Ipv6Range::new( std::net::Ipv6Addr::new(0xfd00, 0, 0, 0, 0, 0, 0, 0), @@ -869,11 +900,11 @@ async fn test_ip_pool_utilization_total(cptestctx: &ControlPlaneTestContext) { .unwrap(), ); datastore - .ip_pool_add_range(&opctx, &authz_pool, &big_range) + .ip_pool_add_range(&opctx, &authz_pool, &db_pool, &big_range) .await .expect("could not add range"); - assert_ip_pool_utilization(client, "p0", 0, 5, 0, 2u128.pow(80)).await; + assert_ip_pool_utilization(client, "p0", 0, 0, 0, 2u128.pow(80)).await; } // Data for testing overlapping IP ranges @@ -1392,9 +1423,12 @@ async fn test_ip_pool_service(cptestctx: &ControlPlaneTestContext) { .unwrap(); assert_eq!( fetched_pool.identity.name, - nexus_db_queries::db::datastore::SERVICE_IP_POOL_NAME + nexus_db_queries::db::datastore::SERVICE_IPV4_POOL_NAME + ); + assert_eq!( + fetched_pool.identity.description, + "IPv4 IP Pool for Oxide Services" ); - assert_eq!(fetched_pool.identity.description, "IP Pool for Oxide Services"); // Fetch any ranges already present. let existing_ranges = diff --git a/nexus/types/src/deployment/network_resources.rs b/nexus/types/src/deployment/network_resources.rs index c44c2884e2f..2e0cbccd82c 100644 --- a/nexus/types/src/deployment/network_resources.rs +++ b/nexus/types/src/deployment/network_resources.rs @@ -7,6 +7,7 @@ use daft::Diffable; use iddqd::TriHashItem; use iddqd::TriHashMap; use iddqd::tri_upcast; +use omicron_common::api::external::IpVersion; use omicron_common::api::external::MacAddr; use omicron_common::api::internal::shared::SourceNatConfig; use omicron_uuid_kinds::ExternalIpUuid; @@ -193,6 +194,14 @@ impl OmicronZoneExternalIp { } } } + + /// Return the IP version of the contained address. + pub fn ip_version(&self) -> IpVersion { + match self.ip() { + IpAddr::V4(_) => IpVersion::V4, + IpAddr::V6(_) => IpVersion::V6, + } + } } /// An IP-based key suitable for uniquely identifying an diff --git a/nexus/types/src/external_api/shared.rs b/nexus/types/src/external_api/shared.rs index 2bb4beb6f12..10f38bf7df4 100644 --- a/nexus/types/src/external_api/shared.rs +++ b/nexus/types/src/external_api/shared.rs @@ -23,7 +23,7 @@ use slog_error_chain::InlineErrorChain; use strum::EnumIter; use uuid::Uuid; -pub use omicron_common::address::{IpRange, Ipv4Range, Ipv6Range}; +pub use omicron_common::address::{IpRange, IpVersion, Ipv4Range, Ipv6Range}; pub use omicron_common::api::external::BfdMode; /// Maximum number of role assignments allowed on any one resource diff --git a/nexus/types/src/external_api/views.rs b/nexus/types/src/external_api/views.rs index 5cfd94ef18e..d6a81606b25 100644 --- a/nexus/types/src/external_api/views.rs +++ b/nexus/types/src/external_api/views.rs @@ -13,6 +13,7 @@ use api_identity::ObjectIdentity; use chrono::DateTime; use chrono::Utc; use daft::Diffable; +pub use omicron_common::api::external::IpVersion; use omicron_common::api::external::{ AffinityPolicy, AllowedSourceIps as ExternalAllowedSourceIps, ByteCount, Digest, Error, FailureDomain, IdentityMetadata, InstanceState, Name, diff --git a/schema/crdb/add-ip-version-to-pools/up01.sql b/schema/crdb/add-ip-version-to-pools/up01.sql new file mode 100644 index 00000000000..404a0fb0751 --- /dev/null +++ b/schema/crdb/add-ip-version-to-pools/up01.sql @@ -0,0 +1,4 @@ +CREATE TYPE IF NOT EXISTS omicron.public.ip_version AS ENUM ( + 'v4', + 'v6' +); diff --git a/schema/crdb/add-ip-version-to-pools/up02.sql b/schema/crdb/add-ip-version-to-pools/up02.sql new file mode 100644 index 00000000000..32498981e1c --- /dev/null +++ b/schema/crdb/add-ip-version-to-pools/up02.sql @@ -0,0 +1,7 @@ +/* + * First, add the IP version column that is nullable. + * The next migration file will update all the rows with the currently-valid + * value of 'v4`, and then we'll drop the nullability at the end. + */ +ALTER TABLE IF EXISTS omicron.public.ip_pool +ADD COLUMN IF NOT EXISTS ip_version omicron.public.ip_version; diff --git a/schema/crdb/add-ip-version-to-pools/up03.sql b/schema/crdb/add-ip-version-to-pools/up03.sql new file mode 100644 index 00000000000..02921faaf5c --- /dev/null +++ b/schema/crdb/add-ip-version-to-pools/up03.sql @@ -0,0 +1,3 @@ +SET disallow_full_table_scans = 'off'; +UPDATE omicron.public.ip_pool SET ip_version = 'v4'; +SET disallow_full_table_scans = 'on'; diff --git a/schema/crdb/add-ip-version-to-pools/up04.sql b/schema/crdb/add-ip-version-to-pools/up04.sql new file mode 100644 index 00000000000..5b7218f6c8b --- /dev/null +++ b/schema/crdb/add-ip-version-to-pools/up04.sql @@ -0,0 +1,8 @@ +/* + * All rows were given the only value that's currently valid + * in the previous migration file. Set the column to be non-null + * now that we've filled all the data in. + */ +ALTER TABLE omicron.public.ip_pool +ALTER COLUMN ip_version +SET NOT NULL; diff --git a/schema/crdb/add-ip-version-to-pools/up05.sql b/schema/crdb/add-ip-version-to-pools/up05.sql new file mode 100644 index 00000000000..5acd3123389 --- /dev/null +++ b/schema/crdb/add-ip-version-to-pools/up05.sql @@ -0,0 +1,5 @@ +SET disallow_full_table_scans = 'off'; +UPDATE omicron.public.ip_pool +SET name = 'oxide-service-pool-v4' +WHERE name = 'oxide-service-pool'; +SET disallow_full_table_scans = 'on'; diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 8ec998970f1..4ef07d9dbdf 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -2057,6 +2057,11 @@ CREATE UNIQUE INDEX IF NOT EXISTS lookup_internet_gateway_ip_address_by_igw_id O ) WHERE time_deleted IS NULL; +/* The IP version of an IP address. */ +CREATE TYPE IF NOT EXISTS omicron.public.ip_version AS ENUM ( + 'v4', + 'v6' +); /* * An IP Pool, a collection of zero or more IP ranges for external IPs. @@ -2071,7 +2076,10 @@ CREATE TABLE IF NOT EXISTS omicron.public.ip_pool ( time_deleted TIMESTAMPTZ, /* The collection's child-resource generation number */ - rcgen INT8 NOT NULL + rcgen INT8 NOT NULL, + + /* The IP version of the ranges contained in this pool. */ + ip_version omicron.public.ip_version NOT NULL ); /* @@ -2130,6 +2138,7 @@ CREATE TABLE IF NOT EXISTS omicron.public.ip_pool_range ( first_address INET NOT NULL, /* The range is inclusive of the last address. */ last_address INET NOT NULL, + /* FK into the `ip_pool` table. */ ip_pool_id UUID NOT NULL, /* Tracks child resources, IP addresses allocated out of this range. */ rcgen INT8 NOT NULL @@ -6553,7 +6562,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '182.0.0', NULL) + (TRUE, NOW(), NOW(), '183.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; From e8ff83a9428fb797a6401cfedf490e024c994b56 Mon Sep 17 00:00:00 2001 From: Benjamin Naecker Date: Mon, 25 Aug 2025 23:44:09 +0000 Subject: [PATCH 2/2] Review feedback - Helper IP Pool constructors - Local SQL settings - Simplify caching IP Pools when ensuring an external address - Add sanity check that DB / Authz pool IDs match --- nexus/db-model/src/ip_pool.rs | 12 ++++ .../deployment/external_networking.rs | 63 ++++++------------- nexus/db-queries/src/db/datastore/ip_pool.rs | 14 ++++- nexus/db-queries/src/db/datastore/rack.rs | 8 +-- schema/crdb/add-ip-version-to-pools/up03.sql | 3 +- schema/crdb/add-ip-version-to-pools/up05.sql | 3 +- 6 files changed, 50 insertions(+), 53 deletions(-) diff --git a/nexus/db-model/src/ip_pool.rs b/nexus/db-model/src/ip_pool.rs index e05498b7f3a..68de06ad24d 100644 --- a/nexus/db-model/src/ip_pool.rs +++ b/nexus/db-model/src/ip_pool.rs @@ -105,6 +105,18 @@ impl IpPool { rcgen: 0, } } + + pub fn new_v4( + pool_identity: &external::IdentityMetadataCreateParams, + ) -> Self { + Self::new(pool_identity, IpVersion::V4) + } + + pub fn new_v6( + pool_identity: &external::IdentityMetadataCreateParams, + ) -> Self { + Self::new(pool_identity, IpVersion::V6) + } } impl From for views::IpPool { 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 85c9656f174..e8cb951f85b 100644 --- a/nexus/db-queries/src/db/datastore/deployment/external_networking.rs +++ b/nexus/db-queries/src/db/datastore/deployment/external_networking.rs @@ -30,47 +30,6 @@ use slog::info; use slog::warn; use slog_error_chain::InlineErrorChain; -// Helper type to lookup service IP Pools only when needed, at most once. -// -// In `ensure_zone_external_networking_deallocated_on_connection`, we lookup -// pools only when we're sure we need them, based on the versions of the IP -// addresses we need to provide for each zone. -struct ServiceIpPools<'a> { - maybe_ipv4_pool: Option, - maybe_ipv6_pool: Option, - datastore: &'a DataStore, - opctx: &'a OpContext, -} - -impl<'a> ServiceIpPools<'a> { - fn new(datastore: &'a DataStore, opctx: &'a OpContext) -> Self { - Self { maybe_ipv4_pool: None, maybe_ipv6_pool: None, datastore, opctx } - } - - /// Get the service IP Pool for the given address. - /// - /// This may need to call out to the database to fetch the pool. - async fn pool_for( - &mut self, - external_ip: &OmicronZoneExternalIp, - ) -> Result<&IpPool, Error> { - let version = external_ip.ip_version(); - let pool = match version { - IpVersion::V4 => &mut self.maybe_ipv4_pool, - IpVersion::V6 => &mut self.maybe_ipv6_pool, - }; - if let Some(pool) = pool { - return Ok(&*pool); - } - let new_pool = self - .datastore - .ip_pools_service_lookup(self.opctx, version.into()) - .await? - .1; - Ok(pool.insert(new_pool)) - } -} - impl DataStore { pub(super) async fn ensure_zone_external_networking_allocated_on_connection( &self, @@ -81,7 +40,8 @@ impl DataStore { // Looking up the service pool IDs requires an opctx; we'll do this at // most once inside the loop below, when we first encounter an address // of the same IP version. - let mut ip_pools = ServiceIpPools::new(self, opctx); + let mut v4_pool = None; + let mut v6_pool = None; for z in zones_to_allocate { let Some((external_ip, nic)) = z.zone_type.external_networking() @@ -98,7 +58,24 @@ impl DataStore { )); // Get existing pool or look it up and cache it. - let pool = ip_pools.pool_for(&external_ip).await?; + let version = external_ip.ip_version(); + let pool_ref = match version { + IpVersion::V4 => &mut v4_pool, + IpVersion::V6 => &mut v6_pool, + }; + let pool = match pool_ref { + Some(p) => p, + None => { + let new = self + .ip_pools_service_lookup(opctx, version.into()) + .await? + .1; + *pool_ref = Some(new); + pool_ref.as_ref().unwrap() + } + }; + + // Actually ensure the IP address. let kind = z.zone_type.kind(); self.ensure_external_service_ip( conn, diff --git a/nexus/db-queries/src/db/datastore/ip_pool.rs b/nexus/db-queries/src/db/datastore/ip_pool.rs index 48b9f28d250..436c7f1227e 100644 --- a/nexus/db-queries/src/db/datastore/ip_pool.rs +++ b/nexus/db-queries/src/db/datastore/ip_pool.rs @@ -1087,16 +1087,26 @@ impl DataStore { ) -> CreateResult { use nexus_db_schema::schema::ip_pool_range::dsl; opctx.authorize(authz::Action::CreateChild, authz_pool).await?; - let pool_id = authz_pool.id(); + + // Sanity check that the provided DB and authz pools match. + if pool.id() != authz_pool.id() { + return Err(Error::internal_error(&format!( + "DB and authz IP Pool object IDs must match, but \ + DB ID is '{}' and authz ID is '{}'", + pool.id(), + authz_pool.id(), + ))); + } // First ensure the IP range matches the IP version of the pool. + let pool_id = authz_pool.id(); if pool.ip_version != range.version().into() { return Err(Error::invalid_request(format!( "Cannot add IP{} address range to \ IP{} pool with ID \"{}\"", range.version(), pool.ip_version, - pool.id(), + pool_id, ))); } diff --git a/nexus/db-queries/src/db/datastore/rack.rs b/nexus/db-queries/src/db/datastore/rack.rs index 3f4ccf52612..70cf81c1bb6 100644 --- a/nexus/db-queries/src/db/datastore/rack.rs +++ b/nexus/db-queries/src/db/datastore/rack.rs @@ -990,10 +990,10 @@ impl DataStore { // make default for the internal silo. only need to do this if // the create went through, i.e., if it wasn't already there // - // TODO-completeness: We're linking the IPv4 pool only here, but we - // need a way for the operator to control this, either at RSS or - // through the API. An alternative is to not set a default at all, - // even though both are linked. + // TODO-completeness: We're linking both IP pools here, but only the + // IPv4 pool is set as a default. We need a way for the operator to + // control this, either at RSS or through the API. An alternative is + // to not set a default at all, even though both are linked. // // See https://github.com/oxidecomputer/omicron/issues/8884 if internal_created { diff --git a/schema/crdb/add-ip-version-to-pools/up03.sql b/schema/crdb/add-ip-version-to-pools/up03.sql index 02921faaf5c..37311049a3e 100644 --- a/schema/crdb/add-ip-version-to-pools/up03.sql +++ b/schema/crdb/add-ip-version-to-pools/up03.sql @@ -1,3 +1,2 @@ -SET disallow_full_table_scans = 'off'; +SET LOCAL disallow_full_table_scans = 'off'; UPDATE omicron.public.ip_pool SET ip_version = 'v4'; -SET disallow_full_table_scans = 'on'; diff --git a/schema/crdb/add-ip-version-to-pools/up05.sql b/schema/crdb/add-ip-version-to-pools/up05.sql index 5acd3123389..7bb3f06949b 100644 --- a/schema/crdb/add-ip-version-to-pools/up05.sql +++ b/schema/crdb/add-ip-version-to-pools/up05.sql @@ -1,5 +1,4 @@ -SET disallow_full_table_scans = 'off'; +SET LOCAL disallow_full_table_scans = 'off'; UPDATE omicron.public.ip_pool SET name = 'oxide-service-pool-v4' WHERE name = 'oxide-service-pool'; -SET disallow_full_table_scans = 'on';