diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 23c98529fab..99d7a9712af 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -660,7 +660,15 @@ CREATE TABLE omicron.public.network_interface ( vpc_id UUID NOT NULL, /* FK into VPCSubnet table. */ subnet_id UUID NOT NULL, - mac STRING(17) NOT NULL, -- e.g., "ff:ff:ff:ff:ff:ff" + + /* + * The EUI-48 MAC address of the guest interface. + * + * Note that we use the bytes of a 64-bit integer, in big-endian byte order + * to represent the MAC. + */ + mac INT8 NOT NULL, + ip INET NOT NULL, /* * Limited to 8 NICs per instance. This value must be kept in sync with diff --git a/nexus/src/app/instance.rs b/nexus/src/app/instance.rs index 6ea7a022ced..754bfddcf9d 100644 --- a/nexus/src/app/instance.rs +++ b/nexus/src/app/instance.rs @@ -662,14 +662,12 @@ impl super::Nexus { .vpc_subnet_name(&subnet_name) .fetch() .await?; - let mac = db::model::MacAddr::new()?; let interface_id = Uuid::new_v4(); let interface = db::model::IncompleteNetworkInterface::new( interface_id, authz_instance.id(), authz_vpc.id(), db_subnet, - mac, params.identity.clone(), params.ip, )?; diff --git a/nexus/src/app/sagas/instance_create.rs b/nexus/src/app/sagas/instance_create.rs index d3389881a17..fa04ad373c4 100644 --- a/nexus/src/app/sagas/instance_create.rs +++ b/nexus/src/app/sagas/instance_create.rs @@ -311,14 +311,11 @@ async fn sic_create_custom_network_interfaces( .fetch() .await .map_err(ActionError::action_failed)?; - let mac = - db::model::MacAddr::new().map_err(ActionError::action_failed)?; let interface = db::model::IncompleteNetworkInterface::new( interface_id, instance_id, authz_vpc.id(), db_subnet.clone(), - mac, params.identity.clone(), params.ip, ) @@ -412,14 +409,12 @@ async fn sic_create_default_network_interface( .await .map_err(ActionError::action_failed)?; - let mac = db::model::MacAddr::new().map_err(ActionError::action_failed)?; let interface_id = Uuid::new_v4(); let interface = db::model::IncompleteNetworkInterface::new( interface_id, instance_id, authz_vpc.id(), db_subnet.clone(), - mac, interface_params.identity.clone(), interface_params.ip, ) diff --git a/nexus/src/db/model/macaddr.rs b/nexus/src/db/model/macaddr.rs index f94a0356017..29dc983d0a9 100644 --- a/nexus/src/db/model/macaddr.rs +++ b/nexus/src/db/model/macaddr.rs @@ -8,55 +8,87 @@ use diesel::pg::Pg; use diesel::serialize::{self, ToSql}; use diesel::sql_types; use omicron_common::api::external; -use rand::{rngs::StdRng, SeedableRng}; -use std::convert::TryFrom; +use rand::thread_rng; +use rand::Rng; #[derive(Clone, Copy, Debug, PartialEq, AsExpression, FromSqlRow)] -#[diesel(sql_type = sql_types::Text)] +#[diesel(sql_type = sql_types::BigInt)] pub struct MacAddr(pub external::MacAddr); impl MacAddr { - /// Generate a unique MAC address for an interface - pub fn new() -> Result { - use rand::Fill; - // Use the Oxide OUI A8 40 25 - let mut addr = [0xA8, 0x40, 0x25, 0x00, 0x00, 0x00]; - addr[3..].try_fill(&mut StdRng::from_entropy()).map_err(|_| { - external::Error::internal_error("failed to generate MAC") - })?; - // From RFD 174, Oxide virtual MACs are constrained to have these bits - // set. - addr[3] |= 0xF0; - // TODO-correctness: We should use an explicit allocator for the MACs - // given the small address space. Right now creation requests may fail - // due to MAC collision, especially given the 20-bit space. - Ok(Self(external::MacAddr(macaddr::MacAddr6::from(addr)))) + // Guest MAC addresses begin with the Oxide OUI A8:40:25. Further, guest + // address are constrained to be in the virtual address range + // A8:40:24:F_:__:__. Even further, the range F0:00:00 - FE:FF:FF is + // reserved for customer-visible addresses (FF:00:00-FF:FF:FF is for + // system MAC addresses). See RFD 174 for the discussion of the virtual + // range, and + // https://github.com/oxidecomputer/omicron/pull/955#discussion_r856432498 + // for an initial discussion of the customer/system address range split. + pub(crate) const MIN_GUEST_ADDR: i64 = 0xA8_40_25_F0_00_00; + pub(crate) const MAX_GUEST_ADDR: i64 = 0xA8_40_25_FE_FF_FF; + + /// Generate a random MAC address for a guest network interface + pub fn random_guest() -> Self { + let value = + thread_rng().gen_range(Self::MIN_GUEST_ADDR..=Self::MAX_GUEST_ADDR); + Self::from_i64(value) + } + + /// Construct a MAC address from its i64 big-endian byte representation. + // NOTE: This is the representation used in the database. + pub(crate) fn from_i64(value: i64) -> Self { + let bytes = value.to_be_bytes(); + Self(external::MacAddr(macaddr::MacAddr6::new( + bytes[2], bytes[3], bytes[4], bytes[5], bytes[6], bytes[7], + ))) + } + + /// Convert a MAC address to its i64 big-endian byte representation + // NOTE: This is the representation used in the database. + pub(crate) fn to_i64(self) -> i64 { + let bytes = self.0.as_bytes(); + i64::from_be_bytes([ + 0, 0, bytes[0], bytes[1], bytes[2], bytes[3], bytes[4], bytes[5], + ]) } } NewtypeFrom! { () pub struct MacAddr(external::MacAddr); } NewtypeDeref! { () pub struct MacAddr(external::MacAddr); } -impl ToSql for MacAddr { +impl ToSql for MacAddr { fn to_sql<'a>( &'a self, out: &mut serialize::Output<'a, '_, Pg>, ) -> serialize::Result { - >::to_sql( - &self.0.to_string(), + >::to_sql( + &self.to_i64(), &mut out.reborrow(), ) } } -impl FromSql for MacAddr +impl FromSql for MacAddr where DB: Backend, - String: FromSql, + i64: FromSql, { fn from_sql(bytes: RawValue) -> deserialize::Result { - external::MacAddr::try_from(String::from_sql(bytes)?) - .map(MacAddr) - .map_err(|e| e.into()) + let value = i64::from_sql(bytes)?; + Ok(MacAddr::from_i64(value)) + } +} + +#[cfg(test)] +mod tests { + use super::MacAddr; + + #[test] + fn test_mac_to_int_conversions() { + let original: i64 = 0xa8_40_25_ff_00_01; + let mac = MacAddr::from_i64(original); + assert_eq!(mac.0.as_bytes(), &[0xa8, 0x40, 0x25, 0xff, 0x00, 0x01]); + let conv = mac.to_i64(); + assert_eq!(original, conv); } } diff --git a/nexus/src/db/model/network_interface.rs b/nexus/src/db/model/network_interface.rs index a1922295670..7afb6e3f25a 100644 --- a/nexus/src/db/model/network_interface.rs +++ b/nexus/src/db/model/network_interface.rs @@ -46,11 +46,9 @@ impl From for external::NetworkInterface { #[derive(Clone, Debug)] pub struct IncompleteNetworkInterface { pub identity: NetworkInterfaceIdentity, - pub instance_id: Uuid, pub vpc_id: Uuid, pub subnet: VpcSubnet, - pub mac: MacAddr, pub ip: Option, } @@ -60,7 +58,6 @@ impl IncompleteNetworkInterface { instance_id: Uuid, vpc_id: Uuid, subnet: VpcSubnet, - mac: MacAddr, identity: external::IdentityMetadataCreateParams, ip: Option, ) -> Result { @@ -68,7 +65,6 @@ impl IncompleteNetworkInterface { subnet.check_requestable_addr(ip)?; }; let identity = NetworkInterfaceIdentity::new(interface_id, identity); - - Ok(Self { identity, instance_id, subnet, vpc_id, mac, ip }) + Ok(Self { identity, instance_id, subnet, vpc_id, ip }) } } diff --git a/nexus/src/db/queries/network_interface.rs b/nexus/src/db/queries/network_interface.rs index 59c2f7d4605..95c752f4975 100644 --- a/nexus/src/db/queries/network_interface.rs +++ b/nexus/src/db/queries/network_interface.rs @@ -7,6 +7,7 @@ use crate::app::MAX_NICS_PER_INSTANCE; use crate::db; use crate::db::model::IncompleteNetworkInterface; +use crate::db::model::MacAddr; use crate::db::queries::next_item::DefaultShiftGenerator; use crate::db::queries::next_item::NextItem; use crate::defaults::NUM_INITIAL_RESERVED_IP_ADDRESSES; @@ -41,6 +42,8 @@ pub enum NetworkInterfaceError { DuplicatePrimaryKey(Uuid), /// There are no slots available on the instance NoSlotsAvailable, + /// There are no MAC addresses available + NoMacAddrressesAvailable, /// Any other error External(external::Error), } @@ -110,6 +113,11 @@ impl NetworkInterfaceError { MAX_NICS_PER_INSTANCE )) } + NetworkInterfaceError::NoMacAddrressesAvailable => { + external::Error::invalid_request( + "No available MAC addresses for interface", + ) + } NetworkInterfaceError::External(e) => e, } } @@ -174,6 +182,15 @@ fn decode_database_error( "((slot >= 0:::INT8) AND (slot < 8:::INT8))", ); + // Error message generated when we attempt to insert NULL in the `mac` + // column, which only happens when we run out of MAC addresses. This is + // probably quite unlikely, but not impossible given that MACs are unique + // within an entire VPC. We'll probably have other constraints we hit first, + // or explicit limits, but until those are in place, we opt for an explicit + // message. + const MAC_EXHAUSTION_ERROR_MESSAGE: &str = + r#"null value in column "mac" violates not-null constraint"#; + match err { // If the address allocation subquery fails, we'll attempt to insert // NULL for the `ip` column. This checks that the non-NULL constraint on @@ -204,6 +221,15 @@ fn decode_database_error( NetworkInterfaceError::NoSlotsAvailable } + // If the MAC allocation subquery fails, we'll attempt to insert NULL + // for the `mac` column. This checks that the non-NULL constraint on + // that colum has been violated. + PoolError::Connection(ConnectionError::Query( + Error::DatabaseError(DatabaseErrorKind::NotNullViolation, ref info), + )) if info.message() == MAC_EXHAUSTION_ERROR_MESSAGE => { + NetworkInterfaceError::NoMacAddrressesAvailable + } + // This path looks specifically at constraint names. PoolError::Connection(ConnectionError::Query( Error::DatabaseError(DatabaseErrorKind::UniqueViolation, ref info), @@ -408,6 +434,32 @@ impl QueryFragment for NextNicSlot { } } +/// A [`NextItem`] query that selects a random available MAC address for a guest +/// network interface. +#[derive(Debug, Clone, Copy)] +pub struct NextGuestMacAddress { + inner: NextItem< + db::schema::network_interface::table, + MacAddr, + db::schema::network_interface::dsl::mac, + Uuid, + db::schema::network_interface::dsl::vpc_id, + >, +} + +impl NextGuestMacAddress { + pub fn new(vpc_id: Uuid) -> Self { + let base = MacAddr::random_guest(); + let x = base.to_i64(); + let max_shift = MacAddr::MAX_GUEST_ADDR - x; + let min_shift = x - MacAddr::MIN_GUEST_ADDR; + let generator = DefaultShiftGenerator { base, max_shift, min_shift }; + Self { inner: NextItem::new_scoped(generator, vpc_id) } + } +} + +delegate_query_fragment_impl!(NextGuestMacAddress); + /// Add a subquery intended to verify that an Instance's networking does not /// span multiple VPCs. /// @@ -658,8 +710,10 @@ fn push_interface_allocation_subquery<'a>( out.push_identifier(dsl::subnet_id::NAME)?; out.push_sql(", "); - out.push_bind_param::(&query.mac_sql)?; - out.push_sql(" AS "); + // Push the subquery for selecting the a MAC address. + out.push_sql("("); + query.next_mac_subquery.walk_ast(out.reborrow())?; + out.push_sql(") AS "); out.push_identifier(dsl::mac::NAME)?; out.push_sql(", "); @@ -768,7 +822,7 @@ pub struct InsertNetworkInterfaceQuery { // long as the entire call to [`QueryFragment::walk_ast`]. vpc_id_str: String, ip_sql: Option, - mac_sql: String, + next_mac_subquery: NextGuestMacAddress, next_ipv4_address_subquery: NextGuestIpv4Address, next_slot_subquery: NextNicSlot, } @@ -777,20 +831,18 @@ impl InsertNetworkInterfaceQuery { pub fn new(interface: IncompleteNetworkInterface) -> Self { let vpc_id_str = interface.vpc_id.to_string(); let ip_sql = interface.ip.map(|ip| ip.into()); - let mac_sql = interface.mac.to_string(); - + let next_mac_subquery = NextGuestMacAddress::new(interface.vpc_id); let next_ipv4_address_subquery = NextGuestIpv4Address::new( interface.subnet.ipv4_block.0 .0, interface.subnet.identity.id, ); let next_slot_subquery = NextNicSlot::new(interface.instance_id); - Self { interface, now: Utc::now(), vpc_id_str, ip_sql, - mac_sql, + next_mac_subquery, next_ipv4_address_subquery, next_slot_subquery, } @@ -939,8 +991,8 @@ mod tests { use super::NetworkInterfaceError; use super::MAX_NICS_PER_INSTANCE; use crate::context::OpContext; - use crate::db::model; use crate::db::model::IncompleteNetworkInterface; + use crate::db::model::MacAddr; use crate::db::model::NetworkInterface; use crate::db::model::VpcSubnet; use nexus_test_utils::db::test_setup_database; @@ -1010,7 +1062,6 @@ mod tests { instance_id, vpc_id, subnet.clone(), - model::MacAddr::new().unwrap(), IdentityMetadataCreateParams { name: "interface-a".parse().unwrap(), description: String::from("description"), @@ -1038,7 +1089,6 @@ mod tests { instance_id, vpc_id, subnet.clone(), - model::MacAddr::new().unwrap(), IdentityMetadataCreateParams { name: "interface-b".parse().unwrap(), description: String::from("description"), @@ -1063,7 +1113,6 @@ mod tests { instance_id, vpc_id, subnet.clone(), - model::MacAddr::new().unwrap(), IdentityMetadataCreateParams { name: "interface-c".parse().unwrap(), description: String::from("description"), @@ -1089,7 +1138,6 @@ mod tests { instance_id, vpc_id, subnet.clone(), - model::MacAddr::new().unwrap(), IdentityMetadataCreateParams { name: "interface-b".parse().unwrap(), description: String::from("description"), @@ -1116,7 +1164,6 @@ mod tests { instance_id, other_vpc_id, other_subnet.clone(), - model::MacAddr::new().unwrap(), IdentityMetadataCreateParams { name: "interface-a".parse().unwrap(), description: String::from("description"), @@ -1146,7 +1193,6 @@ mod tests { Uuid::new_v4(), vpc_id, subnet.clone(), - model::MacAddr::new().unwrap(), IdentityMetadataCreateParams { name: format!("interface-{}", i).try_into().unwrap(), description: String::from("description"), @@ -1167,7 +1213,6 @@ mod tests { instance_id, vpc_id, subnet.clone(), - model::MacAddr::new().unwrap(), IdentityMetadataCreateParams { name: "interface-d".parse().unwrap(), description: String::from("description"), @@ -1195,7 +1240,6 @@ mod tests { Uuid::new_v4(), // New instance ID other_vpc_id, other_subnet.clone(), - model::MacAddr::new().unwrap(), IdentityMetadataCreateParams { name: "interface-e".parse().unwrap(), // Same name description: String::from("description"), @@ -1231,7 +1275,12 @@ mod tests { assert_eq!(inserted.instance_id, incomplete.instance_id); assert_eq!(inserted.vpc_id, incomplete.vpc_id); assert_eq!(inserted.subnet_id, incomplete.subnet.id()); - assert_eq!(inserted.mac, incomplete.mac); + assert!( + inserted.mac.to_i64() >= MacAddr::MIN_GUEST_ADDR + && inserted.mac.to_i64() <= MacAddr::MAX_GUEST_ADDR, + "The random MAC address {:?} is not a valid guest address", + inserted.mac, + ); } // Test that inserting a record into the database with the same primary key @@ -1278,7 +1327,6 @@ mod tests { instance_id, vpc_id, subnet, - model::MacAddr::new().unwrap(), IdentityMetadataCreateParams { name: "interface-a".parse().unwrap(), description: String::from("description"), @@ -1347,7 +1395,6 @@ mod tests { instance_id, vpc_id, subnet.clone(), - model::MacAddr::new().unwrap(), IdentityMetadataCreateParams { name: format!("interface-{}", slot).parse().unwrap(), description: String::from("description"), @@ -1376,7 +1423,6 @@ mod tests { instance_id, vpc_id, subnet, - model::MacAddr::new().unwrap(), IdentityMetadataCreateParams { name: "interface-8".parse().unwrap(), description: String::from("description"), diff --git a/nexus/src/db/schema.rs b/nexus/src/db/schema.rs index 66d25ea7a8f..931b6789154 100644 --- a/nexus/src/db/schema.rs +++ b/nexus/src/db/schema.rs @@ -127,7 +127,7 @@ table! { instance_id -> Uuid, vpc_id -> Uuid, subnet_id -> Uuid, - mac -> Text, + mac -> Int8, ip -> Inet, slot -> Int2, }