diff --git a/Cargo.lock b/Cargo.lock index 433222067ca..ae596ebe1ad 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1973,6 +1973,7 @@ dependencies = [ "macaddr", "parse-display", "progenitor", + "rand", "reqwest", "ring", "schemars", diff --git a/common/Cargo.toml b/common/Cargo.toml index 7b1e639d0f9..c52ab7c9b9e 100644 --- a/common/Cargo.toml +++ b/common/Cargo.toml @@ -15,6 +15,7 @@ http = "0.2.5" hyper = "0.14" ipnetwork = "0.18" macaddr = { version = "1.0.1", features = [ "serde_std" ] } +rand = "0.8.4" reqwest = { version = "0.11", default-features = false, features = ["rustls-tls"] } ring = "0.16" schemars = { version = "0.8", features = [ "chrono", "uuid" ] } diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index 5f4a7d12af3..7ac33ca7c3a 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -1043,6 +1043,14 @@ impl From for SagaState { #[derive(Clone, Copy, Debug, Deserialize, PartialEq, Serialize)] pub struct Ipv4Net(pub ipnetwork::Ipv4Network); +impl Ipv4Net { + /// Return `true` if this IPv4 subnetwork is from an RFC 1918 private + /// address space. + pub fn is_private(&self) -> bool { + self.0.network().is_private() + } +} + impl std::ops::Deref for Ipv4Net { type Target = ipnetwork::Ipv4Network; fn deref(&self) -> &Self::Target { @@ -1099,6 +1107,38 @@ impl JsonSchema for Ipv4Net { #[derive(Clone, Copy, Debug, Deserialize, PartialEq, Serialize)] pub struct Ipv6Net(pub ipnetwork::Ipv6Network); +impl Ipv6Net { + /// The length for all VPC IPv6 prefixes + pub const VPC_IPV6_PREFIX_LENGTH: u8 = 48; + + /// The prefix length for all VPC Sunets + pub const VPC_SUBNET_IPV6_PREFIX_LENGTH: u8 = 64; + + /// Return `true` if this subnetwork is in the IPv6 Unique Local Address + /// range defined in RFC 4193, e.g., `fd00:/8` + pub fn is_unique_local(&self) -> bool { + // TODO: Delegate to `Ipv6Addr::is_unique_local()` when stabilized. + self.0.network().octets()[0] == 0xfd + } + + /// Return `true` if this subnetwork is a valid VPC prefix. + /// + /// This checks that the subnet is a unique local address, and has the VPC + /// prefix length required. + pub fn is_vpc_prefix(&self) -> bool { + self.is_unique_local() + && self.0.prefix() == Self::VPC_IPV6_PREFIX_LENGTH + } + + /// Return `true` if this subnetwork is a valid VPC Subnet, given the VPC's + /// prefix. + pub fn is_vpc_subnet(&self, vpc_prefix: &Ipv6Net) -> bool { + self.is_unique_local() + && self.is_subnet_of(vpc_prefix.0) + && self.prefix() == Self::VPC_SUBNET_IPV6_PREFIX_LENGTH + } +} + impl std::ops::Deref for Ipv6Net { type Target = ipnetwork::Ipv6Network; fn deref(&self) -> &Self::Target { @@ -2308,4 +2348,26 @@ mod test { parse_display::ParseError::new() ); } + + #[test] + fn test_ipv6_net_operations() { + use super::Ipv6Net; + assert!(Ipv6Net("fd00::/8".parse().unwrap()).is_unique_local()); + assert!(!Ipv6Net("fe00::/8".parse().unwrap()).is_unique_local()); + + assert!(Ipv6Net("fd00::/48".parse().unwrap()).is_vpc_prefix()); + assert!(!Ipv6Net("fe00::/48".parse().unwrap()).is_vpc_prefix()); + assert!(!Ipv6Net("fd00::/40".parse().unwrap()).is_vpc_prefix()); + + let vpc_prefix = Ipv6Net("fd00::/48".parse().unwrap()); + assert!( + Ipv6Net("fd00::/64".parse().unwrap()).is_vpc_subnet(&vpc_prefix) + ); + assert!( + !Ipv6Net("fd10::/64".parse().unwrap()).is_vpc_subnet(&vpc_prefix) + ); + assert!( + !Ipv6Net("fd00::/63".parse().unwrap()).is_vpc_subnet(&vpc_prefix) + ); + } } diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index bdc7f21fae4..f2b4dc403dd 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -411,8 +411,8 @@ CREATE TABLE omicron.public.vpc_subnet ( /* Indicates that the object has been deleted */ time_deleted TIMESTAMPTZ, vpc_id UUID NOT NULL, - ipv4_block INET, - ipv6_block INET + ipv4_block INET NOT NULL, + ipv6_block INET NOT NULL ); /* Subnet and network interface names are unique per VPC, not project */ diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index cc14293d587..5b6afba8a88 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -78,6 +78,8 @@ use crate::db::{ pagination::paginated, pagination::paginated_multicolumn, subnet_allocation::AllocateIpQuery, + subnet_allocation::FilterConflictingVpcSubnetRangesQuery, + subnet_allocation::SubnetError, update_and_check::{UpdateAndCheck, UpdateStatus}, }; @@ -1571,6 +1573,7 @@ impl DataStore { match interface.ip { // Attempt an insert with a requested IP address Some(ip) => { + interface.subnet.contains(ip)?; let row = NetworkInterface { identity: interface.identity, instance_id: interface.instance_id, @@ -1596,11 +1599,10 @@ impl DataStore { } // Insert and allocate an IP address None => { - let block = interface.subnet.ipv4_block.ok_or_else(|| { - Error::internal_error("assuming subnets all have v4 block") - })?; let allocation_query = AllocateIpQuery { - block: ipnetwork::IpNetwork::V4(block.0 .0), + block: ipnetwork::IpNetwork::V4( + interface.subnet.ipv4_block.0 .0, + ), interface, now: Utc::now(), }; @@ -2185,30 +2187,35 @@ impl DataStore { }) } + /// Insert a VPC Subnet, checking for unique IP address ranges. pub async fn vpc_create_subnet( &self, subnet: VpcSubnet, - ) -> CreateResult { + ) -> Result { use db::schema::vpc_subnet::dsl; - let name = subnet.name().clone(); - let subnet = diesel::insert_into(dsl::vpc_subnet) - .values(subnet) - .on_conflict(dsl::id) - .do_nothing() + let values = FilterConflictingVpcSubnetRangesQuery(subnet); + diesel::insert_into(dsl::vpc_subnet) + .values(values) .returning(VpcSubnet::as_returning()) .get_result_async(self.pool()) .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::Conflict( - ResourceType::VpcSubnet, - name.as_str(), - ), - ) - })?; - Ok(subnet) + .map_err(|err| { + if let PoolError::Connection(ConnectionError::Query( + diesel::result::Error::NotFound, + )) = err + { + SubnetError::OverlappingIpRange + } else { + SubnetError::External(public_error_from_diesel_pool( + err, + ErrorHandler::Conflict( + ResourceType::VpcSubnet, + name.to_string().as_str(), + ), + )) + } + }) } pub async fn vpc_delete_subnet(&self, subnet_id: &Uuid) -> DeleteResult { @@ -3251,6 +3258,7 @@ mod test { // scan are, in fact, runnable without a FULL SCAN. #[tokio::test] async fn test_queries_do_not_require_full_table_scan() { + use omicron_common::api::external; let logctx = dev::test_setup_log("test_queries_do_not_require_full_table_scan"); let mut db = test_setup_database(&logctx.log).await; @@ -3278,6 +3286,29 @@ mod test { explanation ); + let subnet = db::model::VpcSubnet::new( + Uuid::nil(), + Uuid::nil(), + external::IdentityMetadataCreateParams { + name: external::Name::try_from(String::from("name")).unwrap(), + description: String::from("description"), + }, + external::Ipv4Net("172.30.0.0/22".parse().unwrap()), + external::Ipv6Net("fd00::/64".parse().unwrap()), + ); + let values = FilterConflictingVpcSubnetRangesQuery(subnet); + let query = + diesel::insert_into(db::schema::vpc_subnet::dsl::vpc_subnet) + .values(values) + .returning(VpcSubnet::as_returning()); + println!("{}", diesel::debug_query(&query)); + let explanation = query.explain_async(datastore.pool()).await.unwrap(); + assert!( + !explanation.contains("FULL SCAN"), + "Found an unexpected FULL SCAN: {}", + explanation, + ); + let _ = db.cleanup().await; } } diff --git a/nexus/src/db/explain.rs b/nexus/src/db/explain.rs index 9b03904e592..3dd5262a83a 100644 --- a/nexus/src/db/explain.rs +++ b/nexus/src/db/explain.rs @@ -104,9 +104,8 @@ where Q: QueryFragment, { fn walk_ast(&self, mut out: AstPass) -> QueryResult<()> { - out.push_sql("EXPLAIN ("); + out.push_sql("EXPLAIN "); self.query.walk_ast(out.reborrow())?; - out.push_sql(")"); Ok(()) } } diff --git a/nexus/src/db/mod.rs b/nexus/src/db/mod.rs index 84b80802638..3ccf2846d4a 100644 --- a/nexus/src/db/mod.rs +++ b/nexus/src/db/mod.rs @@ -21,7 +21,7 @@ mod pool; mod saga_recovery; mod saga_types; mod sec_store; -mod subnet_allocation; +pub(crate) mod subnet_allocation; mod update_and_check; #[cfg(test)] diff --git a/nexus/src/db/model.rs b/nexus/src/db/model.rs index f3a8511bbca..c0e8fbbae60 100644 --- a/nexus/src/db/model.rs +++ b/nexus/src/db/model.rs @@ -31,6 +31,7 @@ use ref_cast::RefCast; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use std::convert::TryFrom; +use std::net::IpAddr; use std::net::SocketAddr; use uuid::Uuid; @@ -352,6 +353,58 @@ pub struct Ipv6Net(pub external::Ipv6Net); NewtypeFrom! { () pub struct Ipv6Net(external::Ipv6Net); } NewtypeDeref! { () pub struct Ipv6Net(external::Ipv6Net); } +impl Ipv6Net { + /// Generate a random subnetwork from this one, of the given prefix length. + /// + /// `None` is returned if: + /// + /// - `prefix` is less than this address's prefix + /// - `prefix` is greater than 128 + /// + /// Note that if the prefix is the same as this address's prefix, a copy of + /// `self` is returned. + pub fn random_subnet(&self, prefix: u8) -> Option { + use rand::RngCore; + + const MAX_IPV6_SUBNET_PREFIX: u8 = 128; + if prefix < self.prefix() || prefix > MAX_IPV6_SUBNET_PREFIX { + return None; + } + if prefix == self.prefix() { + return Some(*self); + } + + // Generate a random address + let mut rng = if cfg!(test) { + StdRng::seed_from_u64(0) + } else { + StdRng::from_entropy() + }; + let random = + u128::from(rng.next_u64()) << 64 | u128::from(rng.next_u64()); + + // Generate a mask for the new address. + // + // We're operating on the big-endian byte representation of the address. + // So shift down by the prefix, and then invert, so that we have 1's + // on the leading bits up to the prefix. + let full_mask = !(u128::MAX >> prefix); + + // Get the existing network address and mask. + let network = u128::from_be_bytes(self.network().octets()); + let network_mask = u128::from_be_bytes(self.mask().octets()); + + // Take random bits _only_ where the new mask is set. + let random_mask = full_mask ^ network_mask; + + let out = (network & network_mask) | (random & random_mask); + let addr = std::net::Ipv6Addr::from(out.to_be_bytes()); + let net = ipnetwork::Ipv6Network::new(addr, prefix) + .expect("Failed to create random subnet"); + Some(Self(external::Ipv6Net(net))) + } +} + impl ToSql for Ipv6Net where DB: Backend, @@ -1231,11 +1284,9 @@ impl Vpc { ) -> Result { let identity = VpcIdentity::new(vpc_id, params.identity); let ipv6_prefix = match params.ipv6_prefix { - None => defaults::random_unique_local_ipv6(), + None => defaults::random_vpc_ipv6_prefix(), Some(prefix) => { - // TODO: Delegate to `Ipv6Addr::is_unique_local()` when stabilized. - if prefix.0.prefix() == 48 && prefix.0.ip().octets()[0] == 0xfd - { + if prefix.is_vpc_prefix() { Ok(prefix) } else { Err(external::Error::invalid_request( @@ -1291,22 +1342,59 @@ pub struct VpcSubnet { identity: VpcSubnetIdentity, pub vpc_id: Uuid, - pub ipv4_block: Option, - pub ipv6_block: Option, + pub ipv4_block: Ipv4Net, + pub ipv6_block: Ipv6Net, } impl VpcSubnet { + /// Create a new VPC Subnet. + /// + /// NOTE: This assumes that the IP address ranges provided in `params` are + /// valid for the VPC, and does not do any further validation. pub fn new( subnet_id: Uuid, vpc_id: Uuid, - params: params::VpcSubnetCreate, + identity: external::IdentityMetadataCreateParams, + ipv4_block: external::Ipv4Net, + ipv6_block: external::Ipv6Net, ) -> Self { - let identity = VpcSubnetIdentity::new(subnet_id, params.identity); + let identity = VpcSubnetIdentity::new(subnet_id, identity); Self { identity, vpc_id, - ipv4_block: params.ipv4_block.map(Ipv4Net), - ipv6_block: params.ipv6_block.map(Ipv6Net), + ipv4_block: Ipv4Net(ipv4_block), + ipv6_block: Ipv6Net(ipv6_block), + } + } + + /// Verify that the provided IP address is contained in the VPC Subnet. + /// + /// This checks: + /// + /// - The subnet has an allocated block of the same version as the address + /// - The allocated block contains the address. + pub fn contains(&self, addr: IpAddr) -> Result<(), external::Error> { + match addr { + IpAddr::V4(addr) => { + if self.ipv4_block.contains(addr) { + Ok(()) + } else { + Err(external::Error::invalid_request(&format!( + "Address '{}' not in IPv4 subnet '{}'", + addr, self.ipv4_block.0, + ))) + } + } + IpAddr::V6(addr) => { + if self.ipv6_block.contains(addr) { + Ok(()) + } else { + Err(external::Error::invalid_request(&format!( + "Address '{}' not in IPv6 subnet '{}'", + addr, self.ipv6_block.0, + ))) + } + } } } } @@ -1998,3 +2086,84 @@ impl RoleAssignmentBuiltin { } } } + +#[cfg(test)] +mod tests { + use super::Uuid; + use super::VpcSubnet; + use ipnetwork::Ipv4Network; + use ipnetwork::Ipv6Network; + use omicron_common::api::external::IdentityMetadataCreateParams; + use omicron_common::api::external::Ipv4Net; + use omicron_common::api::external::Ipv6Net; + use std::net::IpAddr; + use std::net::Ipv4Addr; + use std::net::Ipv6Addr; + + #[test] + fn test_vpc_subnet_contains() { + let ipv4_block = + Ipv4Net("192.168.0.0/16".parse::().unwrap()); + let ipv6_block = Ipv6Net("fd00::/48".parse::().unwrap()); + let identity = IdentityMetadataCreateParams { + name: "net-test-vpc".parse().unwrap(), + description: "A test VPC".parse().unwrap(), + }; + let vpc = VpcSubnet::new( + Uuid::new_v4(), + Uuid::new_v4(), + identity, + ipv4_block, + ipv6_block, + ); + assert!(vpc + .contains(IpAddr::from(Ipv4Addr::new(192, 168, 1, 1))) + .is_ok()); + assert!(vpc + .contains(IpAddr::from(Ipv4Addr::new(192, 160, 1, 1))) + .is_err()); + assert!(vpc + .contains(IpAddr::from(Ipv6Addr::new( + 0xfd00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 + ))) + .is_ok()); + assert!(vpc + .contains(IpAddr::from(Ipv6Addr::new( + 0xfc00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 + ))) + .is_err()); + } + + #[test] + fn test_ipv6_net_random_subnet() { + let base = super::Ipv6Net(Ipv6Net( + "fd00::/48".parse::().unwrap(), + )); + assert!( + base.random_subnet(8).is_none(), + "random_subnet() should fail when prefix is less than the base prefix" + ); + assert!( + base.random_subnet(130).is_none(), + "random_subnet() should fail when prefix is greater than 128" + ); + let subnet = base.random_subnet(64).unwrap(); + assert_eq!( + subnet.prefix(), + 64, + "random_subnet() returned an incorrect prefix" + ); + let octets = subnet.network().octets(); + const EXPECTED_RANDOM_BYTES: [u8; 8] = [253, 0, 0, 0, 0, 0, 111, 127]; + assert_eq!(octets[..8], EXPECTED_RANDOM_BYTES); + assert!( + octets[8..].iter().all(|x| *x == 0), + "Host address portion should be 0" + ); + assert!( + base.is_supernet_of(subnet.0 .0), + "random_subnet should generate an actual subnet" + ); + assert_eq!(base.random_subnet(base.prefix()), Some(base)); + } +} diff --git a/nexus/src/db/schema.rs b/nexus/src/db/schema.rs index dfb24909550..20eee82b550 100644 --- a/nexus/src/db/schema.rs +++ b/nexus/src/db/schema.rs @@ -238,8 +238,8 @@ table! { time_modified -> Timestamptz, time_deleted -> Nullable, vpc_id -> Uuid, - ipv4_block -> Nullable, - ipv6_block -> Nullable, + ipv4_block -> Inet, + ipv6_block -> Inet, } } diff --git a/nexus/src/db/subnet_allocation.rs b/nexus/src/db/subnet_allocation.rs index 7ebfbc5d4e8..ece69aa0e4d 100644 --- a/nexus/src/db/subnet_allocation.rs +++ b/nexus/src/db/subnet_allocation.rs @@ -2,7 +2,7 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at https://mozilla.org/MPL/2.0/. -//! Diesel queries used for subnet allocation +//! Diesel queries used for subnet and IP allocation use crate::db; use crate::db::identity::Resource; @@ -12,6 +12,7 @@ use diesel::pg::Pg; use diesel::prelude::*; use diesel::query_builder::*; use diesel::sql_types; +use omicron_common::api::external; use std::convert::TryFrom; use uuid::Uuid; @@ -31,7 +32,8 @@ use uuid::Uuid; /// WHERE ip IS NULL LIMIT 1; /// /// Note that generate_series receives a start value of 5 in accordance with -/// RFD 21's reservation of addresses 0 through 4 in a subnet. +/// RFD 21's reservation of addresses 0 through 4 in a subnet. See +/// , for details. // TODO-performance: This query scales linearly with the number of IPs // allocated, which is highly undesirable. It will also return the same // candidate address to two parallel executors, which will cause additional @@ -64,6 +66,11 @@ impl QueryFragment for AllocateIpQuery { fn walk_ast(&self, mut out: AstPass) -> diesel::QueryResult<()> { use db::schema::network_interface::dsl; + // Generate last address in the range. + // + // NOTE: First subtraction is to convert from the subnet size to an + // offset, since `generate_series` is inclusive of the last value. + // Example: 256 -> 255. let last_address_offset = match self.block { ipnetwork::IpNetwork::V4(network) => network.size() as i64 - 1, ipnetwork::IpNetwork::V6(network) => { @@ -142,10 +149,9 @@ impl QueryFragment for AllocateIpQuery { out.push_sql(" AS "); out.push_identifier(dsl::ip::NAME)?; - // Start the offsets from 1 to exclude the network base address. + // Skip the initial reserved addresses and the broadcast address. out.push_sql(" FROM generate_series(5, "); out.push_bind_param::( - // Subtract 1 to exclude the broadcast address &(last_address_offset - 1), )?; out.push_sql(") AS "); @@ -219,9 +225,276 @@ impl QueryFragment for AllocateIpQueryValues { } } +/// Errors related to allocating VPC Subnets. +#[derive(Debug)] +pub enum SubnetError { + OverlappingIpRange, + External(external::Error), +} + +/// Generate a CTE that can be used to insert a VPC Subnet, only if the IP +/// address ranges of that subnet don't overlap with existing Subnets in the +/// same VPC. +/// +/// In particular, this generates a CTE like so: +/// +/// ```sql +/// WITH candidate( +/// id, +/// name, +/// description, +/// time_created, +/// time_modified, +/// time_deleted, +/// vpc_id, +/// ipv4_block, +/// ipv6_block +/// ) AS (VALUES ( +/// , +/// , +/// , +/// , +/// , +/// NULL::TIMESTAMPTZ, +/// , +/// , +/// +/// )) +/// SELECT * +/// FROM candidate +/// WHERE NOT EXISTS ( +/// SELECT ipv4_block, ipv6_block +/// FROM vpc_subnet +/// WHERE +/// vpc_id = AND +/// time_deleted IS NULL AND +/// ( +/// inet_contains_or_equals(ipv4_block, candidate.ipv4_block) OR +/// inet_contains_or_equals(ipv6_block, candidate.ipv6_block) +/// ) +/// ) +/// ``` +pub struct FilterConflictingVpcSubnetRangesQuery(pub db::model::VpcSubnet); + +impl QueryId for FilterConflictingVpcSubnetRangesQuery { + type QueryId = (); + const HAS_STATIC_QUERY_ID: bool = false; +} + +impl QueryFragment for FilterConflictingVpcSubnetRangesQuery { + fn walk_ast(&self, mut out: AstPass) -> diesel::QueryResult<()> { + use db::schema::vpc_subnet::dsl; + + // "SELECT * FROM (WITH candidate(" + out.push_sql("SELECT * FROM (WITH candidate("); + + // "id, " + out.push_identifier(dsl::id::NAME)?; + out.push_sql(", "); + + // "name, " + out.push_identifier(dsl::name::NAME)?; + out.push_sql(", "); + + // "description, " + out.push_identifier(dsl::description::NAME)?; + out.push_sql(", "); + + // "time_created, " + out.push_identifier(dsl::time_created::NAME)?; + out.push_sql(", "); + + // "time_modified, " + out.push_identifier(dsl::time_modified::NAME)?; + out.push_sql(", "); + + // "time_deleted, " + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(", "); + + // "vpc_id, " + out.push_identifier(dsl::vpc_id::NAME)?; + out.push_sql(", "); + + // "ipv4_block, " + out.push_identifier(dsl::ipv4_block::NAME)?; + out.push_sql(", "); + + // "ipv6_block) AS (VALUES (" + out.push_identifier(dsl::ipv6_block::NAME)?; + out.push_sql(") AS (VALUES ("); + + // ", " + out.push_bind_param::(&self.0.id())?; + out.push_sql(", "); + + // ", " + out.push_bind_param::( + &self.0.name().to_string(), + )?; + out.push_sql(", "); + + // ", " + out.push_bind_param::(&self.0.description())?; + out.push_sql(", "); + + // ", " + out.push_bind_param::>( + &self.0.time_created(), + )?; + out.push_sql(", "); + + // ", " + out.push_bind_param::>( + &self.0.time_modified(), + )?; + out.push_sql(", "); + + // "NULL::TIMESTAMPTZ, " + out.push_sql("NULL::TIMESTAMPTZ, "); + + // ", " + out.push_bind_param::(&self.0.vpc_id)?; + out.push_sql(", "); + + // , " + out.push_bind_param::( + &ipnetwork::IpNetwork::from(self.0.ipv4_block.0 .0), + )?; + out.push_sql(", "); + + // ")" + out.push_bind_param::( + &ipnetwork::IpNetwork::from(self.0.ipv6_block.0 .0), + )?; + out.push_sql("))"); + + /* + * Possibly filter the candidate row. + * + * This selects everything in the `candidate` CTE, where there is no + * "overlapping" row in the `vpc_subnet` table. Specifically, we search + * that table for rows with: + * + * - The same `vpc_id` + * - Not soft-deleted + * - The IPv4 range overlaps _or_ the IPv6 range overlaps + * + * Those are removed from `candidate`. + */ + + // " SELECT * FROM candidate WHERE NOT EXISTS (" + out.push_sql(" SELECT * FROM candidate WHERE NOT EXISTS ("); + + // " SELECT " + out.push_sql("SELECT "); + + // "ipv4_block, " + out.push_identifier(dsl::ipv4_block::NAME)?; + out.push_sql(", "); + + // "ipv6_block " + out.push_identifier(dsl::ipv6_block::NAME)?; + + // "FROM vpc_subnet" + out.push_sql(" FROM "); + dsl::vpc_subnet.from_clause().walk_ast(out.reborrow())?; + + // " WHERE " + out.push_sql(" WHERE "); + + // "vpc_id = " + out.push_identifier(dsl::vpc_id::NAME)?; + out.push_sql(" = "); + out.push_bind_param::(&self.0.vpc_id)?; + + // " AND time_deleted IS NULL AND ((" + out.push_sql(" AND "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(" IS NULL AND ("); + + // "inet_contains_or_equals(ipv4_block, + out.push_sql("inet_contains_or_equals("); + out.push_identifier(dsl::ipv4_block::NAME)?; + out.push_sql(", "); + out.push_bind_param::( + &ipnetwork::IpNetwork::from(self.0.ipv4_block.0 .0), + )?; + + // ") OR inet_contains_or_equals(ipv6_block, ))" + out.push_sql(") OR inet_contains_or_equals("); + out.push_identifier(dsl::ipv6_block::NAME)?; + out.push_sql(", "); + out.push_bind_param::( + &ipnetwork::IpNetwork::from(self.0.ipv6_block.0 .0), + )?; + out.push_sql("))))"); + + Ok(()) + } +} + +impl Insertable + for FilterConflictingVpcSubnetRangesQuery +{ + type Values = FilterConflictingVpcSubnetRangesQueryValues; + + fn values(self) -> Self::Values { + FilterConflictingVpcSubnetRangesQueryValues(self) + } +} + +/// Used to allow inserting the result of the +/// `FilterConflictingVpcSubnetRangesQuery`, as in +/// `diesel::insert_into(foo).values(_). Should not be used directly. +pub struct FilterConflictingVpcSubnetRangesQueryValues( + pub FilterConflictingVpcSubnetRangesQuery, +); + +impl QueryId for FilterConflictingVpcSubnetRangesQueryValues { + type QueryId = (); + const HAS_STATIC_QUERY_ID: bool = false; +} + +impl diesel::insertable::CanInsertInSingleQuery + for FilterConflictingVpcSubnetRangesQueryValues +{ + fn rows_to_insert(&self) -> Option { + Some(1) + } +} + +impl QueryFragment for FilterConflictingVpcSubnetRangesQueryValues { + fn walk_ast(&self, mut out: AstPass) -> diesel::QueryResult<()> { + use db::schema::vpc_subnet::dsl; + out.push_sql("("); + out.push_identifier(dsl::id::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::name::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::description::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::time_created::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::time_modified::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::time_deleted::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::vpc_id::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::ipv4_block::NAME)?; + out.push_sql(", "); + out.push_identifier(dsl::ipv6_block::NAME)?; + out.push_sql(") "); + self.0.walk_ast(out) + } +} + #[cfg(test)] mod test { use super::AllocateIpQuery; + use super::FilterConflictingVpcSubnetRangesQuery; + use super::SubnetError; use crate::db::model::{ IncompleteNetworkInterface, NetworkInterface, VpcSubnet, }; @@ -230,10 +503,14 @@ mod test { use chrono::{DateTime, NaiveDateTime, Utc}; use diesel::pg::Pg; use diesel::prelude::*; + use nexus_test_utils::db::test_setup_database; use omicron_common::api::external::{ - IdentityMetadataCreateParams, Ipv4Net, MacAddr, + IdentityMetadataCreateParams, Ipv4Net, Ipv6Net, MacAddr, Name, }; + use omicron_test_utils::dev; use std::convert::TryInto; + use std::sync::Arc; + use uuid::Uuid; #[test] fn test_verify_query() { @@ -249,18 +526,18 @@ mod test { let subnet_id = uuid::Uuid::parse_str("223cb7f7-0d3a-4a4e-a5e1-ad38ecb785d3") .unwrap(); - let block: ipnetwork::Ipv4Network = "192.168.1.0/24".parse().unwrap(); + let ipv4_block: ipnetwork::Ipv4Network = + "192.168.1.0/24".parse().unwrap(); + let ipv6_block: ipnetwork::Ipv6Network = "fd00::/48".parse().unwrap(); let subnet = VpcSubnet::new( subnet_id, vpc_id, - params::VpcSubnetCreate { - identity: IdentityMetadataCreateParams { - name: "test-subnet".to_string().try_into().unwrap(), - description: "subnet description".to_string(), - }, - ipv4_block: Some(Ipv4Net(block.clone()).into()), - ipv6_block: None, + IdentityMetadataCreateParams { + name: "test-subnet".to_string().try_into().unwrap(), + description: "subnet description".to_string(), }, + Ipv4Net(ipv4_block.clone()).into(), + Ipv6Net(ipv6_block), ); let mac = MacAddr(macaddr::MacAddr6::from([0xA8, 0x40, 0x25, 0x0, 0x0, 0x1])) @@ -281,7 +558,7 @@ mod test { ); let select = AllocateIpQuery { interface, - block: block.into(), + block: ipv4_block.into(), now: DateTime::::from_utc( NaiveDateTime::from_timestamp(0, 0), Utc, @@ -348,4 +625,181 @@ mod test { V4(Ipv4Network { addr: 192.168.1.0, prefix: 32 })]"; assert_eq!(query, expected_query); } + + #[test] + fn test_filter_conflicting_vpc_subnet_ranges_query_string() { + use crate::db::identity::Resource; + let ipv4_block = Ipv4Net("172.30.0.0/22".parse().unwrap()); + let ipv6_block = Ipv6Net("fd12:3456:7890::/64".parse().unwrap()); + let name = "a-name".to_string().try_into().unwrap(); + let description = "some description".to_string(); + let identity = IdentityMetadataCreateParams { name, description }; + let vpc_id = Uuid::new_v4(); + let subnet_id = Uuid::new_v4(); + let row = + VpcSubnet::new(subnet_id, vpc_id, identity, ipv4_block, ipv6_block); + let query = FilterConflictingVpcSubnetRangesQuery(row.clone()); + let query_str = diesel::debug_query::(&query).to_string(); + let expected_query = format!( + concat!( + "SELECT * FROM (WITH candidate(", + r#""id", "name", "description", "time_created", "time_modified", "#, + r#""time_deleted", "vpc_id", "ipv4_block", "ipv6_block") AS "#, + "(VALUES ($1, $2, $3, $4, $5, NULL::TIMESTAMPTZ, $6, $7, $8)) ", + "SELECT * FROM candidate WHERE NOT EXISTS (", + r#"SELECT "ipv4_block", "ipv6_block" FROM "vpc_subnet" WHERE "#, + r#""vpc_id" = $9 AND "time_deleted" IS NULL AND ("#, + r#"inet_contains_or_equals("ipv4_block", $10) OR inet_contains_or_equals("ipv6_block", $11)))) "#, + r#"-- binds: [{subnet_id}, "{name}", "{description}", {time_created:?}, "#, + r#"{time_modified:?}, {vpc_id}, V4({ipv4_block:?}), V6({ipv6_block:?}), "#, + r#"{vpc_id}, V4({ipv4_block:?}), V6({ipv6_block:?})]"#, + ), + subnet_id = row.id(), + name = row.name(), + description = row.description(), + vpc_id = row.vpc_id, + ipv4_block = row.ipv4_block.0 .0, + ipv6_block = row.ipv6_block.0 .0, + time_created = row.time_created(), + time_modified = row.time_modified(), + ); + assert_eq!(query_str, expected_query); + } + + #[tokio::test] + async fn test_filter_conflicting_vpc_subnet_ranges_query() { + let make_id = + |name: &Name, description: &str| IdentityMetadataCreateParams { + name: name.clone(), + description: description.to_string(), + }; + let ipv4_block = Ipv4Net("172.30.0.0/22".parse().unwrap()); + let other_ipv4_block = Ipv4Net("172.31.0.0/22".parse().unwrap()); + let ipv6_block = Ipv6Net("fd12:3456:7890::/64".parse().unwrap()); + let other_ipv6_block = Ipv6Net("fd00::/64".parse().unwrap()); + let name = "a-name".to_string().try_into().unwrap(); + let other_name = "b-name".to_string().try_into().unwrap(); + let description = "some description".to_string(); + let identity = make_id(&name, &description); + let vpc_id = "d402369d-c9ec-c5ad-9138-9fbee732d53e".parse().unwrap(); + let other_vpc_id = + "093ad2db-769b-e3c2-bc1c-b46e84ce5532".parse().unwrap(); + let subnet_id = "093ad2db-769b-e3c2-bc1c-b46e84ce5532".parse().unwrap(); + let other_subnet_id = + "695debcc-e197-447d-ffb2-976150a7b7cf".parse().unwrap(); + let row = + VpcSubnet::new(subnet_id, vpc_id, identity, ipv4_block, ipv6_block); + + // Setup the test database + let logctx = + dev::test_setup_log("test_filter_conflicting_vpc_subnet_ranges"); + let log = logctx.log.new(o!()); + let mut db = test_setup_database(&log).await; + let cfg = crate::db::Config { url: db.pg_config().clone() }; + let pool = Arc::new(crate::db::Pool::new(&cfg)); + let db_datastore = + Arc::new(crate::db::DataStore::new(Arc::clone(&pool))); + + // We should be able to insert anything into an empty table. + assert!( + matches!(db_datastore.vpc_create_subnet(row).await, Ok(_)), + "Should be able to insert VPC subnet into empty table" + ); + + // We shouldn't be able to insert a row with the same IP ranges, even if + // the other data does not conflict. + let new_row = VpcSubnet::new( + other_subnet_id, + vpc_id, + make_id(&other_name, &description), + ipv4_block, + ipv6_block, + ); + assert!( + matches!( + db_datastore.vpc_create_subnet(new_row).await, + Err(SubnetError::OverlappingIpRange) + ), + "Should not be able to insert new VPC subnet with the same IP ranges" + ); + + // We should be able to insert data with the same ranges, if we change + // the VPC ID. + let new_row = VpcSubnet::new( + other_subnet_id, + other_vpc_id, + make_id(&name, &description), + ipv4_block, + ipv6_block, + ); + assert!( + matches!(db_datastore.vpc_create_subnet(new_row).await, Ok(_)), + "Should be able to insert a VPC Subnet with the same ranges in a different VPC", + ); + + // We shouldn't be able to insert a subnet if we change only the + // IPv4 or IPv6 block. They must _both_ be non-overlapping. + let new_row = VpcSubnet::new( + other_subnet_id, + vpc_id, + make_id(&other_name, &description), + other_ipv4_block, + ipv6_block, + ); + assert!( + matches!( + db_datastore.vpc_create_subnet(new_row).await, + Err(SubnetError::OverlappingIpRange), + ), + "Should not be able to insert VPC Subnet with overlapping IPv4 range" + ); + let new_row = VpcSubnet::new( + other_subnet_id, + vpc_id, + make_id(&other_name, &description), + ipv4_block, + other_ipv6_block, + ); + assert!( + matches!( + db_datastore.vpc_create_subnet(new_row).await, + Err(SubnetError::OverlappingIpRange), + ), + "Should not be able to insert VPC Subnet with overlapping IPv6 range" + ); + + // We should get an _external error_ if the IP address ranges are OK, + // but the name conflicts. + let new_row = VpcSubnet::new( + other_subnet_id, + vpc_id, + make_id(&name, &description), + other_ipv4_block, + other_ipv6_block, + ); + assert!( + matches!( + db_datastore.vpc_create_subnet(new_row).await, + Err(SubnetError::External(_)) + ), + "Should get an error inserting a VPC Subnet with unique IP ranges, but the same name" + ); + + // We should be able to insert the row if _both ranges_ are different, + // and the name is unique as well. + let new_row = VpcSubnet::new( + Uuid::new_v4(), + vpc_id, + make_id(&other_name, &description), + other_ipv4_block, + other_ipv6_block, + ); + assert!( + matches!(db_datastore.vpc_create_subnet(new_row).await, Ok(_)), + "Should be able to insert new VPC Subnet with non-overlapping IP ranges" + ); + + db.cleanup().await.unwrap(); + logctx.cleanup_successful(); + } } diff --git a/nexus/src/defaults.rs b/nexus/src/defaults.rs index ef45f82820b..0df2a6344aa 100644 --- a/nexus/src/defaults.rs +++ b/nexus/src/defaults.rs @@ -4,12 +4,33 @@ //! Default values for data in the Nexus API, when not provided explicitly in a request. +use ipnetwork::Ipv4Network; use ipnetwork::Ipv6Network; use lazy_static::lazy_static; use omicron_common::api::external; +use omicron_common::api::external::Ipv4Net; use omicron_common::api::external::Ipv6Net; +use std::net::Ipv4Addr; use std::net::Ipv6Addr; +/// Minimum prefix size supported in IPv4 VPC Subnets. +/// +/// NOTE: This is the minimum _prefix_, which sets the maximum subnet size. +pub const MIN_VPC_IPV4_SUBNET_PREFIX: u8 = 8; + +/// Maximum prefix size supported in IPv4 VPC Subnets. +/// +/// NOTE: This is the maximum _prefix_, which sets the minimum subnet size. +pub const MAX_VPC_IPV4_SUBNET_PREFIX: u8 = 26; + +lazy_static! { + /// The default IPv4 subnet range assigned to the default VPC Subnet, when + /// the VPC is created, if one is not provided in the request. See + /// for details. + pub static ref DEFAULT_VPC_SUBNET_IPV4_BLOCK: external::Ipv4Net = + Ipv4Net(Ipv4Network::new(Ipv4Addr::new(172, 30, 0, 0), 22).unwrap()); +} + lazy_static! { pub static ref DEFAULT_FIREWALL_RULES: external::VpcFirewallRuleUpdateParams = serde_json::from_str(r#"{ @@ -52,7 +73,8 @@ lazy_static! { }"#).unwrap(); } -pub fn random_unique_local_ipv6() -> Result { +/// Generate a random VPC IPv6 prefix, in the range `fd00::/48`. +pub fn random_vpc_ipv6_prefix() -> Result { use rand::Rng; let mut bytes = [0u8; 16]; bytes[0] = 0xfd; @@ -61,7 +83,13 @@ pub fn random_unique_local_ipv6() -> Result { "Unable to allocate random IPv6 address range", ) })?; - Ok(Ipv6Net(Ipv6Network::new(Ipv6Addr::from(bytes), 48).unwrap())) + Ok(Ipv6Net( + Ipv6Network::new( + Ipv6Addr::from(bytes), + Ipv6Net::VPC_IPV6_PREFIX_LENGTH, + ) + .unwrap(), + )) } #[cfg(test)] @@ -69,11 +97,10 @@ mod tests { use super::*; #[test] - fn test_random_unique_local_ipv6() { - let network = random_unique_local_ipv6().unwrap().0; - assert_eq!(network.prefix(), 48); + fn test_random_vpc_ipv6_prefix() { + let network = random_vpc_ipv6_prefix().unwrap(); + assert!(network.is_vpc_prefix()); let octets = network.network().octets(); - assert_eq!(octets[0], 0xfd); assert!(octets[6..].iter().all(|x| *x == 0)); } } diff --git a/nexus/src/external_api/params.rs b/nexus/src/external_api/params.rs index dd035034859..40f37ec63e4 100644 --- a/nexus/src/external_api/params.rs +++ b/nexus/src/external_api/params.rs @@ -107,7 +107,14 @@ pub struct InstanceMigrate { pub struct VpcCreate { #[serde(flatten)] pub identity: IdentityMetadataCreateParams, + + /// The IPv6 prefix for this VPC. + /// + /// All IPv6 subnets created from this VPC must be taken from this range, + /// which sould be a Unique Local Address in the range `fd00::/48`. The + /// default VPC Subnet will have the first `/64` range from this prefix. pub ipv6_prefix: Option, + pub dns_name: Name, } @@ -128,7 +135,19 @@ pub struct VpcUpdate { pub struct VpcSubnetCreate { #[serde(flatten)] pub identity: IdentityMetadataCreateParams, - pub ipv4_block: Option, + + /// The IPv4 address range for this subnet. + /// + /// It must be allocated from an RFC 1918 private address range, and must + /// not overlap with any other existing subnet in the VPC. + pub ipv4_block: Ipv4Net, + + /// The IPv6 address range for this subnet. + /// + /// It must be allocated from the RFC 4193 Unique Local Address range, with + /// the prefix equal to the parent VPC's prefix. A random `/64` block will + /// be assigned if one is not provided. It must not overlap with any + /// existing subnet in the VPC. pub ipv6_block: Option, } @@ -139,6 +158,8 @@ pub struct VpcSubnetCreate { pub struct VpcSubnetUpdate { #[serde(flatten)] pub identity: IdentityMetadataUpdateParams, + // TODO-correctness: It seems fraught to allow changing these, since it can + // invalidate arbitrary sub-resources (e.g., network interfaces). pub ipv4_block: Option, pub ipv6_block: Option, } diff --git a/nexus/src/external_api/views.rs b/nexus/src/external_api/views.rs index 97086eba818..3958a825083 100644 --- a/nexus/src/external_api/views.rs +++ b/nexus/src/external_api/views.rs @@ -113,18 +113,11 @@ pub struct VpcSubnet { /** The VPC to which the subnet belongs. */ pub vpc_id: Uuid, - // TODO-design: RFD 21 says that V4 subnets are currently required, and V6 are optional. If a - // V6 address is _not_ specified, one is created with a prefix that depends on the VPC and a - // unique subnet-specific portion of the prefix (40 and 16 bits for each, respectively). - // - // We're leaving out the "view" types here for the external HTTP API for now, so it's not clear - // how to do the validation of user-specified CIDR blocks, or how to create a block if one is - // not given. /** The IPv4 subnet CIDR block. */ - pub ipv4_block: Option, + pub ipv4_block: Ipv4Net, /** The IPv6 subnet CIDR block. */ - pub ipv6_block: Option, + pub ipv6_block: Ipv6Net, } impl Into for model::VpcSubnet { @@ -132,8 +125,8 @@ impl Into for model::VpcSubnet { VpcSubnet { identity: self.identity(), vpc_id: self.vpc_id, - ipv4_block: self.ipv4_block.map(|ip| ip.into()), - ipv6_block: self.ipv6_block.map(|ip| ip.into()), + ipv4_block: self.ipv4_block.0, + ipv6_block: self.ipv6_block.0, } } } diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index c6b04e7b930..12a239953fc 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -14,6 +14,7 @@ use crate::db; use crate::db::identity::{Asset, Resource}; use crate::db::model::DatasetKind; use crate::db::model::Name; +use crate::db::subnet_allocation::SubnetError; use crate::defaults; use crate::external_api::params; use crate::internal_api::params::{OximeterInfo, ZpoolPutRequest}; @@ -27,8 +28,6 @@ use async_trait::async_trait; use futures::future::ready; use futures::StreamExt; use hex; -use ipnetwork::Ipv4Network; -use ipnetwork::Ipv6Network; use omicron_common::api::external; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DataPageParams; @@ -37,8 +36,6 @@ use omicron_common::api::external::DiskState; use omicron_common::api::external::Error; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::InstanceState; -use omicron_common::api::external::Ipv4Net; -use omicron_common::api::external::Ipv6Net; use omicron_common::api::external::ListResult; use omicron_common::api::external::ListResultVec; use omicron_common::api::external::LookupResult; @@ -570,7 +567,7 @@ impl Nexus { name: "default".parse().unwrap(), description: "Default VPC".to_string(), }, - ipv6_prefix: Some(defaults::random_unique_local_ipv6()?), + ipv6_prefix: Some(defaults::random_vpc_ipv6_prefix()?), // TODO-robustness this will need to be None if we decide to // handle the logic around name and dns_name by making // dns_name optional @@ -1636,32 +1633,54 @@ impl Nexus { )?; let vpc = self.db_datastore.project_create_vpc(vpc).await?; + // Allocate the first /64 sub-range from the requested or created + // prefix. + let ipv6_block = external::Ipv6Net( + ipnetwork::Ipv6Network::new(vpc.ipv6_prefix.network(), 64) + .map_err(|_| { + external::Error::internal_error( + "Failed to allocate default IPv6 subnet", + ) + })?, + ); + // TODO: batch this up with everything above let subnet = db::model::VpcSubnet::new( default_subnet_id, vpc_id, - params::VpcSubnetCreate { - identity: IdentityMetadataCreateParams { - name: "default".parse().unwrap(), - description: format!( - "The default subnet for {}", - params.identity.name - ), - }, - ipv4_block: Some(Ipv4Net( - // TODO: This value should be replaced with the correct ipv4 - // range for a default subnet - "10.1.9.32/16".parse::().unwrap(), - )), - ipv6_block: Some(Ipv6Net( - // TODO: This value should be replaced w/ the first `/64` - // ipv6 from the address block - "2001:db8::0/64".parse::().unwrap(), - )), + IdentityMetadataCreateParams { + name: "default".parse().unwrap(), + description: format!( + "The default subnet for {}", + params.identity.name + ), }, + *defaults::DEFAULT_VPC_SUBNET_IPV4_BLOCK, + ipv6_block, ); - self.db_datastore.vpc_create_subnet(subnet).await?; + // Create the subnet record in the database. Overlapping IP ranges should be translated + // into an internal error. That implies that there's already an existing VPC Subnet, but + // we're explicitly creating the _first_ VPC in the project. Something is wrong, and likely + // a bug in our code. + let _ = self.db_datastore.vpc_create_subnet(subnet).await.map_err(|err| { + match err { + SubnetError::OverlappingIpRange => { + warn!( + self.log, + "failed to create default VPC Subnet, found overlapping IP address ranges"; + "vpc_id" => ?vpc_id, + "subnet_id" => ?default_subnet_id, + "ipv4_block" => ?*defaults::DEFAULT_VPC_SUBNET_IPV4_BLOCK, + "ipv6_block" => ?ipv6_block, + ); + external::Error::internal_error( + "Failed to create default VPC Subnet, found overlapping IP address ranges" + ) + }, + SubnetError::External(e) => e, + } + })?; self.create_default_vpc_firewall(&vpc_id).await?; Ok(vpc) } @@ -1815,10 +1834,140 @@ impl Nexus { let vpc = self .project_lookup_vpc(organization_name, project_name, vpc_name) .await?; - let id = Uuid::new_v4(); - let subnet = db::model::VpcSubnet::new(id, vpc.id(), params.clone()); - let subnet = self.db_datastore.vpc_create_subnet(subnet).await?; - Ok(subnet) + + // Validate IPv4 range + if !params.ipv4_block.network().is_private() { + return Err(external::Error::invalid_request( + "VPC Subnet IPv4 address ranges must be from a private range", + )); + } + if params.ipv4_block.prefix() < defaults::MIN_VPC_IPV4_SUBNET_PREFIX + || params.ipv4_block.prefix() > defaults::MAX_VPC_IPV4_SUBNET_PREFIX + { + return Err(external::Error::invalid_request(&format!( + concat!( + "VPC Subnet IPv4 address ranges must have prefix ", + "length between {} and {}, inclusive" + ), + defaults::MIN_VPC_IPV4_SUBNET_PREFIX, + defaults::MAX_VPC_IPV4_SUBNET_PREFIX + ))); + } + + // Allocate an ID and insert the record. + // + // If the client provided an IPv6 range, we try to insert that or fail + // with a conflict error. + // + // If they did _not_, we randomly generate a subnet valid for the VPC's + // prefix, and the insert that. There's a small retry loop if we get + // unlucky and conflict with an existing IPv6 range. In the case we + // cannot find a subnet within a small number of retries, we fail the + // request with a 503. + // + // TODO-robustness: We'd really prefer to allocate deterministically. + // See for + // details. + let subnet_id = Uuid::new_v4(); + match params.ipv6_block { + None => { + const NUM_RETRIES: usize = 2; + let mut retry = 0; + let result = loop { + let ipv6_block = vpc + .ipv6_prefix + .random_subnet( + external::Ipv6Net::VPC_SUBNET_IPV6_PREFIX_LENGTH, + ) + .map(|block| block.0) + .ok_or_else(|| { + external::Error::internal_error( + "Failed to create random IPv6 subnet", + ) + })?; + let subnet = db::model::VpcSubnet::new( + subnet_id, + vpc.id(), + params.identity.clone(), + params.ipv4_block, + ipv6_block, + ); + let result = + self.db_datastore.vpc_create_subnet(subnet).await; + match result { + // Allow NUM_RETRIES retries, after the first attempt. + Err(SubnetError::OverlappingIpRange) + if retry <= NUM_RETRIES => + { + debug!( + self.log, "autogenerated random IPv6 range overlap"; + "subnet_id" => ?subnet_id, "ipv6_block" => %ipv6_block.0 + ); + retry += 1; + continue; + } + other => break other, + } + }; + match result { + Err(SubnetError::OverlappingIpRange) => { + // TODO-monitoring TODO-debugging + // + // We should maintain a counter for this occurrence, and + // export that via `oximeter`, so that we can see these + // failures through the timeseries database. The main + // goal here is for us to notice that this is happening + // before it becomes a major issue for customers. + let vpc_id = vpc.id(); + warn!( + self.log, + "failed to generate unique random IPv6 address range in {} retries", + NUM_RETRIES; + "vpc_id" => ?vpc_id, + "subnet_id" => ?subnet_id, + ); + Err(external::Error::internal_error( + "Unable to allocate unique IPv6 address range for VPC Subnet" + )) + } + Err(SubnetError::External(e)) => Err(e), + Ok(subnet) => Ok(subnet), + } + } + Some(ipv6_block) => { + if !ipv6_block.is_vpc_subnet(&vpc.ipv6_prefix) { + return Err(external::Error::invalid_request(&format!( + concat!( + "VPC Subnet IPv6 address range '{}' is not valid for ", + "VPC with IPv6 prefix '{}'", + ), + ipv6_block, vpc.ipv6_prefix.0 .0, + ))); + } + let subnet = db::model::VpcSubnet::new( + subnet_id, + vpc.id(), + params.identity.clone(), + params.ipv4_block, + ipv6_block, + ); + self.db_datastore.vpc_create_subnet(subnet).await.map_err( + |err| match err { + SubnetError::OverlappingIpRange => { + external::Error::invalid_request(&format!( + concat!( + "IPv4 block '{}' and/or IPv6 block '{}' ", + "overlaps with existing VPC Subnet ", + "IP address ranges" + ), + params.ipv4_block, ipv6_block, + )) + } + SubnetError::External(e) => e, + }, + ) + } + } } // TODO: When a subnet is deleted it should remove its entry from the VPC's system router. diff --git a/nexus/tests/integration_tests/vpc_subnets.rs b/nexus/tests/integration_tests/vpc_subnets.rs index e7f0c13cc27..f2342f69f09 100644 --- a/nexus/tests/integration_tests/vpc_subnets.rs +++ b/nexus/tests/integration_tests/vpc_subnets.rs @@ -4,7 +4,6 @@ use http::method::Method; use http::StatusCode; -use ipnetwork::{Ipv4Network, Ipv6Network}; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_common::api::external::IdentityMetadataUpdateParams; use omicron_common::api::external::Ipv4Net; @@ -72,10 +71,17 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) { assert_eq!(error.message, "not found: vpc-subnet with name \"subnet1\""); /* Create a VPC Subnet. */ - let ipv4_block = - Some(Ipv4Net("10.1.9.32/16".parse::().unwrap())); + let ipv4_block = Ipv4Net("10.0.0.0/24".parse().unwrap()); + let other_ipv4_block = Ipv4Net("172.31.0.0/16".parse().unwrap()); + // Create the first two available IPv6 address ranges. */ + let prefix = vpc.ipv6_prefix.network(); let ipv6_block = - Some(Ipv6Net("2001:db8::0/96".parse::().unwrap())); + Some(Ipv6Net(ipnetwork::Ipv6Network::new(prefix, 64).unwrap())); + let mut segments = prefix.segments(); + segments[3] = 1; + let addr = std::net::Ipv6Addr::from(segments); + let other_ipv6_block = + Some(Ipv6Net(ipnetwork::Ipv6Network::new(addr, 64).unwrap())); let new_subnet = params::VpcSubnetCreate { identity: IdentityMetadataCreateParams { name: subnet_name.parse().unwrap(), @@ -90,7 +96,8 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) { assert_eq!(subnet.identity.description, "it's below the net"); assert_eq!(subnet.vpc_id, vpc.identity.id); assert_eq!(subnet.ipv4_block, ipv4_block); - assert_eq!(subnet.ipv6_block, ipv6_block); + assert_eq!(subnet.ipv6_block, ipv6_block.unwrap()); + assert!(subnet.ipv6_block.is_vpc_subnet(&vpc.ipv6_prefix)); // try to update ipv4_block with IPv6 value, should 400 assert_put_400( @@ -120,7 +127,35 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) { assert_eq!(subnets.len(), 1); subnets_eq(&subnets[0], &subnet); - // creating another subnet in the same VPC with the same name fails + // creating another subnet in the same VPC with the same IP ranges fails + let new_subnet = params::VpcSubnetCreate { + identity: IdentityMetadataCreateParams { + name: "new-name".parse().unwrap(), + description: "it's below the net".to_string(), + }, + ipv4_block, + ipv6_block, + }; + let error = client + .make_request_error_body( + Method::POST, + &subnets_url, + new_subnet.clone(), + StatusCode::BAD_REQUEST, + ) + .await; + assert!(error.message.starts_with("IPv4 block '")); + + // creating another subnet in the same VPC with the same name, but different + // IP address ranges also fails. + let new_subnet = params::VpcSubnetCreate { + identity: IdentityMetadataCreateParams { + name: subnet_name.parse().unwrap(), + description: "it's below the net".to_string(), + }, + ipv4_block: other_ipv4_block, + ipv6_block: other_ipv6_block, + }; let error = client .make_request_error_body( Method::POST, @@ -140,13 +175,14 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) { .await; assert_eq!(error.message, "not found: vpc-subnet with name \"subnet2\""); - // create second subnet + // create second subnet, this time with an autogenerated IPv6 range. + let ipv4_block = Ipv4Net("192.168.0.0/16".parse().unwrap()); let new_subnet = params::VpcSubnetCreate { identity: IdentityMetadataCreateParams { name: subnet2_name.parse().unwrap(), description: "it's also below the net".to_string(), }, - ipv4_block: None, + ipv4_block, ipv6_block: None, }; let subnet2: VpcSubnet = @@ -154,8 +190,8 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) { assert_eq!(subnet2.identity.name, subnet2_name); assert_eq!(subnet2.identity.description, "it's also below the net"); assert_eq!(subnet2.vpc_id, vpc.identity.id); - assert_eq!(subnet2.ipv4_block, None); - assert_eq!(subnet2.ipv6_block, None); + assert_eq!(subnet2.ipv4_block, ipv4_block); + assert!(subnet2.ipv6_block.is_vpc_subnet(&vpc.ipv6_prefix)); // subnets list should now have two in it let subnets = @@ -245,8 +281,11 @@ async fn test_vpc_subnets(cptestctx: &ControlPlaneTestContext) { "it's also below the net" ); assert_eq!(subnet_same_name.vpc_id, vpc2.identity.id); - assert_eq!(subnet_same_name.ipv4_block, None); - assert_eq!(subnet_same_name.ipv6_block, None); + assert_eq!( + subnet_same_name.ipv4_block, + Ipv4Net("192.168.0.0/16".parse().unwrap()) + ); + assert!(subnet_same_name.ipv6_block.is_unique_local()); } fn subnets_eq(sn1: &VpcSubnet, sn2: &VpcSubnet) { diff --git a/nexus/tests/integration_tests/vpcs.rs b/nexus/tests/integration_tests/vpcs.rs index 75e398b204a..87b26e998eb 100644 --- a/nexus/tests/integration_tests/vpcs.rs +++ b/nexus/tests/integration_tests/vpcs.rs @@ -88,9 +88,8 @@ async fn test_vpcs(cptestctx: &ControlPlaneTestContext) { 48, "Expected a 48-bit ULA IPv6 address prefix" ); - assert_eq!( - vpc.ipv6_prefix.ip().octets()[0], - 0xfd, + assert!( + vpc.ipv6_prefix.is_unique_local(), "Expected a ULA IPv6 address prefix" ); diff --git a/openapi/nexus.json b/openapi/nexus.json index 57797b2eab0..4546196155e 100644 --- a/openapi/nexus.json +++ b/openapi/nexus.json @@ -4913,6 +4913,7 @@ }, "ipv6_prefix": { "nullable": true, + "description": "The IPv6 prefix for this VPC.\n\nAll IPv6 subnets created from this VPC must be taken from this range, which sould be a Unique Local Address in the range `fd00::/48`. The default VPC Subnet will have the first `/64` range from this prefix.", "allOf": [ { "$ref": "#/components/schemas/Ipv6Net" @@ -5487,7 +5488,6 @@ "format": "uuid" }, "ipv4_block": { - "nullable": true, "description": "The IPv4 subnet CIDR block.", "allOf": [ { @@ -5496,7 +5496,6 @@ ] }, "ipv6_block": { - "nullable": true, "description": "The IPv6 subnet CIDR block.", "allOf": [ { @@ -5531,6 +5530,8 @@ "required": [ "description", "id", + "ipv4_block", + "ipv6_block", "name", "time_created", "time_modified", @@ -5545,7 +5546,7 @@ "type": "string" }, "ipv4_block": { - "nullable": true, + "description": "The IPv4 address range for this subnet.\n\nIt must be allocated from an RFC 1918 private address range, and must not overlap with any other existing subnet in the VPC.", "allOf": [ { "$ref": "#/components/schemas/Ipv4Net" @@ -5554,6 +5555,7 @@ }, "ipv6_block": { "nullable": true, + "description": "The IPv6 address range for this subnet.\n\nIt must be allocated from the RFC 4193 Unique Local Address range, with the prefix equal to the parent VPC's prefix. A random `/64` block will be assigned if one is not provided. It must not overlap with any existing subnet in the VPC.", "allOf": [ { "$ref": "#/components/schemas/Ipv6Net" @@ -5566,6 +5568,7 @@ }, "required": [ "description", + "ipv4_block", "name" ] },